[ovs-dev] ABI versioning (was Re: [PATCH] rhel: Support...)

[Aaron Conole]

>> > Should we bump the libtool version now?  We're nearing a release, so
>> > it's about the right time.
>> 
>> Sorry for the late response.  I think it's probably appropriate to bump
>> at least the libtool 'age'.  I'm not sure what kinds of ABI changes have
>> happened since introduction though - the interface 'major' version may
>> need to change.
>
> I'm pretty sure we've busted ABIs left and right since 2014, so a major
> version update should be appropriate.

I was about to post a patch that simply bumped this version, but it sent
me off onto a tangent leading to this question:

Will Open vSwitch support the libopenvswitch.so as an externally
linkable library?

I know there was some work done in this direction, but nothing for ABI
versioning or API compatibility has been done yet.

I've CCd folks who were on the original discussions for the various
series I've found (linked below).

I'm uncomfortable with bumping this and just 'let the chips fall where
they may,' since having a version that hasn't changed is the virtually
the same as not having a version.  The instant we bump, we state that
the version means something, so I'm not comfortable just shipping a
patch that changes the version without some accompanying documentation
explaining *what* that means.  It also means we need to be more diligent
with reviews and watch for potential ABI breakages, with a compatibility
strategy in place (when ABI/API changes can be made, how they are made,
etc.).  I think it's got more implication than just a single line change
to the configure.ac file.

Below are a few snippets I found;  there may be more discussions I've
missed, so please feel free to include links (or summaries).

Versioning patch (which mentions that it isn't yet intended to be for
  3rd party linking):
  https://mail.openvswitch.org/pipermail/ovs-dev/2014-November/291959.html

The first non-rfc 'third-party linking' series I could find (with note
  from Panu at least warning about this):
  https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/310703.html
  https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/310773.html

The series 'third party linking' series has all parts accepted:
  https://mail.openvswitch.org/pipermail/ovs-dev/2016-April/313183.html

-Aaron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5 1/3] netdev-dpdk: add hotplug support

[Stephen Finucane]

On Thu, 2017-01-05 at 10:42 +, Ciara Loftus wrote:
> From: Mauricio Vásquez 
> 
> In order to use dpdk ports in ovs they have to be bound to a DPDK
> compatible driver before ovs is started.
> 
> This patch adds the possibility to hotplug (or hot-unplug) a device
> after ovs has been started. The implementation adds two appctl
> commands:
> netdev-dpdk/attach and netdev-dpdk/detach
> 
> After the user attaches a new device, it has to be added to a bridge
> using the add-port command, similarly, before detaching a device,
> it has to be removed using the del-port command.
> 
> Signed-off-by: Mauricio Vasquez B  lito.it>
> Signed-off-by: Ciara Loftus 
> Co-authored-by: Ciara Loftus 

The docs aspects of all three patches all look a-OK to me now. Cheers
:)

Acked-by: Stephen Finucane   # docs only

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6] ovn-ctl: add support for SSL nb/sb db connections

[Ben Pfaff]

On Tue, Jan 03, 2017 at 01:29:10PM -0500, Lance Richardson wrote:
> Add support for SSL connections to OVN northbound and/or
> southbound databases.

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tunneling: Combining actions to avoid packet recirculation in tunnel encapsulation.

[Chandran, Sugesh]

Hi Jarno,

Thank you for looking into the path, 
Please find my answers inline blow.


Regards
_Sugesh


> -Original Message-
> From: Jarno Rajahalme [mailto:ja...@ovn.org]
> Sent: Wednesday, January 4, 2017 12:00 AM
> To: Chandran, Sugesh 
> Cc: zoltan.bal...@ericsson.com; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] tunneling: Combining actions to avoid packet
> recirculation in tunnel encapsulation.
> 
> Sugesh,
> 
> So the issue is that instead of recirculating on the datapath, we can compute
> the “recirculation” actions on the tunnel push translation time, as these
> actions only depend on the tunnel attributes? This explains why the
> recirculation in this case is unnecessary, and mentioning this in the commit
> message would make the motivation/correctness easier to understand.
[Sugesh] Make sense, Will update the commit message accordingly in the v2.
> 
> It seems to me that
> OVS_ACTION_ATTR_START_COMBINE/OVS_ACTION_ATTR_END_COMBINE
> semantics is to clone the packet and execute the actions in between. A
> cleaner way of doing this would be with (e.g.) “OVS_ACTION_ATTR_CLONE”
> that would be a nested attribute, the length of which would span to the end
> of the nested attributes (i.e., the actions produced by the tunnel output).
> This would be likely also faster to execute as the end does not need to be
> searched. Semantically this would be the same as the sample action with
> probability of 1, but simpler. Obviously the removal of the final cloning 
> could
> be avoided like now.
[Sugesh] Sure it make sense to take out the END_COMBINE action.
I am not very sure to call it as CLONE_ACTION as its not guarantee the packet 
cloning
in every time(For eg: the final action case). The new dummy actions are used to 
inform dp about set of actions that
are grouped/combined. These actions are two distinct set of actions which are 
merged to avoid recirculation in dp.
Can we call it OVS_ACTION_ATTR_COMBINE/OVS_ACTION_ATTR_GROUP/OVS_ACTION_ATTR_SET
than calling them CLONE.? 
> 
> Finally, translating the tunnel output looks a lot like translating patch port
> output. I wonder if it would be possible to share some of the code via some
> new helpers rather than duplicating it?
[Sugesh] Though both share same concept, the fields that used for combine are 
slightly different.
Do you think a single helper function for combine on translation will work for 
both path and tunnel.
This may need 'ifs' here and there in the function to do some of the field 
population and validation exclusively either for
tunneling or  patch port. Do you think its cleaner than having independent 
functions?
Anyway we will look into this possibility of combining these functions.
>   Jarno
> 
> > On Dec 30, 2016, at 7:36 AM, Sugesh Chandran
>  wrote:
> >
> > Currently the tunnel packets are recirculated in OVS after the tunnel
> > encapsulation. This patch combines the post tunnel actions with the
> > tunnel push to avoid unnecessary recirculation.
> > Two new datapath actions OVS_ATTR_COMBINE_START and
> > OVS_ATTR_COMBINE_END are added to implement this feature. These
> dummy
> > new actions  used as a delimiters for combined action subset.
> > In our testing this patch improves the VxLAN encapsulation performance
> upto 30%.
> >
> > Signed-off-by: Sugesh Chandran 
> > Signed-off-by: Zoltán Balogh 
> > Co-authored-by: Zoltán Balogh 
> > ---
> > datapath/linux/compat/include/linux/openvswitch.h |  2 +
> > lib/dpif-netdev.c | 22 ++
> > lib/dpif.c|  2 +
> > lib/odp-execute.c | 59 ++-
> > lib/odp-util.c| 68 -
> > lib/odp-util.h|  6 ++
> > ofproto/ofproto-dpif-sflow.c  |  2 +
> > ofproto/ofproto-dpif-xlate.c  | 89 
> > +++
> > 8 files changed, 228 insertions(+), 22 deletions(-)
> >
> > diff --git a/datapath/linux/compat/include/linux/openvswitch.h
> > b/datapath/linux/compat/include/linux/openvswitch.h
> > index 12260d8..4e9c7f7 100644
> > --- a/datapath/linux/compat/include/linux/openvswitch.h
> > +++ b/datapath/linux/compat/include/linux/openvswitch.h
> > @@ -818,6 +818,8 @@ enum ovs_action_attr { #ifndef __KERNEL__
> > OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct
> ovs_action_push_tnl*/
> > OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
> > +   OVS_ACTION_ATTR_START_COMBINE,
> > +   OVS_ACTION_ATTR_END_COMBINE,
> > #endif
> > __OVS_ACTION_ATTR_MAX,/* Nothing past this will be
> accepted
> >* from userspace. */
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > 0b73056..2e59bd1 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ 

Re: [ovs-dev] Questions about implementation of meters in ovs

[Ben Pfaff]

On Thu, Jan 05, 2017 at 08:15:06AM +, Hsu YuTing wrote:
> As far as i know that currently meters is not available in latest
> ovs release. Are there any alternative ways for experimenting the
> meters in ovs? Does anyone know when meters will be officially
> supported in ovs? Thanks!

Meters will be available when someone who wants them goes to the trouble
of implementing them.  So far, no one has wanted them badly enough to
contribute an implementation.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/9] expr: Rename "macros" to "addr_sets".

[Ben Pfaff]

On Wed, Jan 04, 2017 at 06:13:03PM -0800, Justin Pettit wrote:
> Macro is a very generic term, but the arguments are only ever address
> sets, so rename for clarity.
> 
> Signed-off-by: Justin Pettit 

Still some hits for "macro":

ovn/lib/expr.c:911:/* Adds a macro named 'name' to 'addr_sets', replacing any 
existing
ovn/lib/expr.c:926:/* Use the lexer to convert each macro into the 
proper
ovn/lib/lex.c:565:lex_parse_macro(const char *p, struct lex_token *token)
ovn/lib/lex.c:747:p = lex_parse_macro(p, token);

If you fix those,
Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/9] ovn-controller: Expose address sets to the main loop.

[Ben Pfaff]

On Wed, Jan 04, 2017 at 06:13:04PM -0800, Justin Pettit wrote:
> Other functions in the main loop will need access to address sets in a
> future commit.
> 
> Signed-off-by: Justin Pettit 

I'd prefer to limit the actual scope of the data structure, because that
makes it clear where the data is actually up-to-date.  Like this, for
example.

--8<--cut here-->8--

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 2533899..16d1ce2 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -261,11 +261,8 @@ get_chassis_id(const struct ovsdb_idl *ovs_idl)
 /* Iterate address sets in the southbound database.  Create and update the
  * corresponding symtab entries as necessary. */
 static void
-update_addr_sets(struct controller_ctx *ctx, struct shash *addr_sets)
-
+addr_sets_init(struct controller_ctx *ctx, struct shash *addr_sets)
 {
-expr_addr_sets_destroy(addr_sets);
-
 const struct sbrec_address_set *as;
 SBREC_ADDRESS_SET_FOR_EACH (as, ctx->ovnsb_idl) {
 expr_addr_sets_add(addr_sets, as->name,
@@ -545,8 +542,6 @@ main(int argc, char *argv[])
 unixctl_command_register("ct-zone-list", "", 0, 0,
  ct_zone_list, _zones);
 
-struct shash addr_sets = SHASH_INITIALIZER(_sets);
-
 /* Main loop. */
 exiting = false;
 while (!exiting) {
@@ -607,7 +602,9 @@ main(int argc, char *argv[])
 update_ct_zones(_lports, _datapaths, _zones,
 ct_zone_bitmap, _ct_zones);
 if (ctx.ovs_idl_txn) {
-update_addr_sets(, _sets);
+struct shash addr_sets = SHASH_INITIALIZER(_sets);
+addr_sets_init(, _sets);
+
 commit_ct_zones(br_int, _ct_zones);
 
 struct hmap flow_table = HMAP_INITIALIZER(_table);
@@ -621,6 +618,10 @@ main(int argc, char *argv[])
 ofctrl_put(_table, _ct_zones,
get_nb_cfg(ctx.ovnsb_idl));
 hmap_destroy(_table);
+
+expr_addr_sets_destroy(_sets);
+shash_destroy(_sets);
+
 if (ctx.ovnsb_idl_txn) {
 int64_t cur_cfg = ofctrl_get_cur_cfg();
 if (cur_cfg && cur_cfg != chassis->nb_cfg) {
@@ -710,7 +711,6 @@ main(int argc, char *argv[])
 pinctrl_destroy();
 
 simap_destroy(_zones);
-shash_destroy(_sets);
 
 bitmap_free(group_table.group_ids);
 hmap_destroy(_table.desired_groups);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 6/9] ofproto-macros: Quote "$@".

[Ben Pfaff]

On Wed, Jan 04, 2017 at 06:13:08PM -0800, Justin Pettit wrote:
> Quote "$@" so that arguments aren't split when being called.
> 
> Signed-off-by: Justin Pettit 

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 8/9] ovn-test: Add 'expr-to-packets' command.

[Ben Pfaff]

On Wed, Jan 04, 2017 at 06:13:10PM -0800, Justin Pettit wrote:
> Parses OVN expressions from stdin and prints out matching packets in
> hexadecimal on stdout.
> 
> Signed-off-by: Justin Pettit 

'\0' looks weird here, a flow is not a character string:
struct flow uflow;
memset(, '\0', sizeof uflow);
But you can just remove the memset() call entirely because
expr_parse_microflow() always initializes the whole flow.

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] python: Add TCP/SSL probes for OVSDB python lib

[Ben Pfaff]

On Thu, Jan 05, 2017 at 09:58:11AM -0500, Russell Bryant wrote:
> On Wed, Jan 4, 2017 at 6:41 PM, Ben Pfaff  wrote:
> 
> > On Sun, Jan 01, 2017 at 07:04:55PM +0800, Guoshuai Li wrote:
> > > stream_or_pstream_needs_probes always return 0. This causes TCP/SSL
> > > connection not be probed, and no reconnect when the connection
> > > is aborted
> > >
> > > Signed-off-by: Guoshuai Li 
> >
> > Applied to master, thanks!
> 
> Are you OK with backporting this to branch-2.6?  I don't mind doing it if
> so.

Fine with me.  I considered it but at least superficially it had patch
rejects and I did not look further.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 5/9] ovs-sim: Quote "$@".

[Ben Pfaff]

On Wed, Jan 04, 2017 at 06:13:07PM -0800, Justin Pettit wrote:
> Quote "$@" so that arguments aren't split when being called.
> 
> Signed-off-by: Justin Pettit 

It must have been nightmarish finding this.

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/9] flow: Fix small typo in comment describing flow_compose().

[Ben Pfaff]

On Wed, Jan 04, 2017 at 06:13:06PM -0800, Justin Pettit wrote:
> Signed-off-by: Justin Pettit 

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 7/9] ovn-controller: Introduce "inject-pkt" ovs-appctl command.

[Ben Pfaff]

On Wed, Jan 04, 2017 at 06:13:09PM -0800, Justin Pettit wrote:
> Add the ability to inject a packet into the connected Open vSwitch
> instance.  This is primarily useful for testing when a test requires
> side-effects from an actual packet, so ovn-trace won't do.
> 
> Signed-off-by: Justin Pettit 

This looks quite useful.  Thank you.

ofctrl_inject_packet() should check that rconn_get_version() does not
return -1, because that indicates that we can't encode or send a packet
right now.

At least in the long run we might want to break the pending_pkt handling
out of main() into help functions.

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] doc: Use ovs theme

[Stephen Finucane]

On Wed, 2017-01-04 at 10:32 -0800, Joe Stringer wrote:
> On 4 January 2017 at 01:03, Stephen Finucane 
> wrote:
> > On Tue, 2017-01-03 at 21:33 +, Stephen Finucane wrote:
> > > The recently published 'ovs' theme [1] copies the styling of the
> > > Open
> > > vSwitch website. Start using this, with fallbacks for users who
> > > do
> > > not
> > > have the package installed.
> > > 
> > > This extends support for building docs to users of Sphinx 1.2 as
> > > the
> > > previous theme - bizstyle - was only available in 1.3+.
> > 
> > Oops - forgot [1].
> > 
> > [1] https://pypi.python.org/pypi/ovs-sphinx-theme
> 
> Looks good!
> 
> One minor nit - on the main OVS website, the logo links to the main
> website; on the docs page, it links to the docs home page. Is there a
> way to make this more consistent?

Cheers :) I've pushed an updated version that resolves this (along with
some funky styling of the logo when viewing on a mobile device). Should
see it soon as the docs are rebuilt.

Stephen
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] python: Add TCP/SSL probes for OVSDB python lib

[Russell Bryant]

On Wed, Jan 4, 2017 at 6:41 PM, Ben Pfaff  wrote:

> On Sun, Jan 01, 2017 at 07:04:55PM +0800, Guoshuai Li wrote:
> > stream_or_pstream_needs_probes always return 0. This causes TCP/SSL
> > connection not be probed, and no reconnect when the connection
> > is aborted
> >
> > Signed-off-by: Guoshuai Li 
>
> Applied to master, thanks!


Are you OK with backporting this to branch-2.6?  I don't mind doing it if
so.

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] ovn support DPDK?

[Ben Pfaff]

I'm glad to hear that.  I have not heard reports before from OVN users
who run it on DPDK, so it is good to know that it works.

On Thu, Jan 05, 2017 at 04:35:00PM +0800, 赵占旭 wrote:
> Thanks very much!
> With your advice, I resolve the problem.
> I thought ovn only support one bridge: br-int, now I add one.
> 
> 
> 
> 
> 
> 
> 
> 
> At 2017-01-05 01:44:26, "Ben Pfaff"  wrote:
> >On Tue, Jan 03, 2017 at 05:25:26PM +0800, 赵占旭 wrote:
> >> I installed ovn and ovs-dpdk. when I created two VMs on one host, they 
> >> could communicate, but I created two VMs on diffrent hosts, they couldn't 
> >> communicate.
> >> I think the problem is geneve, ovs and ovs-dpdk has different flows, so my 
> >> question is the ovn support dpdk?
> >> I need to configure sth, or I need patch, or I need to modify code??? orz
> >
> >Probably, you need to configure a physical bridge and set up routes, so
> >that OVS-DPDK can properly tunnel packets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] util: Remove obsolete comment.

[Ben Pfaff]

On Thu, Jan 05, 2017 at 09:17:22AM +0100, Simon Horman wrote:
> On Wed, Jan 04, 2017 at 09:23:54AM -0800, Ben Pfaff wrote:
> > Signed-off-by: Ben Pfaff 
> 
> Acked-by: Simon Horman 

Thanks, applied.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/9] ovn-controller: Remove "_p" from pointer arguments in lflow.c.

[Ben Pfaff]

On Wed, Jan 04, 2017 at 06:13:05PM -0800, Justin Pettit wrote:
> This more closely follows our coding standards.
> 
> Signed-off-by: Justin Pettit 

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH net-next] net: make ndo_get_stats64 a void function

[Stephen Hemminger]

The network device operation for reading statistics is only called
in one place, and it ignores the return value. Having a structure
return value is potentially confusing because some future driver could
incorrectly assume that the return value was used.

Fix all drivers with ndo_get_stats64 to have a void function.

Signed-off-by: Stephen Hemminger 
---
 drivers/net/bonding/bond_main.c  | 10 --
 drivers/net/dummy.c  |  5 ++---
 drivers/net/ethernet/alacritech/slicoss.c|  6 ++
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 10 --
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c |  6 ++
 drivers/net/ethernet/apm/xgene/xgene_enet_main.c |  4 +---
 drivers/net/ethernet/atheros/alx/main.c  |  6 ++
 drivers/net/ethernet/broadcom/b44.c  |  5 ++---
 drivers/net/ethernet/broadcom/bnx2.c |  3 +--
 drivers/net/ethernet/broadcom/bnxt/bnxt.c|  6 ++
 drivers/net/ethernet/broadcom/tg3.c  |  8 +++-
 drivers/net/ethernet/brocade/bna/bnad.c  |  6 ++
 drivers/net/ethernet/calxeda/xgmac.c |  5 ++---
 drivers/net/ethernet/cavium/thunder/nicvf_main.c |  5 ++---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c  |  7 +++
 drivers/net/ethernet/cisco/enic/enic_main.c  |  8 +++-
 drivers/net/ethernet/ec_bhf.c|  4 +---
 drivers/net/ethernet/emulex/benet/be_main.c  |  5 ++---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c   |  6 ++
 drivers/net/ethernet/hisilicon/hns/hns_enet.c|  6 ++
 drivers/net/ethernet/ibm/ehea/ehea_main.c|  5 ++---
 drivers/net/ethernet/intel/e1000e/e1000.h|  4 ++--
 drivers/net/ethernet/intel/e1000e/netdev.c   |  5 ++---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c  |  6 ++
 drivers/net/ethernet/intel/i40e/i40e.h   |  5 ++---
 drivers/net/ethernet/intel/i40e/i40e_main.c  | 18 ++
 drivers/net/ethernet/intel/igb/igb_main.c| 10 --
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c|  7 ---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c|  6 ++
 drivers/net/ethernet/marvell/mvneta.c|  4 +---
 drivers/net/ethernet/marvell/mvpp2.c |  4 +---
 drivers/net/ethernet/marvell/sky2.c  |  6 ++
 drivers/net/ethernet/mediatek/mtk_eth_soc.c  |  6 ++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c   |  4 +---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c|  3 +--
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c |  3 +--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c   |  4 +---
 drivers/net/ethernet/mellanox/mlxsw/switchx2.c   |  3 +--
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c |  9 -
 drivers/net/ethernet/neterion/vxge/vxge-main.c   |  4 +---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c  |  6 ++
 drivers/net/ethernet/nvidia/forcedeth.c  |  4 +---
 drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c | 10 --
 drivers/net/ethernet/qlogic/qede/qede_main.c |  7 ++-
 drivers/net/ethernet/qualcomm/emac/emac.c|  6 ++
 drivers/net/ethernet/realtek/8139too.c   |  9 +++--
 drivers/net/ethernet/realtek/r8169.c |  4 +---
 drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c  |  8 ++--
 drivers/net/ethernet/sfc/efx.c   |  6 ++
 drivers/net/ethernet/sfc/falcon/efx.c|  6 ++
 drivers/net/ethernet/sun/niu.c   |  6 ++
 drivers/net/ethernet/synopsys/dwc_eth_qos.c  |  4 +---
 drivers/net/ethernet/tile/tilepro.c  |  4 ++--
 drivers/net/ethernet/via/via-rhine.c |  8 +++-
 drivers/net/fjes/fjes_main.c |  7 ++-
 drivers/net/hyperv/netvsc_drv.c  |  6 ++
 drivers/net/ifb.c|  6 ++
 drivers/net/ipvlan/ipvlan_main.c |  5 ++---
 drivers/net/loopback.c   |  5 ++---
 drivers/net/macsec.c |  6 ++
 drivers/net/macvlan.c|  5 ++---
 drivers/net/nlmon.c  |  4 +---
 drivers/net/ppp/ppp_generic.c|  4 +---
 drivers/net/slip/slip.c  |  3 +--
 drivers/net/team/team.c  |  3 +--
 drivers/net/tun.c|  3 +--
 drivers/net/veth.c   |  6 ++
 drivers/net/virtio_net.c |  6 ++
 drivers/net/vmxnet3/vmxnet3_ethtool.c|  4 +---
 drivers/net/vmxnet3/vmxnet3_int.h|  4 ++--
 

Re: [ovs-dev] [PATCH] Windows tests: Applications abort when using threading (ovs_rwlock_init)

[Alin Serdean]

> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Wednesday, January 4, 2017 7:21 PM
> To: Alin Serdean 
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] Windows tests: Applications abort when
> using threading (ovs_rwlock_init)
> 
> On Wed, Dec 28, 2016 at 10:27:17PM +, Alin Serdean wrote:
> > Commit number: 1a15f390afd66efd161db78b86600832214dfb3c
> >
> https://github.com/openvswitch/ovs/commit/1a15f390afd66efd161db78b86
> 60
> > 0832214dfb3c switched from `NULL` to `attr`. According to POSIX
> > documentation this is correct, unfortunately on Windows the current
> > implementation of pthreads does not support a pre-initialized
> > attribute. Please see a fork of the implementation
> > https://github.com/GerHobbelt/pthread-
> win32/blob/19fd5054b29af1b4e3b32
> > 78bfffbb6274c6c89f5/pthread_rwlock_init.c#L59-L63
> > This is the same implementation as the official version found under:
> > ftp://sourceware.org/pub/pthreads-win32/)
> 
> Thanks.
> 
> I didn't like that this was Windows-specific, since it was possible to make it
> generic.
> 
> I changed the patch to the following, and applied it to master.
> 
[Alin Serdean] Much nicer. Thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Conntrack: Fix L4 Checksums in kernel <4.6 when using NAT and helpers

[John Hurley]

Ok, this makes sense now. Ive updated and tested the patch:


From 93bc4c59b4eb814ed4bc3da3b7459498863add1e Mon Sep 17 00:00:00 2001
From: John Hurley 
Date: Thu, 5 Jan 2017 17:08:37 +
Subject: [PATCH 1/1] ensure correct checksum when a packet is sent to NAT

---
 datapath/conntrack.c | 47 +--
 1 file changed, 17 insertions(+), 30 deletions(-)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index d942884..3faa209 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -314,6 +314,11 @@ static int ovs_ct_helper(struct sk_buff *skb, u16
proto)
  u8 nexthdr;
  int err;

+#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
+ bool dst_set = false;
+ struct rtable rt = { .rt_flags = 0 };
+#endif
+
  ct = nf_ct_get(skb, );
  if (!ct || ctinfo == IP_CT_RELATED_REPLY)
  return NF_ACCEPT;
@@ -352,43 +357,25 @@ static int ovs_ct_helper(struct sk_buff *skb, u16
proto)
 #if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
  /* Linux 4.5 and older depend on skb_dst being set when recalculating
  * checksums after NAT helper has mangled TCP or UDP packet payload.
- * This dependency is avoided when skb is CHECKSUM_PARTIAL or when UDP
- * has no checksum.
- *
- * The dependency is not triggered when the main NAT code updates
- * checksums after translating the IP header (address, port), so this
- * fix only needs to be executed on packets that are both being NATted
- * and that have a helper assigned.
+ * skb_dst is cast to a rtable struct and the flags examined.
+ * Forcing these flags to have RTCF_LOCAL not set ensures checksum mod
+ * is carried out in the same way as kernel versions > 4.5
  */
