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? 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. >> + 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.
