On Wed, Dec 14, 2011 at 5:16 PM, Jesse Gross <je...@nicira.com> wrote:
> On Wed, Dec 14, 2011 at 4:48 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>> On Wed, Dec 14, 2011 at 4:37 PM, Jesse Gross <je...@nicira.com> wrote:
>>> On Wed, Dec 14, 2011 at 3:47 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>>>> On Wed, Dec 14, 2011 at 3:34 PM, Jesse Gross <je...@nicira.com> wrote:
>>>>> On Wed, Dec 14, 2011 at 3:07 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>>>>>> On Wed, Dec 14, 2011 at 2:47 PM, Ansis Atteka <aatt...@nicira.com> wrote:
>>>>>>> On Wed, Dec 14, 2011 at 2:08 PM, Pravin B Shelar <pshe...@nicira.com>
>>>>>>> wrote:
>>>>>>>> +int genl_exec(genl_exec_func_t func, void *data)
>>>>>>>> +{
>>>>>>>> + struct sk_buff *skb;
>>>>>>>> +
>>>>>>>> + skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>>>>>>> + if (!skb)
>>>>>>>> + return -ENOMEM;
>>>>>>>> +
>>>>>>>> + genlmsg_put(skb, 0, 0, &genl_exec_family, NLM_F_REQUEST,
>>>>>>>> GENL_EXEC_RUN);
>>>>>>>> +
>>>>>>>> + mutex_lock(&genl_exec_lock);
>>>>>>>> + genl_exec_function = func;
>>>>>>>> + genl_exec_data = data;
>>>>>>>> + init_completion(&done);
>>>>>>>> + genlmsg_unicast(&init_net, skb, 0);
>>>>>>>> + wait_for_completion(&done);
>>>>>>>> + genl_exec_function = NULL;
>>>>>>>
>>>>>>> Shouldn't you prepare here a copy of genl_exec_function_ret before
>>>>>>> actually
>>>>>>> returning it? I sense a small-probability of a race condition here.
>>>>>>>>
>>>>>> right, I need to do it inside lock. Even with lock it is not
>>>>>> completely safe as callback is not taking any lock. But I think that
>>>>>> is not issue for now.
>>>>>
>>>>> Isn't all of this executing synchronously anyways, so the callback
>>>>> actually is protected by genl_exec_lock and the completion stuff isn't
>>>>> necessary?
>>>> Its not same across kernel that we care.
>>>> callbacks are executed synchronously from 2.6.24 onward.
>>>
>>> Are you sure? I think it just takes a slightly more convoluted path
>>> to get there. If doesn't execute in this context, what context does
>>> it execute in?
>>>
>>> On 2.6.18, for example, I think this is the call stack:
>>> - genl_exec
>>> - genlmsg_unicast
>>> - nlmsg_unicast
>>> - netlink_unicast
>>> - netlink_sendskb (skb gets enqueued to socket buffer)
>>> - netlink_data_ready AKA sk->sk_data_ready
>>> - genl_rcv AKA nlk->data_ready
>>> - netlink_run_queue (skb gets dequeued from socket buffer)
>>> - netlink_rcv_skb
>>> - genl_rcv_mesg
>>> - genl_exec_cmd AKA doit
>>>
>>> So there's a lot of useless enqueueing, dequeueing, waking up
>>> processes, etc. that are really only needed when sending data to
>>> userspace. That was shortcut in later kernels but it doesn't really
>>> change anything.
>>
>> right, but there is genl_trylock() called in genl_rcv().
>
> Hmm, I guess you're right. Getting back to Ansis's original question,
> assuming that you move retrieving the return value into the lock which
> part were you saying isn't completely safe?
GENL_EXEC_RUN msg can be send from other function without taking
genl_exec_lock. But we can always enforce locking, so it is not real
issue. Never mind. It is just weird that sender is taking lock for rcv
function.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