- if (ct->status & IPS_NAT_MASK && skb->ip_summed != CHECKSUM_PARTIAL) {
- u8 ipproto = (proto == NFPROTO_IPV4)
- ? ip_hdr(skb)->protocol : nexthdr;
- u16 offset = 0;
-
- switch (ipproto) {
- case IPPROTO_TCP:
- offset = offsetof(struct tcphdr, check);
- break;
- case IPPROTO_UDP:
- /* Skip if no csum. */
- if (udp_hdr(skb)->check)
- offset = offsetof(struct udphdr, check);
- break;
- }
- if (offset) {
- if (unlikely(!pskb_may_pull(skb, protoff + offset + 2)))
- return NF_DROP;
-
- skb->csum_start = skb_headroom(skb) + protoff;
- skb->csum_offset = offset;
- skb->ip_summed = CHECKSUM_PARTIAL;
- }
+ if (ct->status & IPS_NAT_MASK && skb->ip_summed != CHECKSUM_PARTIAL
+ && !skb_rtable(skb)) {
+ dst_set = true;
+ skb_dst_set(skb, (struct dst_entry *));
  }
 #endif
  err = helper->help(skb, protoff, ct, ctinfo);
  if (err != NF_ACCEPT)
  return err;

+#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
+ if (dst_set)
+ skb_dst_set(skb, NULL);
+#endif
+
  /* Adjust seqs after helper.  This is needed due to some helpers (e.g.,
  * FTP with NAT) adusting the TCP payload size when mangling IP
  * addresses and/or port numbers in the text-based control connection.
-- 
2.7.4


On Wed, Jan 4, 2017 at 9:33 PM, Jarno Rajahalme  wrote:

>
> On Jan 4, 2017, at 9:50 AM, John Hurley  wrote:
>
> From 64ce83672aab5634990426e0a51af16d882ac2f9 Mon Sep 17 00:00:00 2001
> From: John Hurley 
> Date: Wed, 4 Jan 2017 16:52:43 +
> Subject: [PATCH] ensure correct checksum when a packet is sent to NAT
> helper
>  in kernel < 4.6
>
> ---
>  datapath/conntrack.c | 45 -
>  1 file changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index d942884..fa3b0b5 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -314,6 +314,10 @@ static int ovs_ct_helper(struct sk_buff *skb, u16
> proto)
>   u8 nexthdr;
>   int err;
>
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
> + struct rtable rt;
> +#endif
> +
>
>
> This should be properly initialized (e.g., " = { .rt_flags = 0 };”). With
> this RFCF_LOCAL will be unset (see below) and everything else will also be
> initialized to 0.
>
>   ct = nf_ct_get(skb, );
>   if (!ct || ctinfo == IP_CT_RELATED_REPLY)
>   return NF_ACCEPT;
> @@ -352,43 +356,26 @@ static int ovs_ct_helper(struct sk_buff *skb, u16
> proto)
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
>   /* Linux 4.5 and older depend on skb_dst being set when recalculating
>   * checksums after NAT helper has mangled TCP or UDP packet payload.
> - * This dependency is avoided when skb is CHECKSUM_PARTIAL or when UDP
> - * has no checksum.
> - *
> - * The dependency is not triggered when the main NAT code updates
> - * checksums after translating the IP header (address, port), so this
> - * fix only needs to be executed on packets that are both being NATted
> - * and that have a helper assigned.
> + * skb_dst is cast to a rtable struct and the flags examined.
> + * Forcing these flags to have RTCF_LOCAL set allows checksum mod
> + * to be carried out in the same way as kernel versions > 4.5
>
>
> As noted in the previous email, the reverse is the case. When RTCF_LOCAL
> is not set, then the checksum update 

Re: [ovs-dev] [PATCH net-next] net: make ndo_get_stats64 a void function

[David Miller]

From: Stephen Hemminger 
Date: Thu,  5 Jan 2017 09:31:36 -0800

> The network device operation for reading statistics is only called
> in one place, and it ignores the return value. Having a structure
> return value is potentially confusing because some future driver could
> incorrectly assume that the return value was used.
> 
> Fix all drivers with ndo_get_stats64 to have a void function.
> 
> Signed-off-by: Stephen Hemminger 

You missed at least one new warning, please do a fresh allmodconfig build
and watch the logs.

drivers/net/ethernet/broadcom/bnx2.c: In function ‘bnx2_get_stats64’:
drivers/net/ethernet/broadcom/bnx2.c:6830:10: warning: ‘return’ with a value, 
in function returning void

Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovs V2 09/21] dpif-netlink: Dump netdevs flows on flow dump

[Joe Stringer]

On 25 December 2016 at 03:39, Paul Blakey  wrote:
> While dumping flows, dump flows that were offloaded to
> netdev and parse them back to dpif flow.
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> ---
>  lib/dpif-netlink.c | 179 
> +
>  1 file changed, 179 insertions(+)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 36f2888..3d8940e 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -38,6 +38,7 @@
>  #include "flow.h"
>  #include "fat-rwlock.h"
>  #include "netdev.h"
> +#include "netdev-provider.h"
>  #include "netdev-linux.h"
>  #include "netdev-vport.h"
>  #include "netlink-conntrack.h"
> @@ -55,6 +56,7 @@
>  #include "unaligned.h"
>  #include "util.h"
>  #include "openvswitch/vlog.h"
> +#include "openvswitch/match.h"
>
>  VLOG_DEFINE_THIS_MODULE(dpif_netlink);
>  #ifdef _WIN32
> @@ -68,6 +70,8 @@ enum { MAX_PORTS = USHRT_MAX };
>   * missing if we have old headers. */
>  #define ETH_FLAG_LRO  (1 << 15)/* LRO is enabled */
>
> +#define FLOW_DUMP_MAX_BATCH 50
> +
>  struct dpif_netlink_dp {
>  /* Generic Netlink header. */
>  uint8_t cmd;
> @@ -1355,6 +1359,10 @@ struct dpif_netlink_flow_dump {
>  struct dpif_flow_dump up;
>  struct nl_dump nl_dump;
>  atomic_int status;
> +struct netdev_flow_dump **netdev_dumps;
> +int netdev_num;
> +int netdev_given;
> +struct ovs_mutex netdev_lock;

Could you add a brief comment above these variables that describes
their use? (It's also common in OVS code to mention that, eg,
netdev_lock protects the following elements. )

If there's a more descriptive name than "netdev_num", like
netdev_max_dumps or something then please use that instead. At a
glance, "given" and "num" don't provide particularly much context
about how they relate to each other or to the dump.

>  };
>
>  static struct dpif_netlink_flow_dump *
> @@ -1363,6 +1371,34 @@ dpif_netlink_flow_dump_cast(struct dpif_flow_dump 
> *dump)
>  return CONTAINER_OF(dump, struct dpif_netlink_flow_dump, up);
>  }
>
> +static void start_netdev_dump(const struct dpif *dpif_,
> +  struct dpif_netlink_flow_dump *dump) {
> +
> +if (!netdev_flow_api_enabled) {
> +dump->netdev_num = 0;
> +return;
> +}

Typically for style we still place all variable declarations at the
top of a function, in a christmas tree long lines to short lines,
before functional code like this.

> +
> +struct netdev_list_element *element;
> +struct ovs_list port_list;
> +int ports = netdev_hmap_port_get_list(dpif_->dpif_class, _list);
> +int i = 0;
> +
> +dump->netdev_dumps =
> +ports ? xzalloc(sizeof(struct netdev_flow_dump *) * ports) : 0;

Can this be sizeof(dump->netdev_dumps)?

> +dump->netdev_num = ports;
> +dump->netdev_given = 0;
> +
> +LIST_FOR_EACH(element, node, _list) {
> +dump->netdev_dumps[i] = netdev_flow_dump_create(element->netdev);
> +dump->netdev_dumps[i]->port = element->port_no;
> +i++;
> +}

As a matter of style, it's easier to see that this loop is bounded by
'ports' (and that number is correct) if it's structured as

for (i = 0; i < ports; i++) {
element = get_next_node;
...
}

Also, it seems that even if the netdev doesn't support flow_dump, we
allocate a netdev_flow_dump and add it to the netdev_dumps here..
perhaps we could/should skip it for these netdevs instead?

> +netdev_port_list_del(_list);
> +
> +ovs_mutex_init(>netdev_lock);

I don't see a corresponding ovs_mutex_destroy() call for this.

> +}
> +
>  static struct dpif_flow_dump *
>  dpif_netlink_flow_dump_create(const struct dpif *dpif_, bool terse)
>  {
> @@ -1387,6 +1423,8 @@ dpif_netlink_flow_dump_create(const struct dpif *dpif_, 
> bool terse)
>  atomic_init(>status, 0);
>  dump->up.terse = terse;
>
> +start_netdev_dump(dpif_, dump);
> +
>  return >up;
>  }
>
> @@ -1397,6 +1435,16 @@ dpif_netlink_flow_dump_destroy(struct dpif_flow_dump 
> *dump_)
>  unsigned int nl_status = nl_dump_done(>nl_dump);
>  int dump_status;
>
> +if (netdev_flow_api_enabled) {
> +for (int i = 0; i < dump->netdev_num; i++) {
> +int err = netdev_flow_dump_destroy(dump->netdev_dumps[i]);
> +if (err != 0 && err != EOPNOTSUPP) {
> +VLOG_ERR("failed dumping netdev: %s", ovs_strerror(err));
> +}
> +}
> +free(dump->netdev_dumps);
> +}

You don't really need to check for netdev_flow_api_enabled here;
netdev_num will be 0 if it is disabled, so that for loop turns into a
no-op; then you could initialize dump->netdev_dumps to NULL in that
case and unconditionally free it. It's a bit simpler to read the code
if you don't have to think about whether or not hardware offloads are
enabled.

> +
>  /* No other thread has access to 'dump' at this point. */
>  

Re: [ovs-dev] [PATCH ovs V2 20/21] netdev-linux: always add ingress qdisc

[Joe Stringer]

On 25 December 2016 at 03:39, Paul Blakey  wrote:
> flow offloading by tc needs ingress qdisc on the device.
> Deleting the ingress qdisc was done in order to flush
> policing filters, so instead we just flush the filter and
> leave the ingress added (and add it if there wasn't any).
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 

I'm concerned that this patch will break a bunch of things.

I don't think we generally want an ingress qdisc unless there is
specific policing configuration, or hardware offloads is enabled. This
looks like it sets it up regardless.

I suspect that if you changed policing configuration, it will not take
effect with this patch.

Hardware offloads is orthogonal to policing; I suggest that there
should be a new netdev-provider API to enable hardware offloads, which
will ensure that the qdiscs are set up appropriately.

If policing is configured with a rate/burst, I would expect this to
take effect. If this policing is not possible to be offloaded to
hardware, then I'd expect either OVS complains in the logs that
hardware offloads are configured with policing, and describe how it
works - either A), policing is ignored and flows are offloaded, or B)
policing works, but flows are not offloaded.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovs V2 19/21] dpctl: read vswitch config on start

[Joe Stringer]

On 25 December 2016 at 03:39, Paul Blakey  wrote:
> Use Open vSwitch IDL pattern to read OVS configuration on dpctl start,
> needed as some functionality is dependent on that configuration.
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 

Why? What functionality?

What if OVSDB isn't running, does ovs-dpctl block?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 9/9] ovn.at: Rewrite a test using ovn-controller 'inject-pkt' command.

[Justin Pettit]

> On Jan 5, 2017, at 7:48 AM, Ben Pfaff  wrote:
> 
> On Wed, Jan 04, 2017 at 06:13:11PM -0800, Justin Pettit wrote:
>> Provide an example of using ovn-controller 'inject-pkt' and ovn-test
>> 'expr-to-packets' commands to generate and verify proper handling of
>> packets.  Tests written in this way should be easier to understand than
>> raw packets written in hexadecimal.
>> 
>> Signed-off-by: Justin Pettit 
> 
> Much nicer.  Thank you.
> 
> Acked-by: Ben Pfaff 

Thanks for the reviews.  I've pushed the series to master.

--Justin



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Conntrack: Fix L4 Checksums in kernel <4.6 when using NAT and helpers

[Jarno Rajahalme]

John,

This looks functionally OK, but could you repost this with the following 
changes:

- Include a commit message explaining what was broken and how it is fixed.
- Refer to the commit that introduced the deleted code with a "Fixes:  
(“Title”)” line.
- Include a "Signed-off-by:” -line.


> On Jan 5, 2017, at 9:12 AM, John Hurley  wrote:
> 
> Ok, this makes sense now. Ive updated and tested the patch:
> 

Include info about the kernel versions in which you tested to the commit 
message.

Then some final nits below,

  Jarno

> 
> From 93bc4c59b4eb814ed4bc3da3b7459498863add1e Mon Sep 17 00:00:00 2001
> From: John Hurley  >
> Date: Thu, 5 Jan 2017 17:08:37 +
> Subject: [PATCH 1/1] ensure correct checksum when a packet is sent to NAT
> 
> ---
>  datapath/conntrack.c | 47 +--
>  1 file changed, 17 insertions(+), 30 deletions(-)
> 
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index d942884..3faa209 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -314,6 +314,11 @@ static int ovs_ct_helper(struct sk_buff *skb, u16 proto)
>   u8 nexthdr;
>   int err;
>  
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
> + bool dst_set = false;
> + struct rtable rt = { .rt_flags = 0 };
> +#endif
> +
>   ct = nf_ct_get(skb, );
>   if (!ct || ctinfo == IP_CT_RELATED_REPLY)
>   return NF_ACCEPT;
> @@ -352,43 +357,25 @@ static int ovs_ct_helper(struct sk_buff *skb, u16 proto)
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
>   /* Linux 4.5 and older depend on skb_dst being set when recalculating
>* checksums after NAT helper has mangled TCP or UDP packet payload.
> -  * This dependency is avoided when skb is CHECKSUM_PARTIAL or when UDP
> -  * has no checksum.
> -  *
> -  * The dependency is not triggered when the main NAT code updates
> -  * checksums after translating the IP header (address, port), so this
> -  * fix only needs to be executed on packets that are both being NATted
> -  * and that have a helper assigned.
> +  * skb_dst is cast to a rtable struct and the flags examined.
> +  * Forcing these flags to have RTCF_LOCAL not set ensures checksum mod
> +  * is carried out in the same way as kernel versions > 4.5
>*/
> - if (ct->status & IPS_NAT_MASK && skb->ip_summed != CHECKSUM_PARTIAL) {
> - u8 ipproto = (proto == NFPROTO_IPV4)
> - ? ip_hdr(skb)->protocol : nexthdr;
> - u16 offset = 0;
> -
> - switch (ipproto) {
> - case IPPROTO_TCP:
> - offset = offsetof(struct tcphdr, check);
> - break;
> - case IPPROTO_UDP:
> - /* Skip if no csum. */
> - if (udp_hdr(skb)->check)
> - offset = offsetof(struct udphdr, check);
> - break;
> - }
> - if (offset) {
> - if (unlikely(!pskb_may_pull(skb, protoff + offset + 2)))
> - return NF_DROP;
> -
> - skb->csum_start = skb_headroom(skb) + protoff;
> - skb->csum_offset = offset;
> - skb->ip_summed = CHECKSUM_PARTIAL;
> - }
> + if (ct->status & IPS_NAT_MASK && skb->ip_summed != CHECKSUM_PARTIAL
> + && !skb_rtable(skb)) {

On a second thought, maybe using "!skb_dst(skb)” directly would be more 
appropriate?

> + dst_set = true;
> + skb_dst_set(skb, (struct dst_entry *));

"skb_dst_set(skb, );” seems to be a more common way of doing this in the 
kernel.

>   }
>  #endif
>   err = helper->help(skb, protoff, ct, ctinfo);
>   if (err != NF_ACCEPT)
>   return err;
>  
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0)
> + if (dst_set)
> + skb_dst_set(skb, NULL);
> +#endif
> +
>   /* Adjust seqs after helper.  This is needed due to some helpers (e.g.,
>* FTP with NAT) adusting the TCP payload size when mangling IP
>* addresses and/or port numbers in the text-based control connection.
> -- 
> 2.7.4
> 
> 
> On Wed, Jan 4, 2017 at 9:33 PM, Jarno Rajahalme  > wrote:
> 
>> On Jan 4, 2017, at 9:50 AM, John Hurley > > wrote:
>> 
>> From 64ce83672aab5634990426e0a51af16d882ac2f9 Mon Sep 17 00:00:00 2001
>> From: John Hurley > >
>> Date: Wed, 4 Jan 2017 16:52:43 +
>> Subject: [PATCH] ensure correct checksum when a packet is sent to NAT helper
>>  in kernel < 4.6
>> 
>> ---
>>  datapath/conntrack.c | 45 -
>>  1 file changed, 16 insertions(+), 29 deletions(-)
>> 
>> diff --git 

Re: [ovs-dev] [PATCH] tunneling: Combining actions to avoid packet recirculation in tunnel encapsulation.

[Jarno Rajahalme]

> On Jan 5, 2017, at 6:57 AM, Chandran, Sugesh  
> wrote:
> 
> Hi Jarno,
> 
> Thank you for looking into the path, 
> Please find my answers inline blow.
> 
> 
> Regards
> _Sugesh
> 
> 
>> -Original Message-
>> From: Jarno Rajahalme [mailto:ja...@ovn.org]
>> Sent: Wednesday, January 4, 2017 12:00 AM
>> To: Chandran, Sugesh 
>> Cc: zoltan.bal...@ericsson.com; d...@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH] tunneling: Combining actions to avoid packet
>> recirculation in tunnel encapsulation.
>> 
>> Sugesh,
>> 
>> So the issue is that instead of recirculating on the datapath, we can compute
>> the “recirculation” actions on the tunnel push translation time, as these
>> actions only depend on the tunnel attributes? This explains why the
>> recirculation in this case is unnecessary, and mentioning this in the commit
>> message would make the motivation/correctness easier to understand.
> [Sugesh] Make sense, Will update the commit message accordingly in the v2.
>> 
>> It seems to me that
>> OVS_ACTION_ATTR_START_COMBINE/OVS_ACTION_ATTR_END_COMBINE
>> semantics is to clone the packet and execute the actions in between. A
>> cleaner way of doing this would be with (e.g.) “OVS_ACTION_ATTR_CLONE”
>> that would be a nested attribute, the length of which would span to the end
>> of the nested attributes (i.e., the actions produced by the tunnel output).
>> This would be likely also faster to execute as the end does not need to be
>> searched. Semantically this would be the same as the sample action with
>> probability of 1, but simpler. Obviously the removal of the final cloning 
>> could
>> be avoided like now.
> [Sugesh] Sure it make sense to take out the END_COMBINE action.
> I am not very sure to call it as CLONE_ACTION as its not guarantee the packet 
> cloning
> in every time(For eg: the final action case). The new dummy actions are used 
> to inform dp about set of actions that
> are grouped/combined. These actions are two distinct set of actions which are 
> merged to avoid recirculation in dp.
> Can we call it 
> OVS_ACTION_ATTR_COMBINE/OVS_ACTION_ATTR_GROUP/OVS_ACTION_ATTR_SET
> than calling them CLONE.? 

I think CLONE is more understandable than COMBINE, as the semantics is that any 
nested actions are applied to a clone of the packet, so that none of those 
actions need to be undone prior to the remaining actions, which see the packet 
as it was before any of the nested actions. To me none of the alternatives you 
provided even hint to this semantics, and the fact that the final cloning can 
be avoided is a lower layer optimization that does not make any semantic 
difference.

>> 
>> Finally, translating the tunnel output looks a lot like translating patch 
>> port
>> output. I wonder if it would be possible to share some of the code via some
>> new helpers rather than duplicating it?
> [Sugesh] Though both share same concept, the fields that used for combine are 
> slightly different.
> Do you think a single helper function for combine on translation will work 
> for both path and tunnel.
> This may need 'ifs' here and there in the function to do some of the field 
> population and validation exclusively either for
> tunneling or  patch port. Do you think its cleaner than having independent 
> functions?
> Anyway we will look into this possibility of combining these functions.

I’d appreciate that. But let me know if it becomes too messy and in the end is 
not worth it.

  Jarno

>>  Jarno
>> 
>>> On Dec 30, 2016, at 7:36 AM, Sugesh Chandran
>>  wrote:
>>> 
>>> Currently the tunnel packets are recirculated in OVS after the tunnel
>>> encapsulation. This patch combines the post tunnel actions with the
>>> tunnel push to avoid unnecessary recirculation.
>>> Two new datapath actions OVS_ATTR_COMBINE_START and
>>> OVS_ATTR_COMBINE_END are added to implement this feature. These
>> dummy
>>> new actions  used as a delimiters for combined action subset.
>>> In our testing this patch improves the VxLAN encapsulation performance
>> upto 30%.
>>> 
>>> Signed-off-by: Sugesh Chandran 
>>> Signed-off-by: Zoltán Balogh 
>>> Co-authored-by: Zoltán Balogh 
>>> ---
>>> datapath/linux/compat/include/linux/openvswitch.h |  2 +
>>> lib/dpif-netdev.c | 22 ++
>>> lib/dpif.c|  2 +
>>> lib/odp-execute.c | 59 ++-
>>> lib/odp-util.c| 68 -
>>> lib/odp-util.h|  6 ++
>>> ofproto/ofproto-dpif-sflow.c  |  2 +
>>> ofproto/ofproto-dpif-xlate.c  | 89 
>>> +++
>>> 8 files changed, 228 insertions(+), 22 deletions(-)
>>> 
>>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h
>>> 

[ovs-dev] [PATCH 2/2] ofctrl: Fix warning from sparse.

[Ben Pfaff]

We've used sparse "bitwise" annotations to make ofp_ports into a different
type, so this is required to avoid a sparse warning.

CC: Justin Pettit 
Fixes: 714651c7db6a ("ovn-controller: Introduce "inject-pkt" ovs-appctl 
command.")
Signed-off-by: Ben Pfaff 
---
 ovn/controller/ofctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 44c2210..3876ff6 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -1133,7 +1133,7 @@ ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, 
const char *flow_s,
 
 /* The physical OpenFlow port was stored in the logical ingress
  * port, so put it in the correct location for a flow structure. */
-uflow.in_port.ofp_port = uflow.regs[MFF_LOG_INPORT - MFF_REG0];
+uflow.in_port.ofp_port = u16_to_ofp(uflow.regs[MFF_LOG_INPORT - MFF_REG0]);
 uflow.regs[MFF_LOG_INPORT - MFF_REG0] = 0;
 
 if (!uflow.in_port.ofp_port) {
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] python: Fix nroff indentation for XML .

[Joe Stringer]

On 5 January 2017 at 17:08, Ben Pfaff  wrote:
> On Thu, Jan 05, 2017 at 03:48:22PM -0800, Joe Stringer wrote:
>> When XML is used for writing manpages, the nroff python utility indents
>> the  tags an extra level which is unnecessary and makes the
>> formatting inconsistent between manpages written directly in nroff vs
>> manpages written in XML and converted to nroff. Fix the indentation by
>> removing the extraneous .RS / .RE tags added to generated nroff.
>>
>> Signed-off-by: Joe Stringer 
>
> I've found that there are landmines in this area.  Did you look around
> at a few manpages to make sure that this doesn't cause funny behavior in
> some cases, in situations like multiple paragraphs in s, or s
> nested in s or vice versa, etc.?

Looks like it does cause trouble with these.

What I was trying to achieve, was that if the  is directly nested
under a toplevel header (that is, 'h1'), it should only indent one
level rather than two.

You're right, this version breaks a bunch of other cases; let me
revise to make it only take effect in this case I'd like to fix.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/5] ovs-ofctl: Document the "clone" action.

[Ben Pfaff]

This was overlooked when "clone" was introduced.

Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
Signed-off-by: Ben Pfaff 
---
 NEWS | 1 +
 utilities/ovs-ofctl.8.in | 8 
 2 files changed, 9 insertions(+)

diff --git a/NEWS b/NEWS
index 1b0aa5d..0c39cc4 100644
--- a/NEWS
+++ b/NEWS
@@ -33,6 +33,7 @@ Post-v2.6.0
details.
  * The "sample" action now supports "ingress" and "egress" options.
  * The "ct" action now supports the TFTP ALG where support is available.
+ * New action "clone".
- ovs-ofctl:
  * 'bundle' command now supports packet-out messages.
  * New syntax for 'ovs-ofctl packet-out' command, which uses the
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index d8c03db..72aa89a 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -2623,6 +2623,14 @@ than one flow.  Open vSwitch does not enforce this.
 .IP
 The \fBconjunction\fR action and \fBconj_id\fR field were introduced
 in Open vSwitch 2.4.
+.
+.IP "\fBclone(\fR[\fIaction\fR][\fB,\fIaction\fR...]\fB)\fR"
+Executes each nested \fIaction\fR, saving much of the packet and
+pipeline state beforehand and then restoring it afterward.  The state
+that is saved and restored includes all flow data and metadata
+(including, for example, \fBct_state\fR).
+.IP
+This action was added in Open vSwitch 2.6.90.
 .RE
 .
 .PP
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] xml2nroff: Port to python3.

[Joe Stringer]

Signed-off-by: Joe Stringer 
---
 build-aux/xml2nroff | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/build-aux/xml2nroff b/build-aux/xml2nroff
index df1df28d56cc..bd4e87928e4f 100755
--- a/build-aux/xml2nroff
+++ b/build-aux/xml2nroff
@@ -24,7 +24,7 @@ argv0 = sys.argv[0]
 
 
 def usage():
-print """\
+print("""\
 %(argv0)s: XML to nroff converter
 Converts the XML format supplied as input into an nroff-formatted manpage.
 usage: %(argv0)s [OPTIONS] INPUT.XML [VAR=VALUE]...
@@ -37,14 +37,14 @@ The following options are also available:
   -I, --include=DIR   search DIR for include files (default: .)
   --version=VERSION   use VERSION to display on document footer
   -h, --help  display this help message\
-""" % {'argv0': argv0}
+""" % {'argv0': argv0})
 sys.exit(0)
 
 
 def manpage_to_nroff(xml_file, subst, include_path, version=None):
 with open(xml_file) as f:
 content = f.read()
-for k, v in subst.iteritems():
+for k, v in subst.items():
 content = content.replace(k, v)
 doc = xml.dom.minidom.parseString(content).documentElement
 
@@ -62,7 +62,7 @@ def manpage_to_nroff(xml_file, subst, include_path, 
version=None):
 sys.stderr.write("%s: could not open include file %s\n"
  % (argv0, fn))
 sys.exit(1)
-for k, v in subst.iteritems():
+for k, v in subst.items():
 content = content.replace(k, v)
 xi_doc = xml.dom.minidom.parseString(content).documentElement
 doc.replaceChild(xi_doc, node)
@@ -102,7 +102,7 @@ if __name__ == "__main__":
 try:
 options, args = getopt.gnu_getopt(sys.argv[1:], 'hVI:',
   ['version=', 'help', 'include='])
-except getopt.GetoptError, geo:
+except getopt.GetoptError as geo:
 sys.stderr.write("%s: %s\n" % (argv0, geo.msg))
 sys.exit(1)
 
@@ -139,13 +139,13 @@ if __name__ == "__main__":
 
 try:
 s = manpage_to_nroff(args[0], subst, include_path, version)
-except build.nroff.error.Error, e:
+except build.nroff.error.Error as e:
 sys.stderr.write("%s: %s\n" % (argv0, e.msg))
 sys.exit(1)
 for line in s.splitlines():
 line = line.strip()
 if line:
-print line
+print(line)
 
 
 # Local variables:
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Issues with the use of the clone action for resubmission to the pipeline

[Mickey Spiegel]

On Thu, Jan 5, 2017 at 4:28 PM, Ben Pfaff  wrote:

> On Tue, Jan 03, 2017 at 02:55:19AM -0800, Mickey Spiegel wrote:
> > One of the motivations for clone is to use it as a lightweight way to
> > resubmit to an earlier table at the beginning of the pipeline, without
> > incurring all of the overhead associated with openflow patch ports.
> > One such usage is in OVN, where a recent patch set replaced the
> > use of openflow patch ports with clone, for OVN patch ports within
> > the same bridge (br-int).
> >
> > Over the holidays, some issues arose related to this usage of clone
> > (see the thread ovn ping from VM to external gateway IP failed).
>
> Thanks for bringing this up.  I had overlooked these questions and this
> issue.
>

Thanks for addressing all of this. The patch set should clear up any
existing and potential future problems with the use of clone as a
lightweight way to resubmit to the pipeline.

>
> > While investigating these issues, I noticed some significant
> > differences between the way flow and other context information
> > is saved and restored when using openflow patch ports, versus
> > the way it is done with clone. At least one such difference, with
> > regard to conntrack, is causing failures.
>
> What's the difference that is causing failures?  I do not know about it,
> so it is a high priority to fix it.  Is it already fixed, or do we need
> to do something about it?
>

There are two issues causing failures.

