Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1396520

Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

On Sun, 2016-05-08 at 13:55 -0700, Shrikrishna Khare wrote:
> 
> On Sat, 7 May 2016, Ben Hutchings wrote:
> 
> > On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> > [...]
> > > +static int
> > > +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> > > +{
> > [...]
> > > +   switch (ec->rx_coalesce_usecs) {
> > > +   case VMXNET3_COALESCE_DEFAULT:
> > > +   case VMXNET3_COALESCE_DISABLED:
> > > +   case VMXNET3_COALESCE_ADAPT:
> > > +           if (ec->tx_max_coalesced_frames ||
> > > +               ec->tx_max_coalesced_frames_irq ||
> > > +               ec->rx_max_coalesced_frames_irq) {
> > > +                   return -EINVAL;
> > > +           }
> > > +           memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> > > +           adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> > > +           break;
> > > +   case VMXNET3_COALESCE_STATIC:
> > [...]
> > 
> > I don't want to see drivers introducing magic values for fields that
> > are denominated in microseconds (especially not for 0, which is the
> > correct way to specify 'no coalescing').  If the current
> > ethtool_coalesce structure is inadequate, propose an extension.
> 
> For vmxnet3, we need an ethtool mechanism to indicate coalescing mode to 
> the device.
> 
> Would a patch that maps 0 to 'no coalescing' be acceptable? That is:
> 
> rx-usecs = 0 -> coalescing disabled.
> rx-usecs = 1 -> default (chosen by the device).
> rx-usecs = 2 -> adaptive coalescing.
> rx-usecs = 3 -> static coalescing.

I still don't like it much.  For the 3 special values (0 isn't really
special):

1 = default: When the driver sets the virtual device to this mode, can it then read back what the actual settings are, or are they hidden?  If it can, then userland can also read the defaults and explicitly return to them later.  But I do see the usefulness of an explicit request to reset to defaults.

2 = adaptive coalescing: There are already fields to request adaptive coalescing; you should support them.

3 = static coalescing: I don't understand what this means.

> all other rx-usecs values -> rate based coalescing where rx-usecs denotes 
> rate.
> 
> Alternatively: I don't think new members could be added to struct 
> ethtool_coalesce without breaking compatibility.

That's right, unfortunately.

> Thus, I could extend coalescing as follows:
> - new struct ethtool_coalesce_v2 with coalesce_mode (along with all the 
> members of struct ethtool_coalesce).
> - introduce new ETHTOOL_{G,S}COALESCE_V2 commands.
> - extend userspace ethtool to invoke new commands.
> 
> Could you please advice?

That's roughly how you would extend it.  Though we would probably want
to consider making other extensions to interrupt coalescing control at
the same time.

Ben.

>  
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations Shrikrishna Khare <skhare@vmware.com> - 2016-05-07 01:20 +0200
  Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations Ben Hutchings <ben@decadent.org.uk> - 2016-05-07 14:10 +0200
    Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations David Miller <davem@davemloft.net> - 2016-05-07 19:50 +0200
    Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce,  set_coalesce ethtool operations Shrikrishna Khare <skhare@vmware.com> - 2016-05-08 23:00 +0200
      Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations Ben Hutchings <ben@decadent.org.uk> - 2016-05-09 02:20 +0200
        RE: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations David Laight <David.Laight@ACULAB.COM> - 2016-05-10 13:30 +0200
          Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations Ben Hutchings <ben@decadent.org.uk> - 2016-05-10 14:10 +0200

csiph-web