netdev - Re: [PATCH net-next 2/2] net: enetc: add support for software TSO

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: <20211007092655.xzvf76pjzkjwteiz@skbuf>
Date:   Thu, 7 Oct 2021 09:26:56 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Claudiu Manoil <claudiu.manoil@....com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next 2/2] net: enetc: add support for software TSO

On Thu, Oct 07, 2021 at 09:06:45AM +0000, Claudiu Manoil wrote:
> > -----Original Message-----
> > From: Ioana Ciornei <ioana.ciornei@....com>
> > Sent: Thursday, October 7, 2021 11:33 AM
> > To: Claudiu Manoil <claudiu.manoil@....com>
> > Cc: davem@...emloft.net; kuba@...nel.org; netdev@...r.kernel.org;
> > Vladimir Oltean <vladimir.oltean@....com>
> > Subject: Re: [PATCH net-next 2/2] net: enetc: add support for software TSO
> > 
> > On Thu, Oct 07, 2021 at 07:59:25AM +0000, Claudiu Manoil wrote:
> > > > -----Original Message-----
> > > > From: Ioana Ciornei <ioana.ciornei@....com>
> > > > Sent: Wednesday, October 6, 2021 11:13 PM
> > > [...]
> > > > +static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct
> > > > sk_buff *skb)
> > > > +{
> > > > +	int hdr_len, total_len, data_len;
> > > > +	struct enetc_tx_swbd *tx_swbd;
> > > > +	union enetc_tx_bd *txbd;
> > > > +	struct tso_t tso;
> > > > +	__wsum csum, csum2;
> > > > +	int count = 0, pos;
> > > > +	int err, i;
> > > > +
> > > > +	/* Check that we have enough BDs for this skb */
> > > > +	if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
> > > > +		if (net_ratelimit())
> > > > +			netdev_err(tx_ring->ndev, "Not enough BDs for TSO!\n");
> > > > +		return 0;
> > > > +	}
> > > > +
> > >
> > > On this path, in case the interface is congested, you will drop the packet in the driver,
> > > and the stack will think transmission was successful and will continue to deliver skbs
> > > to the driver. Is this the right thing to do?
> > >
> > 
> > Good point. I should have mimicked the non-GSO code path when
> > congestion occurs and stop the subqueue.
> > 
> > For symmetry I'll also move this check outside of the
> > enetc_map_tx_tso_buffs() to get the code looking somewhat like this:
> > 
> > 
> > 	if (skb_is_gso(skb)) {
> > 		if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
> > 			netif_stop_subqueue(ndev, tx_ring->index);
> > 			return NETDEV_TX_BUSY;
> > 		}
> > 
> > 		enetc_lock_mdio();
> > 		count = enetc_map_tx_tso_buffs(tx_ring, skb);
> > 		enetc_unlock_mdio();G
> > 	} else {
> > 		if (unlikely(skb_shinfo(skb)->nr_frags > ENETC_MAX_SKB_FRAGS))
> > 			if (unlikely(skb_linearize(skb)))
> > 				goto drop_packet_err;
> > 
> 
> Ok for handling congestion, the idea is good, but now another issue emerges.
> The ENETC_MAX_SKB_FRAGS check is due to a hardware limitation. Enetc cannot
> handle more than 15 chained buffer descriptors for transmission (i.e. 13 frags + 1
> for the linear part + 1 optional extension BD). This limitation is specified in the
> hardware manual now (after I've hit it during development).

On the TSO processing case this is way less likely to happen since the
resulted segment would have to need 15 chained BDs, not the entire skb.
At a maximum, I have seen one frame needing 4-5 BDs: 1 for the header,
1 extention BD in case of VLAN and 1-3 for the data part.

> So you should add this check on the TSO processing path too, but adapted to that
> case, of course, since skb_linearize() would not work with TSO.
> 

Anyhow, I'll add this check directly in the while loop which creates the
chain of BDs for a frame, since that is the only time that I know how
many BDs are needed for a resulted frame.

Thanks.