diff mbox series

[2/6] RDMA/rxe: consolidate code for calculating ICRC of packets

Message ID 20250127223840.67280-3-ebiggers@kernel.org (mailing list archive)
State New
Headers show
Series RDMA: switch to using CRC32 library functions | expand

Commit Message

Eric Biggers Jan. 27, 2025, 10:38 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

Since rxe_icrc_hdr() is always immediately followed by updating the CRC
with the packet's payload, just rename it to rxe_icrc() and make it
include the payload in the CRC, so it now handles the entire packet.

This is a refactor with no change in behavior.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/infiniband/sw/rxe/rxe_icrc.c | 36 ++++++++++++----------------
 1 file changed, 15 insertions(+), 21 deletions(-)

Comments

Zhu Yanjun Jan. 29, 2025, 6:11 p.m. UTC | #1 
在 2025/1/27 23:38, Eric Biggers 写道:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since rxe_icrc_hdr() is always immediately followed by updating the CRC
> with the packet's payload, just rename it to rxe_icrc() and make it
> include the payload in the CRC, so it now handles the entire packet.
> 
> This is a refactor with no change in behavior.

In this commit, currently the entire packet are checked while the header 
is checked in the original source code.

Now it can work between RXE <----> RXE.
I am not sure whether RXE <---> MLX can work or not.

If it can work well, I am fine with this patch.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>   drivers/infiniband/sw/rxe/rxe_icrc.c | 36 ++++++++++++----------------
>   1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
> index ee417a0bbbdca..c7b0b4673b959 100644
> --- a/drivers/infiniband/sw/rxe/rxe_icrc.c
> +++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
> @@ -60,26 +60,24 @@ static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
>   
>   	return icrc;
>   }
>   
>   /**
> - * rxe_icrc_hdr() - Compute the partial ICRC for the network and transport
> - *		  headers of a packet.
> + * rxe_icrc() - Compute the ICRC of a packet
>    * @skb: packet buffer
>    * @pkt: packet information
>    *
> - * Return: the partial ICRC
> + * Return: the ICRC
>    */
> -static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> +static u32 rxe_icrc(struct sk_buff *skb, struct rxe_pkt_info *pkt)
>   {
>   	unsigned int bth_offset = 0;
>   	struct iphdr *ip4h = NULL;
>   	struct ipv6hdr *ip6h = NULL;
>   	struct udphdr *udph;
>   	struct rxe_bth *bth;
>   	u32 crc;
> -	int length;
>   	int hdr_size = sizeof(struct udphdr) +
>   		(skb->protocol == htons(ETH_P_IP) ?
>   		sizeof(struct iphdr) : sizeof(struct ipv6hdr));
>   	/* pseudo header buffer size is calculate using ipv6 header size since
>   	 * it is bigger than ipv4
> @@ -118,17 +116,23 @@ static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
>   	bth = (struct rxe_bth *)&pshdr[bth_offset];
>   
>   	/* exclude bth.resv8a */
>   	bth->qpn |= cpu_to_be32(~BTH_QPN_MASK);
>   
> -	length = hdr_size + RXE_BTH_BYTES;
> -	crc = rxe_crc32(pkt->rxe, crc, pshdr, length);
> +	/* Update the CRC with the first part of the headers. */
> +	crc = rxe_crc32(pkt->rxe, crc, pshdr, hdr_size + RXE_BTH_BYTES);
>   
> -	/* And finish to compute the CRC on the remainder of the headers. */
> +	/* Update the CRC with the remainder of the headers. */
>   	crc = rxe_crc32(pkt->rxe, crc, pkt->hdr + RXE_BTH_BYTES,
>   			rxe_opcode[pkt->opcode].length - RXE_BTH_BYTES);
> -	return crc;
> +
> +	/* Update the CRC with the payload. */
> +	crc = rxe_crc32(pkt->rxe, crc, payload_addr(pkt),
> +			payload_size(pkt) + bth_pad(pkt));
> +
> +	/* Invert the CRC and return it. */
> +	return ~crc;
>   }
>   
>   /**
>    * rxe_icrc_check() - Compute ICRC for a packet and compare to the ICRC
>    *		      delivered in the packet.
> @@ -146,18 +150,12 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
>   	 * descending order from the x^31 coefficient to the x^0 one.  When the
>   	 * result is interpreted as a 32-bit integer using the required reverse
>   	 * mapping between bits and polynomial coefficients, it's a __le32.
>   	 */
>   	__le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> -	u32 icrc;
>   
> -	icrc = rxe_icrc_hdr(skb, pkt);
> -	icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> -				payload_size(pkt) + bth_pad(pkt));
> -	icrc = ~icrc;
> -
> -	if (unlikely(icrc != le32_to_cpu(*icrcp)))
> +	if (unlikely(rxe_icrc(skb, pkt) != le32_to_cpu(*icrcp)))
>   		return -EINVAL;
>   
>   	return 0;
>   }
>   
> @@ -167,12 +165,8 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
>    * @pkt: packet information
>    */
>   void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
>   {
>   	__le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> -	u32 icrc;
>   
> -	icrc = rxe_icrc_hdr(skb, pkt);
> -	icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> -				payload_size(pkt) + bth_pad(pkt));
> -	*icrcp = cpu_to_le32(~icrc);
> +	*icrcp = cpu_to_le32(rxe_icrc(skb, pkt));
>   }
Eric Biggers Jan. 30, 2025, 2:15 a.m. UTC | #2 
On Wed, Jan 29, 2025 at 07:11:35PM +0100, Zhu Yanjun wrote:
> 在 2025/1/27 23:38, Eric Biggers 写道:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Since rxe_icrc_hdr() is always immediately followed by updating the CRC
> > with the packet's payload, just rename it to rxe_icrc() and make it
> > include the payload in the CRC, so it now handles the entire packet.
> > 
> > This is a refactor with no change in behavior.
> 
> In this commit, currently the entire packet are checked while the header is
> checked in the original source code.
> 
> Now it can work between RXE <----> RXE.
> I am not sure whether RXE <---> MLX can work or not.
> 
> If it can work well, I am fine with this patch.
> 
> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> 

