[ovs-dev] RFC: the meaning of LINUX_DATAPATH and HAVE_IF_DL

[Daniele Di Proietto]

Hi guys,

I'll just attach the patch (just for a quick preview, it is not ready jet)
that builds the userspace components for the kernel datapath in FreeBSD. We
provide some linux functionalities (netlink and epoll) through wrappers
(not included here) so that LINUX_DATAPATH is true. What we may need is a
better way to understand which features are present to exclude some blocks
inside LINUX_DATAPATH.

cheers

Daniele


2014/1/24 Ben Pfaff <blp at nicira.com>

> On Thu, Jan 23, 2014 at 04:04:24PM -0800, Luigi Rizzo wrote:
> > On Thu, Jan 23, 2014 at 3:39 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > > On Thu, Jan 23, 2014 at 11:51:26PM +0100, Luigi Rizzo wrote:
> > > > following the port of the OVS kernel code to FreeBSD (most of it
> > > > done by Daniele Di Proietto), the varia that control feature
> > > > inclusion may benefit from some rediscussion.
> > > >
> > > > At the moment ovs uses LINUX_DATAPATH for different purposes:
> > > >
> > > > 1. indicate that an in-kernel datapath is available
> > > > 2. indicate the availability of certain linux features
> > > >    (rtnetlink, traffic control, vlandev, stats collection)
> > > >    that interact with the in-kernel datapath.
> > > >
> > > > #1 and #2 are now decoupled (and #2 could be split even further at
> > > > some point), so in places where we want to check for #2 we added
> > > > an extra test on HAVE_IF_DL, which at the moment basically
> discriminates
> > > > between Linux and *BSD.
> > > > Case #2 occurs in openvswitch/lib/automake.mk,
> openvswitch/lib/vlandev.c
> > > > and openvswitch/vswitchd/system-stats.c whereas there are over 100
> > > > instances of LINUX_DATAPATH
> > > >
> > > > Before submitting a patch we'd like to know which of the following
> > > > sounds more suitable:
> > > >
> > > > a) use " #if defined(LINUX_DATAPATH) && !defined(HAVE_IF_DL)"
> > > >    to test for case 2.
> > > >
> > > > b) define some other indentifiers for specific features to be used in
> > > >    case 2 (say HAVE_RTNETLINK ...), same as it is done for HAVE_IF_DL
> > > >
> > > > We have (a) ready, but for readability maybe (b) would be preferable.
> > > >
> > > > And as an orthogonal issue, it may make sense to rename
> LINUX_DATAPATH
> > > > into KERNEL_DATAPATH (though it would be just for aesthetic reasons,
> > > > and i am not sure if it is worthwhile considering the number
> > > > of files and places affected)
> > >
> > > After review, I think we can just change them all from LINUX_DATAPATH
> > > to just __linux__.  I guess there are a few where you'll want to
> > > either change them to __linux__ || __FreeBSD__ when the port is
> > > available, or to something like KERNEL_DATAPATH if you prefer, but it
> > > seems like a clean way forward.
> > >
> > hmmm... I believe we should look for features, not assume
> > their presence based on OS name. so I'd rather go for
> > KERNEL_DATAPATH and HAVE_* rather than __linux__ or __FreeBSD__.
>
> Feature tests are always nice, but a lot of the stuff protected by
> LINUX_DATAPATH is really Linux specific, like the structure of /proc.
>
> > Also, our porting approach (which we found very effective in
> > many cases) remaps linux APIs into FreeBSD equivalent ones,
> > and for the most part there is a 1-1 mapping with
> > no significant performance hit at runtime.
> >
> > Using a handful of private, fine-grained HAVE_* names
> > makes the porting simpler.
> >
> > Daniele can probably send our current diff to see what
> > are the components involved.
>
> Why don't you just send along the patches and if they make sense we'll
> apply them.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140124/9f6666db/attachment-0003.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: user.patch
Type: text/x-patch
Size: 6515 bytes
Desc: not available
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140124/9f6666db/attachment-0005.bin>