Messages in this thread | | | Date | Fri, 07 Apr 2006 18:03:04 +0800 | From | Yi Yang <> | Subject | Re: [2.6.16 PATCH] Filessytem Events Reporter V2 |
| |
Evgeniy Polyakov wrote: > On Fri, Apr 07, 2006 at 04:13:45PM +0800, Yi Yang (yang.y.yi@gmail.com) 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? > I didn't consider this case, thanks, I will do with it. > >>>> + } >>>> + 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. > Yes, mutex should be the best choice. > >>> 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. > No, I also use clone to send skb, so they should have the same overhead. > >>> 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. > >
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |