netdev - Re: [PATCH 3/3] net: mvneta: Introduce a software TSO implementation

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:	Wed, 21 May 2014 03:11:31 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <eric.dumazet@...il.com>, cmetcalf@...era.com,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	Simon Guinot <simon.guinot@...uanux.org>,
	Willy Tarreau <w@....eu>, Tawfik Bayouk <tawfik@...vell.com>,
	Lior Amsalem <alior@...vell.com>,
	Simon Guinot <sguinot@...ie.com>
Subject: Re: [PATCH 3/3] net: mvneta: Introduce a software TSO implementation

On Mon, 2014-05-05 at 11:47 -0300, Ezequiel Garcia wrote:
> Hi all,
> 
> On 10 Apr 07:58 PM, Ezequiel Garcia wrote:
> [..]
> > +
> > +	/* Calculate expected number of TX descriptors */
> > +	desc_count = skb_shinfo(skb)->gso_segs * 2 + skb_shinfo(skb)->nr_frags;
> > +	if ((txq->count + desc_count) >= txq->size)
> > +		return 0;
> > +
> 
> Is this calculus correct? Does it give the accurate number of needed
> descriptors or is it an approximation?
> 
> Tilera's tilegx driver does a much stricter descriptor count (see
> tso_count_edescs). This functions loops through the skb_frag_t fragments
> as it's done later in the data egress, hence strictly counting the
> needed descriptors.
> 
> However, as it's a much heavier routine than the one shown above,
> I'm wondering if we can get away without it.
> 
> Willy, Any ideas here?

There are (at least) three potential bugs you need to avoid:

1. You must not underestimate the number of descriptors required here,
otherwise the ring may overflow.  Hopefully that's obvious.
2. You must ensure that the worst case number of descriptors does
actually fit in the ring, and will probably need to set
net_dev->gso_max_segs accordingly.  Eric pointed that out already.
3. If you stop the queue because an skb doesn't fit, you should not wake
it before there is enough space.

A simple way to do this is:

- Set a maximum number of segments to support (for sfc, I reckoned that
100 was enough)
- Calculate the maximum number of descriptors that could be needed for
that number of segments
- Stop the queue when the free space in the ring is less than that
maximum
- Wake the queue when the free space reaches a threshold somewhere
between that maximum and completely free

It may make more sense to work backward from the ring size to maximum
number of segments.

Ben.

-- 
Ben Hutchings
Life would be so much easier if we could look at the source code.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)