Re: [ovs-dev] [PATCH] stuck with 16-bit ifr_flags in FreeBSD

[Ed Maste]

On 3 April 2015 at 13:46, Ben Pfaff  wrote:
>
>> > Also, the commit you name was in 2013, and we've had other
>> > contributions from FreeBSD contributors since then
>> > (namely Ed Maste ) and it seems like he
>> > would have noticed if it were totally busted.
>>
>> You mean this message:
>> http://openvswitch.org/pipermail/dev/2013-May/027594.html
>>
>> If so, Ed replied that he reviewed the patch but didn't apply it to test. :-)
>
> Ed is the author of the following 37 commits in the Open vSwitch
> repository.  Ed, did you really never test Open vSwitch on FreeBSD,
> despite this long list of contributions?

The discussion above happened around the time I was moving away from
using Open vSwitch in near-production on a daily basis on FreeBSD. I
reviewed, but did not test, the change -- and clearly that review was
insufficient. Prior to that time I was building, testing, and using it
in a near-production environment on an ongoing basis.

I know others are using Open vSwitch on FreeBSD in various capacities
as well, such as Prof. Luigi Rizzo's team. I'm surprised that nobody
spotted this until now.

The offending flag that ends up in the sign bit is:
#define IFF_MULTICAST   0x8000  /* (i) supports multicast */
so will certainly be set in many cases. Kevin, what failure mode did
you observe?

Note that we have Open vSwitch in the FreeBSD ports tree, and it's a
relatively old version from around that time. As a result, FreeBSD
users installing from ports wouldn't have encountered this issue (but
would have to deal with an outdated version).

Kevin's patch looks correct and I'm happy that he'll be updating the
version in the FreeBSD ports tree as well.

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] stuck with 16-bit ifr_flags in FreeBSD

[Ed Maste]

On 4 April 2015 at 12:56, Kevin Lo  wrote:
>> Kevin, what failure mode did you observe?
>
> On FreeBSD, multicast is enabled on the interface by default, the value of
> ifr_flags could be negative if multicast is enabled.

Right, but what actually happens in this case? The sign extension bug
means it will appear as if the flags in ifr_flagshigh are set (and
perhaps those flags will be written back to the kernel, too). But I
could see certain use cases being unaffected by the bug.

To rephrase my original question perhaps: how did you discover the
bug? It doesn't matter in terms of the correctness of your fix; I'm
just curious about the fact that we didn't discover it for so long.

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH V2 09/10] dpif-netdev: Add DPDK netdev.

[Ed Maste]

On 21 March 2014 14:03, Pravin  wrote:
> Following patch adds DPDK netdev-class to userspace datapath. Now
> OVS can use DPDK port for IO by just configuring DPDK port and then
> adding dpdk type port to userspace datapath.

Intel recently released DPDK also for FreeBSD, so it may be worth
referring explicitly to Linux DPDK in INSTALL.DPDK (until we can port
/ test it on FreeBSD).

Most of the existing DPDK docs also assume Linux, but I assume this
will be updated over time.  The FreeBSD DPDK getting started guide is
here:
http://www.intel.com/content/www/us/en/intelligent-systems/intel-technology/intel-dpdk-freebsd-getting-started-guide.html

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] AUTHORS: use actual address for ema...@freebsd.org

[Ed Maste]

Signed-off-by: Ed Maste 
---
I don't recall how I ended up with this naive anti-spam obfuscation in
AUTHORS. In any case there's no reason to have one inconsistent entry
here.

 AUTHORS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/AUTHORS b/AUTHORS
index f4e1ca9..8a3f861 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -55,7 +55,7 @@ Dmitry Krivenok krivenok.dmi...@gmail.com
 Dominic Curran  dominic.cur...@citrix.com
 Dongdongdongdo...@huawei.com
 Duffie Cooley   dcoo...@nicira.com
-Ed Masteemaste at freebsd.org
+Ed Masteema...@freebsd.org
 Ed Swierk   eswi...@skyportsystems.com
 Edouard Bourguignon ma...@linuxed.net
 Edward Tomasz Napierała tr...@freebsd.org
-- 
2.4.6

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev: Add NULL get_tunnel_config for BSD.

[Ed Maste]

Commit f431bf7d78f3212d32bb3d122f783c5c796a1576 added new method
get_tunnel_config to netdev_class but did not update netdev-bsd.

Signed-off-by: Ed Maste 
---
 lib/netdev-bsd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 9094d04..538f91a 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -1255,6 +1255,7 @@ const struct netdev_class netdev_bsd_class = {
 netdev_bsd_destroy,
 NULL, /* get_config */
 NULL, /* set_config */
+NULL, /* get_tunnel_config */
 netdev_bsd_open_system,
 netdev_bsd_close,
 
@@ -1315,6 +1316,7 @@ const struct netdev_class netdev_tap_class = {
 netdev_bsd_destroy,
 NULL, /* get_config */
 NULL, /* set_config */
+NULL, /* get_tunnel_config */
 netdev_bsd_open_system,
 netdev_bsd_close,
 
-- 
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [patch_ports (rebased) 3/4] ofproto-dpif: Implement patch ports in userspace.

[Ed Maste]

On 23 January 2013 15:51, Ethan Jackson  wrote:
> diff --git a/FAQ b/FAQ
> index ab1c1cc..a466ca4 100644
> --- a/FAQ
> +++ b/FAQ
> @@ -172,17 +172,24 @@ A: The kernel module in upstream Linux 3.3 and later 
> does not include
>   vSwitch distribution instead of the upstream Linux kernel
>   module.
>
> -   - Patch virtual ports, that is, interfaces with type "patch".
> - You can use Linux "veth" devices as a substitute.
> -
> - We don't have any plans to add patch ports upstream.
> -
>  Q: What features are not available when using the userspace datapath?
>
> -A: Tunnel and patch virtual ports are not supported, as described in the
> -   previous answer.  It is also not possible to use queue-related
> -   actions.  On Linux kernels before 2.6.39, maximum-sized VLAN packets
> -   may not be transmitted.
> +A: The kernel module in upstream Linux 3.3 and later does not include
> +   tunnel virtual ports, that is, interfaces with type "gre",
> +   "ipsec_gre", "gre64", "ipsec_gre64", "vxlan", or "capwap".  It is
> +   possible to create tunnels in Linux and attach them to Open vSwitch
> +   as system devices.  However, they cannot be dynamically created
> +   through the OVSDB protocol or set the tunnel ids as a flow action.
...

This new answer seems to be exclusively about Linux kernel modules,
which seems surprising for a question about the userspace datapath.
On FreeBSD (which uses only the userspace datapath) the new answer has
no meaning.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [patch_ports (rebased) 3/4] ofproto-dpif: Implement patch ports in userspace.

[Ed Maste]

On 23 January 2013 18:40, Ben Pfaff  wrote:
> I think something got mixed up.  Here's the change that I expected to
> see here:
>
> --
> diff --git a/FAQ b/FAQ
> index ab1c1cc..bd4d37e 100644
> --- a/FAQ
> +++ b/FAQ
> @@ -158,28 +158,25 @@ Q: What features are not available in the Open vSwitch 
> kernel datapath
> that ships as part of the upstream Linux kernel?
>
>  A: The kernel module in upstream Linux 3.3 and later does not include
> -   the following features:
> -
> -   - Tunnel virtual ports, that is, interfaces with type "gre",
> - "ipsec_gre", "capwap".  It is possible to create tunnels in
> - Linux and attach them to Open vSwitch as system devices.
> - However, they cannot be dynamically created through the OVSDB
> - protocol or set the tunnel ids as a flow action.
> -
> - Work is in progress in adding these features to the upstream
> - Linux version of the Open vSwitch kernel module.  For now, if
> - you need these features, use the kernel module from the Open
> - vSwitch distribution instead of the upstream Linux kernel
> - module.
> -
> -   - Patch virtual ports, that is, interfaces with type "patch".
> - You can use Linux "veth" devices as a substitute.
> -
> - We don't have any plans to add patch ports upstream.
> +   tunnel virtual ports, that is, interfaces with type "gre",
> +   "ipsec_gre", "gre64", "ipsec_gre64", "vxlan", or "capwap".  It is
> +   possible to create tunnels in Linux and attach them to Open vSwitch
> +   as system devices.  However, they cannot be dynamically created
> +   through the OVSDB protocol or set the tunnel ids as a flow action.
> +
> +   Work is in progress in adding tunnel virtual ports to the upstream
> +   Linux version of the Open vSwitch kernel module.  For now, if you
> +   need these features, use the kernel module from the Open vSwitch
> +   distribution instead of the upstream Linux kernel module.
> +
> +   The upstream kernel module does not include patch ports, but this
> +   only matters for Open vSwitch 1.9 and earlier, because Open vSwitch
> +   1.10 and later implement patch ports without using this kernel
> +   feature.
>
>  Q: What features are not available when using the userspace datapath?
>
> -A: Tunnel and patch virtual ports are not supported, as described in the
> +A: Tunnel virtual ports are not supported, as described in the
> previous answer.  It is also not possible to use queue-related
> actions.  On Linux kernels before 2.6.39, maximum-sized VLAN packets
> may not be transmitted.

Aha, that makes much more sense, thanks.

> I can believe that we still need clarification here (and elsewhere?)
> in the FAQ to make sure that everything applies properly to FreeBSD.
> I'd welcome improvements.

I'll see if I can come up with a suitable patch for the FAQ.  I went
over INSTALL in the course of getting the FreeBSD patch ready but
didn't really spend any time on the FAQ.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [tunnel 00/11] Flow based tunneling.

[Ed Maste]

On 27 January 2013 19:58, Ethan Jackson  wrote:
> This version of the series implements enough of the flow based
> tunneling that we can begin testing.  There are still some things
> which need to be done (described in ofproto/tunnel.c) which we will
> need to complete before making a release.

As expected, this set fixes the build on FreeBSD for me.

A number of new tests fail though:

  3: lacp - negotiation  FAILED (lacp.at:160)
471: tunnel - input  FAILED (tunnel.at:17)
472: tunnel - ECN decapsulation  FAILED (tunnel.at:81)
473: tunnel - output FAILED (tunnel.at:109)
474: tunnel - ToS and TTL inheritanceFAILED (tunnel.at:144)
475: tunnel - set_tunnel FAILED (tunnel.at:189)
476: tunnel - keyFAILED (tunnel.at:224)
477: tunnel - key match  FAILED (tunnel.at:279)
587: ofproto-dpif - patch ports  FAILED
(ofproto-dpif.at:1329)

af37354 is the last version I can build, and it passes all tests.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [tunnel 00/11] Flow based tunneling.

[Ed Maste]

On 28 January 2013 12:44, Ben Pfaff  wrote:
> On Mon, Jan 28, 2013 at 10:43:23AM -0500, Ed Maste wrote:
>> On 27 January 2013 19:58, Ethan Jackson  wrote:
>> > This version of the series implements enough of the flow based
>> > tunneling that we can begin testing.  There are still some things
>> > which need to be done (described in ofproto/tunnel.c) which we will
>> > need to complete before making a release.
>>
>> As expected, this set fixes the build on FreeBSD for me.
>>
>> A number of new tests fail though:
>
> Will you pass along testsuite.log?

Sure - it's larger than I'd like to attach, so I've made it available
at http://people.freebsd.org/~emaste/testsuite.log
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [tunnel 07/11] netdev: New function netdev_get_dpif_port().

[Ed Maste]

On 27 January 2013 19:58, Ethan Jackson  wrote:
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index f0f1dc2..a06aef3 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2430,6 +2430,7 @@ netdev_linux_change_seq(const struct netdev *netdev)
>  \
>  CREATE, \
>  netdev_linux_destroy,   \
> +NULL,   /* get_dpif_port */ \
>  NULL,   /* get_config */\
>  NULL,   /* set_config */\
>  NULL,   /* get_tunnel_config */ \

Can you fold in the equivalent change to netdev-bsd.c?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [tunnel 00/11] Flow based tunneling.

[Ed Maste]

On 29 January 2013 00:12, Ben Pfaff  wrote:
> It looks like Ethan was able to fix the problems--he posted a new
> version of patch 11 that says it fixes FreeBSD test failures.

I didn't receive the new version of patch 11 (maybe Gmail decided it
was a duplicate), but I grabbed it from the archive and applied the
change.  Thanks Ethan, it does fix the unit tests for me.

Is there a reason to put the netdev_vport_(tunnel_)register in
netdev-dummy.c for FreeBSD, instead of calling it always from
lib/netdev.c?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 01/18] fix an obvious mistake in a test of IFM_ETHER

