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

[Jesse Gross]

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.


> +#define vort_get(dev) ((struct vport *) dev->rx_handler_data)
>

Is this actually used anywhere?


> +/* 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.


> -       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().


> +#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


>  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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100808/702212b4/attachment-0003.html>