Re: [ovs-dev] [PATCH 5/5] ovsdb-idl: Wake up ovsdb_idl_loop when a transaction commits.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 10:24:43PM -0500, Ryan Moats wrote:
> "dev"  wrote on 07/27/2016 02:03:25 AM:
> 
> > From: Ben Pfaff 
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff 
> > Date: 07/27/2016 02:04 AM
> > Subject: [ovs-dev] [PATCH 5/5] ovsdb-idl: Wake up ovsdb_idl_loop
> > when a transaction commits.
> > Sent by: "dev" 
> >
> > There is a fair amount of code that defers modifying the database when a
> > transaction cannot be created (because there is already one outstanding).
> > This code tends to assume that the main loop will wake up again when it
> > becomes possible again to modify the database, but the actual
> ovsdb_id_loop
> > implementation only did this if the database had changed.  This is too
> > conservative a policy and may account for some failures I've seen in
> tests.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> 
> Ben, while I can't tell you to hold up on this if somebody else acks it,
> my first thought (after looking at it) is that checking it against the
> unit tests cases answers only half of the questions that it needs to
> answer.
> Since it is changing the cycle time, there is the question of whether it
> provides a measurable change in E2E performance. Therefore, with your
> indulgence, I'm going to try and throw it into an openstack cloud tomorrow
> and run some rally tests against it to see if indeed does result in a
> measurable change in performance.

This commit is not supposed to be a performance fix, it's supposed to be
a correctness fix.

I don't have a really good example of the kind of thing that it fixes so
I'm happy to sit on it for a while until I do.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v5] JSON serialization via Python's json lib

[Ben Pfaff]

On Mon, Jul 25, 2016 at 06:55:49PM -0500, Terry Wilson wrote:
> Sigh. And of course I had libopenvswitch installed on the system as
> well and removing it breaks building the extensions with the above
> patch. This is the kind of thing that would be much easier if the
> Python lib was its own project with its own test suite and it could
> just always assume libopenvswitch was installed on the system as a
> build dependency.

The Open vSwitch build process uses some of the OVS Python libraries, so
as far as I can tell that would create a circular dependency.

I don't know what is the right thing to do.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] tests: Ignore proxy configuration.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 05:29:44PM -0700, Jarno Rajahalme wrote:
> As any proxy configuration may ruin kernel testsuite tests, it is
> better to ignore all proxy configuration.
> 
> Suggested-by: Ben Pfaff 
> Signed-off-by: Jarno Rajahalme 

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

Re: [ovs-dev] [PATCH 2/4] tests: Remove trim_zeros() from ovn tests.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 07:58:04PM -0700, Daniele Di Proietto wrote:
> trim_zeros() is not necessary anymore, since now we don't pad packets in
> the userspace datapath.
> 
> Signed-off-by: Daniele Di Proietto 

Oops, I just committed a patch that made a change related to trim_zeros,
so you'll have to adjust this.  Sorry!  Still, I imagine that most of it was
sed -e 's/| trim_zeros//' -i ovn.at

Acked-by: Ben Pfaff 


Thanks,

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

Re: [ovs-dev] [PATCH 1/4] netdev-*: Do not use dp_packet_pad() in recv() functions.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 07:58:03PM -0700, Daniele Di Proietto wrote:
> All the netdevs used by dpif-netdev (except for netdev-dpdk) have a
> dp_packet_pad() call in the receive function, probably because the
> userspace datapath couldn't handle properly short packets.
> 
> This doesn't appear to be the case anymore.
> 
> This commit removes the call to have a more consistent behavior with the
> kernel datapath.
> 
> All the testsuite changes in this commit adjust the expectations for
> packet lengths in flow dumps and other stats.  There's only one fix in
> ovn.at: one of the test_ip() functions generated an incomplete udp
> packet, which was not a problem until now, because of the padding.
> 
> Signed-off-by: Daniele Di Proietto 

It must have been a pain updating all the tests.  Thanks for dealing
with that.

As long as the tests pass:
Acked-by: Ben Pfaff 

Thanks,

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

Re: [ovs-dev] [PATCH 3/5] ovn-nbctl: Add "sync" command to wait for previous changes to take effect.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 10:10:57PM -0500, Ryan Moats wrote:
> "dev"  wrote on 07/27/2016 02:03:23 AM:
> 
> > From: Ben Pfaff 
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff 
> > Date: 07/27/2016 02:04 AM
> > Subject: [ovs-dev] [PATCH 3/5] ovn-nbctl: Add "sync" command to wait
> > for previous changes to take effect.
> > Sent by: "dev" 
> >
> > It's slow to add --wait to every ovn-nbctl command; only the last command
> > needs it.  But it's sometimes inconvenient to add it to the last command
> > if it's in a loop, etc.  This makes it possible to separately wait for
> > the OVN southbound or hypervisors to catch up to the northbound.
> >
> > Also, modify the tests to replace "sleep" invocations that were waiting
> > for hypervisors to catch up by --wait=hv or by invocations of "ovn-nbctl
> > --wait=hv sync".
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> 
> Paradoxically, after applying this, I saw more failures at the sync check
> points that I was seeing with the original code.  Rechecking showed that
> the tests pass, so I'm thinking that we might want to consider either
> increasing the timeouts from 5 seconds or making them configurable so
> that people building/testsing on slower systems (and/or running tests in
> parallel) can tune them to avoid false positives.
> 
> Honestly, all of that can be handled in a later patchset, so...
> 
> Acked-by: Ryan Moats 

Thanks for the review.

It seems likely to me that there's still a small remaining race with
sync (or equivalently with --wait=hv).  I know that there's one in the
case where ovs-vswitchd has already handled packets in a given flow, but
most of these cases don't have that.  I think there must be something
else in ovn-controller itself; maybe it's not properly updating
everything southbound of it before it reports that it's in sync.

I think I'll sit on this one for a bit until we get that sorted.

(Patch 5 in the series fixes some additional races that this patch
exposes, but it doesn't fix all of them.)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 4/5] tests: Remove most packet-forwarding related"sleep"s from OVN tests.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 10:44:45PM -0500, Flaviof wrote:
> On Thu, Jul 28, 2016 at 10:16 PM, Ryan Moats  wrote:
> 
> > "dev"  wrote on 07/27/2016 02:03:24 AM:
> >
> > > From: Ben Pfaff 
> > > To: dev@openvswitch.org
> > > Cc: Ben Pfaff 
> > > Date: 07/27/2016 02:04 AM
> > > Subject: [ovs-dev] [PATCH 4/5] tests: Remove most packet-forwarding
> > > related "sleep"s from OVN tests.
> > > Sent by: "dev" 
> > >
> > > These arbitrary sleeps are often longer than necessary and can be too
> > short
> > > in corner cases.  This commit replaces them by a common macro that waits
> > > only as long as necessary.
> > >
> > > Signed-off-by: Ben Pfaff 
> > > ---
> >
> > Much, much, much nicer!
> >
> > Acked-by: Ryan Moats 
> >
> 
> nice
> 
> Acked-by: Flavio Fernandes 

Thanks, applied to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Reference:

[British Lottery Payment Review Panel]

 

 



  
 

Attention: Recipient,
We have enclosed the IRREVOCABLE PAYMENT ORDER for your perusal.
Your co-operation is essential to this demand.
Truly,
Rosemary Clark(Information Department)British Lottery Payment Review Commission















 
   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

   

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

[ovs-dev] [PATCH] ovn: Add second ACL stage

[Mickey Spiegel]

From: Mickey Spiegel 

This patch adds a second logical switch ingress ACL stage, and
correspondingly a second logical switch egress ACL stage.  This
allows for more than one ACL-based feature to be applied in the
ingress and egress logical switch pipelines.  The features
driving the different ACL stages may be configured by different
users, for example an application deployer managing security
groups and a network or security admin configuring network ACLs
or firewall rules.

Each ACL stage is self contained.  The "action" for the
highest-"priority" matching row in an ACL stage determines a
packet's treatment.  A separate "action" will be determined in
each ACL stage, according to the ACL rules configured for that
ACL stage.  The "priority" values are only relevant within the
context of an ACL stage.

ACL rules that do not specify an ACL stage are applied to the
default "acl" stage.

Signed-off-by: Mickey Spiegel 
---
 ovn/northd/ovn-northd.c   | 319 +++---
 ovn/ovn-nb.ovsschema  |   7 +-
 ovn/ovn-nb.xml|  25 
 ovn/utilities/ovn-nbctl.c |  35 +++--
 tests/ovn-nbctl.at|  30 +++--
 5 files changed, 264 insertions(+), 152 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 3047b76..2e1d314 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -97,12 +97,13 @@ enum ovn_stage {
 PIPELINE_STAGE(SWITCH, IN,  PRE_LB, 4, "ls_in_pre_lb") \
 PIPELINE_STAGE(SWITCH, IN,  PRE_STATEFUL,   5, "ls_in_pre_stateful")\
 PIPELINE_STAGE(SWITCH, IN,  ACL,6, "ls_in_acl")  \
-PIPELINE_STAGE(SWITCH, IN,  LB, 7, "ls_in_lb")   \
-PIPELINE_STAGE(SWITCH, IN,  STATEFUL,   8, "ls_in_stateful") \
-PIPELINE_STAGE(SWITCH, IN,  ARP_ND_RSP, 9, "ls_in_arp_rsp")  \
-PIPELINE_STAGE(SWITCH, IN,  DHCP_OPTIONS,   10, "ls_in_dhcp_options") \
-PIPELINE_STAGE(SWITCH, IN,  DHCP_RESPONSE,  11, "ls_in_dhcp_response") \
-PIPELINE_STAGE(SWITCH, IN,  L2_LKUP,12, "ls_in_l2_lkup")  \
+PIPELINE_STAGE(SWITCH, IN,  ACL2,   7, "ls_in_acl2")  \
+PIPELINE_STAGE(SWITCH, IN,  LB, 8, "ls_in_lb")   \
+PIPELINE_STAGE(SWITCH, IN,  STATEFUL,   9, "ls_in_stateful") \
+PIPELINE_STAGE(SWITCH, IN,  ARP_ND_RSP, 10, "ls_in_arp_rsp")  \
+PIPELINE_STAGE(SWITCH, IN,  DHCP_OPTIONS,   11, "ls_in_dhcp_options") \
+PIPELINE_STAGE(SWITCH, IN,  DHCP_RESPONSE,  12, "ls_in_dhcp_response") \
+PIPELINE_STAGE(SWITCH, IN,  L2_LKUP,13, "ls_in_l2_lkup")  \
   \
 /* Logical switch egress stages. */   \
 PIPELINE_STAGE(SWITCH, OUT, PRE_LB,   0, "ls_out_pre_lb") \
@@ -110,9 +111,10 @@ enum ovn_stage {
 PIPELINE_STAGE(SWITCH, OUT, PRE_STATEFUL, 2, "ls_out_pre_stateful")  \
 PIPELINE_STAGE(SWITCH, OUT, LB,   3, "ls_out_lb")\
 PIPELINE_STAGE(SWITCH, OUT, ACL,  4, "ls_out_acl")\
-PIPELINE_STAGE(SWITCH, OUT, STATEFUL, 5, "ls_out_stateful")   \
-PIPELINE_STAGE(SWITCH, OUT, PORT_SEC_IP,  6, "ls_out_port_sec_ip")\
-PIPELINE_STAGE(SWITCH, OUT, PORT_SEC_L2,  7, "ls_out_port_sec_l2")\
+PIPELINE_STAGE(SWITCH, OUT, ACL2, 5, "ls_out_acl2")\
+PIPELINE_STAGE(SWITCH, OUT, STATEFUL, 6, "ls_out_stateful")   \
+PIPELINE_STAGE(SWITCH, OUT, PORT_SEC_IP,  7, "ls_out_port_sec_ip")\
+PIPELINE_STAGE(SWITCH, OUT, PORT_SEC_L2,  8, "ls_out_port_sec_l2")\
   \
 /* Logical router ingress stages. */  \
 PIPELINE_STAGE(ROUTER, IN,  ADMISSION,   0, "lr_in_admission")\
@@ -193,6 +195,48 @@ ovn_stage_to_datapath_type(enum ovn_stage stage)
 default: OVS_NOT_REACHED();
 }
 }
+
+static enum ovn_stage
+ovn_stage_from_acl_stage(const char *acl_stage, enum ovn_pipeline pipeline) {
+if ((pipeline == P_IN) && !acl_stage) {
+return S_SWITCH_IN_ACL;
+} else if ((pipeline == P_OUT) && !acl_stage) {
+return S_SWITCH_OUT_ACL;
+} else if ((pipeline == P_IN) && !strcmp(acl_stage, "acl")) {
+return S_SWITCH_IN_ACL;
+} else if ((pipeline == P_OUT) && !strcmp(acl_stage, "acl")) {
+return S_SWITCH_OUT_ACL;
+} else if ((pipeline == P_IN) && !strcmp(acl_stage, "acl2")) {
+return S_SWITCH_IN_ACL2;
+} else if ((pipeline == P_OUT) && !strcmp(acl_stage, "acl2")) {
+return S_SWITCH_OUT_ACL2;
+} else if (pipeline == P_IN) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_WARN_RL(, "Bad configuration: invalid ACL stage %s",
+ acl_stage);
+return S_SWITCH_IN_ACL;
+} else {
+static struct 

Re: [ovs-dev] [PATCH 2/5] ovn: Make two end-to-end tests more reliable.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 10:02:53PM -0500, Ryan Moats wrote:
> "dev"  wrote on 07/27/2016 02:03:22 AM:
> 
> > From: Ben Pfaff 
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff 
> > Date: 07/27/2016 02:03 AM
> > Subject: [ovs-dev] [PATCH 2/5] ovn: Make two end-to-end tests more
> reliable.
> > Sent by: "dev" 
> >
> > These tests change the northbound configuration and then immediately
> check
> > that the changes have taken effect on the hypervisors.  This can't work
> > reliably, so add a sleep to each one.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  tests/ovn.at | 9 +
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 0fe2527..34fdd5d 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -912,6 +912,11 @@ done
> >  # set address for lp13 with invalid characters.
> >  # lp13 should be configured with only 192.168.0.13.
> >  ovn-nbctl lsp-set-addresses lp13 "f0:00:00:00:00:13 192.168.0.13
> > invalid 192.169.0.13"
> > +
> > +# Allow some time for ovn-northd and ovn-controller to catch up.
> > +# XXX This should be more systematic.
> > +sleep 1
> > +
> >  sip=`ip_to_hex 192 168 0 11`
> >  tip=`ip_to_hex 192 168 0 13`
> >  test_arp 11 f011  $sip $tip f013
> > @@ -2494,6 +2499,10 @@ as hv1 ovs-ofctl dump-flows br-int
> >  #Disable router R1
> >  ovn-nbctl set Logical_Router R1 enabled=false
> >
> > +# Allow some time for ovn-northd and ovn-controller to catch up.
> > +# XXX This should be more systematic.
> > +sleep 1
> > +
> >  echo "-SB dump-"
> >  ovn-sbctl list datapath_binding
> >  echo "-"
> > --
> > 2.1.3
> 
> works for me, so...
> 
> Acked-by: Ryan Moats 
> 

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

Re: [ovs-dev] [PATCH 1/5] tests: Define trim_zeros in only one place.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 09:56:09PM -0500, Ryan Moats wrote:
> "dev"  wrote on 07/27/2016 02:03:21 AM:
> 
> > From: Ben Pfaff 
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff 
> > Date: 07/27/2016 02:03 AM
> > Subject: [ovs-dev] [PATCH 1/5] tests: Define trim_zeros in only one
> place.
> > Sent by: "dev" 
> >
> > Defining trim_zeros in a common place allows us to skip defining it in
> > every test that needs it.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> 
> cleaning up code is always a good thing and tests pass for me
> after applying it, so ...
> 
> Acked-by: Ryan Moats 

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

Re: [ovs-dev] [PATCH v3] ovn-controller: Persist desired conntrack groups.

[Flaviof]

On Thu, Jul 28, 2016 at 5:17 PM, Ryan Moats  wrote:

> With incremental processing of logical flows desired conntrack groups
> are not being persisted.  This patch adds this capability, with the
> side effect of adding a ds_clone method that this capability leverages.
>
> Signed-off-by: Ryan Moats 
> Reported-by: Guru Shetty 
> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
> Fixes: 70c7cfe ("ovn-controller: Add incremental processing to lflow_run
> and physical_run")
> ---
>  include/openvswitch/dynamic-string.h |  1 +
>  include/ovn/actions.h|  6 +
>  lib/dynamic-string.c |  9 
>  ovn/controller/lflow.c   |  2 ++
>  ovn/controller/ofctrl.c  | 43
> 
>  ovn/controller/ofctrl.h  |  5 -
>  ovn/controller/ovn-controller.c  |  2 +-
>  ovn/lib/actions.c|  1 +
>  8 files changed, 53 insertions(+), 16 deletions(-)
>
> diff --git a/include/openvswitch/dynamic-string.h
> b/include/openvswitch/dynamic-string.h
> index dfe2688..bf1f64a 100644
> --- a/include/openvswitch/dynamic-string.h
> +++ b/include/openvswitch/dynamic-string.h
> @@ -73,6 +73,7 @@ void ds_swap(struct ds *, struct ds *);
>
>  int ds_last(const struct ds *);
>  bool ds_chomp(struct ds *, int c);
> +void ds_clone(struct ds *, struct ds *);
>
>  /* Inline functions. */
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 114c71e..55720ce 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -22,7 +22,9 @@
>  #include "compiler.h"
>  #include "openvswitch/hmap.h"
>  #include "openvswitch/dynamic-string.h"
> +#include "openvswitch/uuid.h"
>  #include "util.h"
> +#include "uuid.h"
>

Isn't   #include "openvswitch/uuid.h"enough?
Consider not doing   #include "uuid.h"


>
>  struct expr;
>  struct lexer;
> @@ -43,6 +45,7 @@ struct group_table {
>  struct group_info {
>  struct hmap_node hmap_node;
>  struct ds group;
> +struct uuid lflow_uuid;
>  uint32_t group_id;
>  };
>
> @@ -107,6 +110,9 @@ struct action_params {
>  /* A struct to figure out the group_id for group actions. */
>  struct group_table *group_table;
>
> +/* The logical flow uuid that drove this action. */
> +struct uuid lflow_uuid;
> +
>  /* OVN maps each logical flow table (ltable), one-to-one, onto a
> physical
>   * OpenFlow flow table (ptable).  A number of parameters describe this
>   * mapping and data related to flow tables:
> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
> index 1f17a9f..6f7b610 100644
> --- a/lib/dynamic-string.c
> +++ b/lib/dynamic-string.c
> @@ -456,3 +456,12 @@ ds_chomp(struct ds *ds, int c)
>  return false;
>  }
>  }
> +
> +void
> +ds_clone(struct ds *dst, struct ds *source)
> +{
> +dst->length = source->length;
> +dst->allocated = dst->length;
> +dst->string = xmalloc(dst->allocated + 1);
> +memcpy(dst->string, source->string, dst->allocated + 1);
> +}
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index a4f3322..e38c32a 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -383,6 +383,7 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
>
>  if (full_flow_processing) {
>  ovn_flow_table_clear();
> +ovn_group_table_clear(group_table, false);
>  full_logical_flow_processing = true;
>  full_neighbor_flow_processing = true;
>  full_flow_processing = false;
> @@ -515,6 +516,7 @@ consider_logical_flow(const struct lport_index *lports,
>  .aux = ,
>  .ct_zones = ct_zones,
>  .group_table = group_table,
> +.lflow_uuid = lflow->header_.uuid,
>
>  .n_tables = LOG_PIPELINE_LEN,
>  .first_ptable = first_ptable,
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index dd9f5ec..54bea99 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
>


shouldn't  we also need to call
ovn_group_table_clear(groups, true);
from existing function ovn_flow_table_destroy(void)  ?


Besides these, lgtm

Acked-by: Flavio Fernandes 



> @@ -140,8 +140,6 @@ static ovs_be32 queue_msg(struct ofpbuf *);
>  static void ovn_flow_table_destroy(void);
>  static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
>
> -static void ovn_group_table_clear(struct group_table *group_table,
> -  bool existing);
>  static struct ofpbuf *encode_group_mod(const struct ofputil_group_mod *);
>
>  static void ofctrl_recv(const struct ofp_header *, enum ofptype);
> @@ -150,12 +148,15 @@ static struct hmap match_flow_table =
> HMAP_INITIALIZER(_flow_table);
>  static struct hindex uuid_flow_table =
> HINDEX_INITIALIZER(_flow_table);
>
>  void
> -ofctrl_init(void)
> +ofctrl_init(struct group_table *group_table)
>  {
> 

Re: [ovs-dev] [PATCH 3/5] ovn-nbctl: Add "sync" command to wait for previous changes to take effect.

[Ryan Moats]

"dev"  wrote on 07/27/2016 02:03:23 AM:

> From: Ben Pfaff 
> To: dev@openvswitch.org
> Cc: Ben Pfaff 
> Date: 07/27/2016 02:04 AM
> Subject: [ovs-dev] [PATCH 3/5] ovn-nbctl: Add "sync" command to wait
> for previous changes to take effect.
> Sent by: "dev" 
>
> It's slow to add --wait to every ovn-nbctl command; only the last command
> needs it.  But it's sometimes inconvenient to add it to the last command
> if it's in a loop, etc.  This makes it possible to separately wait for
> the OVN southbound or hypervisors to catch up to the northbound.
>
> Also, modify the tests to replace "sleep" invocations that were waiting
> for hypervisors to catch up by --wait=hv or by invocations of "ovn-nbctl
> --wait=hv sync".
>
> Signed-off-by: Ben Pfaff 
> ---

Paradoxically, after applying this, I saw more failures at the sync check
points that I was seeing with the original code.  Rechecking showed that
the tests pass, so I'm thinking that we might want to consider either
increasing the timeouts from 5 seconds or making them configurable so
that people building/testsing on slower systems (and/or running tests in
parallel) can tune them to avoid false positives.

Honestly, all of that can be handled in a later patchset, so...

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

Re: [ovs-dev] [PATCH 1/5] tests: Define trim_zeros in only one place.

[Flaviof]

On Wed, Jul 27, 2016 at 2:03 AM, Ben Pfaff  wrote:

> Defining trim_zeros in a common place allows us to skip defining it in
> every test that needs it.
>
> Signed-off-by: Ben Pfaff 
>

Acked-by: Flavio Fernandes 



> ---
>  tests/ovn.at | 61
> +++-
>  1 file changed, 11 insertions(+), 50 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 86efcf5..0fe2527 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1,3 +1,14 @@
> +# trim_zeros()
> +#
> +# Removes pairs of trailing zeros from lines of text.  Useful for
> +# fairly comparing Ethernet packets that might have been padded out to
> +# a minimum (e.g. 64-byte) length.
> +m4_divert_text([PREPARE_TESTS],
> +  [trim_zeros() {
> + sed 's/\(00\)\{1,\}$//' "$@"
> +   }
> +])
> +
>  AT_BANNER([OVN components])
>
>  AT_SETUP([ovn -- lexer])
> @@ -737,9 +748,6 @@ vif_to_hv() {
>  # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
>  # more) list the VIFs on which the packet should be received.  INPORT and
> the
>  # OUTPORTs are specified as logical switch port numbers, e.g. 11 for
> vif11.
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>  for i in 1 2 3; do
>  for j in 1 2 3; do
>  : > $i$j.expected
> @@ -1038,9 +1046,6 @@ vif_to_hv() {
>  # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
>  # more) list the VIFs on which the packet should be received.  INPORT and
> the
>  # OUTPORTs are specified as logical switch port numbers, e.g. 11 for
> vif11.
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>  for i in 1 2; do
>  for j in 1 2 3 4 5; do
>  : > $i$j.expected
> @@ -1223,9 +1228,6 @@ sleep 1
>  # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
>  # more) list the VIFs on which the packet should be received.  INPORT and
> the
>  # OUTPORTs are specified as logical switch port numbers, e.g. 1 for vif1.
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>  for i in 1 2 3; do
>  : > $i.expected
>  done
> @@ -1384,9 +1386,6 @@ sleep 1
>  # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
>  # more) list the VIFs on which the packet should be received.  INPORT and
> the
>  # OUTPORTs are specified as lport numbers, e.g. 1 for vif1.
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>  for i in 1 2 3; do
>  : > $i.expected
>  done
> @@ -1593,9 +1592,6 @@ sleep 1
>  # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
>  # more) list the VIFs on which the packet should be received.  INPORT and
> the
>  # OUTPORTs are specified as logical switch port numbers, e.g. 123 for
> vif123.
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>  for i in 1 2 3; do
>  for j in 1 2 3; do
>  for k in 1 2 3; do
> @@ -1951,10 +1947,6 @@ vif_to_hv() {
>  echo hv${1%?}
>  }
>
> -
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>  for i in 1 2 3; do
>  for j in 1 2 3; do
>  : > $i$j.expected
> @@ -2372,9 +2364,6 @@ sleep 1
>  ip_to_hex() {
>  printf "%02x%02x%02x%02x" "$@"
>  }
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>
>  # Packet to send.
>  src_mac="f0010203"
> @@ -2474,9 +2463,6 @@ sleep 1
>  ip_to_hex() {
>  printf "%02x%02x%02x%02x" "$@"
>  }
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>
>  # Packet to send.
>  src_mac="f0010203"
> @@ -2610,9 +2596,6 @@ sleep 1
>  ip_to_hex() {
>  printf "%02x%02x%02x%02x" "$@"
>  }
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>
>  # Packet to send.
>  src_mac="f0010203"
> @@ -2768,9 +2751,6 @@ sleep 1
>  ip_to_hex() {
>  printf "%02x%02x%02x%02x" "$@"
>  }
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>
>  # Send ip packets between foo1 and alice1
>  src_mac="f0010203"
> @@ -2865,9 +2845,6 @@ AT_CHECK([ovs-vsctl add-port br-int localvif1 -- set
> Interface localvif1 externa
>
>  # Wait for packet to be received.
>  OVS_WAIT_UNTIL([test `wc -c < "hv/snoopvif-tx.pcap"` -ge 50])
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>  $PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv/snoopvif-tx.pcap |
> trim_zeros > packets
>
>  
> expected="f00108060001080006040001f001c0a80102c0a80102"
>  echo $expected > expout
> @@ -2996,9 +2973,6 @@ sleep 1
>  ip_to_hex() {
>  printf "%02x%02x%02x%02x" "$@"
>  }
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>
>  # Send ip packets between foo1 and alice1
>  src_mac="f0010203"
> @@ -3138,10 +3112,6 @@ sleep 2
>
>  as hv1 ovs-vsctl show
>
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
> -
>  # This shell function sends a DHCP request packet
>  # test_dhcp INPORT SRC_MAC DHCP_TYPE OFFER_IP ...
>  test_dhcp() {
> @@ -3443,9 +3413,6 @@ sleep 2
>  ip_to_hex() {
>  printf "%02x%02x%02x%02x" "$@"
>  }
> -trim_zeros() {
> -sed 's/\(00\)\{1,\}$//'
> -}
>
>  

Re: [ovs-dev] [PATCH 4/5] tests: Remove most packet-forwarding related"sleep"s from OVN tests.

[Flaviof]

On Thu, Jul 28, 2016 at 10:16 PM, Ryan Moats  wrote:

> "dev"  wrote on 07/27/2016 02:03:24 AM:
>
> > From: Ben Pfaff 
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff 
> > Date: 07/27/2016 02:04 AM
> > Subject: [ovs-dev] [PATCH 4/5] tests: Remove most packet-forwarding
> > related "sleep"s from OVN tests.
> > Sent by: "dev" 
> >
> > These arbitrary sleeps are often longer than necessary and can be too
> short
> > in corner cases.  This commit replaces them by a common macro that waits
> > only as long as necessary.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
>
> Much, much, much nicer!
>
> Acked-by: Ryan Moats 
>

nice

Acked-by: Flavio Fernandes 



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

Re: [ovs-dev] [PATCH 2/5] ovn: Make two end-to-end tests more reliable.

[Ryan Moats]

"dev"  wrote on 07/27/2016 02:03:22 AM:

> From: Ben Pfaff 
> To: dev@openvswitch.org
> Cc: Ben Pfaff 
> Date: 07/27/2016 02:03 AM
> Subject: [ovs-dev] [PATCH 2/5] ovn: Make two end-to-end tests more
reliable.
> Sent by: "dev" 
>
> These tests change the northbound configuration and then immediately
check
> that the changes have taken effect on the hypervisors.  This can't work
> reliably, so add a sleep to each one.
>
> Signed-off-by: Ben Pfaff 
> ---
>  tests/ovn.at | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 0fe2527..34fdd5d 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -912,6 +912,11 @@ done
>  # set address for lp13 with invalid characters.
>  # lp13 should be configured with only 192.168.0.13.
>  ovn-nbctl lsp-set-addresses lp13 "f0:00:00:00:00:13 192.168.0.13
> invalid 192.169.0.13"
> +
> +# Allow some time for ovn-northd and ovn-controller to catch up.
> +# XXX This should be more systematic.
> +sleep 1
> +
>  sip=`ip_to_hex 192 168 0 11`
>  tip=`ip_to_hex 192 168 0 13`
>  test_arp 11 f011  $sip $tip f013
> @@ -2494,6 +2499,10 @@ as hv1 ovs-ofctl dump-flows br-int
>  #Disable router R1
>  ovn-nbctl set Logical_Router R1 enabled=false
>
> +# Allow some time for ovn-northd and ovn-controller to catch up.
> +# XXX This should be more systematic.
> +sleep 1
> +
>  echo "-SB dump-"
>  ovn-sbctl list datapath_binding
>  echo "-"
> --
> 2.1.3

works for me, so...

Acked-by: Ryan Moats 

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

Re: [ovs-dev] [PATCH 4/5] tests: Remove most packet-forwarding related"sleep"s from OVN tests.

[Ryan Moats]

"dev"  wrote on 07/27/2016 02:03:24 AM:

> From: Ben Pfaff 
> To: dev@openvswitch.org
> Cc: Ben Pfaff 
> Date: 07/27/2016 02:04 AM
> Subject: [ovs-dev] [PATCH 4/5] tests: Remove most packet-forwarding
> related "sleep"s from OVN tests.
> Sent by: "dev" 
>
> These arbitrary sleeps are often longer than necessary and can be too
short
> in corner cases.  This commit replaces them by a common macro that waits
> only as long as necessary.
>
> Signed-off-by: Ben Pfaff 
> ---

Much, much, much nicer!

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

Re: [ovs-dev] [PATCH 5/5] ovsdb-idl: Wake up ovsdb_idl_loop when a transaction commits.

[Ryan Moats]

"dev"  wrote on 07/27/2016 02:03:25 AM:

> From: Ben Pfaff 
> To: dev@openvswitch.org
> Cc: Ben Pfaff 
> Date: 07/27/2016 02:04 AM
> Subject: [ovs-dev] [PATCH 5/5] ovsdb-idl: Wake up ovsdb_idl_loop
> when a transaction commits.
> Sent by: "dev" 
>
> There is a fair amount of code that defers modifying the database when a
> transaction cannot be created (because there is already one outstanding).
> This code tends to assume that the main loop will wake up again when it
> becomes possible again to modify the database, but the actual
ovsdb_id_loop
> implementation only did this if the database had changed.  This is too
> conservative a policy and may account for some failures I've seen in
tests.
>
> Signed-off-by: Ben Pfaff 
> ---

Ben, while I can't tell you to hold up on this if somebody else acks it,
my first thought (after looking at it) is that checking it against the
unit tests cases answers only half of the questions that it needs to
answer.
Since it is changing the cycle time, there is the question of whether it
provides a measurable change in E2E performance. Therefore, with your
indulgence, I'm going to try and throw it into an openstack cloud tomorrow
and run some rally tests against it to see if indeed does result in a
measurable change in performance.

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

Re: [ovs-dev] [PATCH] ovn-northd: create patch ports when necessary.

[nickcooper-zhangtonghao]

I only consider the OVN without CMS. Your tips may sound great. 

> On Jul 29, 2016, at 10:48 AM, Ryan Moats  wrote:
> 
> "dev"  wrote on 07/27/2016 09:47:47 PM:
> 
> > From: nickcooper-zhangtonghao 
> > To: Ben Pfaff 
> > Cc: dev@openvswitch.org
> > Date: 07/27/2016 09:48 PM
> > Subject: Re: [ovs-dev] [PATCH] ovn-northd: create patch ports when 
> > necessary.
> > Sent by: "dev" 
> > 
> > 
> > > On Jul 28, 2016, at 5:03 AM, Ben Pfaff  wrote:
> > > 
> > > On Mon, Jul 25, 2016 at 09:23:19AM -0700, nickcooper-zhangtonghao wrote:
> > >> If the logical router ports without 'peer' or the port named peer
> > >> is not created, it is unnecessary to insert the ports into the
> > >> southbound Port_Binding table.
> > >> 
> > >> Similarly, if logical switch ports of type 'router' don't contain
> > 'router-port'
> > >> value, these ports will not be inserted.
> > >> 
> > >> This patch may optimize the process of 'build_ports'.
> > >> 
> > >> Signed-off-by: nickcooper-zhangtonghao  > zhangtong...@opencloud.tech>
> > > 
> > > This doesn't seem like a common case.  Is it worth optimizing?
> > 
> > It seems to me that it’s unnecessary to create the ports unused currently.
> 
> If I think about it, the router port and its peer will be created by the
> CMS.  That means this would optimize for the case where the CMS creates
> these two ports at substantially different times.  I can't think of any
> CMS that works that way today, so I'm with Ben, this doesn't sound like a
> common enough case.
> 

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

Re: [ovs-dev] [PATCH v5 13/16] system-tests: Run conntrack tests with userspace.

[Daniele Di Proietto]

On 27/07/2016 14:18, "Ben Pfaff"  wrote:

>On Wed, Jul 27, 2016 at 01:51:00PM -0700, Jesse Gross wrote:
>> On Wed, Jul 27, 2016 at 1:40 PM, Daniele Di Proietto
>>  wrote:
>> > On 27/07/2016 13:12, "Joe Stringer"  wrote:
>> >
>> >>On 26 July 2016 at 17:58, Daniele Di Proietto  
>> >>wrote:
>> >>> The userspace connection tracker doesn't support ALGs, frag reassembly
>> >>> or NAT yet, so skip those tests.
>> >>>
>> >>> Also, connection tracking state input from a local port is not possible
>> >>> in userspace.
>> >>>
>> >>> The userspace datapath pads all frames with 0, to make them at
>> >>> least 64 bytes.
>> >>>
>> >>> Finally, the userspace datapath checks for the IPv4 header checksum, so
>> >>> fix those in the hardcoded packets.
>> >>>
>> >>> Signed-off-by: Daniele Di Proietto 
>> >>> Acked-by: Joe Stringer 
>> >>> Acked-by: Flavio Leitner 
>> >>> ---
>> >>
>> >>
>> >>
>> >>> @@ -1324,11 +1327,11 @@ dnl UDP packets from ns0->ns1 should solicit 
>> >>> "destination unreachable" response.
>> >>>  NS_CHECK_EXEC([at_ns0], [bash -c "echo a | nc $NC_EOF_OPT -u 10.1.1.2 
>> >>> 1"])
>> >>>
>> >>>  AT_CHECK([ovs-appctl revalidator/purge], [0])
>> >>> -AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort | grep -v 
>> >>> drop], [0], [dnl
>> >>> - n_packets=1, n_bytes=44, priority=100,udp,in_port=1 
>> >>> actions=ct(commit,exec(load:0x1->NXM_NX_CT_MARK[[]])),output:2
>> >>> - n_packets=1, n_bytes=72, 
>> >>> priority=100,ct_state=+rel+trk,ct_mark=0x1,icmp,in_port=2 
>> >>> actions=output:1
>> >>> - n_packets=1, n_bytes=72, priority=100,ct_state=-trk,icmp,in_port=2 
>> >>> actions=ct(table=0)
>> >>> - n_packets=2, n_bytes=84, priority=10,arp actions=NORMAL
>> >>> +AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort | grep -v drop 
>> >>> | sed -e 's/n_bytes=[[0-9]]*/n_bytes=/g'], [0], [dnl
>> >>> + n_packets=1, n_bytes=, priority=100,udp,in_port=1 
>> >>> actions=ct(commit,exec(load:0x1->NXM_NX_CT_MARK[[]])),output:2
>> >>> + n_packets=1, n_bytes=, 
>> >>> priority=100,ct_state=+rel+trk,ct_mark=0x1,icmp,in_port=2 
>> >>> actions=output:1
>> >>> + n_packets=1, n_bytes=, 
>> >>> priority=100,ct_state=-trk,icmp,in_port=2 actions=ct(table=0)
>> >>> + n_packets=2, n_bytes=, priority=10,arp actions=NORMAL
>> >>>  NXST_FLOW reply:
>> >>>  ])
>> >>
>> >>I think this is a completely orthogonal point, but it's still a bit
>> >>surprising to me that the n_bytes would differ when receiving short
>> >>packets in kernel vs. userspace datapaths. I follow that userspace
>> >>pads shorter packets on receive, but shouldn't we be able to attribute
>> >>these stats consistently, regardless of the datapath?
>> >
>> > That's a good point.
>> >
>> > We call dp_packet_pad() in netdev_linux_recv().  That used to be in 
>> > netdev_recv() and can be traced back to the initial OVS commit.  Here's a 
>> > comment in netdev_recv():
>> >
>> > COVERAGE_INC(netdev_received);
>> > buffer->size += n_bytes;
>> >
>> > /* When the kernel internally sends out an Ethernet frame on an
>> >  * interface, it gives us a copy *before* padding the frame to the
>> >  * minimum length.  Thus, when it sends out something like an ARP
>> >  * request, we see a too-short frame.  So pad it out to the minimum
>> >  * length. */
>> > pad_to_minimum_length(buffer);
>> 
>> I wonder if anything in OVS actually cares about this anymore? I don't
>> know the history of that comment.
>
>I don't remember the origin anymore but it was probably my comment.
>It's possible that some code in OVS assumed that packets were at least
>64 bytes long at some point.  For example, flow_extract() might have
>been able to be slightly simplified on the basis that access to the IPv4
>header couldn't be beyond the end of the buffer.
>
>I doubt we do that kind of optimization any longer.

Thanks for the clarifying.  I agree, it doesn't look like it's needed anymore.

I've sent a couple of patches to remove that, along with the connection tracker 
system tests for userspace here:

http://openvswitch.org/pipermail/dev/2016-July/076659.html
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 4/4] system-tests: Add ping through conntrack test.

[Daniele Di Proietto]

Signed-off-by: Daniele Di Proietto 
Acked-by: Joe Stringer 
---
 tests/system-traffic.at | 84 +
 1 file changed, 84 insertions(+)

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 5732d9b..de657e6 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -608,6 +608,90 @@ NS_CHECK_EXEC([at_ns1], [wget http://[[fc00::1]] -t 3 -T 1 
-v -o wget1.log], [4]
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([conntrack - IPv4 ping])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from 
ns1->ns0.
+AT_DATA([flows.txt], [dnl
+priority=1,action=drop
+priority=10,arp,action=normal
+priority=100,in_port=1,icmp,action=ct(commit),2
+priority=100,in_port=2,icmp,ct_state=-trk,action=ct(table=0)
+priority=100,in_port=2,icmp,ct_state=+trk+est,action=1
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl Pings from ns0->ns1 should work fine.
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], 
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
+icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=,type=0,code=0)
+])
+
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
+dnl Pings from ns1->ns0 should fail.
+NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.1 | FORMAT_PING], 
[0], [dnl
+7 packets transmitted, 0 received, 100% packet loss, time 0ms
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([conntrack - IPv6 ping])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "fc00::1/96")
+ADD_VETH(p1, at_ns1, br0, "fc00::2/96")
+
+AT_DATA([flows.txt], [dnl
+
+dnl ICMPv6 echo request and reply go to table 1.  The rest of the traffic goes
+dnl through normal action.
+table=0,priority=10,icmp6,icmp_type=128,action=goto_table:1
+table=0,priority=10,icmp6,icmp_type=129,action=goto_table:1
+table=0,priority=1,action=normal
+
+dnl Allow everything from ns0->ns1. Only allow return traffic from ns1->ns0.
+table=1,priority=100,in_port=1,icmp6,action=ct(commit),2
+table=1,priority=100,in_port=2,icmp6,ct_state=-trk,action=ct(table=0)
+table=1,priority=100,in_port=2,icmp6,ct_state=+trk+est,action=1
+table=1,priority=1,action=drop
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+OVS_WAIT_UNTIL([ip netns exec at_ns0 ping6 -c 1 fc00::2])
+
+dnl Pings from ns1->ns0 should fail.
+NS_CHECK_EXEC([at_ns1], [ping6 -q -c 3 -i 0.3 -w 2 fc00::1 | FORMAT_PING], 
[0], [dnl
+7 packets transmitted, 0 received, 100% packet loss, time 0ms
+])
+
+dnl Pings from ns0->ns1 should work fine.
+NS_CHECK_EXEC([at_ns0], [ping6 -q -c 3 -i 0.3 -w 2 fc00::2 | FORMAT_PING], 
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl
+icmpv6,orig=(src=fc00::1,dst=fc00::2,id=,type=128,code=0),reply=(src=fc00::2,dst=fc00::1,id=,type=129,code=0)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - commit, recirc])
 CHECK_CONNTRACK()
 OVS_TRAFFIC_VSWITCHD_START()
-- 
2.8.1

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

[ovs-dev] [PATCH 2/4] tests: Remove trim_zeros() from ovn tests.

[Daniele Di Proietto]

trim_zeros() is not necessary anymore, since now we don't pad packets in
the userspace datapath.

Signed-off-by: Daniele Di Proietto 
---
 tests/ovn.at | 135 +++
 1 file changed, 43 insertions(+), 92 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index dfdd110..5761981 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -737,9 +737,6 @@ vif_to_hv() {
 # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
 # more) list the VIFs on which the packet should be received.  INPORT and the
 # OUTPORTs are specified as logical switch port numbers, e.g. 11 for vif11.
-trim_zeros() {
-sed 's/\(00\)\{1,\}$//'
-}
 for i in 1 2 3; do
 for j in 1 2 3; do
 : > $i$j.expected
@@ -751,7 +748,7 @@ test_packet() {
 vif=vif$inport
 as $hv ovs-appctl netdev-dummy/receive $vif $packet
 for outport; do
-echo $packet | trim_zeros >> $outport.expected
+echo $packet >> $outport.expected
 done
 }
 
@@ -935,7 +932,7 @@ for i in 1 2 3; do
 for j in 1 2 3; do
 file=hv$i/vif$i$j-tx.pcap
 echo $file
-$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $file | trim_zeros > 
$i$j.packets
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $file > $i$j.packets
 sort $i$j.expected > expout
 AT_CHECK([sort $i$j.packets], [0], [expout])
 echo
@@ -1038,9 +1035,6 @@ vif_to_hv() {
 # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
 # more) list the VIFs on which the packet should be received.  INPORT and the
 # OUTPORTs are specified as logical switch port numbers, e.g. 11 for vif11.
-trim_zeros() {
-sed 's/\(00\)\{1,\}$//'
-}
 for i in 1 2; do
 for j in 1 2 3 4 5; do
 : > $i$j.expected
@@ -1053,7 +1047,7 @@ test_packet() {
 vif=vif$inport
 as $hv ovs-appctl netdev-dummy/receive $vif $packet
 for outport; do
-echo $packet | trim_zeros >> $outport.expected
+echo $packet >> $outport.expected
 done
 }
 
@@ -1120,7 +1114,7 @@ for i in 1 2; do
 for j in 1 2 3 4 5; do
 file=hv$i/vif$i$j-tx.pcap
 echo $file
-$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $file | trim_zeros > 
$i$j.packets
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $file > $i$j.packets
 sort $i$j.expected > expout
 AT_CHECK([sort $i$j.packets], [0], [expout])
 echo
@@ -1223,9 +1217,6 @@ sleep 1
 # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
 # more) list the VIFs on which the packet should be received.  INPORT and the
 # OUTPORTs are specified as logical switch port numbers, e.g. 1 for vif1.
-trim_zeros() {
-sed 's/\(00\)\{1,\}$//'
-}
 for i in 1 2 3; do
 : > $i.expected
 done
@@ -1236,7 +1227,7 @@ test_packet() {
 vif=vif$inport
 as $hv ovs-appctl netdev-dummy/receive $vif $packet
 for outport; do
-echo $packet | trim_zeros >> $outport.expected
+echo $packet >> $outport.expected
 done
 }
 
@@ -1302,7 +1293,7 @@ AT_CHECK([as hv3 ovs-ofctl -O OpenFlow13 show br-int], 
[1], [],
 for i in 1 2 3; do
 file=hv$i/vif$i-tx.pcap
 echo $file
-$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $file | trim_zeros > $i.packets
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $file > $i.packets
 sort $i.expected > expout
 AT_CHECK([sort $i.packets], [0], [expout])
 echo
@@ -1384,9 +1375,6 @@ sleep 1
 # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
 # more) list the VIFs on which the packet should be received.  INPORT and the
 # OUTPORTs are specified as lport numbers, e.g. 1 for vif1.
-trim_zeros() {
-sed 's/\(00\)\{1,\}$//'
-}
 for i in 1 2 3; do
 : > $i.expected
 done
@@ -1397,7 +1385,7 @@ test_packet() {
 vif=vif$inport
 as $hv ovs-appctl netdev-dummy/receive $vif $packet
 for outport; do
-echo $packet | trim_zeros >> $outport.expected
+echo $packet >> $outport.expected
 done
 }
 
@@ -1468,7 +1456,7 @@ as hv3 ovs-ofctl -O OpenFlow13 dump-flows br-phys
 for i in 1 2 3; do
 file=hv$i/vif$i-tx.pcap
 echo $file
-$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $file | trim_zeros > $i.packets
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" $file > $i.packets
 sort $i.expected > expout
 AT_CHECK([sort $i.packets], [0], [expout])
 echo
@@ -1593,9 +1581,6 @@ sleep 1
 # digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
 # more) list the VIFs on which the packet should be received.  INPORT and the
 # OUTPORTs are specified as logical switch port numbers, e.g. 123 for vif123.
-trim_zeros() {
-sed 's/\(00\)\{1,\}$//'
-}
 for i in 1 2 3; do
 for j in 1 2 3; do
 for k in 1 2 3; do
@@ -1623,7 +1608,7 @@ test_ip() {
 # (and checksum).
 out_lrp=`vif_to_lrp $outport`
 echo 

[ovs-dev] [PATCH 3/4] system-tests: Run conntrack tests with userspace.

[Daniele Di Proietto]

The userspace connection tracker doesn't support ALGs, frag reassembly
or NAT yet, so skip those tests.

Also, connection tracking state input from a local port is not possible
in userspace.

Finally, the userspace datapath checks for the IPv4 header checksum, so
fix those in the hardcoded packets.

Signed-off-by: Daniele Di Proietto 
Acked-by: Joe Stringer 
Acked-by: Flavio Leitner 
---
 tests/system-kmod-macros.at  | 28 +
 tests/system-ovn.at  |  3 +++
 tests/system-traffic.at  | 32 +---
 tests/system-userspace-macros.at | 45 +---
 4 files changed, 102 insertions(+), 6 deletions(-)

diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index 2134db7..e1b5707 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -67,3 +67,31 @@ m4_define([CHECK_CONNTRACK],
  on_exit 'ovstest test-netlink-conntrack flush'
 ]
 )
+
+# CHECK_CONNTRACK_ALG()
+#
+# Perform requirements checks for running conntrack ALG tests. The kernel
+# supports ALG, so no check is needed.
+#
+m4_define([CHECK_CONNTRACK_ALG])
+
+# CHECK_CONNTRACK_FRAG()
+#
+# Perform requirements checks for running conntrack fragmentations tests.
+# The kernel always supports fragmentation, so no check is needed.
+m4_define([CHECK_CONNTRACK_FRAG])
+
+# CHECK_CONNTRACK_LOCAL_STACK()
+#
+# Perform requirements checks for running conntrack tests with local stack.
+# The kernel always supports reading the connection state of an skb coming
+# from an internal port, without an explicit ct() action, so no check is
+# needed.
+m4_define([CHECK_CONNTRACK_LOCAL_STACK])
+
+# CHECK_CONNTRACK_NAT()
+#
+# Perform requirements checks for running conntrack NAT tests. The kernel
+# always supports NAT, so no check is needed.
+#
+m4_define([CHECK_CONNTRACK_NAT])
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 2a94d68..b96b260 100755
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -2,6 +2,7 @@ AT_SETUP([ovn -- 2 LRs connected via LS, gateway router, SNAT 
and DNAT])
 AT_KEYWORDS([ovnnat])
 
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
 ovn_start
 OVS_TRAFFIC_VSWITCHD_START()
 ADD_BR([br-int])
@@ -172,6 +173,7 @@ AT_SETUP([ovn -- 2 LRs connected via LS, gateway router, 
easy SNAT])
 AT_KEYWORDS([ovnnat])
 
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
 ovn_start
 OVS_TRAFFIC_VSWITCHD_START()
 ADD_BR([br-int])
@@ -277,6 +279,7 @@ AT_SETUP([ovn -- load-balancing])
 AT_KEYWORDS([ovnlb])
 
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
 ovn_start
 OVS_TRAFFIC_VSWITCHD_START()
 ADD_BR([br-int])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 1cdc2d2..5732d9b 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -510,13 +510,13 @@ AT_CAPTURE_FILE([ofctl_monitor.log])
 AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir 
--pidfile 2> ofctl_monitor.log])
 
 dnl Send an unsolicited reply from port 2. This should be dropped.
-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 ct\(table=0\) 
'5054000a505400090800451c00110a0101020a010101000200010008'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 ct\(table=0\) 
'5054000a505400090800451c0011a4cd0a0101020a010101000200010008'])
 
 dnl OK, now start a new connection from port 1.
