| Message ID | 20230706150017.5065-1-muhammad.husaini.zulkifli@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net,v1] igc: Expose and modify tx-usecs coalesce setting | expand |
On 7/6/2023 8:00 AM, Muhammad Husaini Zulkifli wrote: > When users attempt to obtain the coalesce setting using the > ethtool command, current code always returns 0 for tx-usecs. > This is because I225/6 always uses a queue pair setting, hence Should there be a follow-on patch to remove 'IGC_FLAG_QUEUE_PAIRS' altogether then? > tx_coalesce_usecs does not return a value during the > igc_ethtool_get_coalesce() callback process. The pair queue > condition checking in igc_ethtool_get_coalesce() is removed by > this patch so that the user gets information of the value of tx-usecs. > > Even if i225/6 is using queue pair setting, there is no harm in > notifying the user of the tx-usecs. The implementation of the current > code may have previously been a copy of the legacy code i210. > > This patch also enables users to modify the tx-usecs parameter. > The rx-usecs value will adhere to the same value as the set tx-usecs > because the queue pair setting is enabled. This seems like it should be a seperate patch. > How to test: > User can set or get the coalesce value using ethtool command. > > Example command: > Get: ethtool -c <interface> > Set: ethtool -C <interface> > > Previous output (using get command): > > rx-usecs: 3 > rx-frames: n/a > rx-usecs-irq: n/a > rx-frames-irq: n/a > > tx-usecs: 0 > tx-frames: n/a > tx-usecs-irq: n/a > tx-frames-irq: n/a > > New output (using get command): > > rx-usecs: 3 > rx-frames: n/a > rx-usecs-irq: n/a > rx-frames-irq: n/a > > tx-usecs: 3 > tx-frames: n/a > tx-usecs-irq: n/a > tx-frames-irq: n/a > > Previous output (using set command): > > root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 > netlink error: Invalid argument > > New output (using set command): > > root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 > rx-usecs: 10 > rx-frames: n/a > rx-usecs-irq: n/a > rx-frames-irq: n/a > > tx-usecs: 10 > tx-frames: n/a > tx-usecs-irq: n/a > tx-frames-irq: n/a > > Fixes: 7df76bd19181 ("igc: Add 'igc_ethtool_' prefix to functions in igc_ethtool.c") Please use the commit that introduced the functional problem, not one that had no functional change. > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_ethtool.c | 46 +++++++++++++++----- > 1 file changed, 34 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c > index 93bce729be76..1cf7131a82c5 100644 > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > @@ -880,12 +880,10 @@ static int igc_ethtool_get_coalesce(struct net_device *netdev, > else > ec->rx_coalesce_usecs = adapter->rx_itr_setting >> 2; > > - if (!(adapter->flags & IGC_FLAG_QUEUE_PAIRS)) { > - if (adapter->tx_itr_setting <= 3) > - ec->tx_coalesce_usecs = adapter->tx_itr_setting; > - else > - ec->tx_coalesce_usecs = adapter->tx_itr_setting >> 2; > - } > + if (adapter->tx_itr_setting <= 3) > + ec->tx_coalesce_usecs = adapter->tx_itr_setting; > + else > + ec->tx_coalesce_usecs = adapter->tx_itr_setting >> 2; > > return 0; > } > @@ -910,15 +908,40 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev, > ec->tx_coalesce_usecs == 2) > return -EINVAL; > > - if ((adapter->flags & IGC_FLAG_QUEUE_PAIRS) && ec->tx_coalesce_usecs) > - return -EINVAL; > - > /* If ITR is disabled, disable DMAC */ > if (ec->rx_coalesce_usecs == 0) { > if (adapter->flags & IGC_FLAG_DMAC) > adapter->flags &= ~IGC_FLAG_DMAC; > } > > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { This is removed from igc_ethtool_get_coalesce() becuase 'I225/6 always uses a queue pair setting', but you add the condition check here? > + u32 old_tx_itr, old_rx_itr; > + > + /* This is to get back the original value before byte shifting */ > + old_tx_itr = (adapter->tx_itr_setting <= 3) ? > + adapter->tx_itr_setting : adapter->tx_itr_setting >> 2; > + > + old_rx_itr = (adapter->rx_itr_setting <= 3) ? > + adapter->rx_itr_setting : adapter->rx_itr_setting >> 2; > + > + if (old_tx_itr != ec->tx_coalesce_usecs) { > + if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3) > + adapter->tx_itr_setting = ec->tx_coalesce_usecs; > + else > + adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2; > + > + adapter->rx_itr_setting = adapter->tx_itr_setting; > + } else if (old_rx_itr != ec->rx_coalesce_usecs) { > + if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3) > + adapter->rx_itr_setting = ec->rx_coalesce_usecs; > + else > + adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2; > + > + adapter->tx_itr_setting = adapter->rx_itr_setting; > + } > + goto program_itr; > + } > + > /* convert to rate of irq's per second */ > if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3) > adapter->rx_itr_setting = ec->rx_coalesce_usecs; > @@ -926,13 +949,12 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev, > adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2; > > /* convert to rate of irq's per second */ > - if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) > - adapter->tx_itr_setting = adapter->rx_itr_setting; > - else if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3) > + if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3) > adapter->tx_itr_setting = ec->tx_coalesce_usecs; > else > adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2; > > +program_itr: > for (i = 0; i < adapter->num_q_vectors; i++) { > struct igc_q_vector *q_vector = adapter->q_vector[i]; >
Dear Anthony, Thanks for reviewing. > -----Original Message----- > From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Sent: Saturday, 8 July, 2023 1:30 AM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; > intel-wired-lan@osuosl.org > Cc: Neftin, Sasha <sasha.neftin@intel.com>; Gomes, Vinicius > <vinicius.gomes@intel.com>; naamax.meir@linux.intel.com > Subject: Re: [PATCH iwl-net v1] igc: Expose and modify tx-usecs coalesce > setting > > On 7/6/2023 8:00 AM, Muhammad Husaini Zulkifli wrote: > > When users attempt to obtain the coalesce setting using the ethtool > > command, current code always returns 0 for tx-usecs. > > This is because I225/6 always uses a queue pair setting, hence > > Should there be a follow-on patch to remove 'IGC_FLAG_QUEUE_PAIRS' > altogether then? This setting is still been used for certain case. > > > tx_coalesce_usecs does not return a value during the > > igc_ethtool_get_coalesce() callback process. The pair queue condition > > checking in igc_ethtool_get_coalesce() is removed by this patch so > > that the user gets information of the value of tx-usecs. > > > > Even if i225/6 is using queue pair setting, there is no harm in > > notifying the user of the tx-usecs. The implementation of the current > > code may have previously been a copy of the legacy code i210. > > > > This patch also enables users to modify the tx-usecs parameter. > > The rx-usecs value will adhere to the same value as the set tx-usecs > > because the queue pair setting is enabled. > > This seems like it should be a seperate patch. Yes, that is possible. Previously I thought of combining them all into a single patch. Let me separate it. > > > How to test: > > User can set or get the coalesce value using ethtool command. > > > > Example command: > > Get: ethtool -c <interface> > > Set: ethtool -C <interface> > > > > Previous output (using get command): > > > > rx-usecs: 3 > > rx-frames: n/a > > rx-usecs-irq: n/a > > rx-frames-irq: n/a > > > > tx-usecs: 0 > > tx-frames: n/a > > tx-usecs-irq: n/a > > tx-frames-irq: n/a > > > > New output (using get command): > > > > rx-usecs: 3 > > rx-frames: n/a > > rx-usecs-irq: n/a > > rx-frames-irq: n/a > > > > tx-usecs: 3 > > tx-frames: n/a > > tx-usecs-irq: n/a > > tx-frames-irq: n/a > > > > Previous output (using set command): > > > > root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 netlink error: > > Invalid argument > > > > New output (using set command): > > > > root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 > > rx-usecs: 10 > > rx-frames: n/a > > rx-usecs-irq: n/a > > rx-frames-irq: n/a > > > > tx-usecs: 10 > > tx-frames: n/a > > tx-usecs-irq: n/a > > tx-frames-irq: n/a > > > > Fixes: 7df76bd19181 ("igc: Add 'igc_ethtool_' prefix to functions in > > igc_ethtool.c") > > Please use the commit that introduced the functional problem, not one that > had no functional change. Ops. Thank you for pointing that out. Must be a mistake during copy/paste. It should be this fixes -> " igc: Add ethtool support" > > > Signed-off-by: Muhammad Husaini Zulkifli > > <muhammad.husaini.zulkifli@intel.com> > > --- > > drivers/net/ethernet/intel/igc/igc_ethtool.c | 46 +++++++++++++++----- > > 1 file changed, 34 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c > > b/drivers/net/ethernet/intel/igc/igc_ethtool.c > > index 93bce729be76..1cf7131a82c5 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > > @@ -880,12 +880,10 @@ static int igc_ethtool_get_coalesce(struct > net_device *netdev, > > else > > ec->rx_coalesce_usecs = adapter->rx_itr_setting >> 2; > > > > - if (!(adapter->flags & IGC_FLAG_QUEUE_PAIRS)) { > > - if (adapter->tx_itr_setting <= 3) > > - ec->tx_coalesce_usecs = adapter->tx_itr_setting; > > - else > > - ec->tx_coalesce_usecs = adapter->tx_itr_setting >> > 2; > > - } > > + if (adapter->tx_itr_setting <= 3) > > + ec->tx_coalesce_usecs = adapter->tx_itr_setting; > > + else > > + ec->tx_coalesce_usecs = adapter->tx_itr_setting >> 2; > > > > return 0; > > } > > @@ -910,15 +908,40 @@ static int igc_ethtool_set_coalesce(struct > net_device *netdev, > > ec->tx_coalesce_usecs == 2) > > return -EINVAL; > > > > - if ((adapter->flags & IGC_FLAG_QUEUE_PAIRS) && ec- > >tx_coalesce_usecs) > > - return -EINVAL; > > - > > /* If ITR is disabled, disable DMAC */ > > if (ec->rx_coalesce_usecs == 0) { > > if (adapter->flags & IGC_FLAG_DMAC) > > adapter->flags &= ~IGC_FLAG_DMAC; > > } > > > > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { > > This is removed from igc_ethtool_get_coalesce() becuase 'I225/6 always > uses a queue pair setting', but you add the condition check here? We still need the IGC_FLAG_QUEUE_PAIRS setting here to configure the interrupt moderation because the queue pair option is still used dependent on queue configuration. If the user changes the value of tx-usecs, rx-usecs will follow similarly. Thanks, Husaini > > > + u32 old_tx_itr, old_rx_itr; > > + > > + /* This is to get back the original value before byte shifting */ > > + old_tx_itr = (adapter->tx_itr_setting <= 3) ? > > + adapter->tx_itr_setting : adapter->tx_itr_setting > >> 2; > > + > > + old_rx_itr = (adapter->rx_itr_setting <= 3) ? > > + adapter->rx_itr_setting : adapter->rx_itr_setting > >> 2; > > + > > + if (old_tx_itr != ec->tx_coalesce_usecs) { > > + if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs > <= 3) > > + adapter->tx_itr_setting = ec- > >tx_coalesce_usecs; > > + else > > + adapter->tx_itr_setting = ec- > >tx_coalesce_usecs << 2; > > + > > + adapter->rx_itr_setting = adapter->tx_itr_setting; > > + } else if (old_rx_itr != ec->rx_coalesce_usecs) { > > + if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs > <= 3) > > + adapter->rx_itr_setting = ec- > >rx_coalesce_usecs; > > + else > > + adapter->rx_itr_setting = ec- > >rx_coalesce_usecs << 2; > > + > > + adapter->tx_itr_setting = adapter->rx_itr_setting; > > + } > > + goto program_itr; > > + } > > + > > /* convert to rate of irq's per second */ > > if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3) > > adapter->rx_itr_setting = ec->rx_coalesce_usecs; @@ - > 926,13 > > +949,12 @@ static int igc_ethtool_set_coalesce(struct net_device > *netdev, > > adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2; > > > > /* convert to rate of irq's per second */ > > - if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) > > - adapter->tx_itr_setting = adapter->rx_itr_setting; > > - else if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3) > > + if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3) > > adapter->tx_itr_setting = ec->tx_coalesce_usecs; > > else > > adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2; > > > > +program_itr: > > for (i = 0; i < adapter->num_q_vectors; i++) { > > struct igc_q_vector *q_vector = adapter->q_vector[i]; > >
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index 93bce729be76..1cf7131a82c5 100644 --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c @@ -880,12 +880,10 @@ static int igc_ethtool_get_coalesce(struct net_device *netdev, else ec->rx_coalesce_usecs = adapter->rx_itr_setting >> 2; - if (!(adapter->flags & IGC_FLAG_QUEUE_PAIRS)) { - if (adapter->tx_itr_setting <= 3) - ec->tx_coalesce_usecs = adapter->tx_itr_setting; - else - ec->tx_coalesce_usecs = adapter->tx_itr_setting >> 2; - } + if (adapter->tx_itr_setting <= 3) + ec->tx_coalesce_usecs = adapter->tx_itr_setting; + else + ec->tx_coalesce_usecs = adapter->tx_itr_setting >> 2; return 0; } @@ -910,15 +908,40 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev, ec->tx_coalesce_usecs == 2) return -EINVAL; - if ((adapter->flags & IGC_FLAG_QUEUE_PAIRS) && ec->tx_coalesce_usecs) - return -EINVAL; - /* If ITR is disabled, disable DMAC */ if (ec->rx_coalesce_usecs == 0) { if (adapter->flags & IGC_FLAG_DMAC) adapter->flags &= ~IGC_FLAG_DMAC; } + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { + u32 old_tx_itr, old_rx_itr; + + /* This is to get back the original value before byte shifting */ + old_tx_itr = (adapter->tx_itr_setting <= 3) ? + adapter->tx_itr_setting : adapter->tx_itr_setting >> 2; + + old_rx_itr = (adapter->rx_itr_setting <= 3) ? + adapter->rx_itr_setting : adapter->rx_itr_setting >> 2; + + if (old_tx_itr != ec->tx_coalesce_usecs) { + if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3) + adapter->tx_itr_setting = ec->tx_coalesce_usecs; + else + adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2; + + adapter->rx_itr_setting = adapter->tx_itr_setting; + } else if (old_rx_itr != ec->rx_coalesce_usecs) { + if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3) + adapter->rx_itr_setting = ec->rx_coalesce_usecs; + else + adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2; + + adapter->tx_itr_setting = adapter->rx_itr_setting; + } + goto program_itr; + } + /* convert to rate of irq's per second */ if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3) adapter->rx_itr_setting = ec->rx_coalesce_usecs; @@ -926,13 +949,12 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev, adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2; /* convert to rate of irq's per second */ - if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) - adapter->tx_itr_setting = adapter->rx_itr_setting; - else if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3) + if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3) adapter->tx_itr_setting = ec->tx_coalesce_usecs; else adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2; +program_itr: for (i = 0; i < adapter->num_q_vectors; i++) { struct igc_q_vector *q_vector = adapter->q_vector[i];
When users attempt to obtain the coalesce setting using the ethtool command, current code always returns 0 for tx-usecs. This is because I225/6 always uses a queue pair setting, hence tx_coalesce_usecs does not return a value during the igc_ethtool_get_coalesce() callback process. The pair queue condition checking in igc_ethtool_get_coalesce() is removed by this patch so that the user gets information of the value of tx-usecs. Even if i225/6 is using queue pair setting, there is no harm in notifying the user of the tx-usecs. The implementation of the current code may have previously been a copy of the legacy code i210. This patch also enables users to modify the tx-usecs parameter. The rx-usecs value will adhere to the same value as the set tx-usecs because the queue pair setting is enabled. How to test: User can set or get the coalesce value using ethtool command. Example command: Get: ethtool -c <interface> Set: ethtool -C <interface> Previous output (using get command): rx-usecs: 3 rx-frames: n/a rx-usecs-irq: n/a rx-frames-irq: n/a tx-usecs: 0 tx-frames: n/a tx-usecs-irq: n/a tx-frames-irq: n/a New output (using get command): rx-usecs: 3 rx-frames: n/a rx-usecs-irq: n/a rx-frames-irq: n/a tx-usecs: 3 tx-frames: n/a tx-usecs-irq: n/a tx-frames-irq: n/a Previous output (using set command): root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 netlink error: Invalid argument New output (using set command): root@P12DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 rx-usecs: 10 rx-frames: n/a rx-usecs-irq: n/a rx-frames-irq: n/a tx-usecs: 10 tx-frames: n/a tx-usecs-irq: n/a tx-frames-irq: n/a Fixes: 7df76bd19181 ("igc: Add 'igc_ethtool_' prefix to functions in igc_ethtool.c") Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> --- drivers/net/ethernet/intel/igc/igc_ethtool.c | 46 +++++++++++++++----- 1 file changed, 34 insertions(+), 12 deletions(-)