On Fri, Apr 07, 2006 at 04:13:45PM +0800, Yi Yang ([email protected]) wrote:
> >>+
> >>+ return (netlink_unicast(fsevent_sock, skb, pid, MSG_DONTWAIT));
> >>
> >
> >netlink_unicast() uses boolean value but ont MSG_* flags for nonblocking,
> >so this should be netlink_unicast(fsevent_sock, skb, pid, 0);
> >
> a example invocation in file net/netlink/af_netlink.c:
> netlink_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).pid, MSG_DONTWAIT);
> so, it hasn't any problem.
Well...
static inline long sock_sndtimeo(const struct sock *sk, int noblock)
{
return noblock ? 0 : sk->sk_sndtimeo;
}
int netlink_unicast(struct sock *ssk, struct sk_buff *skb, u32 pid, int nonblock)
{
struct sock *sk;
int err;
long timeo;
skb = netlink_trim(skb, gfp_any());
timeo = sock_sndtimeo(ssk, nonblock);
I mean that it is boolean value, MSG_PEEK will produce the same result.
But it is a matter of coding style probably.
> >>+nlmsg_failure:
> >>+ kfree_skb(skb);
> >>+ return -1;
> >>+}
> >>
> >
> >...
> >
> >
> >>+static void fsevent_recv(struct sock *sk, int len)
> >>+{
> >>+ struct sk_buff *skb = NULL;
> >>+ struct nlmsghdr *nlhdr = NULL;
> >>+ struct fsevent_filter * filter = NULL;
> >>+ pid_t pid;
> >>+
> >>+ while ((skb = skb_dequeue(&sk->sk_receive_queue)) != NULL) {
> >>+ skb_get(skb);
> >>+ if (skb->len >= FSEVENT_FILTER_MSGSIZE) {
> >>+ nlhdr = (struct nlmsghdr *)skb->data;
> >>+ filter = NLMSG_DATA(nlhdr);
> >>+ pid = NETLINK_CREDS(skb)->pid;
> >>+ if (find_fsevent_listener(pid) == NULL)
> >>+ atomic_inc(&fsevent_listener_num);
> >>+ set_fsevent_filter(filter, pid);
> >>
> >
> >What is the logic behind this steps?
> >If there are no listeners you increment it's number no matter if it will
> >or not be added in set_fsevent_filter().
> >
> fsevent_recv is used to receive listener's commands, a listener must
> send commands in order to get fsevents it
> interests, so this is the best point to increment number of listeners.
> set_fsevent_filter will add listener to listener
> list, so it is OK.
And what if set_fsevent_filter() fails?
> >>+ }
> >>+ kfree_skb(skb);
> >>+ }
> >>+}
> >>+
> >>+#define DEFINE_FILTER_MATCH_FUNC(filtertype, key) \
> >>+ static int match_##filtertype(listener * p, \
> >>+ struct fsevent * event, \
> >>+ struct sk_buff * skb) \
> >>+ { \
> >>+ int ret = 0; \
> >>+ filtertype * xfilter = NULL; \
> >>+ struct sk_buff * skb2 = NULL; \
> >>+ struct list_head * head = &(p->key##_filter_list_head); \
> >>+ list_for_each_entry(xfilter, head, list) { \
> >>+ if (xfilter->key != event->key) \
> >>+ continue; \
> >>+ ret = filter_fsevent(xfilter->mask, event->type); \
> >>+ if ( ret != 0) \
> >>+ return -1; \
> >>+ skb2 = skb_clone(skb, GFP_KERNEL); \
> >>+ if (skb2 == NULL) \
> >>+ return -ENOMEM; \
> >>+ NETLINK_CB(skb2).dst_group = 0; \
> >>+ NETLINK_CB(skb2).dst_pid = p->pid; \
> >>+ NETLINK_CB(skb2).pid = 0; \
> >>+ return (netlink_unicast(fsevent_sock, skb2, \
> >>+ p->pid, MSG_DONTWAIT)); \
> >>
> >
> >The same issue about nonblocking sending.
> >
> >
> >>+ } \
> >>+ return -ENODEV; \
> >>+ } \
> >>+
> >>+DEFINE_FILTER_MATCH_FUNC(pid_filter, pid)
> >>+
> >>+DEFINE_FILTER_MATCH_FUNC(uid_filter, uid)
> >>+
> >>+DEFINE_FILTER_MATCH_FUNC(gid_filter, gid)
> >>+
> >>+#define MATCH_XID(key, listenerp, event, skb) \
> >>+ ret = match_##key##_filter(listenerp, event, skb); \
> >>+ if (ret == 0) { \
> >>+ kfree_skb(skb); \
> >>+ continue; \
> >>+ } \
> >>+ do {} while (0) \
> >>+
> >>+static int fsevent_send_to_process(struct sk_buff * skb)
> >>+{
> >>+ listener * p = NULL, * q = NULL;
> >>+ struct fsevent * event = NULL;
> >>+ struct sk_buff * skb2 = NULL;
> >>+ int ret = 0;
> >>+
> >>+ event = (struct fsevent *)(skb->data + sizeof(struct nlmsghdr));
> >>+ spin_lock(&listener_list_lock);
> >>+ list_for_each_entry_safe(p, q, &listener_list_head, list) {
> >>+ MATCH_XID(pid, p, event, skb);
> >>+ MATCH_XID(uid, p, event, skb);
> >>+ MATCH_XID(gid, p, event, skb);
> >>+
> >>+ if (filter_fsevent(p->mask, event->type) == 0) {
> >>+ skb2 = skb_clone(skb, GFP_KERNEL);
> >>+ if (skb2 == NULL)
> >>+ return -ENOMEM;
> >>+ NETLINK_CB(skb2).dst_group = 0;
> >>+ NETLINK_CB(skb2).dst_pid = p->pid;
> >>+ NETLINK_CB(skb2).pid = 0;
> >>+ ret = netlink_unicast(fsevent_sock, skb2,
> >>+ p->pid, MSG_DONTWAIT);
> >>+ if (ret == -ECONNREFUSED) {
> >>+ atomic_dec(&fsevent_listener_num);
> >>+ cleanup_dead_listener(p);
> >>+ }
> >>+ }
> >>+ }
> >>+ spin_unlock(&listener_list_lock);
> >>+ return ret;
> >>+}
> >>+
> >>+static void fsevent_commit(void * unused)
> >>+{
> >>+ struct sk_buff * skb = NULL;
> >>+
> >>+ while((skb = skb_dequeue(&get_cpu_var(fsevent_send_queue)))
> >>+ != NULL) {
> >>+ fsevent_send_to_process(skb);
> >>+ put_cpu_var(fsevent_send_queue);
> >>+ }
> >>+}
> >>
> >
> >Really strange mix of per-cpu variables for optimized performance and
> >global spin locking.
> >Consider using RCU for list of listeners.
> >
> per cpu queue is used to avoid raise_fsevent to contend spinlock, but
> listener_list_lock just is used
> to synchronize the operations of userspace applications(listener) on
> listener list, it just protect listener
> list.
>
> Of course, your advice is good, RCU will be better, I'm considering
> substitute spinlock with RCU,
> maybe list*_rcu functions can help me.
You get global lock in each processor when traverse the list
&listener_list_lock.
And you call GFP_KERNEL allocation under that lock, which is wrong.
If all your code is called from process context (it looks so), you
could mutexes.
> >You use unicast delivery for netlink messages.
> >According to my investigation [1], it's performance is better only when
> >there is only one listener (or maybe two in some cases), but then it is
> >noticebly slower than broadcasting.
> >
> >1. http://marc.theaimsgroup.com/?l=linux-netdev&m=114424884216006&w=2
> >
> Because fsevent has to deliver different events to different listeners,
> so I must use netlink_unicast,
> in fact, netlink_broadcast also must send skb to every member of the
> group, so in my opinion,
> they haven't big difference.
And what if there are several listeners for the same type of events?
> Can you explain why there is such a big difference between
> netlink_unicast and netlink_broadcast?
Netlink broadcast clones skbs, while unicasting requires the whole new
one.
> >Btw, you need some rebalancing of the per-cpu queues, probably in
> >keventd, since CPUs can go offline and your messages will stuck foreve
> >there.
> >
> Does keventd not do it? if so, keventd should be modified.
How does keventd know about your own structures?
You have an per-cpu object, but your keventd function gets object
from running cpu, not from any other cpus.
--
Evgeniy Polyakov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
