[ovs-dev] [netlink 03/16] datapath: Merge "struct dp_port" into "struct vport".

Ben Pfaff blp at nicira.com
Fri Nov 5 20:02:43 UTC 2010


On Fri, Nov 05, 2010 at 12:01:07PM -0700, Jesse Gross wrote:
> On Fri, Nov 5, 2010 at 11:28 AM, Ben Pfaff <blp at nicira.com> wrote:
> > On Wed, Nov 03, 2010 at 04:43:08PM -0700, Ben Pfaff wrote:
> >> On Thu, Sep 23, 2010 at 07:11:43PM -0700, Jesse Gross wrote:
> >> > On Fri, Sep 10, 2010 at 3:55 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > > diff --git a/datapath/vport.c b/datapath/vport.c
> >> > > index 2ae0053..2363802 100644
> >> > > @@ -47,13 +47,12 @@ static struct hlist_head *dev_table;
> >> > >  * one of these locks if you don't want the vport to be deleted out from under
> >> > >  * you.
> >> > >  *
> >> > > - * If you get a reference to a vport through a dp_port, it is protected
> >> > > + * If you get a reference to a vport through a datapath, it is protected
> >> > >  * by RCU and you need to hold rcu_read_lock instead when reading.
> >> > >  *
> >> > >  * If multiple locks are taken, the hierarchy is:
> >> > >  * 1. RTNL
> >> > > - * 2. DP
> >> > > - * 3. vport
> >> > > + * 2. vport
> >> > >  */
> >> > >  static DEFINE_MUTEX(vport_mutex);
> >> >
> >> > It would be nice if we can simply this a little bit.  vport_mutex was
> >> > introduced because vports existed outside the context of a particular
> >> > datapath and therefore it wasn't possible to rely on dp_mutex to
> >> > protect them from disappearing.
> >
> > Hmm, vport_mutex is almost completely redundant with rtnl_lock now.  If
> > I insert rtnl_lock/rtnl_unlock pairs into query_port(),
> > vport_user_stats_get(), vport_user_ether_get(), and
> > vport_user_mtu_get(), then I can drop vport_mutex entirely.
> >
> > This seems reasonable to me so I've added patches to that effect to the
> > series.
> 
> One of the purposes of vport_mutex was actually to avoid doing exactly
> this.  All of these are read only functions, which is why they don't
> need RTNL.  I don't see a good justification for extending the use of
> RTNL to cover our private data structures, it already covers a huge
> number of things throughout the kernel.  This should really use
> dp_mutex instead.

Now that I look again, I don't see an obvious reason why those four uses
couldn't be protected just by rcu_read_lock().  Do you know of a reason?




More information about the dev mailing list