netdev - Re: [PATCH net-next] tcp: reduce out_of

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:	Sun, 18 Mar 2012 12:40:46 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Neal Cardwell <ncardwell@...gle.com>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Tom Herbert <therbert@...gle.com>,
	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>,
	"H.K. Jerry Chu" <hkchu@...gle.com>,
	Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH net-next] tcp: reduce out_of_order memory use

On Sun, 2012-03-18 at 12:55 -0400, Neal Cardwell wrote:

> 
> At this point in tcp_data_queue() we have already called
> skb_set_owner_r() to charge the full truesize to the socket.  So
> probably we need to adjust rmem accounting variables to account for
> the fact that the memory corresponding to the overhead of the freed
> skb is now gone?

This is automatically done in the __kfree_skb(skb) call.

I thought of avoiding the skb_set_owner_r() but it was simpler to let
the code as is.

Thinking again, we might just defer it at the end of function if skb is
not NULL, but it adds a new conditional and a "returb;" must be changed
to "goto end;"

> 
> The patch as written seems to only coalesce if the incoming skb fits
> immediately after the first skb in the ofo queue. If we're going to
> add logic to coalesce then it would be nice to also coalesce in the
> case where the skb falls immediately after any later skb in the ofo queue,

I tried this but it was almost never used code in my experiments.

Most of the times packets come in natural order (no OOO).

> i.e. at some point after the "Find place to insert this segment" loop,
> probably right before or after the "Do skb overlap to previous one?"
> check. Perhaps the coalescing logic could be factored out into its own
> little function, and called in either of the two places?
> 

Absolutely, I was already working on splitting tcp_data_queue() in two
functions to reduce indentation level by one tabulation make it more
readable.

Yes, we probably can expand the coalescing logic to be able to cope with
OOO, so that not only we coalesce this skb with prior one in queue, but
also with next one if there is one.

> (BTW, a few lines are longer than 80 characters.)

This wont be the case once tcp_data_queue_ofo() is introduced.

I'll send a V2 with two patches.

1/2 tcp: introduce tcp_data_queue_ofo()
    (No change, only code movement to make smaller code units)

2/2 tcp: reduce out_of_order memory use
    (with the attempt to avoid the skb_set_owner_r()/__kfree_skb() as
you suggested)

Thanks !


--
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