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
diff -u -r1.3 af_netlink.c
--- a/net/netlink/af_netlink.c 19 Sep 2004 19:46:20 -0000 1.3
+++ b/net/netlink/af_netlink.c 19 Sep 2004 20:04:44 -0000
@@ -629,8 +629,9 @@
kmalloc(sizeof(struct netlink_work),
GFP_KERNEL);
if (!nlwork)
- return -1;
-
+ return 0;
+
+ sock_hold(sk);
INIT_WORK(&nlwork->work, netlink_wq_handler, nlwork);
nlwork->sk = sk;
nlwork->len = skb->len;
@@ -638,9 +639,9 @@
} else
sk->sk_data_ready(sk, skb->len);
- return 0;
+ return 1;
}
- return -1;
+ return 0;
}
int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 pid,
@@ -683,14 +684,13 @@
netlink_overrun(sk);
/* Clone failed. Notify ALL listeners. */
failure = 1;
- sock_put(sk);
} else if (netlink_broadcast_deliver(sk, skb2)) {
- netlink_overrun(sk);
- sock_put(sk);
- } else {
delivered = 1;
skb2 = NULL;
- }
+ } else
+ netlink_overrun(sk);
+
+ sock_put(sk);
}
netlink_unlock_table();
|