netdev - Re: [PATCH] net/core/sock.c remove extra wakeup

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

Date:	Fri, 30 May 2008 04:05:00 -0600
From:	"Gregory Haskins" <GHaskins@...ell.com>
To:	<davem@...emloft.net>, "Patrick Mullaney" <PMullaney@...ell.com>
Cc:	<netdev@...r.kernel.org>
Subject: Re: [PATCH] net/core/sock.c remove extra wakeup

Sorry for the top post (on a phone)

Point taken that the fix needs to be analysed for races, but the problem is very real.  We noticed that our udp netperf test would get two wake-ups per packet.  I don't remember the exact path where the udp waits occur off the top of my head, but the fundamental problem is the overloaded use of the single wait-queue.

So basically it was waiting for a packet to arrive, and the net-rx softirq would first trigger a NOSPACE wakeup (of which was a noop by the udp-rx code) followed by an rx-wakeup.  So the netperf thread woke twice for one packet.  Fixing this boosted performance by a large margin (don't recall exact figures off the top of my head....somewhere around 20%)

Long stort short, its potentially worth fixing this, even if this patch isn't "it" :)

HTH

Regards
-Greg  

-----Original Message-----
From: David Miller <davem@...emloft.net>
To: Patrick Mullaney <PMullaney@...ell.com>
Cc: Gregory Haskins <GHaskins@...ell.com>
Cc:  <netdev@...r.kernel.org>

Sent: 5/30/2008 3:52:54 AM
Subject: Re: [PATCH] net/core/sock.c remove extra wakeup

From: "Patrick Mullaney" <pmullaney@...ell.com>
Date: Thu, 29 May 2008 10:08:43 -0600

> We have noticed an extra wakeup of a task waiting on a udp receive
> while testing with CONFIG_PREEMPT_RT. The following patch
> eliminates the source of the extra wakeup by only waking a task
> if it is waiting on the writable condition(the SOCK_NOSPACE
> bit has been set). The patch is against 2.6.25.
> 
> Signed-off-by: Patrick Mullaney <pmullaney@...ell.com>

This looks Ok on the surface, but I'm really worried about
races.

If you clear that bit, another thread which set that bit
and already checked the socket space, might sleep and never
wake up.

I think that is fundamentally why we don't try to optimize
this in that way.

And why is this so expensive?  Once the first wakeup occurs,
the subsequent attemps will merely see that the wait queue
is empty and do nothing.

The only cost is the read lock on the callback lock, and that
is about as expensive as this new atomic operation you are
adding to clear the SOCK_NOSPACE bit.

So the benefit is not clear, and neither is the correctness of
this change.

Sorry.

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