One is http://patchwork.ozlabs.org/patch/709981/, which needs to be
reviewed. This is not an issue with the usage of clone, it is an
underlying issue that was revealed by the removal of ofports for
OVN patch ports.

We need to call your new ct_clear action inside the clone in
ovn/controller/physical.c. Numan tried a diff
http://patchwork.ozlabs.org/patch/710368/ that cleared flow
conntrack state as part of the clone action, and that fixed the
problem that he was seeing. Since Numan is able to reproduce the
issue, he should probably be the one to submit this OVN fix.

>
> > I would classify the differences (those for which there is no current
> > workaround by specifying nested actions within the clone) into two
> > categories:
> > 1. State that is kept outside of flow, which is currently saved,
> >cleared, and restored when using openflow patch ports.
> >This includes:
> >ctx->wc->masks.tunnel
>
> I think that clone should not save and restore this.  The situation is
> different from a patch port because clone does not change the ingress
> port.  The patch port code has the following comment:
>
> /* Since this packet came in on a patch port (from the perspective
> of
>  * the peer bridge), it cannot have useful tunnel information. As a
>  * result, any wildcards generated on that tunnel also cannot be
> valid.
>  * The tunnel wildcards must be restored to their original version
> since
>  * the peer bridge uses a separate tunnel metadata table and
> therefore
>  * any generated wildcards will be garbage in the context of our
>  * metadata table. */
> ctx->wc->masks.tunnel = old_flow_tnl_wc;
>

I am still a little unclear on the implications of clearing or restoring
this.
If this is like flow->tunnel.metadata.tab and is only derived from the
bridge, then there would be no need to save and restore.

>
> >ctx->conntracked
>
> I guess that we should save and restore this since we're saving and
> restoring the conntrack metadata.  I've written up a patch.
>

Agreed.

>
> >ctx->was_mpls
>
> I do not think that that this is a correctness issue, but it's a nice
> optimization to save and restore this.  I've written up a patch.
>
> >ctx->xin->tables_version (not an issue if bridge does not change)
>
> clone doesn't change the bridge, so this shouldn't matter.
>

Agreed

>
> >ctx->stack
> >ctx->action_set
>
> I think it's cleanest if a clone starts off with both of these empty and
> saves and restores them.  I've written up a patch.
>
> >ctx->xbridge (not an issue if bridge does not change)
> >ctx->mirrors (not an issue if bridge does not change)
>
> clone doesn't change the bridge, so these shouldn't matter.
>

Agreed.

>
> > 2. State that resides inside the flow, but for which there is no
> >workaround to clear or reset the fields. This includes:
> >flow->tunnel ?
>
> Like the tunnel mask, this doesn't matter for clone, because clone
> doesn't change the ingress port.
>

I am still a little unclear on the implications of clearing or restoring
this.
The ingress port is cleared in ovn/controller/physical.c.
Does that matter?

>
> >flow->tunnel.metadata.tab (not an issue if bridge does not change)
>
> clone doesn't change the bridge, so this shouldn't matter.
>

Agreed.

>
> >flow->ct_state (read only)
> >flow->ct_mark (restricted so can only be set in ct action)
> >flow->ct_label (restricted so can only be set in ct action)
>
> It's not obvious 

Re: [ovs-dev] ovs-vswitch kernel panic randomly started after 400+ days uptime

[Uri Foox]

Hey Joe,

Thank you so much for responding! After 10 days of trying to figure this
out I'm at a loss.

root@node-8:~# modinfo openvswitch
filename:
/lib/modules/3.13.0-106-generic/kernel/net/openvswitch/openvswitch.ko
license:GPL
description:Open vSwitch switching datapath
srcversion: 94294A72258BA583D07
depends:libcrc32c,vxlan,gre
intree: Y
vermagic:   3.13.0-106-generic SMP mod_unload modversions


Everything you've mentioned is what I've understood so far including the
line of code that's triggered. That is what led me to upgrade the kernel to
3.13.0-106 because it claims that the CHECKSUM problems are fixed which I
thought this might be related, guess not.

You're saying that skb_headlen is too short for the ethernet header. Do you
know what would cause this? This hardware configuration has been running
for 400+ days of uptime with no errors or problems and this suddenly
started to happen and no matter how many time we reboot things it doesn't
go away.  I assume given your interpretation we should try to restart the
switches connected to the servers. Is there any way to log what packet is
causing this issue? Perhaps that would provide more insight?

As far as 4.4/newer kernel - I wish. I tried to go that far up but Ubuntu
wouldn't even boot. The best I could do is 3.13.0-106. I'll try to report
it over there as well.

Thanks again.

Uri


On Thu, Jan 5, 2017 at 10:16 PM, Joe Stringer  wrote:

> On 5 January 2017 at 17:13, Uri Foox  wrote:
> > Hi,
> >
> > Since about 10 days ago, every few hours, one of our 10 compute nodes on
> > our Openstack cluster kernel panics at the host level kernel panics
> > (captured through netconsole). The kernel panic is identical across all
> 10
> > nodes and happens at random times but at least 1 node kernel panics every
> > 3-12 hours. We have tried numerous things including upgrading the kernel
> > (Ubuntu 12.04 LTS running 3.1.0-106-generic), modifying sysctl,
> restarting
> > switches, restarting all openstack networking services, changing BIOS
> > settings etc...but no luck. We have not restarted the control nodes or
> the
> > Juniper switch that routes all inbound internet traffic.
> >
> > Based on research we did around skbuff.h we found two kernel patches to
> > address a checksum failure and also some OVS discussions about it. I was
> > hoping that the kernel upgrade would solve it but it did not. I do not
> know
> > if Openstack will tolerate us upgrading OVS and the fact that it started
> > completely randomly leads me to believe it's some other factor that we
> are
> > unaware of.
> >
> >
> >- https://patchwork.ozlabs.org/patch/512625/
> >-
> >https://github.com/openvswitch/ovs/commit/
> 51b7a90217369f6bbbf164ba471f54ec2817665e
> >- https://patchwork.kernel.org/patch/7475491/
> >- https://patchwork.ozlabs.org/patch/523632/
> >
> >
> > Here is one of them. If you have any ideas what we can do, please let me
> > know.
> >
> > Thanks,
> > Uri
> >
> >
> > Connection from 172.25.2.157 port 5404 [udp/*] accepted
> > [68240.441681] [ cut here ]
> > [68240.496918] kernel BUG at
> > /build/linux-lts-trusty-D60X6T/linux-lts-trusty-3.13.
> 0/include/linux/skbuff.h:1486!
> > [68240.615520] invalid opcode:  [#1] SMP
> > [68240.664751] Modules linked in: netconsole configfs xt_mac xt_physdev
> > xt_set ip_set_hash_ip ip_set nfnetlink vhost_net macvtap macvlan vhost
> veth
> > bridge stp llc ipt_REJECT xt_state xt_conntrack xt_multiport xt_CT
> > xt_comment iptable_raw xt_CHECKSUM xt_tcpudp iptable_mangle
> ipt_MASQUERADE
> > iptable_nat nf_nat_ipv4 nf_nat ip6table_filter ip6_tables iptable_filter
> > ip_tables ebtable_nat ebtables x_tables kvm_intel kvm nbd ib_iser rdma_cm
> > ib_cm iw_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp libiscsi_tcp libiscsi
> > scsi_transport_iscsi openvswitch vxlan ip_tunnel gre nfsd nfs_acl
> > auth_rpcgss nfs fscache lockd sunrpc dm_multipath gpio_ich dcdbas scsi_dh
> > mei_me shpchp sb_edac mei edac_core lpc_ich joydev acpi_power_meter
> > nf_conntrack_ipv6 mac_hid nf_defrag_ipv6 wmi nf_conntrack_ipv4 ipmi_si
> xfs
> > nf_conntrack nf_defrag_ipv4 lp parport igb btrfs hid_generic dca
> > i2c_algo_bit usbhid raid6_pq ptp ahci bnx2x hid libahci mdio megaraid_sas
> > pps_core xor libcrc32c
> > [68241.670838] CPU: 33 PID: 0 Comm: swapper/33 Not tainted
> > 3.13.0-106-generic #153~precise1-Ubuntu
> > [68241.774871] Hardware name: Dell Inc. PowerEdge R820/066N7P, BIOS 2.2.3
> > 07/09/2014
> > [68241.864406] task: 881028b94800 ti: 881028ba task.ti:
> > 881028ba
> > [68241.953939] RIP: 0010:[]  []
> > __skb_pull.part.7+0x4/0x6 [openvswitch]
> > [68242.067531] RSP: 0018:88203fb03b08  EFLAGS: 00010297
> > [68242.131087] RAX: 88165c791966 RBX: 88202639e900 RCX:
> > 88165c791900
> > [68242.216458] RDX: 0210 RSI: 001a RDI:
> > 0214
> > [68242.301842] RBP: 88203fb03b08 

Re: [ovs-dev] [PATCH ovs V2 12/21] dpif-netlink: Use netdev flow put api to insert a flow

[Joe Stringer]

On 25 December 2016 at 03:39, Paul Blakey  wrote:
> Using the new netdev flow api operate will now try and
> offload flows to the relevant netdev of the input port.
> Other operate methods flows will come in later patches.
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> ---
>  lib/dpif-netlink.c | 232 
> -
>  1 file changed, 228 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 3d8940e..717af90 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1908,15 +1908,239 @@ dpif_netlink_operate__(struct dpif_netlink *dpif,
>  return n_ops;
>  }
>
> +static int
> +parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len,
> +const struct nlattr *mask, size_t mask_len,
> +struct match *match)
> +{
> +enum odp_key_fitness fitness;
> +
> +fitness = odp_flow_key_to_flow(key, key_len, >flow);
> +if (fitness) {
> +/* This should not happen: it indicates that odp_flow_key_from_flow()
> + * and odp_flow_key_to_flow() disagree on the acceptable form of a
> + * flow.  Log the problem as an error, with enough details to enable
> + * debugging. */
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +
> +if (!VLOG_DROP_ERR()) {
> +struct ds s;
> +
> +ds_init();
> +odp_flow_format(key, key_len, NULL, 0, NULL, , true);
> +VLOG_ERR("internal error parsing flow key %s", ds_cstr());
> +ds_destroy();
> +}
> +
> +return EINVAL;
> +}
> +
> +fitness = odp_flow_key_to_mask(mask, mask_len, >wc, >flow);
> +if (fitness) {
> +/* This should not happen: it indicates that
> + * odp_flow_key_from_mask() and odp_flow_key_to_mask()
> + * disagree on the acceptable form of a mask.  Log the problem
> + * as an error, with enough details to enable debugging. */
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +
> +if (!VLOG_DROP_ERR()) {
> +struct ds s;
> +
> +VLOG_ERR("internal error parsing flow mask %s (%s)",
> + ds_cstr(), odp_key_fitness_to_string(fitness));
> +ds_destroy();
> +}
> +
> +return EINVAL;
> +}
> +
> +return 0;
> +}

Duplicated code. Please refactor and reuse. (Separate patch for
refactoring to keep the patches clear).

> +
> +static bool
> +parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
> +{
> +struct match match;
> +odp_port_t in_port;
> +const struct nlattr *nla;
> +size_t left;
> +int outputs = 0;
> +struct ofpbuf buf;
> +uint64_t act_stub[1024 / 8];
> +size_t offset;
> +struct nlattr *act;
> +struct netdev *dev;
> +int err;
> +
> +/* 0x1234 - fake eth type sent to probe feature */
> +if (put->flags & DPIF_FP_PROBE || match.flow.dl_type == htons(0x1234)) {
> +return false;
> +}

Shouldn't DPIF_FP_PROBE be sufficient?

> +
> +if (parse_key_and_mask_to_match(put->key, put->key_len, put->mask,
> +put->mask_len, )) {
> +return false;
> +}
> +
> +in_port = match.flow.in_port.odp_port;
> +ofpbuf_use_stub(, act_stub, sizeof act_stub);
> +offset = nl_msg_start_nested(, OVS_FLOW_ATTR_ACTIONS);
> +NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
> +if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
> +struct netdev *outdev;
> +int ifindex_out;
> +const struct netdev_tunnel_config *tnl_cfg;
> +size_t out_off;
> +odp_port_t out_port;
> +
> +outputs++;
> +if (outputs > 1) {
> +break;
> +}
> +
> +out_port = nl_attr_get_u32(nla);
> +outdev = netdev_hmap_port_get(out_port, dpif->dpif.dpif_class);
> +tnl_cfg = netdev_get_tunnel_config(outdev);
> +
> +out_off = nl_msg_start_nested(, OVS_ACTION_ATTR_OUTPUT);
> +ifindex_out = netdev_get_ifindex(outdev);

Can this fail? (port not backed by device with ifindex?)

> +nl_msg_put_u32(, OVS_ACTION_ATTR_OUTPUT, ifindex_out);
> +if (tnl_cfg && tnl_cfg->dst_port != 0) {
> +nl_msg_put_u32(, OVS_TUNNEL_KEY_ATTR_TP_DST, 
> tnl_cfg->dst_port);
> +}
> +nl_msg_end_nested(, out_off);
> +
> +if (outdev) {
> +netdev_close(outdev);
> +}
> +} else {
> +nl_msg_put_unspec(, nl_attr_type(nla), nl_attr_get(nla),
> +  nl_attr_get_size(nla));
> +}
> +}
> +nl_msg_end_nested(, offset);

Hmm. I'm a bit uneasy about this whole copying/rewriting of the
actions. Firstly, 

Re: [ovs-dev] [PATCH] nx-match: Only store significant bytes to stack.

[Jarno Rajahalme]

> On Jan 5, 2017, at 4:48 PM, Ben Pfaff  wrote:
> 
> On Thu, Jan 05, 2017 at 04:03:17PM -0800, Jarno Rajahalme wrote:
>> 
>>> On Jan 4, 2017, at 11:03 PM, Ben Pfaff  wrote:
>>> 
>>> On Wed, Jan 04, 2017 at 07:21:44PM -0800, Jarno Rajahalme wrote:
 
> On Dec 21, 2016, at 2:36 PM, Ben Pfaff  wrote:
> 
> On Wed, Dec 07, 2016 at 04:49:00PM -0800, Jarno Rajahalme wrote:
> I'd be more comfortable if nx_stack_pop() had assertions to check for
> underflow.
 
 I don’t think OVS should assert fail if controller issues one pop
 too many? Do you mean that current users of nx_stack_pop() do not
 check for NULL return? I had a look and think that setting “*bytes”
 to zero when returning NULL should be enough for all users.
>>> 
>>> It appears to me that if stack->size is greater than 0 but less than the
>>> number of bytes indicated by its last byte, then it will corrupt the
>>> ofpbuf size and that this will later cause some kind of failure that
>>> will be harder to debug than an assertion failure. 
>>> 
>> 
>> OK, now i got it.  This is just to guard against (future) bugs in OVS itself.
> 
> Yes.
> 
> In ofputil_decode_packet_in_private(), it's probably worth checking the
> format of the stack we pull from the payload, since a badly formatted
> stack can segfault us (if we leave out assertions) or assert-fail us (if
> we include them).
> 
 
 What do you mean with badly formatted stack? Zero-sized property? IMO
 even that would be properly pushed and popped from the stack, storing
 only the length (of zero) in the stack.
>>> 
>>> I mean that if the property contains, for example, a single byte with
>>> value 0xff, then it's badly formatted because we can pop off a length
>>> (255) but then popping off that number of bytes will underflow.
>> 
>> I did not change the encoding of the stack as properties, so each
>> value in the stack is still encoded as a separate property, where the
>> (aligned) value length is used as the property length. 
> 
> I guess I forgot that.
> 
> Thanks, that's fine then.
> 
>> ofpprop_pull() does the length checking for the properties and the
>> current code in ofputil_decode_packet_in_private() assert fails on any
>> error, which is not good, as a controller bug would crash OVS?
> 
> That's bad.  Maybe the fix is as simple as this, though.
> 
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 156d8d2..421b9d7 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 
> Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 
> Nicira, Inc.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -4061,7 +4061,9 @@ ofputil_decode_packet_in_private(const struct 
> ofp_header *oh, bool loose,
> uint64_t type;
> 
> error = ofpprop_pull(, , );
> -ovs_assert(!error);
> +if (error) {
> +break;
> +}
> 
> switch (type) {
> case NXCPT_BRIDGE:
> @@ -4124,7 +4126,7 @@ ofputil_decode_packet_in_private(const struct 
> ofp_header *oh, bool loose,
> ofputil_packet_in_private_destroy(pin);
> }
> 
> -return 0;
> +return error;
> }
> 
> /* Frees data in 'pin' that is dynamically allocated by
> 

I folded this in to v3.

  Jarno

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] docs: Fix formatting of patch comments line.

[Joe Stringer]

On 5 January 2017 at 01:28, Stephen Finucane  wrote:
> On Wed, 2017-01-04 at 13:58 -0800, Joe Stringer wrote:
>> Sphinx was formatting the `---` as an extended dash, not verbatim as
>> three hyphens (which is what is necessary for git to determine that
>> it's
>> a comment, and ignore it when applying the patch).
>>
>> Signed-off-by: Joe Stringer 
>
> Aye, good catch.
>
> Acked-by: Stephen Finucane 

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2] ovs-ofctl.8: Document automatic helper assignment.

[Joe Stringer]

On 3 January 2017 at 11:41, Jarno Rajahalme  wrote:
> Acked-by: Jarno Rajahalme 

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] xlate: Recirculate also when MPLS POP is implicit.

[Jarno Rajahalme]

'ctx->was_mpls' is used to flag when an MPLS packet has been popped to
a non-MPLS packet, but it was not set when the MPLS POP is implicit
due to the 'ctx->xin->flow' being restored after a patch port
traversal to group bucket execution.

If MPLS push on a non-MPLS packet is followed by an MPLS POP in the
same actions list, a recirculation after the MPLS POP would not be
necessary, as the parsed L3/4 fields are still available.  Even so,
the current action validation and execution code in the Linux kernel
datapath implementation would need to be enhanced to support this
case.  For now we trigger recirculation instead.

Reported-by: Thomas Morin 
Suggested-by: Takashi YAMAMOTO 
Fixes: 742c0ac3c0ab ("mpls: Fix MPLS restoration after patch port and group 
bucket.")
Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-dpif-xlate.c | 24 ++--
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 371ced4..27b64d9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3053,9 +3053,15 @@ compose_output_action__(struct xlate_ctx *ctx, 
ofp_port_t ofp_port,
  * metadata table. */
 ctx->wc->masks.tunnel = old_flow_tnl_wc;
 
-/* The peer bridge popping MPLS should have no effect on the original
- * bridge. */
-ctx->was_mpls = old_was_mpls;
+/* An MPLS packet can be implicitly popped back to a non-MPLS packet
+ * when the flow key is restored, if the patch port peer pushed MPLS to
+ * a non-MPLS packet.  In this case the now restored flow key is of
+ * non-MPLS type, while the base flow is of an MPLS type.  Set the
+ * 'was_mpls' flag also in that case. */
+ctx->was_mpls = old_was_mpls ||
+(eth_type_mpls(ctx->base_flow.dl_type)
+ && !eth_type_mpls(ctx->xin->flow.dl_type)
+ && ctx->xbridge->support.odp.recirc);
 
 /* The peer bridge's conntrack execution should have no effect on the
  * original bridge. */
@@ -3379,9 +3385,15 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct 
ofputil_bucket *bucket)
  * group buckets. */
 ctx->xin->flow = old_flow;
 
-/* The group bucket popping MPLS should have no effect after bucket
- * execution. */
-ctx->was_mpls = old_was_mpls;
+/* An MPLS packet can be implicitly popped back to a non-MPLS packet
+ * when the flow key is restored, if the group bucket pushed MPLS to
+ * a non-MPLS packet.  In this case the now restored flow key is of
+ * non-MPLS type, while the base flow is of an MPLS type.  Set the
+ * 'was_mpls' flag also in that case. */
+ctx->was_mpls = old_was_mpls ||
+(eth_type_mpls(ctx->base_flow.dl_type)
+ && !eth_type_mpls(ctx->xin->flow.dl_type)
+ && ctx->xbridge->support.odp.recirc);
 
 /* The fact that the group bucket exits (for any reason) does not mean that
  * the translation after the group action should exit.  Specifically, if
-- 
2.1.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] ovs-ofctl: Document the "clone" action.

[Mickey Spiegel]

On Thu, Jan 5, 2017 at 4:28 PM, Ben Pfaff  wrote:

> This was overlooked when "clone" was introduced.
>
> Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
> Signed-off-by: Ben Pfaff 
>

Acked-by: Mickey Spiegel 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] python: Add TCP/SSL probes for OVSDB python lib

[Guoshuai Li]

stream_or_pstream_needs_probes always return 0. This causes TCP/SSL
connection not be probed, and no reconnect when the connection
is aborted

Signed-off-by: Guoshuai Li 
---
 python/ovs/stream.py | 32 +++-
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/python/ovs/stream.py b/python/ovs/stream.py
index 97b22ac..c5fe1dc 100644
--- a/python/ovs/stream.py
+++ b/python/ovs/stream.py
@@ -26,16 +26,18 @@ vlog = ovs.vlog.Vlog("stream")
 
 
 def stream_or_pstream_needs_probes(name):
-""" 1 if the stream or pstream specified by 'name' needs periodic probes to
-verify connectivity.  For [p]streams which need probes, it can take a long
-time to notice the connection was dropped.  Returns 0 if probes aren't
-needed, and -1 if 'name' is invalid"""
-
-if PassiveStream.is_valid_name(name) or Stream.is_valid_name(name):
-# Only unix and punix are supported currently.
-return 0
+""" True if the stream or pstream specified by 'name' needs periodic probes
+to verify connectivity.  For [p]streams which need probes, it can take a
+long time to notice the connection was dropped.  Returns False if probes
+aren't needed, and None if 'name' is invalid"""
+
+cls = Stream._find_method(name)
+if cls:
+return cls.needs_probes()
+elif PassiveStream.is_valid_name(name):
+return PassiveStream.needs_probes(name)
 else:
-return -1
+return None
 
 
 class Stream(object):
@@ -267,6 +269,10 @@ class Stream(object):
 
 class PassiveStream(object):
 @staticmethod
+def needs_probes(name):
+return False if name.startswith("punix:") else True
+
+@staticmethod
 def is_valid_name(name):
 """Returns True if 'name' is a passive stream name in the form
 "TYPE:ARGS" and TYPE is a supported passive stream type (currently
@@ -369,6 +375,10 @@ Passive %s connection methods:
 
 class UnixStream(Stream):
 @staticmethod
+def needs_probes():
+return False
+
+@staticmethod
 def _open(suffix, dscp):
 connect_path = suffix
 return ovs.socket_util.make_unix_socket(socket.SOCK_STREAM,
@@ -378,6 +388,10 @@ Stream.register_method("unix", UnixStream)
 
 class TCPStream(Stream):
 @staticmethod
+def needs_probes():
+return True
+
+@staticmethod
 def _open(suffix, dscp):
 error, sock = ovs.socket_util.inet_open_active(socket.SOCK_STREAM,
suffix, 0, dscp)
-- 
2.10.1.windows.1

This patch is based on branch-2.6.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] dpcls: Avoid one 8-byte chunk in subtable mask.

[Jarno Rajahalme]

> On Jan 5, 2017, at 1:03 AM, antonio.fische...@intel.com wrote:
> 
> This patch allows to skip the chunk comprising of dp_hash and in_port
> in the subtable mask when the packet is not recirculated.  This will
> slightly speed up the hash computation as one expensive function call
> to hash_add64() can be skipped.
> For each new netdev flow we wildcard in_port in the mask, so in the
> physical case where dp_hash is also wildcarded, the resulting 8-byte
> chunk will not be part of the subtable mask.
> 

It appears you have not run the testsuite with this patch, as it consistently 
fails. This is due to dumped flows now missing the in_port match, which is 
caused by the subtable mask change. This can be fixed like this:

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 0b73056..5f6fc01 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2098,6 +2098,9 @@ dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow 
*netdev_flow,
 };
 
 miniflow_expand(_flow->cr.mask->mf, );
+/* in_port is exact matched, but we have left it out from the mask for
+ * optimization reasons. Add in_port back to the mask. */
+wc.masks.in_port.odp_port = ODPP_NONE;
 
 /* Key */
 offset = key_buf->size;

> The idea behind is that all the packets within a given dpcls will
> have the same in_port value and typically dp_hash == 0.  So they will
> have the same 8-bytes chunk in their {dp_hash, in_port} portion of the
> miniflow. This doesn't contribute effectively in spreading hash values
> and avoiding collisions.
> 
> v2: Using ovs_assert
> 

Now there are two asserts for the same thing, I think the latter (the old one) 
should be removed.

  Jarno

> Signed-off-by: Antonio Fischetti 
> Signed-off-by: Bhanuprakash Bodireddy 
> Co-authored-by: Bhanuprakash Bodireddy 
> Signed-off-by: Jarno Rajahalme 
> Co-authored-by: Jarno Rajahalme 
> ---
> lib/dpif-netdev.c | 11 +++
> 1 file changed, 11 insertions(+)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 1a47a45..68b434f 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2278,7 +2278,18 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
> struct dpcls *cls;
> odp_port_t in_port = match->flow.in_port.odp_port;
> 
> +/* As we select the dpcls based on the port number, each netdev flow
> + * belonging to the same dpcls will have the same odp_port value.
> + * For performance reasons here we wildcard odp_port in the mask.  In the
> + * physical case where dp_hash is also wildcarded, the resulting 8-byte
> + * chunk {dp_hash, in_port} will be ignored by netdev_flow_mask_init() 
> and
> + * will not be part of the subtable mask.
> + * This will speed up the hash computation during dpcls_lookup() because
> + * one call to hash_add64() will be skipped. */
> +ovs_assert(match->wc.masks.in_port.odp_port == ODPP_NONE);
> +match->wc.masks.in_port.odp_port = 0;
> netdev_flow_mask_init(, match);
> +match->wc.masks.in_port.odp_port = ODPP_NONE;
> /* Make sure wc does not have metadata. */
> ovs_assert(!FLOWMAP_HAS_FIELD(, metadata)
>&& !FLOWMAP_HAS_FIELD(, regs));
> -- 
> 2.4.11
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 5/5] New action "ct_clear".

[Ben Pfaff]

This is being introduced specifically to allow a user of the "clone" action
to clear the connection tracking state, but it's implemented as a separate
action as a matter of clean design and in case another use case arises
later.

Reported-by: Mickey Spiegel 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/326981.html
Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
Signed-off-by: Ben Pfaff 
---
 NEWS  |  2 +-
 include/openvswitch/ofp-actions.h |  3 +-
 lib/ofp-actions.c | 43 +++-
 manpages.mk   |  4 ---
 ofproto/ofproto-dpif-xlate.c  | 19 +
 tests/ofp-actions.at  |  3 ++
 tests/ofproto-dpif.at | 60 +++
 utilities/ovs-ofctl.8.in  |  6 
 8 files changed, 128 insertions(+), 12 deletions(-)

diff --git a/NEWS b/NEWS
index 0c39cc4..5bc3273 100644
--- a/NEWS
+++ b/NEWS
@@ -33,7 +33,7 @@ Post-v2.6.0
details.
  * The "sample" action now supports "ingress" and "egress" options.
  * The "ct" action now supports the TFTP ALG where support is available.
