netdev - Re: [PATCH net-next v4] net: virtio_net: notifications coalescing support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 20 Jul 2022 05:09:08 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Alvaro Karsz <alvaro.karsz@...id-run.com>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v4] net: virtio_net: notifications coalescing
 support

On Wed, Jul 20, 2022 at 03:45:59PM +0800, Jason Wang wrote:
> On Wed, Jul 20, 2022 at 3:42 PM Jason Wang <jasowang@...hat.com> wrote:
> >
> > On Wed, Jul 20, 2022 at 3:15 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > >
> > > On Wed, Jul 20, 2022 at 10:07:11AM +0300, Alvaro Karsz wrote:
> > > > > Hmm. we currently (ab)use tx_max_coalesced_frames values 0 and 1 to mean tx
> > > > napi on/off.
> > > > > However I am not sure we should treat any value != 1 as napi on.
> > > > >
> > > > > I don't really have good ideas - I think abusing coalescing might
> > > > > have been a mistake. But now that we are there, I feel we need
> > > > > a way for userspace to at least be able to figure out whether
> > > > > setting coalescing to 0 will have nasty side effects.
> > > >
> > > >
> > > > So, how can I proceed from here?
> > > > Maybe we don't need to use tx napi when this feature is negotiated (like Jakub
> > > > suggested in prev. versions)?
> > > > It makes sense, since the number of TX notifications can be reduced by setting
> > > > tx_usecs/tx_max_packets with ethtool.
> > >
> > >
> > > Hmm Jason had some ideas about extensions in mind when he
> > > coded the current UAPI, let's see if he has ideas.
> > > I'll ruminate on compatibility a bit too.
> >
> > I might be wrong, but using 1 to enable tx napi is a way to try to be
> > compatible with the interrupt coalescing.
> >
> > That is, without notification coalescing, if 1 is set, we enable tx
> > notifications (and NAPI) for each packet. This works as if
> > tx-max-coalesced-frames is set to 1 when notification coalescing is
> > negotiated.
> >
> > Thanks
> >
> > >
> > > > > It's also a bit of a spec defect that it does not document corner cases
> > > > > like what do 0 values do, are they different from 1? or what are max values.
> > > > > Not too late to fix?
> > > >
> > > >
> > > > I think that some of the corner cases can be understood from the coalescing
> > > > values.
> > > > For example:
> > > > if rx_usecs=0 we should wait for 0 usecs, meaning that we should send a
> > > > notification immediately.
> > > > But if rx_usecs=1 we should wait for 1 usec.
> > > > The case with max_packets is a little bit unclear for the values 0/1, and it
> > > > seems that in both cases we should send a notification immediately after
> > > > receiving/sending a packet.
> 
> Then we probably need to document this in the spec.
> 
> And we need an upper limit for those values, this helps for e.g
> migration compatibility.
> 
> Thanks

Not a bad idea generally but I suspect this is better done
as part of the admin queue/migration work then.


> > > >
> > > >
> > > > > So the spec says
> > > > >         Device supports notifications coalescing.
> > > > >
> > > > > which makes more sense - there's not a lot guest needs to do here.
> > > >
> > > >
> > > > Noted.
> > > >
> > > > > parameters?
> > > >
> > > >
> > > > I'll change it to "settings".
> > > >
> > > > > why with dash here? And why not just put the comments near the fields
> > > > > themselves?
> > > >
> > > >
> > > > Noted.
> > >