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?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