[Ed Maste]

On 31 January 2013 05:49, YAMAMOTO Takashi  wrote:
> From: YAMAMOTO Takashi 
>
> -if (!IFM_TYPE(ifmr.ifm_current) & IFM_ETHER) {
> +if (!(IFM_TYPE(ifmr.ifm_current) & IFM_ETHER)) {
>  VLOG_DBG_RL(&rl, "%s: doesn't appear to be ethernet",
>  netdev_get_name(netdev));

Should probably be IFM_TYPE(ifmr.ifm_current) != IFM_ETHER instead as
IFM_TYPE masks the network type field.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 04/18] use ax_check_openssl.m4 instead of a direct use of pkg-config.

[Ed Maste]

On 31 January 2013 05:49, YAMAMOTO Takashi  wrote:
> From: YAMAMOTO Takashi 
>
> on some platforms, including NetBSD-6, openssl is available but
> its pkg-config files (*.pc) are not.

FreeBSD as well (this is a common issue in the FreeBSD ports tree).  Thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofpbuf: Remove OFPBUF_STACK_BUFFER.

[Ed Maste]

On 27 February 2013 20:03, Ben Pfaff  wrote:
> Its alleged convenience just doesn't outweigh the syntactical ugliness, and
> so it didn't have any users.

Not entirely true: there's a consumer in lib/netdev-bsd.c.  I don't
have any time right now to look at it, but will try if nobody gets to
it first.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofpbuf: Remove OFPBUF_STACK_BUFFER.

[Ed Maste]

On 8 March 2013 16:06, Ed Maste  wrote:
> On 27 February 2013 20:03, Ben Pfaff  wrote:
>> Its alleged convenience just doesn't outweigh the syntactical ugliness, and
>> so it didn't have any users.
>
> Not entirely true: there's a consumer in lib/netdev-bsd.c.  I don't
> have any time right now to look at it, but will try if nobody gets to
> it first.

Oh, sorry; I'm mistaken.  The consumer is in my local patch set (the
threading changes).
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] INSTALL: Admit that only GNU make works.

[Ed Maste]

On 15 March 2013 17:30, Ben Pfaff  wrote:

> I promised some time ago to take a look at fixing the behavior of the
> Makefiles with non-GNU make, but it doesn't realistically seem that I will.
>

Thanks.  Even if we addressed the current issues I wouldn't be surprised to
find new failures appearing in the future, so agree that just being
explicit that GNU make is required is best.

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 04/22] use ax_check_openssl.m4 instead of a direct use of pkg-config.

[Ed Maste]

On 22 April 2013 09:20, YAMAMOTO Takashi  wrote:
> on some platforms, including NetBSD-6, openssl is available but
> its pkg-config files (*.pc) are not.  according to Ed Maste,
> this is the case for FreeBSD as well.

That's correct.  This should be a nice convenience for anyone building
on FreeBSD from a git checkout.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 15/22] add minimal NetBSD support

[Ed Maste]

> Ed, do you want to look over any of these changes?  They affect code
> for which you are a primary author, so if you would prefer to see
> anything done a different way, etc., for maintainability purposes then
> it would be valuable to get that in review.

Yep - I'm looking at them now.  I've reviewed and unit-tested the ones
you've committed so far and will review patches 15 through 22.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 17/22] netdev-bsd: NetBSD: keep a AF_LINK socket open

[Ed Maste]

On 22 April 2013 09:20, YAMAMOTO Takashi  wrote:
> this will be used for get_stats and set_etheraddr
>
> Signed-off-by: YAMAMOTO Takashi 
> ---
>  lib/netdev-bsd.c | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index ba351f5..e63ac3f 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -119,6 +119,11 @@ enum {
>  /* An AF_INET socket (used for ioctl operations). */
>  static int af_inet_sock = -1;
>
> +#if defined(__NetBSD__)
> +/* AF_LINK socket used for netdev_bsd_get_stats and set_etheraddr */
> +static int af_link_sock = -1;
> +#endif /* defined(__NetBSD__) */
> +
>  #define PCAP_SNAPLEN 2048
>
>
> @@ -185,9 +190,17 @@ netdev_bsd_init(void)
>
>  af_inet_sock = socket(AF_INET, SOCK_DGRAM, 0);
>  status = af_inet_sock >= 0 ? 0 : errno;
> -
>  if (status) {
>  VLOG_ERR("failed to create inet socket: %s", strerror(status));
> +return status;
> +}
> +
> +af_link_sock = socket(AF_LINK, SOCK_DGRAM, 0);

Missing #if defined(__NetBSD__) guard here.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 20/22] some NetBSD related documentation changes

[Ed Maste]

On 22 April 2013 09:20, YAMAMOTO Takashi  wrote:
>
> Signed-off-by: YAMAMOTO Takashi 

Acked-by: Ed Maste 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 22/22] acinclude.m4: update a comment for NetBSD support

[Ed Maste]

On 22 April 2013 09:20, YAMAMOTO Takashi  wrote:
>
> Signed-off-by: YAMAMOTO Takashi 

Acked-by: Ed Maste 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 15/22] add minimal NetBSD support

[Ed Maste]

Yamamoto-san,

Thanks for this work.  I'm happy that it will result in BSD systems as
a group getting more OVS test coverage.

The patch set builds and passes unit tests for me on FreeBSD, with the
exception of the missing #if defined() guard described in a previous
email.

I have only one overall comment: I personally don't like the style of
platform-specific #if #elif cases within the function definition,
where none of the body is common to the implementations.  I'll comment
on the two cases (netdev_bsd_get_stats and set_etheraddr) in a reply
to the specific patch that introduces them.  That said, I don't really
have a better idea here, since I don't think the differences (yet)
justify creating a netdev-freebsd.c and netdev-netbsd.c.  Perhaps just
go with the current approach unless and until another BSD port comes
along.

Ben, any comment on this, with respect to general OVS coding style?


Some specific comments:

> -#ifndef __FreeBSD__
> -/* On FreeBSD we #define this to setproctitle. */
> +#if !(defined(__FreeBSD__) || defined(__NetBSD__))
> +/* On FreeBSD and NetBSD we #define this to setproctitle. */

Perhaps use "On these platforms..." so the comment doesn't just repeat
the same condition one line above.  It will also remain true if
another BSD port comes along.

>  static uint32_t
> @@ -1202,7 +1221,9 @@ nd_to_iff_flags(enum netdev_flags nd)
>  }
>  if (nd & NETDEV_PROMISC) {
>  iff |= IFF_PROMISC;
> +#if defined(IFF_PPROMISC)
>  iff |= IFF_PPROMISC;
> +#endif

On NetBSD do you require the user to manually set promisc on the
interface at the moment?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 16/22] keep "kernel name" for each netdev

[Ed Maste]

On 22 April 2013 09:20, YAMAMOTO Takashi  wrote:
> where interface renaming is not supported (NetBSD), remember both of
> our netdev name and the correspoinding kernel name separately.
> the latter is necessary to talk with kernel using interface names.
> eg. ifioctls, bpf

Acked-by: Ed Maste 

Although it's not needed on FreeBSD, I definitely prefer this approach
to additional #ifdef blocks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 19/22] implement get_stats for NetBSD

[Ed Maste]

On 22 April 2013 09:20, YAMAMOTO Takashi  wrote:
>
> Signed-off-by: YAMAMOTO Takashi 
> ---
>  lib/netdev-bsd.c | 47 +++
>  1 file changed, 43 insertions(+), 4 deletions(-)
>
...
> +#elif defined(__NetBSD__)
> +struct netdev_dev_bsd *netdev_dev =
> +netdev_dev_bsd_cast(netdev_get_dev(netdev_));
> +struct ifdatareq ifdr;
> +struct if_data *ifd;
> +int saved_errno;
> +int ret;
> +
> +memset(&ifdr, 0, sizeof(ifdr));
> +strncpy(ifdr.ifdr_name, netdev_dev->kernel_name, sizeof(ifdr.ifdr_name));
> +ret = ioctl(af_link_sock, SIOCGIFDATA, &ifdr);
> +saved_errno = errno;
> +if (ret == -1) {
> +return saved_errno;
> +}
> +ifd = &ifdr.ifdr_data;

I'd like to propose that we factor out the common code to update each
stats member, in order to eliminate the almost-but-not-quite identical
code.  For the FreeBSD case struct if_data *ifd can just be set to
&ifmd.ifmd_data. (As an aside, it looks like I should consider adding
SIOCGIFDATA to FreeBSD -- OpenBSD, NetBSD, and DragonFly all have it.)

> +/*
> + * note: UINT64_MAX means unsupported
> + */

Is this checked anywhere (e.g., to avoid displaying unsupported
stats)?  Or will it result in the display of bogus values?

> +stats->multicast = ifd->ifi_imcasts;

Does Linux count in & out multicast frames in the same counter?
Perhaps this should be imcasts + omcasts (and if so, on FreeBSD too).
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 18/22] implement set_etheraddr for NetBSD

[Ed Maste]

On 22 April 2013 09:20, YAMAMOTO Takashi  wrote:
>
> Signed-off-by: YAMAMOTO Takashi 
> ---
>  lib/netdev-bsd.c | 60 
> +---
>  1 file changed, 53 insertions(+), 7 deletions(-)

Acked-by: Ed Maste 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 19/22] implement get_stats for NetBSD

[Ed Maste]

On 24 April 2013 11:56, Ben Pfaff  wrote:
> On Wed, Apr 24, 2013 at 11:28:45AM -0400, Ed Maste wrote:
>> On 22 April 2013 09:20, YAMAMOTO Takashi  wrote:
>> > +/*
>> > + * note: UINT64_MAX means unsupported
>> > + */
>>
>> Is this checked anywhere (e.g., to avoid displaying unsupported
>> stats)?  Or will it result in the display of bogus values?
>
> It's the return value convention.  It's documented in netdev-provider.h:
>
> /* Retrieves current device stats for 'netdev' into 'stats'.
>  *
>  * A network device that supports some statistics but not others, it 
> should
>  * set the values of the unsupported statistics to all-1-bits
>  * (UINT64_MAX). */
> int (*get_stats)(const struct netdev *netdev, struct netdev_stats *);

Thanks Ben, not sure how I missed that.  I'm happy enough to have
Yamamoto-san's patch go in and then I can submit a followup to merge
the NetBSD and FreeBSD parts.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 21/22] implement get_next_hop for NetBSD

[Ed Maste]

> +static int
> +netdev_bsd_get_next_hop(const struct in_addr *host, struct in_addr *next_hop,
> +char **netdev_name)
> +{
> +#if defined(__NetBSD__)

Acked-by: Ed Maste 

This is close to being usable on FreeBSD as well, with the exception
that we don't have the RT_ADVANCE macro.  I'm happy to have this patch
go in and then look at refactoring it to work on FreeBSD later on.

It seems like there should be an opportunity to share code between
this function and route-table-bsd.c's route_table_get_name() too,
although it's probably easier to do so after getting
netdev_bsd_get_next_hop() working on FreeBSD first.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] socket-util: restore building on FreeBSD.

[Ed Maste]

FreeBSD does not have EAI_ADDRFAMILY or EAI_NODATA and thus failed to build
after commit 3cbb5dc7e89df2b40bb6f715873cf2b6b25a7054 "socket-util: Use
getaddrinfo() instead of gethostbyname() for thread safety."

Signed-off-by: Ed Maste 
---
 lib/socket-util.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/lib/socket-util.c b/lib/socket-util.c
index 906b970..2dff9f5 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -201,7 +201,9 @@ lookup_hostname(const char *host_name, struct in_addr *addr)
 freeaddrinfo(result);
 return 0;
 
+#ifdef EAI_ADDRFAMILY
 case EAI_ADDRFAMILY:
+#endif
 case EAI_NONAME:
 case EAI_SERVICE:
 return ENOENT;
@@ -220,8 +222,10 @@ lookup_hostname(const char *host_name, struct in_addr 
*addr)
 case EAI_MEMORY:
 return ENOMEM;
 
+#ifdef EAI_NODATA
 case EAI_NODATA:
 return ENXIO;
+#endif
 
 case EAI_SYSTEM:
 return errno;
-- 
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev-bsd: Use UINT64_MAX for unsupported stats.

[Ed Maste]

As documented in netdev-provider.h for the get_stats method.

Signed-off-by: Ed Maste 
---
 lib/netdev-bsd.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index e3461b8..7ab9d3e 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -907,22 +907,22 @@ netdev_bsd_get_stats(const struct netdev *netdev_, struct 
netdev_stats *stats)
 stats->rx_errors = ifmd.ifmd_data.ifi_ierrors;
 stats->tx_errors = ifmd.ifmd_data.ifi_oerrors;
 stats->rx_dropped = ifmd.ifmd_data.ifi_iqdrops;
-stats->tx_dropped = 0;
+stats->tx_dropped = UINT64_MAX;
 stats->multicast = ifmd.ifmd_data.ifi_imcasts;
 stats->collisions = ifmd.ifmd_data.ifi_collisions;
 
-stats->rx_length_errors = 0;
-stats->rx_over_errors = 0;
-stats->rx_crc_errors = 0;
-stats->rx_frame_errors = 0;
-stats->rx_fifo_errors = 0;
-stats->rx_missed_errors = 0;
-
-stats->tx_aborted_errors = 0;
-stats->tx_carrier_errors = 0;
-stats->tx_fifo_errors = 0;
-stats->tx_heartbeat_errors = 0;
-stats->tx_window_errors = 0;
+stats->rx_length_errors = UINT64_MAX;
+stats->rx_over_errors = UINT64_MAX;
+stats->rx_crc_errors = UINT64_MAX;
+stats->rx_frame_errors = UINT64_MAX;
+stats->rx_fifo_errors = UINT64_MAX;
+stats->rx_missed_errors = UINT64_MAX;
+
+stats->tx_aborted_errors = UINT64_MAX;
+stats->tx_carrier_errors = UINT64_MAX;
+stats->tx_fifo_errors = UINT64_MAX;
+stats->tx_heartbeat_errors = UINT64_MAX;
+stats->tx_window_errors = UINT64_MAX;
 break;
 }
 }
-- 
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Allow master to build on Fedora with the recent threading changes

[Ed Maste]

On 6 May 2013 13:47, Kyle Mestery  wrote:
> The recent threading changes have broken the build on Fedora,
> and presumably other Red Hat based distributions. This adds an explicit
> "-lpthread" to the linker command line and allows the latest master to build
> on Fedora. I've also tested this on Ubuntu and it builds fine there.
>
> Signed-off-by: Kyle Mestery 

Acked-by: Ed Maste 

Builds fine for me on FreeBSD with this change.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [netdev v3 1/4] netdev: Factor restoring flags into new "struct netdev_saved_flags".

[Ed Maste]

On 9 May 2013 18:35, Ben Pfaff  wrote:
> This gets rid of the only per-instance data in "struct netdev", which
> will make it possible to merge "struct netdev_dev" into "struct netdev" in
> a later commit.

The four patches generally look good to me, and when I apply the whole
set they pass the unit tests on FreeBSD.

I tried rebasing the original BSD threaded userspace datapath patch on
top of the set, but it didn't work properly.  To further that
investigation I planned to try merging each of these patches
incrementally, and upon testing just this first one ran into a
use-after-free (with the default malloc diagnostics enabled for
FreeBSD in the unit tests):

(gdb) bt
#0  netdev_dev_unref (dev=0x5a5a5a5a5a5a5a5a) at lib/netdev.c:314
#1  0x00412158 in update_port (ofproto=0x801cce010,
name=) at ofproto/ofproto.c:1975
#2  0x00414405 in reinit_ports (p=)
at ofproto/ofproto.c:1660
#3  process_port_change (devname=, error=,
ofproto=) at ofproto/ofproto.c:1115
#4  ofproto_run (p=0x801cce010) at ofproto/ofproto.c:1186
#5  0x0040ccd7 in bridge_run () at vswitchd/bridge.c:2327
#6  0x0040dcfa in main (argc=, argv=)
at vswitchd/ovs-vswitchd.c:125

It looks like a ref_cnt increment is missing somewhere.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [netdev v3 1/4] netdev: Factor restoring flags into new "struct netdev_saved_flags".

[Ed Maste]

On 10 May 2013 11:55, Ben Pfaff  wrote:
> Thanks, I found the problem with valgrind, and here is an incremental
> fix:
>
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 9590f83..415cdb4 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -322,8 +322,10 @@ void
>  netdev_close(struct netdev *netdev)
>  {
>  if (netdev) {
> +struct netdev_dev *dev = netdev_get_dev(netdev);
> +
>  netdev_uninit(netdev, true);
> -netdev_dev_unref(netdev_get_dev(netdev));
> +netdev_dev_unref(dev);
>  }
>  }

Ahh, of course.  This fixes the segfaulting tests for me, but the unit
tests now get stuck in "tunnel - input."

(gdb) bt
#0  0x0008015b4c9c in poll () from /lib/libc.so.7
#1  0x0041f693 in time_poll (pollfds=0x801c1a850, n_pollfds=2,
timeout_when=9223372036854775807, elapsed=0x7fffca74) at
lib/timeval.c:388
#2  0x00416472 in poll_block () at lib/poll-loop.c:249
#3  0x0040ab55 in do_vsctl (args=Variable "args" is not available.
) at utilities/ovs-vsctl.c:4126
#4  0x0040b56f in main (argc=47, argv=0x7fffcef8) at
utilities/ovs-vsctl.c:220
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [netdev v3 1/4] netdev: Factor restoring flags into new "struct netdev_saved_flags".

[Ed Maste]

On 10 May 2013 13:12, Ed Maste  wrote:
> Ahh, of course.  This fixes the segfaulting tests for me, but the unit
> tests now get stuck in "tunnel - input."

Oh, and of course the reason ovs-vsctl is stuck is that ovs-vswitchd segfaulted.

#0  0x00080164b560 in strncpy () from /lib/libc.so.7
#1  0x0049ee29 in netdev_bsd_do_ioctl (netdev=0x801c231d0,
ifr=0x7fffc9a0, cmd=3223349521, cmd_name=0x4d3303 "SIOCGIFFLAGS")
at lib/netdev-bsd.c:1480
#2  0x0049eef2 in get_flags (flags=,
netdev=) at lib/netdev-bsd.c:1389
#3  netdev_bsd_update_flags (netdev=0x7fffc9a0, off=0, on=0,
old_flagsp=0x7fffca24) at lib/netdev-bsd.c:1230
#4  0x0044f527 in do_update_flags (netdev=0x801c22310, off=0,
on=0, old_flagsp=0x7fffca6c, sfp=0x0) at lib/netdev.c:816
#5  0x004a03ea in netdev_bsd_open_system
(netdev_dev_=0x801c231d0, netdevp=0x7fffcad8) at
lib/netdev-bsd.c:412
#6  0x0044fa19 in netdev_open (name=0x70e2d0 "alc0",
type=0x4a9d1f "system", netdevp=0x7fffcad8) at lib/netdev.c:241
#7  0x0044d366 in tunnel_get_status (netdev=,
smap=0x7fffcb20) at lib/netdev-vport.c:226
#8  0x00406bf9 in iface_refresh_status (iface=0x801c25220) at
vswitchd/bridge.c:1799
#9  0x00408f3b in iface_create (br=0x801cbf100,
if_cfg=0x801c221c0, ofp_port=3) at vswitchd/bridge.c:1520
#10 0x00409335 in bridge_reconfigure_ofp () at vswitchd/bridge.c:568
#11 0x0040941e in bridge_reconfigure_continue
(ovs_cfg=0x801c1d280) at vswitchd/bridge.c:589
#12 0x0040c85e in bridge_run () at vswitchd/bridge.c:2374
#13 0x0040dcfa in main (argc=, argv=) at vswitchd/ovs-vswitchd.c:125
(gdb) frame 0

I'm taking a look at it, just wanted to reply because my previous
email is clearly lacking sufficient detail.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev: Incremental BSD fix for netdev refactoriong

[Ed Maste]

This is an incremental to patch v3 of
netdev: Factor restoring flags into new "struct netdev_saved_flags",
implementing the equivalent change for netdev-bsd.

Signed-off-by: Ed Maste 
---
With this, the first patch now passes unit tests for me on FreeBSD.

 lib/netdev-bsd.c | 43 +++
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 7ab9d3e..8b384ba 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -133,11 +133,11 @@ static int cache_notifier_refcount;
 
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
 
-static int netdev_bsd_do_ioctl(const struct netdev *, struct ifreq *,
- unsigned long cmd, const char *cmd_name);
+static int netdev_bsd_do_ioctl(const char *, struct ifreq *, unsigned long cmd,
+   const char *cmd_name);
 static void destroy_tap(int fd, const char *name);
-static int get_flags(const struct netdev *, int *flagsp);
-static int set_flags(struct netdev *, int flags);
+static int get_flags(const struct netdev_dev *, int *flagsp);
+static int set_flags(const char *, int flags);
 static int do_set_addr(struct netdev *netdev,
int ioctl_nr, const char *ioctl_name,
struct in_addr addr);
@@ -813,7 +813,8 @@ netdev_bsd_get_mtu(const struct netdev *netdev_, int *mtup)
 struct ifreq ifr;
 int error;
 
-error = netdev_bsd_do_ioctl(netdev_, &ifr, SIOCGIFMTU, "SIOCGIFMTU");
+error = netdev_bsd_do_ioctl(netdev_get_name(netdev_), &ifr, SIOCGIFMTU,
+"SIOCGIFMTU");
 if (error) {
 return error;
 }
@@ -1089,7 +1090,7 @@ netdev_bsd_get_in4(const struct netdev *netdev_, struct 
in_addr *in4,
 int error;
 
 ifr.ifr_addr.sa_family = AF_INET;
-error = netdev_bsd_do_ioctl(netdev_, &ifr,
+error = netdev_bsd_do_ioctl(netdev_get_name(netdev_), &ifr,
   SIOCGIFADDR, "SIOCGIFADDR");
 if (error) {
 return error;
@@ -1098,7 +1099,7 @@ netdev_bsd_get_in4(const struct netdev *netdev_, struct 
in_addr *in4,
 sin = (struct sockaddr_in *) &ifr.ifr_addr;
 netdev_dev->in4 = sin->sin_addr;
 netdev_dev->cache_valid |= VALID_IN4;
-error = netdev_bsd_do_ioctl(netdev_, &ifr,
+error = netdev_bsd_do_ioctl(netdev_get_name(netdev_), &ifr,
   SIOCGIFNETMASK, "SIOCGIFNETMASK");
 if (error) {
 return error;
@@ -1221,19 +1222,21 @@ iff_to_nd_flags(int iff)
 }
 
 static int
-netdev_bsd_update_flags(struct netdev *netdev, enum netdev_flags off,
+netdev_bsd_update_flags(struct netdev_dev *dev_, enum netdev_flags off,
   enum netdev_flags on, enum netdev_flags *old_flagsp)
 {
+struct netdev_dev_bsd *netdev_dev;
 int old_flags, new_flags;
 int error;
 
-error = get_flags(netdev, &old_flags);
+netdev_dev = netdev_dev_bsd_cast(dev_);
+error = get_flags(dev_, &old_flags);
 if (!error) {
 *old_flagsp = iff_to_nd_flags(old_flags);
 new_flags = (old_flags & ~nd_to_iff_flags(off)) | nd_to_iff_flags(on);
 if (new_flags != old_flags) {
-error = set_flags(netdev, new_flags);
-
netdev_dev_bsd_changed(netdev_dev_bsd_cast(netdev_get_dev(netdev)));
+error = set_flags(netdev_dev_get_name(dev_), new_flags);
+netdev_dev_bsd_changed(netdev_dev);
 }
 }
 return error;
@@ -1381,12 +1384,12 @@ destroy_tap(int fd, const char *name)
 }
 
 static int
-get_flags(const struct netdev *netdev, int *flags)
+get_flags(const struct netdev_dev *dev, int *flags)
 {
 struct ifreq ifr;
 int error;
 
-error = netdev_bsd_do_ioctl(netdev, &ifr, SIOCGIFFLAGS, "SIOCGIFFLAGS");
+error = netdev_bsd_do_ioctl(dev->name, &ifr, SIOCGIFFLAGS, "SIOCGIFFLAGS");
 
 *flags = 0x & (ifr.ifr_flagshigh << 16);
 *flags |= 0x & ifr.ifr_flags;
@@ -1395,14 +1398,14 @@ get_flags(const struct netdev *netdev, int *flags)
 }
 
 static int
-set_flags(struct netdev *netdev, int flags)
+set_flags(const char *name, int flags)
 {
 struct ifreq ifr;
 
 ifr.ifr_flags = 0x & flags;
 ifr.ifr_flagshigh = (0x & flags) >> 16;
 
-return netdev_bsd_do_ioctl(netdev, &ifr, SIOCSIFFLAGS, "SIOCSIFFLAGS");
+return netdev_bsd_do_ioctl(name, &ifr, SIOCSIFFLAGS, "SIOCSIFFLAGS");
 }
 
 static int
@@ -1474,13 +1477,13 @@ set_etheraddr(const char *netdev_name, int 
hwaddr_family,
 }
 
 static int
-netdev_bsd_do_ioctl(const struct netdev *netdev, struct ifreq *ifr,
-unsigned long cmd, const char *cmd_name)
+netdev_bs

Re: [ovs-dev] [PATCH] netdev: Incremental BSD fix for netdev refactoriong

[Ed Maste]

On 10 May 2013 14:38, Ben Pfaff  wrote:
> On Fri, May 10, 2013 at 02:15:50PM -0400, Ed Maste wrote:
>> This is an incremental to patch v3 of
>> netdev: Factor restoring flags into new "struct netdev_saved_flags",
>> implementing the equivalent change for netdev-bsd.
>>
>> Signed-off-by: Ed Maste 
>> ---
>> With this, the first patch now passes unit tests for me on FreeBSD.
>
> Great, I folded that in and applied this patch to master.

Thanks.  I find it a little ironic that the trouble here was caused by
the netdev vs. netdev_dev issue itself.  This also explains why the
end result is almost the same (when netdev_dev is removed in patch 4).

> There were merge conflicts in netdev-bsd when I continued to rebase
> through the rest of the series, and I am not confident that I resolved
> them correctly (I did not try a build on FreeBSD), so I will repost the
> rest for your review.

The conflict resolutions match what I ended up with when I reapplied
patches 2-4 on top of my incremental, at least.  All unit tests now
pass on FreeBSD for patches 2-4 in v4; I haven't tried an actual
system test.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev-bsd: Adjust argument line wrapping

[Ed Maste]

This file presumably started out life as a copy of netdev-linux.c, and
some indentation was not updated after s/linux/bsd/.

Signed-off-by: Ed Maste 
---
There are still a few cases that seem not to match style; I adjusted these
because Ethan noticed them in commit 4b60911.

 lib/netdev-bsd.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index ea0adb1..3ddd1d5 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -221,7 +221,7 @@ netdev_dev_bsd_changed(struct netdev_dev_bsd *dev)
 /* Invalidate cache in case of interface status change. */
 static void
 netdev_bsd_cache_cb(const struct rtbsd_change *change,
-  void *aux OVS_UNUSED)
+void *aux OVS_UNUSED)
 {
 struct netdev_dev_bsd *dev;
 
@@ -836,7 +836,7 @@ netdev_bsd_send_wait(struct netdev *netdev_)
  */
 static int
 netdev_bsd_set_etheraddr(struct netdev *netdev_,
-   const uint8_t mac[ETH_ADDR_LEN])
+ const uint8_t mac[ETH_ADDR_LEN])
 {
 struct netdev_dev_bsd *netdev_dev =
 netdev_dev_bsd_cast(netdev_get_dev(netdev_));
@@ -863,7 +863,7 @@ netdev_bsd_set_etheraddr(struct netdev *netdev_,
  */
 static int
 netdev_bsd_get_etheraddr(const struct netdev *netdev_,
-   uint8_t mac[ETH_ADDR_LEN])
+ uint8_t mac[ETH_ADDR_LEN])
 {
 struct netdev_dev_bsd *netdev_dev =
 netdev_dev_bsd_cast(netdev_get_dev(netdev_));
@@ -1095,8 +1095,8 @@ netdev_bsd_parse_media(int media)
  */
 static int
 netdev_bsd_get_features(const struct netdev *netdev,
-  enum netdev_features *current, uint32_t *advertised,
-  enum netdev_features *supported, uint32_t *peer)
+enum netdev_features *current, uint32_t *advertised,
+enum netdev_features *supported, uint32_t *peer)
 {
 struct ifmediareq ifmr;
 int *media_list;
@@ -1174,7 +1174,7 @@ netdev_bsd_get_in4(const struct netdev *netdev_, struct 
in_addr *in4,
 
 ifr.ifr_addr.sa_family = AF_INET;
 error = netdev_bsd_do_ioctl(netdev_get_name(netdev_), &ifr,
-  SIOCGIFADDR, "SIOCGIFADDR");
+SIOCGIFADDR, "SIOCGIFADDR");
 if (error) {
 return error;
 }
@@ -1183,7 +1183,7 @@ netdev_bsd_get_in4(const struct netdev *netdev_, struct 
in_addr *in4,
 netdev_dev->in4 = sin->sin_addr;
 netdev_dev->cache_valid |= VALID_IN4;
 error = netdev_bsd_do_ioctl(netdev_get_name(netdev_), &ifr,
-  SIOCGIFNETMASK, "SIOCGIFNETMASK");
+SIOCGIFNETMASK, "SIOCGIFNETMASK");
 if (error) {
 return error;
 }
@@ -1201,7 +1201,7 @@ netdev_bsd_get_in4(const struct netdev *netdev_, struct 
in_addr *in4,
  */
 static int
 netdev_bsd_set_in4(struct netdev *netdev_, struct in_addr addr,
- struct in_addr mask)
+   struct in_addr mask)
 {
 struct netdev_dev_bsd *netdev_dev =
 netdev_dev_bsd_cast(netdev_get_dev(netdev_));
@@ -1306,7 +1306,7 @@ iff_to_nd_flags(int iff)
 
 static int
 netdev_bsd_update_flags(struct netdev_dev *dev_, enum netdev_flags off,
-  enum netdev_flags on, enum netdev_flags *old_flagsp)
+enum netdev_flags on, enum netdev_flags *old_flagsp)
 {
 struct netdev_dev_bsd *netdev_dev;
 int old_flags, new_flags;
-- 
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] netdev-dummy: Remove FreeBSD dependency.

[Ed Maste]

On 17 May 2013 18:57, Ben Pfaff  wrote:
> There's no particular reason that netdev_dummy_register() has to care about
> the particular OS, except that the tests like to use the special Linux-only
> tunnel vport types.  But that can be done better, I think, by just always
> registering them from netdev_dummy_register() and making that function
> idempotent, so that calling it twice under Linux has no additional effect.
> This commit implements that solution.
>
> CC: Ed Maste 
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 

Looks good to me, and unit tests continue to pass on FreeBSD.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 02/13] add minimal NetBSD support

[Ed Maste]

On 21 May 2013 10:59, Ben Pfaff  wrote:
> On Tue, May 21, 2013 at 05:49:54PM +0900, y...@mwd.biglobe.ne.jp wrote:
>> From: YAMAMOTO Takashi 
>>
>> mostly ride on the existing FreeBSD support.
>>
>> Signed-off-by: YAMAMOTO Takashi 
>
> I'd like to refine this patch a little in the following way (which is
> followed by a full revised version).  Ed and Yamamoto-san, are you happy
> with this version?

I'm happy with your version (based on review only; I didn't apply the
patch to test), with one question.

> - * Copyright (c) 2011 Gaetano Catalli.
> + * Copyright (c) 2011, 2013 Gaetano Catalli.

Would this not be more correct if written as:

* Copyright (c) 2011 Gaetano Catalli.
* Copyright (c) 2013 YAMAMOTO Takashi.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] configure: Add if_mib.h prerequisites.

[Ed Maste]

Signed-off-by: Ed Maste 
---
 configure.ac | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1e23289..91f3835 100644
--- a/configure.ac
+++ b/configure.ac
@@ -63,8 +63,9 @@ AC_CHECK_MEMBERS([struct stat.st_mtim.tv_nsec, struct 
stat.st_mtimensec],
   [], [], [[#include ]])
 AC_CHECK_MEMBERS([struct ifreq.ifr_flagshigh], [], [], [[#include ]])
 AC_CHECK_FUNCS([mlockall strnlen strsignal getloadavg statvfs getmntent_r])
-AC_CHECK_HEADERS(
-  [mntent.h sys/statvfs.h linux/types.h linux/if_ether.h net/if_mib.h])
+AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h])
+AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include 
+#include ]])
 
 OVS_CHECK_PKIDIR
 OVS_CHECK_RUNDIR
-- 
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev-bsd: Correct pointer use after refactoring.

[Ed Maste]

Introduced in commit 666afb55e84e9118812de81a75655ec9567b7a5b.

Signed-off-by: Ed Maste 
---
 lib/netdev-bsd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 1301aad..87ac79f 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -1705,9 +1705,9 @@ static int
 ifr_get_flags(const struct ifreq *ifr)
 {
 #ifdef HAVE_STRUCT_IFREQ_IFR_FLAGSHIGH
-return (ifr.ifr_flagshigh << 16) | ifr.ifr_flags;
+return (ifr->ifr_flagshigh << 16) | ifr->ifr_flags;
 #else
-return ifr.ifr_flags;
+return ifr->ifr_flags;
 #endif
 }
 
-- 
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 00/13] NetBSD support

[Ed Maste]

On 23 May 2013 00:05, Ben Pfaff  wrote:
> I have all of these patches ready to apply, after Ed reviewed my changes
> to patch 2.

Apparently I didn't review them well enough; sorry for letting some
minor nits slip through.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev-bsd: Silence warnings on unimplemented platform.

[Ed Maste]

netdev_bsd_get_next_hop currently lacks an implementation on FreeBSD, so
its arguments are unused; mark them so.

Signed-off-by: Ed Maste 
---
 lib/netdev-bsd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 87ac79f..0fb0057 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -1226,8 +1226,9 @@ netdev_bsd_convert_kernel_name_to_ovs_name(const char 
*kernel_name)
 #endif
 
 static int
-netdev_bsd_get_next_hop(const struct in_addr *host, struct in_addr *next_hop,
-char **netdev_name)
+netdev_bsd_get_next_hop(const struct in_addr *host OVS_UNUSED,
+struct in_addr *next_hop OVS_UNUSED,
+char **netdev_name OVS_UNUSED)
 {
 #if defined(__NetBSD__)
 static int seq = 0;
-- 
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Possible Regression in 'netdev: Add new "struct netdev_rx" for capturing packets from a netdev.'

[Ed Maste]

On 12 June 2013 07:04, Murphy McCauley  wrote:
> (Sorry this isn't an actual reply and is missing context -- I wasn't on the 
> list when it was originally posted.)
>
> Simon and I have been in touch about this, and I thought I'd share my 
> findings for what they're worth.
>
> The problem is from the commit Simon mentioned 
> (796223f5bc3a4896e6398733c798390158479400).  Specifically, it's in 
> netdev-linux.c in netdev_linux_send().
>
> The new version always sends using the "sender" socket made by 
> af_packet_sock() unless the interface is a tap, in which case it sends it 
> using the tap fd.  This differs from the old version which sent using 
> whatever was in the fd field of the netdev if it was available.  For tap 
> interfaces, this was the tap fd, so the result was the same as it is now.  
> But for other interfaces, this held the socket opened for receiving if the 
> interface was listening (which was maybe never "right" in some sense and 
> isn't convenient anymore since this socket descriptor is no longer stored in 
> the non-rx netdev).
>
> The comments indicate that the exception is made for tap interfaces since 
> writing to a tap interface with an AF_PACKET socket results in receiving the 
> packet you just wrote.  However, I don't think this behavior is limited to 
> taps.  Since the old version of the code sent and received with the same 
> socket descriptor, I think the loop was fixed by the check in 
> dev_queue_xmit_nit() in net/core/dev.c.  Since they're two different socket 
> descriptors now, this no longer works and you get the loop.

Ahh, it turns out Ben explained this to me when I ran into a related
issue with the FreeBSD userspace implementation.  Ben's message in the
thread is at http://openvswitch.org/pipermail/dev/2012-July/018806.html
.

> I fixed it (I think) by adding a BPF packet filter on the rx socket so that 
> it only receives incoming packets.  There's probably a better fix, but you're 
> welcome to the patch if you want it.

I think it's worth taking a look.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 00/11] basic thread support library

[Ed Maste]

On 19 June 2013 16:17, Ben Pfaff  wrote:
> This series has two purposes:
>
> * Add a basic thread support library to the tree.
>
> * Get rid of all calls to C library functions that POSIX describes
>   as inherently unsafe in a multithreaded program, and then add a
>   make-time check that flags an error if any calls get reintroduced.

This fails to build on FreeBSD for me, because it appears to rely on
header leakage for stdint.h types.

In file included from lib/ovs-atomic.h:237,
 from lib/ovs-thread.h:23,
 from lib/command-line.c:22:
lib/ovs-atomic-c11.h:26: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'uint8_t'
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 06/11] New function ovs_strerror() as a thread-safe replacement for strerror().

[Ed Maste]

On 19 June 2013 16:17, Ben Pfaff  wrote:
> +#if STRERROR_R_CHAR_P
> +/* GNU style strerror_r() might return an immutable static string, or it
> + * might write and return 'buffer', but in either case we can pass the
> + * returned string directly to the caller. */
> +s = strerror_r(error, buffer, BUFSIZE);
> +#else  /* strerror_r() returns an int. */
> +s = buffer;
> +if (strerror_r(error, buffer, BUFSIZE)) {
> +/* strerror_r() is only allowed to fail on ERANGE (because the buffer
> + * is too short).  We don't check the actual failure reason because
> + * POSIX requires strerror_r() to return the error but old glibc
> + * (before 2.13) returns -1 and sets errno. */
> +snprintf(buffer, ptb.bufsize, "Unknown error %d", error);

This one fails to build with:

lib/util.c:342:26: error: use of undeclared identifier 'ptb'
snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
 ^
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 00/11] basic thread support library

[Ed Maste]

On 19 June 2013 17:22, Ben Pfaff  wrote:
> On Wed, Jun 19, 2013 at 05:20:11PM -0400, Ed Maste wrote:
>> On 19 June 2013 16:17, Ben Pfaff  wrote:
>> > This series has two purposes:
>> >
>> > * Add a basic thread support library to the tree.
>> >
>> > * Get rid of all calls to C library functions that POSIX describes
>> >   as inherently unsafe in a multithreaded program, and then add a
>> >   make-time check that flags an error if any calls get reintroduced.
>>
>> This fails to build on FreeBSD for me, because it appears to rely on
>> header leakage for stdint.h types.
>>
>> In file included from lib/ovs-atomic.h:237,
>>  from lib/ovs-thread.h:23,
>>  from lib/command-line.c:22:
>> lib/ovs-atomic-c11.h:26: error: expected '=', ',', ';', 'asm' or
>> '__attribute__' before 'uint8_t'
>
> Does this mean that FreeBSD has ?  Is it fully
> functional?  That's the one implementation I wasn't able to test,
> because I didn't know of any C11 implementations with .

Indeed we do, and it should be functional.  SVN history is here if
you're interested:
http://svnweb.freebsd.org/base/head/sys/sys/stdatomic.h

That said, it seems it wasn't stdint.h per se that was the problem; I
needed to change:

-typedef _Atomic uint8_t   atomic_uint8_t;
+typedef _Atomic(uint8_t)   atomic_uint8_t;

etc.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 00/11] basic thread support library

[Ed Maste]

On 19 June 2013 17:54, Ben Pfaff  wrote:
> Oh, that's odd.  C11 appears to say that the parentheses are optional
> with _Atomic in this case (you can use _Atomic(...) as a type
> specifier or _Atomic by itself as a type qualifier) but I'll change it
> since that fixes the problem.

Ahh, it looks like _Atomic as a type qualifier is a recent addition to
clang: http://llvm.org/viewvc/llvm-project?view=revision&revision=178210
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 06/11] New function ovs_strerror() as a thread-safe replacement for strerror().

[Ed Maste]

On 19 June 2013 18:48, Ben Pfaff  wrote:
> I pushed a fix for this issue (just s/ptb.bufsize/BUFSIZE/) and the
> ovs-atomic-c11.h issue you also mentioned to the "threads" branch at
> https://github.com/blp/ovs-reviews/commits/threads

Thanks Ben, with those two changes your threads branch passes unit
tests for me on FreeBSD 9.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 02/11] ovs-thread: Add per-thread data support.

[Ed Maste]

On 20 June 2013 16:18, Ben Pfaff  wrote:
> On Wed, Jun 19, 2013 at 01:17:03PM -0700, Ben Pfaff wrote:
>> POSIX defines a portable pthread_key_t API for per-thread data.  GCC and
>> C11 have two different forms of per-thread data that are generally faster
>> than the POSIX API, where they are available.  This commit adds a
>> macro-based wrapper, DEFINE_PER_THREAD_DATA, that takes advantage of the
>> GCC extension where it is available and falls back to the POSIX API
>> otherwise.  (I'm not aware of any compilers that implement the C11 feature,
>> so this commit doesn't try to use it.)
>
> Ed Maste pointed out off-list that clang on FreeBSD supports
> _Thread_local.  Here's a revised version of the patch that supports
> both _Thread_local and __thread.  I've also updated the "reviews"
> branch.

I changed the autoconf test like so:

diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4
index 3895346..eb91b1a 100644
--- a/m4/openvswitch.m4
+++ b/m4/openvswitch.m4
@@ -402,7 +402,10 @@ AC_DEFUN([OVS_CHECK__THREAD_LOCAL],
  [whether $CC supports _Thread_local],
  [ovs_cv__Thread_local],
  [AC_LINK_IFELSE(
-[AC_LANG_PROGRAM([static _Thread_local var;], [return var;])],
+[AC_LANG_PROGRAM(
+  [#include 
+   static _Thread_local int var;],
+  [return var;])],
 [ovs_cv__Thread_local=yes],
 [ovs_cv__Thread_local=no])])
if test $ovs_cv__Thread_local = no; then

This version builds and passes unit tests for me on FreeBSD 9.x, with
both GCC and Clang.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 02/11] ovs-thread: Add per-thread data support.

[Ed Maste]

On 20 June 2013 20:10, Ben Pfaff  wrote:
> Can you confirm that the autoconf test needs #include  but
> the actual program doesn't?  It looks funny.

Yes.  It looks like it should be added in the actual program; right
wow we end up with the definition leaking from another header.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] autoconf test OVS_ENABLE_OPTION fails to detect options unsupported by clang

[Ed Maste]

In clang's attempt to be GCC compatible it ignores, but does not fail
on, unknown -W options.  Configure then ends up added the option to
WARNING_FLAGS, and the warning is then emitted for every file
compiled.

>From config.log:

configure:7388: checking whether clang accepts -Wno-override-init
configure:7406: clang -c -g -O2 -Wno-override-init  conftest.c >&5
clang: warning: unknown warning option '-Wno-override-init'; did you
mean '-Wno-over-aligned'?
warning: unknown warning option '-Wno-override-init'; did you mean
'-Wno-over-aligned'? [-Wunknown-warning-option]
1 warning generated.
configure:7406: $? = 0

With a quick search I didn't find a canonical way to test this in
autoconf, although adding -Werror does change the return value to 1 in
this case.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] autoconf test OVS_ENABLE_OPTION fails to detect options unsupported by clang

[Ed Maste]

On 21 June 2013 12:05, Ben Pfaff  wrote:
> By default, clang warns about but does not fail on unknown -W options.
> This made configure add the option to WARNING_FLAGS, which caused the
> warning about not-understood warnings to be emitted for every file
> compiled.
>
> In combination with -Werror, clang does fail on unknown -W options.  This
> commit adds -Werror during configure's warning tests, which should cause
> the not-understood warnings to be detected that way.
>
> Reported-by: Ed Maste 
> Signed-off-by: Ben Pfaff 

Thanks Ben, looks good and it works for me.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 28/28] vlog: Make thread-safe.

