On Wed, Oct 2, 2019 at 4:27 AM Steffen Klassert <steffen.klass...@secunet.com> wrote: > > On Tue, Oct 01, 2019 at 08:43:05AM -0400, Willem de Bruijn wrote: > > On Tue, Oct 1, 2019 at 2:18 AM Steffen Klassert > > <steffen.klass...@secunet.com> wrote: > > > > > > On Mon, Sep 30, 2019 at 11:26:55AM -0400, Willem de Bruijn wrote: > > > > > > > > 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. > > > 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). > > > > > 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. > > > > > 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.
