netdev - Re: [PATCH net-next v3 6/8] igc: Add support for tuning frame preemption via ethtool

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210126003243.x3c44pmxmieqsa6e@skbuf>
Date:   Tue, 26 Jan 2021 00:32:44 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Vinicius Costa Gomes <vinicius.gomes@...el.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "Jose.Abreu@...opsys.com" <Jose.Abreu@...opsys.com>,
        Po Liu <po.liu@....com>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "anthony.l.nguyen@...el.com" <anthony.l.nguyen@...el.com>,
        "mkubecek@...e.cz" <mkubecek@...e.cz>
Subject: Re: [PATCH net-next v3 6/8] igc: Add support for tuning frame
 preemption via ethtool

On Fri, Jan 22, 2021 at 02:44:51PM -0800, Vinicius Costa Gomes wrote:
> The tc subsystem sets which queues are marked as preemptible, it's the
> role of ethtool to control more hardware specific parameters. These
> parameters include:
> 
>  - enabling the frame preemption hardware: As enabling frame
>  preemption may have other requirements before it can be enabled, it's
>  exposed via the ethtool API;
> 
>  - mininum fragment size multiplier: expressed in usually in the form
>  of (1 + N)*64, this number indicates what's the size of the minimum
>  fragment that can be preempted.

And not one word has been said about the patch...

> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h         | 12 +++++
>  drivers/net/ethernet/intel/igc/igc_defines.h |  4 ++
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 53 ++++++++++++++++++++
>  drivers/net/ethernet/intel/igc/igc_tsn.c     | 25 +++++++--
>  4 files changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 35baae900c1f..1067c46e0bc2 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -87,6 +87,7 @@ struct igc_ring {
>  	u8 queue_index;                 /* logical index of the ring*/
>  	u8 reg_idx;                     /* physical index of the ring */
>  	bool launchtime_enable;         /* true if LaunchTime is enabled */
> +	bool preemptible;		/* true if not express */

Mixing tabs and spaces?

> +static int igc_ethtool_set_preempt(struct net_device *netdev,
> +				   struct ethtool_fp *fpcmd,
> +				   struct netlink_ext_ack *extack)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	int i;
> +
> +	if (fpcmd->add_frag_size < 68 || fpcmd->add_frag_size > 260) {
> +		NL_SET_ERR_MSG_MOD(extack, "Invalid value for add-frag-size");
> +		return -EINVAL;
> +	}

This check should belong in ethtool, since there's nothing unusual about
this supported range.

Also, I believe that Jakub requested the min-frag-size to be passed as
0, 1, 2, 3 as the standard specifies it, and not its multiplied version?

> +
> +	adapter->frame_preemption_active = fpcmd->enabled;
> +	adapter->add_frag_size = fpcmd->add_frag_size;
> +
> +	if (!adapter->frame_preemption_active)
> +		goto done;
> +
> +	/* Enabling frame preemption requires TSN mode to be enabled,
> +	 * which requires a schedule to be active. So, if there isn't
> +	 * a schedule already configured, configure a simple one, with
> +	 * all queues open, with 1ms cycle time.
> +	 */
> +	if (adapter->base_time)
> +		goto done;

Unless I'm missing something, you are interpreting an adapter->base_time
value of zero as "no Qbv schedule on port", as if it was invalid to have
a base-time of zero, which it isn't.

> @@ -115,6 +130,9 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
>  		if (ring->launchtime_enable)
>  			txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
>  
> +		if (ring->preemptible)
> +			txqctl |= IGC_TXQCTL_PREEMPTABLE;

I think this is the only place in the series where you use PREEMPTABLE
instead of PREEMPTIBLE.

> +
>  		wr32(IGC_TXQCTL(i), txqctl);
>  	}

Out of curiosity, where is the ring to traffic class mapping configured
in the igc driver? I suppose that you have more rings than traffic classes.