- * New action "clone".
+ * New actions "clone" and "ct_clear".
- ovs-ofctl:
  * 'bundle' command now supports packet-out messages.
  * New syntax for 'ovs-ofctl packet-out' command, which uses the
diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index df9025c..8ca787a 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -107,6 +107,7 @@
 OFPACT(SAMPLE,  ofpact_sample,  ofpact, "sample")   \
 OFPACT(UNROLL_XLATE,ofpact_unroll_xlate, ofpact, "unroll_xlate") \
 OFPACT(CT,  ofpact_conntrack,   ofpact, "ct")   \
+OFPACT(CT_CLEAR,ofpact_null,ofpact, "ct_clear") \
 OFPACT(NAT, ofpact_nat, ofpact, "nat")  \
 OFPACT(OUTPUT_TRUNC,ofpact_output_trunc,ofpact, "output_trunc") \
 OFPACT(CLONE,   ofpact_nest,actions, "clone")   \
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index f29673f..4736521 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008-2016 Nicira, Inc.
+ * Copyright (c) 2008-2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -328,6 +328,9 @@ enum ofp_raw_action_type {
 /* NX1.0+(42): struct ext_action_header, ... */
 NXAST_RAW_CLONE,
 
+/* NX1.0+(43): void. */
+NXAST_RAW_CT_CLEAR,
+
 /* ## -- ## */
 /* ## Debugging actions. ## */
 /* ## -- ## */
@@ -449,6 +452,7 @@ ofpact_next_flattened(const struct ofpact *ofpact)
 case OFPACT_EXIT:
 case OFPACT_SAMPLE:
 case OFPACT_UNROLL_XLATE:
+case OFPACT_CT_CLEAR:
 case OFPACT_DEBUG_RECIRC:
 case OFPACT_METER:
 case OFPACT_CLEAR_ACTIONS:
@@ -5495,6 +5499,36 @@ format_CT(const struct ofpact_conntrack *a, struct ds *s)
 ds_put_format(s, "%s)%s", colors.paren, colors.end);
 }
 
+/* ct_clear action. */
+
+static enum ofperr
+decode_NXAST_RAW_CT_CLEAR(struct ofpbuf *out)
+{
+ofpact_put_CT_CLEAR(out);
+return 0;
+}
+
+static void
+encode_CT_CLEAR(const struct ofpact_null *null OVS_UNUSED,
+enum ofp_version ofp_version OVS_UNUSED,
+struct ofpbuf *out)
+{
+put_NXAST_CT_CLEAR(out);
+}
+
+static char * OVS_WARN_UNUSED_RESULT
+parse_CT_CLEAR(char *arg OVS_UNUSED, struct ofpbuf *ofpacts,
+   enum ofputil_protocol *usable_protocols OVS_UNUSED)
+{
+ofpact_put_CT_CLEAR(ofpacts);
+return NULL;
+}
+
+static void
+format_CT_CLEAR(const struct ofpact_null *a OVS_UNUSED, struct ds *s)
+{
+ds_put_format(s, "%sct_clear%s", colors.value, colors.end);
+}
 /* NAT action. */
 
 /* Which optional fields are present? */
@@ -6280,6 +6314,7 @@ ofpact_is_set_or_move_action(const struct ofpact *a)
 case OFPACT_BUNDLE:
 case OFPACT_CLEAR_ACTIONS:
 case OFPACT_CT:
+case OFPACT_CT_CLEAR:
 case OFPACT_CLONE:
 case OFPACT_NAT:
 case OFPACT_CONTROLLER:
@@ -6360,6 +6395,7 @@ ofpact_is_allowed_in_actions_set(const struct ofpact *a)
 case OFPACT_CLONE:
 case OFPACT_CONTROLLER:
 case OFPACT_CT:
+case OFPACT_CT_CLEAR:
 case OFPACT_NAT:
 case OFPACT_ENQUEUE:
 case OFPACT_EXIT:
@@ -6594,6 +6630,7 @@ ovs_instruction_type_from_ofpact_type(enum ofpact_type 
type)
 case OFPACT_SAMPLE:
 case OFPACT_DEBUG_RECIRC:
 case OFPACT_CT:
+case 

[ovs-dev] [PATCH 4/5] ofproto-dpif-xlate: Make clone save "was_mpls".

[Ben Pfaff]

This seems like it's an optimization rather than a correctness issue, but
in general it's best to make "clone" like patch ports where there is no
reason to depart from its design, since we know that patch ports work well.

Reported-by: Mickey Spiegel 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/326981.html
Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto-dpif-xlate.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 6736c12..c6c17a4 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4331,6 +4331,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
 static void
 compose_clone_action(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
 {
+bool old_was_mpls = ctx->was_mpls;
 bool old_conntracked = ctx->conntracked;
 struct flow old_flow = ctx->xin->flow;
 
@@ -4355,6 +4356,10 @@ compose_clone_action(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 /* The clone's conntrack execution should have no effect on the original
  * packet. */
 ctx->conntracked = old_conntracked;
+
+/* Popping MPLS from the clone should have no effect on the original
+ * packet. */
+ctx->was_mpls = old_was_mpls;
 }
 
 static bool
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] ovs-vswitch kernel panic randomly started after 400+ days uptime

[Uri Foox]

Hi,

Since about 10 days ago, every few hours, one of our 10 compute nodes on
our Openstack cluster kernel panics at the host level kernel panics
(captured through netconsole). The kernel panic is identical across all 10
nodes and happens at random times but at least 1 node kernel panics every
3-12 hours. We have tried numerous things including upgrading the kernel
(Ubuntu 12.04 LTS running 3.1.0-106-generic), modifying sysctl, restarting
switches, restarting all openstack networking services, changing BIOS
settings etc...but no luck. We have not restarted the control nodes or the
Juniper switch that routes all inbound internet traffic.

Based on research we did around skbuff.h we found two kernel patches to
address a checksum failure and also some OVS discussions about it. I was
hoping that the kernel upgrade would solve it but it did not. I do not know
if Openstack will tolerate us upgrading OVS and the fact that it started
completely randomly leads me to believe it's some other factor that we are
unaware of.


   - https://patchwork.ozlabs.org/patch/512625/
   -
   
https://github.com/openvswitch/ovs/commit/51b7a90217369f6bbbf164ba471f54ec2817665e
   - https://patchwork.kernel.org/patch/7475491/
   - https://patchwork.ozlabs.org/patch/523632/


Here is one of them. If you have any ideas what we can do, please let me
know.

Thanks,
Uri


Connection from 172.25.2.157 port 5404 [udp/*] accepted
[68240.441681] [ cut here ]
[68240.496918] kernel BUG at
/build/linux-lts-trusty-D60X6T/linux-lts-trusty-3.13.0/include/linux/skbuff.h:1486!
[68240.615520] invalid opcode:  [#1] SMP
[68240.664751] Modules linked in: netconsole configfs xt_mac xt_physdev
xt_set ip_set_hash_ip ip_set nfnetlink vhost_net macvtap macvlan vhost veth
bridge stp llc ipt_REJECT xt_state xt_conntrack xt_multiport xt_CT
xt_comment iptable_raw xt_CHECKSUM xt_tcpudp iptable_mangle ipt_MASQUERADE
iptable_nat nf_nat_ipv4 nf_nat ip6table_filter ip6_tables iptable_filter
ip_tables ebtable_nat ebtables x_tables kvm_intel kvm nbd ib_iser rdma_cm
ib_cm iw_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp libiscsi_tcp libiscsi
scsi_transport_iscsi openvswitch vxlan ip_tunnel gre nfsd nfs_acl
auth_rpcgss nfs fscache lockd sunrpc dm_multipath gpio_ich dcdbas scsi_dh
mei_me shpchp sb_edac mei edac_core lpc_ich joydev acpi_power_meter
nf_conntrack_ipv6 mac_hid nf_defrag_ipv6 wmi nf_conntrack_ipv4 ipmi_si xfs
nf_conntrack nf_defrag_ipv4 lp parport igb btrfs hid_generic dca
i2c_algo_bit usbhid raid6_pq ptp ahci bnx2x hid libahci mdio megaraid_sas
pps_core xor libcrc32c
[68241.670838] CPU: 33 PID: 0 Comm: swapper/33 Not tainted
3.13.0-106-generic #153~precise1-Ubuntu
[68241.774871] Hardware name: Dell Inc. PowerEdge R820/066N7P, BIOS 2.2.3
07/09/2014
[68241.864406] task: 881028b94800 ti: 881028ba task.ti:
881028ba
[68241.953939] RIP: 0010:[]  []
__skb_pull.part.7+0x4/0x6 [openvswitch]
[68242.067531] RSP: 0018:88203fb03b08  EFLAGS: 00010297
[68242.131087] RAX: 88165c791966 RBX: 88202639e900 RCX:
88165c791900
[68242.216458] RDX: 0210 RSI: 001a RDI:
0214
[68242.301842] RBP: 88203fb03b08 R08:  R09:
0140
[68242.387207] R10: 000c R11: 72221c0c R12:
88203fb03b70
[68242.472576] R13: 88402794d0c0 R14: 88203fb03b70 R15:
88302324e180
[68242.557945] FS:  () GS:88203fb0()
knlGS:
[68242.654780] CS:  0010 DS:  ES:  CR0: 80050033
[68242.723550] CR2: 7f9c7466ab90 CR3: 00302689e000 CR4:
000427e0
[68242.808931] Stack:
[68242.832981]  88203fb03b38 a0524e64 8112d1e1
88202639e900
[68242.921980]  e8e000305800 88402794d0c0 88203fb03c28
a0523a80
[68243.010963]  88203fb13180 88203fb03b90 810a090b
0001
[68243.099945] Call Trace:
[68243.129188]  
[68243.152204]  [] ovs_flow_extract+0x664/0x720
[openvswitch]
[68243.238893]  [] ? tracing_record_cmdline+0x21/0x50
[68243.314912]  []
ovs_dp_process_received_packet+0x60/0x130 [openvswitch]
[68243.412793]  [] ? ttwu_do_wakeup+0xfb/0x110
[68243.481559]  [] ovs_vport_receive+0x2a/0x30
[openvswitch]
[68243.564884]  [] gre_rcv+0xa4/0xb8 [openvswitch]
[68243.637802]  [] gre_cisco_rcv+0x75/0xbc [gre]
[68243.708621]  [] gre_rcv+0x65/0x90 [gre]
[68243.773214]  [] ip_local_deliver_finish+0xa8/0x220
[68243.849244]  [] ip_local_deliver+0x4b/0x90
[68243.916951]  [] ip_rcv_finish+0x121/0x380
[68243.983627]  [] ip_rcv+0x286/0x380
[68244.043023]  [] __netif_receive_skb_core+0x61a/0x760
[68244.121122]  [] __netif_receive_skb+0x21/0x70
[68244.191942]  [] process_backlog+0xb1/0x190
[68244.259642]  [] net_rx_action+0x139/0x280
[68244.326305]  [] __do_softirq+0xed/0x360
[68244.390887]  [] irq_exit+0x11e/0x140
[68244.452358]  [] do_IRQ+0x63/0xe0
[68244.509674]  [] common_interrupt+0x6d/0x6d
[68244.577366]  
[68244.600371]  [] ? finish_task_switch+0x53/0x160

Re: [ovs-dev] Issues with the use of the clone action for resubmission to the pipeline

[Jarno Rajahalme]

> On Jan 5, 2017, at 4:28 PM, Ben Pfaff  wrote:
> 
> On Tue, Jan 03, 2017 at 02:55:19AM -0800, Mickey Spiegel wrote:
>> One of the motivations for clone is to use it as a lightweight way to
>> resubmit to an earlier table at the beginning of the pipeline, without
>> incurring all of the overhead associated with openflow patch ports.
>> One such usage is in OVN, where a recent patch set replaced the
>> use of openflow patch ports with clone, for OVN patch ports within
>> the same bridge (br-int).
>> 
>> Over the holidays, some issues arose related to this usage of clone
>> (see the thread ovn ping from VM to external gateway IP failed).
> 
> Thanks for bringing this up.  I had overlooked these questions and this
> issue.
> 
>> While investigating these issues, I noticed some significant
>> differences between the way flow and other context information
>> is saved and restored when using openflow patch ports, versus
>> the way it is done with clone. At least one such difference, with
>> regard to conntrack, is causing failures.
> 
> What's the difference that is causing failures?  I do not know about it,
> so it is a high priority to fix it.  Is it already fixed, or do we need
> to do something about it?
> 
>> I would classify the differences (those for which there is no current
>> workaround by specifying nested actions within the clone) into two
>> categories:
>> 1. State that is kept outside of flow, which is currently saved,
>>   cleared, and restored when using openflow patch ports.
>>   This includes:
>>   ctx->wc->masks.tunnel
> 
> I think that clone should not save and restore this.  The situation is
> different from a patch port because clone does not change the ingress
> port.  The patch port code has the following comment:
> 
>/* Since this packet came in on a patch port (from the perspective of
> * the peer bridge), it cannot have useful tunnel information. As a
> * result, any wildcards generated on that tunnel also cannot be valid.
> * The tunnel wildcards must be restored to their original version 
> since
> * the peer bridge uses a separate tunnel metadata table and therefore
> * any generated wildcards will be garbage in the context of our
> * metadata table. */
>ctx->wc->masks.tunnel = old_flow_tnl_wc;
> 
>>   ctx->conntracked
> 
> I guess that we should save and restore this since we're saving and
> restoring the conntrack metadata.  I've written up a patch.
> 
>>   ctx->was_mpls
> 
> I do not think that that this is a correctness issue, but it's a nice
> optimization to save and restore this.  I've written up a patch.

I think the rationale is that ‘was_mpls’ is used to track the validity of the 
flow key across an MPLS POP operation. Since the flow key is saved and 
restored, we should save and restore ‘was_mpls’ as well.

> 
>>   ctx->xin->tables_version (not an issue if bridge does not change)
> 
> clone doesn't change the bridge, so this shouldn't matter.
> 
>>   ctx->stack
>>   ctx->action_set
> 
> I think it's cleanest if a clone starts off with both of these empty and
> saves and restores them.  I've written up a patch.

I think saving and restoring is needed, but I’m not so sure of clearing these. 
However, it seems that there is no way for the action set to be executed within 
the clone, so I guess it does not matter. It would be good to add these changes 
to the documentation as well.

> 
>>   ctx->xbridge (not an issue if bridge does not change)
>>   ctx->mirrors (not an issue if bridge does not change)
> 
> clone doesn't change the bridge, so these shouldn't matter.
> 
>> 2. State that resides inside the flow, but for which there is no
>>   workaround to clear or reset the fields. This includes:
>>   flow->tunnel ?
> 
> Like the tunnel mask, this doesn't matter for clone, because clone
> doesn't change the ingress port.
> 
>>   flow->tunnel.metadata.tab (not an issue if bridge does not change)
> 
> clone doesn't change the bridge, so this shouldn't matter.
> 
>>   flow->ct_state (read only)
>>   flow->ct_mark (restricted so can only be set in ct action)
>>   flow->ct_label (restricted so can only be set in ct action)
> 
> It's not obvious to me that clone should always clear these.  However,
> if it saves and restores them, then we can support clearing them by
> adding a ct_clear action.  I've written up a patch.
> 
> I sent out a series that addresses these issues.  Probably, we also need
> to modify OVN to insert a "ct_clear" action into its use of "clone", but
> my series doesn't do that.
> 
> The series starts here:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327204.html
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCHv2] python: Fix nroff indentation for after .

[Joe Stringer]

When XML is used for writing manpages, in the case that there is a 
tag followed by , the nroff python utility indents the  tag (and
children) an extra level which is unnecessary and makes the formatting
inconsistent between manpages written directly in nroff vs manpages
written in XML and converted to nroff. Fix the indentation by removing
the extraneous .RS / .RE tags added to generated nroff in this case.

This fixes the formatting of ovn/utilities/ovn-nbctl.8 man page.

Signed-off-by: Joe Stringer 
---
 python/build/nroff.py | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/python/build/nroff.py b/python/build/nroff.py
index aed60ebbd592..8af1719f9dc4 100644
--- a/python/build/nroff.py
+++ b/python/build/nroff.py
@@ -221,6 +221,7 @@ fillval = .2
 
 def block_xml_to_nroff(nodes, para='.PP'):
 s = ''
+prev = ''
 for node in nodes:
 if node.nodeType == node.TEXT_NODE:
 s += text_to_nroff(node.data)
@@ -248,9 +249,13 @@ def block_xml_to_nroff(nodes, para='.PP'):
   " children" % node.tagName)
 s += ".RE\n"
 elif node.tagName == 'dl':
+indent = True
+if prev == 'h1':
+indent = False
 if s != "":
 s += "\n"
-s += ".RS\n"
+if indent:
+s += ".RS\n"
 prev = "dd"
 for li_node in node.childNodes:
 if (li_node.nodeType == node.ELEMENT_NODE
@@ -272,7 +277,8 @@ def block_xml_to_nroff(nodes, para='.PP'):
 raise error.Error(" element may only have "
   " and  children")
 s += block_xml_to_nroff(li_node.childNodes, ".IP")
-s += ".RE\n"
+if indent:
+s += ".RE\n"
 elif node.tagName == 'p':
 if s != "":
 if not s.endswith("\n"):
@@ -300,6 +306,7 @@ def block_xml_to_nroff(nodes, para='.PP'):
 s += diagram_to_nroff(node.childNodes, para)
 else:
 s += inline_xml_to_nroff(node, r'\fR')
+prev = node.tagName
 elif node.nodeType == node.COMMENT_NODE:
 pass
 else:
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/5] ofproto-dpif-xlate: Make "clone" save action set and stack.

[Mickey Spiegel]

On Thu, Jan 5, 2017 at 4:28 PM, Ben Pfaff  wrote:

> This is a design decision but it seems conceptually cleaner than having
> them leak through into the clone.
>

I agree that saving and restoring is a good thing.

Whether to clear, as Jarno brought up, that is a good question.
I guess it would be more flexible to allow a clone to pop things
off the stack, at its own risk?
We cannot really predict all of the places where clone might be
used in the future.
In most cases the cloned packet will not pop off more than it
pushes, so I don't think leaving the stack there can do much
damage.

>
> Reported-by: Mickey Spiegel 
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> January/326981.html
> Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
> Signed-off-by: Ben Pfaff 
>

Acked-by: Mickey Spiegel 


> ---
>  ofproto/ofproto-dpif-xlate.c | 14 ++
>  utilities/ovs-ofctl.8.in |  3 ++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index b02e317..6736c12 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4334,8 +4334,22 @@ compose_clone_action(struct xlate_ctx *ctx, const
> struct ofpact_nest *oc)
>  bool old_conntracked = ctx->conntracked;
>  struct flow old_flow = ctx->xin->flow;
>
> +struct ofpbuf old_stack = ctx->stack;
> +union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
> +ofpbuf_use_stub(>stack, new_stack, sizeof new_stack);
> +
> +struct ofpbuf old_action_set = ctx->action_set;
> +uint64_t actset_stub[1024 / 8];
> +ofpbuf_use_stub(>action_set, actset_stub, sizeof actset_stub);
> +
>  do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
>
> +ofpbuf_uninit(>action_set);
> +ctx->action_set = old_action_set;
> +
> +ofpbuf_uninit(>stack);
> +ctx->stack = old_stack;
> +
>  ctx->xin->flow = old_flow;
>
>  /* The clone's conntrack execution should have no effect on the
> original
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 72aa89a..2f9606b 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -2628,7 +2628,8 @@ in Open vSwitch 2.4.
>  Executes each nested \fIaction\fR, saving much of the packet and
>  pipeline state beforehand and then restoring it afterward.  The state
>  that is saved and restored includes all flow data and metadata
> -(including, for example, \fBct_state\fR).
> +(including, for example, \fBct_state\fR), the stack accessed by
> +\fBpush\fR and \fBpop\fR actions, and the OpenFlow action set.
>  .IP
>  This action was added in Open vSwitch 2.6.90.
>  .RE
> --
> 2.10.2
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] python: Add TCP/SSL probes for OVSDB python lib

[Guoshuai Li]

On Thu, Jan 05, 2017 at 09:58:11AM -0500, Russell Bryant wrote:

On Wed, Jan 4, 2017 at 6:41 PM, Ben Pfaff  wrote:


On Sun, Jan 01, 2017 at 07:04:55PM +0800, Guoshuai Li wrote:

stream_or_pstream_needs_probes always return 0. This causes TCP/SSL
connection not be probed, and no reconnect when the connection
is aborted

Signed-off-by: Guoshuai Li 

Applied to master, thanks!

Are you OK with backporting this to branch-2.6?  I don't mind doing it if
so.

Fine with me.  I considered it but at least superficially it had patch
rejects and I did not look further.
This patch conflicts with branch-2.6 because of SSLStream, so I recommit 
a patch:

https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327237.html
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn: Support ARP proxy in logical switches.

[Han Zhou]

On Wed, Jan 4, 2017 at 3:29 PM, Ben Pfaff  wrote:
>
> [adding Bruce to leverage his knowledge of networking, if he's willing]

Thanks! It would be great to get some views from networking experts.

>
> It seems like the mechanics of this patch are mostly OK.  There is at
> least one inconsistency in ovn-nb.xml, where it says in one place that
> the new option is for all port types and in another that it's only for
> ports with an empty 'type'.  (For the record, if it's for every port
> type, then it should be in other_config.  'options' is intended for
> type-specific configuration, not for generic configuration.)

Ah, my bad. In the beginning I thought it may be applicable for any type,
but then I realized it is not. My current use case is for empty type only.
I'll move the text to the section "VMI (or VIF) Options". It may also be
useful for router type, but not likely for l2gateway, and definitely not
applicable for localnet.

>
> However, I'm concerned about the general utility here.  I usually think
> of proxy ARP as being used for the kinds of applications you see in the
> wikipedia on proxy ARP: https://en.wikipedia.org/wiki/Proxy_ARP.  This
> seems to aimed at a different application that is more like a weak form
> of service function chaining than for the traditional applications of
> proxy ARP.
>
> Is this an appropriate application for proxy ARP?  Is it a common enough
> use case to support in OVN?  Should it instead be handled through a more
> general service function chaining interface?
>

I have similar concerns about how useful it is. I agree it is kind of
lightweight SFC, but in my opinion it is not contradict with the fact that
it is also Proxy ARP. At least the firewall example (the 3rd use case)
described in https://en.wikipedia.org/wiki/Proxy_ARP seems to be very
close. SFC may be a more general approach to redirect packets to a
firewall, but it is heavier, and may be difficult in a non-overlay
environment, while Proxy ARP is sufficient and simple for such specific use
cases when the nodes are L2 adjacent. So I think it may be good to have
such feature in OVN, if the change is not intrusive.

> Bruce, the first file in the patch explains the application here pretty
> well, that's probably the bit that you should read.
>
> Thanks,
>
> Ben.
>
> On Thu, Dec 22, 2016 at 09:43:16PM -0800, Han Zhou wrote:
> > This patch support "arp_proxy" option for logical switch ports. If
> > a lsp with arp_proxy=true, all the arp request to known ipv4
> > addresses on the ls will be responded with the arp proxy lsp's MAC
> > address, except when the arp request come from the arp proxy lsp
> > itself.
> >
> > Signed-off-by: Han Zhou 
> > ---
> >  ovn/northd/ovn-northd.8.xml |  23 
> >  ovn/northd/ovn-northd.c | 119
--
> >  ovn/ovn-nb.xml  |  11 
> >  tests/ovn.at| 136

> >  4 files changed, 271 insertions(+), 18 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index f3c1682..aefa086 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> > @@ -446,6 +446,19 @@
> >  
> >
> >  
> > +  In particular, if a logical switch port has option proxy_arp set
to
> > +  true, we call it an arp-proxy port. In such case, ARP requests to
> > +  IPv4 addresses of any logical port on the same logical switch
will
> > +  be responded with the MAC address of that arp-proxy port, except
when
> > +  the request comes from that arp-proxy port, which can still
learn the
> > +  real MAC of destination VM/Container. This feature is useful if
we
> > +  want all the traffic to/from any logical ports on the logical
switch
> > +  being forwarded to a particular logical port, which may act as a
proxy
> > +  and will forward the traffic to the real destination after some
pre-
> > +  processing.
> > +
> > +
> > +
> >ARP requests arrive from VMs from a logical switch inport of type
> >default.  For this case, the logical switch proxy ARP rules can
be
> >for other VMs or logical router ports.  Logical switch proxy ARP
> > @@ -521,6 +534,16 @@ output;
> >  
> >
> >  
> > +  If arp-proxy port is present in the logical switch, the above
> > +  mentioned Ethernet address E will be the one
belonging
> > +  to the arp-proxy port, and there will be Priority-60 flows
that
> > +  match ARP requests from the arp-proxy port to each known IP
address
> > +  of every logical switch port, and respond with ARP replies
in the
> > +  same format as above but with Ethernet address E
being
> > +  the real one belonging to each target logical switch port.
> > +
> > +
> > +
> >These flows are omitted for logical ports (other than router
ports)
> >

[ovs-dev] [PATCH v2] dpcls: Avoid one 8-byte chunk in subtable mask.

[antonio . fischetti]

This patch allows to skip the chunk comprising of dp_hash and in_port
in the subtable mask when the packet is not recirculated.  This will
slightly speed up the hash computation as one expensive function call
to hash_add64() can be skipped.
For each new netdev flow we wildcard in_port in the mask, so in the
physical case where dp_hash is also wildcarded, the resulting 8-byte
chunk will not be part of the subtable mask.

The idea behind is that all the packets within a given dpcls will
have the same in_port value and typically dp_hash == 0.  So they will
have the same 8-bytes chunk in their {dp_hash, in_port} portion of the
miniflow. This doesn't contribute effectively in spreading hash values
and avoiding collisions.

v2: Using ovs_assert

Signed-off-by: Antonio Fischetti 
Signed-off-by: Bhanuprakash Bodireddy 
Co-authored-by: Bhanuprakash Bodireddy 
Signed-off-by: Jarno Rajahalme 
Co-authored-by: Jarno Rajahalme 
---
 lib/dpif-netdev.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1a47a45..68b434f 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2278,7 +2278,18 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
 struct dpcls *cls;
 odp_port_t in_port = match->flow.in_port.odp_port;
 
+/* As we select the dpcls based on the port number, each netdev flow
+ * belonging to the same dpcls will have the same odp_port value.
+ * For performance reasons here we wildcard odp_port in the mask.  In the
+ * physical case where dp_hash is also wildcarded, the resulting 8-byte
+ * chunk {dp_hash, in_port} will be ignored by netdev_flow_mask_init() and
+ * will not be part of the subtable mask.
+ * This will speed up the hash computation during dpcls_lookup() because
+ * one call to hash_add64() will be skipped. */
+ovs_assert(match->wc.masks.in_port.odp_port == ODPP_NONE);
+match->wc.masks.in_port.odp_port = 0;
 netdev_flow_mask_init(, match);
+match->wc.masks.in_port.odp_port = ODPP_NONE;
 /* Make sure wc does not have metadata. */
 ovs_assert(!FLOWMAP_HAS_FIELD(, metadata)
&& !FLOWMAP_HAS_FIELD(, regs));
-- 
2.4.11

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpcls: Avoid one 8-byte chunk in subtable mask.

[Fischetti, Antonio]

