[ovs-dev] [PATCH V2] ofproto-dpif-upcall: Remove the dispatcher thread.

[Alex Wang]

Hey Han,

I'd very much like to get a full review from you.  Are you planning to do
that?

Also want to ask about your timetable, do you think you could finish review
this week?

Appreciate your help, ;D
Alex Wang,


On Wed, Feb 12, 2014 at 6:37 AM, Zhou, Han <hzhou8 at ebay.com> wrote:

> Hi Alex,
>
> On Fri, 2014-02-07 at 17:17 -0800, Alex Wang wrote:
> > This commit removes the 'dispatcher' thread by allowing 'handler'
> > threads to read upcalls directly from dpif.  vport in dpif will
> > open netlink sockets for each handler and will use the 5-tuple
> > hash from the missed packet to choose which socket (handler) to
> > send the upcall.
>
> It is very nice to see this improvement implemented. I just started the
> review and now have a comment:
>
> > +int ovs_vport_set_upcall_pids(struct vport *vport,  struct nlattr *pids)
> > +{
> > +     struct vport_pids *old;
> > +
> > +     if (pids && nla_len(pids) % sizeof(u32))
> > +             return -EINVAL;
> > +
> > +     rcu_read_lock();
>
> The rcu_read_lock here doesn't make sense. Real lock (e.g. spin-lock)
> should be used instead if we want to protect concurrent execution of
> this code.
>
> > +     old = vport->upcall_pids ? ovsl_dereference(vport->upcall_pids)
> > +             : NULL;
> > +
> Assume that you have no spin-lock/mutex hold before calling this
> function, then two threads can dereference vport->upcall_pids, and save
> in "old".
>
> > +     if (pids) {
> > +             struct vport_pids *vport_pids;
> > +
> > +             vport_pids = kmalloc(sizeof *vport_pids, GFP_KERNEL);
> > +             vport_pids->pids = kmalloc(nla_len(pids), GFP_KERNEL);
> > +             vport_pids->n_pids = nla_len(pids)
> > +                     / (sizeof *vport_pids->pids);
> > +             memcpy(vport_pids->pids, nla_data(pids), nla_len(pids));
> > +
> > +             rcu_assign_pointer(vport->upcall_pids, vport_pids);
> > +     } else if (old) {
> > +             rcu_assign_pointer(vport->upcall_pids, NULL);
> > +     }
> > +
> > +     if (old)
> > +             call_rcu(&old->rcu, vport_pids_destroy_rcu_cb);
>
> And then both threads will call this call_rcu, leading to double free.
>
> > +
> > +     rcu_read_unlock();
> > +     return 0;
> > +}
> > +
>
> So, if updating upcall_pids need protection, we'd better use spin-lock
> here. If we are sure concurrent execution is already prevented in
> calling code, then no locks needed here.
>
> Best regards,
> Han
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140212/5d56e9ef/attachment-0005.html>