On Wed, Oct 2, 2019 at 4:27 AM Steffen Klassert
[off-list ref] wrote:

On Tue, Oct 01, 2019 at 08:43:05AM -0400, Willem de Bruijn wrote:

quoted

On Tue, Oct 1, 2019 at 2:18 AM Steffen Klassert
[off-list ref] wrote:

quoted

On Mon, Sep 30, 2019 at 11:26:55AM -0400, Willem de Bruijn wrote:

quoted

Instead, how about adding a UDP GRO ethtool feature independent of
forwarding, analogous to fraglist GRO? Then both are explicitly under
admin control. And can be enabled by default (either now, or after
getting more data).

We could add a protocol specific feature, but what would it mean
if UDP GRO is enabled?

Would it be enabled for forwarding, and for local input only if there
is a GRO capable socket? Or would it be enabled even if there
is no GRO capable socket? Same question when UDP GRO is disabled.

Enable UDP GRO for all traffic if GRO and UDP GRO are set, and only
then.

But this means that we would need to enable UDP GRO by default then.

That is what your patch 1/5 does. My concern was that that is a bold
change without an admin opt-out.

Currently, if an application uses a UDP GRO capable socket, it
can expect that it gets GROed packets without doing any additional
configuration. This would change if we disable it by default.
Unfortunately, enabling UDP GRO by default has the biggest
risk because most applications don't use UDP GRO capable sockets.

The most condervative way would be to leave standard GRO as it is.
But on some workloads standard GRO might be preferable, in
particular on forwarding to a NIC that can do UDP segmentation
in hardware.

quoted

That seems like the easiest to understand behavior to me, and
gives administrators an opt-out for workloads where UDP GRO causes a
regression. We cannot realistically turn off all GRO on a mixed
TCP/UDP workload (like, say, hosting TCP and QUIC).

quoted

Also, what means enabling GRO then? Enable GRO for all protocols
but UDP? Either UDP becomes something special then,

Yes and true. But it is something special. We don't know whether UDP
GRO is safe to deploy everywhere.

Only enabling it for the forwarding case is more conservative, but
gives no path to enabling it systemwide, is arguably confusing and
still lacks the admin control to turn off in case of unexpected
regressions. I do think that for a time this needs to be configurable
unless you're confident that the forwarding path is such a win that
no plan B is needed. But especially without fraglist, I'm not sure.

On my tests it was a win on forwarding, but there might be
usecases where it is not. I guess the only way to find this out
is to enable is and wait what happens.

I'm a bit hesitating on adding a feature flag that might be only
temporary usefull. In particular on the background of the talk
that Jesse Brandeburg gave on the LPC last year. Maybe you
remember the slide where he showed the output of
ethtool --show-offloads, it filled the whole page.

I was using ethtool -K just yesterday to debug a peculiar mix of
tunneling protocols. And yes, used grep on it ;) But I don't have much
of a problem with this.

But agreed that if default on works in all cases, then it's unnecessary.

quoted

quoted

or we need
to create protocol specific features for the other protocols
too. Same would apply for fraglist GRO.

We don't need it for other protocols after the fact, but it's a good
question: I don't know how it was enabled for them. Perhaps confidence
was gained based on testing. Or it was enabled for -rc1, no one
complained and stayed turned on. In which case you could do the same.

Maybe we should go that way to enable it and wait whether somebody
complains. A patch to add the feature flag could be prepared
beforehand for that case.

This early in the cycle, that may work. Yes, it's definitely good to
have the plan B at the ready.

It is easy to make a suboptimal design decision here, so
some more opinions would be helpfull.

Agreed.