Thanks Jarno, will create a v2.
Antonio

> -Original Message-
> From: Jarno Rajahalme [mailto:ja...@ovn.org]
> Sent: Wednesday, January 4, 2017 9:43 PM
> To: Fischetti, Antonio 
> Cc: d...@openvswitch.org; Bodireddy, Bhanuprakash
> 
> Subject: Re: [PATCH] dpcls: Avoid one 8-byte chunk in subtable mask.
> 
> 
> > On Jan 4, 2017, at 6:41 AM, antonio.fische...@intel.com wrote:
> >
> > This patch allows to skip the chunk comprising of dp_hash and in_port
> > in the subtable mask when the packet is not recirculated.  This will
> > slightly speed up the hash computation as one expensive function call
> > to hash_add64() can be skipped.
> > For each new netdev flow we wildcard in_port in the mask, so in the
> > physical case where dp_hash is also wildcarded, the resulting 8-byte
> > chunk will not be part of the subtable mask.
> >
> > The idea behind is that all the packets within a given dpcls will
> > have the same in_port value and typically dp_hash == 0.  So they will
> > have the same 8-bytes chunk in their {dp_hash, in_port} portion of the
> > miniflow. This doesn't contribute effectively in spreading hash values
> > and avoiding collisions.
> >
> > Signed-off-by: Antonio Fischetti 
> > Signed-off-by: Bhanuprakash Bodireddy 
> > Co-authored-by: Bhanuprakash Bodireddy
> 
> > Signed-off-by: Jarno Rajahalme 
> > Co-authored-by: Jarno Rajahalme 
> > ---
> > lib/dpif-netdev.c | 14 +-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 1a47a45..a808f4b 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2277,8 +2277,20 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread
> *pmd,
> > struct netdev_flow_key mask;
> > struct dpcls *cls;
> > odp_port_t in_port = match->flow.in_port.odp_port;
> > -
> > +uint32_t old_odp_port;
> > +
> > +/* As we select the dpcls based on the port number, each netdev
> flow
> > + * belonging to the same dpcls will have the same odp_port value.
> > + * For performance reasons here we wildcard odp_port in the mask.
> In the
> > + * physical case where dp_hash is also wildcarded, the resulting 8-
> byte
> > + * chunk {dp_hash, in_port} will be ignored by
> netdev_flow_mask_init() and
> > + * will not be part of the subtable mask.
> > + * This will speed up the hash computation during dpcls_lookup()
> because
> > + * one call to hash_add64() will be skipped. */
> > +old_odp_port = match->wc.masks.in_port.odp_port;
> 
> 
> Please move this line further down in the function here and do not save
> the old value:
> 
> ovs_assert(match->wc.masks.in_port.odp_port == ODPP_NONE);
> 
> > +match->wc.masks.in_port.odp_port = 0;
> > netdev_flow_mask_init(, match);
> > +match->wc.masks.in_port.odp_port = old_odp_port;
> 
> And do this instead:
> 
> match->wc.masks.in_port.odp_port = ODPP_NONE;
> 
> > /* Make sure wc does not have metadata. */
> > ovs_assert(!FLOWMAP_HAS_FIELD(, metadata)
> >&& !FLOWMAP_HAS_FIELD(, regs));
> > --
> > 2.4.11
> >

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] ovn support DPDK?

[赵占旭]

Thanks very much!
With your advice, I resolve the problem.
I thought ovn only support one bridge: br-int, now I add one.








At 2017-01-05 01:44:26, "Ben Pfaff"  wrote:
>On Tue, Jan 03, 2017 at 05:25:26PM +0800, 赵占旭 wrote:
>> I installed ovn and ovs-dpdk. when I created two VMs on one host, they could 
>> communicate, but I created two VMs on diffrent hosts, they couldn't 
>> communicate.
>> I think the problem is geneve, ovs and ovs-dpdk has different flows, so my 
>> question is the ovn support dpdk?
>> I need to configure sth, or I need patch, or I need to modify code??? orz
>
>Probably, you need to configure a physical bridge and set up routes, so
>that OVS-DPDK can properly tunnel packets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] util: Remove obsolete comment.

[Simon Horman]

On Wed, Jan 04, 2017 at 09:23:54AM -0800, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff 

Acked-by: Simon Horman 

