Thanks Han for the review, ;D
I misunderstood how rcu works.
The access of "vport->upcall_pids" is a "single writer, multiple readers"
problem.
And it has already been protected by the 'ovs_lock()' in callers.
I'll remove the rcu read critical section in the V3 patch.
Alex Wang,
On Wed, Feb 12, 2014 at 6:37 AM, Zhou, Han <hzh...@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
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
