netdev - Re: [PATCH 2/2] net: emac: implement TCP 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]
Date:   Fri, 19 Oct 2018 17:39:30 +0200
From:   Christian Lamparter <chunkeey@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH 2/2] net: emac: implement TCP TSO

Hello,

On Wednesday, October 17, 2018 10:09:44 PM CEST Florian Fainelli wrote:
> On 10/17/2018 12:53 PM, Christian Lamparter wrote:
> > This patch enables TSO(v4) hw feature for emac driver.
> > As atleast the APM82181's TCP/IP acceleration hardware
> > controller (TAH) provides TCP segmentation support in
> > the transmit path.
> > 
> > Signed-off-by: Christian Lamparter <chunkeey@...il.com>
> > ---
> > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > index be560f9031f4..49ffbd6e1707 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.c
> > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > @@ -1410,6 +1413,52 @@ static inline u16 emac_tx_csum(struct emac_instance *dev,
> >  	return 0;
> >  }
> >  
> > +const u32 tah_ss[TAH_NO_SSR] = { 9000, 4500, 1500, 1300, 576, 176 };
> > +
> > +static int emac_tx_tso(struct emac_instance *dev, struct sk_buff *skb,
> > +		       u16 *ctrl)
> > +{
> > +	if (emac_has_feature(dev, EMAC_FTR_TAH_HAS_TSO) &&
> > +	    skb_is_gso(skb) && !!(skb_shinfo(skb)->gso_type &
> > +				(SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
> > +		u32 seg_size = 0, i;
> > +
> > +		/* Get the MTU */
> > +		seg_size = skb_shinfo(skb)->gso_size + tcp_hdrlen(skb)
> > +			+ skb_network_header_len(skb);
> > +
> > +		/* Restriction applied for the segmentation size
> > +		 * to use HW segmentation offload feature: the size
> > +		 * of the segment must not be less than 168 bytes for
> > +		 * DIX formatted segments, or 176 bytes for
> > +		 * IEEE formatted segments.
> > +		 *
> > +		 * I use value 176 to check for the segment size here
> > +		 * as it can cover both 2 conditions above.
> > +		 */
> > +		if (seg_size < 176)
> > +			return -ENODEV;
> > +
> > +		/* Get the best suitable MTU */
> > +		for (i = 0; i < ARRAY_SIZE(tah_ss); i++) {
> > +			u32 curr_seg = tah_ss[i];
> > +
> > +			if (curr_seg > dev->ndev->mtu ||
> > +			    curr_seg > seg_size)
> > +				continue;
> > +
> > +			*ctrl &= ~EMAC_TX_CTRL_TAH_CSUM;
> > +			*ctrl |= EMAC_TX_CTRL_TAH_SSR(i);
> > +			return 0;
> 
> This is something that you can possibly take out of your hot path and
> recalculate when the MTU actually changes?

Do you mean the curr_seg > dev->ndev->mtu check? Because from what I
know, the MSS can be manually set by a socket option (TCP_MAXSEG) on a
"per-socket" base.

(Altough, iperf warns that "Setting MSS is very buggy"... Which it is
as I see more way retries with a iperf run with a MSS less than
768-ish. Ideally, the change_mtu could update the TAH_SS register )

> [snip]
> 
> > +static netdev_tx_t emac_sw_tso(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > +	struct emac_instance *dev = netdev_priv(ndev);
> > +	struct sk_buff *segs, *curr;
> > +
> > +	segs = skb_gso_segment(skb, ndev->features &
> > +					~(NETIF_F_TSO | NETIF_F_TSO6));
> > +	if (IS_ERR_OR_NULL(segs)) {
> > +		goto drop;
> > +	} else {
> > +		while (segs) {
> > +			/* check for overflow */
> > +			if (dev->tx_cnt >= NUM_TX_BUFF) {
> > +				dev_kfree_skb_any(segs);
> > +				goto drop;
> > +			}
> 
> Would setting dev->max_gso_segs somehow help make sure the stack does
> not feed you oversized GSO'd skbs?
Ok, thanks's for that pointer. I'll look at dev->gso_max_segs and
dev->gso_max_size. The hardware documentation doesn't mention any
hard upper limit for how many segments it can do. What it does tell
is that it just needs an extra 512Bytes in the TX FIFO as a buffer
to store the header template and the calculated checksums and what
not.
 
But this should be no problem because that TX FIFO is 10 KiB. so even
the 9000 Jumbo frames should have plenty of "free real estate".

As for the "overflow" case:

There's a check in emac_start_xmit_sg() before emac_tx_tso() call that
does an *estimation* check and goes to "stop_queue" if it doesn't fit:

|        /* Note, this is only an *estimation*, we can still run out of empty
|         * slots because of the additional fragmentation into
|         * MAL_MAX_TX_SIZE-sized chunks
|         */
|        if (unlikely(dev->tx_cnt + nr_frags + mal_tx_chunks(len) > NUM_TX_BUFF))
|                goto stop_queue;
|        [...]
|
|        if (emac_tx_tso(dev, skb, &ctrl))
|               return emac_sw_tso(skb, ndev);
|        [....]
|
|		stop_queue:
|		 netif_stop_queue(ndev);
|		 DBG2(dev, "stopped TX queue" NL);
|		 return NETDEV_TX_BUSY;

emac_start_xmit_sg() can also drop the whole skb later if it turns
out that it doesn't fit. (see the "undo_frame" goto label in emac's
core.c file (not included in the extract above, but it is there).

That said, I could do a better job at making sure that no incomplete
skb gets sent to the network though. 
@Dave: Please ignore this version of the TSO patch for now.

Regards,
Christian