Re: [net-next PATCH v2 5/8] udp: Partially unroll handling of first segment and last segment

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> This patch allows us to take care of unrolling the first segment and the
> last segment

Only the last segment needs to be unrolled, right?

> of the loop for processing the segmented skb. Part of the
> motivation for this is that it makes it easier to process the fact that the
> first fame and all of the frames in between should be mostly identical
> in terms of header data, and the last frame has differences in the length
> and partial checksum.
>
> In addition I am dropping the header length calculation since we don't
> really need it for anything but the last frame and it can be easily
> obtained by just pulling the data_len and offset of tail from the transport
> header.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>
> v2: New break-out patch based on one patch from earlier series
>
>  net/ipv4/udp_offload.c |   35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 5c4bb8b9e83e..946d06d2aa0c 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -193,7 +193,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>         struct sk_buff *seg, *segs = ERR_PTR(-EINVAL);
>         struct sock *sk = gso_skb->sk;
>         unsigned int sum_truesize = 0;
> -       unsigned int hdrlen;
>         struct udphdr *uh;
>         unsigned int mss;
>         __sum16 check;
> @@ -206,7 +205,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>         if (!pskb_may_pull(gso_skb, sizeof(*uh)))
>                 goto out;
>
> -       hdrlen = gso_skb->data - skb_mac_header(gso_skb);
>         skb_pull(gso_skb, sizeof(*uh));
>
>         /* clear destructor to avoid skb_segment assigning it to tail */
> @@ -219,7 +217,8 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>                 return segs;
>         }
>
> -       uh = udp_hdr(segs);
> +       seg = segs;
> +       uh = udp_hdr(seg);
>
>         /* compute checksum adjustment based on old length versus new */
>         newlen = htons(sizeof(*uh) + mss);
> @@ -227,25 +226,31 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>                                             ((__force u32)uh->len ^ 0xFFFF) +
>                                             (__force u32)newlen));
>
> -       for (seg = segs; seg; seg = seg->next) {
> -               uh = udp_hdr(seg);
> +       for (;;) {
> +               seg->destructor = sock_wfree;
> +               seg->sk = sk;
> +               sum_truesize += seg->truesize;
>
> -               /* last packet can be partial gso_size */
> -               if (!seg->next) {
> -                       newlen = htons(seg->len - hdrlen);
> -                       check = ~csum_fold((__force __wsum)((__force u32)uh->check +
> -                                                           ((__force u32)uh->len ^ 0xFFFF) +
> -                                                           (__force u32)newlen));
> -               }
> +               if (!seg->next)
> +                       break;

Not critical, but I find replacing

  for (seg = segs; seg; seg = seg->next) {
    uh = udp_hdr(seg);
    ...
  }

with

  uh = udp_hdr(seg);
  for (;;) {
    ...
    if (!seg->next)
      break;

    uh = udp_hdr(seg);
  }

much less easy to parse and it inflates patch size. How about just

  - for (seg = segs; seg; seg = seg->next)
  + for( seg = segs; seg->next; seg = seg->next)


>
>                 uh->len = newlen;
>                 uh->check = check;
>
> -               seg->destructor = sock_wfree;
> -               seg->sk = sk;
> -               sum_truesize += seg->truesize;
> +               seg = seg->next;
> +               uh = udp_hdr(seg);
>         }
>
> +       /* last packet can be partial gso_size, account for that in checksum */
> +       newlen = htons(skb_tail_pointer(seg) - skb_transport_header(seg) +
> +                      seg->data_len);
> +       check = ~csum_fold((__force __wsum)((__force u32)uh->check +
> +                                           ((__force u32)uh->len ^ 0xFFFF)  +
> +                                           (__force u32)newlen));
> +       uh->len = newlen;
> +       uh->check = check;
> +
> +       /* update refcount for the packet */
>         refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
>  out:
>         return segs;
>