Both the header and payload are checksummed both before and after this patch.
Can you elaborate on why you think this patch changed any behavior?

- Eric
Zhu Yanjun Jan. 30, 2025, 7:24 a.m. UTC | #3 
在 2025/1/30 3:15, Eric Biggers 写道:
> On Wed, Jan 29, 2025 at 07:11:35PM +0100, Zhu Yanjun wrote:
>> 在 2025/1/27 23:38, Eric Biggers 写道:
>>> From: Eric Biggers <ebiggers@google.com>
>>>
>>> Since rxe_icrc_hdr() is always immediately followed by updating the CRC
>>> with the packet's payload, just rename it to rxe_icrc() and make it
>>> include the payload in the CRC, so it now handles the entire packet.
>>>
>>> This is a refactor with no change in behavior.
>>
>> In this commit, currently the entire packet are checked while the header is
>> checked in the original source code.
>>
>> Now it can work between RXE <----> RXE.
>> I am not sure whether RXE <---> MLX can work or not.
>>
>> If it can work well, I am fine with this patch.
>>
>> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
> 
> Both the header and payload are checksummed both before and after this patch.
> Can you elaborate on why you think this patch changed any behavior?

 From the source code, it seems that only the header is checked. And RXE 
can connect to MLX RDMA NIC. That is, the CRC of the header can be 
verified both in RXE and MLX RDMA NIC.

Now in your commit, the header and payload are checked. Thus, the CRC 
value in RDMA header may be different from the CRC of the header(that 
CRC can be verified in RXE and MLX RDMA NIC). Finally the CRC of the 
header and payload will not be verified in MLX RDMA NIC?

IMO, after your patchset is applied, if RXE can connect to MLX RDMA NIC, 
I am fine with it.

In the function rxe_rcv as below,
"
...
     err = rxe_icrc_check(skb, pkt);
     if (unlikely(err))
         goto drop;
