On Thu, Sep 13, 2018 at 11:27 PM Jason Wang <jasow...@redhat.com> wrote: > > > > On 2018年09月13日 22:58, Willem de Bruijn wrote: > > On Thu, Sep 13, 2018 at 5:02 AM Jason Wang <jasow...@redhat.com> wrote: > >> > >> > >> On 2018年09月13日 07:27, Willem de Bruijn wrote: > >>> On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn > >>> <willemdebruijn.ker...@gmail.com> wrote: > >>>> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.faine...@gmail.com> > >>>> wrote: > >>>>> > >>>>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote: > >>>>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli > >>>>>> <f.faine...@gmail.com> wrote: > >>>>>>> > >>>>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote: > >>>>>>>> From: Willem de Bruijn <will...@google.com> > >>>>>>>> > >>>>>>>> Implement ethtool .set_coalesce (-C) and .get_coalesce (-c) handlers. > >>>>>>>> Interrupt moderation is currently not supported, so these accept and > >>>>>>>> display the default settings of 0 usec and 1 frame. > >>>>>>>> > >>>>>>>> Toggle tx napi through a bit in tx-frames. So as to not interfere > >>>>>>>> with possible future interrupt moderation, use bit 10, well outside > >>>>>>>> the reasonable range of real interrupt moderation values. > >>>>>>>> > >>>>>>>> Changes are not atomic. The tx IRQ, napi BH and transmit path must > >>>>>>>> be quiesced when switching modes. Only allow changing this setting > >>>>>>>> when the device is down. > >>>>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be > >>>>>>> more > >>>>>>> appropriate rather than use the coalescing configuration API here? > >>>>>> What do you mean by private ethtool flag? A new field in ethtool > >>>>>> --features (-k)? > >>>>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then > >>>>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that > >>>>> private flag. mlx5 has a number of privates flags for instance. > >>>> Interesting, thanks! I was not at all aware of those ethtool flags. > >>>> Am having a look. It definitely looks promising. > >>> Okay, I made that change. That is indeed much cleaner, thanks. > >>> Let me send the patch, initially as RFC. > >>> > >>> I've observed one issue where if we toggle the flag before bringing > >>> up the device, it hits a kernel BUG at include/linux/netdevice.h:515 > >>> > >>> BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); > >> This reminds me that we need to check netif_running() before trying to > >> enable and disable tx napi in ethtool_set_coalesce(). > > The first iteration of my patch checked IFF_UP and effectively > > only allowed the change when not running. What do you mean > > by need to check? > > I mean if device is not up, there's no need to toggle napi state and tx > lock. > > > > > And to respond to the other follow-up notes at once: > > > >> Consider we may have interrupt moderation in the future, I tend to use > >> set_coalesce. Otherwise we may need two steps to enable moderation: > >> > >> - tx-napi on > >> - set_coalesce > > FWIW, I don't care strongly whether we do this through coalesce or > > priv_flags. > > Ok.
Since you prefer coalesce, let's go with that (and a revision of your latest patch). > > >>> + if (!napi_weight) > >>> + virtqueue_enable_cb(vi->sq[i].vq); > >> I don't get why we need to disable enable cb here. > > To avoid entering no-napi mode with too few descriptors to > > make progress and no way to get out of that state. This is a > > pretty crude attempt at handling that, admittedly. > > But in this case, we will call enable_cb_delayed() and we will finally > get a interrupt? Right. It's a bit of a roundabout way to ensure that netif_tx_wake_queue and thus eventually free_old_xmit_skbs are called. It might make more sense to just wake the device without going through an interrupt.
