Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads

From: Tom Talpey <tom@talpey.com>
To: Doug Ledford <dledford@redhat.com>, Leon Romanovsky <leon@kernel.org>
Cc: Bart Van Assche <bvanassche@acm.org>,
	Steve Wise <larrystevenwise@gmail.com>,
	linux-rdma@vger.kernel.org, 3100102071@zju.edu.cn
Subject: Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
Date: Wed, 11 Dec 2019 09:42:41 -0500	[thread overview]
Message-ID: <5f98ecdc-44d1-e185-19e9-7710c2c7c991@talpey.com> (raw)
In-Reply-To: <c20696208c239bd11621ad3101735255738bcc97.camel@redhat.com>

On 12/10/2019 11:24 PM, Doug Ledford wrote:
> On Tue, 2019-12-10 at 08:54 +0200, Leon Romanovsky wrote:
>> On Mon, Dec 09, 2019 at 02:07:06PM -0500, Doug Ledford wrote:
>>>
>>> I've taken these two patches into for-rc (with fixups to the commit
>>> message on the second, as well as adding a Fixes: tag on the
>>> second).
>>>
>>> I stand by what I said about not needing a compatibility flag or
>>> module
>>> option for the user to set.  However, that isn't to say that we
>>> can't
>>> autodetect old soft-RoCE peers.  If we get a packet that fails CRC
>>> and
>>> has pad bytes, then re-run the CRC without the pad bytes and see if
>>> it
>>> matches.  If it does, we could A) mark the current QP as being to an
>>> old
>>> soft-RoCE device (causing us to send without including the pad bytes
>>> in
>>> the CRC) and B) allocate a struct old_soft_roce_peer and save the
>>> guid
>>> into that struct and then put that struct on a list that we then
>>> search
>>> any time we are creating a new queue pair and if the new queue pair
>>> goes
>>> to a guid in the list, then we immediately flag that qp as being to
>>> an
>>> old soft roce device and get the right behavior.  It would slow down
>>> qp
>>> creation somewhat due to the list search, but probably not enough to
>>> worry about.  No one will be doing a 1,000 node cluster MPI job over
>>> soft-RoCE, so we should never notice the list length causing search
>>> problems.  A patch to do something like that would be welcome.
>>
>> Do you find this implementation needed? I see RXE as a development
>> platform and in my view it is unlikely that someone will run RXE in
>> production with mixture of different kernel versions, which requires
>> such compatibility fallback.
> 
> It's not a requirement, that's why I took the patches as they were.  It
> would just be a "nice to have".

The counterargument to this is that it only extends the protocol bug
into the future, and for one single RoCE implementation. No hardware
implementation will do this, as it violates the protocol. And, it
potentially opens a silent data corruption, by accepting packets which
don't actually pass the checksum.

Personally, I'd say it "nice to avoid", i.e. don't apply such a patch.

MHO.