On Mon, Mar 22, 2021 at 1:12 PM Paolo Abeni <pab...@redhat.com> wrote:
>
> On Mon, 2021-03-22 at 09:42 -0400, Willem de Bruijn wrote:
> > On Sun, Mar 21, 2021 at 1:01 PM Paolo Abeni <pab...@redhat.com> wrote:
> > > Currently the UDP protocol delivers GSO_FRAGLIST packets to
> > > the sockets without the expected segmentation.
> > >
> > > This change addresses the issue introducing and maintaining
> > > a per socket bitmask of GSO types requiring segmentation.
> > > Enabling GSO removes SKB_GSO_UDP_L4 from such mask, while
> > > GSO_FRAGLIST packets are never accepted
> > >
> > > Note: this also updates the 'unused' field size to really
> > > fit the otherwise existing hole. It's size become incorrect
> > > after commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").
> > >
> > > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> > > Signed-off-by: Paolo Abeni <pab...@redhat.com>
> > > ---
> > > include/linux/udp.h | 10 ++++++----
> > > net/ipv4/udp.c | 12 +++++++++++-
> > > 2 files changed, 17 insertions(+), 5 deletions(-)
> > >
> > > /*
> > > * Following member retains the information to create a UDP header
> > > * when the socket is uncorked.
> > > @@ -68,7 +68,10 @@ struct udp_sock {
> > > #define UDPLITE_SEND_CC 0x2 /* set via udplite setsockopt
> > > */
> > > #define UDPLITE_RECV_CC 0x4 /* set via udplite setsocktopt
> > > */
> > > __u8 pcflag; /* marks socket as UDP-Lite if >
> > > 0 */
> > > - __u8 unused[3];
> > > + __u8 unused[1];
> > > + unsigned int unexpected_gso;/* GSO types this socket can't
> > > accept,
> > > + * any of SKB_GSO_UDP_L4 or
> > > SKB_GSO_FRAGLIST
> > > + */
> >
> > An extra unsigned int for this seems overkill.
>
> Should be more clear after the next patch.
>
> Using an explicit 'acceptable GSO types' field makes the patch 5/8
> quite simple.
>
> After this patch the 'udp_sock' struct size remains unchanged and even
> the number of 'udp_sock' cachelines touched for every packet is
> unchanged.
But there is opportunity cost, of course. Next time we need to add
something to the struct, we will add a new cacheline.
A 32-bit field for just 2 bits, where 1 already exists does seem like overkill.
More importantly, I just think it's less obvious code than a pair of fields
accepts_udp_l4:1,
accepts_udp_fraglist:1,
Local sockets can only accept the first, as there does not exist an
interface to pass along the multiple frag sizes that a frag_list based
approach might have.
Sockets with encap_rcv != NULL may opt-in to being able to handle either.
I think explicit code will be more maintainable. At the cost of
perhaps two branches instead of one, admittedly. But that seems
premature optimization.
