| Message ID | 20180419154929.25718-1-edumazet@google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net/mlx4_en: optimizes get_fixed_ipv6_csum() | expand |
On Thu, 2018-04-19 at 08:49 -0700, Eric Dumazet wrote: > While trying to support CHECKSUM_COMPLETE for IPV6 fragments, > I had to experiments various hacks in get_fixed_ipv6_csum(). > I must admit I could not find how to implement this :/ > > However, get_fixed_ipv6_csum() does a lot of redundant operations, > calling csum_partial() twice. > > First csum_partial() computes the checksum of saddr and daddr, > put in @csum_pseudo_hdr. Undone later in the second csum_partial() > computed on whole ipv6 header. > > Then nexthdr is added once, added a second time, then substracted. > > payload_len is added once, then substracted. > > Really all this can be reduced to two add_csum(), to add back 6 bytes > that were removed by mlx4 when providing hw_checksum in RX > descriptor. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Saeed Mahameed <saeedm@mellanox.com> > Cc: Tariq Toukan <tariqt@mellanox.com> > --- > Note: This patch, like other mlx4 patches can definitely wait > Tariq approval, thanks ! > LGTM, Reviewed-by: Saeed Mahameed <saeedm@mellanox.com> > drivers/net/ethernet/mellanox/mlx4/en_rx.c | 21 ++++++++---------- > --- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > index > 5c613c6663da51a4ae792eeb4d8956b54655786b..38c56fb6e5f5970f245dd56c38e > 1fc63a9349a07 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > @@ -593,30 +593,25 @@ static int get_fixed_ipv4_csum(__wsum > hw_checksum, struct sk_buff *skb, > } > > #if IS_ENABLED(CONFIG_IPV6) > -/* In IPv6 packets, besides subtracting the pseudo header checksum, > - * we also compute/add the IP header checksum which > - * is not added by the HW. > +/* In IPv6 packets, hw_checksum lacks 6 bytes from IPv6 header: > + * 4 first bytes : priority, version, flow_lbl > + * and 2 additional bytes : nexthdr, hop_limit. > */ > static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff > *skb, > struct ipv6hdr *ipv6h) > { > __u8 nexthdr = ipv6h->nexthdr; > - __wsum csum_pseudo_hdr = 0; > + __wsum temp; > > if (unlikely(nexthdr == IPPROTO_FRAGMENT || > nexthdr == IPPROTO_HOPOPTS || > nexthdr == IPPROTO_SCTP)) > return -1; > - hw_checksum = csum_add(hw_checksum, (__force > __wsum)htons(nexthdr)); > > - csum_pseudo_hdr = csum_partial(&ipv6h->saddr, > - sizeof(ipv6h->saddr) + > sizeof(ipv6h->daddr), 0); > - csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force > __wsum)ipv6h->payload_len); > - csum_pseudo_hdr = csum_add(csum_pseudo_hdr, > - (__force __wsum)htons(nexthdr)); > - > - skb->csum = csum_sub(hw_checksum, csum_pseudo_hdr); > - skb->csum = csum_add(skb->csum, csum_partial(ipv6h, > sizeof(struct ipv6hdr), 0)); > + /* priority, version, flow_lbl */ > + temp = csum_add(hw_checksum, *(__wsum *)ipv6h); > + /* nexthdr and hop_limit */ > + skb->csum = csum_add(temp, (__force __wsum)*(__be16 > *)&ipv6h->nexthdr); > return 0; > } > #endif
On 27/04/2018 1:56 AM, Saeed Mahameed wrote: > On Thu, 2018-04-19 at 08:49 -0700, Eric Dumazet wrote: >> While trying to support CHECKSUM_COMPLETE for IPV6 fragments, >> I had to experiments various hacks in get_fixed_ipv6_csum(). >> I must admit I could not find how to implement this :/ >> >> However, get_fixed_ipv6_csum() does a lot of redundant operations, >> calling csum_partial() twice. >> >> First csum_partial() computes the checksum of saddr and daddr, >> put in @csum_pseudo_hdr. Undone later in the second csum_partial() >> computed on whole ipv6 header. >> >> Then nexthdr is added once, added a second time, then substracted. >> >> payload_len is added once, then substracted. >> >> Really all this can be reduced to two add_csum(), to add back 6 bytes >> that were removed by mlx4 when providing hw_checksum in RX >> descriptor. >> >> Signed-off-by: Eric Dumazet <edumazet@google.com> >> Cc: Saeed Mahameed <saeedm@mellanox.com> >> Cc: Tariq Toukan <tariqt@mellanox.com> >> --- >> Note: This patch, like other mlx4 patches can definitely wait >> Tariq approval, thanks ! >> > > LGTM, > > Reviewed-by: Saeed Mahameed <saeedm@mellanox.com> > Acked-by: Tariq Toukan <tariqt@mellanox.com> Thanks Eric. >> drivers/net/ethernet/mellanox/mlx4/en_rx.c | 21 ++++++++---------- >> --- >> 1 file changed, 8 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c >> b/drivers/net/ethernet/mellanox/mlx4/en_rx.c >> index >> 5c613c6663da51a4ae792eeb4d8956b54655786b..38c56fb6e5f5970f245dd56c38e >> 1fc63a9349a07 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c >> @@ -593,30 +593,25 @@ static int get_fixed_ipv4_csum(__wsum >> hw_checksum, struct sk_buff *skb, >> } >> >> #if IS_ENABLED(CONFIG_IPV6) >> -/* In IPv6 packets, besides subtracting the pseudo header checksum, >> - * we also compute/add the IP header checksum which >> - * is not added by the HW. >> +/* In IPv6 packets, hw_checksum lacks 6 bytes from IPv6 header: >> + * 4 first bytes : priority, version, flow_lbl >> + * and 2 additional bytes : nexthdr, hop_limit. >> */ >> static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff >> *skb, >> struct ipv6hdr *ipv6h) >> { >> __u8 nexthdr = ipv6h->nexthdr; >> - __wsum csum_pseudo_hdr = 0; >> + __wsum temp; >> >> if (unlikely(nexthdr == IPPROTO_FRAGMENT || >> nexthdr == IPPROTO_HOPOPTS || >> nexthdr == IPPROTO_SCTP)) >> return -1; >> - hw_checksum = csum_add(hw_checksum, (__force >> __wsum)htons(nexthdr)); >> >> - csum_pseudo_hdr = csum_partial(&ipv6h->saddr, >> - sizeof(ipv6h->saddr) + >> sizeof(ipv6h->daddr), 0); >> - csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force >> __wsum)ipv6h->payload_len); >> - csum_pseudo_hdr = csum_add(csum_pseudo_hdr, >> - (__force __wsum)htons(nexthdr)); >> - >> - skb->csum = csum_sub(hw_checksum, csum_pseudo_hdr); >> - skb->csum = csum_add(skb->csum, csum_partial(ipv6h, >> sizeof(struct ipv6hdr), 0)); >> + /* priority, version, flow_lbl */ >> + temp = csum_add(hw_checksum, *(__wsum *)ipv6h); >> + /* nexthdr and hop_limit */ >> + skb->csum = csum_add(temp, (__force __wsum)*(__be16 >> *)&ipv6h->nexthdr); >> return 0; >> } >> #endif
On 05/02/2018 07:18 AM, Tariq Toukan wrote: > > > On 27/04/2018 1:56 AM, Saeed Mahameed wrote: >> LGTM, >> >> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com> >> > > Acked-by: Tariq Toukan <tariqt@mellanox.com> > > Thanks Eric. Thanks guys. I see this patch ( http://patchwork.ozlabs.org/patch/901336/ ) in a state I do not know : "Awaiting Upstream"
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 3 May 2018 17:05:06 -0700 > > > On 05/02/2018 07:18 AM, Tariq Toukan wrote: >> >> >> On 27/04/2018 1:56 AM, Saeed Mahameed wrote: > >>> LGTM, >>> >>> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com> >>> >> >> Acked-by: Tariq Toukan <tariqt@mellanox.com> >> >> Thanks Eric. > > Thanks guys. > > I see this patch ( http://patchwork.ozlabs.org/patch/901336/ ) in > a state I do not know : "Awaiting Upstream" THat means I expect to see this change from the upstream maintainer, which in this case is Tariq.
On 05/03/2018 06:52 PM, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Thu, 3 May 2018 17:05:06 -0700 > >> >> >> On 05/02/2018 07:18 AM, Tariq Toukan wrote: >>> >>> >>> On 27/04/2018 1:56 AM, Saeed Mahameed wrote: >> >>>> LGTM, >>>> >>>> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com> >>>> >>> >>> Acked-by: Tariq Toukan <tariqt@mellanox.com> >>> >>> Thanks Eric. >> >> Thanks guys. >> >> I see this patch ( http://patchwork.ozlabs.org/patch/901336/ ) in >> a state I do not know : "Awaiting Upstream" > > THat means I expect to see this change from the upstream > maintainer, which in this case is Tariq. > I see, but it seems Tariq does not know that, otherwise he would not have sent an "Acked-by:" I guess this will need an extra round-trip ...
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 3 May 2018 19:10:29 -0700 > > > On 05/03/2018 06:52 PM, David Miller wrote: >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Date: Thu, 3 May 2018 17:05:06 -0700 >> >>> >>> >>> On 05/02/2018 07:18 AM, Tariq Toukan wrote: >>>> >>>> >>>> On 27/04/2018 1:56 AM, Saeed Mahameed wrote: >>> >>>>> LGTM, >>>>> >>>>> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com> >>>>> >>>> >>>> Acked-by: Tariq Toukan <tariqt@mellanox.com> >>>> >>>> Thanks Eric. >>> >>> Thanks guys. >>> >>> I see this patch ( http://patchwork.ozlabs.org/patch/901336/ ) in >>> a state I do not know : "Awaiting Upstream" >> >> THat means I expect to see this change from the upstream >> maintainer, which in this case is Tariq. >> > > I see, but it seems Tariq does not know that, otherwise he would > not have sent an "Acked-by:" > > I guess this will need an extra round-trip ... No need to extra round-trip, I applied it directly ;)
On 05/04/2018 08:59 AM, David Miller wrote: > > No need to extra round-trip, I applied it directly ;) > Very nice, thanks David !
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c index 5c613c6663da51a4ae792eeb4d8956b54655786b..38c56fb6e5f5970f245dd56c38e1fc63a9349a07 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c @@ -593,30 +593,25 @@ static int get_fixed_ipv4_csum(__wsum hw_checksum, struct sk_buff *skb, } #if IS_ENABLED(CONFIG_IPV6) -/* In IPv6 packets, besides subtracting the pseudo header checksum, - * we also compute/add the IP header checksum which - * is not added by the HW. +/* In IPv6 packets, hw_checksum lacks 6 bytes from IPv6 header: + * 4 first bytes : priority, version, flow_lbl + * and 2 additional bytes : nexthdr, hop_limit. */ static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff *skb, struct ipv6hdr *ipv6h) { __u8 nexthdr = ipv6h->nexthdr; - __wsum csum_pseudo_hdr = 0; + __wsum temp; if (unlikely(nexthdr == IPPROTO_FRAGMENT || nexthdr == IPPROTO_HOPOPTS || nexthdr == IPPROTO_SCTP)) return -1; - hw_checksum = csum_add(hw_checksum, (__force __wsum)htons(nexthdr)); - csum_pseudo_hdr = csum_partial(&ipv6h->saddr, - sizeof(ipv6h->saddr) + sizeof(ipv6h->daddr), 0); - csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force __wsum)ipv6h->payload_len); - csum_pseudo_hdr = csum_add(csum_pseudo_hdr, - (__force __wsum)htons(nexthdr)); - - skb->csum = csum_sub(hw_checksum, csum_pseudo_hdr); - skb->csum = csum_add(skb->csum, csum_partial(ipv6h, sizeof(struct ipv6hdr), 0)); + /* priority, version, flow_lbl */ + temp = csum_add(hw_checksum, *(__wsum *)ipv6h); + /* nexthdr and hop_limit */ + skb->csum = csum_add(temp, (__force __wsum)*(__be16 *)&ipv6h->nexthdr); return 0; } #endif
While trying to support CHECKSUM_COMPLETE for IPV6 fragments, I had to experiments various hacks in get_fixed_ipv6_csum(). I must admit I could not find how to implement this :/ However, get_fixed_ipv6_csum() does a lot of redundant operations, calling csum_partial() twice. First csum_partial() computes the checksum of saddr and daddr, put in @csum_pseudo_hdr. Undone later in the second csum_partial() computed on whole ipv6 header. Then nexthdr is added once, added a second time, then substracted. payload_len is added once, then substracted. Really all this can be reduced to two add_csum(), to add back 6 bytes that were removed by mlx4 when providing hw_checksum in RX descriptor. Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Saeed Mahameed <saeedm@mellanox.com> Cc: Tariq Toukan <tariqt@mellanox.com> --- Note: This patch, like other mlx4 patches can definitely wait Tariq approval, thanks ! drivers/net/ethernet/mellanox/mlx4/en_rx.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)