Update mailmap info for Steve Wise
Subsystems (2)
PATCH 1/2 Update mailmap info for Steve Wise
--- .mailmap | 2 ++ 1 file changed, 2 insertions(+)
.mailmap +2 -0
@@ -267,3 +267,5 @@ Gustavo Padovan <[email protected]> Gustavo Padovan <[email protected]> Changbin Du <[email protected]> <[email protected]> Changbin Du <[email protected]> <[email protected]> +Steve Wise <[email protected]> <[email protected]> +Steve Wise <[email protected]> <[email protected]>
Signature · 1 line
2.20.1
PATCH 2/2 rxe: correctly calculate iCRC for unaligned payloads
If RoCE PDUs being sent or received contain pad bytes, then the iCRC is miscalculated resulting PDUs being emitted by RXE with an incorrect iCRC, as well as ingress PDUs being dropped due to erroneously detecting a bad iCRC in the PDU. The fix is to include the pad bytes, if any, in iCRC computations.
--- drivers/infiniband/sw/rxe/rxe_recv.c | 2 +- drivers/infiniband/sw/rxe/rxe_req.c | 6 ++++++ drivers/infiniband/sw/rxe/rxe_resp.c | 7 +++++++ 3 files changed, 14 insertions(+), 1 deletion(-)
drivers/infiniband/sw/rxe/rxe_recv.c +1 -1
@@ -389,7 +389,7 @@ void rxe_rcv(struct sk_buff *skb) calc_icrc =rxe_icrc_hdr (pkt, skb); calc_icrc =rxe_crc32 (rxe, calc_icrc, (u8 *)payload_addr (pkt), -payload_size (pkt)); +payload_size (pkt) +bth_pad (pkt)); calc_icrc = (__force u32 )cpu_to_be32 (~calc_icrc);if (unlikely (calc_icrc != pack_icrc)) {if (skb->protocol ==htons (ETH_P_IPV6 ))
drivers/infiniband/sw/rxe/rxe_req.c +6 -0
@@ -500,6 +500,12 @@ static int fill_packet(struct rxe_qp *qp, struct rxe_send_wqe *wqe,if (err)return err; } +if (bth_pad (pkt)) { +u8 *pad =payload_addr (pkt) + paylen; + +memset (pad,0 ,bth_pad (pkt)); + crc =rxe_crc32 (rxe, crc, pad,bth_pad (pkt)); + } } p =payload_addr (pkt) + paylen +bth_pad (pkt);
drivers/infiniband/sw/rxe/rxe_resp.c +7 -0
@@ -732,6 +732,13 @@ static enum resp_states read_reply(struct rxe_qp *qp,if (err)pr_err ("Failed copying memory\n" ); +if (bth_pad (&ack_pkt)) { +struct rxe_dev *rxe =to_rdev (qp->ibqp.device); +u8 *pad =payload_addr (&ack_pkt) + payload; + +memset (pad,0 ,bth_pad (&ack_pkt)); + icrc =rxe_crc32 (rxe, icrc, pad,bth_pad (&ack_pkt)); + } p =payload_addr (&ack_pkt) + payload +bth_pad (&ack_pkt); *p = ~icrc;
Signature · 1 line
2.20.1
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
Quoted 5 lines · Steve Wise
If RoCE PDUs being sent or received contain pad bytes, then the iCRC is miscalculated resulting PDUs being emitted by RXE with an incorrect iCRC, as well as ingress PDUs being dropped due to erroneously detecting a bad iCRC in the PDU. The fix is to include the pad bytes, if any, in iCRC computations.
Should this description mention that this patch breaks compatibility with SoftRoCE drivers that do not include this fix? Do we need a kernel module parameter that allows to select either the old or the new behavior?
Quoted 1 line
Should this Cc-line perhaps be converted into three Cc-lines?
Otherwise this patch looks fine to me.
Thanks,
Bart.
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
Quoted 8 lines · Bart Van Assche
Quoted 5 lines · Steve Wise
If RoCE PDUs being sent or received contain pad bytes, then the iCRC is miscalculated resulting PDUs being emitted by RXE with an incorrect iCRC, as well as ingress PDUs being dropped due to erroneously detecting a bad iCRC in the PDU. The fix is to include the pad bytes, if any, in iCRC computations.
Quoted 4 lines
Should this description mention that this patch breaks compatibility with SoftRoCE drivers that do not include this fix? Do we need a kernel module parameter that allows to select either the old or the new behavior?
Good point. I defer to others on how they want to handle that.
Quoted 3 lines
Quoted 1 line
Should this Cc-line perhaps be converted into three Cc-lines?
Yea I screwed this up. I really didn't want this in the commit log
vs just CCing on the email submission, but I was having issues with git send-email. Pilot error. ;)
Stevo
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
In reply to Bart Van AsscheQuoted 9 lines · Bart Van Assche
Quoted 7 lines · Steve Wise
If RoCE PDUs being sent or received contain pad bytes, then the iCRC is miscalculated resulting PDUs being emitted by RXE with an incorrect
iCRC, as well as ingress PDUs being dropped due to erroneously detecting
a bad iCRC in the PDU. The fix is to include the pad bytes, if any, in iCRC computations.
Quoted 5 lines
Should this description mention that this patch breaks compatibility with SoftRoCE drivers that do not include this fix? Do we need a kernel
module parameter that allows to select either the old or the new behavior?
No. The original soft-RoCE driver was supposed to be compatible with hardware devices. Because of this bug, it obviously wasn't. This is a bug fix, and we do not need to do anything to be compatible with the broken behavior. Instead, it just needs noting that the soft-RoCE implementation in prior kernels has a known wire format bug that impacts communications with both fixed versions of the driver and real hardware devices.
Quoted 10 lines
Quoted 1 line
Should this Cc-line perhaps be converted into three Cc-lines?
Otherwise this patch looks fine to me.
Thanks,
Bart.
Signature · 3 lines
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
Quoted 19 lines · Doug Ledford
Quoted 11 lines · Bart Van Assche
Quoted 9 lines · Steve Wise
If RoCE PDUs being sent or received contain pad bytes, then the iCRC
is miscalculated resulting PDUs being emitted by RXE with an incorrect
iCRC, as well as ingress PDUs being dropped due to erroneously detecting
a bad iCRC in the PDU. The fix is to include the pad bytes, if any,
in iCRC computations.Quoted 6 lines
Should this description mention that this patch breaks compatibility
with SoftRoCE drivers that do not include this fix? Do we need a kernel
module parameter that allows to select either the old or the new behavior?
Quoted 10 lines
No. The original soft-RoCE driver was supposed to be compatible with hardware devices. Because of this bug, it obviously wasn't. This is a
bug fix, and we do not need to do anything to be compatible with the broken behavior. Instead, it just needs noting that the soft-RoCE implementation in prior kernels has a known wire format bug that impacts
communications with both fixed versions of the driver and real hardware
devices.
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.
Signature · 3 lines
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
Quoted 34 lines · Doug Ledford
Quoted 19 lines · Doug Ledford
Quoted 11 lines · Bart Van Assche
Quoted 9 lines · Steve Wise
If RoCE PDUs being sent or received contain pad bytes, then the iCRC
is miscalculated resulting PDUs being emitted by RXE with an incorrect
iCRC, as well as ingress PDUs being dropped due to erroneously detecting
a bad iCRC in the PDU. The fix is to include the pad bytes, if any,
in iCRC computations.Quoted 6 lines
Should this description mention that this patch breaks compatibility
with SoftRoCE drivers that do not include this fix? Do we need a kernel
module parameter that allows to select either the old or the new behavior?Quoted 10 lines
No. The original soft-RoCE driver was supposed to be compatible with hardware devices. Because of this bug, it obviously wasn't. This is a
bug fix, and we do not need to do anything to be compatible with the broken behavior. Instead, it just needs noting that the soft-RoCE implementation in prior kernels has a known wire format bug that impacts
communications with both fixed versions of the driver and real hardware
devices.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).
Quoted 15 lines
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_peerand 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.
Thanks
Quoted 5 lines
--
Doug Ledford <[email protected]>GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
Quoted 33 lines · Leon Romanovsky
Quoted 23 lines · Doug Ledford
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 structold_soft_roce_peerand 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 goesQuoted 8 lines
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.
Quoted 4 lines
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".
Signature · 3 lines
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
Quoted 34 lines · Doug Ledford
Quoted 33 lines · Leon Romanovsky
Quoted 31 lines · Doug Ledford
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 structold_soft_roce_peerand 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.
Quoted 7 lines
Quoted 4 lines
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".
I see, thanks
Quoted 5 lines
--
Doug Ledford <[email protected]>GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
In reply to Doug LedfordQuoted 34 lines · Doug Ledford
Quoted 33 lines · Leon Romanovsky
Quoted 31 lines · Doug Ledford
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 structold_soft_roce_peerand 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.
Quoted 7 lines
Quoted 4 lines
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.
Re: [PATCH 2/2] rxe: correctly calculate iCRC for unaligned payloads
Quoted 11 lines · Tom Talpey
Quoted 10 lines · Doug Ledford
Quoted 8 lines · Leon Romanovsky
Quoted 1 line · Doug Ledford
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.
Quoted 6 lines
Quoted 3 lines
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.
It just allows buggy implementations to talk to newer soft-RoCE (although not to hardware).
Quoted 2 lines
No hardware
implementation will do this, as it violates the protocol.
Right.
Quoted 3 lines
And, it
potentially opens a silent data corruption, by accepting packets which don't actually pass the checksum.
This, I highly doubt. For packets without padding, it would be the same. For packets with padding, it would only allow packets where the data bytes had a correct CRC, so it's not like it just allows anything to come through. And it would only allow it if the flag was set, it's not like we would allow two different CRCs on every packet with padding, it's either on or off, and the check still covers all data bytes. I find it highly unlikely that this would introduce any sort of data consistency problems for the specific case of old soft-RoCE talking to new soft-RoCE.
Quoted 1 line
Personally, I'd say it "nice to avoid", i.e. don't apply such a patch.
No one has submitted a patch, so we seem to be good regardless ;-)
Signature · 3 lines
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD