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

[Ben Pfaff]

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.