-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 1 ct\(commit\),controller 
'5054000a505400090800451c00110a0101010a010102000100020008'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 1 ct\(commit\),controller 
'5054000a505400090800451c0011a4cd0a0101010a010102000100020008'])
 
 dnl Now try a reply from port 2.
-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 ct\(table=0\) 
'5054000a505400090800451c00110a0101020a010101000200010008'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 ct\(table=0\) 
'5054000a505400090800451c0011a4cd0a0101020a010101000200010008'])
 
 dnl Check this output. We only see the latter two packets, not the first.
 AT_CHECK([cat ofctl_monitor.log], [0], [dnl
@@ -906,6 +906,7 @@ AT_CLEANUP
 
 AT_SETUP([conntrack - multiple zones, local])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_LOCAL_STACK()
 OVS_TRAFFIC_VSWITCHD_START()
 
 ADD_NAMESPACES(at_ns0)
@@ -953,6 +954,7 @@ AT_CLEANUP
 
 AT_SETUP([conntrack - multiple namespaces, internal ports])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_LOCAL_STACK()
 OVS_TRAFFIC_VSWITCHD_START(
[set-fail-mode br0 secure -- ])
 
@@ -993,6 +995,7 @@ AT_CLEANUP
 
 AT_SETUP([conntrack - multi-stage pipeline, local])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_LOCAL_STACK()
 OVS_TRAFFIC_VSWITCHD_START()
 
 ADD_NAMESPACES(at_ns0)
@@ -1382,6 +1385,7 @@ AT_CLEANUP
 AT_SETUP([conntrack - FTP])
 AT_SKIP_IF([test $HAVE_PYFTPDLIB = no])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_ALG()
 OVS_TRAFFIC_VSWITCHD_START()

Re: [ovs-dev] [PATCH 1/5] tests: Define trim_zeros in only one place.

[Ryan Moats]

"dev"  wrote on 07/27/2016 02:03:21 AM:

> From: Ben Pfaff 
> To: dev@openvswitch.org
> Cc: Ben Pfaff 
> Date: 07/27/2016 02:03 AM
> Subject: [ovs-dev] [PATCH 1/5] tests: Define trim_zeros in only one
place.
> Sent by: "dev" 
>
> Defining trim_zeros in a common place allows us to skip defining it in
> every test that needs it.
>
> Signed-off-by: Ben Pfaff 
> ---

cleaning up code is always a good thing and tests pass for me
after applying it, so ...

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

Re: [ovs-dev] [PATCH] ovn-northd: create patch ports when necessary.

[Ryan Moats]

"dev"  wrote on 07/27/2016 09:47:47 PM:

> From: nickcooper-zhangtonghao 
> To: Ben Pfaff 
> Cc: dev@openvswitch.org
> Date: 07/27/2016 09:48 PM
> Subject: Re: [ovs-dev] [PATCH] ovn-northd: create patch ports when
necessary.
> Sent by: "dev" 
>
>
> > On Jul 28, 2016, at 5:03 AM, Ben Pfaff  wrote:
> >
> > On Mon, Jul 25, 2016 at 09:23:19AM -0700, nickcooper-zhangtonghao
wrote:
> >> If the logical router ports without 'peer' or the port named peer
> >> is not created, it is unnecessary to insert the ports into the
> >> southbound Port_Binding table.
> >>
> >> Similarly, if logical switch ports of type 'router' don't contain
> 'router-port'
> >> value, these ports will not be inserted.
> >>
> >> This patch may optimize the process of 'build_ports'.
> >>
> >> Signed-off-by: nickcooper-zhangtonghao  zhangtong...@opencloud.tech>
> >
> > This doesn't seem like a common case.  Is it worth optimizing?
>
> It seems to me that it’s unnecessary to create the ports unused
currently.

If I think about it, the router port and its peer will be created by the
CMS.  That means this would optimize for the case where the CMS creates
these two ports at substantially different times.  I can't think of any
CMS that works that way today, so I'm with Ben, this doesn't sound like a
common enough case.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] HI

[Automatic Email Delivery Software]

Dear user of openvswitch.org,

Your e-mail account has been used to send a large amount of spam during the 
last week.
Probably, your computer was compromised and now contains a hidden proxy server.

We recommend that you follow our instruction in order to keep your computer 
safe.

Have a nice day,
The openvswitch.org support team.

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

Re: [ovs-dev] [PATCH] ovn-controller: Remove old values from local_ids.

[Russell Bryant]

On Thu, Jul 28, 2016 at 6:13 PM, Ryan Moats  wrote:

> "dev"  wrote on 07/28/2016 04:22:41 PM:
>
> > From: Russell Bryant 
> > To: dev@openvswitch.org
> > Date: 07/28/2016 04:23 PM
> > Subject: [ovs-dev] [PATCH] ovn-controller: Remove old values from
> local_ids.
> > Sent by: "dev" 
> >
> > local_ids is supposed to be the set of interface iface-id values from
> > this chassis that correspond to OVN logical ports.  We use this for
> > detecting when an interface has been removed as well as if child-ports
> > should be bound to this chassis.
> >
> > Old values were not being removed from local_ids.  The most immediate
> > effect of this was that once an interface has been removed from a
> > chassis, we would think a removal has occured *every* time through
> > binding_run and trigger the full binding processing.  This was
> > a performance problem.
> >
> > The second problem this would cause is if a port that had child ports
> > was moved to another chassis.  We would end up with two chassis fighting
> > over the binding of the child ports.
> >
> > Signed-off-by: Russell Bryant 
> > ---
> >  ovn/controller/binding.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> > index 78ebec4..41165bc 100644
> > --- a/ovn/controller/binding.c
> > +++ b/ovn/controller/binding.c
> > @@ -103,6 +103,11 @@ get_local_iface_ids(const struct ovsrec_bridge
> *br_int,
> >   * that has been removed. */
> >  if (!changed && !sset_is_empty(_local_ids)) {
> >  changed = true;
> > +
> > +const char *cur_id;
> > +SSET_FOR_EACH(cur_id, _local_ids) {
> > +sset_find_and_delete(_ids, cur_id);
> > +}
> >  }
> >
> >  sset_destroy(_local_ids);
> > --
> > 2.7.4
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
> lgtm...
>
> Acked-by: Ryan Moats 
>
> Thanks, I applied this to master.

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

Re: [ovs-dev] [PATCH 1/2] ovn:add easy SNAT test case

[Dong Jun]

Thanks, it's really more realistic.

On 2016/7/28 22:53, Guru Shetty wrote:



On 28 July 2016 at 07:48, Guru Shetty > wrote:




On 28 July 2016 at 06:55, Dong Jun > wrote:

Yes, this test case fail currently and success with the
modification.
If there is another same test case, ignore this patch is OK.


Thank you for the test. I applied the linked patch by Chandra as
it was already reviewed and applied your unit test and also added
you in AUTHORS. I did look at your second patch of the series.
Please read CodingStyle.md for any future contributions.


I forgot to mention that I did add the following incremental to your 
test to make the test case more realistic.
(Also, we still have a bug wherein we allow the router to respond to 
ICMP messages for a SNAT IP. This can be racy.)



@@ -243,19 +243,19 @@ ovn-nbctl lsp-add alice alice1 \

 # Add a SNAT rule
 ovn-nbctl -- --id=@nat create nat type="snat" logical_ip=192.168.1.2 \
-external_ip=20.0.0.2 -- add logical_router R2 nat @nat
+external_ip=172.16.1.1 -- add logical_router R2 nat @nat

 # South-North SNAT: 'foo1' pings 'alice1'. But 'alice1' receives traffic
-# from 20.0.0.2
+# from 172.16.1.1
 NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | 
FORMAT_PING], \

 [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])

 # We verify that SNAT indeed happened via 'dump-conntrack' command.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(20.0.0.2) | \
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
 sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
-icmp,orig=(src=192.168.1.2,dst=172.16.1.2,id=),reply=(src=172.16.1.2,dst=20.0.0.2,id=),zon
+icmp,orig=(src=192.168.1.2,dst=172.16.1.2,id=),reply=(src=172.16.1.2,dst=172.16.1.1,id=),z
 ])


On 2016/7/28 21:31, Guru Shetty wrote:



On 27 July 2016 at 23:30, Dongjun  >> wrote:

Signed-off-by: Dongjun  >>


Usually, we have a test with the corresponding code
change. Can you have a look at
https://www.mail-archive.com/dev@openvswitch.org/msg66099.html
and see whether you are happy with the code change?

---
 tests/system-ovn.at 
 | 106
+++-
 1 file changed, 105 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 tests/system-ovn.at



diff --git a/tests/system-ovn.at
 
b/tests/system-ovn.at 

old mode 100644
new mode 100755
index 13f380f..cb50fd4
--- a/tests/system-ovn.at 

+++ b/tests/system-ovn.at 


@@ -1,4 +1,4 @@
-AT_SETUP([ovn -- 2 LRs connected via LS, gateway
router, NAT])
+AT_SETUP([ovn -- 2 LRs connected via LS, gateway
router, SNAT and
DNAT])
 AT_KEYWORDS([ovnnat])

 CHECK_CONNTRACK()
@@ -168,6 +168,110 @@ as
 OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
patch-.*/d"])
 AT_CLEANUP

+AT_SETUP([ovn -- 2 LRs connected via LS, gateway
router, easy SNAT])
+AT_KEYWORDS([ovnnat])
+
+CHECK_CONNTRACK()
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+-- set Open_vSwitch .
external-ids:system-id=hv1 \
+-- set Open_vSwitch .
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+-- set Open_vSwitch .
external-ids:ovn-encap-type=geneve \
+-- set Open_vSwitch .
external-ids:ovn-encap-ip=169.0.0.1 \
+-- set bridge br-int fail-mode=secure
other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+
+# 

Re: [ovs-dev] [PATCH 01/13] lib: Separate versioning to its own module.

[Jarno Rajahalme]

> On Jul 26, 2016, at 11:08 AM, Ben Pfaff  wrote:
> 
> On Fri, Jul 15, 2016 at 03:19:07AM -0700, Jarno Rajahalme wrote:
>> Separate rule versioning to lib/versions.h to make it easier to use
>> versioning for other data types.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Doesn't add versions.h, can't build.

Sorry for the trouble! I just sent out v2.

Thanks,

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

[ovs-dev] [PATCH v2 26/26] ofproto: Reduce bundle memory use.

[Jarno Rajahalme]

Instead of storing the (big) struct ofputil_flow_mod, create the new
rule and/or create the rule criteria for matching at bundle message
insert time.  These can be uninitialized right after the start phase
during the commit, which may also reduce the total memory needed
during the bundle commit.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/bundles.c  |   2 +-
 ofproto/bundles.h  |   4 +-
 ofproto/ofproto-provider.h |  43 +++-
 ofproto/ofproto.c  | 570 +
 tests/ofproto.at   |   4 -
 5 files changed, 414 insertions(+), 209 deletions(-)

diff --git a/ofproto/bundles.c b/ofproto/bundles.c
index aa8e58c..e8fb7ba 100644
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -66,7 +66,7 @@ ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle 
*bundle,
 LIST_FOR_EACH_POP (msg, node, >msg_list) {
 if (success && msg->type == OFPTYPE_FLOW_MOD) {
 /* Tell connmgr about successful flow mods. */
-ofconn_report_flow_mod(ofconn, msg->ofm.fm.command);
+ofconn_report_flow_mod(ofconn, msg->ofm.command);
 }
 ofp_bundle_entry_free(msg);
 }
diff --git a/ofproto/bundles.h b/ofproto/bundles.h
index 2054303..802de77 100644
--- a/ofproto/bundles.h
+++ b/ofproto/bundles.h
@@ -36,7 +36,7 @@ struct ofp_bundle_entry {
 enum ofptype  type;  /* OFPTYPE_FLOW_MOD, OFPTYPE_PORT_MOD, or
   * OFPTYPE_GROUP_MOD. */
 union {
-struct ofproto_flow_mod ofm;   /* ofm.fm.ofpacts must be malloced. */
+struct ofproto_flow_mod ofm;
 struct ofproto_port_mod opm;
 struct ofproto_group_mod ogm;
 };
@@ -90,7 +90,7 @@ ofp_bundle_entry_free(struct ofp_bundle_entry *entry)
 {
 if (entry) {
 if (entry->type == OFPTYPE_FLOW_MOD) {
-free(entry->ofm.fm.ofpacts);
+ofproto_flow_mod_uninit(>ofm);
 } else if (entry->type == OFPTYPE_GROUP_MOD) {
 ofputil_uninit_group_mod(>ogm.gm);
 }
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 35ce4f4..fce23da 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1904,25 +1904,64 @@ extern const struct ofproto_class ofproto_dpif_class;
 int ofproto_class_register(const struct ofproto_class *);
 int ofproto_class_unregister(const struct ofproto_class *);
 
+/* Criteria that flow_mod and other operations use for selecting rules on
+ * which to operate. */
+struct rule_criteria {
+/* An OpenFlow table or 255 for all tables. */
+uint8_t table_id;
+
+/* OpenFlow matching criteria.  Interpreted different in "loose" way by
+ * collect_rules_loose() and "strict" way by collect_rules_strict(), as
+ * defined in the OpenFlow spec. */
+struct cls_rule cr;
+ovs_version_t version;
+
+/* Matching criteria for the OpenFlow cookie.  Consider a bit B in a rule's
+ * cookie and the corresponding bits C in 'cookie' and M in 'cookie_mask'.
+ * The rule will not be selected if M is 1 and B != C.  */
+ovs_be64 cookie;
+ovs_be64 cookie_mask;
+
+/* Selection based on actions within a rule:
+ *
+ * If out_port != OFPP_ANY, selects only rules that output to out_port.
+ * If out_group != OFPG_ALL, select only rules that output to out_group. */
+ofp_port_t out_port;
+uint32_t out_group;
+
+/* If true, collects only rules that are modifiable. */
+bool include_hidden;
+bool include_readonly;
+};
+
 /* flow_mod with execution context. */
 struct ofproto_flow_mod {
-struct ofputil_flow_mod fm;
+/* Allocated by 'init' phase, may be freed after 'start' phase, as these
+ * are not needed for 'revert' nor 'finish'. */
+struct rule *temp_rule;
+struct rule_criteria criteria;
+struct cls_conjunction *conjs;
+size_t n_conjs;
 
 /* Replicate needed fields from ofputil_flow_mod to not need it after the
  * flow has been created. */
 uint16_t command;
 uint32_t buffer_id;
 bool modify_cookie;
-
+/* Fields derived from ofputil_flow_mod. */
 bool modify_may_add_flow;
 enum nx_flow_update_event event;
 
+/* These are only used during commit execution.
+ * ofproto_flow_mod_uninit() does NOT clean these up. */
 ovs_version_t version;  /* Version in which changes take
  * effect. */
 struct rule_collection old_rules;   /* Affected rules. */
 struct rule_collection new_rules;   /* Replacement rules. */
 };
 
+void ofproto_flow_mod_uninit(struct ofproto_flow_mod *);
+
 /* port_mod with execution context. */
 struct ofproto_port_mod {
 struct ofputil_port_mod pm;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b8b6bc2..f7286b5 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -129,36 +129,6 @@ static void eviction_group_add_rule(struct rule *)
 static void eviction_group_remove_rule(struct rule *)
  

[ovs-dev] [PATCH v2 25/26] ofproto: Add 'command' to ofproto_flow_mod.

[Jarno Rajahalme]

This helps releasing ofputil_flow_mod earlier in a later patch.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-provider.h | 1 +
 ofproto/ofproto.c  | 9 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index f328b4a..35ce4f4 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1910,6 +1910,7 @@ struct ofproto_flow_mod {
 
 /* Replicate needed fields from ofputil_flow_mod to not need it after the
  * flow has been created. */
+uint16_t command;
 uint32_t buffer_id;
 bool modify_cookie;
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 47020b8..b8b6bc2 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -7140,13 +7140,14 @@ ofproto_flow_mod_start(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm)
 }
 
 /* Forward flow mod fields we need later. */
+ofm->command = ofm->fm.command;
 ofm->buffer_id = ofm->fm.buffer_id;
 ofm->modify_cookie = ofm->fm.modify_cookie;
 
 ofm->modify_may_add_flow = (ofm->fm.new_cookie != OVS_BE64_MAX
 && ofm->fm.cookie_mask == htonll(0));
 
-switch (ofm->fm.command) {
+switch (ofm->command) {
 case OFPFC_ADD:
 ofm->event = NXFME_ADDED;
 return add_flow_start(ofproto, ofm);
@@ -7173,7 +7174,7 @@ static void
 ofproto_flow_mod_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
 OVS_REQUIRES(ofproto_mutex)
 {
-switch (ofm->fm.command) {
+switch (ofm->command) {
 case OFPFC_ADD:
 add_flow_revert(ofproto, ofm);
 break;
@@ -7199,7 +7200,7 @@ ofproto_flow_mod_finish(struct ofproto *ofproto,
 const struct openflow_mod_requester *req)
 OVS_REQUIRES(ofproto_mutex)
 {
-switch (ofm->fm.command) {
+switch (ofm->command) {
 case OFPFC_ADD:
 add_flow_finish(ofproto, ofm, req);
 break;
@@ -7219,7 +7220,7 @@ ofproto_flow_mod_finish(struct ofproto *ofproto,
 }
 
 if (req) {
-ofconn_report_flow_mod(req->ofconn, ofm->fm.command);
+ofconn_report_flow_mod(req->ofconn, ofm->command);
 }
 }
 
-- 
2.1.4

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

[ovs-dev] [PATCH v2 24/26] ofproto: Add 'modify_cookie' to ofproto_flow_mod.

[Jarno Rajahalme]

ofproto internally modifies 'modify_cookie' field, and adding a
replica to ofproto_flow_mod allows the ofputil_flow_mod argument to be
changed to a const.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-dpif.c | 27 +--
 ofproto/ofproto-provider.h |  3 ++-
 ofproto/ofproto.c  | 36 +++-
 3 files changed, 30 insertions(+), 36 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index aa933b1..fd2c2bd 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -362,16 +362,7 @@ void
 ofproto_dpif_flow_mod(struct ofproto_dpif *ofproto,
   const struct ofputil_flow_mod *fm)
 {
-struct ofproto_flow_mod ofm;
-
-/* Multiple threads may do this for the same 'fm' at the same time.
- * Allocate ofproto_flow_mod with execution context from stack.
- *
- * Note: This copy could be avoided by making ofproto_flow_mod more
- * complex, but that may not be desireable, and a learn action is not that
- * fast to begin with. */
-ofm.fm = *fm;
-ofproto_flow_mod(>up, );
+ofproto_flow_mod(>up, fm);
 }
 
 /* Appends 'am' to the queue of asynchronous messages to be sent to the
@@ -5546,11 +5537,11 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif 
*ofproto,
const struct ofpbuf *ofpacts,
struct rule **rulep)
 {
-struct ofproto_flow_mod ofm;
+struct ofputil_flow_mod fm;
 struct rule_dpif *rule;
 int error;
 
-ofm.fm = (struct ofputil_flow_mod) {
+fm = (struct ofputil_flow_mod) {
 .match = *match,
 .priority = priority,
 .table_id = TBL_INTERNAL,
@@ -5561,7 +5552,7 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif 
*ofproto,
 .ofpacts_len = ofpacts->size,
 };
 
-error = ofproto_flow_mod(>up, );
+error = ofproto_flow_mod(>up, );
 if (error) {
 VLOG_ERR_RL(, "failed to add internal flow (%s)",
 ofperr_to_string(error));
@@ -5571,8 +5562,8 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif 
*ofproto,
 
 rule = rule_dpif_lookup_in_table(ofproto,
  ofproto_dpif_get_tables_version(ofproto),
- TBL_INTERNAL, ,
- );
+ TBL_INTERNAL, ,
+ );
 if (rule) {
 *rulep = >up;
 } else {
@@ -5585,10 +5576,10 @@ int
 ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto,
   struct match *match, int priority)
 {
-struct ofproto_flow_mod ofm;
+struct ofputil_flow_mod fm;
 int error;
 
-ofm.fm = (struct ofputil_flow_mod) {
+fm = (struct ofputil_flow_mod) {
 .match = *match,
 .priority = priority,
 .table_id = TBL_INTERNAL,
@@ -5596,7 +5587,7 @@ ofproto_dpif_delete_internal_flow(struct ofproto_dpif 
*ofproto,
 .command = OFPFC_DELETE_STRICT,
 };
 
-error = ofproto_flow_mod(>up, );
+error = ofproto_flow_mod(>up, );
 if (error) {
 VLOG_ERR_RL(, "failed to delete internal flow (%s)",
 ofperr_to_string(error));
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index fb5fc95..f328b4a 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1911,6 +1911,7 @@ struct ofproto_flow_mod {
 /* Replicate needed fields from ofputil_flow_mod to not need it after the
  * flow has been created. */
 uint32_t buffer_id;
+bool modify_cookie;
 
 bool modify_may_add_flow;
 enum nx_flow_update_event event;
@@ -1937,7 +1938,7 @@ struct ofproto_group_mod {
 struct group_collection old_groups; /* Affected groups. */
 };
 
-enum ofperr ofproto_flow_mod(struct ofproto *, struct ofproto_flow_mod *)
+enum ofperr ofproto_flow_mod(struct ofproto *, const struct ofputil_flow_mod *)
 OVS_EXCLUDED(ofproto_mutex);
 void ofproto_add_flow(struct ofproto *, const struct match *, int priority,
   const struct ofpact *ofpacts, size_t ofpacts_len)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 853b2a3..47020b8 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -305,7 +305,7 @@ static void ofproto_flow_mod_finish(struct ofproto *,
 const struct openflow_mod_requester *)
 OVS_REQUIRES(ofproto_mutex);
 static enum ofperr handle_flow_mod__(struct ofproto *,
- struct ofproto_flow_mod *,
+ const struct ofputil_flow_mod *,
  const struct openflow_mod_requester *)
 OVS_EXCLUDED(ofproto_mutex);
 static void calc_duration(long long int start, long long int now,
@@ -2123,11 +2123,11 @@ simple_flow_mod(struct ofproto *ofproto,
 const struct ofpact *ofpacts, size_t 

[ovs-dev] [PATCH v2 23/26] ofproto: Reduce dependency on ofputil_flow_mod after rule has been created.

[Jarno Rajahalme]

One step towards the goal of removing the ofputil_flow_mod from the
bundle message.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-provider.h |  7 +
 ofproto/ofproto.c  | 68 ++
 2 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 0610e78..fb5fc95 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1908,6 +1908,13 @@ int ofproto_class_unregister(const struct ofproto_class 
*);
 struct ofproto_flow_mod {
 struct ofputil_flow_mod fm;
 
+/* Replicate needed fields from ofputil_flow_mod to not need it after the
+ * flow has been created. */
+uint32_t buffer_id;
+
+bool modify_may_add_flow;
+enum nx_flow_update_event event;
+
 ovs_version_t version;  /* Version in which changes take
  * effect. */
 struct rule_collection old_rules;   /* Affected rules. */
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index f467125..853b2a3 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -265,7 +265,8 @@ struct openflow_mod_requester {
 
 /* OpenFlow. */
 static enum ofperr replace_rule_create(struct ofproto *,
-   struct ofputil_flow_mod *,
+   struct ofproto_flow_mod *,
+   const struct ofputil_flow_mod *,
struct cls_rule *cr, uint8_t table_id,
struct rule *old_rule,
struct rule **new_rule)
@@ -280,7 +281,7 @@ static void replace_rule_revert(struct ofproto *, struct 
rule *old_rule,
 struct rule *new_rule)
 OVS_REQUIRES(ofproto_mutex);
 
-static void replace_rule_finish(struct ofproto *, struct ofputil_flow_mod *,
+static void replace_rule_finish(struct ofproto *, struct ofproto_flow_mod *,
 const struct openflow_mod_requester *,
 struct rule *old_rule, struct rule *new_rule,
 struct ovs_list *dead_cookies)
@@ -4873,7 +4874,7 @@ add_flow_start(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm)
 }
 
 /* Allocate new rule. */
-error = replace_rule_create(ofproto, fm, , table - ofproto->tables,
+error = replace_rule_create(ofproto, ofm, fm, , table - ofproto->tables,
 rule, new_rule);
 if (error) {
 return error;
@@ -4908,12 +4909,11 @@ add_flow_finish(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm,
 const struct openflow_mod_requester *req)
 OVS_REQUIRES(ofproto_mutex)
 {
-struct ofputil_flow_mod *fm = >fm;
 struct rule *old_rule = rule_collection_stub(>old_rules)[0];
 struct rule *new_rule = rule_collection_stub(>new_rules)[0];
 struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(_cookies);
 
-replace_rule_finish(ofproto, fm, req, old_rule, new_rule, _cookies);
+replace_rule_finish(ofproto, ofm, req, old_rule, new_rule, _cookies);
 learned_cookies_flush(ofproto, _cookies);
 
 if (old_rule) {
@@ -4927,7 +4927,7 @@ add_flow_finish(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm,
 send_table_status(ofproto, new_rule->table_id);
 }
 
-send_buffered_packet(req, fm->buffer_id, new_rule);
+send_buffered_packet(req, ofm->buffer_id, new_rule);
 }
 
 /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
@@ -4936,7 +4936,8 @@ add_flow_finish(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm,
  * and 'old_rule'.  Note that the rule is NOT inserted into a any data
  * structures yet.  Takes ownership of 'cr'. */
 static enum ofperr
-replace_rule_create(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
+replace_rule_create(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
+const struct ofputil_flow_mod *fm,
 struct cls_rule *cr, uint8_t table_id,
 struct rule *old_rule, struct rule **new_rule)
 {
@@ -5065,7 +5066,7 @@ replace_rule_revert(struct ofproto *ofproto,
 
 /* Adds the 'new_rule', replacing the 'old_rule'. */
 static void
-replace_rule_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
+replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
 const struct openflow_mod_requester *req,
 struct rule *old_rule, struct rule *new_rule,
 struct ovs_list *dead_cookies)
@@ -5088,27 +5089,25 @@ replace_rule_finish(struct ofproto *ofproto, struct 
ofputil_flow_mod *fm,
 
 if (old_rule) {
 const struct rule_actions *old_actions = rule_get_actions(old_rule);
+const struct rule_actions *new_actions = rule_get_actions(new_rule);
 
 learned_cookies_dec(ofproto, old_actions, dead_cookies);
 
   

[ovs-dev] [PATCH v2 21/26] ofproto: Support group mods in bundles.

[Jarno Rajahalme]

Allow adding group mods in OpenFlow bundles.  Group mods are executed
atomically with any flow mods in the same bundle.  Mods are executed
in order, so that groups appearing in flow actions need to be inserted
in to the bundle before the dependent flow mods.

ovs-ofctl is enhanced to allow the '--bundle' option with group mod
commands.  add-groups file format is enhanced to allow each line to be
preceded by one of the keywords "add", "modify", "delete",
"add_or_mod", "insert_bucket", or "remove_bucket".

ovs-ofctl also has a new "bundle" command that reads a file in which
each line contains one flow mod or group mod, and then executes them
all as a single atomic bundle transaction.

Signed-off-by: Jarno Rajahalme 
---
 NEWS|   7 +-
 include/openvswitch/ofp-parse.h |  10 +-
 include/openvswitch/ofp-util.h  |  16 +-
 lib/ofp-parse.c | 152 ++-
 lib/ofp-util.c  |  33 +++-
 ofproto/bundles.h   |   8 +-
 ofproto/ofproto.c   | 149 ++
 tests/ofproto-macros.at |   2 +
 tests/ofproto.at| 418 +++-
 utilities/ovs-ofctl.8.in|  56 --
 utilities/ovs-ofctl.c   |  95 -
 11 files changed, 871 insertions(+), 75 deletions(-)

diff --git a/NEWS b/NEWS
index 6f6b9a3..8221505 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@ Post-v2.5.0
  * New "monitor_cond" "monitor_cond_update" and "update2" extensions to
RFC 7047.
- OpenFlow:
+ * OpenFlow 1.3+ bundles are now supported for group mods as well as
+   flow mods and port mods.  Both 'atomic' and 'ordered' bundle
+   flags are supported for group mods as well as flow mods.
  * OpenFlow 1.1+ OFPT_QUEUE_GET_CONFIG_REQUEST now supports OFPP_ANY.
  * OpenFlow 1.4+ OFPMP_QUEUE_DESC is now supported.
  * OpenFlow 1.4+ OFPT_TABLE_STATUS is now supported.
@@ -27,7 +30,9 @@ Post-v2.5.0
properly translated to OpenFlow 1.0.
- ovs-ofctl:
  * queue-get-config command now allows a queue ID to be specified.
- * '--bundle' option can now be used with OpenFlow 1.3.
+ * '--bundle' option can now be used with OpenFlow 1.3 and with group mods.
+ * New "bundle" command allows executing a mixture of flow and group mods
+   as a single atomic transaction.
  * New option "--color" to produce colorized output for some commands.
  * New option '--may-create' to use OFPGC_ADD_OR_MOD in mod-group command.
- IPFIX:
diff --git a/include/openvswitch/ofp-parse.h b/include/openvswitch/ofp-parse.h
index a460d8a..df60b18 100644
--- a/include/openvswitch/ofp-parse.h
+++ b/include/openvswitch/ofp-parse.h
@@ -33,6 +33,7 @@ struct ofputil_flow_stats_request;
 struct ofputil_group_mod;
 struct ofputil_meter_mod;
 struct ofputil_table_mod;
+struct ofputil_bundle_msg;
 struct ofputil_tlv_table_mod;
 struct simap;
 enum ofputil_protocol;
@@ -74,16 +75,21 @@ char *parse_flow_monitor_request(struct 
ofputil_flow_monitor_request *,
  enum ofputil_protocol *usable_protocols)
 OVS_WARN_UNUSED_RESULT;
 
-char *parse_ofp_group_mod_file(const char *file_name, uint16_t command,
+char *parse_ofp_group_mod_file(const char *file_name, int command,
struct ofputil_group_mod **gms, size_t *n_gms,
enum ofputil_protocol *usable_protocols)
 OVS_WARN_UNUSED_RESULT;
 
-char *parse_ofp_group_mod_str(struct ofputil_group_mod *, uint16_t command,
+char *parse_ofp_group_mod_str(struct ofputil_group_mod *, int command,
   const char *string,
   enum ofputil_protocol *usable_protocols)
 OVS_WARN_UNUSED_RESULT;
 
+char *parse_ofp_bundle_file(const char *file_name,
+struct ofputil_bundle_msg **, size_t *n_bms,
+enum ofputil_protocol *)
+OVS_WARN_UNUSED_RESULT;
+
 char *parse_ofp_tlv_table_mod_str(struct ofputil_tlv_table_mod *,
  uint16_t command, const char *string,
  enum ofputil_protocol *usable_protocols)
diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
index fecdb43..c2dfe9d 100644
--- a/include/openvswitch/ofp-util.h
+++ b/include/openvswitch/ofp-util.h
@@ -27,6 +27,7 @@
 #include "openvswitch/netdev.h"
 #include "openflow/netronome-ext.h"
 #include "openflow/nicira-ext.h"
+#include "openvswitch/ofp-msgs.h"
 #include "openvswitch/ofpbuf.h"
 #include "openvswitch/types.h"
 #include "openvswitch/type-props.h"
@@ -1327,8 +1328,6 @@ struct ofputil_bundle_add_msg {
 const struct ofp_header   *msg;
 };
 
-enum ofptype;
-
 enum ofperr ofputil_decode_bundle_ctrl(const struct ofp_header *,
struct ofputil_bundle_ctrl_msg *);
 
@@ -1344,6 +1343,19 @@ enum ofperr ofputil_decode_bundle_add(const struct 

[ovs-dev] [PATCH v2 22/26] ofp-util: remove flow mod's delete_reason.

[Jarno Rajahalme]

We can use the rule's removed_reason instead.

Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/ofp-util.h |  3 ---
 lib/learn.c|  1 -
 lib/ofp-parse.c|  1 -
 lib/ofp-util.c |  4 
 ofproto/ofproto-dpif.c |  1 -
 ofproto/ofproto.c  | 21 ++---
 utilities/ovs-ofctl.c  |  1 -
 7 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
index c2dfe9d..177bf2b 100644
--- a/include/openvswitch/ofp-util.h
+++ b/include/openvswitch/ofp-util.h
@@ -325,9 +325,6 @@ struct ofputil_flow_mod {
 uint16_t importance; /* Eviction precedence. */
 struct ofpact *ofpacts;  /* Series of "struct ofpact"s. */
 size_t ofpacts_len;  /* Length of ofpacts, in bytes. */
-
-/* Reason for delete; ignored for non-delete commands */
-enum ofp_flow_removed_reason delete_reason;
 };
 
 enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *,
diff --git a/lib/learn.c b/lib/learn.c
index 33d4169..a8469fa 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -111,7 +111,6 @@ learn_execute(const struct ofpact_learn *learn, const 
struct flow *flow,
 }
 fm->ofpacts = NULL;
 fm->ofpacts_len = 0;
-fm->delete_reason = OFPRR_DELETE;
 
 if (learn->fin_idle_timeout || learn->fin_hard_timeout) {
 struct ofpact_fin_timeout *oft;
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 780c9df..d3ef140 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -340,7 +340,6 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, 
char *string,
 .buffer_id = UINT32_MAX,
 .out_port = OFPP_ANY,
 .out_group = OFPG_ANY,
-.delete_reason = OFPRR_DELETE,
 };
 /* For modify, by default, don't update the cookie. */
 if (command == OFPFC_MODIFY || command == OFPFC_MODIFY_STRICT) {
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index b62827e..ccb06fe 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1576,10 +1576,6 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
 {
 ovs_be16 raw_flags;
 enum ofperr error;
-
-/* Ignored for non-delete actions */
-fm->delete_reason = OFPRR_DELETE;
-
 struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
 enum ofpraw raw = ofpraw_pull_assert();
 if (raw == OFPRAW_OFPT11_FLOW_MOD) {
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d57068d..aa933b1 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5559,7 +5559,6 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif 
*ofproto,
 .flags = OFPUTIL_FF_HIDDEN_FIELDS | OFPUTIL_FF_NO_READONLY,
 .ofpacts = ofpacts->data,
 .ofpacts_len = ofpacts->size,
-.delete_reason = OVS_OFPRR_NONE,
 };
 
 error = ofproto_flow_mod(>up, );
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 83a4282..f467125 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2113,7 +2113,6 @@ flow_mod_init(struct ofputil_flow_mod *fm,
 .out_group = OFPG_ANY,
 .ofpacts = CONST_CAST(struct ofpact *, ofpacts),
 .ofpacts_len = ofpacts_len,
-.delete_reason = OFPRR_DELETE,
 };
 }
 
@@ -4866,7 +4865,7 @@ add_flow_start(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm)
 eviction_group_remove_rule(rule);
 /* Marks '*old_rule' as an evicted rule rather than replaced rule.
  */
-fm->delete_reason = OFPRR_EVICTION;
+rule->removed_reason = OFPRR_EVICTION;
 *old_rule = rule;
 }
 } else {
@@ -4892,11 +4891,10 @@ static void
 add_flow_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
 OVS_REQUIRES(ofproto_mutex)
 {
-struct ofputil_flow_mod *fm = >fm;
 struct rule *old_rule = rule_collection_stub(>old_rules)[0];
 struct rule *new_rule = rule_collection_stub(>new_rules)[0];
 
-if (old_rule && fm->delete_reason == OFPRR_EVICTION) {
+if (old_rule && old_rule->removed_reason == OFPRR_EVICTION) {
 /* Revert the eviction. */
 eviction_group_add_rule(old_rule);
 }
@@ -4979,7 +4977,7 @@ replace_rule_create(struct ofproto *ofproto, struct 
ofputil_flow_mod *fm,
 rule->modify_seqno = 0;
 
 /* Copy values from old rule for modify semantics. */
-if (old_rule && fm->delete_reason != OFPRR_EVICTION) {
+if (old_rule && old_rule->removed_reason != OFPRR_EVICTION) {
 bool change_cookie = (fm->modify_cookie
   && fm->new_cookie != OVS_BE64_MAX
   && fm->new_cookie != old_rule->flow_cookie);
@@ -5076,7 +5074,8 @@ replace_rule_finish(struct ofproto *ofproto, struct 
ofputil_flow_mod *fm,
 bool forward_counts = !(new_rule->flags & OFPUTIL_FF_RESET_COUNTS);
 struct rule *replaced_rule;
 
-replaced_rule = fm->delete_reason != OFPRR_EVICTION ? old_rule : NULL;
+replaced_rule = 

[ovs-dev] [PATCH v2 16/26] meta-flow: Clean up masking with prerequisities checking.

[Jarno Rajahalme]

Change mf_are_prereqs_ok() take a flow_wildcards pointer, so that the
wildcards can be set at the same time as the prerequisiteis are
checked.  This makes it easier to write more obviously correct code.

Remove the functions mf_mask_field_and_prereqs() and
mf_mask_field_and_prereqs__(), and make the callers first check the
prerequisites, while supplying 'wc' to mf_are_prereqs_ok(), and if
successful, mask the bits of the field that were read or set using
mf_mask_field_masked().

Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/meta-flow.h |  8 +---
 lib/classifier.c|  2 +-
 lib/flow.h  | 45 ++
 lib/meta-flow.c | 82 -
 lib/nx-match.c  | 26 -
 lib/ofp-actions.c   |  2 +-
 lib/ofp-parse.c |  2 +-
 ofproto/ofproto-dpif-xlate.c| 21 ++-
 ofproto/ofproto.c   |  2 +-
 utilities/ovs-ofctl.c   |  6 ++-
 10 files changed, 92 insertions(+), 104 deletions(-)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 900cd6a..10e1f77 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2060,12 +2060,8 @@ void mf_get_mask(const struct mf_field *, const struct 
flow_wildcards *,
  union mf_value *mask);
 
 /* Prerequisites. */
-bool mf_are_prereqs_ok(const struct mf_field *, const struct flow *);
-void mf_mask_field_and_prereqs(const struct mf_field *,
-   struct flow_wildcards *);
-void mf_mask_field_and_prereqs__(const struct mf_field *,
- const union mf_value *,
- struct flow_wildcards *);
+bool mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
+   struct flow_wildcards *wc);
 void mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct
  mf_bitmap *bm);
 
diff --git a/lib/classifier.c b/lib/classifier.c
index d5fd759..3546b3d 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -1973,7 +1973,7 @@ trie_lookup(const struct cls_trie *trie, const struct 
flow *flow,
 /* Check that current flow matches the prerequisites for the trie
  * field.  Some match fields are used for multiple purposes, so we
  * must check that the trie is relevant for this flow. */
-if (mf_are_prereqs_ok(mf, flow)) {
+if (mf_are_prereqs_ok(mf, flow, NULL)) {
 return trie_lookup_value(>root,
  &((ovs_be32 *)flow)[mf->flow_be32ofs],
  >be32, mf->n_bits);
diff --git a/lib/flow.h b/lib/flow.h
index fd9c712..e3f88b2 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -854,11 +854,56 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const 
struct flow *flow)
 md->ct_label = flow->ct_label;
 }
 
+static inline bool is_vlan(const struct flow *flow,
+   struct flow_wildcards *wc)
+{
+if (wc) {
+WC_MASK_FIELD_MASK(wc, vlan_tci, htons(VLAN_CFI));
+}
+return (flow->vlan_tci & htons(VLAN_CFI)) != 0;
+}
+
 static inline bool is_ip_any(const struct flow *flow)
 {
 return dl_type_is_ip_any(flow->dl_type);
 }
 
+static inline bool is_tcp(const struct flow *flow,
+  struct flow_wildcards *wc)
+{
+if (is_ip_any(flow)) {
+if (wc) {
+WC_MASK_FIELD(wc, nw_proto);
+}
+return flow->nw_proto == IPPROTO_TCP;
+}
+return false;
+}
+
+static inline bool is_udp(const struct flow *flow,
+  struct flow_wildcards *wc)
+{
+if (is_ip_any(flow)) {
+if (wc) {
+WC_MASK_FIELD(wc, nw_proto);
+}
+return flow->nw_proto == IPPROTO_UDP;
+}
+return false;
+}
+
+static inline bool is_sctp(const struct flow *flow,
+   struct flow_wildcards *wc)
+{
+if (is_ip_any(flow)) {
+if (wc) {
+WC_MASK_FIELD(wc, nw_proto);
+}
+return flow->nw_proto == IPPROTO_SCTP;
+}
+return false;
+}
+
 static inline bool is_icmpv4(const struct flow *flow,
  struct flow_wildcards *wc)
 {
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 28079d7..1701520 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -365,99 +365,49 @@ mf_is_mask_valid(const struct mf_field *mf, const union 
mf_value *mask)
 OVS_NOT_REACHED();
 }
 
-/* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise. */
+/* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise.
+ * Sets inspected bits in 'wc', if non-NULL. */
 bool
-mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow)
+mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
+  struct flow_wildcards *wc)
 {
 switch (mf->prereqs) {
 case MFP_NONE:
  

[ovs-dev] [PATCH v2 15/26] meta-flow: Add mf_mask_field_masked().

[Jarno Rajahalme]

Having a masked version allows generating better wildcarding.

Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/meta-flow.h |  4 +++-
 include/openvswitch/ofp-parse.h |  4 ++--
 lib/dpctl.c |  4 ++--
 lib/meta-flow.c | 37 +++--
 lib/ofp-parse.c | 31 ---
 ofproto/ofproto-dpif-xlate.c|  2 +-
 tests/test-odp.c|  2 +-
 7 files changed, 52 insertions(+), 32 deletions(-)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 533a4e6..900cd6a 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2090,7 +2090,9 @@ void mf_set_flow_value_masked(const struct mf_field *,
   struct flow *);
 bool mf_is_tun_metadata(const struct mf_field *);
 bool mf_is_set(const struct mf_field *, const struct flow *);
-void mf_mask_field(const struct mf_field *, struct flow *);
+void mf_mask_field(const struct mf_field *, struct flow_wildcards *);
+void mf_mask_field_masked(const struct mf_field *, const union mf_value *mask,
+  struct flow_wildcards *);
 int mf_field_len(const struct mf_field *, const union mf_value *value,
  const union mf_value *mask, bool *is_masked);
 
diff --git a/include/openvswitch/ofp-parse.h b/include/openvswitch/ofp-parse.h
index 1ab5095..a460d8a 100644
--- a/include/openvswitch/ofp-parse.h
+++ b/include/openvswitch/ofp-parse.h
@@ -61,8 +61,8 @@ char *parse_ofp_flow_stats_request_str(struct 
ofputil_flow_stats_request *,
enum ofputil_protocol *usable_protocols)
 OVS_WARN_UNUSED_RESULT;
 
-char *parse_ofp_exact_flow(struct flow *flow, struct flow *mask, const char *s,
-   const struct simap *portno_names);
+char *parse_ofp_exact_flow(struct flow *flow, struct flow_wildcards *wc,
+   const char *s, const struct simap *portno_names);
 
 char *parse_ofp_meter_mod_str(struct ofputil_meter_mod *, const char *string,
   int command,
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 76b701c..b470ab0 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -802,8 +802,8 @@ dpctl_dump_flows(int argc, const char *argv[], struct 
dpctl_params *dpctl_p)
 }
 
 if (filter) {
-char *err = parse_ofp_exact_flow(_filter, _filter.masks,
- filter, _portno);
+char *err = parse_ofp_exact_flow(_filter, _filter, filter,
+ _portno);
 if (err) {
 dpctl_error(dpctl_p, 0, "Failed to parse filter (%s)", err);
 error = EINVAL;
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 503da0c..28079d7 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -1122,20 +1122,37 @@ mf_set_value(const struct mf_field *mf,
 }
 }
 
-/* Unwildcard 'mask' member field described by 'mf'.  The caller is
- * responsible for ensuring that 'mask' meets 'mf''s prerequisites. */
+/* Unwildcard the bits in 'mask' of the 'wc' member field described by 'mf'.
+ * The caller is responsible for ensuring that 'wc' meets 'mf''s
+ * prerequisites. */
 void
-mf_mask_field(const struct mf_field *mf, struct flow *mask)
+mf_mask_field_masked(const struct mf_field *mf, const union mf_value *mask,
+ struct flow_wildcards *wc)
 {
-/* For MFF_DL_VLAN, we cannot send a all 1's to flow_set_dl_vlan()
- * as that will be considered as OFP10_VLAN_NONE. So consider it as a
- * special case. For the rest, calling mf_set_flow_value() is good
- * enough. */
+union mf_value temp_mask;
+/* For MFF_DL_VLAN, we cannot send a all 1's to flow_set_dl_vlan() as that
+ * will be considered as OFP10_VLAN_NONE. So make sure the mask only has
+ * valid bits in this case. */
 if (mf->id == MFF_DL_VLAN) {
-flow_set_dl_vlan(mask, htons(VLAN_VID_MASK));
-} else {
-mf_set_flow_value(mf, _match_mask, mask);
+temp_mask.be16 = htons(VLAN_VID_MASK) & mask->be16;
+mask = _mask;
+}
+
+union mf_value mask_value;
+
+mf_get_value(mf, >masks, _value);
+for (size_t i = 0; i < mf->n_bytes; i++) {
+mask_value.b[i] |= mask->b[i];
 }
+mf_set_flow_value(mf, _value, >masks);
+}
+
+/* Unwildcard 'wc' member field described by 'mf'.  The caller is
+ * responsible for ensuring that 'mask' meets 'mf''s prerequisites. */
+void
+mf_mask_field(const struct mf_field *mf, struct flow_wildcards *wc)
+{
+mf_mask_field_masked(mf, _match_mask, wc);
 }
 
 static int
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 370e3e5..73072d6 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1115,23 +1115,23 @@ parse_ofp_flow_stats_request_str(struct 
ofputil_flow_stats_request *fsr,
 /* Parses a specification of a flow from 's' into 'flow'.  's' must take the
  * form 

[ovs-dev] [PATCH v2 13/26] ofproto-dpif: Always forward 'used' from the old_rule.

[Jarno Rajahalme]

Use new rule's flags to determine whether stats should be forwarded
from the old, modified rule to the new rule.  This captures the fact
that prior to OpenFlow 1.2, which defines the reset counts flag, the
reset counts semantics was assumed by default.  However, in that case
the reset counts flag is only present in the new flow, not on the
corresponding flow mod.

Having the above fixed revealed that the 'used' timestamp was not
forwarded from the old rule to the new rule when counts were not being
forwarded.  Fix this by always forwarding the 'used' timestamp.

Fixes: 39c9459355 ("Use classifier versioning.")
Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-dpif.c | 38 +++---
 ofproto/ofproto-provider.h |  7 ---
 ofproto/ofproto.c  |  9 +
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 0f00cef..d57068d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -96,6 +96,8 @@ struct rule_dpif {
 * 'rule->new_rule->stats_mutex' must be held together, acquire them in that
 * order, */
 struct rule_dpif *new_rule OVS_GUARDED;
+bool forward_counts OVS_GUARDED;   /* Forward counts? 'used' time will be
+* forwarded in all cases. */
 
 /* If non-zero then the recirculation id that has
  * been allocated for use with this rule.
@@ -3832,17 +3834,30 @@ ofproto_dpif_execute_actions(struct ofproto_dpif 
*ofproto,
   ofpacts_len, 0, 0, 0, packet);
 }
 
+static void
+rule_dpif_credit_stats__(struct rule_dpif *rule,
+ const struct dpif_flow_stats *stats,
+ bool credit_counts)
+OVS_REQUIRES(rule->stats_mutex)
+{
+if (credit_counts) {
+rule->stats.n_packets += stats->n_packets;
+rule->stats.n_bytes += stats->n_bytes;
+}
+rule->stats.used = MAX(rule->stats.used, stats->used);
+}
+
 void
 rule_dpif_credit_stats(struct rule_dpif *rule,
const struct dpif_flow_stats *stats)
 {
 ovs_mutex_lock(>stats_mutex);
 if (OVS_UNLIKELY(rule->new_rule)) {
-rule_dpif_credit_stats(rule->new_rule, stats);
+ovs_mutex_lock(>new_rule->stats_mutex);
+rule_dpif_credit_stats__(rule->new_rule, stats, rule->forward_counts);
+ovs_mutex_unlock(>new_rule->stats_mutex);
 } else {
-rule->stats.n_packets += stats->n_packets;
-rule->stats.n_bytes += stats->n_bytes;
-rule->stats.used = MAX(rule->stats.used, stats->used);
+rule_dpif_credit_stats__(rule, stats, true);
 }
 ovs_mutex_unlock(>stats_mutex);
 }
@@ -4171,17 +4186,18 @@ rule_construct(struct rule *rule_)
 rule->stats.used = rule->up.modified;
 rule->recirc_id = 0;
 rule->new_rule = NULL;
+rule->forward_counts = false;
 
 return 0;
 }
 
 static void
-rule_insert(struct rule *rule_, struct rule *old_rule_, bool forward_stats)
+rule_insert(struct rule *rule_, struct rule *old_rule_, bool forward_counts)
 OVS_REQUIRES(ofproto_mutex)
 {
 struct rule_dpif *rule = rule_dpif_cast(rule_);
 
-if (old_rule_ && forward_stats) {
+if (old_rule_) {
 struct rule_dpif *old_rule = rule_dpif_cast(old_rule_);
 
 ovs_assert(!old_rule->new_rule);
@@ -4193,7 +4209,15 @@ rule_insert(struct rule *rule_, struct rule *old_rule_, 
bool forward_stats)
 ovs_mutex_lock(_rule->stats_mutex);
 ovs_mutex_lock(>stats_mutex);
 old_rule->new_rule = rule;   /* Forward future stats. */
-rule->stats = old_rule->stats;   /* Transfer stats to the new rule. */
+old_rule->forward_counts = forward_counts;
+
+if (forward_counts) {
+rule->stats = old_rule->stats;   /* Transfer stats to the new
+  * rule. */
+} else {
+/* Used timestamp must be forwarded whenever a rule is modified. */
+rule->stats.used = old_rule->stats.used;
+}
 ovs_mutex_unlock(>stats_mutex);
 ovs_mutex_unlock(_rule->stats_mutex);
 }
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index c6300e7..0610e78 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1311,8 +1311,9 @@ struct ofproto_class {
  * must add the new rule to the datapath flow table and return only after
  * this is complete.  The 'new_rule' may be a duplicate of an 'old_rule'.
  * In this case the 'old_rule' is non-null, and the implementation should
- * forward rule statistics from the 'old_rule' to the 'new_rule' if
- * 'forward_stats' is 'true'.  This may not fail.
+ * forward rule statistics counts from the 'old_rule' to the 'new_rule' if
+ * 'forward_counts' is 'true', 'used' timestamp is always forwarded.  This
+ * may not fail.
  *
  *
  * Deletion
@@ -1336,7 

[ovs-dev] [PATCH v2 12/26] vconn: Better bundle error management.

[Jarno Rajahalme]

It is possible that a bundle add message fails, but the following
commit succeeds, since the message was not added to the bundle.  Make
ovs-ofctl fail also in these cases.

Also, the commit should not be sent if any of the bundled messages
failed.  To make sure all the errors are received before the commit is
sent, a barrier is required before sending the commit message.

Finally, make vconn collect bundle errors into a list instead of
calling a callback.  This makes bundle error management simpler.

Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/vconn.h |  12 +++-
 lib/vconn.c | 130 +---
 tests/ofproto.at|  34 +++-
 tests/ovs-ofctl.at  |   4 ++
 utilities/ovs-ofctl.c   |  50 +++--
 5 files changed, 167 insertions(+), 63 deletions(-)

diff --git a/include/openvswitch/vconn.h b/include/openvswitch/vconn.h
index f8b6655..c44f0bf 100644
--- a/include/openvswitch/vconn.h
+++ b/include/openvswitch/vconn.h
@@ -55,9 +55,19 @@ int vconn_transact(struct vconn *, struct ofpbuf *, struct 
ofpbuf **);
 int vconn_transact_noreply(struct vconn *, struct ofpbuf *, struct ofpbuf **);
 int vconn_transact_multiple_noreply(struct vconn *, struct ovs_list *requests,
 struct ofpbuf **replyp);
+
+/* Bundle errors must be free()d by the caller. */
+struct vconn_bundle_error {
+struct ovs_list list_node;
+
+/* OpenFlow header and some of the message contents for error reporting. */
+struct ofp_header ofp_msg[DIV_ROUND_UP(64, sizeof(struct ofp_header))];
+};
+
+/* Bundle errors must be free()d by the caller. */
 int vconn_bundle_transact(struct vconn *, struct ovs_list *requests,
   uint16_t bundle_flags,
-  void (*error_reporter)(const struct ofp_header *));
+  struct ovs_list *errors);
 
 void vconn_run(struct vconn *);
 void vconn_run_wait(struct vconn *);
diff --git a/lib/vconn.c b/lib/vconn.c
index 917ad28..2759c1a 100644
--- a/lib/vconn.c
+++ b/lib/vconn.c
@@ -744,9 +744,21 @@ vconn_recv_block(struct vconn *vconn, struct ofpbuf **msgp)
 return retval;
 }
 
+static void
+vconn_add_bundle_error(const struct ofp_header *oh, struct ovs_list *errors)
+{
+if (errors) {
+struct vconn_bundle_error *err = xmalloc(sizeof *err);
+size_t len = ntohs(oh->length);
+
+memcpy(err->ofp_msg, oh, MIN(len, sizeof err->ofp_msg));
+ovs_list_push_back(errors, >list_node);
+}
+}
+
 static int
 vconn_recv_xid__(struct vconn *vconn, ovs_be32 xid, struct ofpbuf **replyp,
- void (*error_reporter)(const struct ofp_header *))
+ struct ovs_list *errors)
 {
 for (;;) {
 ovs_be32 recv_xid;
@@ -768,8 +780,8 @@ vconn_recv_xid__(struct vconn *vconn, ovs_be32 xid, struct 
ofpbuf **replyp,
 }
 
 error = ofptype_decode(, oh);
-if (!error && type == OFPTYPE_ERROR && error_reporter) {
-error_reporter(oh);
+if (!error && type == OFPTYPE_ERROR) {
+vconn_add_bundle_error(oh, errors);
 } else {
 VLOG_DBG_RL(_ofmsg_rl, "%s: received reply with xid %08"PRIx32
 " != expected %08"PRIx32,
@@ -793,8 +805,7 @@ vconn_recv_xid(struct vconn *vconn, ovs_be32 xid, struct 
ofpbuf **replyp)
 
 static int
 vconn_transact__(struct vconn *vconn, struct ofpbuf *request,
- struct ofpbuf **replyp,
- void (*error_reporter)(const struct ofp_header *))
+ struct ofpbuf **replyp, struct ovs_list *errors)
 {
 ovs_be32 send_xid = ((struct ofp_header *) request->data)->xid;
 int error;
@@ -804,8 +815,7 @@ vconn_transact__(struct vconn *vconn, struct ofpbuf 
*request,
 if (error) {
 ofpbuf_delete(request);
 }
-return error ? error : vconn_recv_xid__(vconn, send_xid, replyp,
-error_reporter);
+return error ? error : vconn_recv_xid__(vconn, send_xid, replyp, errors);
 }
 
 /* Sends 'request' to 'vconn' and blocks until it receives a reply with a
@@ -825,6 +835,22 @@ vconn_transact(struct vconn *vconn, struct ofpbuf *request,
 return vconn_transact__(vconn, request, replyp, NULL);
 }
 
+static int
+vconn_send_barrier(struct vconn *vconn, ovs_be32 *barrier_xid)
+{
+struct ofpbuf *barrier;
+int error;
+
+/* Send barrier. */
+barrier = ofputil_encode_barrier_request(vconn_get_version(vconn));
+*barrier_xid = ((struct ofp_header *) barrier->data)->xid;
+error = vconn_send_block(vconn, barrier);
+if (error) {
+ofpbuf_delete(barrier);
+}
+return error;
+}
+
 /* Sends 'request' followed by a barrier request to 'vconn', then blocks until
  * it receives a reply to the barrier.  If successful, stores the reply to
  * 'request' in '*replyp', if one was received, and otherwise NULL, then
@@ -842,7 +868,6 @@ 

[ovs-dev] [PATCH v2 18/26] ofp-util: Do not free() field that is not allocated.

[Jarno Rajahalme]

Group properties field array is not dynamically allocated, so it
should not be freed.  This has not been a problem, as this function
has not been called by anyone so far, but following patch will.

Signed-off-by: Jarno Rajahalme 
---
 lib/ofp-util.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 06b48b8..175ae2d 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -8205,7 +8205,6 @@ void
 ofputil_uninit_group_desc(struct ofputil_group_desc *gd)
 {
 ofputil_bucket_list_destroy(>buckets);
-free(>props.fields);
 }
 
 /* Decodes the OpenFlow group description request in 'oh', returning the group
-- 
2.1.4

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

[ovs-dev] [PATCH v2 17/26] ofproto-dpif-xlate: Hash only fields specified for 'hash' selection method.

[Jarno Rajahalme]

The mask for non-present fields in struct field_array is always zero,
so hashing a prerequisite field that was not also specified for the
"hash" selection method boiled down to hashing a all-zeroes value and
unwildcarding the prerequisite field.  Now that mf_are_prereqs_ok()
already takes care of unwildcarding, we can simplify the code by
hashing only the specified fields.

Also change the test case to include fields that have prerequisities.

Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/meta-flow.h |  2 --
 lib/meta-flow.c | 36 
 ofproto/ofproto-dpif-xlate.c| 36 +++-
 tests/ofproto-dpif.at   |  2 +-
 4 files changed, 8 insertions(+), 68 deletions(-)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 10e1f77..d8c5dd8 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2062,8 +2062,6 @@ void mf_get_mask(const struct mf_field *, const struct 
flow_wildcards *,
 /* Prerequisites. */
 bool mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
struct flow_wildcards *wc);
-void mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct
- mf_bitmap *bm);
 
 static inline bool
 mf_is_l3_or_higher(const struct mf_field *mf)
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 1701520..c44dd2a 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -408,42 +408,6 @@ mf_are_prereqs_ok(const struct mf_field *mf, const struct 
flow *flow,
 OVS_NOT_REACHED();
 }
 
-/* Set bits of 'bm' corresponding to the field 'mf' and it's prerequisities. */
-void
-mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct mf_bitmap 
*bm)
-{
-bitmap_set1(bm->bm, mf->id);
-
-switch (mf->prereqs) {
-case MFP_ND:
-case MFP_ND_SOLICIT:
-case MFP_ND_ADVERT:
-bitmap_set1(bm->bm, MFF_TCP_SRC);
-bitmap_set1(bm->bm, MFF_TCP_DST);
-/* Fall through. */
-case MFP_TCP:
-case MFP_UDP:
-case MFP_SCTP:
-case MFP_ICMPV4:
-case MFP_ICMPV6:
-/* nw_frag always unwildcarded. */
-bitmap_set1(bm->bm, MFF_IP_PROTO);
-/* Fall through. */
-case MFP_ARP:
-case MFP_IPV4:
-case MFP_IPV6:
-case MFP_MPLS:
-case MFP_IP_ANY:
-bitmap_set1(bm->bm, MFF_ETH_TYPE);
-break;
-case MFP_VLAN_VID:
-bitmap_set1(bm->bm, MFF_VLAN_TCI);
-break;
-case MFP_NONE:
-break;
-}
-}
-
 /* Returns true if 'value' may be a valid value *as part of a masked match*,
  * false otherwise.
  *
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 1fc738d..3efba3a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3490,7 +3490,6 @@ xlate_default_select_group(struct xlate_ctx *ctx, struct 
group_dpif *group)
 static void
 xlate_hash_fields_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
 {
-struct mf_bitmap hash_fields = MF_BITMAP_INITIALIZER;
 const struct field_array *fields;
 struct ofputil_bucket *bucket;
 uint32_t basis;
@@ -3499,44 +3498,23 @@ xlate_hash_fields_select_group(struct xlate_ctx *ctx, 
struct group_dpif *group)
 fields = group_dpif_get_fields(group);
 basis = hash_uint64(group_dpif_get_selection_method_param(group));
 
-/* Determine which fields to hash */
 for (i = 0; i < MFF_N_IDS; i++) {
 if (bitmap_is_set(fields->used.bm, i)) {
-const struct mf_field *mf;
-
-/* If the field is already present in 'hash_fields' then
- * this loop has already checked that it and its pre-requisites
- * are present in the flow and its pre-requisites have
- * already been added to 'hash_fields'. There is nothing more
- * to do here and as an optimisation the loop can continue. */
-if (bitmap_is_set(hash_fields.bm, i)) {
-continue;
-}
-
-mf = mf_from_id(i);
+const struct mf_field *mf = mf_from_id(i);
 
-/* Only hash a field if it and its pre-requisites are present
- * in the flow. */
+/* Skip fields for which prerequisities are not met. */
 if (!mf_are_prereqs_ok(mf, >xin->flow, ctx->wc)) {
 continue;
 }
 
-/* Hash both the field and its pre-requisites */
-mf_bitmap_set_field_and_prereqs(mf, _fields);
-}
-}
-
-/* Hash the fields */
-for (i = 0; i < MFF_N_IDS; i++) {
-if (bitmap_is_set(hash_fields.bm, i)) {
-const struct mf_field *mf = mf_from_id(i);
 union mf_value value;
-int j;
+union mf_value mask;
 
 mf_get_value(mf, >xin->flow, );
 /* This seems inefficient but so does apply_mask() */
-for (j = 0; j < 

[ovs-dev] [PATCH v2 19/26] ofproto: Use ofputil_uninit_group_mod().

[Jarno Rajahalme]

Use ofputil_uninit_group_mod() instead of
ofputil_bucket_list_destroy().  Currently these have the same effect,
but this will change in a following patch.

Signed-off-by: Jarno Rajahalme 
---
 lib/ofp-parse.c | 6 +++---
 lib/ofp-print.c | 4 ++--
 ofproto/ofproto.c   | 2 +-
 ovn/controller/ofctrl.c | 6 +++---
 utilities/ovs-ofctl.c   | 6 +-
 5 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 8659079..56607f6 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1590,7 +1590,7 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, 
uint16_t command,
 
 return NULL;
  out:
-ofputil_bucket_list_destroy(>buckets);
+ofputil_uninit_group_mod(gm);
 return error;
 }
 
@@ -1605,7 +1605,7 @@ parse_ofp_group_mod_str(struct ofputil_group_mod *gm, 
uint16_t command,
 free(string);
 
 if (error) {
-ofputil_bucket_list_destroy(>buckets);
+ofputil_uninit_group_mod(gm);
 }
 return error;
 }
@@ -1653,7 +1653,7 @@ parse_ofp_group_mod_file(const char *file_name, uint16_t 
command,
 size_t i;
 
 for (i = 0; i < *n_gms; i++) {
-ofputil_bucket_list_destroy(&(*gms)[i].buckets);
+ofputil_uninit_group_mod(&(*gms)[i]);
 }
 free(*gms);
 *gms = NULL;
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 919c95d..0b3bf01 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2586,7 +2586,7 @@ ofp_print_group_desc(struct ds *s, const struct 
ofp_header *oh)
 ds_put_char(s, ' ');
 ofp_print_group(s, gd.group_id, gd.type, , ,
 oh->version, false);
-ofputil_bucket_list_destroy();
+ofputil_uninit_group_desc();
  }
 }
 
@@ -2747,7 +2747,7 @@ ofp_print_group_mod(struct ds *s, const struct ofp_header 
*oh)
 return;
 }
 ofp_print_group_mod__(s, oh->version, );
-ofputil_bucket_list_destroy();
+ofputil_uninit_group_mod();
 }
 
 static void
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index dca620c..cd0c90f 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6980,7 +6980,7 @@ handle_group_mod(struct ofconn *ofconn, const struct 
ofp_header *oh)
 ofmonitor_flush(ofproto->connmgr);
 ovs_mutex_unlock(_mutex);
 
-ofputil_bucket_list_destroy();
+ofputil_uninit_group_mod();
 
 return error;
 }
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index dd9f5ec..79d840d 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -365,7 +365,7 @@ run_S_CLEAR_FLOWS(void)
 gm.command_bucket_id = OFPG15_BUCKET_ALL;
 ovs_list_init();
 queue_msg(encode_group_mod());
-ofputil_bucket_list_destroy();
+ofputil_uninit_group_mod();
 
 /* Clear installed_flows, to match the state of the switch. */
 ovn_flow_table_clear();
@@ -936,7 +936,7 @@ ofctrl_put(struct group_table *group_table, int64_t nb_cfg)
 free(error);
 }
 ds_destroy(_string);
-ofputil_bucket_list_destroy();
+ofputil_uninit_group_mod();
 }
 }
 
@@ -1051,7 +1051,7 @@ ofctrl_put(struct group_table *group_table, int64_t 
nb_cfg)
 free(error);
 }
 ds_destroy(_string);
-ofputil_bucket_list_destroy();
+ofputil_uninit_group_mod();
 
 /* Remove 'installed' from 'group_table->existing_groups' */
 hmap_remove(_table->existing_groups, >hmap_node);
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 3ee7879..db4cf8a 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2525,10 +2525,10 @@ ofctl_group_mod__(const char *remote, struct 
ofputil_group_mod *gms,
 if (request) {
 transact_noreply(vconn, request);
 }
+ofputil_uninit_group_mod(gm);
 }
 
 vconn_close(vconn);
-
 }
 
 
@@ -2547,9 +2547,6 @@ ofctl_group_mod_file(int argc OVS_UNUSED, char *argv[], 
uint16_t command)
 ovs_fatal(0, "%s", error);
 }
 ofctl_group_mod__(argv[1], gms, n_gms, usable_protocols);
-for (i = 0; i < n_gms; i++) {
-ofputil_bucket_list_destroy([i].buckets);
-}
 free(gms);
 }
 
@@ -2569,7 +2566,6 @@ ofctl_group_mod(int argc, char *argv[], uint16_t command)
 ovs_fatal(0, "%s", error);
 }
 ofctl_group_mod__(argv[1], , 1, usable_protocols);
-ofputil_bucket_list_destroy();
 }
 }
 
-- 
2.1.4

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

[ovs-dev] [PATCH v2 20/26] meta-flow: Compact struct field_array.

[Jarno Rajahalme]

struct field_array is included in each ofgroup, but the current
implementation is very sparse, using more than 20kb of data.

Also loop over 1-bits instead of each and every MF type to make
processing faster.

Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/meta-flow.h |  6 --
 include/openvswitch/ofp-util.h  |  3 +++
 lib/meta-flow.c | 23 +++-
 lib/nx-match.c  | 32 ---
 lib/ofp-util.c  | 16 ++
 ofproto/ofproto-dpif-xlate.c| 48 +
 ofproto/ofproto.c   |  9 ++--
 7 files changed, 96 insertions(+), 41 deletions(-)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index d8c5dd8..aaf7da1 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2035,10 +2035,12 @@ int mf_subvalue_width(const union mf_subvalue *);
 void mf_subvalue_shift(union mf_subvalue *, int n);
 void mf_subvalue_format(const union mf_subvalue *, struct ds *);
 
-/* An array of fields with values */
+/* Set of field values. 'values' only includes the actual data bytes for each
+ * field for which is used, as marked by 1-bits in 'used'. */
 struct field_array {
 struct mf_bitmap used;
-union mf_value value[MFF_N_IDS];
+size_t values_size;  /* Number of bytes currently in 'values'. */
+uint8_t *values; /* Dynamically allocated to the correct size. */
 };
 
 /* Finding mf_fields. */
diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
index 7a5c905..fecdb43 100644
--- a/include/openvswitch/ofp-util.h
+++ b/include/openvswitch/ofp-util.h
@@ -1262,6 +1262,9 @@ struct ofputil_group_desc {
 struct ofputil_group_props props; /* Group properties. */
 };
 
+void ofputil_group_properties_destroy(struct ofputil_group_props *);
+void ofputil_group_properties_copy(struct ofputil_group_props *to,
+   const struct ofputil_group_props *from);
 void ofputil_bucket_list_destroy(struct ovs_list *buckets);
 void ofputil_bucket_clone_list(struct ovs_list *dest,
const struct ovs_list *src,
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index c44dd2a..2c89613 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -2515,7 +2515,28 @@ void
 field_array_set(enum mf_field_id id, const union mf_value *value,
 struct field_array *fa)
 {
+size_t i, offset = 0;
+
 ovs_assert(id < MFF_N_IDS);
+
+/* Find the spot for 'id'. */
+BITMAP_FOR_EACH_1 (i, id, fa->used.bm) {
+offset += mf_from_id(i)->n_bytes;
+}
+
+size_t value_size = mf_from_id(id)->n_bytes;
+
+/* make room if necessary. */
+if (!bitmap_is_set(fa->used.bm, id)) {
+fa->values = xrealloc(fa->values, fa->values_size + value_size);
+/* Move remainder forward, if any. */
+if (offset < fa->values_size) {
+memmove(fa->values + offset + value_size, fa->values + offset,
+fa->values_size - offset);
+}
+fa->values_size += value_size;
+}
 bitmap_set1(fa->used.bm, id);
-fa->value[id] = *value;
+
+memcpy(fa->values + offset, value, value_size);
 }
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 297e361..7e324e4 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1178,12 +1178,15 @@ void
 oxm_format_field_array(struct ds *ds, const struct field_array *fa)
 {
 size_t start_len = ds->length;
-int i;
+size_t i, offset = 0;
 
-for (i = 0; i < MFF_N_IDS; i++) {
-if (bitmap_is_set(fa->used.bm, i)) {
-nx_format_mask_tlv(ds, i, >value[i]);
-}
+BITMAP_FOR_EACH_1 (i, MFF_N_IDS, fa->used.bm) {
+const struct mf_field *mf = mf_from_id(i);
+union mf_value value;
+
+memcpy(, fa->values + offset, mf->n_bytes);
+nx_format_mask_tlv(ds, i, );
+offset += mf->n_bytes;
 }
 
 if (ds->length > start_len) {
@@ -1205,7 +1208,6 @@ oxm_put_field_array(struct ofpbuf *b, const struct 
field_array *fa,
 enum ofp_version version)
 {
 size_t start_len = b->size;
-int i;
 
 /* Field arrays are only used with the group selection method
  * property and group properties are only available in OpenFlow 1.5+.
@@ -1220,13 +1222,17 @@ oxm_put_field_array(struct ofpbuf *b, const struct 
field_array *fa,
  */
 ovs_assert(version >= OFP15_VERSION);
 
-for (i = 0; i < MFF_N_IDS; i++) {
-if (bitmap_is_set(fa->used.bm, i)) {
-int len = mf_field_len(mf_from_id(i), >value[i], NULL, NULL);
-nxm_put__(b, i, version,
-  >value[i].u8 + mf_from_id(i)->n_bytes - len, NULL,
-  len);
-}
+size_t i, offset = 0;
+
+BITMAP_FOR_EACH_1 (i, MFF_N_IDS, fa->used.bm) {
+const struct mf_field *mf = mf_from_id(i);
+union mf_value value;
+
+

[ovs-dev] [PATCH v2 14/26] meta-flow: Add byte access to struct mf_value.

[Jarno Rajahalme]

This allows reducing pointer casting when individual bytes of mf_value
are accessed.  First users are in the following patches.

Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/meta-flow.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 196868a..533a4e6 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -1954,6 +1954,7 @@ struct mf_field {
 
 /* The representation of a field's value. */
 union mf_value {
+uint8_t b[128];
 uint8_t tun_metadata[128];
 struct in6_addr ipv6;
 struct eth_addr mac;
-- 
2.1.4

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

[ovs-dev] [PATCH v2 11/26] vconn: Update length of bundled messages.

[Jarno Rajahalme]

Variable length messages need their length updated before they can be
added to the bundle.

Message length updating after encoding is sometimes done by the
encoding function, but always latest when the message is sent out.  As
an OpenFlow message is added to a bundle add message, it will not be
sent by itself, and we need to update the length explicitly instead.

Signed-off-by: Jarno Rajahalme 
---
 lib/ofp-util.c | 3 ++-
 lib/vconn.c| 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 6d73e69..06b48b8 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -10011,13 +10011,14 @@ ofputil_encode_bundle_add(enum ofp_version 
ofp_version,
 request = ofpraw_alloc_xid(ofp_version == OFP13_VERSION
? OFPRAW_ONFT13_BUNDLE_ADD_MESSAGE
: OFPRAW_OFPT14_BUNDLE_ADD_MESSAGE, ofp_version,
-   msg->msg->xid, 0);
+   msg->msg->xid, ntohs(msg->msg->length));
 m = ofpbuf_put_zeros(request, sizeof *m);
 
 m->bundle_id = htonl(msg->bundle_id);
 m->flags = htons(msg->flags);
 ofpbuf_put(request, msg->msg, ntohs(msg->msg->length));
 
+ofpmsg_update_length(request);
 return request;
 }
 
diff --git a/lib/vconn.c b/lib/vconn.c
index 50b4047..917ad28 100644
--- a/lib/vconn.c
+++ b/lib/vconn.c
@@ -1040,12 +1040,13 @@ vconn_bundle_add_msg(struct vconn *vconn, struct 
ofputil_bundle_ctrl_msg *bc,
 struct ofpbuf *request;
 int error;
 
+ofpmsg_update_length(msg);
+
 bam.bundle_id = bc->bundle_id;
 bam.flags = bc->flags;
 bam.msg = msg->data;
 
 request = ofputil_encode_bundle_add(vconn->version, );
-ofpmsg_update_length(request);
 
 error = vconn_send_block(vconn, request);
 if (!error) {
-- 
2.1.4

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

[ovs-dev] [PATCH v2 05/26] ofproto: Use ofproto_mutex for groups and keep track of referring flows.

[Jarno Rajahalme]

Adding groups support for bundles is simpler if also groups are
modified under ofproto_mutex.

Eliminate the search for rules when deleting a group so that we will
not keep the mutex for too long.

Signed-off-by: Jarno Rajahalme 
---
 include/openvswitch/ofp-actions.h |  40 +++
 ofproto/ofproto-provider.h|  10 +-
 ofproto/ofproto.c | 234 +-
 3 files changed, 204 insertions(+), 80 deletions(-)

diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index 0b8ccbb..6355eed 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -214,12 +214,45 @@ ofpact_end(const struct ofpact *ofpacts, size_t 
ofpacts_len)
 return (void *) ((uint8_t *) ofpacts + ofpacts_len);
 }
 
+static inline const struct ofpact *
+ofpact_find_type(const struct ofpact *a, enum ofpact_type type,
+ const struct ofpact * const end)
+{
+while (a < end) {
+if (a->type == type) {
+return a;
+}
+a = ofpact_next(a);
+}
+return NULL;
+}
+
+static inline const struct ofpact *
+ofpact_find_type_flattened(const struct ofpact *a, enum ofpact_type type,
+   const struct ofpact * const end)
+{
+while (a < end) {
+if (a->type == type) {
+return a;
+}
+a = ofpact_next_flattened(a);
+}
+return NULL;
+}
+
 /* Assigns POS to each ofpact, in turn, in the OFPACTS_LEN bytes of ofpacts
  * starting at OFPACTS. */
 #define OFPACT_FOR_EACH(POS, OFPACTS, OFPACTS_LEN)  \
 for ((POS) = (OFPACTS); (POS) < ofpact_end(OFPACTS, OFPACTS_LEN);  \
  (POS) = ofpact_next(POS))
 
+#define OFPACT_FOR_EACH_TYPE(POS, TYPE, OFPACTS, OFPACTS_LEN)   \
+for ((POS) = ofpact_find_type(OFPACTS, TYPE,\
+  ofpact_end(OFPACTS, OFPACTS_LEN));\
+ (POS); \
+ (POS) = ofpact_find_type(ofpact_next(POS), TYPE,   \
+  ofpact_end(OFPACTS, OFPACTS_LEN)))
+
 /* Assigns POS to each ofpact, in turn, in the OFPACTS_LEN bytes of ofpacts
  * starting at OFPACTS.
  *
@@ -228,6 +261,13 @@ ofpact_end(const struct ofpact *ofpacts, size_t 
ofpacts_len)
 #define OFPACT_FOR_EACH_FLATTENED(POS, OFPACTS, OFPACTS_LEN)   \
 for ((POS) = (OFPACTS); (POS) < ofpact_end(OFPACTS, OFPACTS_LEN);  \
  (POS) = ofpact_next_flattened(POS))
+
+#define OFPACT_FOR_EACH_TYPE_FLATTENED(POS, TYPE, OFPACTS, OFPACTS_LEN) \
+for ((POS) = ofpact_find_type_flattened(OFPACTS, TYPE,  \
+  ofpact_end(OFPACTS, OFPACTS_LEN));\
+ (POS); \
+ (POS) = ofpact_find_type_flattened(ofpact_next_flattened(POS), TYPE, \
+  ofpact_end(OFPACTS, OFPACTS_LEN)))
 
 /* Action structure for each OFPACT_*. */
 
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index dd014ea..8a898a5 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -124,7 +124,6 @@ struct ofproto {
 int min_mtu;/* Current MTU of non-internal ports. */
 
 /* Groups. */
-struct ovs_rwlock groups_rwlock;
 struct cmap groups;   /* Contains "struct ofgroup"s. */
 uint32_t n_groups[4] OVS_GUARDED; /* # of existing groups of each type. */
 struct ofputil_group_features ogf;
@@ -437,6 +436,7 @@ struct rule_actions {
  * action whose flags include NX_LEARN_F_DELETE_LEARNED. */
 bool has_meter;
 bool has_learn_with_delete;
+bool has_groups;
 
 /* Actions. */
 uint32_t ofpacts_len; /* Size of 'ofpacts', in bytes. */
@@ -455,11 +455,14 @@ struct rule_collection {
 size_t n;   /* Number of rules collected. */
 
 size_t capacity;/* Number of rules that will fit in 'rules'. */
-struct rule *stub[64];  /* Preallocated rules to avoid malloc(). */
+struct rule *stub[5];   /* Preallocated rules to avoid malloc(). */
 };
 
 void rule_collection_init(struct rule_collection *);
 void rule_collection_add(struct rule_collection *, struct rule *);
+void rule_collection_remove(struct rule_collection *, struct rule *);
+void rule_collection_move(struct rule_collection *to,
+  struct rule_collection *from);
 void rule_collection_ref(struct rule_collection *) OVS_REQUIRES(ofproto_mutex);
 void rule_collection_unref(struct rule_collection *);
 void rule_collection_destroy(struct rule_collection *);
@@ -513,6 +516,7 @@ struct ofgroup {
 const struct ofproto *ofproto;  /* The ofproto that contains this group. */
 const uint32_t group_id;
 const enum ofp11_group_type type; /* One of OFPGT_*. */
+bool being_deleted;   /* Group 

[ovs-dev] [PATCH v2 08/26] ofproto: Report flow mods also from bundles.

[Jarno Rajahalme]

Flow mod stats get skewed if they are not reported from bundles.  Move
reporting to ofproto_flow_mod_finish() so that it will be done in all
cases.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index a5d681a..0e8200b 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5489,7 +5489,7 @@ handle_flow_mod(struct ofconn *ofconn, const struct 
ofp_header *oh)
 
 error = reject_slave_controller(ofconn);
 if (error) {
-goto exit;
+return error;
 }
 
 ofpbuf_use_stub(, ofpacts_stub, sizeof ofpacts_stub);
@@ -5501,15 +5501,8 @@ handle_flow_mod(struct ofconn *ofconn, const struct 
ofp_header *oh)
 struct openflow_mod_requester req = { ofconn, oh };
 error = handle_flow_mod__(ofproto, , );
 }
-if (error) {
-goto exit_free_ofpacts;
-}
 
-ofconn_report_flow_mod(ofconn, ofm.fm.command);
-
-exit_free_ofpacts:
 ofpbuf_uninit();
-exit:
 return error;
 }
 
@@ -7077,6 +7070,10 @@ ofproto_flow_mod_finish(struct ofproto *ofproto,
 default:
 break;
 }
+
+if (req) {
+ofconn_report_flow_mod(req->ofconn, ofm->fm.command);
+}
 }
 
 /* Commit phases (all while locking ofproto_mutex):
-- 
2.1.4

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

[ovs-dev] [PATCH v2 06/26] ofproto: Add generic ofproto_collection.

[Jarno Rajahalme]

Define rule_collection in terms of a new ofproto_collection.  This
makes it easier to add other types of collections later.

This patch makes no functional changes.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-provider.h | 112 ++---
 ofproto/ofproto.c  | 237 +
 2 files changed, 208 insertions(+), 141 deletions(-)

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 8a898a5..a82a398 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -449,23 +449,107 @@ void rule_actions_destroy(const struct rule_actions *);
 bool ofproto_rule_has_out_port(const struct rule *, ofp_port_t port)
 OVS_REQUIRES(ofproto_mutex);
 
-/* A set of rules to which an OpenFlow operation applies. */
-struct rule_collection {
-struct rule **rules;/* The rules. */
-size_t n;   /* Number of rules collected. */
+/* A set of ofproto objects to which an OpenFlow operation applies. */
+struct ofproto_collection {
+void **objs;/* Objects. */
+size_t n;   /* Number of objects collected. */
 
-size_t capacity;/* Number of rules that will fit in 'rules'. */
-struct rule *stub[5];   /* Preallocated rules to avoid malloc(). */
+size_t capacity;/* Number of objects that fit in 'objs'. */
+void *stub[5];  /* Preallocated array to avoid malloc(). */
 };
 
-void rule_collection_init(struct rule_collection *);
-void rule_collection_add(struct rule_collection *, struct rule *);
-void rule_collection_remove(struct rule_collection *, struct rule *);
-void rule_collection_move(struct rule_collection *to,
-  struct rule_collection *from);
-void rule_collection_ref(struct rule_collection *) OVS_REQUIRES(ofproto_mutex);
-void rule_collection_unref(struct rule_collection *);
-void rule_collection_destroy(struct rule_collection *);
+void ofproto_collection_init(struct ofproto_collection *);
+void ofproto_collection_add(struct ofproto_collection *, void *);
+void ofproto_collection_remove(struct ofproto_collection *, void *);
+void ofproto_collection_move(struct ofproto_collection *to,
+ struct ofproto_collection *from);
+void *ofproto_collection_detach(struct ofproto_collection *);
+void ofproto_collection_destroy(struct ofproto_collection *);
+
+
+#define DECL_COLLECTION(TYPE, NAME) \
+struct NAME##_collection {  \
+struct ofproto_collection collection;   \
+};  \
+\
+static inline void NAME##_collection_init(struct NAME##_collection *coll) \
+{   \
+ofproto_collection_init(>collection); \
+}   \
+\
+static inline void NAME##_collection_add(struct NAME##_collection *coll, \
+ TYPE obj)  \
+{   \
+ofproto_collection_add(>collection, obj); \
+}   \
+\
+static inline void NAME##_collection_remove(struct NAME##_collection *coll, \
+TYPE obj)   \
+{   \
+ofproto_collection_remove(>collection, obj);  \
+}   \
+\
+static inline void NAME##_collection_move(struct NAME##_collection *to, \
+  struct NAME##_collection *from) \
+{   \
+ofproto_collection_move(>collection, >collection);\
+}   \
+\
+static inline void NAME##_collection_destroy(struct NAME##_collection *coll) \
+{   \
+ofproto_collection_destroy(>collection);  \
+}   \
+\
+static inline TYPE* NAME##_collection_##NAME##s(const struct NAME##_collection 
*coll) \
+{  

[ovs-dev] [PATCH v2 10/26] ofproto: Make groups versioned.

[Jarno Rajahalme]

This is a prepatory step for adding group mod support for bundles in a
following patch.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-dpif-xlate.c |   5 +-
 ofproto/ofproto-dpif.c   |   4 +-
 ofproto/ofproto-dpif.h   |   3 +-
 ofproto/ofproto-provider.h   |  11 +++-
 ofproto/ofproto.c| 150 ++-
 5 files changed, 122 insertions(+), 51 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ca468c6..a847557 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1503,7 +1503,8 @@ group_is_alive(const struct xlate_ctx *ctx, uint32_t 
group_id, int depth)
 {
 struct group_dpif *group;
 
-group = group_dpif_lookup(ctx->xbridge->ofproto, group_id, false);
+group = group_dpif_lookup(ctx->xbridge->ofproto, group_id,
+  ctx->tables_version, false);
 if (group) {
 return group_first_live_bucket(ctx, group, depth) != NULL;
 }
@@ -3611,7 +3612,7 @@ xlate_group_action(struct xlate_ctx *ctx, uint32_t 
group_id)
 
 /* Take ref only if xcache exists. */
 group = group_dpif_lookup(ctx->xbridge->ofproto, group_id,
-  ctx->xin->xcache);
+  ctx->tables_version, ctx->xin->xcache);
 if (!group) {
 /* XXX: Should set ctx->error ? */
 return true;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6fa76c2..0f00cef 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4366,10 +4366,10 @@ group_get_stats(const struct ofgroup *group_, struct 
ofputil_group_stats *ogs)
  * a reference to the group. */
 struct group_dpif *
 group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
-  bool take_ref)
+  ovs_version_t version, bool take_ref)
 {
 struct ofgroup *ofgroup = ofproto_group_lookup(>up, group_id,
-   take_ref);
+   version, take_ref);
 return ofgroup ? group_dpif_cast(ofgroup) : NULL;
 }
 
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 4c6f088..f1e1209 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -137,7 +137,8 @@ void group_dpif_credit_stats(struct group_dpif *,
  struct ofputil_bucket *,
  const struct dpif_flow_stats *);
 struct group_dpif *group_dpif_lookup(struct ofproto_dpif *ofproto,
- uint32_t group_id, bool take_ref);
+ uint32_t group_id, ovs_version_t version,
+ bool take_ref);
 const struct ovs_list *group_dpif_get_buckets(const struct group_dpif *group);
 
 enum ofp11_group_type group_dpif_get_type(const struct group_dpif *group);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 7ad1d16..c6300e7 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -49,6 +49,7 @@
 #include "openvswitch/shash.h"
 #include "simap.h"
 #include "timeval.h"
+#include "versions.h"
 
 struct match;
 struct ofputil_flow_mod;
@@ -584,6 +585,9 @@ void ofproto_rule_reduce_timeouts(struct rule *rule, 
uint16_t idle_timeout,
 struct ofgroup {
 struct cmap_node cmap_node; /* In ofproto's "groups" cmap. */
 
+/* Group versioning. */
+struct versions versions;
+
 /* Number of references.
  *
  * This is needed to keep track of references to the group in the xlate
@@ -597,7 +601,7 @@ struct ofgroup {
 
 /* No lock is needed to protect the fields below since they are not
  * modified after construction. */
-const struct ofproto *ofproto;  /* The ofproto that contains this group. */
+struct ofproto * const ofproto;  /* The ofproto that contains this group. 
*/
 const uint32_t group_id;
 const enum ofp11_group_type type; /* One of OFPGT_*. */
 bool being_deleted;   /* Group removal has begun. */
@@ -615,7 +619,8 @@ struct ofgroup {
 };
 
 struct ofgroup *ofproto_group_lookup(const struct ofproto *ofproto,
-  uint32_t group_id, bool take_ref);
+ uint32_t group_id, ovs_version_t version,
+ bool take_ref);
 
 void ofproto_group_ref(struct ofgroup *);
 bool ofproto_group_try_ref(struct ofgroup *);
@@ -1918,6 +1923,8 @@ struct ofproto_port_mod {
 struct ofproto_group_mod {
 struct ofputil_group_mod gm;
 
+ovs_version_t version;  /* Version in which changes take
+ * effect. */
 struct ofgroup *new_group;  /* New group. */
 struct group_collection old_groups; /* Affected groups. */
 };
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 2037d89..8ebbad0 100644
--- 

[ovs-dev] [PATCH v2 09/26] ofproto: refactor group mods.

[Jarno Rajahalme]

This changes ofproto providers modify_group() to never fail.

Separating major refactoring to a separate patch should make following
patches easier to review.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-dpif.c |   4 +-
 ofproto/ofproto-provider.h |  27 +++-
 ofproto/ofproto.c  | 319 +
 3 files changed, 206 insertions(+), 144 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 53ba3b2..6fa76c2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4328,14 +4328,12 @@ group_destruct(struct ofgroup *group_)
 ovs_mutex_destroy(>stats_mutex);
 }
 
-static enum ofperr
+static void
 group_modify(struct ofgroup *group_)
 {
 struct ofproto_dpif *ofproto = ofproto_dpif_cast(group_->ofproto);
 
 ofproto->backer->need_revalidate = REV_FLOW_TABLE;
-
-return 0;
 }
 
 static enum ofperr
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index a82a398..7ad1d16 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -623,6 +623,23 @@ void ofproto_group_unref(struct ofgroup *);
 
 void ofproto_group_delete_all(struct ofproto *);
 
+DECL_COLLECTION(struct ofgroup *, group)
+
+#define GROUP_COLLECTION_FOR_EACH(GROUP, GROUPS)\
+for (size_t i__ = 0;\
+ i__ < group_collection_n(GROUPS)   \
+ ? (GROUP = group_collection_groups(GROUPS)[i__]) : false;  \
+ i__++)
+
+#define GROUP_COLLECTIONS_FOR_EACH(GROUP1, GROUP2, GROUPS1, GROUPS2)\
+for (size_t i__ = 0;\
+ i__ < group_collection_n(GROUPS1)  \
+ ? ((GROUP1 = group_collection_groups(GROUPS1)[i__]),   \
+(GROUP2 = group_collection_groups(GROUPS2)[i__]))   \
+ : false;   \
+ i__++)
+
+
 /* ofproto class structure, to be defined by each ofproto implementation.
  *
  *
@@ -1857,7 +1874,7 @@ struct ofproto_class {
 void (*group_destruct)(struct ofgroup *);
 void (*group_dealloc)(struct ofgroup *);
 
-enum ofperr (*group_modify)(struct ofgroup *);
+void (*group_modify)(struct ofgroup *);
 
 enum ofperr (*group_get_stats)(const struct ofgroup *,
struct ofputil_group_stats *);
@@ -1897,6 +1914,14 @@ struct ofproto_port_mod {
 struct ofport *port;/* Affected port. */
 };
 
+/* flow_mod with execution context. */
+struct ofproto_group_mod {
+struct ofputil_group_mod gm;
+
+struct ofgroup *new_group;  /* New group. */
+struct group_collection old_groups; /* Affected groups. */
+};
+
 enum ofperr ofproto_flow_mod(struct ofproto *, struct ofproto_flow_mod *)
 OVS_EXCLUDED(ofproto_mutex);
 void ofproto_add_flow(struct ofproto *, const struct match *, int priority,
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 0e8200b..2037d89 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -295,9 +295,6 @@ static void send_buffered_packet(const struct 
openflow_mod_requester *,
 OVS_REQUIRES(ofproto_mutex);
 
 static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id);
-static enum ofperr add_group(struct ofproto *,
- const struct ofputil_group_mod *)
-OVS_REQUIRES(ofproto_mutex);
 static void handle_openflow(struct ofconn *, const struct ofpbuf *);
 static enum ofperr ofproto_flow_mod_start(struct ofproto *,
   struct ofproto_flow_mod *)
@@ -1575,9 +1572,6 @@ ofproto_flush__(struct ofproto *ofproto)
 ovs_mutex_unlock(_mutex);
 }
 
-static void delete_group(struct ofproto *ofproto, uint32_t group_id)
-OVS_REQUIRES(ofproto_mutex);
-
 static void
 ofproto_destroy__(struct ofproto *ofproto)
 OVS_EXCLUDED(ofproto_mutex)
@@ -6534,37 +6528,28 @@ init_group(struct ofproto *ofproto, const struct 
ofputil_group_mod *gm,
  * 'ofproto''s group table.  Returns 0 on success or an OpenFlow error code on
  * failure. */
 static enum ofperr
-add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
+add_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm)
 OVS_REQUIRES(ofproto_mutex)
 {
-struct ofgroup *ofgroup;
 enum ofperr error;
 
-/* Allocate new group and initialize it. */
-error = init_group(ofproto, gm, );
-if (error) {
-return error;
+if (ofproto_group_exists(ofproto, ogm->gm.group_id)) {
+return OFPERR_OFPGMFC_GROUP_EXISTS;
 }
 
-if (ofproto->n_groups[gm->type] >= ofproto->ogf.max_groups[gm->type]) {
-error = OFPERR_OFPGMFC_OUT_OF_GROUPS;
-goto unlock_out;
+if (ofproto->n_groups[ogm->gm.type]
+>= ofproto->ogf.max_groups[ogm->gm.type]) {
+return OFPERR_OFPGMFC_OUT_OF_GROUPS;
 }
 
-  

[ovs-dev] [PATCH v2 07/26] ofproto: Generalize flow_mod_requester.

[Jarno Rajahalme]

Group mods also need a 'requester', so rename 'flow_mod_requester' as
'openflow_mod_requester'.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto.c | 50 --
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 8df93d3..a5d681a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -252,13 +252,13 @@ static void ofproto_rule_insert__(struct ofproto *, 
struct rule *)
 static void ofproto_rule_remove__(struct ofproto *, struct rule *)
 OVS_REQUIRES(ofproto_mutex);
 
-/* The source of a flow_mod request, in the code that processes flow_mods.
+/* The source of an OpenFlow request.
  *
- * A flow table modification request can be generated externally, via OpenFlow,
- * or internally through a function call.  This structure indicates the source
- * of an OpenFlow-generated flow_mod.  For an internal flow_mod, it isn't
- * meaningful and thus supplied as NULL. */
-struct flow_mod_requester {
+ * A table modification request can be generated externally, via OpenFlow, or
+ * internally through a function call.  This structure indicates the source of
+ * an OpenFlow-generated table modification.  For an internal flow_mod, it
+ * isn't meaningful and thus supplied as NULL. */
+struct openflow_mod_requester {
 struct ofconn *ofconn;  /* Connection on which flow_mod arrived. */
 const struct ofp_header *request;
 };
@@ -281,16 +281,16 @@ static void replace_rule_revert(struct ofproto *, struct 
rule *old_rule,
 OVS_REQUIRES(ofproto_mutex);
 
 static void replace_rule_finish(struct ofproto *, struct ofputil_flow_mod *,
-const struct flow_mod_requester *,
+const struct openflow_mod_requester *,
 struct rule *old_rule, struct rule *new_rule,
 struct ovs_list *dead_cookies)
 OVS_REQUIRES(ofproto_mutex);
 static void delete_flows__(struct rule_collection *,
enum ofp_flow_removed_reason,
-   const struct flow_mod_requester *)
+   const struct openflow_mod_requester *)
 OVS_REQUIRES(ofproto_mutex);
 
-static void send_buffered_packet(const struct flow_mod_requester *,
+static void send_buffered_packet(const struct openflow_mod_requester *,
  uint32_t buffer_id, struct rule *)
 OVS_REQUIRES(ofproto_mutex);
 
@@ -304,11 +304,11 @@ static enum ofperr ofproto_flow_mod_start(struct ofproto 
*,
 OVS_REQUIRES(ofproto_mutex);
 static void ofproto_flow_mod_finish(struct ofproto *,
 struct ofproto_flow_mod *,
-const struct flow_mod_requester *)
+const struct openflow_mod_requester *)
 OVS_REQUIRES(ofproto_mutex);
 static enum ofperr handle_flow_mod__(struct ofproto *,
  struct ofproto_flow_mod *,
- const struct flow_mod_requester *)
+ const struct openflow_mod_requester *)
 OVS_EXCLUDED(ofproto_mutex);
 static void calc_duration(long long int start, long long int now,
   uint32_t *sec, uint32_t *nsec);
@@ -4855,7 +4855,7 @@ add_flow_revert(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm)
 /* To be called after version bump. */
 static void
 add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
-const struct flow_mod_requester *req)
+const struct openflow_mod_requester *req)
 OVS_REQUIRES(ofproto_mutex)
 {
 struct ofputil_flow_mod *fm = >fm;
@@ -5016,7 +5016,7 @@ replace_rule_revert(struct ofproto *ofproto,
 /* Adds the 'new_rule', replacing the 'old_rule'. */
 static void
 replace_rule_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-const struct flow_mod_requester *req,
+const struct openflow_mod_requester *req,
 struct rule *old_rule, struct rule *new_rule,
 struct ovs_list *dead_cookies)
 OVS_REQUIRES(ofproto_mutex)
@@ -5183,7 +5183,7 @@ modify_flows_revert(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm)
 
 static void
 modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
-const struct flow_mod_requester *req)
+const struct openflow_mod_requester *req)
 OVS_REQUIRES(ofproto_mutex)
 {
 struct ofputil_flow_mod *fm = >fm;
@@ -5267,7 +5267,7 @@ static void
 delete_flows_finish__(struct ofproto *ofproto,
   struct rule_collection *rules,
   enum ofp_flow_removed_reason reason,
-  const struct flow_mod_requester *req)
+  const struct openflow_mod_requester *req)
 

[ovs-dev] [PATCH v2 04/26] ofproto: Make flow handling more symmetric.

[Jarno Rajahalme]

Remove flow from ofproto data structures in the 'start' phase, even if
we may need to add them back in 'revert' phase.

This makes bundled group mods easier, as a group delete may also
delete flows, and we need the referring flows to be updated in the
'start' phase so that we will not have stale references to the
referring flows.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 83fe3a1..83719a5 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4933,6 +4933,9 @@ replace_rule_start(struct ofproto *ofproto, ovs_version_t 
version,
 if (old_rule) {
 /* Mark the old rule for removal in the next version. */
 cls_rule_make_invisible_in_version(_rule->cr, version);
+
+/* Remove the old rule from data structures. */
+ofproto_rule_remove__(ofproto, old_rule);
 } else {
 table->n_flows++;
 }
@@ -4951,6 +4954,9 @@ static void replace_rule_revert(struct ofproto *ofproto,
 struct oftable *table = >tables[new_rule->table_id];
 
 if (old_rule) {
+/* Restore the old rule to data structures. */
+ofproto_rule_insert__(ofproto, old_rule);
+
 /* Restore the original visibility of the old rule. */
 cls_rule_restore_visibility(_rule->cr);
 } else {
@@ -4992,10 +4998,6 @@ replace_rule_finish(struct ofproto *ofproto, struct 
ofputil_flow_mod *fm,
 if (old_rule) {
 const struct rule_actions *old_actions = rule_get_actions(old_rule);
 
-/* Remove the old rule from data structures.  Removal from the
- * classifier and the deletion of the rule is RCU postponed by the
- * caller. */
-ofproto_rule_remove__(ofproto, old_rule);
 learned_cookies_dec(ofproto, old_actions, dead_cookies);
 
 if (replaced_rule) {
@@ -5209,6 +5211,9 @@ delete_flows_start__(struct ofproto *ofproto, 
ovs_version_t version,
 
 table->n_flows--;
 cls_rule_make_invisible_in_version(>cr, version);
+
+/* Remove rule from ofproto data structures. */
+ofproto_rule_remove__(ofproto, rule);
 }
 }
 
@@ -5236,7 +5241,6 @@ delete_flows_finish__(struct ofproto *ofproto,
 /* Send Vacancy Event for OF1.4+. */
 send_table_status(ofproto, rule->table_id);
 
-ofproto_rule_remove__(ofproto, rule);
 learned_cookies_dec(ofproto, rule_get_actions(rule),
 _cookies);
 }
@@ -5300,6 +5304,9 @@ delete_flows_revert(struct ofproto *ofproto, struct 
ofproto_flow_mod *ofm)
 struct rule *rule = rules->rules[i];
 struct oftable *table = >tables[rule->table_id];
 
+/* Add rule back to ofproto data structures. */
+ofproto_rule_insert__(ofproto, rule);
+
 /* Restore table's rule count. */
 table->n_flows++;
 
-- 
2.1.4

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

[ovs-dev] [PATCH v2 02/26] ofproto: Lockless group lookups.

[Jarno Rajahalme]

Make groups RCU protected and make group lookups lockless.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-dpif-xlate.c |  22 +++---
 ofproto/ofproto-dpif.c   |  27 +++
 ofproto/ofproto-dpif.h   |   7 +-
 ofproto/ofproto-provider.h   |  12 ++--
 ofproto/ofproto.c| 165 +--
 tests/ofproto.at |  16 ++---
 6 files changed, 119 insertions(+), 130 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 3565c70..519e211 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1503,7 +1503,8 @@ group_is_alive(const struct xlate_ctx *ctx, uint32_t 
group_id, int depth)
 {
 struct group_dpif *group;
 
-if (group_dpif_lookup(ctx->xbridge->ofproto, group_id, )) {
+group = group_dpif_lookup(ctx->xbridge->ofproto, group_id);
+if (group) {
 struct ofputil_bucket *bucket;
 
 bucket = group_first_live_bucket(ctx, group, depth);
@@ -1542,7 +1543,7 @@ group_first_live_bucket(const struct xlate_ctx *ctx,
 struct ofputil_bucket *bucket;
 const struct ovs_list *buckets;
 
-group_dpif_get_buckets(group, );
+buckets = group_dpif_get_buckets(group);
 LIST_FOR_EACH (bucket, list_node, buckets) {
 if (bucket_is_alive(ctx, bucket, depth)) {
 return bucket;
@@ -1563,7 +1564,7 @@ group_best_live_bucket(const struct xlate_ctx *ctx,
 struct ofputil_bucket *bucket;
 const struct ovs_list *buckets;
 
-group_dpif_get_buckets(group, );
+buckets = group_dpif_get_buckets(group);
 LIST_FOR_EACH (bucket, list_node, buckets) {
 if (bucket_is_alive(ctx, bucket, 0)) {
 uint32_t score =
@@ -3449,8 +3450,7 @@ xlate_all_group(struct xlate_ctx *ctx, struct group_dpif 
*group)
 struct ofputil_bucket *bucket;
 const struct ovs_list *buckets;
 
-group_dpif_get_buckets(group, );
-
+buckets = group_dpif_get_buckets(group);
 LIST_FOR_EACH (bucket, list_node, buckets) {
 xlate_group_bucket(ctx, bucket);
 }
@@ -3606,14 +3606,13 @@ xlate_group_action(struct xlate_ctx *ctx, uint32_t 
group_id)
 {
 if (xlate_resubmit_resource_check(ctx)) {
 struct group_dpif *group;
-bool got_group;
 
-got_group = group_dpif_lookup(ctx->xbridge->ofproto, group_id, );
-if (got_group) {
-xlate_group_action__(ctx, group);
-} else {
+group = group_dpif_lookup(ctx->xbridge->ofproto, group_id);
+if (!group) {
+/* XXX: Should set ctx->error ? */
 return true;
 }
+xlate_group_action__(ctx, group);
 }
 
 return false;
@@ -4773,6 +4772,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 case OFPACT_GROUP:
 if (xlate_group_action(ctx, ofpact_get_GROUP(a)->group_id)) {
 /* Group could not be found. */
+
+/* XXX: Terminates action list translation, but does not
+ * terminate the pipeline. */
 return;
 }
 break;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fa16af2..681a12e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4279,7 +4279,7 @@ group_construct_stats(struct group_dpif *group)
 group->packet_count = 0;
 group->byte_count = 0;
 
-group_dpif_get_buckets(group, );
+buckets = group_dpif_get_buckets(group);
 LIST_FOR_EACH (bucket, list_node, buckets) {
 bucket->stats.packet_count = 0;
 bucket->stats.byte_count = 0;
@@ -4300,7 +4300,7 @@ group_dpif_credit_stats(struct group_dpif *group,
 } else { /* Credit to all buckets */
 const struct ovs_list *buckets;
 
-group_dpif_get_buckets(group, );
+buckets = group_dpif_get_buckets(group);
 LIST_FOR_EACH (bucket, list_node, buckets) {
 bucket->stats.packet_count += stats->n_packets;
 bucket->stats.byte_count += stats->n_bytes;
@@ -4350,7 +4350,7 @@ group_get_stats(const struct ofgroup *group_, struct 
ofputil_group_stats *ogs)
 ogs->packet_count = group->packet_count;
 ogs->byte_count = group->byte_count;
 
-group_dpif_get_buckets(group, );
+buckets = group_dpif_get_buckets(group);
 bucket_stats = ogs->bucket_stats;
 LIST_FOR_EACH (bucket, list_node, buckets) {
 bucket_stats->packet_count = bucket->stats.packet_count;
@@ -4366,24 +4366,17 @@ group_get_stats(const struct ofgroup *group_, struct 
ofputil_group_stats *ogs)
  *
  * Make sure to call group_dpif_unref() after no longer needing to maintain
  * a reference to the group. */
-bool
-group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
-  struct group_dpif **group)
+struct group_dpif *
+group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id)
 {
-struct ofgroup *ofgroup;
-bool found;
-
-found = ofproto_group_lookup(>up, group_id, );

[ovs-dev] [PATCH v2 01/26] lib: Separate versioning to its own module.

[Jarno Rajahalme]

Separate rule versioning to lib/versions.h to make it easier to use
versioning for other data types.

Signed-off-by: Jarno Rajahalme 
---
 lib/automake.mk  |  1 +
 lib/classifier-private.h | 31 +--
 lib/classifier.c | 45 +--
 lib/classifier.h | 29 +++--
 lib/ovs-router.c |  6 ++--
 lib/tnl-ports.c  |  8 ++---
 lib/versions.h   | 74 
 ofproto/ofproto-dpif-xlate.c |  4 +--
 ofproto/ofproto-dpif.c   | 14 -
 ofproto/ofproto-dpif.h   |  4 +--
 ofproto/ofproto-provider.h   |  6 ++--
 ofproto/ofproto.c| 44 +-
 tests/test-classifier.c  | 46 +--
 tests/test-ovn.c |  4 +--
 utilities/ovs-ofctl.c|  2 +-
 15 files changed, 185 insertions(+), 133 deletions(-)
 create mode 100644 lib/versions.h

diff --git a/lib/automake.mk b/lib/automake.mk
index 4110e5f..f420a9c 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -279,6 +279,7 @@ lib_libopenvswitch_la_SOURCES = \
lib/vconn-provider.h \
lib/vconn-stream.c \
lib/vconn.c \
+   lib/versions.h \
lib/vlan-bitmap.c \
lib/vlan-bitmap.h \
lib/vlog.c \
diff --git a/lib/classifier-private.h b/lib/classifier-private.h
index eb47a4c..1d5ee00 100644
--- a/lib/classifier-private.h
+++ b/lib/classifier-private.h
@@ -72,13 +72,8 @@ struct cls_match {
 /* Accessed by all readers. */
 struct cmap_node cmap_node; /* Within struct cls_subtable 'rules'. */
 
-/* Rule versioning.
- *
- * CLS_NOT_REMOVED_VERSION has a special meaning for 'remove_version',
- * meaning that the rule has been added but not yet removed.
- */
-const cls_version_t add_version;/* Version rule was added in. */
-ATOMIC(cls_version_t) remove_version;   /* Version rule is removed in. */
+/* Rule versioning. */
+struct versions versions;
 
 const struct cls_rule *cls_rule;
 const struct miniflow flow; /* Matching rule. Mask is in the subtable. */
@@ -102,34 +97,22 @@ get_cls_match(const struct cls_rule *rule)
 void cls_match_free_cb(struct cls_match *);
 
 static inline void
-cls_match_set_remove_version(struct cls_match *rule, cls_version_t version)
+cls_match_set_remove_version(struct cls_match *rule, ovs_version_t version)
 {
-atomic_store_relaxed(>remove_version, version);
+versions_set_remove_version(>versions, version);
 }
 
 static inline bool
 cls_match_visible_in_version(const struct cls_match *rule,
- cls_version_t version)
+ ovs_version_t version)
 {
-cls_version_t remove_version;
-
-/* C11 does not want to access an atomic via a const object pointer. */
-atomic_read_relaxed(_CAST(struct cls_match *, rule)->remove_version,
-_version);
-
-return rule->add_version <= version && version < remove_version;
+return versions_visible_in_version(>versions, version);
 }
 
 static inline bool
 cls_match_is_eventually_invisible(const struct cls_match *rule)
 {
-cls_version_t remove_version;
-
-/* C11 does not want to access an atomic via a const object pointer. */
-atomic_read_relaxed(_CAST(struct cls_match *, rule)->remove_version,
-_version);
-
-return remove_version <= CLS_MAX_VERSION;
+return versions_is_eventually_invisible(>versions);
 }
 
 
diff --git a/lib/classifier.c b/lib/classifier.c
index 2b24724..d5fd759 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -84,7 +84,7 @@ cls_conjunction_set_alloc(struct cls_match *match,
 }
 
 static struct cls_match *
-cls_match_alloc(const struct cls_rule *rule, cls_version_t version,
+cls_match_alloc(const struct cls_rule *rule, ovs_version_t version,
 const struct cls_conjunction conj[], size_t n)
 {
 size_t count = miniflow_n_values(rule->match.flow);
@@ -95,9 +95,8 @@ cls_match_alloc(const struct cls_rule *rule, cls_version_t 
version,
 ovsrcu_init(_match->next, NULL);
 *CONST_CAST(const struct cls_rule **, _match->cls_rule) = rule;
 *CONST_CAST(int *, _match->priority) = rule->priority;
-*CONST_CAST(cls_version_t *, _match->add_version) = version;
-atomic_init(_match->remove_version, version);   /* Initially
- * invisible. */
+/* Make rule initially invisible. */
+cls_match->versions = VERSIONS_INITIALIZER(version, version);
 miniflow_clone(CONST_CAST(struct miniflow *, _match->flow),
rule->match.flow, count);
 ovsrcu_set_hidden(_match->conj_set,
@@ -113,7 +112,7 @@ static struct cls_subtable *insert_subtable(struct 
classifier *cls,
 static void destroy_subtable(struct classifier *cls, struct cls_subtable *);
 
 static const struct cls_match *find_match_wc(const struct cls_subtable *,
-

[ovs-dev] [PATCH v2 03/26] ofproto: Take group references only when needed.

[Jarno Rajahalme]

Avoid unnecessary references when RCU protection suffices.  This makes
group lookup memory management more like flow lookup memory
management.

Signed-off-by: Jarno Rajahalme 
---
 ofproto/ofproto-dpif-xlate.c | 22 +-
 ofproto/ofproto-dpif.c   |  6 --
 ofproto/ofproto-dpif.h   |  2 +-
 ofproto/ofproto-provider.h   |  2 +-
 ofproto/ofproto.c|  7 ---
 5 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 519e211..ca468c6 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1503,13 +1503,9 @@ group_is_alive(const struct xlate_ctx *ctx, uint32_t 
group_id, int depth)
 {
 struct group_dpif *group;
 
-group = group_dpif_lookup(ctx->xbridge->ofproto, group_id);
+group = group_dpif_lookup(ctx->xbridge->ofproto, group_id, false);
 if (group) {
-struct ofputil_bucket *bucket;
-
-bucket = group_first_live_bucket(ctx, group, depth);
-group_dpif_unref(group);
-return bucket != NULL;
+return group_first_live_bucket(ctx, group, depth) != NULL;
 }
 
 return false;
@@ -3377,6 +3373,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t 
in_port, uint8_t table_id,
 }
 }
 
+/* Consumes the group reference, which is only taken if xcache exists. */
 static void
 xlate_group_stats(struct xlate_ctx *ctx, struct group_dpif *group,
   struct ofputil_bucket *bucket)
@@ -3388,7 +3385,7 @@ xlate_group_stats(struct xlate_ctx *ctx, struct 
group_dpif *group,
 struct xc_entry *entry;
 
 entry = xlate_cache_add_entry(ctx->xin->xcache, XC_GROUP);
-entry->u.group.group = group_dpif_ref(group);
+entry->u.group.group = group;
 entry->u.group.bucket = bucket;
 }
 }
@@ -3466,6 +3463,8 @@ xlate_ff_group(struct xlate_ctx *ctx, struct group_dpif 
*group)
 if (bucket) {
 xlate_group_bucket(ctx, bucket);
 xlate_group_stats(ctx, group, bucket);
+} else if (ctx->xin->xcache) {
+group_dpif_unref(group);
 }
 }
 
@@ -3482,6 +3481,8 @@ xlate_default_select_group(struct xlate_ctx *ctx, struct 
group_dpif *group)
 if (bucket) {
 xlate_group_bucket(ctx, bucket);
 xlate_group_stats(ctx, group, bucket);
+} else if (ctx->xin->xcache) {
+group_dpif_unref(group);
 }
 }
 
@@ -3551,6 +3552,8 @@ xlate_hash_fields_select_group(struct xlate_ctx *ctx, 
struct group_dpif *group)
 if (bucket) {
 xlate_group_bucket(ctx, bucket);
 xlate_group_stats(ctx, group, bucket);
+} else if (ctx->xin->xcache) {
+group_dpif_unref(group);
 }
 }
 
@@ -3596,7 +3599,6 @@ xlate_group_action__(struct xlate_ctx *ctx, struct 
group_dpif *group)
 default:
 OVS_NOT_REACHED();
 }
-group_dpif_unref(group);
 
 ctx->in_group = was_in_group;
 }
@@ -3607,7 +3609,9 @@ xlate_group_action(struct xlate_ctx *ctx, uint32_t 
group_id)
 if (xlate_resubmit_resource_check(ctx)) {
 struct group_dpif *group;
 
-group = group_dpif_lookup(ctx->xbridge->ofproto, group_id);
+/* Take ref only if xcache exists. */
+group = group_dpif_lookup(ctx->xbridge->ofproto, group_id,
+  ctx->xin->xcache);
 if (!group) {
 /* XXX: Should set ctx->error ? */
 return true;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 681a12e..53ba3b2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4367,9 +4367,11 @@ group_get_stats(const struct ofgroup *group_, struct 
ofputil_group_stats *ogs)
  * Make sure to call group_dpif_unref() after no longer needing to maintain
  * a reference to the group. */
 struct group_dpif *
-group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id)
+group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
+  bool take_ref)
 {
-struct ofgroup *ofgroup = ofproto_group_lookup(>up, group_id);
+struct ofgroup *ofgroup = ofproto_group_lookup(>up, group_id,
+   take_ref);
 return ofgroup ? group_dpif_cast(ofgroup) : NULL;
 }
 
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 18a90b2..4c6f088 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -137,7 +137,7 @@ void group_dpif_credit_stats(struct group_dpif *,
  struct ofputil_bucket *,
  const struct dpif_flow_stats *);
 struct group_dpif *group_dpif_lookup(struct ofproto_dpif *ofproto,
- uint32_t group_id);
+ uint32_t group_id, bool take_ref);
 const struct ovs_list *group_dpif_get_buckets(const struct group_dpif *group);
 
 enum ofp11_group_type group_dpif_get_type(const struct group_dpif *group);
diff --git a/ofproto/ofproto-provider.h 

[ovs-dev] [PATCH v2 00/26] Support groups in bundles.

[Jarno Rajahalme]

This series adds group support for OpenFlow bundles, and reduces the
memory footprint of bundled messages.

To make groups in bundles useful we need to make groups versioned, so
that atomic bundles are supported, which in turn requires RCU to be
used for group's memory management.  We also need the locking to be
compatible with bundles (use ofproto_mutex).  Since we don't want to
hold this mutex for excessive periods of time, we need to keep
reference to the referring rules explicitly, so that we do not need to
do full classifier iterations to find the rules.

Handling of flow mods needs also change to check the existence of the
group referred by an group action to the bundle commit execution time,
as the group may have been just inserted by an earlier message in the
same bundle.

To make groups in bundles practical we also have to reduce group's
memory footprint, which is currently more than 20kb due to an large
array of large mf_values.

Finally, this series also refactors the way flow mods are managed in
bundles to reduce the memory footprint of bundle build-up and
execution.  The main strategy here is to create the new rule at the
time of flow_mod's insertion to the bundle, and then get rid of the
relatively large ofputil_flow_mod.  To this end the initial error
checking is split between the new 'init' and old 'start' phases.
Since the affected 'old' rules can only be collected at the 'start'
phase, the initially created rule may need to be modified later to
implement 'modyfy' semantics.

The series also includes some bug fixes:

 - The 'used' timestamp was not forwarded from the replaced rule when
   'reset_counts' flag was specified.  Earlier we failed to reset the
   counts on OF1.0 and OF1.1 where the behavior is implied, but the
   flag is not present in the ofputil_flow_mod.
 - We failed to report the counts of flow mods from bundles.

Jarno Rajahalme (26):
  lib: Separate versioning to its own module.
  ofproto: Lockless group lookups.
  ofproto: Take group references only when needed.
  ofproto: Make flow handling more symmetric.
  ofproto: Use ofproto_mutex for groups and keep track of referring
flows.
  ofproto: Add generic ofproto_collection.
  ofproto: Generalize flow_mod_requester.
  ofproto: Report flow mods also from bundles.
  ofproto: refactor group mods.
  ofproto: Make groups versioned.
  vconn: Update length of bundled messages.
  vconn: Better bundle error management.
  ofproto-dpif: Always forward 'used' from the old_rule.
  meta-flow: Add byte access to struct mf_value.
  meta-flow: Add mf_mask_field_masked().
  meta-flow: Clean up masking with prerequisities checking.
  ofproto-dpif-xlate: Hash only fields specified for 'hash' selection
method.
  ofp-util: Do not free() field that is not allocated.
  ofproto: Use ofputil_uninit_group_mod().
  meta-flow: Compact struct field_array.
  ofproto: Support group mods in bundles.
  ofp-util: remove flow mod's delete_reason.
  ofproto: Reduce dependency on ofputil_flow_mod after rule has been
created.
  ofproto: Add 'modify_cookie' to ofproto_flow_mod.
  ofproto: Add 'command' to ofproto_flow_mod.
  ofproto: Reduce bundle memory use.

 NEWS  |7 +-
 include/openvswitch/meta-flow.h   |   21 +-
 include/openvswitch/ofp-actions.h |   40 +
 include/openvswitch/ofp-parse.h   |   14 +-
 include/openvswitch/ofp-util.h|   22 +-
 include/openvswitch/vconn.h   |   12 +-
 lib/automake.mk   |1 +
 lib/classifier-private.h  |   31 +-
 lib/classifier.c  |   47 +-
 lib/classifier.h  |   29 +-
 lib/dpctl.c   |4 +-
 lib/flow.h|   45 +
 lib/learn.c   |1 -
 lib/meta-flow.c   |  178 ++--
 lib/nx-match.c|   58 +-
 lib/ofp-actions.c |2 +-
 lib/ofp-parse.c   |  192 -
 lib/ofp-print.c   |4 +-
 lib/ofp-util.c|   57 +-
 lib/ovs-router.c  |6 +-
 lib/tnl-ports.c   |8 +-
 lib/vconn.c   |  133 ++-
 lib/versions.h|   74 ++
 ofproto/bundles.c |2 +-
 ofproto/bundles.h |   12 +-
 ofproto/ofproto-dpif-xlate.c  |  132 ++-
 ofproto/ofproto-dpif.c|  111 +--
 ofproto/ofproto-dpif.h|   12 +-
 ofproto/ofproto-provider.h|  229 -
 ofproto/ofproto.c | 1723 +++--
 ovn/controller/ofctrl.c   |6 +-
 tests/ofproto-dpif.at |2 +-
 tests/ofproto-macros.at   |2 +
 tests/ofproto.at  |  468 +-
 tests/ovs-ofctl.at|4 +
 tests/test-classifier.c   |   46 +-
 tests/test-odp.c  |2 +-
 tests/test-ovn.c  |4 +-
 utilities/ovs-ofctl.8.in  |   56 +-
 utilities/ovs-ofctl.c 

[ovs-dev] [PATCH v2] tests: Ignore proxy configuration.

[Jarno Rajahalme]

As any proxy configuration may ruin kernel testsuite tests, it is
better to ignore all proxy configuration.

Suggested-by: Ben Pfaff 
Signed-off-by: Jarno Rajahalme 
---
 tests/atlocal.in| 10 ++
 tests/system-traffic.at |  8 
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index f174061..55070d8 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -129,3 +129,13 @@ if nc --version 2>&1 | grep -q nmap.org; then
 else
 NC_EOF_OPT="-q 1"
 fi
+
+# Turn off proxies.
+unset http_proxy
+unset https_proxy
+unset ftp_proxy
+unset no_proxy
+unset HTTP_PROXY
+unset HTTPS_PROXY
+unset FTP_PROXY
+unset NO_PROXY
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 1cdc2d2..5d839d1 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1509,7 +1509,7 @@ NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py 
ftp]], [ftp0.pid])
 OVS_WAIT_UNTIL([ip netns exec at_ns1 netstat -l | grep ftp])
 
 dnl FTP requests from p0->p1 should work fine.
-NS_CHECK_EXEC([at_ns0], [wget ftp://[[fc00::2]] -6 --no-passive-ftp -t 3 -T 1 
--retry-connrefused -v --server-response --no-proxy --no-remove-listing -o 
wget0.log -d])
+NS_CHECK_EXEC([at_ns0], [wget ftp://[[fc00::2]] -6 --no-passive-ftp -t 3 -T 1 
--retry-connrefused -v --server-response --no-remove-listing -o wget0.log -d])
 
 dnl Discards CLOSE_WAIT and CLOSING
 AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl
@@ -2343,7 +2343,7 @@ NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py 
ftp]], [ftp0.pid])
 OVS_WAIT_UNTIL([ip netns exec at_ns1 netstat -l | grep ftp])
 
 dnl FTP requests from p0->p1 should work fine.
-NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 -4 --no-passive-ftp -t 3 -T 1 
--retry-connrefused -v --server-response --no-proxy --no-remove-listing -o 
wget0.log -d])
+NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 -4 --no-passive-ftp -t 3 -T 1 
--retry-connrefused -v --server-response --no-remove-listing -o wget0.log -d])
 
 AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
 
tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.240,sport=,dport=),protoinfo=(state=),helper=ftp
@@ -2411,7 +2411,7 @@ NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py 
ftp]], [ftp0.pid])
 OVS_WAIT_UNTIL([ip netns exec at_ns1 netstat -l | grep ftp])
 
 dnl FTP requests from p0->p1 should work fine.
-NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 -4 --no-passive-ftp -t 3 -T 1 
--retry-connrefused -v --server-response --no-proxy --no-remove-listing -o 
wget0.log -d])
+NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 -4 --no-passive-ftp -t 3 -T 1 
--retry-connrefused -v --server-response --no-remove-listing -o wget0.log -d])
 
 dnl Discards CLOSE_WAIT and CLOSING
 AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
@@ -2511,7 +2511,7 @@ NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py 
ftp]], [ftp0.pid])
 OVS_WAIT_UNTIL([ip netns exec at_ns1 netstat -l | grep ftp])
 
 dnl FTP requests from p0->p1 should work fine.
-NS_CHECK_EXEC([at_ns0], [wget ftp://[[fc00::2]] -6 --no-passive-ftp -t 3 -T 1 
--retry-connrefused -v --server-response --no-proxy --no-remove-listing -o 
wget0.log -d])
+NS_CHECK_EXEC([at_ns0], [wget ftp://[[fc00::2]] -6 --no-passive-ftp -t 3 -T 1 
--retry-connrefused -v --server-response --no-remove-listing -o wget0.log -d])
 
 dnl Discards CLOSE_WAIT and CLOSING
 AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl
-- 
2.1.4

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

[ovs-dev] [PATCH 2/2] fedora: Prioritize OVS modules in weak-updates.

[Joe Stringer]

Out-of-tree modules are installed into the kernel's "extra" modules
directory for the version that kmod-openvswitch is compiled against. For
all other kernels on the system at install time, a symlink is created in
the "weak-updates" directory. This provides a path for the same kernel
module to be used when minor kernel updates are done on a system.
However, without updating the depmod configuration the weak-update will
not be prioritized, so modprobe will switch back to using upstream
kernel modules when you upgrade. This patch introduces that depmod
configuration to ensure that the out-of-tree module is always used when
it is installed, regardless of kernel upgrades.

Signed-off-by: Joe Stringer 
---
 rhel/openvswitch-kmod-fedora.spec.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rhel/openvswitch-kmod-fedora.spec.in 
b/rhel/openvswitch-kmod-fedora.spec.in
index ea89d15c8d87..93adb0330cbb 100644
--- a/rhel/openvswitch-kmod-fedora.spec.in
+++ b/rhel/openvswitch-kmod-fedora.spec.in
@@ -48,6 +48,8 @@ do
 modname="$(basename ${module})"
 echo "override ${modname%.ko} * extra" >> \
 $RPM_BUILD_ROOT/etc/depmod.d/kmod-openvswitch.conf
+echo "override ${modname%.ko} * weak-updates" >> \
+$RPM_BUILD_ROOT/etc/depmod.d/kmod-openvswitch.conf
 done
 
 %clean
-- 
2.9.0

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

[ovs-dev] [PATCH 1/2] rhel: Prioritize our vport-foo modules in depmod.

[Joe Stringer]

We've done the same for openvswitch.ko previously, but we really should
be doing this for vport modules as well; otherwise, depmod may try to
pair upstream vport modules with the out-of-tree openvswitch module
(leading to depmod warnings on package install, and failure to load the
module at runtime).

VMware-BZ: #1700293
Signed-off-by: Joe Stringer 
---
 rhel/openvswitch-kmod-rhel6.spec.in | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/rhel/openvswitch-kmod-rhel6.spec.in 
b/rhel/openvswitch-kmod-rhel6.spec.in
index 5d46838a9ed5..82a3312b100f 100644
--- a/rhel/openvswitch-kmod-rhel6.spec.in
+++ b/rhel/openvswitch-kmod-rhel6.spec.in
@@ -43,10 +43,6 @@ Open vSwitch Linux kernel module.
 %prep
 
 %setup -n %{oname}-%{version}
-cat > %{oname}.conf << EOF
-override %{oname} * extra/%{oname}
-override %{oname} * weak-updates/%{oname}
-EOF
 
 %build
 for flavor in %flavors_to_build; do
@@ -66,7 +62,17 @@ for flavor in %flavors_to_build ; do
  find $INSTALL_MOD_PATH/lib/modules -iname 'modules.*' -exec rm {} \;
 done
 install -d %{buildroot}%{_sysconfdir}/depmod.d/
+for module in %{buildroot}/lib/modules/%{kernel_version}/$INSTALL_MOD_DIR/*.ko;
+do
+modname="$(basename ${module})"
+echo "override ${modname%.ko} * extra/${oname}" >> %{oname}.conf
+echo "override ${modname%.ko} * weak-updates/${oname}" >> %{oname}.conf
+done
 install -m 644 %{oname}.conf %{buildroot}%{_sysconfdir}/depmod.d/
 
+%files
+%defattr(644,root,root)
+/etc/depmod.d/openvswitch.conf
+
 %clean
 rm -rf $RPM_BUILD_ROOT
-- 
2.9.0

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

[ovs-dev] [PATCH v3] ovn-controller: Persist desired conntrack groups.

[Ryan Moats]

With incremental processing of logical flows desired conntrack groups
are not being persisted.  This patch adds this capability, with the
side effect of adding a ds_clone method that this capability leverages.

Signed-off-by: Ryan Moats 
Reported-by: Guru Shetty 
Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
Fixes: 70c7cfe ("ovn-controller: Add incremental processing to lflow_run and 
physical_run")
---
 include/openvswitch/dynamic-string.h |  1 +
 include/ovn/actions.h|  6 +
 lib/dynamic-string.c |  9 
 ovn/controller/lflow.c   |  2 ++
 ovn/controller/ofctrl.c  | 43 
 ovn/controller/ofctrl.h  |  5 -
 ovn/controller/ovn-controller.c  |  2 +-
 ovn/lib/actions.c|  1 +
 8 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/include/openvswitch/dynamic-string.h 
b/include/openvswitch/dynamic-string.h
index dfe2688..bf1f64a 100644
--- a/include/openvswitch/dynamic-string.h
+++ b/include/openvswitch/dynamic-string.h
@@ -73,6 +73,7 @@ void ds_swap(struct ds *, struct ds *);
 
 int ds_last(const struct ds *);
 bool ds_chomp(struct ds *, int c);
+void ds_clone(struct ds *, struct ds *);
 
 /* Inline functions. */
 
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 114c71e..55720ce 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -22,7 +22,9 @@
 #include "compiler.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/dynamic-string.h"
+#include "openvswitch/uuid.h"
 #include "util.h"
+#include "uuid.h"
 
 struct expr;
 struct lexer;
@@ -43,6 +45,7 @@ struct group_table {
 struct group_info {
 struct hmap_node hmap_node;
 struct ds group;
+struct uuid lflow_uuid;
 uint32_t group_id;
 };
 
@@ -107,6 +110,9 @@ struct action_params {
 /* A struct to figure out the group_id for group actions. */
 struct group_table *group_table;
 
+/* The logical flow uuid that drove this action. */
+struct uuid lflow_uuid;
+
 /* OVN maps each logical flow table (ltable), one-to-one, onto a physical
  * OpenFlow flow table (ptable).  A number of parameters describe this
  * mapping and data related to flow tables:
diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
index 1f17a9f..6f7b610 100644
--- a/lib/dynamic-string.c
+++ b/lib/dynamic-string.c
@@ -456,3 +456,12 @@ ds_chomp(struct ds *ds, int c)
 return false;
 }
 }
+
+void
+ds_clone(struct ds *dst, struct ds *source)
+{
+dst->length = source->length;
+dst->allocated = dst->length;
+dst->string = xmalloc(dst->allocated + 1);
+memcpy(dst->string, source->string, dst->allocated + 1);
+}
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index a4f3322..e38c32a 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -383,6 +383,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct 
lport_index *lports,
 
 if (full_flow_processing) {
 ovn_flow_table_clear();
+ovn_group_table_clear(group_table, false);
 full_logical_flow_processing = true;
 full_neighbor_flow_processing = true;
 full_flow_processing = false;
@@ -515,6 +516,7 @@ consider_logical_flow(const struct lport_index *lports,
 .aux = ,
 .ct_zones = ct_zones,
 .group_table = group_table,
+.lflow_uuid = lflow->header_.uuid,
 
 .n_tables = LOG_PIPELINE_LEN,
 .first_ptable = first_ptable,
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index dd9f5ec..54bea99 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -140,8 +140,6 @@ static ovs_be32 queue_msg(struct ofpbuf *);
 static void ovn_flow_table_destroy(void);
 static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
 
-static void ovn_group_table_clear(struct group_table *group_table,
-  bool existing);
 static struct ofpbuf *encode_group_mod(const struct ofputil_group_mod *);
 
 static void ofctrl_recv(const struct ofp_header *, enum ofptype);
@@ -150,12 +148,15 @@ static struct hmap match_flow_table = 
HMAP_INITIALIZER(_flow_table);
 static struct hindex uuid_flow_table = HINDEX_INITIALIZER(_flow_table);
 
 void
-ofctrl_init(void)
+ofctrl_init(struct group_table *group_table)
 {
 swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
 tx_counter = rconn_packet_counter_create();
 hmap_init(_flows);
 ovs_list_init(_updates);
+if (!groups) {
+groups = group_table;
+}
 }
 
 /* S_NEW, for a new connection.
@@ -680,6 +681,16 @@ ofctrl_remove_flows(const struct uuid *uuid)
 ovn_flow_destroy(f);
 }
 }
+
+/* Remove any group_info information created by this logical flow. */
+struct group_info *g, *next_g;
+HMAP_FOR_EACH_SAFE (g, next_g, hmap_node, >desired_groups) {
+if 

Re: [ovs-dev] [PATCH] ovn-controller: Remove old values from local_ids.

[Ryan Moats]

"dev"  wrote on 07/28/2016 04:22:41 PM:

> From: Russell Bryant 
> To: dev@openvswitch.org
> Date: 07/28/2016 04:23 PM
> Subject: [ovs-dev] [PATCH] ovn-controller: Remove old values from
local_ids.
> Sent by: "dev" 
>
> local_ids is supposed to be the set of interface iface-id values from
> this chassis that correspond to OVN logical ports.  We use this for
> detecting when an interface has been removed as well as if child-ports
> should be bound to this chassis.
>
> Old values were not being removed from local_ids.  The most immediate
> effect of this was that once an interface has been removed from a
> chassis, we would think a removal has occured *every* time through
> binding_run and trigger the full binding processing.  This was
> a performance problem.
>
> The second problem this would cause is if a port that had child ports
> was moved to another chassis.  We would end up with two chassis fighting
> over the binding of the child ports.
>
> Signed-off-by: Russell Bryant 
> ---
>  ovn/controller/binding.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index 78ebec4..41165bc 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -103,6 +103,11 @@ get_local_iface_ids(const struct ovsrec_bridge
*br_int,
>   * that has been removed. */
>  if (!changed && !sset_is_empty(_local_ids)) {
>  changed = true;
> +
> +const char *cur_id;
> +SSET_FOR_EACH(cur_id, _local_ids) {
> +sset_find_and_delete(_ids, cur_id);
> +}
>  }
>
>  sset_destroy(_local_ids);
> --
> 2.7.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

lgtm...

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

Re: [ovs-dev] [PATCH v2] ovn-controller: Persist desired conntrack groups.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 05:10:56PM -0500, Ryan Moats wrote:
> Ben Pfaff  wrote on 07/28/2016 04:06:09 PM:
> 
> > From: Ben Pfaff 
> > To: Ryan Moats/Omaha/IBM@IBMUS, Gurucharan Shetty 
> > Cc: dev@openvswitch.org
> > Date: 07/28/2016 04:06 PM
> > Subject: Re: [ovs-dev] [PATCH v2] ovn-controller: Persist desired
> > conntrack groups.
> >
> > On Thu, Jul 28, 2016 at 02:18:45PM +, Ryan Moats wrote:
> > > With incremental processing of logical flows desired conntrack groups
> > > are not being persisted.  This patch adds this capability, with the
> > > side effect of adding a ds_clone method that this capability leverages.
> > >
> > > Signed-off-by: Ryan Moats 
> > > Reported-by: Guru Shetty 
> > > Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
> > > Fixes: 70c7cfe ("ovn-controller: Add incremental processing to
> > lflow_run and physical_run")
> > > ---
> > >  v1->v2 addressed review comments
> > >updated commit message
> > >changed name of ds_copy to ds_clone
> > >moved lflow uuid storage to action_params for cleaner code
> >
> > It seems odd that group_clone() doesn't copy the UUID.
> >
> > I'm not sure why group_info's 'group' member is a struct ds instead of
> > just a char *.  (This isn't anything that you introduced.)
> >
> > Coding style:
> > > +static struct group_info *
> > > +group_info_clone(struct group_info *source) {
> >
> > But I'll hold off on further review since Guru reports that this causes
> > test failures in the system tests.
> >
> 
> If you want to hit me with the rest of the style comments, I have the
> crash issue fixed and I can fold the style into v3...

That was the style comment.  The { should be on its own line.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn-controller: squelch expected duplicate flow warnings

[Ryan Moats]

Ben Pfaff  wrote on 07/28/2016 04:28:31 PM:

> From: Ben Pfaff 
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: ovs dev , Guru Shetty 
> Date: 07/28/2016 04:29 PM
> Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected
> duplicate flow warnings
>
> On Wed, Jul 27, 2016 at 09:13:56PM -0500, Ryan Moats wrote:
> >
> >
> > Ben Pfaff  wrote on 07/27/2016 03:53:56 PM:
> >
> > > From: Ben Pfaff 
> > > To: Guru Shetty 
> > > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev 
> > > Date: 07/27/2016 03:54 PM
> > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected
> > > duplicate flow warnings
> > >
> > > On Tue, Jul 26, 2016 at 01:54:29PM -0700, Guru Shetty wrote:
> > > > On 24 July 2016 at 10:07, Ryan Moats  wrote:
> > > >
> > > > > In the physical processing of ovn-controller, there are two
> > > > > sets of OF flows that are still fully recalculated every cycle:
> > > > >
> > > > >   Flows that aren't associated with any logical flow, and
> > > > >   Flows calculated based on multicast groups
> > > > >
> > > > > Because these flows are recalculated fully each cycle, full
> > > > > duplicates of existing OF flows are created and the OF management
> > > > > code in ovn-controller pollutes the logs with false positive
> > > > > warnings about repeated duplicates.
> > > > >
> > > > > As a short term measure, ignore full duplicates for both of
> > > > > these types of flows, but still warn if the action changes
> > > > > (as that is not expected and may be indicative of a problem).
> > > > >
> > > > > Signed-off-by: Ryan Moats 
> > > > >
> > > >
> > > > I also noticed that "commit 70c7cfef188b5ae9940abd5
(ovn-controller:
> > Add
> > > > incremental processing to lflow_run and physical_run)" causes load
> > > > balancing system unit tests to fail. A little debugging shows that
> > groups
> > > > are getting deleted when new flows are added.  My hunch is that
this is
> > > > likely because 'desired_groups' in ofctl_put gets deleted in every
run.
> > But
> > > > in the next run, it does not get updated as we no longer process
all
> > flows.
> > >
> > > It's unclear to me from the discussion of this patch whether it's
> > > beneficial.  Can someone clarify?
> > >
> > > Thanks,
> > >
> > > Ben.
> >
> > Unfortunately, I don't think we maintained reasonable email thread
> > discipline
> > on this discussion - for me, the vast majority of the conversation was
> > about
> > the CPU cycle bug that Guru mentions and not the patch itself.
> >
> > The conditions stated in the commit message still apply even after
fixing
> > Guru's issue and so I still see the squelching as being useful to avoid
> > polluting the ovn-controller logs with false positive messages...
>
> I think that you said in IRC that you're dropping this patch.  Let me
> know if I'm wrong.
>
> Thanks,
>
> Ben.

Yes, I've marked this as not applicable because I'm dropping it - it is
not necessary.

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

Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.

[Ryan Moats]

Ben Pfaff  wrote on 07/28/2016 04:23:57 PM:

> From: Ben Pfaff 
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 07/28/2016 04:24 PM
> Subject: Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created
> for now deleted SB database rows.
>
> On Thu, Jul 28, 2016 at 07:54:30PM +, Ryan Moats wrote:
> > Ensure that rows created for deleted port binding and
> > multicast group rows are cleared when doing full processing.
> >
> > Signed-off-by: Ryan Moats 
>
> I'm choosing to overlook storing UUIDs as strings in a set of strings.
>
> How about this simplification?
>
> --8<--cut here-->8--
>
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index 6bc0095..a4a8fcf 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -598,25 +598,22 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
>  sset_destroy(_chassis);
>  }
>
> +/* Deletes the flows whose UUIDs are in 'old' but not 'new', and
> then replaces
> + * 'old' by 'new'. */
>  static void
>  rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new)
>  {
> -const char *uuid_s, *next_uuid;
> -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) {
> +const char *uuid_s;
> +SSET_FOR_EACH (uuid_s, old) {
>  if (!sset_find(new, uuid_s)) {
>  struct uuid uuid;
>  if (uuid_from_string(, uuid_s)) {
>  ofctrl_remove_flows();
>  }
> -sset_find_and_delete(old, uuid_s);
>  }
>  }
> -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) {
> -if (!sset_find(old, uuid_s)) {
> -sset_add(old, uuid_s);
> -}
> -sset_find_and_delete(new, uuid_s);
> -}
> +sset_swap(old, new);
> +sset_clear(new);
>  }
>
>  void
>

Works for me... Ryan
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] ovn-controller: Persist desired conntrack groups.

[Ryan Moats]

Ben Pfaff  wrote on 07/28/2016 04:06:09 PM:

> From: Ben Pfaff 
> To: Ryan Moats/Omaha/IBM@IBMUS, Gurucharan Shetty 
> Cc: dev@openvswitch.org
> Date: 07/28/2016 04:06 PM
> Subject: Re: [ovs-dev] [PATCH v2] ovn-controller: Persist desired
> conntrack groups.
>
> On Thu, Jul 28, 2016 at 02:18:45PM +, Ryan Moats wrote:
> > With incremental processing of logical flows desired conntrack groups
> > are not being persisted.  This patch adds this capability, with the
> > side effect of adding a ds_clone method that this capability leverages.
> >
> > Signed-off-by: Ryan Moats 
> > Reported-by: Guru Shetty 
> > Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
> > Fixes: 70c7cfe ("ovn-controller: Add incremental processing to
> lflow_run and physical_run")
> > ---
> >  v1->v2 addressed review comments
> >updated commit message
> >changed name of ds_copy to ds_clone
> >moved lflow uuid storage to action_params for cleaner code
>
> It seems odd that group_clone() doesn't copy the UUID.
>
> I'm not sure why group_info's 'group' member is a struct ds instead of
> just a char *.  (This isn't anything that you introduced.)
>
> Coding style:
> > +static struct group_info *
> > +group_info_clone(struct group_info *source) {
>
> But I'll hold off on further review since Guru reports that this causes
> test failures in the system tests.
>

If you want to hit me with the rest of the style comments, I have the
crash issue fixed and I can fold the style into v3...

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

Re: [ovs-dev] [PATCH] ovn-controller: squelch expected duplicate flow warnings

[Ben Pfaff]

On Wed, Jul 27, 2016 at 09:13:56PM -0500, Ryan Moats wrote:
> 
> 
> Ben Pfaff  wrote on 07/27/2016 03:53:56 PM:
> 
> > From: Ben Pfaff 
> > To: Guru Shetty 
> > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev 
> > Date: 07/27/2016 03:54 PM
> > Subject: Re: [ovs-dev] [PATCH] ovn-controller: squelch expected
> > duplicate flow warnings
> >
> > On Tue, Jul 26, 2016 at 01:54:29PM -0700, Guru Shetty wrote:
> > > On 24 July 2016 at 10:07, Ryan Moats  wrote:
> > >
> > > > In the physical processing of ovn-controller, there are two
> > > > sets of OF flows that are still fully recalculated every cycle:
> > > >
> > > >   Flows that aren't associated with any logical flow, and
> > > >   Flows calculated based on multicast groups
> > > >
> > > > Because these flows are recalculated fully each cycle, full
> > > > duplicates of existing OF flows are created and the OF management
> > > > code in ovn-controller pollutes the logs with false positive
> > > > warnings about repeated duplicates.
> > > >
> > > > As a short term measure, ignore full duplicates for both of
> > > > these types of flows, but still warn if the action changes
> > > > (as that is not expected and may be indicative of a problem).
> > > >
> > > > Signed-off-by: Ryan Moats 
> > > >
> > >
> > > I also noticed that "commit 70c7cfef188b5ae9940abd5 (ovn-controller:
> Add
> > > incremental processing to lflow_run and physical_run)" causes load
> > > balancing system unit tests to fail. A little debugging shows that
> groups
> > > are getting deleted when new flows are added.  My hunch is that this is
> > > likely because 'desired_groups' in ofctl_put gets deleted in every run.
> But
> > > in the next run, it does not get updated as we no longer process all
> flows.
> >
> > It's unclear to me from the discussion of this patch whether it's
> > beneficial.  Can someone clarify?
> >
> > Thanks,
> >
> > Ben.
> 
> Unfortunately, I don't think we maintained reasonable email thread
> discipline
> on this discussion - for me, the vast majority of the conversation was
> about
> the CPU cycle bug that Guru mentions and not the patch itself.
> 
> The conditions stated in the commit message still apply even after fixing
> Guru's issue and so I still see the squelching as being useful to avoid
> polluting the ovn-controller logs with false positive messages...

I think that you said in IRC that you're dropping this patch.  Let me
know if I'm wrong.

Thanks,

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

Re: [ovs-dev] [PATCH v2 1/4] netdev-dpdk: Remove dpdkvhostcuse ports

[Flavio Leitner]

On Tue, Jul 26, 2016 at 05:12:51PM +0100, Ciara Loftus wrote:
> This commit removes the 'dpdkvhostcuse' port type from the userspace
> datapath. vhost-cuse ports are quickly becoming obsolete as the
> vhost-user port type begins to support a greater feature-set thanks to
> the addition of things like vhost-user multiqueue and potential
> upcoming features like vhost-user client-mode and vhost-user reconnect.
> The feature is also expected to be removed from DPDK soon.
> 
> One potential drawback of the removal of this support is that a
> userspace vHost port type is not available in OVS for use with older
> versions of QEMU (pre v2.2). Considering v2.2 is nearly two years old
> this should however be a low impact change.
> 
> Signed-off-by: Ciara Loftus 
> Acked-by: Flavio Leitner 
> Acked-by: Daniele Di Proietto 
> ---
>  INSTALL.DPDK-ADVANCED.md | 241 -
>  NEWS |   1 +
>  acinclude.m4 |  12 --
>  lib/netdev-dpdk.c| 101 +---
>  rhel/README.RHEL |   2 -
>  utilities/automake.mk|   1 -
>  utilities/qemu-wrap.py   | 389 
> ---
>  vswitchd/vswitch.xml |  12 --
>  8 files changed, 5 insertions(+), 754 deletions(-)
>  delete mode 100755 utilities/qemu-wrap.py
> 
> diff --git a/INSTALL.DPDK-ADVANCED.md b/INSTALL.DPDK-ADVANCED.md
> index 9ae536d..fb37584 100644
> --- a/INSTALL.DPDK-ADVANCED.md
> +++ b/INSTALL.DPDK-ADVANCED.md
> @@ -374,12 +374,6 @@ For users wanting to do packet forwarding using kernel 
> stack below are the steps
>  
>  ##  6. Vhost Walkthrough
>  
> -DPDK 16.04 supports two types of vhost:
> -
> -1. vhost-user - enabled default
> -
> -2. vhost-cuse - Legacy, disabled by default
> -
>  ### 6.1 vhost-user
>  
>- Prerequisites:
> @@ -533,241 +527,6 @@ DPDK 16.04 supports two types of vhost:
>  
>Note: For information on libvirt and further tuning refer [libvirt].
>  
> -### 6.2 vhost-cuse
> -
> -  - Prerequisites:
> -
> -QEMU version >= 2.2
> -
> -  - Enable vhost-cuse support
> -
> -1. Enable vhost cuse support in DPDK
> -
> -   Set `CONFIG_RTE_LIBRTE_VHOST_USER=n` in config/common_linuxapp and 
> follow the
> -   steps in 2.2 section of INSTALL.DPDK guide to build DPDK with cuse 
> support.
> -   OVS will detect that DPDK has vhost-cuse libraries compiled and in 
> turn will enable
> -   support for it in the switch and disable vhost-user support.
> -
> -2. Insert the Cuse module
> -
> -   `modprobe cuse`
> -
> -3. Build and insert the `eventfd_link` module
> -
> -   ```
> -   cd $DPDK_DIR/lib/librte_vhost/eventfd_link/
> -   make
> -   insmod $DPDK_DIR/lib/librte_vhost/eventfd_link.ko
> -   ```
> -
> -  - Adding vhost-cuse ports to Switch
> -
> -Unlike DPDK ring ports, DPDK vhost-cuse ports can have arbitrary names.
> -For vhost-cuse, the name of the port type is `dpdkvhostcuse`
> -
> -```
> -ovs-vsctl add-port br0 vhost-cuse-1 -- set Interface vhost-cuse-1
> -type=dpdkvhostcuse
> -```
> -
> -When attaching vhost-cuse ports to QEMU, the name provided during the
> -add-port operation must match the ifname parameter on the QEMU cmd line.
> -
> -  - Adding vhost-cuse ports to VM
> -
> -vhost-cuse ports use a Linux* character device to communicate with QEMU.
> -By default it is set to `/dev/vhost-net`. It is possible to reuse this
> -standard device for DPDK vhost, which makes setup a little simpler but it
> -is better practice to specify an alternative character device in order to
> -avoid any conflicts if kernel vhost is to be used in parallel.
> -
> -1. This step is only needed if using an alternative character device.
> -
> -   ```
> -   ./utilities/ovs-vsctl --no-wait set Open_vSwitch . \
> -other_config:cuse-dev-name=my-vhost-net
> -   ```
> -
> -   In the example above, the character device to be used will be
> -   `/dev/my-vhost-net`.
> -
> -2. In case of reusing kernel vhost character device, there would be 
> conflict
> -   user should remove it.
> -
> -   `rm -rf /dev/vhost-net`
> -
> -3. Configure virtio-net adapters
> -
> -   The following parameters must be passed to the QEMU binary, repeat
> -   the below parameters for multiple devices.
> -
> -   ```
> -   -netdev tap,id=,script=no,downscript=no,ifname=,vhost=on
> -   -device virtio-net-pci,netdev=net1,mac=
> -   ```
> -
> -   The DPDK vhost library will negotiate its own features, so they
> -   need not be passed in as command line params. Note that as offloads
> -   are disabled this is the equivalent of setting
> -
> -   `csum=off,gso=off,guest_tso4=off,guest_tso6=off,guest_ecn=off`
> -
> -   When using an alternative character device, it must be explicitly
> -   passed to QEMU using the `vhostfd` argument
> -
> - 

Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 07:54:30PM +, Ryan Moats wrote:
> Ensure that rows created for deleted port binding and
> multicast group rows are cleared when doing full processing.
> 
> Signed-off-by: Ryan Moats 

I'm choosing to overlook storing UUIDs as strings in a set of strings.

How about this simplification?

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

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 6bc0095..a4a8fcf 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -598,25 +598,22 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
 sset_destroy(_chassis);
 }
 
+/* Deletes the flows whose UUIDs are in 'old' but not 'new', and then replaces
+ * 'old' by 'new'. */
 static void
 rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new)
 {
-const char *uuid_s, *next_uuid;
-SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) {
+const char *uuid_s;
+SSET_FOR_EACH (uuid_s, old) {
 if (!sset_find(new, uuid_s)) {
 struct uuid uuid;
 if (uuid_from_string(, uuid_s)) {
 ofctrl_remove_flows();
 }
-sset_find_and_delete(old, uuid_s);
 }
 }
-SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) {
-if (!sset_find(old, uuid_s)) {
-sset_add(old, uuid_s);
-}
-sset_find_and_delete(new, uuid_s);
-}
+sset_swap(old, new);
+sset_clear(new);
 }
 
 void
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn-controller: Remove old values from local_ids.

[Russell Bryant]

local_ids is supposed to be the set of interface iface-id values from
this chassis that correspond to OVN logical ports.  We use this for
detecting when an interface has been removed as well as if child-ports
should be bound to this chassis.

Old values were not being removed from local_ids.  The most immediate
effect of this was that once an interface has been removed from a
chassis, we would think a removal has occured *every* time through
binding_run and trigger the full binding processing.  This was
a performance problem.

The second problem this would cause is if a port that had child ports
was moved to another chassis.  We would end up with two chassis fighting
over the binding of the child ports.

Signed-off-by: Russell Bryant 
---
 ovn/controller/binding.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 78ebec4..41165bc 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -103,6 +103,11 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int,
  * that has been removed. */
 if (!changed && !sset_is_empty(_local_ids)) {
 changed = true;
+
+const char *cur_id;
+SSET_FOR_EACH(cur_id, _local_ids) {
+sset_find_and_delete(_ids, cur_id);
+}
 }
 
 sset_destroy(_local_ids);
-- 
2.7.4

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

Re: [ovs-dev] [PATCH 2/2] README: add referente to DPDK installation

[Ben Pfaff]

On Thu, Jul 28, 2016 at 10:49:26PM +0200, Mauricio Vasquez B wrote:
> there was not any reference to the DPDK installation in the main README file.
> 
> Signed-off-by: Mauricio Vasquez B 

Both applied, thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] ovn-controller: Persist desired conntrack groups.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 02:18:45PM +, Ryan Moats wrote:
> With incremental processing of logical flows desired conntrack groups
> are not being persisted.  This patch adds this capability, with the
> side effect of adding a ds_clone method that this capability leverages.
> 
> Signed-off-by: Ryan Moats 
> Reported-by: Guru Shetty 
> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
> Fixes: 70c7cfe ("ovn-controller: Add incremental processing to lflow_run and 
> physical_run")
> ---
>  v1->v2 addressed review comments
>updated commit message
>changed name of ds_copy to ds_clone
>moved lflow uuid storage to action_params for cleaner code

It seems odd that group_clone() doesn't copy the UUID.

I'm not sure why group_info's 'group' member is a struct ds instead of
just a char *.  (This isn't anything that you introduced.)

Coding style:
> +static struct group_info *
> +group_info_clone(struct group_info *source) {

But I'll hold off on further review since Guru reports that this causes
test failures in the system tests.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4] ovn-northd, tests: Adding IPAM to ovn-northd.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 01:29:42PM -0700, Guru Shetty wrote:
> On 28 July 2016 at 11:37, Ben Pfaff  wrote:
> 
> > On Wed, Jul 27, 2016 at 11:28:24AM -0700, Nimay Desai wrote:
> > > Added an IPv4 and MAC addresses management system to ovn-northd. When a
> > logical
> > > switch's other_config:subnet field is set, logical ports attached to that
> > > switch that have the keyword "dynamic" in their addresses column will
> > > automatically be allocated a globally unique MAC address/unused IPv4
> > address
> > > within the provided subnet. The allocated address will populate the
> > > dynamic_addresses column. This can be useful for a user who wants to
> > deploy
> > > many VM's or containers with networking capabilities, but does not care
> > about
> > > the specific MAC/IPv4 addresses that are assigned.
> > >
> > > Added tests in ovn.at for ipam.
> > >
> > > Signed-off-by: Nimay Desai 
> >
> > The code uses the abbreviations 'ipam' and 'macam' a lot.  IPAM is a
> > fairly common term but I don't know what macam stands for.  I think it
> > would be a good idea to explain both terms in comments.
> >
> 
> Do you think this incremental is helpful?

Yes, thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v1] Enable manager_option configuation for NB and SB

[Ben Pfaff]

On Thu, Jul 28, 2016 at 01:14:37PM -0700, Amitabha Biswas wrote:
> So v2 of this patch will contain Manager Table in the NB and SB
> database. Does that sounds reasonable?

Yes, that sounds OK to me.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/2] README: add missing reference to INSTALL.SELinux.md

[Mauricio Vasquez B]

Signed-off-by: Mauricio Vasquez B 
---
 README.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/README.md b/README.md
index 69c4912..13a132a 100644
--- a/README.md
+++ b/README.md
@@ -120,6 +120,7 @@ b...@openvswitch.org
 [INSTALL.KVM.md]:INSTALL.KVM.md
 [INSTALL.Libvirt.md]:INSTALL.Libvirt.md
 [INSTALL.RHEL.md]:INSTALL.RHEL.md
+[INSTALL.SELinux.md]:INSTALL.SELinux.md
 [INSTALL.SSL.md]:INSTALL.SSL.md
 [INSTALL.userspace.md]:INSTALL.userspace.md
 [INSTALL.XenServer.md]:INSTALL.XenServer.md
-- 
1.9.1

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

[ovs-dev] [PATCH 2/2] README: add referente to DPDK installation

[Mauricio Vasquez B]

there was not any reference to the DPDK installation in the main README file.

Signed-off-by: Mauricio Vasquez B 
---
 README.md | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/README.md b/README.md
index 13a132a..cf53437 100644
--- a/README.md
+++ b/README.md
@@ -94,6 +94,8 @@ To use Open vSwitch...
 
 - ...without using a kernel module, read [INSTALL.userspace.md].
 
+- ...with DPDK, read [INSTALL.DPDK.md].
+
 - ...with SELinux, read [INSTALL.SELinux.md].
 
 For answers to common questions, read [FAQ.md].
@@ -116,6 +118,7 @@ b...@openvswitch.org
 [INSTALL.md]:INSTALL.md
 [INSTALL.Debian.md]:INSTALL.Debian.md
 [INSTALL.Docker.md]:INSTALL.Docker.md
+[INSTALL.DPDK.md]:INSTALL.DPDK.md
 [INSTALL.Fedora.md]:INSTALL.Fedora.md
 [INSTALL.KVM.md]:INSTALL.KVM.md
 [INSTALL.Libvirt.md]:INSTALL.Libvirt.md
-- 
1.9.1

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

Re: [ovs-dev] [PATCH v2] netdev-dpdk: Add Flow Control support.

[Chandran, Sugesh]

Thank you Bhanu for reviewing and testing it.


Regards
_Sugesh


> -Original Message-
> From: Bodireddy, Bhanuprakash
> Sent: Thursday, July 28, 2016 4:54 PM
> To: Chandran, Sugesh ; diproiet...@ovn.org;
> dev@openvswitch.org
> Subject: RE: [PATCH v2] netdev-dpdk: Add Flow Control support.
> 
> >-Original Message-
> >From: Chandran, Sugesh
> >Sent: Thursday, July 28, 2016 4:30 PM
> >To: diproiet...@ovn.org; Bodireddy, Bhanuprakash
> >; dev@openvswitch.org
> >Cc: Chandran, Sugesh 
> >Subject: [PATCH v2] netdev-dpdk: Add Flow Control support.
> >
> >Add support for flow-control(mac control frame) to DPDK enabled physical
> >port types. By default, the flow-control is OFF on both rx and tx side.
> >The flow control can be enabled/disabled either when adding a port to OVS
> or
> >at run time.
> >
> >For eg:
> >To enable flow control support at tx side while adding a port, add the 'tx-
> flow-
> >ctrl' option to the 'ovs-vsctl add-port' command-line as below.
> >
> > 'ovs-vsctl add-port br0 dpdk0 -- \
> >  set Interface dpdk0 type=dpdk options:tx-flow-ctrl=true'
> >
> >Similarly to enable rx flow control,
> > 'ovs-vsctl add-port br0 dpdk0 -- \
> >  set Interface dpdk0 type=dpdk options:rx-flow-ctrl=true'
> >
> >And to enable the flow control auto-negotiation,  'ovs-vsctl add-port br0
> >dpdk0 -- \
> >  set Interface dpdk0 type=dpdk options:flow-ctrl-autoneg=true'
> >
> >To turn ON the tx flow control at run time(After the port is being added to
> >OVS), the command-line input will be,  'ovs-vsctl set Interface dpdk0
> >options:tx-flow-ctrl=true'
> >
> >The flow control parameters can be turned off by setting 'false' to the
> >respective parameter. To dsiable the flow control at tx side,  'ovs-vsctl set
> >Interface dpdk0 options:tx-flow-ctrl=false'
> >
> >Signed-off-by: Sugesh Chandran 
> 
> LGTM, I tested it and can apply the rx flow control setting even when the
> interface is transmitting.
> Acked-by: Bhanuprakash Bodireddy 
> 
> Regards,
> Bhanu Prakash.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4] ovn-northd, tests: Adding IPAM to ovn-northd.

[Guru Shetty]

On 28 July 2016 at 11:37, Ben Pfaff  wrote:

> On Wed, Jul 27, 2016 at 11:28:24AM -0700, Nimay Desai wrote:
> > Added an IPv4 and MAC addresses management system to ovn-northd. When a
> logical
> > switch's other_config:subnet field is set, logical ports attached to that
> > switch that have the keyword "dynamic" in their addresses column will
> > automatically be allocated a globally unique MAC address/unused IPv4
> address
> > within the provided subnet. The allocated address will populate the
> > dynamic_addresses column. This can be useful for a user who wants to
> deploy
> > many VM's or containers with networking capabilities, but does not care
> about
> > the specific MAC/IPv4 addresses that are assigned.
> >
> > Added tests in ovn.at for ipam.
> >
> > Signed-off-by: Nimay Desai 
>
> The code uses the abbreviations 'ipam' and 'macam' a lot.  IPAM is a
> fairly common term but I don't know what macam stands for.  I think it
> would be a good idea to explain both terms in comments.
>

Do you think this incremental is helpful?

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 02b26bb..18756e9 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -62,8 +62,8 @@ static const char *default_sb_db(void);
 #define MAC_ADDR_PREFIX 0x0A00ULL
 #define MAC_ADDR_SPACE 0xff

-/* MAC address table of "struct eth_addr"s, that holds the MAC addresses
- * allocated by the ipam. */
+/* MAC address management (macam) table of "struct eth_addr"s, that holds
the
+ * MAC addresses allocated by the OVN ipam module. */
 static struct hmap macam = HMAP_INITIALIZER();
 ^L
 /* Pipeline stages. */
@@ -879,6 +879,12 @@ static void
 build_ipam(struct northd_context *ctx, struct hmap *datapaths,
struct hmap *ports)
 {
+/* IPAM generally stands for IP address management.  In non-virtualized
+ * world, MAC addresses come with the hardware.  But, with virtualized
+ * workloads, they need to be assigned and managed.  This function
+ * does both IP address management (ipam) and MAC address management
+ * (macam). */
+
 if (!ctx->ovnnb_txn) {
 return;
 }


>
> Here are some suggested improvements.  The code improvements are, I
> hope, self-explanatory.  The changes to the documentation are mainly to
> make it clear that ovn-northd does the work.  (I find it really
> confusing when documentation says that something happens, but not what
> does it, because then you have to assume or guess what does it and it's
> easy to guess wrong.)
>
> I'm done with review.  I'll leave it to Guru to shepherd this in though
> since he's been guiding the work.
>
> Acked-by: Ben Pfaff 
>
> --8<--cut here-->8--
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 25af707..d5cbd37 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -867,21 +867,9 @@ ipam_allocate_addresses(struct ovn_datapath *od,
> struct ovn_port *op,
>  ipam_insert_ip(od, ip, false);
>  ipam_insert_mac(, false);
>
> -/* Convert IP to string form. */
> -struct ds ip_ds;
> -ds_init(_ds);
> -ds_put_format(_ds, IP_FMT, IP_ARGS(htonl(ip)));
> -
> -/* Convert MAC to string form. */
> -struct ds mac_ds;
> -ds_init(_ds);
> -ds_put_format(_ds, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
> -
> -char *new_addr = xasprintf("%s %s", mac_ds.string, ip_ds.string);
> -nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp,
> -(const char*)
> new_addr);
> -ds_destroy(_ds);
> -ds_destroy(_ds);
> +char *new_addr = xasprintf(IP_FMT" "ETH_ADDR_FMT,
> +   IP_ARGS(htonl(ip)), ETH_ADDR_ARGS(mac));
> +nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp, new_addr);
>  free(new_addr);
>
>  return true;
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 249d3c5..85aa2d3 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -125,11 +125,12 @@
>
>
>
> -If set, logical ports that are attached to this switch that have
> the
> -"dynamic" keyword in their addresses column will automatically be
> -allocated a globally unique MAC address/unused IPv4 address
> within the
> -provided IPv4 subnet.  The allocated address will populate the
> - column.
> +Set this to an IPv4 subnet, e.g. 192.168.0.0/24, to
> enable
> +ovn-northd to automatically assign IP addresses
> within
> +that subnet.  Use the dynamic keyword in the  +table="Logical_Switch_Port"/> table's  table="Logical_Switch_Port"
> +column="addresses"/> column to request dynamic address assignment
> for a
> +particular port.
>
>  
>
> @@ -439,22 +440,23 @@
>
>dynamic
>
> -This indicates that the logical port should be 

Re: [ovs-dev] [PATCH v1] Enable manager_option configuation for NB and SB

[Amitabha Biswas]

Hi Ben,

No there is no advantage for having a separate database, we can incorporate a 
Manager Table into the Northbound and Southbound database. This was an initial 
hack (with minimal code and schema change) to verify the efficacy of disabling 
the probe timer on our cloud setups and get some feedback from the community. I 
think our cloud deployment team has come to the conclusion that we do need the 
ability to modify the probe timer values in the ovsdb-servers that manage the 
NB and SB databases.

So v2 of this patch will contain Manager Table in the NB and SB database. Does 
that sounds reasonable?

Thanks
Amitabha


> On Jul 27, 2016, at 11:45 AM, Ben Pfaff  wrote:
> 
> On Mon, Jul 18, 2016 at 10:21:09PM -0700, Amitabha Biswas wrote:
>> NB and SB dbs are handled by separate ovsdb-server processes. The
>> ovsdb-server processes manage dbs based on the schemas for OVN NorthBound
>> and SouthBound. These schemas do not have any Manager Options similar
>> to the Open_vSwitch schema. As a result, for e.g., the frequency of
>> inactivity probes sent to clients from the ovsdb-server cannot be
>> modified.
>> 
>> This patch addresses the above problem by creating independent
>> "conf.db"s for NB and SB. The ovsdb-server process which handles
>> the NB will handle a NB_conf.db as well, ditto for the ovsdb-server
>> that handles the SB.
>> 
>> A similar result can be obtained by adding a Manager Table to the
>> NorthBound and SouthBound DBs as well. In such case there would
>> not be a need for a separate "config" database. I'm willing to
>> consider this solution as well pending feedback.
> 
> Is there an advantage to having a separate database for this purpose?
> It strikes me as an odd design.
> 
> The vtep schema also in the OVS tree (vtep/vtep.ovsschema) has its own
> Manager table.

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

Re: [ovs-dev] [PATCH 1/2] rhel: Fix ifup-ovs to delete ports first.

[Joe Stringer]

On 25 July 2016 at 18:16, Flavio Leitner  wrote:
> When ifdown isn't executed (system didn't shut down properly),
> ports remain in the openvswitch's database.  In that case, an
> inconsitency is left behind when the ifcfg was modified because
> ovs-vsctl won't do anything to update existing port's configuration
> in the database.
>
> The ifup/ifdown will operate only on configured interfaces, so
> this patch fixes the issue by deleting the port from the database
> before attempt to configure it with fresh configuration.
>
> Signed-off-by: Flavio Leitner 

I think that the --may-exist was intended to cover this, but based on
the documentation in ovs-vsctl(8) it doesn't. This looks more
resilient, thanks.

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

Re: [ovs-dev] [PATCH] ovn: Rename "gateway" to "l3gateway".

[Russell Bryant]

On Thu, Jul 28, 2016 at 9:45 AM, Guru Shetty  wrote:

> On 26 July 2016 at 13:49, Russell Bryant  wrote:
>
>> When L3 gateway support was added, it introduced a port type called
>> "gateway" and a corresponding option called "gateway-chassis".  Since
>> that time, we also have an L2 gateway port type called "l2gateway" and a
>> corresponding option called "l2gateway-chassis".  This patch renames the
>> L3 gateway port type and option to "l3gateway" and "l3gateway-chassis"
>> to make things a little more clear and consistent.
>>
>> Signed-off-by: Russell Bryant 
>>
> Acked-by: Gurucharan Shetty 
>

Thanks, I applied this to master.

I forgot to add the Acks, though.  Sorry about that ...

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

Re: [ovs-dev] [PATCH 2/2] rhel: Allow openvswitch to get parent information

[Joe Stringer]

On 28 July 2016 at 06:43, Flavio Leitner  wrote:
>
> Adding William since he is the author of commit 484371776e
>
> On Wed, Jul 27, 2016 at 01:54:45PM -0700, Joe Stringer wrote:
>> On 26 July 2016 at 14:10, Flavio Leitner  wrote:
>> > On Tue, Jul 26, 2016 at 12:57:07PM -0700, Joe Stringer wrote:
>> >> As far as I can follow, this "domain" type is not just for accessing
>> >> OVS directories and files (like openvswitch_t), but ifor a much wider
>> >> range of paths:
>> >> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/SELinux_Guide/rhlcommon-section-0048.html
>> >>
>> >> "# The domain attribute identifies every type that can be
>> >> # assigned to a process.  This attribute is used in TE rules
>> >> # that should be applied to all domains, e.g. permitting
>> >> # init to kill all processes."
>> >>
>> >> Is my understanding (+documentation) correct here? Is there an similar
>> >
>> > Your understanding is correct.  Turns out that we don't know which
>> > process will be the parent, so it could bash unconfined or initrc_t
>> > or in any other context (neutron?).
>> >
>> >> but more restrictive policy that allows ovs-vsctl to access, for
>> >> example, /var/run/openvswitch/* (with var_run_openvswitch_t or
>> >> similar)? Alternatively is there an example of another daemon that has
>> >> a similar policy that set a precedence for writing the policy like
>> >> this?
>> >
>> > I spent few hours on this and I couldn't find a way to restrict it
>> > more that I proposed with selinux.  Basically the above is an expansion
>> > of the interface domain_read_all_domains_state()[1] which other
>> > applications are using to read other processes states.  However, that
>> > seemed relatively new and probably not available on older distros, so
>> > I have expanded to the relevant actions removing what we don't need.
>> >
>> > [1] http://danwalsh.livejournal.com/51435.html
>> >
>> >> Would you also be able to provide the full ovs-vsctl commandline? It
>> >> was a little difficult to understand exactly what was going on during
>> >> this event, or try to reproduce.
>> >
>> > utilities/ovs-vsctl.c:2473
>> >
>> > 2472 static char *
>> > 2473 vsctl_parent_process_info(void)
>> > 2474 {
>> > 2475 #ifdef __linux__
>> > 2476 pid_t parent_pid;
>> > 2477 char *procfile;
>> > 2478 struct ds s;
>> > 2479 FILE *f;
>> > 2480
>> > 2481 parent_pid = getppid();
>> > 2482 procfile = xasprintf("/proc/%d/cmdline", parent_pid);
>> > 2483
>> > 2484 f = fopen(procfile, "r");
>> >
>> > That is called from do_vsctl() to find the parent info.  If you run as
>> > root, then it's unconfined and it works, but it doesn't work during
>> > boot time (initrc_t) for instance.
>> >
>> > To reproduce you just need to configure an OVS interface using ifcfg
>> > with ONBOOT=yes and reboot.
>>
>> Hmm. So all we want to do in this case is to get the parent's name to
>> log it. I'm of two minds:
>
> Yes, funny how that can be a pain :-)
>
>> 1) Be more restrictive. If this were the only place in OVS that we
>> read /proc then maybe it should just print the pid on failure to read
>> /proc, then we just accept that SELinux denies this access. It seems
>> like it's just a niceity for prettyprinting which shouldn't affect
>> actual setup/operation of OVS. While a pid is less than a name, it's
>> still just as much information for cases like this where it's being
>> invoked from initrc_t.
>
> It generates AVC messages which triggers notifications, so we actually
> need to consider three options:
> 1) Allow it (your option 2 below)
> 2) Fix the code  (proposed in your option 1)
> 3) Use dontaudit to deny but not log.
>
> The problem is that PIDs numbers don't mean anything useful for us when
> you are looking at logs from another system.  Even if you are on the
> system, PIDs are ephemeral and probably gone or worse, recycled.

I was primarily thinking of something like it being called from
initrc_t and it printed "pid 1" then that would actually be
informative. But I guess this doesn't address your "various
scripts/programs are changing configuration" use case.

> I think the feature has an interesting value when you consider a OSP
> compute node where agents and scripts are running and configuring OVS.
> I got reports like "we added an port but there is no traffic", the
> next step is to make sense out of ovsdb logs and there you find
> an application adding and deleting the same port.  Without that, we
> have the actions, but we can't tell who did it.  How to find that out
> then?
>
> If we can reproduce the issue and start to record pids and names with
> another script, then it's fine, but most probably it's one of those
> "happened once", "it's in production", "can't enable this or that"
> kind of situation.

I spoke briefly with Ben about this offline, and the way I understand
it is that there were two pieces of debugging information provided
around the time this 

Re: [ovs-dev] [PATCH] netdev-provider: fix comments for netdev_rxq_recv

[Flavio Leitner]

On Tue, Jul 26, 2016 at 02:19:17PM +0100, Mark Kavanagh wrote:
> Commit 64839cf43 applies batch objects to netdev-providers, but
> some comments were not updated accordingly. Fix these:
>- replace 'pkts' with 'batch'
>- replace '*cnt' with 'batch->count'
>- replace MAX_RX_BATCH with NETDEV_MAX_BURST
>- remove superfluous whitespace
> 
> Signed-off-by: Mark Kavanagh 
> ---

Acked-by: Flavio Leitner 


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

Re: [ovs-dev] [PATCH v2] ovn: Add ovn-controller-vtep debian package

[Ben Pfaff]

On Thu, Jul 28, 2016 at 04:53:03PM +, Ryan Moats wrote:
> Having a separate debian package for deploying
> the ovn-controller-vtep binary enables the ability
> to assign specific nodes the role of communicating
> with VTEP enabled TORs.
> 
> Change-Id: Ia36aea7d89bd011a57918820b2a9f6e3469b3e04
> Signed-off-by: Ryan Moats 

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

[ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.

[Ryan Moats]

Ensure that rows created for deleted port binding and
multicast group rows are cleared when doing full processing.

Signed-off-by: Ryan Moats 
---
 ovn/controller/physical.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 2e9fb73..9e9343d 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -58,6 +58,10 @@ static struct simap localvif_to_ofport =
 SIMAP_INITIALIZER(_to_ofport);
 static struct hmap tunnels = HMAP_INITIALIZER();
 
+static struct sset port_binding_uuids = SSET_INITIALIZER(_binding_uuids);
+static struct sset multicast_group_uuids =
+SSET_INITIALIZER(_group_uuids);
+
 /* UUID to identify OF flows not associated with ovsdb rows. */
 static struct uuid *hc_uuid = NULL;
 static bool full_binding_processing = false;
@@ -594,6 +598,27 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
 sset_destroy(_chassis);
 }
 
+static void
+rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new)
+{
+const char *uuid_s, *next_uuid;
+SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) {
+if (!sset_find(new, uuid_s)) {
+struct uuid uuid;
+if (uuid_from_string(, uuid_s)) {
+ofctrl_remove_flows();
+}
+sset_find_and_delete(old, uuid_s);
+}
+}
+SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) {
+if (!sset_find(old, uuid_s)) {
+sset_add(old, uuid_s);
+}
+sset_find_and_delete(new, uuid_s);
+}
+}
+
 void
 physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
  const struct ovsrec_bridge *br_int, const char *this_chassis_id,
@@ -750,6 +775,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id 
mff_ovn_geneve,
  * 64 for logical-to-physical translation. */
 const struct sbrec_port_binding *binding;
 if (full_binding_processing) {
+struct sset new_port_binding_uuids =
+SSET_INITIALIZER(_port_binding_uuids);
 SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
 /* Because it is possible in the above code to enter this
  * for loop without having cleared the flow table first, we
@@ -757,7 +784,12 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id 
mff_ovn_geneve,
 ofctrl_remove_flows(>header_.uuid);
 consider_port_binding(mff_ovn_geneve, ct_zones, local_datapaths,
   patched_datapaths, binding, );
+sset_add(_port_binding_uuids,
+ xasprintf(UUID_FMT, UUID_ARGS(>header_.uuid)));
 }
+rationalize_ssets_and_delete_flows(_binding_uuids,
+   _port_binding_uuids);
+sset_destroy(_port_binding_uuids);
 full_binding_processing = false;
 } else {
 SBREC_PORT_BINDING_FOR_EACH_TRACKED (binding, ctx->ovnsb_idl) {
@@ -777,6 +809,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id 
mff_ovn_geneve,
 const struct sbrec_multicast_group *mc;
 struct ofpbuf remote_ofpacts;
 ofpbuf_init(_ofpacts, 0);
+struct sset new_multicast_group_uuids =
+SSET_INITIALIZER(_multicast_group_uuids);
 SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) {
 /* As multicast groups are always reprocessed each time,
  * the first step is to clean the old flows for the group
@@ -784,7 +818,12 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id 
mff_ovn_geneve,
 ofctrl_remove_flows(>header_.uuid);
 consider_mc_group(mff_ovn_geneve, ct_zones,
   local_datapaths, mc, , _ofpacts);
+sset_add(_multicast_group_uuids,
+ xasprintf(UUID_FMT, UUID_ARGS(>header_.uuid)));
 }
+rationalize_ssets_and_delete_flows(_group_uuids,
+   _multicast_group_uuids);
+sset_destroy(_multicast_group_uuids);
 
 ofpbuf_uninit(_ofpacts);
 
-- 
1.9.1

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

Re: [ovs-dev] [PATCH] ovn-controller: Clean up cases that lead to duplicate OF flows.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 02:11:52PM -0500, Ryan Moats wrote:
> Ben Pfaff  wrote on 07/28/2016 01:39:53 PM:
> 
> > From: Ben Pfaff 
> > To: Ryan Moats/Omaha/IBM@IBMUS
> > Cc: dev@openvswitch.org
> > Date: 07/28/2016 01:40 PM
> > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Clean up cases that
> > lead to duplicate OF flows.
> >
> > On Thu, Jul 28, 2016 at 06:10:16PM +, Ryan Moats wrote:
> > > In physical_run, there are multiple places where OF flows can be
> > > produced each cycle.  Because the desired flow table may not have
> > > been completely cleared first, remove flows created during previous
> > > runs before creating new flows.  This avoid collisions.
> > >
> > > Signed-off-by: Ryan Moats 
> >
> > Hi Ryan.
> >
> > In the "full" cases, if ports or multicast groups have disappeared, what
> > deletes the associated flows?
> >
> > Thanks,
> >
> > Ben.
> >
> 
> That's an excellent question - Since those are created and removed by
> northd (if I read the code right), right now, we'll have stale entries
> until the next time we need full logical flow processing, at which
> point we'll dump the desired flow table and flush them out.
> 
> Since I don't really like that answer, I think code that addresses
> this case would be useful. I propose to put it in a follow-on patch set,
> since I don't think it quite fits here.

Thanks for answering.

I agree that it's related but different, so I pushed this.

Thanks for being persistent with improving the incremental update code
now that it's in.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Mea culpa - rewound master branch

[Flavio Leitner]

On Thu, Jul 28, 2016 at 10:58:25AM -0700, Daniele Di Proietto wrote:
> 2016-07-28 10:52 GMT-07:00 Flavio Leitner :
> 
> > On Thu, Jul 28, 2016 at 09:32:54AM -0700, Ben Pfaff wrote:
> > > On Thu, Jul 28, 2016 at 12:49:37PM -0300, Flavio Leitner wrote:
> > > > On Wed, Jul 27, 2016 at 11:49:18AM -0700, Ben Pfaff wrote:
> > > > > Some time ago this morning, I accidentally pushed several patches
> > that
> > > > > were still under review to master.  I've force-pushed a correction to
> > > > > the branch.  My apologies--I hope that this does not screw up
> > anyone's
> > > > > development process in a bad way.
> > > >
> > > > There is another weird one:
> > > > $ git log master..origin/master --oneline
> > > > [...]
> > > >   f3edb dpif-netdev: Execute conntrack action.
> > > >   b412490 tests: Add test-conntrack pcap test.
> > > >   8cb1462 tests: Add very simple conntrack benchmark.
> > > > * 6c54734 XXX Improve comment.
> > > >   e6ef6cc conntrack: Periodically delete expired connections.
> > > >   a489b16 conntrack: New userspace connection tracker.
> > > > [...]
> > >
> > > Probably should have been squashed before Daniele pushed it, but not
> > > really so bad if that's the only issue.
> >
> > Yup, hopefully it is just that.
> >
> >
> Sorry, I forgot to squash that commit before pushing it and when I realized
> it, it was too late to do a forced push.

Thanks for following up.

> Apologies for the inconvenience guys,

No worries from my side now that it's all clear :)

-- 
fbl

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

Re: [ovs-dev] [PATCH] ovn-controller: Add datapath-type in chassis:external_ids

[Ben Pfaff]

On Thu, Jul 28, 2016 at 02:07:35PM -0400, Russell Bryant wrote:
> On Thu, Jul 28, 2016 at 1:40 PM, Numan Siddique  wrote:
> 
> > On Thu, Jul 28, 2016 at 10:28 PM, Ben Pfaff  wrote:
> >
> > > On Thu, Jul 28, 2016 at 08:05:20PM +0530, Numan Siddique wrote:
> > > > This patch reads the external_ids:datapath-type value from the
> > > > Open_vSwitch table if defined and sets it in the
> > > external_ids:datapath-type
> > > > of Chassis table.
> > >
> > > What sets external_ids:datapath-type?  It isn't documented anywhere.
> > >
> >
> > ​The way "ovn-bridge-mappings" is set, it is expected to set in the same
> > way.
> > I presume either the Administrator will set it or puppet or some other tool
> > would set it.
> > I will update the documentation accordingly.
> >
> >
> We spoke about this briefly on IRC, but I think it would be better to read
> the datapath_type column from the row in the Bridge table for br-int.
> 
> 
> >
> > > > This will provide hints to the CMS or clients monitoring OVN SB DB to
> > > > determine the datapath type (DPDK or non-DPDK) configured and take some
> > > > actions based on it.
> > > >
> > > > One usecase is, OVN neutron plugin can use this information to set the
> > > > vif_type (ovs or vhostuser) during the port binding.
> > >
> > > Why would neutron need this information?  What does it use it for?
> > >
> >
> > ​When a user creates a VM in an open stack deployment, open stack nova will
> > select a compute host for the VM. Before booting the VM, nova asks neutron
> > to create a port and also provides the host name. Neutron can read the
> > datapath-type of the host from the OVN SB DB chassis table and tells nova
> > if it is a dpdk host or normal host so that nova can plug the vif to the
> > ovs bridge properly.
> >
> > With this approach, we can have openstack deployments with DPDK and
> > non-DPDK compute hosts. Right now this mixed deployment is not supported.
> >
> > ​Russel, Ryan - Please correct me if I am wrong here.​
> 
> 
> Yes, that's the use case.  It seems pretty round about, but this is easiest
> way for us to integrate into OpenStack today.  Exposing it as an
> external_id on the Chassis seemed like a pretty non-intrusive way to expose
> the info.  Hopefully it's not *too* offensive.  :-)

It's really the idea of adding a external-ids:datapath-type to the
Open_vSwitch table that bothered me the most.  If we can get it from
existing data in br-int, that's much better.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: Set pmd thread priority

[Flavio Leitner]

On Thu, Jul 28, 2016 at 03:39:58PM +, Bodireddy, Bhanuprakash wrote:
> >-Original Message-
> >From: Daniele Di Proietto [mailto:diproiet...@ovn.org]
> >Sent: Wednesday, July 27, 2016 10:10 PM
> >To: Kavanagh, Mark B 
> >Cc: Bodireddy, Bhanuprakash ;
> >dev@openvswitch.org
> >Subject: Re: [PATCH v5] netdev-dpdk: Set pmd thread priority
> >
> >Thanks for the patch, the implementation looks good to me too.
> >During testing I kept noticing that it's way too easy to make OVS completely
> >unresponsive.  As you point out in the documentation by having dpdk-lcore-
> >mask the same as pmd-cpu-mask, OVS cannot even be killed (a kill -9 is
> >required).  I wonder what happens if one tries to set pmd-cpu-mask to every
> >core in the system.
> >As a way to mitigate the risk perhaps we can avoid setting the main thread
> >affinity to the first core in dpdk-lcore-mask by _always_ restoring it in
> >dpdk_init__(), also if auto_determine is false.
> >Perhaps we should start explicitly prohibiting creating a pmd thread on the
> >first core in dpdk-lcore-mask (I get why previous version of this didn't do 
> >it on
> >core 0.  Perhaps we can generalize that to the first core in 
> >dpdk-lcore-mask).
> I will look in to this and get back to you sometime next week.

Isn't enough to just increase priority to the max with setpriority(2)?
I know it is not the same as SCHED_RR  but for those users that
don't know how to tune it properly, this seems to be less dangerous
while still providing a good share of CPU to the PMD thread.

For instance, I recall to have seen OVS revalidation threads running
with PMD on the same CPU, but that might have been 2.5 only.

Thanks,
-- 
fbl

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

Re: [ovs-dev] [PATCH 2/2] ovs-dev.py: Update for python3.

[Russell Bryant]

On Thu, Jul 28, 2016 at 2:11 PM, Joe Stringer  wrote:

> On 28 July 2016 at 11:02, Russell Bryant  wrote:
> >
> >
> > On Mon, May 16, 2016 at 5:56 PM, Joe Stringer  wrote:
> >>
> >> Adapt to python-2.6+, including support for 3.
> >>
> >> Signed-off-by: Joe Stringer 
> >
> >
> > Acked-by: Russell Bryant 
>
> Thanks for the review, it looks like this has already been pushed but
> it's good to see it was in good shape ;)
>
> If you are currently having some issue with ovs-dev.py and python3 on
> a recent master then maybe this patch didn't cover that, so I suggest
> you follow up with another patch to fix it.
>

Ha, sorry.  I don't actually use this.

It looked like a new unreviewed patch in my email client.  I had done a
search for "2.6" looking for a mail talking about OVS 2.6 plans and then
didn't clear the search.  The commit message talks about Python 2.6.  I
wasn't paying any attention to dates and the mail caught my eye as an easy
quick review.

Oops.  :-)

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

Re: [ovs-dev] [PATCH] ovn-controller: Clean up cases that lead to duplicate OF flows.

[Ryan Moats]

Ben Pfaff  wrote on 07/28/2016 01:39:53 PM:

> From: Ben Pfaff 
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 07/28/2016 01:40 PM
> Subject: Re: [ovs-dev] [PATCH] ovn-controller: Clean up cases that
> lead to duplicate OF flows.
>
> On Thu, Jul 28, 2016 at 06:10:16PM +, Ryan Moats wrote:
> > In physical_run, there are multiple places where OF flows can be
> > produced each cycle.  Because the desired flow table may not have
> > been completely cleared first, remove flows created during previous
> > runs before creating new flows.  This avoid collisions.
> >
> > Signed-off-by: Ryan Moats 
>
> Hi Ryan.
>
> In the "full" cases, if ports or multicast groups have disappeared, what
> deletes the associated flows?
>
> Thanks,
>
> Ben.
>

That's an excellent question - Since those are created and removed by
northd (if I read the code right), right now, we'll have stale entries
until the next time we need full logical flow processing, at which
point we'll dump the desired flow table and flush them out.

Since I don't really like that answer, I think code that addresses
this case would be useful. I propose to put it in a follow-on patch set,
since I don't think it quite fits here.

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

Re: [ovs-dev] [PATCH] ovn-controller: Clean up cases that lead to duplicate OF flows.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 06:10:16PM +, Ryan Moats wrote:
> In physical_run, there are multiple places where OF flows can be
> produced each cycle.  Because the desired flow table may not have
> been completely cleared first, remove flows created during previous
> runs before creating new flows.  This avoid collisions.
> 
> Signed-off-by: Ryan Moats 

Hi Ryan.

In the "full" cases, if ports or multicast groups have disappeared, what
deletes the associated flows?

Thanks,

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

Re: [ovs-dev] [PATCH] system-ovn.at: Fix ICMP conntrack output.

[Joe Stringer]

On 28 July 2016 at 11:19, Guru Shetty  wrote:
>
>
> On 28 July 2016 at 10:48, Joe Stringer  wrote:
>>
>> Recent changes to the dump-conntrack command provide more info
>> (type,code), but the system-ovn tests weren't updated for this.
>> Update the tests.
>>
>> Signed-off-by: Joe Stringer 
>
>
> Acked-by: Gurucharan Shetty 

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

Re: [ovs-dev] [PATCH v4] ovn-northd, tests: Adding IPAM to ovn-northd.

[Ben Pfaff]

On Wed, Jul 27, 2016 at 11:28:24AM -0700, Nimay Desai wrote:
> Added an IPv4 and MAC addresses management system to ovn-northd. When a 
> logical
> switch's other_config:subnet field is set, logical ports attached to that
> switch that have the keyword "dynamic" in their addresses column will
> automatically be allocated a globally unique MAC address/unused IPv4 address
> within the provided subnet. The allocated address will populate the
> dynamic_addresses column. This can be useful for a user who wants to deploy
> many VM's or containers with networking capabilities, but does not care about
> the specific MAC/IPv4 addresses that are assigned.
> 
> Added tests in ovn.at for ipam.
> 
> Signed-off-by: Nimay Desai 

The code uses the abbreviations 'ipam' and 'macam' a lot.  IPAM is a
fairly common term but I don't know what macam stands for.  I think it
would be a good idea to explain both terms in comments.

Here are some suggested improvements.  The code improvements are, I
hope, self-explanatory.  The changes to the documentation are mainly to
make it clear that ovn-northd does the work.  (I find it really
confusing when documentation says that something happens, but not what
does it, because then you have to assume or guess what does it and it's
easy to guess wrong.)

I'm done with review.  I'll leave it to Guru to shepherd this in though
since he's been guiding the work.

Acked-by: Ben Pfaff 

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

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 25af707..d5cbd37 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -867,21 +867,9 @@ ipam_allocate_addresses(struct ovn_datapath *od, struct 
ovn_port *op,
 ipam_insert_ip(od, ip, false);
 ipam_insert_mac(, false);
 
-/* Convert IP to string form. */
-struct ds ip_ds;
-ds_init(_ds);
-ds_put_format(_ds, IP_FMT, IP_ARGS(htonl(ip)));
-
-/* Convert MAC to string form. */
-struct ds mac_ds;
-ds_init(_ds);
-ds_put_format(_ds, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
-
-char *new_addr = xasprintf("%s %s", mac_ds.string, ip_ds.string);
-nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp,
-(const char*) new_addr);
-ds_destroy(_ds);
-ds_destroy(_ds);
+char *new_addr = xasprintf(IP_FMT" "ETH_ADDR_FMT,
+   IP_ARGS(htonl(ip)), ETH_ADDR_ARGS(mac));
+nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp, new_addr);
 free(new_addr);
 
 return true;
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 249d3c5..85aa2d3 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -125,11 +125,12 @@
   
 
   
-If set, logical ports that are attached to this switch that have the
-"dynamic" keyword in their addresses column will automatically be
-allocated a globally unique MAC address/unused IPv4 address within the
-provided IPv4 subnet.  The allocated address will populate the
- column.
+Set this to an IPv4 subnet, e.g. 192.168.0.0/24, to enable
+ovn-northd to automatically assign IP addresses within
+that subnet.  Use the dynamic keyword in the  table's  column to request dynamic address assignment for a
+particular port.
   
 
 
@@ -439,22 +440,23 @@
 
   dynamic
   
-This indicates that the logical port should be automatically
-assigned a globally unique MAC address and an unused IPv4 address
-within the subnet that this logical port belongs to.  The assigned
-addresses will populate the 
-column.  For this keyword to work properly, the other_config:subnet
-of the logical switch that this logical port is attached to must be
-set.
+Use this keyword to make ovn-northd generate a
+globally unique MAC address and choose an unused IPv4 address with
+the logical port's subnet and store them in the port's  column.  ovn-northd will
+use the subnet specified in  in the port's .
   
 
   
 
   
 
-  Addresses assigned to the logical port by the IPAM.  Addresses will
-  be of the same format as those that populate the
-   column.  Note that these addresses are
+  Addresses assigned to the logical port by ovn-northd, if
+  dynamic is specified in .
+  Addresses will be of the same format as those that populate the  column.  Note that these addresses are
   constructed and managed locally in ovn-northd, so they cannot be
   reconstructed in the event that the database is lost.
 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] system-ovn.at: Fix ICMP conntrack output.

[Guru Shetty]

On 28 July 2016 at 10:48, Joe Stringer  wrote:

> Recent changes to the dump-conntrack command provide more info
> (type,code), but the system-ovn tests weren't updated for this.
> Update the tests.
>
> Signed-off-by: Joe Stringer 
>

Acked-by: Gurucharan Shetty 


> ---
>  tests/system-ovn.at | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index fd091b83d667..2a94d68a6e60 100755
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -111,7 +111,7 @@ NS_CHECK_EXEC([alice1], [ping -q -c 3 -i 0.3 -w 2
> 30.0.0.2 | FORMAT_PING], \
>  # Check conntrack entries.
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.2) | \
>  sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
>
> -icmp,orig=(src=172.16.1.2,dst=30.0.0.2,id=),reply=(src=192.168.1.2,dst=172.16.1.2,id=),zone=
>
> +icmp,orig=(src=172.16.1.2,dst=30.0.0.2,id=,type=8,code=0),reply=(src=192.168.1.2,dst=172.16.1.2,id=,type=0,code=0),zone=
>  ])
>
>  # South-North SNAT: 'bar1' pings 'alice1'. But 'alice1' receives traffic
> @@ -124,7 +124,7 @@ NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2
> 172.16.1.2 | FORMAT_PING], \
>  # We verify that SNAT indeed happened via 'dump-conntrack' command.
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \
>  sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
>
> -icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=),reply=(src=172.16.1.2,dst=30.0.0.1,id=),zone=
>
> +icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=,type=8,code=0),reply=(src=172.16.1.2,dst=30.0.0.1,id=,type=0,code=0),zone=
>  ])
>
>  # Add static routes to handle east-west NAT.
> @@ -143,14 +143,14 @@ NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2
> 30.0.0.2 | FORMAT_PING], \
>  # 30.0.0.2 to R2, it hits the DNAT rule and converts 30.0.0.2 to
> 192.168.1.2
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2) | \
>  sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
>
> -icmp,orig=(src=192.168.2.2,dst=30.0.0.2,id=),reply=(src=192.168.1.2,dst=192.168.2.2,id=),zone=
>
> +icmp,orig=(src=192.168.2.2,dst=30.0.0.2,id=,type=8,code=0),reply=(src=192.168.1.2,dst=192.168.2.2,id=,type=0,code=0),zone=
>  ])
>
>  # As we have a SNAT rule that converts 192.168.2.2 to 30.0.0.1, the
> source is
>  # SNATted and 'foo1' receives it.
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \
>  sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
>
> -icmp,orig=(src=192.168.2.2,dst=192.168.1.2,id=),reply=(src=192.168.1.2,dst=30.0.0.1,id=),zone=
>
> +icmp,orig=(src=192.168.2.2,dst=192.168.1.2,id=,type=8,code=0),reply=(src=192.168.1.2,dst=30.0.0.1,id=,type=0,code=0),zone=
>  ])
>
>  OVS_APP_EXIT_AND_WAIT([ovn-controller])
> @@ -255,7 +255,7 @@ NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2
> 172.16.1.2 | FORMAT_PING], \
>  # We verify that SNAT indeed happened via 'dump-conntrack' command.
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
>  sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
>
> -icmp,orig=(src=192.168.1.2,dst=172.16.1.2,id=),reply=(src=172.16.1.2,dst=172.16.1.1,id=),zone=
>
> +icmp,orig=(src=192.168.1.2,dst=172.16.1.2,id=,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=,type=0,code=0),zone=
>  ])
>
>  OVS_APP_EXIT_AND_WAIT([ovn-controller])
> --
> 2.9.0
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] ovs-dev.py: Update for python3.

[Joe Stringer]

On 28 July 2016 at 11:02, Russell Bryant  wrote:
>
>
> On Mon, May 16, 2016 at 5:56 PM, Joe Stringer  wrote:
>>
>> Adapt to python-2.6+, including support for 3.
>>
>> Signed-off-by: Joe Stringer 
>
>
> Acked-by: Russell Bryant 

Thanks for the review, it looks like this has already been pushed but
it's good to see it was in good shape ;)

If you are currently having some issue with ovs-dev.py and python3 on
a recent master then maybe this patch didn't cover that, so I suggest
you follow up with another patch to fix it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn-controller: Clean up cases that lead to duplicate OF flows.

[Ryan Moats]

In physical_run, there are multiple places where OF flows can be
produced each cycle.  Because the desired flow table may not have
been completely cleared first, remove flows created during previous
runs before creating new flows.  This avoid collisions.

Signed-off-by: Ryan Moats 
---
 ovn/controller/physical.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index e5df4f2..2e9fb73 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -603,6 +603,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id 
mff_ovn_geneve,
 if (!hc_uuid) {
 hc_uuid = xmalloc(sizeof(struct uuid));
 uuid_generate(hc_uuid);
+VLOG_INFO("Hard coded uuid is "UUID_FMT, UUID_ARGS(hc_uuid));
 }
 
 /* This bool tracks physical mapping changes. */
@@ -750,6 +751,10 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id 
mff_ovn_geneve,
 const struct sbrec_port_binding *binding;
 if (full_binding_processing) {
 SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
+/* Because it is possible in the above code to enter this
+ * for loop without having cleared the flow table first, we
+ * should clear the old flows to avoid collisions. */
+ofctrl_remove_flows(>header_.uuid);
 consider_port_binding(mff_ovn_geneve, ct_zones, local_datapaths,
   patched_datapaths, binding, );
 }
@@ -773,12 +778,21 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id 
mff_ovn_geneve,
 struct ofpbuf remote_ofpacts;
 ofpbuf_init(_ofpacts, 0);
 SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) {
+/* As multicast groups are always reprocessed each time,
+ * the first step is to clean the old flows for the group
+ * so that we avoid warning messages on collisions. */
+ofctrl_remove_flows(>header_.uuid);
 consider_mc_group(mff_ovn_geneve, ct_zones,
   local_datapaths, mc, , _ofpacts);
 }
 
 ofpbuf_uninit(_ofpacts);
 
+/* Because flows using the hard-coded uuid are recalculated each
+ * cycle, let's first remove the old flows to avoid duplicate flow
+ * warnings. */
+ofctrl_remove_flows(hc_uuid);
+
 /* Table 0, priority 100.
  * ==
  *
-- 
1.9.1

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

Re: [ovs-dev] [PATCH] ovn-controller: Add datapath-type in chassis:external_ids

[Russell Bryant]

On Thu, Jul 28, 2016 at 1:40 PM, Numan Siddique  wrote:

> On Thu, Jul 28, 2016 at 10:28 PM, Ben Pfaff  wrote:
>
> > On Thu, Jul 28, 2016 at 08:05:20PM +0530, Numan Siddique wrote:
> > > This patch reads the external_ids:datapath-type value from the
> > > Open_vSwitch table if defined and sets it in the
> > external_ids:datapath-type
> > > of Chassis table.
> >
> > What sets external_ids:datapath-type?  It isn't documented anywhere.
> >
>
> ​The way "ovn-bridge-mappings" is set, it is expected to set in the same
> way.
> I presume either the Administrator will set it or puppet or some other tool
> would set it.
> I will update the documentation accordingly.
>
>
We spoke about this briefly on IRC, but I think it would be better to read
the datapath_type column from the row in the Bridge table for br-int.


>
> > > This will provide hints to the CMS or clients monitoring OVN SB DB to
> > > determine the datapath type (DPDK or non-DPDK) configured and take some
> > > actions based on it.
> > >
> > > One usecase is, OVN neutron plugin can use this information to set the
> > > vif_type (ovs or vhostuser) during the port binding.
> >
> > Why would neutron need this information?  What does it use it for?
> >
>
> ​When a user creates a VM in an open stack deployment, open stack nova will
> select a compute host for the VM. Before booting the VM, nova asks neutron
> to create a port and also provides the host name. Neutron can read the
> datapath-type of the host from the OVN SB DB chassis table and tells nova
> if it is a dpdk host or normal host so that nova can plug the vif to the
> ovs bridge properly.
>
> With this approach, we can have openstack deployments with DPDK and
> non-DPDK compute hosts. Right now this mixed deployment is not supported.
>
> ​Russel, Ryan - Please correct me if I am wrong here.​


Yes, that's the use case.  It seems pretty round about, but this is easiest
way for us to integrate into OpenStack today.  Exposing it as an
external_id on the Chassis seemed like a pretty non-intrusive way to expose
the info.  Hopefully it's not *too* offensive.  :-)

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

Re: [ovs-dev] [PATCH 2/2] ovs-dev.py: Update for python3.

[Russell Bryant]

On Mon, May 16, 2016 at 5:56 PM, Joe Stringer  wrote:

> Adapt to python-2.6+, including support for 3.
>
> Signed-off-by: Joe Stringer 
>

Acked-by: Russell Bryant 

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

Re: [ovs-dev] [PATCH v5 00/16] Userspace (DPDK) connection tracker

[Daniele Di Proietto]

I pushed the intended version except that I forgot to squash that commit with
the previous one.  Sorry about this


On 28/07/2016 00:40, "Ilya Maximets"  wrote:

>Sorry.
>TO: Daniele Di Proietto 
>
>On 28.07.2016 09:27, Ilya Maximets wrote:
>> I guess, you pushed some development version of this patch set.
>> 
>> There is strange commit there:
>> 
>> commit 6c54734ed27bc22975d7035a6bd5f32a412335a0
>> Author: Daniele Di Proietto 
>> Date:   Wed Jul 27 18:32:15 2016 -0700
>> 
>> XXX Improve comment.
>> 
>> 
>> Best regards, Ilya Maximets.
>> 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Mea culpa - rewound master branch

[Daniele Di Proietto]

2016-07-28 10:52 GMT-07:00 Flavio Leitner :

> On Thu, Jul 28, 2016 at 09:32:54AM -0700, Ben Pfaff wrote:
> > On Thu, Jul 28, 2016 at 12:49:37PM -0300, Flavio Leitner wrote:
> > > On Wed, Jul 27, 2016 at 11:49:18AM -0700, Ben Pfaff wrote:
> > > > Some time ago this morning, I accidentally pushed several patches
> that
> > > > were still under review to master.  I've force-pushed a correction to
> > > > the branch.  My apologies--I hope that this does not screw up
> anyone's
> > > > development process in a bad way.
> > >
> > > There is another weird one:
> > > $ git log master..origin/master --oneline
> > > [...]
> > >   f3edb dpif-netdev: Execute conntrack action.
> > >   b412490 tests: Add test-conntrack pcap test.
> > >   8cb1462 tests: Add very simple conntrack benchmark.
> > > * 6c54734 XXX Improve comment.
> > >   e6ef6cc conntrack: Periodically delete expired connections.
> > >   a489b16 conntrack: New userspace connection tracker.
> > > [...]
> >
> > Probably should have been squashed before Daniele pushed it, but not
> > really so bad if that's the only issue.
>
> Yup, hopefully it is just that.
>
>
Sorry, I forgot to squash that commit before pushing it and when I realized
it, it was too late to do a forced push.

Apologies for the inconvenience guys,

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