[Ed Maste]

On 10 July 2013 19:04, Ben Pfaff  wrote:
>  static void format_log_message(const struct vlog_module *, enum vlog_level,
> -   enum vlog_facility, unsigned int msg_num,
> +   enum vlog_facility,
> const char *message, va_list, struct ds *)
>  PRINTF_FORMAT(5, 0);

This should be PRINTF_FORMAT(4, 0) now.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 05/28] timeval: Always block SIGALRM in time_poll().

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> (The usual reason for ppoll() is to ensure atomicity of changing the signal
> mask and blocking, but time_poll() does not need atomicity.  Therefore,
> even a userspace emulated ppoll() would work here in a race-free manner, if
> the kernel does not support ppoll().)

And we'll need that userspace emulated ppoll() for non-Linux systems;
seem reasonable to just implement it in timeval.c inside an #ifndef
HAVE_PPOLL from autoconf?
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 01/28] fatal-signal: Remove write-only variable fatal_signal_set.

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 
> ---
>  lib/fatal-signal.c |5 -

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 24/28] vlog: Make vlog_should_drop() thread-safe.

[Ed Maste]

On 10 July 2013 19:04, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 13/28] poll-loop: Fix typo in comment.

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 08/28] timeval: Remove backtrace feature.

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> The backtrace feature of timeval is useful because it provides a "poor
> man's profile" view of Open vSwitch.  But it is not likely to be useful in
> a multithreaded process, because signal delivery doesn't necessarily follow
> the profile when there is more than one thread.  (A signal in a
> multithreaded process are delivered to an arbitrary thread.)
>
> Another problem with the backtrace feature is that it is difficult for
> format_backtraces() to synchronize properly with the signal handler in a
> multithreaded process.  In a single-threaded process, it can just block
> the signal handler, but in a multithreaded process this does not prevent
> signal delivery to threads other than the one running format_backtrace().
>
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 23/28] util: Make subprogram_name thread-specific.

[Ed Maste]

On 10 July 2013 19:04, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 

> +void
> +set_subprogram_name(const char *name)
> +{
> +free(subprogram_name_set(xstrdup(name)));
> +}

How about calling (through appropriate abstraction) pthread_setname_np
(Linux) or pthread_set_name_np (FreeBSD)?  The thread names will then
appear in gdb or lldb, and in top(1) (on FreeBSD at least).

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 06/28] timeval: New function xclock_gettime().

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 09/28] timeval: Fix typo in comment.

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 27/28] vlog: Remove unused function vlog_exit().

[Ed Maste]

On 10 July 2013 19:04, Ben Pfaff  wrote:
> This is harder to implement once threads are introduced.
>
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 14/28] vlog: Remove support for worker process.

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> The worker process implementation isn't thread-safe and, once OVS
> itself is threaded, it doesn't make much sense to have a worker
> process anyway.
>
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 26/28] vlog: Remove unused function vlog_get_log_file().

[Ed Maste]

On 10 July 2013 19:04, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 07/28] timeval: Make reading the current time thread-safe.

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 

(I rebased on master and had a trivial conflict to resolve.)
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads 10/28] timeval: Make CPU usage and wakeup tracking per-thread.

[Ed Maste]

On 10 July 2013 19:03, Ben Pfaff  wrote:
> This should make the "timeval" module thread-safe, except for its
> calls into vlog (because vlog is not yet thread-safe).
>
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [threads v2 06/13] latch: New module for a thread-safe, signal-safe, pollable doorbell.

[Ed Maste]

On 12 July 2013 17:54, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
>

Acked-by: Ed Maste 

with one comment,

diff --git a/lib/latch.c b/lib/latch.c
> +void
> +(latch_wait)(const struct latch *latch, const char *where)
>
...

> diff --git a/lib/latch.h b/lib/latch.h
> +#define latch_wait(latch) latch_wait(latch, SOURCE_LOCATOR)
>

This seems like it will be confusing for those who aren't C preprocessor
gurus, and might trip up source indexing / analysis tools.  I see that
there are a few other examples in OVS already though.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Avoid C preprocessor trick where macro has the same name as a function.

[Ed Maste]

On 29 July 2013 12:57, Ben Pfaff  wrote:
> In C, one can do preprocessor tricks by making a macro expansion include
> the macro's own name.  We actually used this in the tree to automatically
> provide function arguments, e.g.:
>
> #define f(x) f(x, __FILE__, __LINE__)
> int f(int x, const char *file, int line);
>
> ...
>
> f(1);/* Expands to a call like f(1, __FILE__, __LINE__); */
>
> However it's somewhat confusing, so this commit stops using that trick.
>
> Reported-by: Ed Maste 
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 

One minor comment:

>#define f(x) f(x, __FILE__, __LINE__)
>int f(int x, const char *file, int line);

