Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
On Sun, 8 May 2016, Ben Hutchings wrote: > > 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. static refers to the number of packets to batch before raising an interrupt - which maps to existing tx_max_coaleced_frames. Have sent out v2 of the patch that no longer uses special values of rx-usecs to distinguish between the coalescing modes. Existing ethtool_coalesce fields are used as appropriate instead. In v2, driver can no longer issue "revert to defaults" command (think such a mechanism might be useful addition but belongs to ethtool framework). Thanks, Shri
Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
On Tue, 2016-05-10 at 11:24 +, David Laight wrote:
> From: Ben Hutchings
> >
> > Sent: 09 May 2016 01:17
> > 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.
> Would it be better to use stupidly large values for the non-zero special
> values?
> That would less problematic if someone expects 2 to mean '2 usecs'.
That seems less problematic, but I think we'd have to check that
current drivers (or at least the most widely used ones) reject those
values.
We should still need to standardise names and definitions for those
special values, so that the ethtool utility can provide keywords and
documentation for them.
Ben.
--
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.
signature.asc
Description: This is a digitally signed message part
RE: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
From: Ben Hutchings
> Sent: 09 May 2016 01:17
> 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.
Would it be better to use stupidly large values for the non-zero special values?
That would less problematic if someone expects 2 to mean '2 usecs'.
David
Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
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.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
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.
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. 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?
Thanks,
Shri
Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
From: Ben Hutchings
Date: Sat, 07 May 2016 13:04:46 +0100
> 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.
Agreed.
Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
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.
Ben.
--
Ben Hutchings
Editing code like this is akin to sticking plasters on the bleeding stump
of a severed limb. - me, 29 June 1999
signature.asc
Description: This is a digitally signed message part
[PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
Signed-off-by: Keyong Sun
Signed-off-by: Manoj Tammali
Signed-off-by: Shrikrishna Khare
---
drivers/net/vmxnet3/vmxnet3_defs.h| 32 +++-
drivers/net/vmxnet3/vmxnet3_drv.c | 47
drivers/net/vmxnet3/vmxnet3_ethtool.c | 135 ++
drivers/net/vmxnet3/vmxnet3_int.h | 5 ++
4 files changed, 218 insertions(+), 1 deletion(-)
diff --git a/drivers/net/vmxnet3/vmxnet3_defs.h
b/drivers/net/vmxnet3/vmxnet3_defs.h
index f3b31c2..a0fdaac 100644
--- a/drivers/net/vmxnet3/vmxnet3_defs.h
+++ b/drivers/net/vmxnet3/vmxnet3_defs.h
@@ -80,6 +80,7 @@ enum {
VMXNET3_CMD_LOAD_PLUGIN,
VMXNET3_CMD_RESERVED2,
VMXNET3_CMD_RESERVED3,
+ VMXNET3_CMD_SET_COALESCE,
VMXNET3_CMD_FIRST_GET = 0xF00D,
VMXNET3_CMD_GET_QUEUE_STATUS = VMXNET3_CMD_FIRST_GET,
@@ -637,6 +638,36 @@ struct Vmxnet3_SetPolling {
u8 enablePolling;
};
+#define VMXNET3_COAL_STATIC_MAX_DEPTH 128
+#define VMXNET3_COAL_RBC_MIN_RATE 100
+#define VMXNET3_COAL_RBC_MAX_RATE 10
+
+enum Vmxnet3_CoalesceMode {
+ VMXNET3_COALESCE_DEFAULT= 0,
+ VMXNET3_COALESCE_DISABLED = 1,
+ VMXNET3_COALESCE_ADAPT = 2,
+ VMXNET3_COALESCE_STATIC = 3,
+ VMXNET3_COALESCE_RBC= 4
+};
+
+struct Vmxnet3_CoalesceRbc {
+ u32 rbc_rate;
+};
+
+struct Vmxnet3_CoalesceStatic {
+ u32 tx_depth;
+ u32 tx_comp_depth;
+ u32 rx_depth;
+};
+
+struct Vmxnet3_CoalesceScheme {
+ enum Vmxnet3_CoalesceMode coalMode;
+ union {
+ struct Vmxnet3_CoalesceRbc coalRbc;
+ struct Vmxnet3_CoalesceStatic coalStatic;
+ } coalPara;
+};
+
/* If the command data <= 16 bytes, use the shared memory directly.
* otherwise, use variable length configuration descriptor.
*/
@@ -673,7 +704,6 @@ struct Vmxnet3_DriverShared {
} cu;
};
-
#define VMXNET3_ECR_RQERR (1 << 0)
#define VMXNET3_ECR_TQERR (1 << 1)
#define VMXNET3_ECR_LINK(1 << 2)
diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c
b/drivers/net/vmxnet3/vmxnet3_drv.c
index a6dc7c7..fe1c6ad 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2491,6 +2491,26 @@ vmxnet3_setup_driver_shared(struct vmxnet3_adapter
*adapter)
/* the rest are already zeroed */
}
+static void
+vmxnet3_init_coalesce(struct vmxnet3_adapter *adapter)
+{
+ struct Vmxnet3_DriverShared *shared = adapter->shared;
+ union Vmxnet3_CmdInfo *cmdInfo = &shared->cu.cmdInfo;
+ unsigned long flags;
+
+ if (!VMXNET3_VERSION_GE_3(adapter) ||
+ adapter->coal_conf->coalMode == VMXNET3_COALESCE_DEFAULT) {
+ return;
+ }
+
+ spin_lock_irqsave(&adapter->cmd_lock, flags);
+ cmdInfo->varConf.confVer = 1;
+ cmdInfo->varConf.confLen = cpu_to_le32(sizeof(*adapter->coal_conf));
+ cmdInfo->varConf.confPA = cpu_to_le64(adapter->coal_conf_pa);
+ VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD,
+ VMXNET3_CMD_SET_COALESCE);
+ spin_unlock_irqrestore(&adapter->cmd_lock, flags);
+}
int
vmxnet3_activate_dev(struct vmxnet3_adapter *adapter)
@@ -2540,6 +2560,8 @@ vmxnet3_activate_dev(struct vmxnet3_adapter *adapter)
goto activate_err;
}
+ vmxnet3_init_coalesce(adapter);
+
for (i = 0; i < adapter->num_rx_queues; i++) {
VMXNET3_WRITE_BAR0_REG(adapter,
VMXNET3_REG_RXPROD + i * VMXNET3_REG_ALIGN,
@@ -3345,6 +3367,21 @@ vmxnet3_probe_device(struct pci_dev *pdev,
goto err_ver;
}
+ if (VMXNET3_VERSION_GE_3(adapter)) {
+ adapter->coal_conf =
+ dma_alloc_coherent(&adapter->pdev->dev,
+ sizeof(struct Vmxnet3_CoalesceScheme)
+ ,
+ &adapter->coal_conf_pa,
+ GFP_KERNEL);
+ if (!adapter->coal_conf) {
+ err = -ENOMEM;
+ goto err_ver;
+ }
+ memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
+ adapter->coal_conf->coalMode = VMXNET3_COALESCE_DEFAULT;
+ }
+
SET_NETDEV_DEV(netdev, &pdev->dev);
vmxnet3_declare_features(adapter, dma64);
@@ -3407,6 +3444,11 @@ vmxnet3_probe_device(struct pci_dev *pdev,
return 0;
err_register:
+ if (VMXNET3_VERSION_GE_3(adapter)) {
+ dma_free_coherent(&adapter->pdev->dev,
+ sizeof(struct Vmxnet3_CoalesceScheme),
+ ad
