[ovs-dev] [patch 4/5] datapath: use rx_handler_data pointer

[Simon Horman]

On Sun, Aug 08, 2010 at 05:50:00PM -0400, Jesse Gross wrote:
> On Fri, Aug 6, 2010 at 11:05 AM, Simon Horman <horms at verge.net.au> wrote:
> 
> > +#ifndef HAVE_BR_PORT
> > +#define vport_get_rcu(dev) \
> > +        ((struct vport *) rcu_dereference(dev->rx_handler_data))
> >
> 
> I would just drop this into netdev_get_vport() and call that.

Done

> > +#define vort_get(dev) ((struct vport *) dev->rx_handler_data)
> >
> 
> Is this actually used anywhere?

Given that its misspelt, no.

> > +/* XXX: Add IFF_OVS_DATAPATH to include/linux/if.h and use it
> > + *     in place of IFF_VPORT in this file. */
> >
> 
> We could could do this with compatibility code: use the desired name in this
> file and then define it to the appropriate value in
> compat/include/linux/if.h.  On pre-2.6.36 kernels it could be defined to 0,
> defined to IFF_BRIDGE_PORT for later kernels, and then we wouldn't need to
> define it all at once we make it upstream.  This would remove a bunch of
> #ifdef's from this file.

I think we can just unconditionally define it to IFF_BRIDGE_PORT,
it will be unused with older kernels. It can then get a unique value
after the merge.

I don't think that this can remove any #ifdefs from the code
as "netdev_vport->dev->priv_flags |= IFF_OVS_DATAPATH" and similar
would still be incompatible with older kernels.

However, I have added some compat #defines to centralise most
of the #ifdef magic.

> > -       if (netdev_vport->dev->br_port) {
> > +#ifdef HAVE_BR_PORT
> > +       if (netdev_vport->dev->br_port)
> > +#else
> > +       if (netdev_vport->dev->priv_flags & IFF_BRIDGE_PORT)
> > +#endif
> > +       {
> >                err = -EBUSY;
> >                goto error_put;
> >        }
> >
> 
> I believe that we actually don't need this block of code here anymore.  On
> older kernel versions it should really be moved to netdev_attach() and on
> newer versions the check is handled implicitly by
> netdev_rx_handler_register().

Great, I'll just move the check into the compat code that I added
for older kernels as part of the revised "[patch 1/5] datapath: dont use
non-existent receive hooks".

> > +#ifdef HAVE_BR_PORT
> >        rcu_assign_pointer(netdev_vport->dev->br_port,
> >                           (struct net_bridge_port *)vport);
> > +       vport = NULL;
> > +#endif
> >  #ifndef HAVE_BR_HANDLE_FRAME_HOOK
> >        err = netdev_rx_handler_register(netdev_vport->dev,
> > -                                        netdev_frame_hook, NULL);
> > +                                        netdev_frame_hook, vport);
> > +       if (err)
> > +               return err;
> >  #endif
> > -       return err;
> > +#ifndef HAVE_BR_PORT
> > +       netdev_vport->dev->priv_flags |= IFF_BRIDGE_PORT;
> > +#endif
> >
> 
> It looks like you are trying to handle the various possibilities for commit
> level changes.  We'll just go crazy if we try to do that and it makes the
> code much harder to read.  It's much cleaner if we do:
> 
> #if < 2.6.36
> assign to dev->br_port
> #else
> use netdev_rx_register_handler()
> #endif

Agreed. I have also refactored the relevant portions of other patches
that touch this code.

> >  struct vport *netdev_get_vport(struct net_device *dev)
> >  {
> > +#ifdef HAVE_BR_PORT
> >        return (struct vport *)dev->br_port;
> >
> 
> If we make this a more general function we should probably add an
> rcu_dereference() here for consistency even if it isn't strictly required.

I'd rather handle that as a separate change. To make bisection of any
breakage more obvious. I'll make a patch.

> There's also a block of code at the bottom of the file that is a hack to
> enforce mutual exclusion with the bridge.  Now that we can safely coexist
> with the bridge, it can be #if'd out on appropriate kernels.

Done. Although registration of the datapath will fail if the bridge is
present and vice versa because IFF_OVS_DATAPATH has the same value
as IFF_BRIDGE_PORT (for now).