Unlike the instances in the source, the example as-is doesn't compile,
which I think helps provide justification for the removal of the trick
:-)

It's missing either an #undef, or the further trick of relying on the
preprocessor only expanding function-like macros if the token
following is a left parenthesis, resulting in the odd-looking int
(f)(int x, const char *file, int line) syntax.
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] lib: Add CRC32C Implementation

[Ed Maste]

On 30 July 2013 20:31, Joe Stringer  wrote:
> This implementation was derived from FreeBSD:
> http://code.google.com/p/freebsd-head/source/browse/sys/libkern/crc32.c

The canonical FreeBSD web repo location for this file would be:
http://svnweb.freebsd.org/base/head/sys/libkern/crc32.c

(It doesn't matter much, but mirrors like the one on code.google.com
may not be updated regularly, or may disappear in the future.)
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] bfd: Include prerequisite header for FreeBSD

[Ed Maste]

Signed-off-by: Ed Maste 
---
 lib/bfd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/bfd.c b/lib/bfd.c
index 74b27c4..6f86f26 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -15,6 +15,7 @@
 #include 
 #include "bfd.h"
 
+#include 
 #include 
 #include 
 #include 
-- 
1.8.3.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] tests: Fix build on FreeBSD

[Ed Maste]

Avoid relying on a non-portable implementation detail for atomic_flag
tests.  Per the standard, the only way to obtain the value of the flag
is via the return value from atomic_flag_test_and_set.

Signed-off-by: Ed Maste 
---
 tests/test-atomic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-atomic.c b/tests/test-atomic.c
index e9bd6bd..e6df1cd 100644
--- a/tests/test-atomic.c
+++ b/tests/test-atomic.c
@@ -66,9 +66,9 @@ test_atomic_flag(void)
 {
 atomic_flag flag = ATOMIC_FLAG_INIT;
 ovs_assert(atomic_flag_test_and_set(&flag) == false);
-ovs_assert(flag.b == true);
+ovs_assert(atomic_flag_test_and_set(&flag) == true);
 atomic_flag_clear(&flag);
-ovs_assert(flag.b == false);
+ovs_assert(atomic_flag_test_and_set(&flag) == false);
 }
 
 int
-- 
1.8.3.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Clang compile failure with C11 atomics

[Ed Maste]

Commit 1514b275 introduces the use of atomic_read_explicit for
once-only initializers. When using C11 atomics this becomes
atomic_load_explicit, and the first argument needs to be non-const --
with FreeBSD HEAD's in-tree clang 3.3 the build fails for me with:

In file included from lib/bfd.c:34:
./lib/ovs-thread.h:482:5: error: first argument to atomic operation must be a
  pointer to non-const _Atomic type ('const atomic_bool *' (aka
  'const _Atomic(bool) *') invalid)
atomic_read_explicit(&once->done, &done, memory_order_relaxed);
^~~~

One workaround is to sprinkle CONST_CASTs around, in ovs-thread.h and
lib/bfd. - e.g.:

--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -479,7 +479,8 @@ ovsthread_once_is_done__(const struct ovsthread_once *once)
 {
 bool done;

-atomic_read_explicit(&once->done, &done, memory_order_relaxed);
+atomic_read_explicit(CONST_CAST(atomic_bool *, &once->done), &done,
+ memory_order_relaxed);
 return done;
 }

Ben, what do you think?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [clang-atomics 4/4] ovs-thread: Work around apparent Clang bug.

[Ed Maste]

On 20 August 2013 14:10, Ben Pfaff  wrote:
> Without this patch, I get Clang warnings that I don't understand for each
> of the non-static functions in dirs.c:
>
> warning: control reaches end of non-void function

This is a clang bug; in your example the call to bar() is omitted by
clang, and the warning is thus actually true.  I encountered the bug
while looking at other issues in FreeBSD's stdatomic.h and my
colleague distilled a simple test case and submitted a bug report[1]
for it yesterday.  As of this morning it's fixed in clang svn[2].
We're going to bring the fix into FreeBSD's in-tree clang.

[1] http://llvm.org/bugs/show_bug.cgi?id=16931
[2] http://llvm.org/viewvc/llvm-project?view=revision&revision=188718
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [clang-atomics 2/4] ovs-atomic: atomic_load() must take a non-const argument.

[Ed Maste]

On 20 August 2013 14:10, Ben Pfaff  wrote:
> C11 says that atomic_load() requires a non-const argument, and Clang
> enforces that.  This fixes warnings with FreeBSD  that uses
> the Clang extensions.

Acked-by: Ed Maste 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [clang-atomics 1/4] ovs-atomic: Fix typo in comment.

[Ed Maste]

On 20 August 2013 14:10, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 

Acked-by: Ed Maste 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] util.c: Include pthread_np.h on FreeBSD

[Ed Maste]

Signed-off-by: Ed Maste 
---
On FreeBSD pthread_set_name_np's prototype is provided by pthread_np.h.
As I believe it is the only platform to provide the "set_name" (with an
underscore) variant I hope it's fine to use the existing autoconf macro
HAVE_PTHREAD_SET_NAME_NP.

 lib/util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/util.c b/lib/util.c
index 76c33cd..3cada4a 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -30,6 +30,9 @@
 #include "coverage.h"
 #include "ovs-thread.h"
 #include "vlog.h"
+#ifdef HAVE_PTHREAD_SET_NAME_NP
+#include 
+#endif
 
 VLOG_DEFINE_THIS_MODULE(util);
 
-- 
1.8.3.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] lib: Determine cpu core count with /proc/cpuinfo

[Ed Maste]

On 5 December 2013 18:42, Joe Stringer  wrote:
> On systems that provide /proc/cpuinfo similar to Linux on x86, this
> should allow us to choose a better default value for the number of
> upcall handler threads---in particular, it avoids counting hyper-thread
> cores. If /proc/cpuinfo cannot be parsed for any reason, fall back to
> using sysconf().
...
> +if (!stream) {
> +VLOG_WARN("%s: open failed (%s)", file_name, ovs_strerror(errno));

The warning causes much of the testsuite to fail on systems that don't
have /proc/cpuinfo.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] timeval: On 64-bit systems refresh time whenever it is requested.

[Ed Maste]

