netdev - RE: [PATCH net-next v3] igc: offload queue max SDU from tc-taprio

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]

Date:   Tue, 14 Feb 2023 06:27:17 +0000
From:   "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "intel-wired-lan@...osl.org" <intel-wired-lan@...osl.org>,
        "Gomes, Vinicius" <vinicius.gomes@...el.com>,
        "naamax.meir@...ux.intel.com" <naamax.meir@...ux.intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "leon@...nel.org" <leon@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "tee.min.tan@...ux.intel.com" <tee.min.tan@...ux.intel.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Neftin, Sasha" <sasha.neftin@...el.com>
Subject: RE: [PATCH net-next v3] igc: offload queue max SDU from tc-taprio

Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Thursday, 9 February, 2023 1:30 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>
> Cc: intel-wired-lan@...osl.org; Gomes, Vinicius
> <vinicius.gomes@...el.com>; naamax.meir@...ux.intel.com; Nguyen,
> Anthony L <anthony.l.nguyen@...el.com>; leon@...nel.org;
> davem@...emloft.net; pabeni@...hat.com; edumazet@...gle.com;
> tee.min.tan@...ux.intel.com; netdev@...r.kernel.org; Neftin, Sasha
> <sasha.neftin@...el.com>
> Subject: Re: [PATCH net-next v3] igc: offload queue max SDU from tc-taprio
> 
> On Wed,  8 Feb 2023 08:33:27 +0800 Muhammad Husaini Zulkifli wrote:
> > From: Tan Tee Min <tee.min.tan@...ux.intel.com>
> >
> > Add support for configuring the max SDU for each Tx queue.
> > If not specified, keep the default.
> 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 0cc327294dfb5..38ad437957ada 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -1508,6 +1508,7 @@ static netdev_tx_t igc_xmit_frame_ring(struct
> sk_buff *skb,
> >  	__le32 launch_time = 0;
> >  	u32 tx_flags = 0;
> >  	unsigned short f;
> > +	u32 max_sdu = 0;
> 
> This variable can be moved to the scope of the if() ?

Sure. Let me move to the if() scope in v5. 

> 
> >  	ktime_t txtime;
> >  	u8 hdr_len = 0;
> >  	int tso = 0;
> > @@ -1527,6 +1528,14 @@ static netdev_tx_t igc_xmit_frame_ring(struct
> sk_buff *skb,
> >  		return NETDEV_TX_BUSY;
> >  	}
> >
> > +	if (tx_ring->max_sdu > 0) {
> > +		max_sdu = tx_ring->max_sdu +
> > +			  (skb_vlan_tagged(skb) ? VLAN_HLEN : 0);
> > +
> > +		if (skb->len > max_sdu)
> 
> You should increment some counter here. Otherwise it's a silent discard.

I am thinking to use tx_dropped counters for this. Is it ok?

> 
> > +			goto skb_drop;
> > +	}
> > +
> >  	if (!tx_ring->launchtime_enable)
> >  		goto done;
> >
> > @@ -1606,6 +1615,11 @@ static netdev_tx_t igc_xmit_frame_ring(struct
> sk_buff *skb,
> >  	dev_kfree_skb_any(first->skb);
> 
> first->skb is skb, as far as I can tell, you can reshuffle this code to
> avoid adding the new return flow.

What we try to do is to check the current max_sdu size at the
beginning stage of the func() and drop it quickly.

> 
> >  	first->skb = NULL;
> >
> > +	return NETDEV_TX_OK;
> > +
> > +skb_drop:
> > +	dev_kfree_skb_any(skb);
> > +
> >  	return NETDEV_TX_OK;
> >  }
> 
> > @@ -6122,6 +6137,16 @@ static int igc_save_qbv_schedule(struct
> igc_adapter *adapter,
> >  		}
> >  	}
> >
> > +	for (i = 0; i < adapter->num_tx_queues; i++) {
> > +		struct igc_ring *ring = adapter->tx_ring[i];
> > +		struct net_device *dev = adapter->netdev;
> > +
> > +		if (qopt->max_sdu[i])
> > +			ring->max_sdu = qopt->max_sdu[i] + dev-
> >hard_header_len;
> 
> why hard_header_len? Isn't it always ETH_HLEN?

We followed the taprio_parse_tc_entries() implementation for this.
But the hard_header_len should be same value as ETH_HLEN.

> 
> > +		else
> > +			ring->max_sdu = 0;