netdev
[Top] [All Lists]

Re: [PATCH] Improve behaviour of Netlink Sockets

To: pablo@xxxxxxxxxxx (Pablo Neira)
Subject: Re: [PATCH] Improve behaviour of Netlink Sockets
From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Date: Sun, 19 Sep 2004 17:58:52 +1000
Cc: herbert@xxxxxxxxxxxxxxxxxxx, davem@xxxxxxxxxx, netdev@xxxxxxxxxxx
In-reply-to: <414D0CCD.90209@xxxxxxxxxxx>
Organization: Core
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: tin/1.7.4-20040225 ("Benbecula") (UNIX) (Linux/2.4.27-hx-1-686-smp (i686))
Pablo Neira <pablo@xxxxxxxxxxx> wrote:
> 
> you are right, I missed that, but I prefer the patches attached to this 
> email. Now if netlink_broadcast_deliver function delivers correctly the 
> packet, you decrease sock refcount and function returns 1. I think that 
> I got confused because netlink_broadcast_deliver returns 0/-1.

Unfortunately it is still wrong.  You missed the exit path at the
very top.

And I think rather than adding all these new sock_put's, it's much
better to do what you mean and add a sock_hold on the new path that
you introduced.

>>Secondly, I'm dubious about the patch as a whole.  For instance, what
>>exactly is the wake_up_process() bit trying to do? Surely that process
>>would've been woken up multiple times already if its queue is full.
> 
> This is what I theorically expected, but in practice if you stress a 
> netlink socket sending a big bunch of information in a short period of 
> time from kernel space to user space, socket overruns easily. That's why 
> I wake up the user process to make it process information stored in the 
> queue. Socket doesn't overrun anymore with my patch.

Yes but your patch does a lot more than that wake_up_process.  Have you
reverted just the wake_up_process and seen a difference?

>>And what is it going to do with thread groups?
> 
> currently broadcast sockets can still overrun. I have a set of patches 
> that I'll submit as soon as I test them and I finish my boring exams.

That's not what I meant.  If you have a thread group where everybody's
got the same pid, your code will probably wake up the wrong thread.

Besides, for any netlink socket but the first for a process, they'll
all have negative pid's which have nothing to do with the real pid.
So I really think that the wake_up_process hunk is bogus.

BTW I apologise for the few bounces that you got from my mail server.
It should be fixed now.  If you're still getting bounces, please let
me know via the list.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

<Prev in Thread] Current Thread [Next in Thread>