>
> +/* On Linux IA64 systems, glibc avoids using syscalls for clock_gettime().
>

I assume you mean x86-64 or one of the equivalent names here, not IA64.


> +#ifndef __LP64__
> +#define CACHE_TIME 1
> +#endif
>

This will disable the syscall mitigation for 64-bit FreeBSD as well, which
is not (yet) desirable; I think this test needs a Linux check as well.

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Threaded userspace datapath

[Ed Maste]

The original developer of the BSD port (Gaetano Catalli) also implemented a
threaded version of the userspace datapath to increase performance.  As
with the original port, the threading work was done against Open vSwitch
1.1.0pre2.  Giuseppe Lettieri and I have ported it forward, and it's
functional and generally passes unit tests (with a few caveats).  The
threaded datapath has shown about 10x improvement in throughput in some
tests, when compared to the current userspace datapath.

There are still a few issues to work through, but I wanted to present the
patch now for discussion of the general approach.  The threaded patches are
available in the bsd-port-threaded branch in this repo:
https://github.com/emaste/openvswitch/tree/bsd-port-threaded

Or as a diff:
http://people.freebsd.org/~emaste/openvswitch/threaded.diff

The patch introduces a configure option to enable threaded mode:
--enable-threaded, and the changes are largely confined to #ifdef THREADED
blocks.  Two mutexes are added to dp_netdev: one for the flow table, and
one for the port list.  (I've also added a mutex over most of lib/vlog.c,
although a less conservative approach may be possible.)

Significant outstanding issues are:
1. Buffer ownership and management isn't really correct right now,
apparently done due to a desire to avoid extra copies.  This seems to be a
consequence of the standard BPF / libpcap interface.
2. The patch needs a better method of ensuring flows aren't deleted while
there may be a reference in the datapath thread.
3. The datapath thread waits in poll() with each port's file descriptor in
the fd array.  However there's currently no fd to signal a bridge
reconfiguration, so the poll() has to timeout before the thread will pick
up the new config on the next time through its loop.

In any case, I wanted to post the current patch for discussion and review
while working on the remaining issues.  I'm very interested in your
thoughts and comments.

-Ed
**
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] lib: Correct "old-style function definition" warning.

[Ed Maste]

Signed-off-by:  Ed Maste 
---
 lib/route-table-bsd.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/route-table-bsd.c b/lib/route-table-bsd.c
index 1c29071..eb4a168 100644
--- a/lib/route-table-bsd.c
+++ b/lib/route-table-bsd.c
@@ -104,7 +104,7 @@ route_table_get_name(ovs_be32 ip, char name[IFNAMSIZ])
 }
 
 void
-route_table_register()
+route_table_register(void)
 {
 if (!register_count)
 {
@@ -115,7 +115,7 @@ route_table_register()
 }
 
 void
-route_table_unregister()
+route_table_unregister(void)
 {
 register_count--;
 }
-- 
1.7.10.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] lib: Add header #include for writev

[Ed Maste]

This fixes a warning on FreeBSD.

Signed-off-by:  Ed Maste 
---
 lib/socket-util.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/socket-util.c b/lib/socket-util.c
index 314761d..7c40ab8 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "dynamic-string.h"
-- 
1.7.10.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev-bsd: Initialize variable to silence a compiler warning.

[Ed Maste]

FreeBSD's system compiler is a somewhat old version of GCC that produced
a spurious warning about a potential unitialized variable use.

Signed-off-by:  Ed Maste 
---
 lib/netdev-bsd.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 0b1a37c..f8b1188 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -453,7 +453,7 @@ netdev_bsd_listen(struct netdev *netdev_)
 struct netdev_bsd *netdev = netdev_bsd_cast(netdev_);
 char errbuf[PCAP_ERRBUF_SIZE];
 int error;
-int fd;
+int fd = -1;
 int one = 1;
 
 if (netdev->netdev_fd >= 0) {
-- 
1.7.10.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Threaded userspace datapath

[Ed Maste]

On 9 August 2012 11:56, Ben Pfaff  wrote:

> I'm curious about the performance improvement.  You mentioned a 10x
> performance improvement.  How much did CPU usage increase as part of
> that?  We already have users who complain when CPU usage spikes to 100%;
> I'm not sure that users will be happy if CPU usage can spike to 1000%
> (!).

Giuseppe did that basic benchmarking on Linux with the threaded
userspace patch and may have observed CPU usage statistics during that
work.  I don't have current numbers on the FreeBSD port, but will get
some soon.  Performance improvements are very important on FreeBSD of
course because we have only the userspace datapath.

> Oh I see, it looks like there is only one thread that does all packet
> processing?  I had a notion that there would be many threads, for
> example on a per-datapath or per-port basis.  It's very impressive
> getting 10x improvement with only a single additional thread.

Eventually I think a thread per datapath may be the way to go, and I
think the model in this patch can be reasonably extended to
per-datapath.

> I looked at the repository and then at the diff, both very briefly.  The
> repository looks more like development notes than something mergeable;
> the diff was easier to read.

Yes, the repo started from a snapshot patch against 1.1.0pre2 and has
evolved over time.  To get to something mergeable I'd expect to roll
it all up, or refactor into two or three committable pieces.

> One early reaction to the code is that there are too many #ifdefs
> (mainly for locking and unlocking mutexes).  I think that we should be
> able to remove many of them with a few well-chosen macros.

Indeed; the original 1.1.0pre2 work included this:

/* We could use these macros instead of using #ifdef and #endif every time we
 * need to call the pthread_mutex_lock/unlock.
#ifdef THREADED
#define LOCK(mutex) pthread_mutex_lock(mutuex)
#define UNLOCK(mutex) pthread_mutex_unlock(mutex)
#else
#define LOCK(mutex)
#define UNLOCK(mutex)
#endif

but then never used those macros.  I can switch it over to this scheme
if this is an agreeable approach (or define macros for each individual
lock, e.g. TABLE_LOCK and PORT_LOCK).

> OVS recently got a library, lib/worker.c, for implementing code in a
> "worker process".  So far, we're using that in a single-worker-process
> model, mostly for asynchronous logging, but we could generalize it so
> that there could be a second worker process used for datapath
> implementation.  If this would achieve acceptable performance, then it
> would avoid the difficulties of threads.  I guess the question there is
> how much communication a process model would require versus a thread
> model.

Right now packets that had a flow lookup miss are the only data
flowing from the datapath thread to the main thread, so that should be
easily done in either a thread or process model.  In the other
direction the datapath thread needs access to the flow table and I
think this would be a bit more awkward to implement in a process
model.  (On the other hand, a userland datapath process may be a
decent analogue to the Linux kernel module.)

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Threaded userspace datapath

[Ed Maste]

> We've had good improvements in a similar issue in dpif-linux by
> switching to use "epoll".  Can kqueue under FreeBSD do something
> similar?

Yes, epoll and kqueue basically provide the same functionality.  What
do you think about incorporating something like libevent to abstract
that?

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Threaded userspace datapath

[Ed Maste]

> There may be some confusion about numbering.  OpenFlow says that the
> port number of the "local port" is 65534 (OFPP_LOCAL).  The datapaths,
> on the other hand, use port number 0 for the "local port".  There is
> supposed to be translation going on at the interface between the
> datapath and the OpenFlow implementation, but mistakes can creep in,
> especially in new code.

Indeed this is the case here - I compared the key for the packet being
looked up and the otherwise-matching entry in the hash, and see 0 vs
65534 for the port.

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Threaded userspace datapath

[Ed Maste]

On 14 August 2012 21:21, Ed Maste  wrote:
>> There may be some confusion about numbering.  OpenFlow says that the
>> port number of the "local port" is 65534 (OFPP_LOCAL).  The datapaths,
>> on the other hand, use port number 0 for the "local port".  There is
>> supposed to be translation going on at the interface between the
>> datapath and the OpenFlow implementation, but mistakes can creep in,
>> especially in new code.
>
> Indeed this is the case here - I compared the key for the packet being
> looked up and the otherwise-matching entry in the hash, and see 0 vs
> 65534 for the port.
>
> -Ed

This fixes it for me, but I'm not certain this is the right place to
do the translation:

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 60fae5f..7112149 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1011,7 +1011,7 @@ dp_netdev_port_input(struct dp_netdev *dp,
struct dp_netdev_port *port,
 if (packet->size < ETH_HEADER_LEN) {
 return;
 }
-flow_extract(packet, 0, 0, port->port_no, &key);
+flow_extract(packet, 0, 0, ofp_port_to_odp_port(port->port_no), &key);
 flow = dp_netdev_lookup_flow(dp, &key);
 if (flow) {
 dp_netdev_flow_used(flow, &key, packet);
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Threaded userspace datapath

[Ed Maste]

>>> There may be some confusion about numbering.  OpenFlow says that the
>>> port number of the "local port" is 65534 (OFPP_LOCAL).  The datapaths,
>>> on the other hand, use port number 0 for the "local port".  There is
>>> supposed to be translation going on at the interface between the
>>> datapath and the OpenFlow implementation, but mistakes can creep in,
>>> especially in new code.
>>
>> Indeed this is the case here - I compared the key for the packet being
>> looked up and the otherwise-matching entry in the hash, and see 0 vs
>> 65534 for the port.
>>
>> -Ed
>
> This fixes it for me, but I'm not certain this is the right place to
> do the translation:
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 60fae5f..7112149 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1011,7 +1011,7 @@ dp_netdev_port_input(struct dp_netdev *dp,
> struct dp_netdev_port *port,
>  if (packet->size < ETH_HEADER_LEN) {
>  return;
>  }
> -flow_extract(packet, 0, 0, port->port_no, &key);
> +flow_extract(packet, 0, 0, ofp_port_to_odp_port(port->port_no), &key);
>  flow = dp_netdev_lookup_flow(dp, &key);
>  if (flow) {
>  dp_netdev_flow_used(flow, &key, packet);

Oops, I had it backwards in the version I posted - thanks Giuseppe for
pointing it out off-list.

This is the version that I tested and pushed to my threaded patch
repo, and Giuseppe confirmed it fixes it for him as well.  It looks
like this bug affects the userspace datapath in general and isn't
specific to the threaded patch or the FreeBSD netdev implementation.

+++ b/lib/dpif-netdev.c
@@ -1011,7 +1011,7 @@ dp_netdev_port_input(struct dp_netdev *dp,
struct dp_netdev_port *port,
 if (packet->size < ETH_HEADER_LEN) {
 return;
 }
-flow_extract(packet, 0, 0, port->port_no, &key);
+flow_extract(packet, 0, 0, odp_port_to_ofp_port(port->port_no), &key);
 flow = dp_netdev_lookup_flow(dp, &key);
 if (flow) {
 dp_netdev_flow_used(flow, &key, packet);

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev: Map to OpenFlow port for flow lookup

[Ed Maste]

In the flow hash special ports are stored using OpenFlow constants.
For example the "local port" is stored as 0xfffe (OFPP_LOCAL).

Signed-off-by: Ed Maste 
---
 lib/dpif-netdev.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 60fae5f..7fa2720 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1011,7 +1011,7 @@ dp_netdev_port_input(struct dp_netdev *dp, struct dp_netde
 if (packet->size < ETH_HEADER_LEN) {
 return;
 }
-flow_extract(packet, 0, 0, port->port_no, &key);
+flow_extract(packet, 0, 0, odp_port_to_ofp_port(port->port_no), &key);
 flow = dp_netdev_lookup_flow(dp, &key);
 if (flow) {
 dp_netdev_flow_used(flow, &key, packet);
-- 
1.7.10.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] dpif-netdev: Fix memory leak.

[Ed Maste]

On 15 August 2012 19:12, Ben Pfaff  wrote:
> upcall->packet is allocated with malloc(), via ofpbuf_new(), but nothing
> ever frees it.
>
> Found by valgrind.
>
> CC: Ed Maste 
> Signed-off-by: Ben Pfaff 
> ---
>  lib/dpif-netdev.c |1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 7fa2720..3d01b17 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -966,6 +966,7 @@ dpif_netdev_recv(struct dpif *dpif, struct dpif_upcall 
> *upcall,
>
>  ofpbuf_uninit(buf);
>  *buf = *upcall->packet;
> +free(upcall->packet);
>
>  return 0;
>  } else {
> --
> 1.7.2.5

This looks like it results in a use-after-free in dpif_recv which
accesses upcall->packet (which may be a moot point after the 2nd
patch; I'm going to look at it now).

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Eliminate two malloc() calls per packet sent to "userspace".

[Ed Maste]

On 15 August 2012 19:12, Ben Pfaff  wrote:
> This is easy enough, so it seems worthwhile now that FreeBSD is starting
> to make more use of the "userspace switch".
>
> CC: Ed Maste 
> Signed-off-by: Ben Pfaff 
> ---
>  lib/dpif-netdev.c |   33 +++--
>  1 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 3d01b17..144b6b6 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -70,8 +70,13 @@ enum { MAX_QUEUE_LEN = 128 };   /* Maximum number of 
> packets per queue. */
>  enum { QUEUE_MASK = MAX_QUEUE_LEN - 1 };
>  BUILD_ASSERT_DECL(IS_POW2(MAX_QUEUE_LEN));
>
> +struct dp_netdev_upcall {
> +struct dpif_upcall upcall;  /* Queued upcall information. */
> +struct ofpbuf buf;  /* ofpbuf instance for upcall.packet. */
> +};
> +
>  struct dp_netdev_queue {
> -struct dpif_upcall *upcalls[MAX_QUEUE_LEN];
> +struct dp_netdev_upcall upcalls[MAX_QUEUE_LEN];
>  unsigned int head, tail;
>  };
>
> @@ -259,10 +264,8 @@ dp_netdev_purge_queues(struct dp_netdev *dp)
>  struct dp_netdev_queue *q = &dp->queues[i];
>
>  while (q->tail != q->head) {
> -struct dpif_upcall *upcall = q->upcalls[q->tail++ & QUEUE_MASK];
> -
> -ofpbuf_delete(upcall->packet);
> -free(upcall);
> +struct dp_netdev_upcall *u = &q->upcalls[q->tail++ & QUEUE_MASK];
> +ofpbuf_uninit(&u->buf);
>  }
>  }
>  }
> @@ -960,13 +963,13 @@ dpif_netdev_recv(struct dpif *dpif, struct dpif_upcall 
> *upcall,
>  {
>  struct dp_netdev_queue *q = find_nonempty_queue(dpif);
>  if (q) {
> -struct dpif_upcall *u = q->upcalls[q->tail++ & QUEUE_MASK];
> -*upcall = *u;
> -free(u);
> +struct dp_netdev_upcall *u = &q->upcalls[q->tail++ & QUEUE_MASK];
> +
> +*upcall = u->upcall;
> +upcall->packet = buf;

Doesn't upcall->key still point into the static buf from upcalls[] in this case?

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] dpif-netdev: Fix memory leak.

[Ed Maste]

> From 3150ae27f6022aa6ffdfdf57f85808a5429f7a07 Mon Sep 17 00:00:00 2001
> From: Ben Pfaff 
> Date: Thu, 16 Aug 2012 08:36:42 -0700
> Subject: [PATCH] dpif-netdev: Fix memory leak.
>
> upcall->packet is allocated with malloc(), via ofpbuf_new(), but nothing
> ever frees it.
>
> Found by valgrind.
>
> CC: Ed Maste 
> Signed-off-by: Ben Pfaff 
> ---
>  lib/dpif-netdev.c |2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 7fa2720..63b59a3 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -966,6 +966,8 @@ dpif_netdev_recv(struct dpif *dpif, struct dpif_upcall 
> *upcall,
>
>  ofpbuf_uninit(buf);
>  *buf = *upcall->packet;
> +free(upcall->packet);
> +upcall->packet = buf;
>
>  return 0;
>  } else {
> --
> 1.7.2.5

This version looks good and passed a quick sanity test on my threaded branch.

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Eliminate two malloc() calls per packet sent to "userspace".

[Ed Maste]

On 16 August 2012 11:42, Ben Pfaff  wrote:
> On Thu, Aug 16, 2012 at 10:22:21AM -0400, Ed Maste wrote:
>> On 15 August 2012 19:12, Ben Pfaff  wrote:
>> > This is easy enough, so it seems worthwhile now that FreeBSD is starting
>> > to make more use of the "userspace switch".
>> >
>> > CC: Ed Maste 
>> > Signed-off-by: Ben Pfaff 
>> > ---
>> >  lib/dpif-netdev.c |   33 +++--
>> >  1 files changed, 19 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> > index 3d01b17..144b6b6 100644
>> > --- a/lib/dpif-netdev.c
>> > +++ b/lib/dpif-netdev.c
>> > @@ -70,8 +70,13 @@ enum { MAX_QUEUE_LEN = 128 };   /* Maximum number of 
>> > packets per queue. */
>> >  enum { QUEUE_MASK = MAX_QUEUE_LEN - 1 };
>> >  BUILD_ASSERT_DECL(IS_POW2(MAX_QUEUE_LEN));
>> >
>> > +struct dp_netdev_upcall {
>> > +struct dpif_upcall upcall;  /* Queued upcall information. */
>> > +struct ofpbuf buf;  /* ofpbuf instance for upcall.packet. */
>> > +};
>> > +
>> >  struct dp_netdev_queue {
>> > -struct dpif_upcall *upcalls[MAX_QUEUE_LEN];
>> > +struct dp_netdev_upcall upcalls[MAX_QUEUE_LEN];
>> >  unsigned int head, tail;
>> >  };
>> >
>> > @@ -259,10 +264,8 @@ dp_netdev_purge_queues(struct dp_netdev *dp)
>> >  struct dp_netdev_queue *q = &dp->queues[i];
>> >
>> >  while (q->tail != q->head) {
>> > -struct dpif_upcall *upcall = q->upcalls[q->tail++ & 
>> > QUEUE_MASK];
>> > -
>> > -ofpbuf_delete(upcall->packet);
>> > -free(upcall);
>> > +struct dp_netdev_upcall *u = &q->upcalls[q->tail++ & 
>> > QUEUE_MASK];
>> > +ofpbuf_uninit(&u->buf);
>> >  }
>> >  }
>> >  }
>> > @@ -960,13 +963,13 @@ dpif_netdev_recv(struct dpif *dpif, struct 
>> > dpif_upcall *upcall,
>> >  {
>> >  struct dp_netdev_queue *q = find_nonempty_queue(dpif);
>> >  if (q) {
>> > -struct dpif_upcall *u = q->upcalls[q->tail++ & QUEUE_MASK];
>> > -*upcall = *u;
>> > -free(u);
>> > +struct dp_netdev_upcall *u = &q->upcalls[q->tail++ & QUEUE_MASK];
>> > +
>> > +*upcall = u->upcall;
>> > +upcall->packet = buf;
>>
>> Doesn't upcall->key still point into the static buf from upcalls[] in
>> this case?
>
> Yes.  We've copied that buf into the function's argument and transferred
> ownership of its data to the caller.  The next time the static buf gets
> used it will be reinitialized with a newly allocated data buffer.

Ahh, of course.  This looks good to me.

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] netdev: Map to OpenFlow port for flow lookup

[Ed Maste]

On 15 August 2012 19:09, Ben Pfaff  wrote:
> On Wed, Aug 15, 2012 at 10:00:34PM +0000, Ed Maste wrote:
>> In the flow hash special ports are stored using OpenFlow constants.
>> For example the "local port" is stored as 0xfffe (OFPP_LOCAL).
>>
>> Signed-off-by: Ed Maste 
>
> Thanks, I applied this to master and branch-1.8.  I think it's
> probably applicable to earlier branches also, but since it's only a
> performance problem in a rare case on those branches I didn't want to
> make the change there without someone requesting it.

The Open vSwitch port in the FreeBSD ports tree[1] is currently based
on 1.7.0 so I wouldn't mind seeing the patch make it to branch-1.7 if
a 1.7.1 release is probable.  (The port contains a patch for
backmerging all of netdev-bsd already, so it's no big deal for us to
add this locally otherwise.)

[1] http://www.freshports.org/net/openvswitch/

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Threaded userspace datapath

[Ed Maste]

I've updated the snapshot of the threaded userspace datapath path with
improvements Giuseppe and I have done over the last couple of weeks.
The current patch is available here:

http://people.freebsd.org/~emaste/openvswitch/threaded-20120820.diff

This patch contains a number of minor improvements and cleanup in
here, relative to the last patch.

The major issues from last time:

> 1. Buffer ownership and management isn't really correct right now,
> apparently done due to a desire to avoid extra copies.  This seems to be a
> consequence of the standard BPF / libpcap interface.

This item is now addressed in the patch.

> 2. The patch needs a better method of ensuring flows aren't deleted while
> there may be a reference in the datapath thread.

Right now this is done by holding the flow table mutex over all of
dp_netdev_port_input, which will prevent future scalability gains.

I'm considering either adding a reference count increment _port_input,
or implementing a deferred deletion scheme (for example, doing the
flow deletion in the separate datapath thread).

> 3. The datapath thread waits in poll() with each port's file descriptor in
> the fd array.  However there's currently no fd to signal a bridge
> reconfiguration, so the poll() has to timeout before the thread will pick up
> the new config on the next time through its loop.

This still needs to be addressed.

-Ed
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] lib: Add xpipe_nonblocking helper

[Ed Maste]

Signed-off-by: Ed Maste 
---
As suggested by Ben Pfaff in the 'Threaded userspace datapath' thread.
This looks like a useful utility function refactoring, independent of
any outcome of that discussion.

 lib/fatal-signal.c | 4 +---
 lib/process.c  | 4 +---
 lib/signals.c  | 4 +---
 lib/socket-util.c  | 7 +++
 lib/socket-util.h  | 1 +
 5 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index 21ebb5a..7cfbd05 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -74,9 +74,7 @@ fatal_signal_init(void)

 inited = true;

-xpipe(signal_fds);
-xset_nonblocking(signal_fds[0]);
-xset_nonblocking(signal_fds[1]);
+xpipe_nonblocking(signal_fds);

 sigemptyset(&fatal_signal_set);
 for (i = 0; i < ARRAY_SIZE(fatal_signals); i++) {
diff --git a/lib/process.c b/lib/process.c
index 91dfc06..9f5c35f 100644
--- a/lib/process.c
+++ b/lib/process.c
@@ -82,9 +82,7 @@ process_init(void)
 inited = true;

 /* Create notification pipe. */
-xpipe(fds);
-xset_nonblocking(fds[0]);
-xset_nonblocking(fds[1]);
+xpipe_nonblocking(fds);

 /* Set up child termination signal handler. */
 memset(&sa, 0, sizeof sa);
diff --git a/lib/signals.c b/lib/signals.c
index b712f7e..152afcf 100644
--- a/lib/signals.c
+++ b/lib/signals.c
@@ -63,9 +63,7 @@ signal_init(void)
 static bool inited;
 if (!inited) {
 inited = true;
-xpipe(fds);
-xset_nonblocking(fds[0]);
-xset_nonblocking(fds[1]);
+xpipe_nonblocking(fds);
 }
 }

diff --git a/lib/socket-util.c b/lib/socket-util.c
index e32c03d..f8b44cc 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -894,6 +894,14 @@ xpipe(int fds[2])
 }

 void
+xpipe_nonblocking(int fds[2])
+{
+xpipe(fds);
+xset_nonblocking(fds[0]);
+xset_nonblocking(fds[1]);
+}
+
+void
 xsocketpair(int domain, int type, int protocol, int fds[2])
 {
 if (socketpair(domain, type, protocol, fds)) {
diff --git a/lib/socket-util.h b/lib/socket-util.h
index bacb236..a00b32e 100644
--- a/lib/socket-util.h
+++ b/lib/socket-util.h
@@ -64,6 +64,7 @@ int fsync_parent_dir(const char *file_name);
 int get_mtime(const char *file_name, struct timespec *mtime);

 void xpipe(int fds[2]);
+void xpipe_nonblocking(int fds[2]);

 char *describe_fd(int fd);

--
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] tests: Also enable FreeBSD libc debugging

[Ed Maste]

Signed-off-by: Ed Maste 
---
There should probably be an OS check to set only the appropriate environment
variables.  I'm not sure if there's a generic autotest way to do that while
building atlocal from atlocal.in though (vs. a switch on $(uname) in the
generated file).  Perhaps not worth addressing, since they're just env vars.

 tests/atlocal.in | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index 8d0f59d..c23f8e9 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -38,3 +38,7 @@ MALLOC_CHECK_=2
 MALLOC_PERTURB_=165
 export MALLOC_CHECK_
 export MALLOC_PERTURB_
+
+# Enable FreeBSD libc malloc debugging features.
+MALLOC_CONF=AJ
+export MALLOC_CONF
-- 
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] tests: Also enable FreeBSD libc debugging

[Ed Maste]

On 1 October 2012 17:19, Ben Pfaff  wrote:
> On Mon, Oct 01, 2012 at 09:11:31PM +0000, Ed Maste wrote:
>> Signed-off-by: Ed Maste 
>
> Applied, thanks.
>
>> There should probably be an OS check to set only the appropriate environment
>> variables.  I'm not sure if there's a generic autotest way to do that while
>> building atlocal from atlocal.in though (vs. a switch on $(uname) in the
>> generated file).  Perhaps not worth addressing, since they're just env vars.
>
> The latter was my own thought.

Ahh, as it turns out the new jemalloc in FreeBSD 10 uses a different
config string format, so I'll eventually need a uname switch anyway
(although the FreeBSD 10 release is a ways off).

Does something like this seem reasonable?

diff --git a/tests/atlocal.in b/tests/atlocal.in
index c23f8e9..5604bf0 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -33,12 +33,23 @@ if test $HAVE_PYTHON = yes; then
 fi
 fi

-# Enable glibc malloc debugging features.
-MALLOC_CHECK_=2
-MALLOC_PERTURB_=165
-export MALLOC_CHECK_
-export MALLOC_PERTURB_
-
-# Enable FreeBSD libc malloc debugging features.
-MALLOC_CONF=AJ
-export MALLOC_CONF
+case $(uname) in
+Linux)
+# Enable glibc malloc debugging features.
+MALLOC_CHECK_=2
+MALLOC_PERTURB_=165
+export MALLOC_CHECK_
+export MALLOC_PERTURB_
+;;
+FreeBSD)
+# Enable FreeBSD libc malloc debugging features.
+case $(uname -r) in
+8*|9*)
+MALLOC_CONF=AJ
+;;
+10*)
+MALLOC_CONF=abort:true,junk:true,redzone:true
+;;
+esac
+export MALLOC_CONF
+esac


