lkml.org 
[lkml]   [2006]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2006-04-07 12:04    [from the cache]
©2003-2011 Jasper Spaans