...
"
rxe_icrc_check is called to check the RDMA packet. In your commit, the 
icrc is changed. I am not sure whether this icrc can also be verified 
correctly in MLX RDMA NIC or not.

Because RXE can connect to MLX RDMA NIC, after your patchset is applied, 
hope that RXE can also connect to MLX RDMA NIC successfully.

Thanks,
Zhu Yanjun


> 
> - Eric
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
index ee417a0bbbdca..c7b0b4673b959 100644
--- a/drivers/infiniband/sw/rxe/rxe_icrc.c
+++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
@@ -60,26 +60,24 @@  static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
 
 	return icrc;
 }
 
 /**
- * rxe_icrc_hdr() - Compute the partial ICRC for the network and transport
- *		  headers of a packet.
+ * rxe_icrc() - Compute the ICRC of a packet
  * @skb: packet buffer
  * @pkt: packet information
  *
- * Return: the partial ICRC
+ * Return: the ICRC
  */
-static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
+static u32 rxe_icrc(struct sk_buff *skb, struct rxe_pkt_info *pkt)
 {
 	unsigned int bth_offset = 0;
 	struct iphdr *ip4h = NULL;
 	struct ipv6hdr *ip6h = NULL;
 	struct udphdr *udph;
 	struct rxe_bth *bth;
 	u32 crc;
-	int length;
 	int hdr_size = sizeof(struct udphdr) +
 		(skb->protocol == htons(ETH_P_IP) ?
 		sizeof(struct iphdr) : sizeof(struct ipv6hdr));
 	/* pseudo header buffer size is calculate using ipv6 header size since
 	 * it is bigger than ipv4
@@ -118,17 +116,23 @@  static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
 	bth = (struct rxe_bth *)&pshdr[bth_offset];
 
 	/* exclude bth.resv8a */
 	bth->qpn |= cpu_to_be32(~BTH_QPN_MASK);
 
-	length = hdr_size + RXE_BTH_BYTES;
-	crc = rxe_crc32(pkt->rxe, crc, pshdr, length);
+	/* Update the CRC with the first part of the headers. */
+	crc = rxe_crc32(pkt->rxe, crc, pshdr, hdr_size + RXE_BTH_BYTES);
 
-	/* And finish to compute the CRC on the remainder of the headers. */
+	/* Update the CRC with the remainder of the headers. */
 	crc = rxe_crc32(pkt->rxe, crc, pkt->hdr + RXE_BTH_BYTES,
 			rxe_opcode[pkt->opcode].length - RXE_BTH_BYTES);
-	return crc;
+
+	/* Update the CRC with the payload. */
+	crc = rxe_crc32(pkt->rxe, crc, payload_addr(pkt),
+			payload_size(pkt) + bth_pad(pkt));
+
+	/* Invert the CRC and return it. */
+	return ~crc;
 }
 
 /**
  * rxe_icrc_check() - Compute ICRC for a packet and compare to the ICRC
  *		      delivered in the packet.
@@ -146,18 +150,12 @@  int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
 	 * descending order from the x^31 coefficient to the x^0 one.  When the
 	 * result is interpreted as a 32-bit integer using the required reverse
 	 * mapping between bits and polynomial coefficients, it's a __le32.
 	 */
 	__le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
-	u32 icrc;
 
-	icrc = rxe_icrc_hdr(skb, pkt);
-	icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
-				payload_size(pkt) + bth_pad(pkt));
-	icrc = ~icrc;
-
-	if (unlikely(icrc != le32_to_cpu(*icrcp)))
+	if (unlikely(rxe_icrc(skb, pkt) != le32_to_cpu(*icrcp)))
 		return -EINVAL;
 
 	return 0;
 }
 
@@ -167,12 +165,8 @@  int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
  * @pkt: packet information
  */
 void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
 {
 	__le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
-	u32 icrc;
 
-	icrc = rxe_icrc_hdr(skb, pkt);
-	icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
-				payload_size(pkt) + bth_pad(pkt));
-	*icrcp = cpu_to_le32(~icrc);
+	*icrcp = cpu_to_le32(rxe_icrc(skb, pkt));
 }