Re: [PATCH] Improve behaviour of Netlink Sockets

Hi Herbert,

Herbert Xu wrote:

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

if netlink_broadcast_deliver returns 0, you decrease sock_put. On the 
other hand if it returns 1, refcount will be decreased by the workqueue 
handler or after calling the callback. I don't see that exit path.

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.
 

yes, it's true that all those sock_put pollutes the source code, I like 
your idea of add a sock_hold, please could you have a look at the patch 
attached? it's basically your patch plus netlink_broadcast_deliver 
revised return value. If you are ok with it, let me know.

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?
 

Yes, it works as well so we could remove it, but I got some extra 
throughput with the wake_up_process call if I send something up to 500 
consecutive messages from kernel to user space. For more than 1000 I 
notice that throughput is similar without wake_up_process, I'll study 
the performance without wake_up_process if it's not a good idea using it 
as you think.

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.
 

no, my patch doesn't wake up the user process with broadcast sockets, 
snipped from broadcast_deliver:

        if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf &&
            !test_bit(0, &nlk->state)) {
             [...]
       }
       return 0;

it returns zero then socket overruns.

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.
 

same reply as above, we don't wake up the user process with broadcast 
sockets.

regards,
Pablo