| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <SJ1PR11MB61801A9E39A77D8D89349FF4B818A@SJ1PR11MB6180.namprd11.prod.outlook.com>
Date: Sat, 19 Aug 2023 13:21:58 +0000
From: "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To: Brett Creeley <bcreeley@....com>, "Nguyen, Anthony L"
<anthony.l.nguyen@...el.com>, "davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>
CC: "Neftin, Sasha" <sasha.neftin@...el.com>, Naama Meir
<naamax.meir@...ux.intel.com>, Simon Horman <horms@...nel.org>
Subject: RE: [PATCH net v2 2/2] igc: Modify the tx-usecs coalesce setting
Dear Brett,
Sorry for the late reply. I truly miss these feedbacks. My bad ☹
> -----Original Message-----
> From: Brett Creeley <bcreeley@....com>
> Sent: Wednesday, 2 August, 2023 2:18 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> davem@...emloft.net; kuba@...nel.org; pabeni@...hat.com;
> edumazet@...gle.com; netdev@...r.kernel.org
> Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>;
> Neftin, Sasha <sasha.neftin@...el.com>; Naama Meir
> <naamax.meir@...ux.intel.com>; Simon Horman <horms@...nel.org>
> Subject: Re: [PATCH net v2 2/2] igc: Modify the tx-usecs coalesce setting
>
> On 8/1/2023 10:27 AM, Tony Nguyen wrote:
> > Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> >
> >
> > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>
> >
> > This patch enables users to modify the tx-usecs parameter.
> > The rx-usecs value will adhere to the same value as tx-usecs if the
> > queue pair setting is enabled.
> >
> > How to test:
> > User can set the coalesce value using ethtool command.
> >
> > Example command:
> > Set: ethtool -C <interface>
> >
> > Previous output:
> >
> > root@...DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10 netlink error:
> > Invalid argument
> >
> > New output:
> >
> > root@...DYHUSAINI:~# 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: 8c5ad0dae93c ("igc: Add ethtool support")
> > Signed-off-by: Muhammad Husaini Zulkifli
> > <muhammad.husaini.zulkifli@...el.com>
> > Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
> > Reviewed-by: Simon Horman <horms@...nel.org>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> > ---
> > drivers/net/ethernet/intel/igc/igc_ethtool.c | 49 +++++++++++++++-----
> > 1 file changed, 37 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > index 62d925b26f2c..ed67d9061452 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > @@ -914,19 +914,44 @@ static int igc_ethtool_set_coalesce(struct
> net_device *netdev,
> > adapter->flags &= ~IGC_FLAG_DMAC;
> > }
> >
> > - /* 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;
> > - else
> > - adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
> > + 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;
>
> It seems like this if/else flow is duplicated multiple times throughout this
> patch. Maybe it would be useful to introduce a helper function so you can
> just do the following:
Yeah that is a great suggestion. Will do that.
>
> adapter->tx_itr_setting =
> igc_ethtool_coalesce_to_itr_setting(ec->tx_coalesce);
>
> > +
> > + 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;
>
> Seems like the helper function could work for both tx/rx:
Yup. Both are using same condition.
>
> adapter->rx_itr_setting =
> igc_ethtool_coalsece_to_itr_setting(ec->rx_coalesce);
> > +
> > + adapter->tx_itr_setting = adapter->rx_itr_setting;
> > + }
> > + } else {
> > + /* 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;
> > + else
> > + adapter->rx_itr_setting =
> > + ec->rx_coalesce_usecs << 2;
>
> adapter->rx_itr_setting =
> igc_ethtool_coalsece_to_itr_setting(ec->rx_coalesce);
Noted.
>
> >
> > - /* 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)
> > - adapter->tx_itr_setting = ec->tx_coalesce_usecs;
> > - else
> > - adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2;
> > + /* convert to rate of irq's per second */
> > + 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->tx_itr_setting =
> igc_ethtool_coalesce_to_itr_setting(ec->tx_coalesce);
Sure. 😊
>
>
> > + }
> >
> > for (i = 0; i < adapter->num_q_vectors; i++) {
> > struct igc_q_vector *q_vector = adapter->q_vector[i];
> > --
> > 2.38.1
> >
> >
Powered by blists - more mailing lists