Re: [PATCH v4 net-next 07/12] ipv6: add IFLA_GRO_IPV6_MAX_SIZE

[Alexander Duyck <alexander.duyck@gmail.com>]

On Fri, May 6, 2022 at 2:22 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, May 6, 2022 at 2:06 PM Alexander H Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > On Fri, 2022-05-06 at 08:30 -0700, Eric Dumazet wrote:
> > > From: Coco Li <lixiaoyan@google.com>
> > >
> > > Enable GRO to have IPv6 specific limit for max packet size.
> > >
> > > This patch introduces new dev->gro_ipv6_max_size
> > > that is modifiable through ip link.
> > >
> > > ip link set dev eth0 gro_ipv6_max_size 185000
> > >
> > > Note that this value is only considered if bigger than
> > > gro_max_size, and for non encapsulated TCP/ipv6 packets.
> > >
> > > Signed-off-by: Coco Li <lixiaoyan@google.com>
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> >
> > This is another spot where it doesn't make much sense to me to add yet
> > another control. Instead it would make much more sense to simply remove
> > the cap from the existing control and simply add a check that caps the
> > non-IPv6 protocols at GRO_MAX_SIZE.
>
> Can you please send a diff on top of our patch series ?

I would rather not as it would essentially just be a revert of the two
problematic patches since what I am suggesting is significantly
smaller.

> It is kind of hard to see what you want, and _why_ you want this.
>
> Note that GRO_MAX_SIZE has been replaced by dev->gro_max_size last year.

I am using GRO_MAX_SIZE as a legacy value for everything that is not
IPv6. If it would help you could go back and take a look at Jakub's
patch series and see what he did with TSO_LEGACY_MAX_SIZE. You could
think of my use here as GRO_LEGACY_MAX_SIZE. What I am doing is
capping all the non-ipv6/tcp flows at the default maximum limit for
legacy setups.

> Yes, yet another control, but some people want more control than others I guess.

Basically these patches are reducing functionality from an existing
control. The g[sr]o_max_size values were applied to all incoming or
outgoing traffic. The patches are adding a special control that only
applies to a subset of ipv6 traffic. Instead of taking that route I
would rather have the max_size values allowed to exceed the legacy
limits, and in those cases that cannot support the new sizes we
default back to the legacy maxes. Doing that I feel like we would get
much more consistent behavior and if somebody is wanting to use these
values for their original intended purpose which was limiting the
traffic they will be able to affect all traffic, not just the
non-ipv6/tcp traffic.