[ovs-dev] [PATCH 1/3] datapath: Add genl_exec().

[Pravin Shelar]

On Wed, Dec 14, 2011 at 4:37 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Wed, Dec 14, 2011 at 3:47 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>> On Wed, Dec 14, 2011 at 3:34 PM, Jesse Gross <jesse at nicira.com> wrote:
>>> On Wed, Dec 14, 2011 at 3:07 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>>>> On Wed, Dec 14, 2011 at 2:47 PM, Ansis Atteka <aatteka at nicira.com> wrote:
>>>>> On Wed, Dec 14, 2011 at 2:08 PM, Pravin B Shelar <pshelar at 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().