netdev - Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: remove pointless clause from if statement

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] [day] [month] [year] [list]

Message-ID: <90991437.20140328123555@eikelenboom.it>
Date:	Fri, 28 Mar 2014 12:35:55 +0100
From:	Sander Eikelenboom <linux@...elenboom.it>
To:	David Laight <David.Laight@...LAB.COM>
CC:	'Paul Durrant' <Paul.Durrant@...rix.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Wei Liu <wei.liu2@...rix.com>,
	Ian Campbell <Ian.Campbell@...rix.com>,
	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>
Subject: Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: remove pointless clause from if statement

Friday, March 28, 2014, 12:11:24 PM, you wrote:

> From: Sander Eikelenboom
>> Friday, March 28, 2014, 11:35:58 AM, you wrote:
>> 
>> > From: Paul Durrant
>> >> > A reasonable high estimate for the number of slots required for a specific
>> >> > message is 'frag_count + total_size/4096'.
>> >> > So if that are that many slots free it is definitely ok to add the message.
>> >> >
>> >>
>> >> Hmm, that may work. By total_size, I assume you mean skb->len, so that calculation is based on an
>> >> overhead of 1 non-optimally packed slot per frag. There'd still need to be a +1 for the GSO 'extra'
>> >> though.
>> 
>> > Except I meant '2 * frag_count + size/4096' :-(
>> 
>> > You have to assume that every fragment starts at n*4096-1 (so need
>> > at least two slots). A third slot is only needed for fragments
>> > longer that 1+4096+2 - but an extra one is needed for every
>> > 4096 bytes after that.
>> 
>> He did that in his followup patch series .. that works .. for small packets
>> But for larger ones it's an extremely wasteful estimate and it quickly get larger than the
>> MAX_SKB_FRAGS
>> we had before and even to large causing stalls. I tried doing this type of calculation with a CAP of
>> the old  MAX_SKB_FRAGS calculation and that works.

> I'm confused (easily done).
> If you are trying to guess at the number of packets to queue waiting for
> the thread that sets things up to run then you want an underestimate.
> Since any packets that can't actually be transferred will stay on the queue

We want to overestimate the max_slots_needed .. so that if we check and the ring
hasn't got that many slots free .. we don't dequeue the SKB and wait until there becomes
more space available on the ring.

This is done by a very very cheap minimum estimate .. and a slightly more costly maximum estimate,

The maximum estimate was changed (in said commit) and believed to be the worst case.
But this didn't take the offset into account so it could lead to an underestimation,
which then leads to trying to overrun the ring .. this then fails at the grant_copy code,
since the grant reference is bogus so the hypervisor refuses to do that.

But if you do take the offset into account worst case .. you end up with a gross overestimation
that could even be larger than the ring size, leading to a stall since the packet can never be processed
since the ring can't possibly free more slots that in has. But i think the old calculation is the
theoretical max (due to the limitation in total packetsize not *all* frags can have a offset and be that large
that it would cost more slots).

So you could use the old calc as a CAP so you don't overestimate to the extent that you would stall.
)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html