> Do you guys use ovs-ctl (or plan to?).  I'd happily take a patch to
> ovs-lib to add freebsd support there.  I guess we'd either rename the
> "glibc" wrapper to something more generic or add a "bsd" or "freebsd"
> wrapper; I'd be OK with either choice.

We're not currently using ovs-ctl.  Probably the closet right now is
start scripts in the FreeBSD ports tree for the db and vswitchd:

http://svnweb.freebsd.org/ports/head/net/openvswitch/files/ovsdb-server.in?revision=302330&view=markup
http://svnweb.freebsd.org/ports/head/net/openvswitch/files/ovs-vswitchd.in?view=markup

On a quick look there are some interesting bits in ovs-ctl and ovs-lib
- I'll try to investigate in more detail.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] tests: jemalloc debug config for FreeBSD 9 and 10.

[Ed Maste]

Signed-off-by: Ed Maste 
---
 tests/atlocal.in | 28 +++-
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index c23f8e9..c736df4 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -33,12 +33,22 @@ if test $HAVE_PYTHON = yes; then
 fi
 fi

-# Enable glibc malloc debugging features.
-MALLOC_CHECK_=2
-MALLOC_PERTURB_=165
-export MALLOC_CHECK_
-export MALLOC_PERTURB_
-
-# Enable FreeBSD libc malloc debugging features.
-MALLOC_CONF=AJ
-export MALLOC_CONF
+# Enable malloc debugging features.
+case `uname` in
+Linux)
+MALLOC_CHECK_=2
+MALLOC_PERTURB_=165
+export MALLOC_CHECK_
+export MALLOC_PERTURB_
+;;
+FreeBSD)
+case `uname -r` in
+[789].*)
+MALLOC_CONF=AJ
+;;
+*)
+MALLOC_CONF=abort:true,junk:true,redzone:true
+;;
+esac
+export MALLOC_CONF
+esac
--
1.7.11.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Changing proc title on FreeBSD

[Ed Maste]

On 9 October 2012 17:18, Ben Pfaff  wrote:
> On Tue, Oct 09, 2012 at 01:54:28PM -0700, Ethan Jackson wrote:
>> > Does the 'argv' code in command-line.c actually malfunction on ESX?
>> > It'd be nice to just leave it in, if not.  It's not really tied to
>> > having the Linux datapath, it's orthogonal.  (It should actually work
>> > on many Unix and Unix-like systems, but we specialize it to Linux
>> > pending testing on other systems.)
>>
>> Yes I disabled it because I had verified that according to ps the
>> process title was not set.
>
> OK, thanks.

Interesting - I didn't notice this code before.

On FreeBSD we have setproctitle(const char *fmt, ...) for this
functionality, which has the same interface as proctitle_set.
Unfortunately there's no decent way to pass the varargs through.

I could add an #elif defined(__FreeBSD__) case to command-line.c and
use a temporary buffer in my proctitle_set implementation, or could
just #define proctitle_set setproctitle in command-line.h.  Thoughts
on which is preferable?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Changing proc title on FreeBSD

[Ed Maste]

On 9 October 2012 18:07, Ben Pfaff  wrote:
> On Tue, Oct 09, 2012 at 06:03:32PM -0400, Ed Maste wrote:
>>...
>>
>> Interesting - I didn't notice this code before.
>>
>> On FreeBSD we have setproctitle(const char *fmt, ...) for this
>> functionality, which has the same interface as proctitle_set.
>> Unfortunately there's no decent way to pass the varargs through.
>>
>> I could add an #elif defined(__FreeBSD__) case to command-line.c and
>> use a temporary buffer in my proctitle_set implementation, or could
>> just #define proctitle_set setproctitle in command-line.h.  Thoughts
>> on which is preferable?
>
> I'm happy enough with the latter; it's simpler, as long as it works.

Ahh, so there's one small caveat.  setproctitle internally prepends
the "program_name: " to the provided string so the resulting proctitle
ends up like:

ovs-vswitchd: ovs-vswitchd: worker process for pid 24853 (ovs-vswitchd)

The current proctitle_set callers already use this format and it seems
to be a reasonable restriction, so it would be possible to move the
program_name logic into proctitle_set for Linux as well.  If that's
undesirable I'll use temporary buffer in the FreeBSD implementation
and strip the program name back off before calling setproctitle.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev