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 <b...@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 <b...@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.
>
commit b279a81995b2c0caffce0b81f1cd3097fef40e9d
Author: Daniele Di Proietto <daniele.di.proie...@gmail.com>
Date: Thu Jan 23 16:09:25 2014 +0100
Userspace modifications for FreeBSD kernel datapath
diff --git a/lib/automake.mk b/lib/automake.mk
index 94ba060..29d289f 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -272,18 +272,23 @@ if LINUX_DATAPATH
lib_libopenvswitch_la_SOURCES += \
lib/dpif-linux.c \
lib/dpif-linux.h \
- lib/netdev-linux.c \
- lib/netdev-linux.h \
lib/netlink-notifier.c \
lib/netlink-notifier.h \
lib/netlink-protocol.h \
lib/netlink-socket.c \
- lib/netlink-socket.h \
+ lib/netlink-socket.h
+
+if HAVE_IF_DL
+else
+lib_libopenvswitch_la_SOURCES += \
+ lib/netdev-linux.c \
+ lib/netdev-linux.h \
lib/rtnetlink-link.c \
lib/rtnetlink-link.h \
lib/route-table.c \
lib/route-table.h
endif
+endif
if HAVE_POSIX_AIO
lib_libopenvswitch_la_SOURCES += lib/async-append-aio.c
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 933c872..92b158c 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -530,7 +530,9 @@ dpif_linux_port_add__(struct dpif *dpif_, struct netdev *netdev,
request.name = name;
if (request.type == OVS_VPORT_TYPE_NETDEV) {
+#ifndef HAVE_IF_DL
netdev_linux_ethtool_set_flag(netdev, ETH_FLAG_LRO, "LRO", false);
+#endif
}
tnl_cfg = netdev_get_tunnel_config(netdev);
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 97291bd..d2460fc 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -292,9 +292,23 @@ netdev_bsd_construct_system(struct netdev *netdev_)
/* Verify that the netdev really exists by attempting to read its flags */
error = netdev_get_flags(netdev_, &flags);
if (error == ENXIO) {
+#ifdef LINUX_DATAPATH
+ if (netdev->up.netdev_class != &netdev_internal_class) {
+ /* The device does not exist, so don't allow it to be opened. */
+ free(netdev->kernel_name);
+ cache_notifier_unref();
+ return error;
+ } else {
+ /* "Internal" netdevs have to be created as netdev objects before
+ * they exist in the kernel, because creating them in the kernel
+ * happens by passing a netdev object to dpif_port_add().
+ * Therefore, ignore the error. */
+ }
+#else
free(netdev->kernel_name);
cache_notifier_unref();
return error;
+#endif
}
return 0;
@@ -849,6 +863,12 @@ netdev_bsd_get_carrier(const struct netdev *netdev_, bool *carrier)
if ((ifmr.ifm_status & IFM_AVALID) == 0) {
netdev->carrier = true;
}
+ } else if (error == EINVAL) {
+ /* FreeBSD tap (and kernel datapath) return always EINVAL.
+ * Assume active. */
+
+ netdev->carrier = true;
+ netdev->cache_valid |= VALID_CARRIER;
} else {
VLOG_DBG_RL(&rl, "%s: ioctl(SIOCGIFMEDIA) failed: %s",
netdev_get_name(netdev_), ovs_strerror(error));
@@ -1539,6 +1559,16 @@ const struct netdev_class netdev_tap_class =
"tap",
netdev_bsd_construct_tap,
netdev_bsd_get_features);
+
+#ifdef LINUX_DATAPATH
+/* TODO get_stats and set stats could be implemented
+ * with minimal effort */
+const struct netdev_class netdev_internal_class =
+ NETDEV_BSD_CLASS(
+ "internal",
+ netdev_bsd_construct_system,
+ NULL);
+#endif
static void
diff --git a/lib/netdev.c b/lib/netdev.c
index 8e62421..1f31b0d 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -101,14 +101,15 @@ netdev_initialize(void)
fatal_signal_add_hook(restore_all_flags, NULL, NULL, true);
netdev_vport_patch_register();
+ netdev_register_provider(&netdev_tap_class);
#ifdef LINUX_DATAPATH
+#ifndef HAVE_IF_DL
netdev_register_provider(&netdev_linux_class);
+#endif
netdev_register_provider(&netdev_internal_class);
- netdev_register_provider(&netdev_tap_class);
netdev_vport_tunnel_register();
#endif
#if defined(__FreeBSD__) || defined(__NetBSD__)
- netdev_register_provider(&netdev_tap_class);
netdev_register_provider(&netdev_bsd_class);
#endif
diff --git a/lib/vlandev.c b/lib/vlandev.c
index b793f77..86102df 100644
--- a/lib/vlandev.c
+++ b/lib/vlandev.c
@@ -38,7 +38,7 @@ struct vlandev_class {
int (*vd_del)(const char *vlan_dev);
};
-#ifdef LINUX_DATAPATH
+#if defined(LINUX_DATAPATH) && !defined(HAVE_IF_DL)
static const struct vlandev_class vlandev_linux_class;
#endif
static const struct vlandev_class vlandev_stub_class;
@@ -61,7 +61,7 @@ static const struct vlandev_class *
vlandev_get_class(void)
{
if (!vd_class) {
-#ifdef LINUX_DATAPATH
+#if defined(LINUX_DATAPATH) && !defined(HAVE_IF_DL)
vd_class = &vlandev_linux_class;
#else
vd_class = &vlandev_stub_class;
@@ -161,7 +161,7 @@ vlandev_get_name(const char *real_dev_name, int vid)
/* The Linux vlandev implementation. */
-#ifdef LINUX_DATAPATH
+#if defined(LINUX_DATAPATH) && !defined(HAVE_IF_DL)
#include "rtnetlink-link.h"
#include <linux/if_vlan.h>
#include <linux/sockios.h>
diff --git a/utilities/automake.mk b/utilities/automake.mk
index ffc48b1..9bcc05d 100644
--- a/utilities/automake.mk
+++ b/utilities/automake.mk
@@ -112,6 +112,8 @@ utilities_ovs_vsctl_SOURCES = utilities/ovs-vsctl.c
utilities_ovs_vsctl_LDADD = lib/libopenvswitch.la $(SSL_LIBS)
if LINUX_DATAPATH
+if HAVE_IF_DL
+else
sbin_PROGRAMS += utilities/ovs-vlan-bug-workaround
utilities_ovs_vlan_bug_workaround_SOURCES = utilities/ovs-vlan-bug-workaround.c
utilities_ovs_vlan_bug_workaround_LDADD = lib/libopenvswitch.la $(SSL_LIBS)
@@ -120,6 +122,7 @@ noinst_PROGRAMS += utilities/nlmon
utilities_nlmon_SOURCES = utilities/nlmon.c
utilities_nlmon_LDADD = lib/libopenvswitch.la $(SSL_LIBS)
endif
+endif
bin_PROGRAMS += utilities/ovs-benchmark
utilities_ovs_benchmark_SOURCES = utilities/ovs-benchmark.c
diff --git a/vswitchd/system-stats.c b/vswitchd/system-stats.c
index 1d9cb78..392f660 100644
--- a/vswitchd/system-stats.c
+++ b/vswitchd/system-stats.c
@@ -50,7 +50,7 @@ VLOG_DEFINE_THIS_MODULE(system_stats);
* Thus, this file tries to compile as much of the code as possible regardless
* of the target, by writing "if (LINUX_DATAPATH)" instead of "#ifdef
* __linux__" where this is possible. */
-#ifdef LINUX_DATAPATH
+#if defined(LINUX_DATAPATH) && !defined(HAVE_IF_DL)
#include <asm/param.h>
#else
#define LINUX_DATAPATH 0
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