> ---
>  include/openvswitch/util.h | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
> index 06bd2d0..8453550 100644
> --- a/include/openvswitch/util.h
> +++ b/include/openvswitch/util.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2016, 2017 
> Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -73,11 +73,7 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const 
> char *, const char *);
>  ((void) sizeof ((int) ((POINTER) == (TYPE) (POINTER
>  
>  /* Casts 'pointer' to 'type' and issues a compiler warning if the cast 
> changes
> - * anything other than an outermost "const" or "volatile" qualifier.
> - *
> - * The cast to int is present only to suppress an "expression using sizeof
> - * bool" warning from "sparse" (see
> - * http://permalink.gmane.org/gmane.comp.parsers.sparse/2967). */
> + * anything other than an outermost "const" or "volatile" qualifier. */
>  #define CONST_CAST(TYPE, POINTER)   \
>  (BUILD_ASSERT_TYPE(POINTER, TYPE),  \
>   (TYPE) (POINTER))
> -- 
> 2.10.2
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] docs: Fix formatting of patch comments line.

[Stephen Finucane]

On Wed, 2017-01-04 at 13:58 -0800, Joe Stringer wrote:
> Sphinx was formatting the `---` as an extended dash, not verbatim as
> three hyphens (which is what is necessary for git to determine that
> it's
> a comment, and ignore it when applying the patch).
> 
> Signed-off-by: Joe Stringer 

Aye, good catch.

Acked-by: Stephen Finucane 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Questions about implementation of meters in ovs

[Hsu YuTing]

Hello All,

As far as i know that currently meters is not available in latest ovs 
release. Are there any alternative ways for

experimenting  the meters in ovs? Does anyone know when meters will be 
officially supported in ovs? Thanks!

Best Regards , YT Hsu





___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v5 1/3] netdev-dpdk: add hotplug support

[Ciara Loftus]

From: Mauricio Vásquez 

In order to use dpdk ports in ovs they have to be bound to a DPDK
compatible driver before ovs is started.

This patch adds the possibility to hotplug (or hot-unplug) a device
after ovs has been started. The implementation adds two appctl commands:
netdev-dpdk/attach and netdev-dpdk/detach

After the user attaches a new device, it has to be added to a bridge
using the add-port command, similarly, before detaching a device,
it has to be removed using the del-port command.

Signed-off-by: Mauricio Vasquez B 
Signed-off-by: Ciara Loftus 
Co-authored-by: Ciara Loftus 
---
Changelog:
* Use xasprintf instead of snprintf when reporting attach/detach errors.
* Updated format of link in docs to comply with new format.
* Remove 'port-' from appctl netdev-dpdk/(port-)attach and detach commands
* Free 'response' in attach and detach functions in netdev_dpdk
* Add my Sign-off and Co-author tags
* Fix docs - use port name for detach not PCI
* Rewording of docs
* Fix docs style & improve wording

 Documentation/howto/dpdk.rst | 27 +
 NEWS |  1 +
 lib/netdev-dpdk.c| 95 +---
 3 files changed, 117 insertions(+), 6 deletions(-)

diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index 2361f3a..13da548 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -298,6 +298,33 @@ physical ports which in turn effects the non-tunnel 
traffic performance.
 So it is advised to turn off the Rx checksum offload for non-tunnel traffic use
 cases to achieve the best performance.
 
+.. _port-hotplug:
+
+Port Hotplug
+
+
+OVS supports port hotplugging, allowing the use of ports that were not bound
+to DPDK when vswitchd was started.
+In order to attach a port, it has to be bound to DPDK using the
+``dpdk_nic_bind.py`` script::
+
+$ $DPDK_DIR/tools/dpdk_nic_bind.py --bind=igb_uio :01:00.0
+
+Then it can be attached to OVS::
+
+$ ovs-appctl netdev-dpdk/attach :01:00.0
+
+At this point, the user can create a dpdk port using the ``add-port`` command.
+
+It is also possible to detach a port from ovs, the user has to remove the
+port using the del-port command, then it can be detached using::
+
+$ ovs-appctl netdev-dpdk/detach dpdk0
+
+This feature is not supported with VFIO and does not work with some NICs.
+For more information please refer to the `DPDK Port Hotplug Framework
+`__.
+
 .. _dpdk-ovs-in-guest:
 
 OVS with DPDK Inside VMs
diff --git a/NEWS b/NEWS
index daa9ff5..8f8c2f7 100644
--- a/NEWS
+++ b/NEWS
@@ -47,6 +47,7 @@ Post-v2.6.0
which set the number of rx and tx descriptors to use for the given port.
  * Support for DPDK v16.11.
  * Support for rx checksum offload. Refer DPDK HOWTO for details.
+ * Port Hotplug is now supported.
- Fedora packaging:
  * A package upgrade does not automatically restart OVS service.
- ovs-vswitchd/ovs-vsctl:
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 8cfb985..b716b57 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2357,6 +2357,82 @@ netdev_dpdk_set_admin_state(struct unixctl_conn *conn, 
int argc,
 unixctl_command_reply(conn, "OK");
 }
 
+static void
+netdev_dpdk_attach(struct unixctl_conn *conn, int argc OVS_UNUSED,
+   const char *argv[], void *aux OVS_UNUSED)
+{
+int ret;
+char *response;
+uint8_t port_id;
+
+ovs_mutex_lock(_mutex);
+
+ret = rte_eth_dev_attach(argv[1], _id);
+if (ret < 0) {
+response = xasprintf("Error attaching device '%s'", argv[1]);
+ovs_mutex_unlock(_mutex);
+unixctl_command_reply_error(conn, response);
+free(response);
+return;
+}
+
+response = xasprintf("Device '%s' has been attached as 'dpdk%d'",
+ argv[1], port_id);
+
+ovs_mutex_unlock(_mutex);
+unixctl_command_reply(conn, response);
+free(response);
+}
+
+static void
+netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
+   const char *argv[], void *aux OVS_UNUSED)
+{
+int ret;
+char *response;
+unsigned int parsed_port;
+uint8_t port_id;
+char devname[RTE_ETH_NAME_MAX_LEN];
+
+ovs_mutex_lock(_mutex);
+
+ret = dpdk_dev_parse_name(argv[1], "dpdk", _port);
+if (ret) {
+response = xasprintf("'%s' is not a valid port", argv[1]);
+goto error;
+}
+
+port_id = parsed_port;
+
+struct netdev *netdev = netdev_from_name(argv[1]);
+if (netdev) {
+netdev_close(netdev);
+response = xasprintf("Port '%s' is being used. Remove it before"
+ "detaching", argv[1]);
+goto error;
+}
+
+rte_eth_dev_close(port_id);
+
+

[ovs-dev] [PATCH v5 3/3] netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)

[Ciara Loftus]

Prior to this commit, the 'dpdk' port type could only be used for
physical DPDK devices. Now, virtual devices (or 'vdevs') are supported.
'vdev' devices are those which use virtual DPDK Poll Mode Drivers eg.
null, pcap. To add a DPDK vdev, a valid 'dpdk-devargs' must be set for
the given dpdk port. The format expected is 'eth_' where
'x' is a number between 0 and RTE_MAX_ETHPORTS -1.

For example to add a port that uses the 'null' DPDK PMD driver:

ovs-vsctl set Interface null0 options:dpdk-devargs=eth_null0

Not all DPDK vdevs have been verified to work at this point in time.

Signed-off-by: Ciara Loftus 
---
Changelog:
* Updated process_vdevargs to work with Daniele's incremental in the
  previous patch.
* Allow vdev detach
* Update docs to show af_packet example
* Fix af_packet docs example
* Fix style issues in docs

 Documentation/howto/dpdk.rst | 29 +
 NEWS |  1 +
 lib/netdev-dpdk.c| 35 +++
 vswitchd/vswitch.xml |  9 +++--
 4 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index 1ff672c..fbb4b53 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -324,6 +324,35 @@ This feature is not supported with VFIO and does not work 
with some NICs.
 For more information please refer to the `DPDK Port Hotplug Framework
 `__.
 
+.. _vdev-support:
+
+Vdev Support
+
+
+DPDK provides drivers for both physical and virtual devices. Physical DPDK
+devices are added to OVS by specifying a valid PCI address in 'dpdk-devargs'.
+Virtual DPDK devices which do not have PCI addresses can be added using a
+different format for 'dpdk-devargs'.
+
+Typically, the format expected is 'eth_' where 'x' is a
+number between 0 and RTE_MAX_ETHPORTS -1 (31).
+
+For example to add a dpdk port that uses the 'null' DPDK PMD driver::
+
+   $ ovs-vsctl add-port br0 null0 -- set Interface null0 type=dpdk \
+   options:dpdk-devargs=eth_null0
+
+Similarly, to add a dpdk port that uses the 'af_packet' DPDK PMD driver::
+
+   $ ovs-vsctl add-port br0 myeth0 -- set Interface myeth0 type=dpdk \
+   options:dpdk-devargs=eth_af_packet0,iface=eth0
+
+More information on the different types of virtual DPDK PMDs can be found in
+the `DPDK documentation
+`__.
+
+Note: Not all DPDK virtual PMD drivers have been tested and verified to work.
+
 .. _dpdk-ovs-in-guest:
 
 OVS with DPDK Inside VMs
diff --git a/NEWS b/NEWS
index d66d402..cc319a9 100644
--- a/NEWS
+++ b/NEWS
@@ -53,6 +53,7 @@ Post-v2.6.0
with the old dpdk naming scheme is broken, and as such a
device will not be available for use until a valid dpdk-devargs is
specified.
+ * Virtual DPDK Poll Mode Driver (vdev PMD) support.
- Fedora packaging:
  * A package upgrade does not automatically restart OVS service.
- ovs-vswitchd/ovs-vsctl:
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e66ff27..79eddb5 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1134,25 +1134,19 @@ netdev_dpdk_lookup_by_port_id(int port_id)
 static int
 netdev_dpdk_process_devargs(const char *devargs)
 {
-struct rte_pci_addr addr;
 uint8_t new_port_id = UINT8_MAX;
 
-if (!eal_parse_pci_DomBDF(devargs, )) {
-/* Valid PCI address format detected - configure physical device */
-if (!rte_eth_dev_count()
-|| rte_eth_dev_get_port_by_name(devargs, _port_id)
-|| !rte_eth_dev_is_valid_port(new_port_id)) {
-/* PCI device not found in DPDK, attempt to attach it */
-if (!rte_eth_dev_attach(devargs, _port_id)) {
-/* Attach successful */
-VLOG_INFO("Device "PCI_PRI_FMT" has been attached to DPDK",
-  addr.domain, addr.bus, addr.devid, addr.function);
-} else {
-/* Attach unsuccessful */
-VLOG_INFO("Error attaching device "PCI_PRI_FMT" to DPDK",
-  addr.domain, addr.bus, addr.devid, addr.function);
-return -1;
-}
+if (!rte_eth_dev_count()
+|| rte_eth_dev_get_port_by_name(devargs, _port_id)
+|| !rte_eth_dev_is_valid_port(new_port_id)) {
+/* Device not found in DPDK, attempt to attach it */
+if (!rte_eth_dev_attach(devargs, _port_id)) {
+/* Attach successful */
+VLOG_INFO("Device '%s' attached to DPDK", devargs);
+} else {
+/* Attach unsuccessful */
+VLOG_INFO("Error attaching device '%s' to DPDK", devargs);
+return -1;
 }
 }
 
@@ -2459,17 +2453,10 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
 char *response;
 

[ovs-dev] [PATCH v5 2/3] netdev-dpdk: Arbitrary 'dpdk' port naming

[Ciara Loftus]

'dpdk' ports no longer have naming restrictions. Now, instead of
specifying the dpdk port ID as part of the name, the PCI address of the
device must be specified via the 'dpdk-devargs' option. eg.

ovs-vsctl add-port br0 my-port
ovs-vsctl set Interface my-port type=dpdk
  options:dpdk-devargs=:06:00.3

The user must no longer hotplug attach DPDK ports by issuing the
specific ovs-appctl netdev-dpdk/attach command. The hotplug is now
automatically invoked when a valid PCI address is set in the
dpdk-devargs. The format for ovs-appctl netdev-dpdk/detach command
has changed in that the user now must specify the relevant PCI address
as input instead of the port name.

Signed-off-by: Ciara Loftus 
Signed-off-by: Daniele Di Proietto 
Co-authored-by: Daniele Di Proietto 
Signed-off-by: Kevin Traynor 
Co-authored-by: Kevin Traynor 
---
Changelog:
* Keep port-detach appctl function - use PCI as input arg
* Add requires_mutex to devargs processing functions
* use reconfigure infrastructure for devargs changes
* process devargs even if valid portid ie. device already configured
* report err if dpdk-devargs is not specified
* Add Daniele's incremental patch & Sign-off + Co-author tags
* Update details of detach command to reflect PCI is needed instead of
  port name
* Update NEWS to mention that the new naming scheme is not backwards
  compatible
* Use existing DPDk functions to get port ID from PCI address/devname
* Merged process_devargs and process_pdevargs functions
* Removed unnecessary requires_mutex from devargs processing fn
* Fix case where port is re-attached after detach
* Add note to documentation that devices won't work until devargs set.
* Set netdev type and dpdk-devargs in one command in docs to avoid
  errors.
* Change port names in documentation to emphasise arbitrary-ness.
* Add Kevin's incremental & sign-off/co-authored-by
* Check if devargs string has changed before processing it as suggested
  by Daniele.
* Print error if attach fails

 Documentation/howto/dpdk.rst |   7 +-
 Documentation/intro/install/dpdk.rst |   9 +-
 NEWS |   5 +
 lib/netdev-dpdk.c| 181 ---
 vswitchd/vswitch.xml |   8 ++
 5 files changed, 148 insertions(+), 62 deletions(-)

diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index 13da548..1ff672c 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -312,14 +312,13 @@ In order to attach a port, it has to be bound to DPDK 
using the
 
 Then it can be attached to OVS::
 
-$ ovs-appctl netdev-dpdk/attach :01:00.0
-
-At this point, the user can create a dpdk port using the ``add-port`` command.
+$ ovs-vsctl add-port br0 dpdkx -- set Interface dpdkx type=dpdk \
+options:dpdk-devargs=:01:00.0
 
 It is also possible to detach a port from ovs, the user has to remove the
 port using the del-port command, then it can be detached using::
 
-$ ovs-appctl netdev-dpdk/detach dpdk0
+$ ovs-appctl netdev-dpdk/detach dpdkx
 
 This feature is not supported with VFIO and does not work with some NICs.
 For more information please refer to the `DPDK Port Hotplug Framework
diff --git a/Documentation/intro/install/dpdk.rst 
b/Documentation/intro/install/dpdk.rst
index 10ceabb..fff0a1a 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -252,8 +252,13 @@ ports. For example, to create a userspace bridge named 
``br0`` and add two
 ``dpdk`` ports to it, run::
 
 $ ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev
-$ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
-$ ovs-vsctl add-port br0 dpdk1 -- set Interface dpdk1 type=dpdk
+$ ovs-vsctl add-port br0 myportnameone -- set Interface myportnameone \
+type=dpdk options:dpdk-devargs=:06:00.0
+$ ovs-vsctl add-port br0 myportnametwo -- set Interface myportnametwo \
+type=dpdk options:dpdk-devargs=:06:00.1
+
+DPDK devices will not be available for use until a valid dpdk-devargs is
+specified.
 
 Refer to ovs-vsctl(8) and :doc:`/howto/dpdk` for more details.
 
diff --git a/NEWS b/NEWS
index 8f8c2f7..d66d402 100644
--- a/NEWS
+++ b/NEWS
@@ -48,6 +48,11 @@ Post-v2.6.0
  * Support for DPDK v16.11.
  * Support for rx checksum offload. Refer DPDK HOWTO for details.
  * Port Hotplug is now supported.
+ * DPDK physical ports can now have arbitrary names. The PCI address of
+   the device must be set using the 'dpdk-devargs' option. Compatibility
+   with the old dpdk naming scheme is broken, and as such a
+   device will not be available for use until a valid dpdk-devargs is
+   specified.
- Fedora packaging:
  * A package upgrade does not automatically restart OVS service.
- ovs-vswitchd/ovs-vsctl:
diff --git 

[ovs-dev] [PATCH v6 1/7] ovn: specify addresses of type "router" lsps as "router"

[Mickey Spiegel]

Currently in OVN, when a logical switch port of type "router" is
created, the MAC and optionally IP addresses of the peer logical
router port must be specified again as the addresses of the logical
switch port.

This patch allows the logical switch port's addresses to be
specified as the string "router", rather than explicitly copying the
logical router port's MAC and optionally IP addresses.  The router
addresses are used to populate the logical switch's destination
lookup, and to populate op->lsp_addrs in ovn-northd.c, which in turn
is used to generate logical switch ARP and ND replies.  Since ipam
already looks at logical router ports, the only ipam modification
necessary is to skip logical switch ports with addresses "router".

Signed-off-by: Mickey Spiegel 
---
 ovn/northd/ovn-northd.c   | 31 +--
 ovn/ovn-nb.xml| 16 
 ovn/utilities/ovn-nbctl.c |  1 +
 tests/ovn.at  |  3 ++-
 4 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index a28327b..5ad544d 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -815,7 +815,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, struct 
ovn_port *op,
   char *address)
 {
 if (!od || !op || !address || !strcmp(address, "unknown")
-|| is_dynamic_lsp_address(address)) {
+|| !strcmp(address, "router") || is_dynamic_lsp_address(address)) {
 return;
 }
 
@@ -1205,7 +1205,8 @@ join_logical_ports(struct northd_context *ctx,
 op->lsp_addrs
 = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses);
 for (size_t j = 0; j < nbsp->n_addresses; j++) {
-if (!strcmp(nbsp->addresses[j], "unknown")) {
+if (!strcmp(nbsp->addresses[j], "unknown")
+|| !strcmp(nbsp->addresses[j], "router")) {
 continue;
 }
 if (is_dynamic_lsp_address(nbsp->addresses[j])) {
@@ -1323,6 +1324,18 @@ join_logical_ports(struct northd_context *ctx,
 op->od->router_ports,
 sizeof *op->od->router_ports * (op->od->n_router_ports + 1));
 op->od->router_ports[op->od->n_router_ports++] = op;
+
+/* Fill op->lsp_addrs for op->nbsp->addresses[] with
+ * contents "router", which was skipped in the loop above. */
+for (size_t j = 0; j < op->nbsp->n_addresses; j++) {
+if (!strcmp(op->nbsp->addresses[j], "router")) {
+if (extract_lrp_networks(peer->nbrp,
+>lsp_addrs[op->n_lsp_addrs])) {
+op->n_lsp_addrs++;
+}
+break;
+}
+}
 } else if (op->nbrp && op->nbrp->peer) {
 struct ovn_port *peer = ovn_port_find(ports, op->nbrp->peer);
 if (peer) {
@@ -3123,6 +3136,20 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
*ports,
 ds_put_format(, "outport = %s; output;", op->json_key);
 ovn_lflow_add(lflows, op->od, S_SWITCH_IN_L2_LKUP, 50,
   ds_cstr(), ds_cstr());
+} else if (!strcmp(op->nbsp->addresses[i], "router")) {
+if (!op->peer || !op->peer->nbrp
+|| !ovs_scan(op->peer->nbrp->mac,
+ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))) {
+continue;
+}
+ds_clear();
+ds_put_format(, "eth.dst == "ETH_ADDR_FMT,
+  ETH_ADDR_ARGS(mac));
+
+ds_clear();
+ds_put_format(, "outport = %s; output;", op->json_key);
+ovn_lflow_add(lflows, op->od, S_SWITCH_IN_L2_LKUP, 50,
+  ds_cstr(), ds_cstr());
 } else {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
 
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index a3dc916..f53fb9b 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -519,6 +519,22 @@
 
   
 
+  router
+  
+
+  This indicates that the Ethernet, IPv4, and IPv6 addresses for
+  this logical switch port should be obtained from the connected
+  logical router port, as specified by router-port in
+  .
+
+
+
+  The resulting addresses are used to populate the logical
+  switch's destination lookup, and also for the logical switch
+  to generate ARP and ND replies.
+
+  
+
 
   
 
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index af1eeab..8ff5020 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c

[ovs-dev] [PATCH v6 3/7] ovn: Introduce "chassisredirect" port binding

[Mickey Spiegel]

Currently OVN handles all logical router ports in a distributed manner,
creating instances on each chassis.  The logical router ingress and
egress pipelines are traversed locally on the source chassis.

In order to support advanced features such as one-to-many NAT (aka IP
masquerading), where multiple private IP addresses spread across
multiple chassis are mapped to one public IP address, it will be
necessary to handle some of the logical router processing on a specific
chassis in a centralized manner.

The goal of this patch is to develop abstractions that allow for a
subset of router gateway traffic to be handled in a centralized manner
(e.g. one-to-many NAT traffic), while allowing for other subsets of
router gateway traffic to be handled in a distributed manner (e.g.
floating IP traffic).

This patch introduces a new type of SB port_binding called
"chassisredirect".  A "chassisredirect" port represents a particular
instance, bound to a specific chassis, of an otherwise distributed
port.  The ovn-controller on that chassis populates the "chassis"
column for this record as an indication for other ovn-controllers of
its physical location.  Other ovn-controllers do not treat this port
as a local port.

A "chassisredirect" port should never be used as an "inport".  When an
ingress pipeline sets the "outport", it may set the value to a logical
port of type "chassisredirect".  This will cause the packet to be
directed to a specific chassis to carry out the egress logical router
pipeline, in the same way that a logical switch forwards egress traffic
to a VIF port residing on a specific chassis.  At the beginning of the
egress pipeline, the "outport" will be reset to the value of the
distributed port.

For outbound traffic to be handled in a centralized manner, the
"outport" should be set to the "chassisredirect" port representing
centralized gateway functionality in the otherwise distributed router.
For outbound traffic to be handled in a distributed manner, locally on
the source chassis, the "outport" should be set to the existing "patch"
port representing distributed gateway functionality.

Inbound traffic will be directed to the appropriate chassis by
restricting source MAC address usage and ARP responses to that chassis,
or by running dynamic routing protocols.

Note that "chassisredirect" ports have no associated IP or MAC addresses.
Any pipeline stages that depend on port specific IP or MAC addresses
should be carried out in the context of the distributed port.

Although the abstraction represented by the "chassisredirect" port
binding is generalized, in this patch the "chassisredirect" port binding
is only created for NB logical router ports that specify the new
"redirect-chassis" option.  There is no explicit notion of a
"chassisredirect" port in the NB database.  The expectation is when
capabilities are implemented that take advantage of "chassisredirect"
ports (e.g. NAT), the addition of flows specifying a "chassisredirect"
port as the outport will also be triggered by the presence of the
"redirect-chassis" option.  Such flows are added for NB logical router
ports that specify the "redirect-chassis" option.

Signed-off-by: Mickey Spiegel 
---
 ovn/controller/binding.c|   8 +
 ovn/controller/ovn-controller.c |   4 +
 ovn/controller/physical.c   |  63 
 ovn/northd/ovn-northd.8.xml | 100 +++-
 ovn/northd/ovn-northd.c | 203 ++--
 ovn/ovn-nb.ovsschema|   9 +-
 ovn/ovn-nb.xml  |  38 +
 ovn/ovn-sb.xml  |  35 +
 ovn/utilities/ovn-trace.c   |  43 +-
 tests/ovn.at| 334 
 10 files changed, 816 insertions(+), 21 deletions(-)

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 2f24e9d..25592c2 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -355,6 +355,14 @@ consider_local_datapath(struct controller_ctx *ctx,
 add_local_datapath(ldatapaths, lports, binding_rec->datapath,
false, local_datapaths);
 }
+} else if (!strcmp(binding_rec->type, "chassisredirect")) {
+const char *chassis_id = smap_get(_rec->options,
+  "redirect-chassis");
+our_chassis = chassis_id && !strcmp(chassis_id, chassis_rec->name);
+if (our_chassis) {
+add_local_datapath(ldatapaths, lports, binding_rec->datapath,
+   false, local_datapaths);
+}
 } else if (!strcmp(binding_rec->type, "l3gateway")) {
 const char *chassis_id = smap_get(_rec->options,
   "l3gateway-chassis");
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 5bddcf3..131c900 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -155,6 +155,10 @@ 

[ovs-dev] [PATCH v6 0/7] ovn: add distributed NAT capability

[Mickey Spiegel]

Currently OVN supports NAT functionality by connecting each distributed
logical router to a centralized "l3gateway" router that resides on a
single chassis.  NAT is only carried out in the "l3gateway" router.

This patch set introduces NAT capability in the distributed logical
router itself, avoiding the need to pass through a transit logical
switch and a second logical router, and in many cases avoiding the need
to pass through a centralized chassis.

NAT functionality is associated with the logical router gateway port.
In order to support one-to-many SNAT (aka IP masquerading), where
multiple private IP addresses spread across multiple chassis are mapped
to a single public IP address, it will be necessary to handle some of
the logical router processing on a specific chassis in a centralized
manner.  Some NAT flows are handled in a distributed manner on all
chassis (following the local "patch" port as is normally done for
distributed logical routers), while other NAT flows are handled on a
centralized "redirect-chassis".

Possible future work items (hopefully not required for this patch set
to be accepted) include:
1. The NAT flows patch lifts the restriction that conntrack zones are
   only assigned to datapaths for gateway routers.  Given recent
   changes to ovn-controller, a hypervisor only sees the datapaths
   for which there is a port resident on this chassis, or datapaths
   reachable from ports resident on this chassis.  Is that good
   enough?  Or should conntrack zone assignment for datapaths be
   restricted further, perhaps only to logical router datapaths?
2. The current automated test for NAT flows is single node, so it does
   not cover the distributed functionality.  Full coverage requires a
   multi-node test with conntrack NAT capability, either in the kernel
   or userspace.  Is this possible?
   Multi-node tests have been added for the chassisdirect patch,
   testing non-NAT aspects of the distributed router gateway port.
3. Consider how to generalize distributed versus centralized handling
   of non-NAT traffic being output on the distributed gateway port.
   If MAC learning is used in the upstream network, then the
   distributed gateway port’s MAC address must be restricted to the
   redirect-chassis by using the chassisredirect port.  In the
   presence of dynamic protocols such as BGP EVPN, non-NAT traffic
   could be handled in a distributed manner.
4. Gratuitous ARP for NAT addresses needs to be updated for
   distributed NAT.
5. Add load balancing on the redirect chassis of an otherwise
   distributed logical router.

PATCH v5 -> PATCH v6
Added patch to automatically add router addresses to the addresses of
type "router" lsps.
Restricted logical switch destination lookup flows for logical router
distributed gateway port's MAC to the redirect chassis.
Automatically add distributed NAT MAC addresses to logical switch
destination lookup flows on the chassis where the NAT logical port resides.
Added tests for reachability from VIFs on the same logical switch as
localnet, through the logical router's distributed gateway port, to
internal VIFs.

PATCH v4 -> PATCH v5
Limited router ingress table 0 flow matching router ethernet address
on distributed gateway to redirect chassis.
Limited router ingress table 0 flows matching NAT ethernet address to
chassis where the NAT rule's logical port resides.
Rolled back changes to ICMP since they are not necessary.

PATCH v3 -> PATCH v4
Rebase

PATCH v2 -> PATCH v3
Added table to set egress loopback flag in the egress pipeline stage,
fixing east-west NAT across multiple chassis.

PATCH v1 -> PATCH v2
Added ovn-trace logic for chassisredirect ports, including automated test.
Added ovn-trace logic for egress loopback.
Fixed some bugs in ovn-trace register handling from ingress to egress,
and across patch ports (should these be filed separately as well?).

RFC v4 -> PATCH v1
Added egress loopback capability
Added east/west NAT tests to system-ovn.at (make check-kernel)
Added REGBIT_NAT_REDIRECT flows to IN_IP_ROUTING and IN_ARP_RESOLVE,
resolving remaining issues with east/west NAT

RFC v3 -> RFC v4
Rebased to pick up recent changes to ovn-controller, including a fix
to the localnet issue where VIFs had to be added on a chassis in order
to cause the localnet port to be instantiated.
The chassisredirect port logic was rewritten to avoid creating an
ofport.  Besides streamlining the code significantly, this fixed the
problem when the distributed port name was longer than 12 characters.
Restricted IPv6 ND replies for the router IP address to the redirect
chassis, similar to IPv4 ARP restrictions.
Added specific gateway redirect flows for unresolved ethernet
destination, so that ARP requests generated by the router are sent
through the redirect chassis regardless of NAT rules.
Relaxed checks in chassisredirect tests so that they are independent
of register assignments.
Renamed ovn-northd.c "l3gateway_port" to "l3dgw_port" in order to
avoid overlaps 

[ovs-dev] [PATCH v6 2/7] ovn: add is_chassis_resident match expression component

[Mickey Spiegel]

This patch introduces a new match expression component
is_chassis_resident().  Unlike match expression comparisons,
is_chassis_resident is not pushed down to OpenFlow.  It is a
conditional that is evaluated in the controller during expr_simplify(),
when it is replaced by a boolean expression.  The is_chassis_resident
conditional evaluates to "true" when the specified string identifies a
port name that is resident on this controller chassis, i.e., the
corresponding southbound database Port_Binding has a chassis column
that matches this chassis.  Otherwise it evaluates to "false".

This allows higher level features to specify flows that are only
installed on some chassis rather than on all chassis with the
corresponding datapath.

Suggested-by: Ben Pfaff 
Signed-off-by: Mickey Spiegel 
---
 include/ovn/expr.h  |  22 +-
 ovn/controller/lflow.c  |  39 --
 ovn/controller/lflow.h  |   5 +-
 ovn/controller/ovn-controller.c |   5 +-
 ovn/lib/expr.c  | 155 ++--
 ovn/utilities/ovn-trace.c   |  21 +-
 tests/ovn.at|  14 
 tests/test-ovn.c|  15 +++-
 8 files changed, 260 insertions(+), 16 deletions(-)

diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 371ba20..d3749fa 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -292,6 +292,15 @@ enum expr_type {
 EXPR_T_AND, /* Logical AND of 2 or more subexpressions. */
 EXPR_T_OR,  /* Logical OR of 2 or more subexpressions. */
 EXPR_T_BOOLEAN, /* True or false constant. */
+EXPR_T_CONDITION,   /* Conditional to be evaluated in the
+ * controller during expr_simplify(),
+ * prior to constructing OpenFlow matches. */
+};
+
+/* Expression condition type. */
+enum expr_cond_type {
+EXPR_COND_CHASSIS_RESIDENT, /* Check if specified logical port name is
+ * resident on the controller chassis. */
 };
 
 /* Relational operator. */
@@ -349,6 +358,14 @@ struct expr {
 
 /* EXPR_T_BOOLEAN. */
 bool boolean;
+
+/* EXPR_T_CONDITION. */
+struct {
+enum expr_cond_type type;
+bool not;
+/* XXX Should arguments for conditions be generic? */
+char *string;
+} cond;
 };
 };
 
@@ -375,7 +392,10 @@ void expr_destroy(struct expr *);
 
 struct expr *expr_annotate(struct expr *, const struct shash *symtab,
char **errorp);
-struct expr *expr_simplify(struct expr *);
+struct expr *expr_simplify(struct expr *,
+   bool (*is_chassis_resident)(const void *c_aux,
+   const char *port_name),
+   const void *c_aux);
 struct expr *expr_normalize(struct expr *);
 
 bool expr_honors_invariants(const struct expr *);
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 9f5341a..3814b09 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -64,12 +64,18 @@ struct lookup_port_aux {
 const struct sbrec_datapath_binding *dp;
 };
 
+struct condition_aux {
+const struct lport_index *lports;
+const struct sbrec_chassis *chassis;
+};
+
 static void consider_logical_flow(const struct lport_index *lports,
   const struct mcgroup_index *mcgroups,
   const struct sbrec_logical_flow *lflow,
   const struct hmap *local_datapaths,
   struct group_table *group_table,
   const struct simap *ct_zones,
+  const struct sbrec_chassis *chassis,
   struct hmap *dhcp_opts_p,
   struct hmap *dhcpv6_opts_p,
   uint32_t *conj_id_ofs_p,
@@ -99,6 +105,20 @@ lookup_port_cb(const void *aux_, const char *port_name, 
unsigned int *portp)
 }
 
 static bool
+is_chassis_resident_cb(const void *c_aux_, const char *port_name)
+{
+const struct condition_aux *c_aux = c_aux_;
+
+const struct sbrec_port_binding *pb
+= lport_lookup_by_name(c_aux->lports, port_name);
+if (pb && pb->chassis && pb->chassis == c_aux->chassis) {
+return true;
+}
+
+return false;
+}
+
+static bool
 is_switch(const struct sbrec_datapath_binding *ldp)
 {
 return smap_get(>external_ids, "logical-switch") != NULL;
@@ -112,6 +132,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct 
lport_index *lports,
   const struct hmap *local_datapaths,
   struct group_table *group_table,
   const struct simap *ct_zones,
+  const struct sbrec_chassis *chassis,
   struct 

[ovs-dev] [PATCH v6 6/7] ovn: avoid snat recirc only on gateway routers

[Mickey Spiegel]

Currently, for performance reasons on gateway routers, ct_snat
that does not specify an IP address does not immediately trigger
recirculation.  On gateway routers, ct_snat that does not specify
an IP address happens in the UNSNAT pipeline stage, which is
followed by the DNAT pipeline stage that triggers recirculation
for all packets.  This DNAT pipeline stage recirculation takes
care of the recirculation needs of UNSNAT as well as other cases
such as UNDNAT.

On distributed routers, UNDNAT is handled in the egress pipeline
stage, separately from DNAT in the ingress pipeline stages.  The
DNAT pipeline stage only triggers recirculation for some packets.
Due to this difference in design, UNSNAT needs to trigger its own
recirculation.

This patch restricts the logic that avoids recirculation for
ct_snat, so that it only applies to datapaths representing
gateway routers.

Signed-off-by: Mickey Spiegel 
---
 include/ovn/actions.h  |  3 +++
 ovn/controller/lflow.c | 10 ++
 ovn/lib/actions.c  | 15 +--
 tests/ovn.at   |  2 +-
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 0bf6145..0451c08 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -417,6 +417,9 @@ struct ovnact_encode_params {
 /* 'true' if the flow is for a switch. */
 bool is_switch;
 
+/* 'true' if the flow is for a gateway router. */
+bool is_gateway_router;
+
 /* A map from a port name to its connection tracking zone. */
 const struct simap *ct_zones;
 
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 3814b09..64c0591 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -125,6 +125,15 @@ is_switch(const struct sbrec_datapath_binding *ldp)
 
 }
 
+static bool
+is_gateway_router(const struct sbrec_datapath_binding *ldp,
+  const struct hmap *local_datapaths)
+{
+struct local_datapath *ld =
+get_local_datapath(local_datapaths, ldp->tunnel_key);
+return ld ? ld->has_local_l3gateway : false;
+}
+
 /* Adds the logical flows from the Logical_Flow table to flow tables. */
 static void
 add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
@@ -238,6 +247,7 @@ consider_logical_flow(const struct lport_index *lports,
 .lookup_port = lookup_port_cb,
 .aux = ,
 .is_switch = is_switch(ldp),
+.is_gateway_router = is_gateway_router(ldp, local_datapaths),
 .ct_zones = ct_zones,
 .group_table = group_table,
 
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 686ecc5..3da3dbe 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -788,12 +788,15 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
 ct = ofpacts->header;
 if (cn->ip) {
 ct->flags |= NX_CT_F_COMMIT;
-} else if (snat) {
-/* XXX: For performance reasons, we try to prevent additional
- * recirculations.  So far, ct_snat which is used in a gateway router
- * does not need a recirculation. ct_snat(IP) does need a
- * recirculation.  Should we consider a method to let the actions
- * specify whether an action needs recirculation if there more use
+} else if (snat && ep->is_gateway_router) {
+/* For performance reasons, we try to prevent additional
+ * recirculations.  ct_snat which is used in a gateway router
+ * does not need a recirculation.  ct_snat(IP) does need a
+ * recirculation.  ct_snat in a distributed router needs
+ * recirculation regardless of whether an IP address is
+ * specified.
+ * XXX Should we consider a method to let the actions specify
+ * whether an action needs recirculation if there are more use
  * cases?. */
 ct->recirc_table = NX_CT_RECIRC_NONE;
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index 428993d..ede0688 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -852,7 +852,7 @@ ct_dnat();
 
 # ct_snat
 ct_snat;
-encodes as ct(zone=NXM_NX_REG12[0..15],nat)
+encodes as ct(table=27,zone=NXM_NX_REG12[0..15],nat)
 has prereqs ip
 ct_snat(192.168.1.2);
 encodes as 
ct(commit,table=27,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2))
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] *** SPAM *** [PATCH v4 2/3] netdev-dpdk: Arbitrary 'dpdk' port naming

[Kevin Traynor]

On 01/05/2017 12:23 AM, Daniele Di Proietto wrote:
> 2017-01-04 7:31 GMT-08:00 Ciara Loftus :
>> > 'dpdk' ports no longer have naming restrictions. Now, instead of
>> > specifying the dpdk port ID as part of the name, the PCI address of the
>> > device must be specified via the 'dpdk-devargs' option. eg.
>> >
>> > ovs-vsctl add-port br0 my-port
>> > ovs-vsctl set Interface my-port type=dpdk
>> >   options:dpdk-devargs=:06:00.3
>> >
>> > The user must no longer hotplug attach DPDK ports by issuing the
>> > specific ovs-appctl netdev-dpdk/attach command. The hotplug is now
>> > automatically invoked when a valid PCI address is set in the
>> > dpdk-devargs. The format for ovs-appctl netdev-dpdk/detach command
>> > has changed in that the user now must specify the relevant PCI address
>> > as input instead of the port name.
>> >
>> > Signed-off-by: Ciara Loftus 
>> > Signed-off-by: Daniele Di Proietto 
>> > Co-authored-by: Daniele Di Proietto 
>> > Signed-off-by: Kevin Traynor 
>> > Co-authored-by: Kevin Traynor 
> Thanks for the new version
> 
> I've been testing this for a while and I couldn't find any problems.
> 
> The code looks good to me.
> 
> I know that's a big change but at this point I think it's better to
> merge it and refine (if we have to) it later.
> 
> Kevin, do you have any more comments?
> 

no more comments - it's a nice usability improvement and I agree it is a
good time to merge.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] how ovn-controller translates lflow into openflow?

[lg.yue]

hi, all:
is  it a big question? ^_^. i am viewing the source code, a little  
difficult. could you explain the logic behind?


thanks very  much.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Auditoría - 12 Temas Inéditos

[Organizar y Optimizar Costos]

 

12 conferencias pregrabadas / Durante 3 meses 

Póliza indispensable para los responsables de AUDITORíA Y CONTROL INTERNO
12 conferencias en cada póliza, pregrabadas, inéditas, 
para capacitar a todo su personal   
 
Ponemos a su alcance y por tiempo limitado, una exclusiva Póliza de 
Capacitación Online especializada en el área y las materias de AUDITORíA Y 
CONTROL INTERNO, con doce temas inéditos específicamente creados para 
actualizar, optimizar y mejorar la planeación de actividades y los 
procedimientos de las empresas, de forma completa, conveniente y accesible. 
TEMARIO:

1. Conceptos básicos de auditoría y control interno para no auditores.

2. Auditoría Administrativa. 

3. Auditoría Forense en la Investigación Criminal del lavado de dinero y 
activos. 

4. Cómo organizar y optimizar costos, presupuestos y utilidades (puntos de 
equilibrio).

5. Auditoría de Recursos Humanos.



- Y mucho más.


 
¿Requiere la información a la Brevedad?
responda este email con la palabra: 
Info - Auditoría.
centro telefónico: 018002129393


 
¿Demasiados mensajes en su cuenta? Responda este mensaje indicando que solo 
desea recibir CALENDARIO y sólo recibirá un correo al mes. Si desea cancelar la 
suscripción, solicite su BAJA..
 

 

 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] ovs-ofctl: Document the "clone" action.

[Ben Pfaff]

On Thu, Jan 05, 2017 at 06:33:53PM -0800, Mickey Spiegel wrote:
> On Thu, Jan 5, 2017 at 4:28 PM, Ben Pfaff  wrote:
> 
> > This was overlooked when "clone" was introduced.
> >
> > Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
> > Signed-off-by: Ben Pfaff 
> >
> 
> Acked-by: Mickey Spiegel 

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] ofctrl: Fix warning from sparse.

[Ben Pfaff]

On Thu, Jan 05, 2017 at 07:15:07PM -0800, Justin Pettit wrote:
> 
> > On Jan 5, 2017, at 5:03 PM, Ben Pfaff  wrote:
> > 
> > We've used sparse "bitwise" annotations to make ofp_ports into a different
> > type, so this is required to avoid a sparse warning.
> > 
> > CC: Justin Pettit 
> > Fixes: 714651c7db6a ("ovn-controller: Introduce "inject-pkt" ovs-appctl 
> > command.")
> > Signed-off-by: Ben Pfaff 
> 
> D'oh.  Forgot to run sparse again. I wish there were a configure flag
> to always enable it.

Wouldn't be hard to add that.  I always use the same script to build, so
I don't run into the problem normally.

> Thanks for catching the problem!
> 
> Acked-by: Justin Pettit 

Thanks, I'll apply this in a moment.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 5/5] New action "ct_clear".

[Ben Pfaff]

On Thu, Jan 05, 2017 at 07:19:21PM -0800, Mickey Spiegel wrote:
> On Thu, Jan 5, 2017 at 4:28 PM, Ben Pfaff  wrote:
> 
> > This is being introduced specifically to allow a user of the "clone" action
> > to clear the connection tracking state, but it's implemented as a separate
> > action as a matter of clean design and in case another use case arises
> > later.
> >
> > Reported-by: Mickey Spiegel 
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> > January/326981.html
> > Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
> > Signed-off-by: Ben Pfaff 
> > ---
> >  NEWS  |  2 +-
> >  include/openvswitch/ofp-actions.h |  3 +-
> >  lib/ofp-actions.c | 43 +++-
> >  manpages.mk   |  4 ---
> >
> 
> I have trouble applying due to changes in manpages.mk.
> I assume those should not be there?

Well, they need to happen, but they shouldn't be in this patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] python: Add TCP/SSL probes for OVSDB python lib

[Ben Pfaff]

On Fri, Jan 06, 2017 at 11:25:36AM +0800, Guoshuai Li wrote:
> stream_or_pstream_needs_probes always return 0. This causes TCP/SSL
> connection not be probed, and no reconnect when the connection
> is aborted
> 
> Signed-off-by: Guoshuai Li 

Thanks, applied to branch-2.6.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] nx-match: Only store significant bytes to stack.

[Ben Pfaff]

On Thu, Jan 05, 2017 at 05:45:29PM -0800, Jarno Rajahalme wrote:
> 
> > On Jan 5, 2017, at 4:48 PM, Ben Pfaff  wrote:
> > 
> > On Thu, Jan 05, 2017 at 04:03:17PM -0800, Jarno Rajahalme wrote:
> >> 
> >>> On Jan 4, 2017, at 11:03 PM, Ben Pfaff  wrote:
> >>> 
> >>> On Wed, Jan 04, 2017 at 07:21:44PM -0800, Jarno Rajahalme wrote:
>  
> > On Dec 21, 2016, at 2:36 PM, Ben Pfaff  wrote:
> > 
> > On Wed, Dec 07, 2016 at 04:49:00PM -0800, Jarno Rajahalme wrote:
> > I'd be more comfortable if nx_stack_pop() had assertions to check for
> > underflow.
>  
>  I don’t think OVS should assert fail if controller issues one pop
>  too many? Do you mean that current users of nx_stack_pop() do not
>  check for NULL return? I had a look and think that setting “*bytes”
>  to zero when returning NULL should be enough for all users.
> >>> 
> >>> It appears to me that if stack->size is greater than 0 but less than the
> >>> number of bytes indicated by its last byte, then it will corrupt the
> >>> ofpbuf size and that this will later cause some kind of failure that
> >>> will be harder to debug than an assertion failure. 
> >>> 
> >> 
> >> OK, now i got it.  This is just to guard against (future) bugs in OVS 
> >> itself.
> > 
> > Yes.
> > 
> > In ofputil_decode_packet_in_private(), it's probably worth checking the
> > format of the stack we pull from the payload, since a badly formatted
> > stack can segfault us (if we leave out assertions) or assert-fail us (if
> > we include them).
> > 
>  
>  What do you mean with badly formatted stack? Zero-sized property? IMO
>  even that would be properly pushed and popped from the stack, storing
>  only the length (of zero) in the stack.
> >>> 
> >>> I mean that if the property contains, for example, a single byte with
> >>> value 0xff, then it's badly formatted because we can pop off a length
> >>> (255) but then popping off that number of bytes will underflow.
> >> 
> >> I did not change the encoding of the stack as properties, so each
> >> value in the stack is still encoded as a separate property, where the
> >> (aligned) value length is used as the property length. 
> > 
> > I guess I forgot that.
> > 
> > Thanks, that's fine then.
> > 
> >> ofpprop_pull() does the length checking for the properties and the
> >> current code in ofputil_decode_packet_in_private() assert fails on any
> >> error, which is not good, as a controller bug would crash OVS?
> > 
> > That's bad.  Maybe the fix is as simple as this, though.
> > 
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > index 156d8d2..421b9d7 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 
> > Nicira, Inc.
> > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 
> > 2017 Nicira, Inc.
> >  *
> >  * Licensed under the Apache License, Version 2.0 (the "License");
> >  * you may not use this file except in compliance with the License.
> > @@ -4061,7 +4061,9 @@ ofputil_decode_packet_in_private(const struct 
> > ofp_header *oh, bool loose,
> > uint64_t type;
> > 
> > error = ofpprop_pull(, , );
> > -ovs_assert(!error);
> > +if (error) {
> > +break;
> > +}
> > 
> > switch (type) {
> > case NXCPT_BRIDGE:
> > @@ -4124,7 +4126,7 @@ ofputil_decode_packet_in_private(const struct 
> > ofp_header *oh, bool loose,
> > ofputil_packet_in_private_destroy(pin);
> > }
> > 
> > -return 0;
> > +return error;
> > }
> > 
> > /* Frees data in 'pin' that is dynamically allocated by
> > 
> 
> I folded this in to v3.

This bug is in 2.6, isn't it?  If so then we should fix it in a separate
patch, for backporting purposes.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5 2/3] netdev-dpdk: Arbitrary 'dpdk' port naming

[Daniele Di Proietto]

If new_port_id == -1 I think it's better to return failure from
set_config(), rather than return failure from a later reconfigure(),
so I squashed the following incremental:

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e66ff2711..038f79b80 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1231,12 +1231,13 @@ netdev_dpdk_set_config(struct netdev *netdev,
const struct smap *args)
  * is valid */
 if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
&& rte_eth_dev_is_valid_port(dev->port_id))) {
-err = EINVAL;
 int new_port_id = netdev_dpdk_process_devargs(new_devargs);
-if (new_port_id == dev->port_id) {
+if (!rte_eth_dev_is_valid_port(new_port_id)) {
+err = EINVAL;
+} else if (new_port_id == dev->port_id) {
 /* Already configured, do not reconfigure again */
 err = 0;
-} else if (rte_eth_dev_is_valid_port(new_port_id)) {
+} else {
 struct netdev_dpdk *dup_dev;
 dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
 if (dup_dev) {

and applied to master, thanks!

2017-01-05 2:42 GMT-08:00 Ciara Loftus :
> 'dpdk' ports no longer have naming restrictions. Now, instead of
> specifying the dpdk port ID as part of the name, the PCI address of the
> device must be specified via the 'dpdk-devargs' option. eg.
>
> ovs-vsctl add-port br0 my-port
> ovs-vsctl set Interface my-port type=dpdk
>   options:dpdk-devargs=:06:00.3
>
> The user must no longer hotplug attach DPDK ports by issuing the
> specific ovs-appctl netdev-dpdk/attach command. The hotplug is now
> automatically invoked when a valid PCI address is set in the
> dpdk-devargs. The format for ovs-appctl netdev-dpdk/detach command
> has changed in that the user now must specify the relevant PCI address
> as input instead of the port name.
>
> Signed-off-by: Ciara Loftus 
> Signed-off-by: Daniele Di Proietto 
> Co-authored-by: Daniele Di Proietto 
> Signed-off-by: Kevin Traynor 
> Co-authored-by: Kevin Traynor 
> ---
> Changelog:
> * Keep port-detach appctl function - use PCI as input arg
> * Add requires_mutex to devargs processing functions
> * use reconfigure infrastructure for devargs changes
> * process devargs even if valid portid ie. device already configured
> * report err if dpdk-devargs is not specified
> * Add Daniele's incremental patch & Sign-off + Co-author tags
> * Update details of detach command to reflect PCI is needed instead of
>   port name
> * Update NEWS to mention that the new naming scheme is not backwards
>   compatible
> * Use existing DPDk functions to get port ID from PCI address/devname
> * Merged process_devargs and process_pdevargs functions
> * Removed unnecessary requires_mutex from devargs processing fn
> * Fix case where port is re-attached after detach
> * Add note to documentation that devices won't work until devargs set.
> * Set netdev type and dpdk-devargs in one command in docs to avoid
>   errors.
> * Change port names in documentation to emphasise arbitrary-ness.
> * Add Kevin's incremental & sign-off/co-authored-by
> * Check if devargs string has changed before processing it as suggested
>   by Daniele.
> * Print error if attach fails
>
>  Documentation/howto/dpdk.rst |   7 +-
>  Documentation/intro/install/dpdk.rst |   9 +-
>  NEWS |   5 +
>  lib/netdev-dpdk.c| 181 
> ---
>  vswitchd/vswitch.xml |   8 ++
>  5 files changed, 148 insertions(+), 62 deletions(-)
>
> diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
> index 13da548..1ff672c 100644
> --- a/Documentation/howto/dpdk.rst
> +++ b/Documentation/howto/dpdk.rst
> @@ -312,14 +312,13 @@ In order to attach a port, it has to be bound to DPDK 
> using the
>
>  Then it can be attached to OVS::
>
> -$ ovs-appctl netdev-dpdk/attach :01:00.0
> -
> -At this point, the user can create a dpdk port using the ``add-port`` 
> command.
> +$ ovs-vsctl add-port br0 dpdkx -- set Interface dpdkx type=dpdk \
> +options:dpdk-devargs=:01:00.0
>
>  It is also possible to detach a port from ovs, the user has to remove the
>  port using the del-port command, then it can be detached using::
>
> -$ ovs-appctl netdev-dpdk/detach dpdk0
> +$ ovs-appctl netdev-dpdk/detach dpdkx
>
>  This feature is not supported with VFIO and does not work with some NICs.
>  For more information please refer to the `DPDK Port Hotplug Framework
> diff --git a/Documentation/intro/install/dpdk.rst 
> b/Documentation/intro/install/dpdk.rst
> index 10ceabb..fff0a1a 100644
> --- a/Documentation/intro/install/dpdk.rst
> +++ b/Documentation/intro/install/dpdk.rst
> @@ -252,8 +252,13 @@ 

Re: [ovs-dev] [PATCH v5 3/3] netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)

[Daniele Di Proietto]

2017-01-05 2:42 GMT-08:00 Ciara Loftus :
> Prior to this commit, the 'dpdk' port type could only be used for
> physical DPDK devices. Now, virtual devices (or 'vdevs') are supported.
> 'vdev' devices are those which use virtual DPDK Poll Mode Drivers eg.
> null, pcap. To add a DPDK vdev, a valid 'dpdk-devargs' must be set for
> the given dpdk port. The format expected is 'eth_' where
> 'x' is a number between 0 and RTE_MAX_ETHPORTS -1.
>
> For example to add a port that uses the 'null' DPDK PMD driver:
>
> ovs-vsctl set Interface null0 options:dpdk-devargs=eth_null0
>
> Not all DPDK vdevs have been verified to work at this point in time.
>
> Signed-off-by: Ciara Loftus 

Thanks for all your work on this!

Applied to master


> ---
> Changelog:
> * Updated process_vdevargs to work with Daniele's incremental in the
>   previous patch.
> * Allow vdev detach
> * Update docs to show af_packet example
> * Fix af_packet docs example
> * Fix style issues in docs
>
>  Documentation/howto/dpdk.rst | 29 +
>  NEWS |  1 +
>  lib/netdev-dpdk.c| 35 +++
>  vswitchd/vswitch.xml |  9 +++--
>  4 files changed, 48 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
> index 1ff672c..fbb4b53 100644
> --- a/Documentation/howto/dpdk.rst
> +++ b/Documentation/howto/dpdk.rst
> @@ -324,6 +324,35 @@ This feature is not supported with VFIO and does not 
> work with some NICs.
>  For more information please refer to the `DPDK Port Hotplug Framework
>  
> `__.
>
> +.. _vdev-support:
> +
> +Vdev Support
> +
> +
> +DPDK provides drivers for both physical and virtual devices. Physical DPDK
> +devices are added to OVS by specifying a valid PCI address in 'dpdk-devargs'.
> +Virtual DPDK devices which do not have PCI addresses can be added using a
> +different format for 'dpdk-devargs'.
> +
> +Typically, the format expected is 'eth_' where 'x' is a
> +number between 0 and RTE_MAX_ETHPORTS -1 (31).
> +
> +For example to add a dpdk port that uses the 'null' DPDK PMD driver::
> +
> +   $ ovs-vsctl add-port br0 null0 -- set Interface null0 type=dpdk \
> +   options:dpdk-devargs=eth_null0
> +
> +Similarly, to add a dpdk port that uses the 'af_packet' DPDK PMD driver::
> +
> +   $ ovs-vsctl add-port br0 myeth0 -- set Interface myeth0 type=dpdk \
> +   options:dpdk-devargs=eth_af_packet0,iface=eth0
> +
> +More information on the different types of virtual DPDK PMDs can be found in
> +the `DPDK documentation
> +`__.
> +
> +Note: Not all DPDK virtual PMD drivers have been tested and verified to work.
> +
>  .. _dpdk-ovs-in-guest:
>
>  OVS with DPDK Inside VMs
> diff --git a/NEWS b/NEWS
> index d66d402..cc319a9 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -53,6 +53,7 @@ Post-v2.6.0
> with the old dpdk naming scheme is broken, and as such a
> device will not be available for use until a valid dpdk-devargs is
> specified.
> + * Virtual DPDK Poll Mode Driver (vdev PMD) support.
> - Fedora packaging:
>   * A package upgrade does not automatically restart OVS service.
> - ovs-vswitchd/ovs-vsctl:
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e66ff27..79eddb5 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1134,25 +1134,19 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>  static int
>  netdev_dpdk_process_devargs(const char *devargs)
>  {
> -struct rte_pci_addr addr;
>  uint8_t new_port_id = UINT8_MAX;
>
> -if (!eal_parse_pci_DomBDF(devargs, )) {
> -/* Valid PCI address format detected - configure physical device */
> -if (!rte_eth_dev_count()
> -|| rte_eth_dev_get_port_by_name(devargs, _port_id)
> -|| !rte_eth_dev_is_valid_port(new_port_id)) {
> -/* PCI device not found in DPDK, attempt to attach it */
> -if (!rte_eth_dev_attach(devargs, _port_id)) {
> -/* Attach successful */
> -VLOG_INFO("Device "PCI_PRI_FMT" has been attached to DPDK",
> -  addr.domain, addr.bus, addr.devid, addr.function);
> -} else {
> -/* Attach unsuccessful */
> -VLOG_INFO("Error attaching device "PCI_PRI_FMT" to DPDK",
> -  addr.domain, addr.bus, addr.devid, addr.function);
> -return -1;
> -}
> +if (!rte_eth_dev_count()
> +|| rte_eth_dev_get_port_by_name(devargs, _port_id)
> +|| !rte_eth_dev_is_valid_port(new_port_id)) {
> +/* Device not found in DPDK, attempt to attach it */
> +if (!rte_eth_dev_attach(devargs, _port_id)) {
> +/* Attach successful */
> +

Re: [ovs-dev] [PATCHv2] python: Fix nroff indentation for after .

[Ben Pfaff]

On Thu, Jan 05, 2017 at 06:09:35PM -0800, Joe Stringer wrote:
> When XML is used for writing manpages, in the case that there is a 
> tag followed by , the nroff python utility indents the  tag (and
> children) an extra level which is unnecessary and makes the formatting
> inconsistent between manpages written directly in nroff vs manpages
> written in XML and converted to nroff. Fix the indentation by removing
> the extraneous .RS / .RE tags added to generated nroff in this case.
> 
> This fixes the formatting of ovn/utilities/ovn-nbctl.8 man page.
> 
> Signed-off-by: Joe Stringer 

nroff sucks so much.

I suspect that this should actually apply to all h*, not just to h1.
None of the headings imply indentation.

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 1/4] ofproto-dpif-xlate: Make "clone" save action set and stack.

[Ben Pfaff]

This is a design decision but it seems conceptually cleaner than having
them leak through into the clone.

Reported-by: Mickey Spiegel 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/326981.html
Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
Signed-off-by: Ben Pfaff 
Acked-by: Mickey Spiegel 
---
 ofproto/ofproto-dpif-xlate.c | 18 +-
 utilities/ovs-ofctl.8.in |  3 ++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index fb552ce..36b7ac0 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, 
Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -4319,8 +4319,24 @@ compose_clone_action(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 bool old_conntracked = ctx->conntracked;
 struct flow old_flow = ctx->xin->flow;
 
+struct ofpbuf old_stack = ctx->stack;
+union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
+ofpbuf_use_stub(>stack, new_stack, sizeof new_stack);
+ofpbuf_put(>stack, old_stack.data, old_stack.size);
+
+struct ofpbuf old_action_set = ctx->action_set;
+uint64_t actset_stub[1024 / 8];
+ofpbuf_use_stub(>action_set, actset_stub, sizeof actset_stub);
+ofpbuf_put(>action_set, old_action_set.data, old_action_set.size);
+
 do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
 
+ofpbuf_uninit(>action_set);
+ctx->action_set = old_action_set;
+
+ofpbuf_uninit(>stack);
+ctx->stack = old_stack;
+
 ctx->xin->flow = old_flow;
 
 /* The clone's conntrack execution should have no effect on the original
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 49d87ab..0fdaa48 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -2638,7 +2638,8 @@ in Open vSwitch 2.4.
 Executes each nested \fIaction\fR, saving much of the packet and
 pipeline state beforehand and then restoring it afterward.  The state
 that is saved and restored includes all flow data and metadata
-(including, for example, \fBct_state\fR).
+(including, for example, \fBct_state\fR), the stack accessed by
+\fBpush\fR and \fBpop\fR actions, and the OpenFlow action set.
 .IP
 This action was added in Open vSwitch 2.6.90.
 .RE
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/5] ofproto-dpif-xlate: Make clone save whether conntracking has occurred.

[Ben Pfaff]

On Thu, Jan 05, 2017 at 06:40:09PM -0800, Mickey Spiegel wrote:
> On Thu, Jan 5, 2017 at 4:28 PM, Ben Pfaff  wrote:
> 
> > The rest of the conntrack state (in the flow) is saved and restored, so
> > this should be also.
> >
> > Reported-by: Mickey Spiegel 
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> > January/326981.html
> > Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
> > Signed-off-by: Ben Pfaff 
> >
> 
> Acked-by: Mickey Spiegel 

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] xml2nroff: Port to python3.

[Ben Pfaff]

On Thu, Jan 05, 2017 at 06:01:08PM -0800, Joe Stringer wrote:
> Signed-off-by: Joe Stringer 

Works for me.

Acked-by: Ben Pfaff 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Issues with the use of the clone action for resubmission to the pipeline

[Ben Pfaff]

On Thu, Jan 05, 2017 at 05:54:46PM -0800, Jarno Rajahalme wrote:
> 
> > On Jan 5, 2017, at 4:28 PM, Ben Pfaff  wrote:
> > 
> > On Tue, Jan 03, 2017 at 02:55:19AM -0800, Mickey Spiegel wrote:
> >> One of the motivations for clone is to use it as a lightweight way to
> >> resubmit to an earlier table at the beginning of the pipeline, without
> >> incurring all of the overhead associated with openflow patch ports.
> >> One such usage is in OVN, where a recent patch set replaced the
> >> use of openflow patch ports with clone, for OVN patch ports within
> >> the same bridge (br-int).
> >> 
> >> Over the holidays, some issues arose related to this usage of clone
> >> (see the thread ovn ping from VM to external gateway IP failed).
> > 
> > Thanks for bringing this up.  I had overlooked these questions and this
> > issue.
> > 
> > I guess that we should save and restore this since we're saving and
> > restoring the conntrack metadata.  I've written up a patch.
> > 
> >>   ctx->was_mpls
> > 
> > I do not think that that this is a correctness issue, but it's a nice
> > optimization to save and restore this.  I've written up a patch.
> 
> I think the rationale is that ‘was_mpls’ is used to track the validity
> of the flow key across an MPLS POP operation. Since the flow key is
> saved and restored, we should save and restore ‘was_mpls’ as well.

OK.  I did send a patch that adds that.  Do you want to make a
suggestion for the commit message?  I may not fully understand the issue
yet, so I don't think the commit message is very good.

Here's the patch:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327207.html

> >>   ctx->xin->tables_version (not an issue if bridge does not change)
> > 
> > clone doesn't change the bridge, so this shouldn't matter.
> > 
> >>   ctx->stack
> >>   ctx->action_set
> > 
> > I think it's cleanest if a clone starts off with both of these empty and
> > saves and restores them.  I've written up a patch.
> 
> I think saving and restoring is needed, but I’m not so sure of
> clearing these. However, it seems that there is no way for the action
> set to be executed within the clone, so I guess it does not matter. 

I guess that it would also be a clean design, and consistent with my new
ct_clear action, to not clear them but instead to start from a copy and
allow for clearing them explicitly within the clone.

There is already an instruction to clear the action set, so we wouldn't
need to add anything.  I think that the action set can only affect what
happens inside the clone in terms of matches or actions based on the
actset_output field, though.

I'm not sure of the value of an action to clear the stack, so I'd be
inclined to hold off on that until we think of one.

I'll revise my patch to work this way.

> It would be good to add these changes to the documentation as well.

My patch does update the documentation on this point.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] *** SPAM *** [PATCH 1/2] ofctrl: Fix version check in ofctrl_inject_packet().

[Ben Pfaff]

On Thu, Jan 05, 2017 at 07:13:53PM -0800, Justin Pettit wrote:
> 
> > On Jan 5, 2017, at 5:03 PM, Ben Pfaff  wrote:
> > 
> > "enum ofp_version" is unsigned in the System V ABI used by Linux, so
> > it will never be less than 0, so an rconn with an unnegotiated version will
> > never be found properly.  This fixes the problem.
> > 
> > CC: Justin Pettit 
> > Fixes: 714651c7db6a ("ovn-controller: Introduce "inject-pkt" ovs-appctl 
> > command.")
> > Signed-off-by: Ben Pfaff 
> 
> Thanks for catching that.
> 
> Acked-by: Justin Pettit 

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 3/4] New action "ct_clear".

[Ben Pfaff]

This is being introduced specifically to allow a user of the "clone" action
to clear the connection tracking state, but it's implemented as a separate
action as a matter of clean design and in case another use case arises
later.

Reported-by: Mickey Spiegel 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/326981.html
Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
Signed-off-by: Ben Pfaff 
Acked-by: Mickey Spiegel 
---
 NEWS  |  2 +-
 include/openvswitch/ofp-actions.h |  3 +-
 lib/ofp-actions.c | 43 +++-
 ofproto/ofproto-dpif-xlate.c  | 17 ---
 tests/ofp-actions.at  |  3 ++
 tests/ofproto-dpif.at | 60 +++
 utilities/ovs-ofctl.8.in  |  6 
 7 files changed, 127 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS
index 48ca679..0a9551c 100644
--- a/NEWS
+++ b/NEWS
@@ -35,7 +35,7 @@ Post-v2.6.0
details.
  * The "sample" action now supports "ingress" and "egress" options.
  * The "ct" action now supports the TFTP ALG where support is available.
- * New action "clone".
+ * New actions "clone" and "ct_clear".
- ovs-ofctl:
  * 'bundle' command now supports packet-out messages.
  * New syntax for 'ovs-ofctl packet-out' command, which uses the
diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index df9025c..8ca787a 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -107,6 +107,7 @@
 OFPACT(SAMPLE,  ofpact_sample,  ofpact, "sample")   \
 OFPACT(UNROLL_XLATE,ofpact_unroll_xlate, ofpact, "unroll_xlate") \
 OFPACT(CT,  ofpact_conntrack,   ofpact, "ct")   \
+OFPACT(CT_CLEAR,ofpact_null,ofpact, "ct_clear") \
 OFPACT(NAT, ofpact_nat, ofpact, "nat")  \
 OFPACT(OUTPUT_TRUNC,ofpact_output_trunc,ofpact, "output_trunc") \
 OFPACT(CLONE,   ofpact_nest,actions, "clone")   \
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index f29673f..4736521 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008-2016 Nicira, Inc.
+ * Copyright (c) 2008-2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -328,6 +328,9 @@ enum ofp_raw_action_type {
 /* NX1.0+(42): struct ext_action_header, ... */
 NXAST_RAW_CLONE,
 
+/* NX1.0+(43): void. */
+NXAST_RAW_CT_CLEAR,
+
 /* ## -- ## */
 /* ## Debugging actions. ## */
 /* ## -- ## */
@@ -449,6 +452,7 @@ ofpact_next_flattened(const struct ofpact *ofpact)
 case OFPACT_EXIT:
 case OFPACT_SAMPLE:
 case OFPACT_UNROLL_XLATE:
+case OFPACT_CT_CLEAR:
 case OFPACT_DEBUG_RECIRC:
 case OFPACT_METER:
 case OFPACT_CLEAR_ACTIONS:
@@ -5495,6 +5499,36 @@ format_CT(const struct ofpact_conntrack *a, struct ds *s)
 ds_put_format(s, "%s)%s", colors.paren, colors.end);
 }
 
+/* ct_clear action. */
+
+static enum ofperr
+decode_NXAST_RAW_CT_CLEAR(struct ofpbuf *out)
+{
+ofpact_put_CT_CLEAR(out);
+return 0;
+}
+
+static void
+encode_CT_CLEAR(const struct ofpact_null *null OVS_UNUSED,
+enum ofp_version ofp_version OVS_UNUSED,
+struct ofpbuf *out)
+{
+put_NXAST_CT_CLEAR(out);
+}
+
+static char * OVS_WARN_UNUSED_RESULT
+parse_CT_CLEAR(char *arg OVS_UNUSED, struct ofpbuf *ofpacts,
+   enum ofputil_protocol *usable_protocols OVS_UNUSED)
+{
+ofpact_put_CT_CLEAR(ofpacts);
+return NULL;
+}
+
+static void
+format_CT_CLEAR(const struct ofpact_null *a OVS_UNUSED, struct ds *s)
+{
+ds_put_format(s, "%sct_clear%s", colors.value, colors.end);
+}
 /* NAT action. */
 
 /* Which optional fields are present? */
@@ -6280,6 +6314,7 @@ ofpact_is_set_or_move_action(const struct ofpact *a)
 case OFPACT_BUNDLE:
 case OFPACT_CLEAR_ACTIONS:
 case OFPACT_CT:
+case OFPACT_CT_CLEAR:
 case OFPACT_CLONE:
 case OFPACT_NAT:
 case OFPACT_CONTROLLER:
@@ -6360,6 +6395,7 @@ ofpact_is_allowed_in_actions_set(const struct ofpact *a)
 case OFPACT_CLONE:
 case OFPACT_CONTROLLER:
 case OFPACT_CT:
+case OFPACT_CT_CLEAR:
 case OFPACT_NAT:
 case OFPACT_ENQUEUE:
 case OFPACT_EXIT:
@@ -6594,6 +6630,7 @@ ovs_instruction_type_from_ofpact_type(enum ofpact_type 
type)
 case OFPACT_SAMPLE:
 case OFPACT_DEBUG_RECIRC:
 case OFPACT_CT:
+case 

[ovs-dev] [PATCH v2 2/4] ofproto-dpif-xlate: Make clone save "was_mpls".

[Ben Pfaff]

This seems like it's an optimization rather than a correctness issue, but
in general it's best to make "clone" like patch ports where there is no
reason to depart from its design, since we know that patch ports work well.

Reported-by: Mickey Spiegel 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/326981.html
Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
Signed-off-by: Ben Pfaff 
Acked-by: Mickey Spiegel 
---
 ofproto/ofproto-dpif-xlate.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 36b7ac0..ceeaac6 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4316,6 +4316,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
 static void
 compose_clone_action(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
 {
+bool old_was_mpls = ctx->was_mpls;
 bool old_conntracked = ctx->conntracked;
 struct flow old_flow = ctx->xin->flow;
 
@@ -4342,6 +4343,10 @@ compose_clone_action(struct xlate_ctx *ctx, const struct 
ofpact_nest *oc)
 /* The clone's conntrack execution should have no effect on the original
  * packet. */
 ctx->conntracked = old_conntracked;
+
+/* Popping MPLS from the clone should have no effect on the original
+ * packet. */
+ctx->was_mpls = old_was_mpls;
 }
 
 static bool
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 4/4] ovn-controller: Clear conntrack state inside clone action.

[Ben Pfaff]

ovn-controller implements traversal from one OVN logical network to another
using the Open vSwitch "clone" action.  The "clone" action preserves
connection tracking state, which is confusing for passing from one logical
datapath to another because this state is only relevant for a single
logical datapath and does not make sense in the new one.  This commit
fixes a problem sometimes seen by ensuring that the connection tracking
state is cleared when these traversals happen.

Reported-by: Numan Siddique 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/326948.html
Fixes: f1a8bd06d58f ("ovn-controller: Drop most uses of OVS patch ports.")
---
 ovn/controller/physical.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 9cc7eb6..1973984 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2015, 2016 Nicira, Inc.
+/* Copyright (c) 2015, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -331,6 +331,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
 
 size_t clone_ofs = ofpacts_p->size;
 struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts_p);
+ofpact_put_CT_CLEAR(ofpacts_p);
 put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
 put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
 put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovs V2 12/21] dpif-netlink: Use netdev flow put api to insert a flow

[Joe Stringer]

On 25 December 2016 at 03:39, Paul Blakey  wrote:
> Using the new netdev flow api operate will now try and
> offload flows to the relevant netdev of the input port.
> Other operate methods flows will come in later patches.
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> ---
>  lib/dpif-netlink.c | 232 
> -
>  1 file changed, 228 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 3d8940e..717af90 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1908,15 +1908,239 @@ dpif_netlink_operate__(struct dpif_netlink *dpif,
>  return n_ops;
>  }
>
> +static int
> +parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len,
> +const struct nlattr *mask, size_t mask_len,
> +struct match *match)
> +{
> +enum odp_key_fitness fitness;
> +
> +fitness = odp_flow_key_to_flow(key, key_len, >flow);
> +if (fitness) {
> +/* This should not happen: it indicates that odp_flow_key_from_flow()
> + * and odp_flow_key_to_flow() disagree on the acceptable form of a
> + * flow.  Log the problem as an error, with enough details to enable
> + * debugging. */
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +
> +if (!VLOG_DROP_ERR()) {
> +struct ds s;
> +
> +ds_init();
> +odp_flow_format(key, key_len, NULL, 0, NULL, , true);
> +VLOG_ERR("internal error parsing flow key %s", ds_cstr());
> +ds_destroy();
> +}
> +
> +return EINVAL;
> +}
> +
> +fitness = odp_flow_key_to_mask(mask, mask_len, >wc, >flow);
> +if (fitness) {
> +/* This should not happen: it indicates that
> + * odp_flow_key_from_mask() and odp_flow_key_to_mask()
> + * disagree on the acceptable form of a mask.  Log the problem
> + * as an error, with enough details to enable debugging. */
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +
> +if (!VLOG_DROP_ERR()) {
> +struct ds s;
> +
> +VLOG_ERR("internal error parsing flow mask %s (%s)",
> + ds_cstr(), odp_key_fitness_to_string(fitness));
> +ds_destroy();
> +}
> +
> +return EINVAL;
> +}
> +
> +return 0;
> +}
> +
> +static bool
> +parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
> +{
> +struct match match;
> +odp_port_t in_port;
> +const struct nlattr *nla;
> +size_t left;
> +int outputs = 0;
> +struct ofpbuf buf;
> +uint64_t act_stub[1024 / 8];
> +size_t offset;
> +struct nlattr *act;
> +struct netdev *dev;
> +int err;
> +
> +/* 0x1234 - fake eth type sent to probe feature */
> +if (put->flags & DPIF_FP_PROBE || match.flow.dl_type == htons(0x1234)) {
> +return false;
> +}
> +
> +if (parse_key_and_mask_to_match(put->key, put->key_len, put->mask,
> +put->mask_len, )) {
> +return false;
> +}
> +
> +in_port = match.flow.in_port.odp_port;
> +ofpbuf_use_stub(, act_stub, sizeof act_stub);
> +offset = nl_msg_start_nested(, OVS_FLOW_ATTR_ACTIONS);
> +NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
> +if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
> +struct netdev *outdev;
> +int ifindex_out;
> +const struct netdev_tunnel_config *tnl_cfg;
> +size_t out_off;
> +odp_port_t out_port;
> +
> +outputs++;
> +if (outputs > 1) {
> +break;
> +}
> +
> +out_port = nl_attr_get_u32(nla);
> +outdev = netdev_hmap_port_get(out_port, dpif->dpif.dpif_class);
> +tnl_cfg = netdev_get_tunnel_config(outdev);
> +
> +out_off = nl_msg_start_nested(, OVS_ACTION_ATTR_OUTPUT);
> +ifindex_out = netdev_get_ifindex(outdev);
> +nl_msg_put_u32(, OVS_ACTION_ATTR_OUTPUT, ifindex_out);
> +if (tnl_cfg && tnl_cfg->dst_port != 0) {
> +nl_msg_put_u32(, OVS_TUNNEL_KEY_ATTR_TP_DST, 
> tnl_cfg->dst_port);
> +}
> +nl_msg_end_nested(, out_off);
> +
> +if (outdev) {
> +netdev_close(outdev);
> +}
> +} else {
> +nl_msg_put_unspec(, nl_attr_type(nla), nl_attr_get(nla),
> +  nl_attr_get_size(nla));
> +}
> +}
> +nl_msg_end_nested(, offset);
> +
> +if (outputs > 1) {
> +return false;
> +}
> +
> +act = ofpbuf_at_assert(, offset, sizeof(struct nlattr));
> +dev = netdev_hmap_port_get(in_port, dpif->dpif.dpif_class);
> +err = netdev_flow_put(dev, , CONST_CAST(struct nlattr *,
> +  

[ovs-dev] [PATCH] python: Fix nroff indentation for XML .

[Joe Stringer]

When XML is used for writing manpages, the nroff python utility indents
the  tags an extra level which is unnecessary and makes the
formatting inconsistent between manpages written directly in nroff vs
manpages written in XML and converted to nroff. Fix the indentation by
removing the extraneous .RS / .RE tags added to generated nroff.

Signed-off-by: Joe Stringer 
---
 python/build/nroff.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/python/build/nroff.py b/python/build/nroff.py
index aed60ebbd592..cdeef6fb1904 100644
--- a/python/build/nroff.py
+++ b/python/build/nroff.py
@@ -250,7 +250,6 @@ def block_xml_to_nroff(nodes, para='.PP'):
 elif node.tagName == 'dl':
 if s != "":
 s += "\n"
-s += ".RS\n"
 prev = "dd"
 for li_node in node.childNodes:
 if (li_node.nodeType == node.ELEMENT_NODE
@@ -272,7 +271,6 @@ def block_xml_to_nroff(nodes, para='.PP'):
 raise error.Error(" element may only have "
   " and  children")
 s += block_xml_to_nroff(li_node.childNodes, ".IP")
-s += ".RE\n"
 elif node.tagName == 'p':
 if s != "":
 if not s.endswith("\n"):
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] nx-match: Only store significant bytes to stack.

[Jarno Rajahalme]

> On Jan 4, 2017, at 11:03 PM, Ben Pfaff  wrote:
> 
> On Wed, Jan 04, 2017 at 07:21:44PM -0800, Jarno Rajahalme wrote:
>> 
>>> On Dec 21, 2016, at 2:36 PM, Ben Pfaff  wrote:
>>> 
>>> On Wed, Dec 07, 2016 at 04:49:00PM -0800, Jarno Rajahalme wrote:
 Always storing the maximum mf_value size wastes about 120 bytes for
 each stack entry.  This patch changes the stack from an mf_value array
 to a string of value-length pairs.
 
 The lenght is stored after the value so that the stack pop may first
>>> 
>>> s/lenght/length/
>>> 
>> 
>> Fixed, thanks!
>> 
 read the length and then the appropriate number of bytes.  Iterating
 the stack in the reverse order is not possible, so some code needs to
 reverse the stack first to then traverse the stack.  This could be
 avoided by storing the length of the value both before and after it.
 
 Signed-off-by: Jarno Rajahalme 
>>> 
>>> I don't think it's worth reversing the stack to serialize for
>>> continuations.  This is part of the "private" part of the continuation
>>> that users are not supposed to look at, so we can put it in any order we
>>> like.  Also, for the sake of printing it for debugging purposes, we can
>>> also print it in any order we like; starting from top-of-stack is not a
>>> problem, although it might be nice to put (top) and (bottom) indicators
>>> in the output.
>> 
>> You mean literally “(top)” in the beginning and “(bottom)” at the end,
>> right? 
> 
> That's what I was thinking, yes.
> 
>> It seems we missed a newline as well..
> 
> I didn't test it, so I could easily have missed that.
> 
>>> I'd be more comfortable if nx_stack_pop() had assertions to check for
>>> underflow.
>>> 
>> 
>> I don’t think OVS should assert fail if controller issues one pop too many? 
>> Do you mean that current users of nx_stack_pop() do not check for NULL 
>> return? I had a look and think that setting “*bytes” to zero when returning 
>> NULL should be enough for all users.
> 
> It appears to me that if stack->size is greater than 0 but less than the
> number of bytes indicated by its last byte, then it will corrupt the
> ofpbuf size and that this will later cause some kind of failure that
> will be harder to debug than an assertion failure. 
> 

OK, now i got it.  This is just to guard against (future) bugs in OVS itself.

>>> In ofputil_decode_packet_in_private(), it's probably worth checking the
>>> format of the stack we pull from the payload, since a badly formatted
>>> stack can segfault us (if we leave out assertions) or assert-fail us (if
>>> we include them).
>>> 
>> 
>> What do you mean with badly formatted stack? Zero-sized property? IMO
>> even that would be properly pushed and popped from the stack, storing
>> only the length (of zero) in the stack.
> 
> I mean that if the property contains, for example, a single byte with
> value 0xff, then it's badly formatted because we can pop off a length
> (255) but then popping off that number of bytes will underflow.

I did not change the encoding of the stack as properties, so each value in the 
stack is still encoded as a separate property, where the (aligned) value length 
is used as the property length. This causes the decoded stack values to be 
aligned to 8 bytes, which should not make any functional difference, as the 
stack is still logically an array of full-size mf_subvalues, only the internal 
storage format has been changed. Do you think this is a problem? If so, we 
could also align the internal representation to 8 bytes?

ofpprop_pull() does the length checking for the properties and the current code 
in ofputil_decode_packet_in_private() assert fails on any error, which is not 
good, as a controller bug would crash OVS?

 Jarno



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] ovs-vswitch kernel panic randomly started after 400+ days uptime

[Joe Stringer]

On 5 January 2017 at 17:13, Uri Foox  wrote:
> Hi,
>
> Since about 10 days ago, every few hours, one of our 10 compute nodes on
> our Openstack cluster kernel panics at the host level kernel panics
> (captured through netconsole). The kernel panic is identical across all 10
> nodes and happens at random times but at least 1 node kernel panics every
> 3-12 hours. We have tried numerous things including upgrading the kernel
> (Ubuntu 12.04 LTS running 3.1.0-106-generic), modifying sysctl, restarting
> switches, restarting all openstack networking services, changing BIOS
> settings etc...but no luck. We have not restarted the control nodes or the
> Juniper switch that routes all inbound internet traffic.
>
> Based on research we did around skbuff.h we found two kernel patches to
> address a checksum failure and also some OVS discussions about it. I was
> hoping that the kernel upgrade would solve it but it did not. I do not know
> if Openstack will tolerate us upgrading OVS and the fact that it started
> completely randomly leads me to believe it's some other factor that we are
> unaware of.
>
>
>- https://patchwork.ozlabs.org/patch/512625/
>-
>
> https://github.com/openvswitch/ovs/commit/51b7a90217369f6bbbf164ba471f54ec2817665e
>- https://patchwork.kernel.org/patch/7475491/
>- https://patchwork.ozlabs.org/patch/523632/
>
>
> Here is one of them. If you have any ideas what we can do, please let me
> know.
>
> Thanks,
> Uri
>
>
> Connection from 172.25.2.157 port 5404 [udp/*] accepted
> [68240.441681] [ cut here ]
> [68240.496918] kernel BUG at
> /build/linux-lts-trusty-D60X6T/linux-lts-trusty-3.13.0/include/linux/skbuff.h:1486!
> [68240.615520] invalid opcode:  [#1] SMP
> [68240.664751] Modules linked in: netconsole configfs xt_mac xt_physdev
> xt_set ip_set_hash_ip ip_set nfnetlink vhost_net macvtap macvlan vhost veth
> bridge stp llc ipt_REJECT xt_state xt_conntrack xt_multiport xt_CT
> xt_comment iptable_raw xt_CHECKSUM xt_tcpudp iptable_mangle ipt_MASQUERADE
> iptable_nat nf_nat_ipv4 nf_nat ip6table_filter ip6_tables iptable_filter
> ip_tables ebtable_nat ebtables x_tables kvm_intel kvm nbd ib_iser rdma_cm
> ib_cm iw_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp libiscsi_tcp libiscsi
> scsi_transport_iscsi openvswitch vxlan ip_tunnel gre nfsd nfs_acl
> auth_rpcgss nfs fscache lockd sunrpc dm_multipath gpio_ich dcdbas scsi_dh
> mei_me shpchp sb_edac mei edac_core lpc_ich joydev acpi_power_meter
> nf_conntrack_ipv6 mac_hid nf_defrag_ipv6 wmi nf_conntrack_ipv4 ipmi_si xfs
> nf_conntrack nf_defrag_ipv4 lp parport igb btrfs hid_generic dca
> i2c_algo_bit usbhid raid6_pq ptp ahci bnx2x hid libahci mdio megaraid_sas
> pps_core xor libcrc32c
> [68241.670838] CPU: 33 PID: 0 Comm: swapper/33 Not tainted
> 3.13.0-106-generic #153~precise1-Ubuntu
> [68241.774871] Hardware name: Dell Inc. PowerEdge R820/066N7P, BIOS 2.2.3
> 07/09/2014
> [68241.864406] task: 881028b94800 ti: 881028ba task.ti:
> 881028ba
> [68241.953939] RIP: 0010:[]  []
> __skb_pull.part.7+0x4/0x6 [openvswitch]
> [68242.067531] RSP: 0018:88203fb03b08  EFLAGS: 00010297
> [68242.131087] RAX: 88165c791966 RBX: 88202639e900 RCX:
> 88165c791900
> [68242.216458] RDX: 0210 RSI: 001a RDI:
> 0214
> [68242.301842] RBP: 88203fb03b08 R08:  R09:
> 0140
> [68242.387207] R10: 000c R11: 72221c0c R12:
> 88203fb03b70
> [68242.472576] R13: 88402794d0c0 R14: 88203fb03b70 R15:
> 88302324e180
> [68242.557945] FS:  () GS:88203fb0()
> knlGS:
> [68242.654780] CS:  0010 DS:  ES:  CR0: 80050033
> [68242.723550] CR2: 7f9c7466ab90 CR3: 00302689e000 CR4:
> 000427e0
> [68242.808931] Stack:
> [68242.832981]  88203fb03b38 a0524e64 8112d1e1
> 88202639e900
> [68242.921980]  e8e000305800 88402794d0c0 88203fb03c28
> a0523a80
> [68243.010963]  88203fb13180 88203fb03b90 810a090b
> 0001
> [68243.099945] Call Trace:
> [68243.129188]  
> [68243.152204]  [] ovs_flow_extract+0x664/0x720
> [openvswitch]
> [68243.238893]  [] ? tracing_record_cmdline+0x21/0x50
> [68243.314912]  []
> ovs_dp_process_received_packet+0x60/0x130 [openvswitch]
> [68243.412793]  [] ? ttwu_do_wakeup+0xfb/0x110
> [68243.481559]  [] ovs_vport_receive+0x2a/0x30
> [openvswitch]
> [68243.564884]  [] gre_rcv+0xa4/0xb8 [openvswitch]
> [68243.637802]  [] gre_cisco_rcv+0x75/0xbc [gre]
> [68243.708621]  [] gre_rcv+0x65/0x90 [gre]
> [68243.773214]  [] ip_local_deliver_finish+0xa8/0x220
> [68243.849244]  [] ip_local_deliver+0x4b/0x90
> [68243.916951]  [] ip_rcv_finish+0x121/0x380
> [68243.983627]  [] ip_rcv+0x286/0x380
> [68244.043023]  [] __netif_receive_skb_core+0x61a/0x760
> [68244.121122]  [] __netif_receive_skb+0x21/0x70
> [68244.191942]  [] process_backlog+0xb1/0x190
> [68244.259642] 

Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif: Unhide structure contents.

[Justin Pettit]

> On Dec 22, 2016, at 9:58 AM, Ben Pfaff  wrote:
> 
> Until now, ofproto-dpif.c has hidden the definitions of several structures,
> such as struct ofproto_dpif and struct rule_dpif.  This kind of information
> hiding is often beneficial, because it forces code outside the file with
> the definition to use the documented interfaces.  However, in this case it
> was starting to burden ofproto-dpif with an increasing number of trivial
> helpers that were not improving or maintaining a useful abstraction and
> that were making code harder to maintain and read.
> 
> Information hiding also made it hard to move blocks of code outside
> ofproto-dpif.c itself, since any code moved out often needed new helpers if
> it used anything that wasn't previously exposed.  In the present instance,
> upcoming patches will move code for tracing outside ofproto-dpif, and this
> would require adding several helpers that would just obscure the function
> of the code otherwise needlessly.
> 
> In balance, it seems that there is more harm than good in the information
> hiding here, so this commit moves the definitions of several structures
> from ofproto-dpif.c into ofproto-dpif.h.  It also removes all of the
> trivial helpers that had accumulated, instead changing their users to
> directly access the members that they needed.  It also reorganizes
> ofproto-dpif.h, grouping structure definitions and function prototypes in a
> sensible way.
> 
> Signed-off-by: Ben Pfaff 
> Acked-by: Lance Richardson 

Acked-by: Justin Pettit 

--Justin


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] nx-match: Only store significant bytes to stack.

[Ben Pfaff]

On Thu, Jan 05, 2017 at 04:03:17PM -0800, Jarno Rajahalme wrote:
> 
> > On Jan 4, 2017, at 11:03 PM, Ben Pfaff  wrote:
> > 
> > On Wed, Jan 04, 2017 at 07:21:44PM -0800, Jarno Rajahalme wrote:
> >> 
> >>> On Dec 21, 2016, at 2:36 PM, Ben Pfaff  wrote:
> >>> 
> >>> On Wed, Dec 07, 2016 at 04:49:00PM -0800, Jarno Rajahalme wrote:
> >>> I'd be more comfortable if nx_stack_pop() had assertions to check for
> >>> underflow.
> >> 
> >> I don’t think OVS should assert fail if controller issues one pop
> >> too many? Do you mean that current users of nx_stack_pop() do not
> >> check for NULL return? I had a look and think that setting “*bytes”
> >> to zero when returning NULL should be enough for all users.
> > 
> > It appears to me that if stack->size is greater than 0 but less than the
> > number of bytes indicated by its last byte, then it will corrupt the
> > ofpbuf size and that this will later cause some kind of failure that
> > will be harder to debug than an assertion failure. 
> > 
> 
> OK, now i got it.  This is just to guard against (future) bugs in OVS itself.

Yes.

> >>> In ofputil_decode_packet_in_private(), it's probably worth checking the
> >>> format of the stack we pull from the payload, since a badly formatted
> >>> stack can segfault us (if we leave out assertions) or assert-fail us (if
> >>> we include them).
> >>> 
> >> 
> >> What do you mean with badly formatted stack? Zero-sized property? IMO
> >> even that would be properly pushed and popped from the stack, storing
> >> only the length (of zero) in the stack.
> > 
> > I mean that if the property contains, for example, a single byte with
> > value 0xff, then it's badly formatted because we can pop off a length
> > (255) but then popping off that number of bytes will underflow.
> 
> I did not change the encoding of the stack as properties, so each
> value in the stack is still encoded as a separate property, where the
> (aligned) value length is used as the property length. 

I guess I forgot that.

Thanks, that's fine then.

> ofpprop_pull() does the length checking for the properties and the
> current code in ofputil_decode_packet_in_private() assert fails on any
> error, which is not good, as a controller bug would crash OVS?

That's bad.  Maybe the fix is as simple as this, though.

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 156d8d2..421b9d7 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, 
Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 
Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -4061,7 +4061,9 @@ ofputil_decode_packet_in_private(const struct ofp_header 
*oh, bool loose,
 uint64_t type;
 
 error = ofpprop_pull(, , );
-ovs_assert(!error);
+if (error) {
+break;
+}
 
 switch (type) {
 case NXCPT_BRIDGE:
@@ -4124,7 +4126,7 @@ ofputil_decode_packet_in_private(const struct ofp_header 
*oh, bool loose,
 ofputil_packet_in_private_destroy(pin);
 }
 
-return 0;
+return error;
 }
 
 /* Frees data in 'pin' that is dynamically allocated by

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] assertion failing in commit_set_ipv4_action(), flow->nw_proto != base_flow->nw_proto

[Jarno Rajahalme]

I sent a patch to do this fix on OVS master. IMO we should also make the 
datapath more flexible so that it would be able to POP back to IP after PUSH 
actions on an IP packet in the same action list. But that will not make it to 
OVS 2.7.

I would appreciate if Thomas could apply the and test!

  Jarno

> On Dec 14, 2016, at 1:52 PM, Jarno Rajahalme  wrote:
> 
>> 
>> On Dec 13, 2016, at 8:44 PM, Takashi YAMAMOTO > > wrote:
>> 
>> 
>> 
>> On Wed, Dec 14, 2016 at 9:02 AM, Jarno Rajahalme > > wrote:
>> 
>>> On Dec 13, 2016, at 3:32 PM, Takashi YAMAMOTO >> > wrote:
>>> 
>>> 
>>> 
>>> On Tue, Dec 13, 2016 at 6:49 PM, Thomas Morin >> > wrote:
>>> Hi Jarno,
>>> 
>>> 2016-12-10, Jarno Rajahalme:
 On Dec 9, 2016, at 7:04 AM, Thomas Morin > wrote:
> 
> 2016-12-09, Thomas Morin:
>> In the same setup as the one on which the bug was observed, [...]
> 
> I was confused, I in fact tested the patch (branch-2.5) on a /different/ 
> setup as the one on which we hit the bug, using MPLS over a GRE tunnel 
> port, rather than plain MPLS over an eth port.
> Sorry if any confusion arised... I can test on the first setup if 
> relevant.
> 
 
 Maybe the kernel datapath does not support MPLS over a GRE tunnel port. 
 Having ‘dmesg’ output for the test run might reveal why the actions 
 validation fails.
>>> 
>>> The dmesg output was the following: 
>>> 
>>> [171295.258939] openvswitch: netlink: Flow actions may not be safe on all 
>>> matching packets.
>>> 
>>> I've tested the patch on the platform on which the bug was initially hit 
>>> (*not* using MPLS/GRE), and I have the following a few times in the logs 
>>> right after I do an "ovs-appctl fdb/flush":
>>> 
>>> 2016-12-13T09:44:08.449Z|1|dpif(handler68)|WARN|Dropped 3 log messages 
>>> in last 1 seconds (most recently, 1 seconds ago) due to excessive rate
>>> 2016-12-13T09:44:08.449Z|2|dpif(handler68)|WARN|system@ovs-system: 
>>> failed to put[create] (Invalid argument) 
>>> ufid:f046c4c4-b97f-436d-bd7c-91ed307275ac 
>>> recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(9),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64),eth_type(0x0800),ipv4(src=10.0.1.29,dst=10.0.0.3,proto=6,tos=0/0xfc,ttl=64,frag=no),tcp(src=54253,dst=8080),tcp_flags(0/0),
>>>  
>>> actions:set(ipv4(src=10.0.1.29,dst=10.0.0.3,ttl=63)),set(eth(src=b8:2a:72:de:1b:e3,dst=00:17:cb:79:2c:01)),push_mpls(label=433680,tc=0,ttl=63,bos=1,eth_type=0x8847),7,set(eth(src=fa:16:3e:61:c0:b5,dst=00:00:5e:00:43:64)),pop_mpls(eth_type=0x800),set(ipv4(src=10.0.1.29,dst=10.0.0.3,tos=0/0xfc,ttl=64)),push_vlan(vid=1,pcp=0),3,8,pop_vlan,13
>>> 
>>> And dmesg:
>>> [926833.612372] openvswitch: netlink: Flow actions may not be safe on all 
>>> matching packets.
>>> 
>>> it's complaining about set ipv4 after pop_mpls because it doesn't know 
>>> about the "restored" l3.
>>> while we can improve the kernel, i guess we need to stick with recirc for 
>>> now.
>>>  
>> 
>> This should do it? Does not break any existing tests on branch-2.5, but I 
>> did not create a test for this yet.
>> 
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index fb25f63..6ee2a72 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -2899,6 +2899,15 @@ xlate_commit_actions(struct xlate_ctx *ctx)
>>  {
>>  bool use_masked = ctx->xbridge->support.masked_set_action;
>>  
>> +/* An MPLS packet can be implicitly popped back to a non-MPLS packet, 
>> if a
>> + * patch port peer or a group bucket pushed MPLS.  Set the 'was_mpls' 
>> flag
>> + * also in that case. */
>> +if (eth_type_mpls(ctx->base_flow.dl_type)
>> +&& !eth_type_mpls(ctx->xin->flow.dl_type)
>> +&& ctx->xbridge->support.odp.recirc) {
>> +ctx->was_mpls = true;
>> +}
>> +
>> 
>> i guess this check needs to be somewhere around "ctx->was_mpls = 
>> old_was_mpls" in
>> affected functions. 
>> 
> 
> Right, as that is where the implicit MPLS POP action happens, when the 
> ‘ctx->xin->flow’ is restored.
> 
>   Jarno
> 
>>  ctx->xout->slow |= commit_odp_actions(>xin->flow, >base_flow,
>>ctx->odp_actions, ctx->wc,
>>use_masked);
>> 
>>   Jarno
>> 
>> 
>>> 
>>> 
>>> -Thomas
>>> 
>>> 
>>> 
>>> 
>> 
 On Dec 1, 2016, at 5:57 PM, Jarno Rajahalme > wrote:
 
 
> On Nov 30, 2016, at 8:50 PM, Ben Pfaff  > wrote:
> 
> On Wed, Nov 30, 2016 at 06:58:57PM -0800, Jarno Rajahalme 

Re: [ovs-dev] [PATCH 2/2] ofctrl: Fix warning from sparse.

[Justin Pettit]

> On Jan 5, 2017, at 5:03 PM, Ben Pfaff  wrote:
> 
> We've used sparse "bitwise" annotations to make ofp_ports into a different
> type, so this is required to avoid a sparse warning.
> 
> CC: Justin Pettit 
> Fixes: 714651c7db6a ("ovn-controller: Introduce "inject-pkt" ovs-appctl 
> command.")
> Signed-off-by: Ben Pfaff 

D'oh.  Forgot to run sparse again. I wish there were a configure flag to always 
enable it.

Thanks for catching the problem!

Acked-by: Justin Pettit 

--Justin




___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovn-nbctl: Specify the range of the tag for "ovn-nbctl lsp-add" command.

[zhaojingjing]

When configuring the wrong tag for "ovn-nbctl lsp-add" command，
it shows "invalid tag". The range of the tag is not known.

Signed-off-by: zhaojingjing 
---
 ovn/utilities/ovn-nbctl.8.xml | 4 +++-
 ovn/utilities/ovn-nbctl.c | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index b8fdd79..9e8c4e5 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -125,7 +125,9 @@
 
   Creates on switch a logical switch port named
   port that is a child of parent that is
-  identified with VLAN ID tag_request.  If
+  identified with VLAN ID tag_request. 
+  Tag_request must be between 0 and 
+  4095, inclusive. If
   tag_request is 0, ovn-northd
   generates a tag that is unique in the scope of parent.
   This is useful in cases such as virtualized container environments
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index af1eeab..ce639aa 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -807,7 +807,7 @@ nbctl_lsp_add(struct ctl_context *ctx)
 parent_name = ctx->argv[3];
 if (!ovs_scan(ctx->argv[4], "%"SCNd64, )
 || tag < 0 || tag > 4095) {
-ctl_fatal("%s: invalid tag", ctx->argv[4]);
+ctl_fatal("%s: invalid tag, the tag in range 0 to 4095.", 
ctx->argv[4]);
 }
 } else {
 ctl_fatal("lsp-add with parent must also specify a tag");
-- 
1.8.3.1


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovn-nbctl:The range of "PRIORITY" for "ovn-nbctl acl-add " command is wrong.

[zhaojingjing]

The range of "PRIORITY" for "ovn-nbctl acl-add " command is 1 to 65534 in
ovn-nbctl.8.xml",When configuring this command, it indicates that "
priority must in range 0...32767".The range of priority is inconsistent
in "ovn-nbctl.8.xml" and "ovn-nbctl.c".

Signed-off-by: zhaojingjing 
---
 ovn/utilities/ovn-nbctl.8.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index b8fdd79..d6bd79e 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -81,7 +81,7 @@
 Adds the specified ACL to switch.
 direction must be either from-lport or
 to-lport.  priority must be between
-1 and 65534, inclusive.  If
+0 and 32767, inclusive.  If
 --log is specified, packet logging is enabled for the
 ACL.  A full description of the fields are in ovn-nb(5).
   
-- 
1.8.3.1


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-data: Add support for integer ranges in database commands

[Ben Pfaff]

On Thu, Dec 29, 2016 at 03:55:46PM -0700, Lukasz Rzasik wrote:
> Adding / removing a range of integers to a column accepting a set of
> integers requires enumarating all of the integers. This patch simplifies
> it by introducing 'range' concept to the database commands. Two integers
> separated by a hyphen represent an inclusive range.
> 
> The patch adds positive and negative tests for the new syntax.
> The patch was tested by 'make check'. Covarage was tested by
> 'make check-lcov'.
> 
> Signed-off-by: Lukasz Rzasik 
> Suggested-by: 
> Suggested-by: Ben Pfaff 

Thanks.  I applied this to master.

I folded in the following incremental, which is mostly stylistic.

--8<--cut here-->8--

diff --git a/NEWS b/NEWS
index beee6cc..1b0aa5d 100644
--- a/NEWS
+++ b/NEWS
@@ -59,6 +59,9 @@ Post-v2.6.0
  * Ports now have a "protected" flag. Protected ports can not forward
frames to other protected ports. Unprotected ports can receive and
forward frames to protected and other unprotected ports.
+   - ovs-vsctl, ovn-nbctl, ovn-sbctl, vtep-ctl:
+ * Database commands now accept integer ranges, e.g. "set port
+   eth0 trunks=1-10" to enable trunking VLANs 1 to 10.
 
 v2.6.0 - 27 Sep 2016
 -
diff --git a/lib/db-ctl-base.man b/lib/db-ctl-base.man
index 26828d6..88529b5 100644
--- a/lib/db-ctl-base.man
+++ b/lib/db-ctl-base.man
@@ -31,7 +31,7 @@ columns can have an empty set of values, represented as 
\fB[]\fR, and
 square brackets may optionally enclose other non-empty sets or single
 values as well. For a column accepting a set of integers, database commands
 accept a range. A range is represented by two integers separated by
-\fB-\fR. A range is inclusive. A range have a maximum size of 4096
+\fB-\fR. A range is inclusive. A range has a maximum size of 4096
 elements. If more elements are needed, they can be specified in seperate
 ranges.
 .PP
diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
index a7a6eef..840e18f 100644
--- a/lib/ovsdb-data.c
+++ b/lib/ovsdb-data.c
@@ -501,9 +501,9 @@ ovsdb_atom_from_string__(union ovsdb_atom *atom,
 long long int integer, end;
 if (range_end_atom
 && str_to_llong_range(s, 10, , )) {
-if (!ovsdb_atom_range_check(integer, end)) {
+if (end < integer) {
 return xasprintf("\"%s\" is not a valid range. "
-"Range start has to be less than end.", s);
+"Range end cannot be before start.", s);
 }
 *range_end_atom = alloc_default_atoms(type, 1);
 if (!(*range_end_atom)) {
@@ -638,24 +638,22 @@ ovsdb_atom_from_string(union ovsdb_atom *atom,
 
 if (!error && range_end_atom && *range_end_atom) {
 /* Check range constraints */
+int64_t start = atom->integer;
+int64_t end = (*range_end_atom)->integer;
 if (base->enum_) {
-/* If enum, every atom in range has to be checked */
-union ovsdb_atom ai = *atom;
-++ai.integer;
-while (!error) {
+for (int64_t i = start + 1; i <= end; i++) {
+union ovsdb_atom ai = { .integer = i };
 error = ovsdb_atom_check_constraints(, base);
-if (ai.integer == (*range_end_atom)->integer) {
+if (error) {
 break;
 }
-++ai.integer;
 }
 } else {
 error = ovsdb_atom_check_constraints(*range_end_atom, base);
 }
 
 if (!error) {
-error = ovsdb_atom_range_check_size(atom->integer,
-(*range_end_atom)->integer);
+error = ovsdb_atom_range_check_size(start, end);
 }
 }
 
@@ -1857,11 +1855,9 @@ ovsdb_datum_add_unsafe(struct ovsdb_datum *datum,
 datum->n += range_end_atom ?
 (range_end_atom->integer - key->integer + 1) : 1;
 datum->keys = xrealloc(datum->keys, datum->n * sizeof *datum->keys);
-if (range_end_atom
-&& ovsdb_atom_range_check(key->integer, range_end_atom->integer)) {
-union ovsdb_atom ai;
-for (ai = *key; ai.integer <= range_end_atom->integer; ++ai.integer) {
-ovsdb_atom_clone(>keys[idx++], , type->key.type);
+if (range_end_atom && key->integer <= range_end_atom->integer) {
+for (int64_t i = key->integer; i <= range_end_atom->integer; i++) {
+datum->keys[idx++].integer = i;
 }
 } else {
 ovsdb_atom_clone(>keys[idx], key, type->key.type);
@@ -2146,7 +2142,8 @@ ovsdb_token_is_delim(unsigned char c)
 struct ovsdb_error *
 ovsdb_atom_range_check_size(int64_t range_start, int64_t range_end)
 {
-if (range_end - range_start >= MAX_OVSDB_ATOM_RANGE_SIZE) {
+if ((uint64_t) range_end - (uint64_t) range_start
+>= 

Re: [ovs-dev] ABI versioning (was Re: [PATCH] rhel: Support...)

[Ben Pfaff]

On Thu, Jan 05, 2017 at 10:52:00AM -0500, Aaron Conole wrote:
> I'm uncomfortable with bumping this and just 'let the chips fall where
> they may,' since having a version that hasn't changed is the virtually
> the same as not having a version.  The instant we bump, we state that
> the version means something, so I'm not comfortable just shipping a
> patch that changes the version without some accompanying documentation
> explaining *what* that means.  It also means we need to be more diligent
> with reviews and watch for potential ABI breakages, with a compatibility
> strategy in place (when ABI/API changes can be made, how they are made,
> etc.).  I think it's got more implication than just a single line change
> to the configure.ac file.

My intent has been that each release branch is a separate major version,
so that we maintain ABI compatibility across all 2.5.x releases, all
2.6.x releases, and so on.  I don't know whether we've succeeded at
this, since it hasn't been a focus, but that was my original intent.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev