Re: [ovs-dev] [PATCH 2/3] ofproto-dpif-rid: Store tunnel metadata in frozen metadata directly.

[Justin Pettit]

> On Jul 27, 2017, at 10:27 PM, Ben Pfaff  wrote:
> 
> On Thu, Jul 27, 2017 at 07:26:49PM -0700, Justin Pettit wrote:
>> 
>>> On Jul 27, 2017, at 4:24 PM, Ben Pfaff  wrote:
>>> 
>>> On Thu, Jul 13, 2017 at 11:30:50PM -0700, Justin Pettit wrote:
 "recirc_id_node" contains a 'state_metadata_tunnel' member field.  The
 "frozen_metadata" structure used by "recird_id_node" had a 'tunnel'
 member that always pointed to 'state_metadata_tunnel".  This commit just
 stores the tunnel information directly in "frozen_metadata" instead of
 accessing it through a pointer.
 
 Signed-off-by: Justin Pettit 
>>> 
>>> Would you mind adding something to the commit message to explain why
>>> this is a good thing?  I think I can guess, but rationale is important.
>>> 
>>> Acked-by: Ben Pfaff 
>> 
>> Thanks.  I pushed this to master with some added rationale.  I also
>> added the patch below to address some Travis errors when using an
>> older version of gcc.
> 
> Sweet, I see that my suggestion worked ;-)

Nailed it.  Thanks!

--Justin



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

Re: [ovs-dev] [PATCH 2/3] ofproto-dpif-rid: Store tunnel metadata in frozen metadata directly.

[Ben Pfaff]

On Thu, Jul 27, 2017 at 07:26:49PM -0700, Justin Pettit wrote:
> 
> > On Jul 27, 2017, at 4:24 PM, Ben Pfaff  wrote:
> > 
> > On Thu, Jul 13, 2017 at 11:30:50PM -0700, Justin Pettit wrote:
> >> "recirc_id_node" contains a 'state_metadata_tunnel' member field.  The
> >> "frozen_metadata" structure used by "recird_id_node" had a 'tunnel'
> >> member that always pointed to 'state_metadata_tunnel".  This commit just
> >> stores the tunnel information directly in "frozen_metadata" instead of
> >> accessing it through a pointer.
> >> 
> >> Signed-off-by: Justin Pettit 
> > 
> > Would you mind adding something to the commit message to explain why
> > this is a good thing?  I think I can guess, but rationale is important.
> > 
> > Acked-by: Ben Pfaff 
> 
> Thanks.  I pushed this to master with some added rationale.  I also
> added the patch below to address some Travis errors when using an
> older version of gcc.

Sweet, I see that my suggestion worked ;-)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovn-controller: Refactor function of consider_port_binding

[wang . qianyu]

The function of consider_port_binding is redundant. This patch divide the 
function to some sub-function by the port type.

Change-Id: I86a408e97e6d6211f3695cf42fc5b858352ac7ff
Signed-off-by: wang qianyu 
---
 ovn/controller/physical.c | 819 
+++---
 1 file changed, 484 insertions(+), 335 deletions(-)

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 719b020..28dcd5e 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2015, 2016, 2017 Nicira, Inc.
+锘?* Copyright (c) 2015, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -291,144 +291,200 @@ load_logical_ingress_metadata(const struct 
sbrec_port_binding *binding,
 }
 
 static void
-consider_port_binding(enum mf_field_id mff_ovn_geneve,
-  const struct simap *ct_zones,
+patch_port_handle(const struct simap *ct_zones,
   const struct lport_index *lports,
-  const struct chassis_index *chassis_index,
-  struct sset *active_tunnels,
-  struct hmap *local_datapaths,
   const struct sbrec_port_binding *binding,
-  const struct sbrec_chassis *chassis,
   struct ofpbuf *ofpacts_p,
-  struct hmap *flow_table)
+  struct hmap *flow_table,
+  uint32_t dp_key,
+  uint32_t port_key)
 {
-uint32_t dp_key = binding->datapath->tunnel_key;
-uint32_t port_key = binding->tunnel_key;
-if (!get_local_datapath(local_datapaths, dp_key)) {
+const char *peer_name = smap_get(>options, "peer");
+if (!peer_name) {
+return;
+}
+
+const struct sbrec_port_binding *peer = lport_lookup_by_name(
+lports, peer_name);
+if (!peer || strcmp(peer->type, binding->type)) {
+return;
+}
+const char *peer_peer_name = smap_get(>options, "peer");
+if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) 
{
 return;
 }
 
+struct zone_ids binding_zones = get_zone_ids(binding, ct_zones);
+put_local_common_flows(dp_key, port_key, false, _zones,
+   ofpacts_p, flow_table);
+
 struct match match;
-if (!strcmp(binding->type, "patch")
-|| (!strcmp(binding->type, "l3gateway")
-&& binding->chassis == chassis)) {
-const char *peer_name = smap_get(>options, "peer");
-if (!peer_name) {
-return;
-}
+match_init_catchall();
+ofpbuf_clear(ofpacts_p);
+match_set_metadata(, htonll(dp_key));
+match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key);
 
-const struct sbrec_port_binding *peer = lport_lookup_by_name(
-lports, peer_name);
-if (!peer || strcmp(peer->type, binding->type)) {
-return;
-}
-const char *peer_peer_name = smap_get(>options, "peer");
-if (!peer_peer_name || strcmp(peer_peer_name, 
binding->logical_port)) {
-return;
-}
+size_t clone_ofs = ofpacts_p->size;
+struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts_p);
+ofpact_put_CT_CLEAR(ofpacts_p);
+put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
+struct zone_ids peer_zones = get_zone_ids(peer, ct_zones);
+load_logical_ingress_metadata(peer, _zones, ofpacts_p);
+put_load(0, MFF_LOG_FLAGS, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
+for (int i = 0; i < MFF_N_LOG_REGS; i++) {
+put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p);
+}
+put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
+put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
+clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone);
+ofpacts_p->header = clone;
+ofpact_finish_CLONE(ofpacts_p, );
 
-struct zone_ids binding_zones = get_zone_ids(binding, ct_zones);
-put_local_common_flows(dp_key, port_key, false, _zones,
-   ofpacts_p, flow_table);
-
-match_init_catchall();
-ofpbuf_clear(ofpacts_p);
-match_set_metadata(, htonll(dp_key));
-match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key);
-
-size_t clone_ofs = ofpacts_p->size;
-struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts_p);
-ofpact_put_CT_CLEAR(ofpacts_p);
-put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
-put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
-put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
-struct zone_ids peer_zones = get_zone_ids(peer, ct_zones);
-load_logical_ingress_metadata(peer, _zones, ofpacts_p);
-

Re: [ovs-dev] [PATCH 3/3] ofproto-dpif-rid: Always store tunnel metadata.

[Justin Pettit]

> On Jul 27, 2017, at 4:27 PM, Ben Pfaff  wrote:
> 
> On Thu, Jul 13, 2017 at 11:30:51PM -0700, Justin Pettit wrote:
>> Tunnel metadata was only stored if the tunnel destination was set.  It's
>> possible, for example, that a flow could set the tunnel id field before
>> recirculation and then set the destination field afterwards.  The
>> previous behavior is that the tunnel id would be lost during
>> recirculation under such a circumstance.  This changes the behavior to
>> always copy the tunnel metadata, regardless of whether the tunnel
>> destination is set.  It also adds a unit test.
>> 
>> Signed-off-by: Justin Pettit 
> 
> Acked-by: Ben Pfaff 

Thanks.  I pushed this to master.

--Justin


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

Re: [ovs-dev] [PATCH 2/3] ofproto-dpif-rid: Store tunnel metadata in frozen metadata directly.

[Justin Pettit]

> On Jul 27, 2017, at 4:24 PM, Ben Pfaff  wrote:
> 
> On Thu, Jul 13, 2017 at 11:30:50PM -0700, Justin Pettit wrote:
>> "recirc_id_node" contains a 'state_metadata_tunnel' member field.  The
>> "frozen_metadata" structure used by "recird_id_node" had a 'tunnel'
>> member that always pointed to 'state_metadata_tunnel".  This commit just
>> stores the tunnel information directly in "frozen_metadata" instead of
>> accessing it through a pointer.
>> 
>> Signed-off-by: Justin Pettit 
> 
> Would you mind adding something to the commit message to explain why
> this is a good thing?  I think I can guess, but rationale is important.
> 
> Acked-by: Ben Pfaff 

Thanks.  I pushed this to master with some added rationale.  I also added the 
patch below to address some Travis errors when using an older version of gcc.

--Justin


-=-=-=-=-=-=-=-=-

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index 013534c511f0..5355a83a0f88 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -291,9 +291,12 @@ recirc_alloc_id(struct ofproto_dpif *ofproto)
 struct frozen_state state = {
 .table_id = TBL_INTERNAL,
 .ofproto_uuid = ofproto->uuid,
-.metadata = { .tunnel.ip_dst = htonl(0),
-  .tunnel.ipv6_dst = in6addr_any,
-  .in_port = OFPP_NONE },
+.metadata = {
+.tunnel = {
+.ip_dst = htonl(0),
+.ipv6_dst = in6addr_any,
+},
+.in_port = OFPP_NONE },
 };
 return recirc_alloc_id__(, frozen_state_hash())->id;


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

Re: [ovs-dev] [PATCH] ovn: Add support for ACL logging.

[Han Zhou]

On Thu, Jul 27, 2017 at 5:59 PM, Justin Pettit  wrote:
>
> > It seems "invalid" packets won't get logged in this patch. I think it
would be useful: in addition to log packets per-ACL, enable logging for
packets returned as "invalid" state from conntrack. It can be a global
configuration to enable/disable this behavior.
>
> That's a great point.  I'd meant to do that when I started this series,
but forgot along the way.  I think we need more than just logging for
invalid packets, though, so I'll add that as a follow-up patch.
>
Sure, I am ok with a follow-up patch. But I wonder what else is needed for
invalid packets beside logging? We are dropping them already.

> > >  static void
> > > +pinctrl_handle_log(struct dp_packet *packet OVS_UNUSED,
> >
> > Why need this parameter if it is not useful?
>
> Fixed.
>
> > > +   const struct flow *headers,
> > > +   struct ofpbuf *userdata)
> > > +{
> > > +struct log_pin_header *lph = ofpbuf_try_pull(userdata, sizeof
*lph);
> > > +if (!lph) {
> > > +VLOG_WARN("log data missing");
> > > +return;
> > > +}
> > > +
> > > +int name_len = ntohs(lph->name_len);
> > > +char *name = xmalloc(name_len+1);
> > > +if (name_len) {
> > > +char *tmp_name = ofpbuf_try_pull(userdata, name_len);
> > > +if (!name) {
> >
> > It should be: if (!tmp_name)
>
> Good catch.  Fixed.
>
> > > +VLOG_WARN("name missing");
> > > +free(name);
> > > +return;
> > > +}
> > > +memcpy(name, tmp_name, name_len);
> > > +}
> > > +name[name_len] = '\0';
> > > +
> > > +char *packet_str = flow_to_string(headers, NULL);
> > > +VLOG_INFO("ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"",
> >
> > The log action may be more generic than just for ACL. So would it
better to avoid hardcode "ACL" here in the message? "ACL" could be
indicated in verdict instead, since there could be more use cases that need
packet logging in the future.
>
> These are part of the ACL infrastructure, so I'd think ACL makes sense.
I also wanted to give some sort of handle that someone can easily grep
through the logs.  Do you have a specific suggestion?
>
I'd suggest to have a more general keyword e.g. "LOG", and include the
"ACL" keyword in the verdict itself, i.e. generated by
log_verdict_to_string(). So user can grep "LOG" for all packet logging, and
grep "ACL" for all ACL related packet logging.

> > > +enum log_verdict {
> > > +LOG_VERDICT_ALLOW,
> > > +LOG_VERDICT_DROP,
> > > +LOG_VERDICT_REJECT,
> >
> > Better to be LOG_VERDICT_ACL_ALLOW, LOG_VERDICT_ACL_DROP,
LOG_VERDICT_ACL_REJECT, so that in the future we can support other use
cases for logging.
>
> Do you have an example or two of the type of thing you think we might
need in the future?  If it needs to be that flexible, we could just pass
around strings, but I do worry about passing too much data through the
datapath upcall mechanism.
>
I have no real example yet. But since the action is defined as "log"
instead of "acl_log", I think it makes sense to be more generic. I don't
think we need to be more flexible but just make it more extensible. I don't
have strong opinion though. Maybe we can change it in the future if new
type of logging is needed. Or maybe there is use case from someone else?

> > >  static void
> > > +build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
> > > +{
> > > +if (!acl->log) {
> > > +return;
> > > +}
> > > +
> > > +ds_put_cstr(actions, "log(");
> > > +
> > > +if (acl->name) {
> > > +ds_put_format(actions, "name=\"%s\", ", acl->name);
> >
> > Would it be good to use first byte of ACL uuid (which is also used as
stage_hint) when name is empty?
>
> That's an interesting idea, but I have mixed feelings about it.  The
first byte would only allow 16 different identifiers, but I think generally
people have many more.  We could use more bytes from the UUID, but these
logs could last quite a while, so it may cause confusion if they've
changed, and people are trying to correlate them later.  Obviously, if they
really want to correlate them, they should use a name, but I wonder if it
would be just useful enough to be frustrating.
>
Sorry, I meant first 8 bytes, not just one :). Existing deployment may
already (e.g. openstack) stored information in external-ids, which could be
used to correlate. However, I agree with you that it is better to have
exactly one behavior.

> > >
> > > +
> > >
> > > -Logging is not yet implemented.
> > > +When  is true indicates the
> > > +severity of the ACL.  The severity levels match those of
syslog
> >
> > s/severity of the ACL/severity of the log/ because ACL doesn't have
severity.
>
> I'm worried that that phrasing may indicate that it's the level it will
logged at, when it's really just an indication by the user about how
concerned they are with this ACL (or 

[ovs-dev] [PATCH] ofproto-dpif-rid: Don't include action_set_len as part of hash.

[Justin Pettit]

It doesn't improve the hashing, since the number of bytes hashed is
included in hash_bytes64() hash calculation.

Signed-off-by: Justin Pettit 
---
 ofproto/ofproto-dpif-rid.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index d546b150b938..cc08772ce72a 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -141,7 +141,6 @@ frozen_state_hash(const struct frozen_state *state)
 hash = hash_bytes(state->stack, state->stack_size, hash);
 }
 hash = hash_int(state->mirrors, hash);
-hash = hash_int(state->action_set_len, hash);
 if (state->action_set_len) {
 hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->action_set),
 state->action_set_len, hash);
-- 
2.7.4

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

Re: [ovs-dev] [PATCH] ovn: Add support for ACL logging.

[Justin Pettit]

> On Jul 27, 2017, at 4:03 PM, Han Zhou  wrote:
> 
> 
> 
> On Wed, Jul 26, 2017 at 1:55 PM, Justin Pettit  wrote:
> >
> > Signed-off-by: Justin Pettit 
> > ---
> >  NEWS|   1 +
> >  include/ovn/actions.h   |  67 ---
> >  ovn/controller/ovn-controller.8.xml |   9 +++
> >  ovn/controller/pinctrl.c|  38 +++
> >  ovn/lib/actions.c   | 130 
> > 
> >  ovn/lib/automake.mk |   2 +
> >  ovn/lib/ovn-log.c   |  69 +++
> >  ovn/lib/ovn-log.h   |  52 +++
> >  ovn/northd/ovn-northd.c |  77 ++---
> >  ovn/ovn-nb.ovsschema|  15 -
> >  ovn/ovn-nb.xml  |  16 -
> >  ovn/ovn-sb.xml  |  32 +
> >  ovn/utilities/ovn-nbctl.8.xml   |  35 +++---
> >  ovn/utilities/ovn-nbctl.c   |  24 ++-
> >  ovn/utilities/ovn-trace.c   |  19 ++
> >  tests/ovn.at| 105 +
> >  16 files changed, 640 insertions(+), 51 deletions(-)
> >  create mode 100644 ovn/lib/ovn-log.c
> >  create mode 100644 ovn/lib/ovn-log.h
> >
> 
> Thanks for the patch!

Thanks for your patience on it.

> It seems "invalid" packets won't get logged in this patch. I think it would 
> be useful: in addition to log packets per-ACL, enable logging for packets 
> returned as "invalid" state from conntrack. It can be a global configuration 
> to enable/disable this behavior.

That's a great point.  I'd meant to do that when I started this series, but 
forgot along the way.  I think we need more than just logging for invalid 
packets, though, so I'll add that as a follow-up patch.

> >  static void
> > +pinctrl_handle_log(struct dp_packet *packet OVS_UNUSED,
> 
> Why need this parameter if it is not useful?

Fixed.

> > +   const struct flow *headers,
> > +   struct ofpbuf *userdata)
> > +{
> > +struct log_pin_header *lph = ofpbuf_try_pull(userdata, sizeof *lph);
> > +if (!lph) {
> > +VLOG_WARN("log data missing");
> > +return;
> > +}
> > +
> > +int name_len = ntohs(lph->name_len);
> > +char *name = xmalloc(name_len+1);
> > +if (name_len) {
> > +char *tmp_name = ofpbuf_try_pull(userdata, name_len);
> > +if (!name) {
> 
> It should be: if (!tmp_name)

Good catch.  Fixed.

> > +VLOG_WARN("name missing");
> > +free(name);
> > +return;
> > +}
> > +memcpy(name, tmp_name, name_len);
> > +}
> > +name[name_len] = '\0';
> > +
> > +char *packet_str = flow_to_string(headers, NULL);
> > +VLOG_INFO("ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"",
> 
> The log action may be more generic than just for ACL. So would it better to 
> avoid hardcode "ACL" here in the message? "ACL" could be indicated in verdict 
> instead, since there could be more use cases that need packet logging in the 
> future.

These are part of the ACL infrastructure, so I'd think ACL makes sense.  I also 
wanted to give some sort of handle that someone can easily grep through the 
logs.  Do you have a specific suggestion?

> > +enum log_verdict {
> > +LOG_VERDICT_ALLOW,
> > +LOG_VERDICT_DROP,
> > +LOG_VERDICT_REJECT,
> 
> Better to be LOG_VERDICT_ACL_ALLOW, LOG_VERDICT_ACL_DROP, 
> LOG_VERDICT_ACL_REJECT, so that in the future we can support other use cases 
> for logging.

Do you have an example or two of the type of thing you think we might need in 
the future?  If it needs to be that flexible, we could just pass around 
strings, but I do worry about passing too much data through the datapath upcall 
mechanism.

> >  static void
> > +build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
> > +{
> > +if (!acl->log) {
> > +return;
> > +}
> > +
> > +ds_put_cstr(actions, "log(");
> > +
> > +if (acl->name) {
> > +ds_put_format(actions, "name=\"%s\", ", acl->name);
> 
> Would it be good to use first byte of ACL uuid (which is also used as 
> stage_hint) when name is empty?

That's an interesting idea, but I have mixed feelings about it.  The first byte 
would only allow 16 different identifiers, but I think generally people have 
many more.  We could use more bytes from the UUID, but these logs could last 
quite a while, so it may cause confusion if they've changed, and people are 
trying to correlate them later.  Obviously, if they really want to correlate 
them, they should use a name, but I wonder if it would be just useful enough to 
be frustrating.

> >
> > +
> >
> > -Logging is not yet implemented.
> > +When  is true indicates the
> > +severity of the ACL.  The severity levels match those of syslog
> 
> s/severity of the ACL/severity of the 

Re: [ovs-dev] [PATCH] docs: Move restart related note to appropriate place.

[Darrell Ball]

I sent a patch here to consolidate and make more coherent the various 
pmd-cpu-mask references

https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/336367.html

This includes removing a reference to the pmd-cpu-mask under the setup section
and pointing the reader to the Performance Tuning section which already has an 
existing
dedicated section on pmd-cpu-mask usage.



-Original Message-
From:  on behalf of Ilya Maximets 

Date: Thursday, July 27, 2017 at 3:53 AM
To: "ovs-dev@openvswitch.org" , Stephen Finucane 

Cc: Ilya Maximets , Heetae Ahn 

Subject: [ovs-dev] [PATCH] docs: Move restart related note to appropriate   
place.

Currently, 'restart' related note is at the end of 'Setup OVS'
section. This makes the reader think that changing the
'pmd-cpu-mask' requires application restart.

It was mistakenly moved while converting to rST.

Fix that by moving the note closer to options it relates to.

CC: Stephen Finucane 
Fixes: 167703d664fc ("doc: Convert INSTALL.DPDK to rST")
Signed-off-by: Ilya Maximets 
---
 Documentation/intro/install/dpdk.rst | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/intro/install/dpdk.rst 
b/Documentation/intro/install/dpdk.rst
index a05aa1a..d2e5bfc 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -225,6 +225,10 @@ listed below. Defaults will be provided for all values 
not explicitly set.
 ``vhost-sock-dir``
   Option to set the path to the vhost-user unix socket files.
 
+.. note::
+  Changing any of these options requires restarting the ovs-vswitchd
+  application
+
 If allocating more than one GB hugepage, you can configure the
 amount of memory used from any given NUMA nodes. For example, to use 1GB 
from
 NUMA node 0 and 0GB for all other NUMA nodes, run::
@@ -247,10 +251,6 @@ threads and pin them to cores 1,2, run::
 Refer to ovs-vswitchd.conf.db(5) for additional information on 
configuration
 options.
 
-.. note::
-  Changing any of these options requires restarting the ovs-vswitchd
-  application
-
 Validating
 --
 
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=7TrfYgRUk38uVTBMof_OnOayR8rg3JLAxoRLRY79yhE=LyPyzjpMzmVtQxNsQ2Pe8072qXsbre1FjADyMVcRl18=
 


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

Re: [ovs-dev] [PATCH 1/2] travis: Explicitly disable LLVM for sparse build.

[Ben Pfaff]

Thanks for the reviews.  I fixed the trailing whitespace and pushed this
to master.

On Thu, Jul 27, 2017 at 02:50:25PM -0700, Justin Pettit wrote:
> BTW, it appears this does add some trailing whitespace.
> 
> --Justin
> 
> 
> > On Jul 27, 2017, at 2:16 PM, Justin Pettit  wrote:
> > 
> > 
> >> On Jul 27, 2017, at 1:41 PM, Ben Pfaff  wrote:
> >> 
> >> Newer travis environments claim to have LLVM support (llvm-config exists
> >> and works) but in reality don't, which prevents sparse from building and
> >> later parts of the build from succeeding.
> >> 
> >> Signed-off-by: Ben Pfaff 
> > 
> > Thanks for fixing this!
> > 
> > Acked-by: Justin Pettit 
> > 
> > --Justin
> > 
> > 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [patch_v1] docs/dpdk: Consolidate pmd-cpu-mask references.

[Darrell Ball]

The DPDK introductory documentation has various references to
pmd-cpu-mask, including a section devoted to it.  These parts of
the documentation seeemed to have been written at different times
and look like they were individually ported from other sources.
They all include an example command which gets repeated several times.
Here, we consolidate those referenes to make the documentation
easier to maintain. At the same time, create linkages to the
pmd-cpu-mask section from other sections to provide some level of
coherence.

Signed-off-by: Darrell Ball 
---
 Documentation/intro/install/dpdk.rst | 141 ---
 1 file changed, 65 insertions(+), 76 deletions(-)

diff --git a/Documentation/intro/install/dpdk.rst 
b/Documentation/intro/install/dpdk.rst
index a05aa1a..2e9cf8d 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -237,20 +237,12 @@ or::
 $ ovs-vsctl --no-wait set Open_vSwitch . \
 other_config:dpdk-socket-mem="1024"
 
-Similarly, if you wish to better scale the workloads across cores, then
-multiple pmd threads can be created and pinned to CPU cores by explicity
-specifying ``pmd-cpu-mask``. Cores are numbered from 0, so to spawn two pmd
-threads and pin them to cores 1,2, run::
-
-$ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x6
-
-Refer to ovs-vswitchd.conf.db(5) for additional information on configuration
-options.
-
 .. note::
   Changing any of these options requires restarting the ovs-vswitchd
   application
 
+See the section ``Performance Tuning`` for important DPDK customizations.
+
 Validating
 --
 
@@ -373,32 +365,6 @@ Now mount the huge pages, if not already done so::
 
 $ mount -t hugetlbfs -o pagesize=1G none /dev/hugepages
 
-Enable HyperThreading
-~
-
-With HyperThreading, or SMT, enabled, a physical core appears as two logical
-cores. SMT can be utilized to spawn worker threads on logical cores of the same
-physical core there by saving additional cores.
-
-With DPDK, when pinning pmd threads to logical cores, care must be taken to set
-the correct bits of the ``pmd-cpu-mask`` to ensure that the pmd threads are
-pinned to SMT siblings.
-
-Take a sample system configuration, with 2 sockets, 2 * 10 core processors, HT
-enabled. This gives us a total of 40 logical cores. To identify the physical
-core shared by two logical cores, run::
-
-$ cat /sys/devices/system/cpu/cpuN/topology/thread_siblings_list
-
-where ``N`` is the logical core number.
-
-In this example, it would show that cores ``1`` and ``21`` share the same
-physical core. As cores are counted from 0, the ``pmd-cpu-mask`` can be used
-to enable these two pmd threads running on these two logical cores (one
-physical core) is::
-
-$ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x22
-
 Isolate Cores
 ~
 
@@ -413,23 +379,6 @@ cmdline.
   It has been verified that core isolation has minimal advantage due to mature
   Linux scheduler in some circumstances.
 
-NUMA/Cluster-on-Die
-~~~
-
-Ideally inter-NUMA datapaths should be avoided where possible as packets will
-go across QPI and there may be a slight performance penalty when compared with
-intra NUMA datapaths. On Intel Xeon Processor E5 v3, Cluster On Die is
-introduced on models that have 10 cores or more.  This makes it possible to
-logically split a socket into two NUMA regions and again it is preferred where
-possible to keep critical datapaths within the one cluster.
-
-It is good practice to ensure that threads that are in the datapath are pinned
-to cores in the same NUMA area. e.g. pmd threads and QEMU vCPUs responsible for
-forwarding. If DPDK is built with ``CONFIG_RTE_LIBRTE_VHOST_NUMA=y``, vHost
-User ports automatically detect the NUMA socket of the QEMU vCPUs and will be
-serviced by a PMD from the same node provided a core on this node is enabled in
-the ``pmd-cpu-mask``. ``libnuma`` packages are required for this feature.
-
 Compiler Optimizations
 ~~
 
@@ -439,6 +388,31 @@ gcc (verified on 5.3.1) can produce performance gains 
though not siginificant.
 ``-march=native`` will produce optimized code on local machine and should be
 used when software compilation is done on Testbed.
 
+Multiple Poll-Mode Driver Threads
+~
+
+With pmd multi-threading support, OVS creates one pmd thread for each NUMA node
+by default, if there is at least one DPDK interface from that NUMA node added
+to OVS. However, in cases where there are multiple ports/rxq's producing
+traffic, performance can be improved by creating multiple pmd threads running
+on separate cores. These pmd threads can share the workload by each being
+responsible for different ports/rxq's. Assignment of ports/rxq's to pmd threads
+is done automatically.
+
+A set bit in the mask means a pmd thread is created and pinned to the
+corresponding CPU core. For 

Re: [ovs-dev] [PATCH v2] datapath-windows: Refactor OvsCreateNewNBLsFromMultipleNBs

[Anand Kumar]

Thanks for refactoring it.

Acked-by: Anand Kumar 

Thanks,
Anand Kumar

On 7/24/17, 3:31 PM, "ovs-dev-boun...@openvswitch.org on behalf of Shashank 
Ram"  wrote:

Previously, the function would take the curNbl and nextNbl
as inputs, and modify the linked list, and merge the input
linked list with the newly generated newNbl list.

This is confusing for the caller, and the function has
unnecessary logic for merging linked lists that instead
the caller should take care of. This is because the
OvsCreateNewNBLsFromMultipleNBs() is a generic API
that can be used by other functions as well, and its
natural for different callers to have different needs.

This patch refactors the behavior of OvsCreateNewNBLsFromMultipleNBs
to take in the curNbl and lastNbl, and it returns
a linked list of NBLs and sets the HEAD and TAIL of the
new list obtained from the curNbl. If the caller wants
to chain a new linked list at the HEAD or TAIL, it
can make use of the curNbl and lastNbl to do so.

Signed-off-by: Shashank Ram 
---
 datapath-windows/ovsext/PacketIO.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/datapath-windows/ovsext/PacketIO.c 
b/datapath-windows/ovsext/PacketIO.c
index a90b556..81c574e 100644
--- a/datapath-windows/ovsext/PacketIO.c
+++ b/datapath-windows/ovsext/PacketIO.c
@@ -49,7 +49,7 @@ static VOID OvsCompleteNBLIngress(POVS_SWITCH_CONTEXT 
switchContext,
 static NTSTATUS OvsCreateNewNBLsFromMultipleNBs(
 POVS_SWITCH_CONTEXT switchContext,
 PNET_BUFFER_LIST *curNbl,
-PNET_BUFFER_LIST *nextNbl);
+PNET_BUFFER_LIST *lastNbl);
 
 VOID
 OvsInitCompletionList(OvsCompletionList *completionList,
@@ -212,7 +212,7 @@ OvsStartNBLIngress(POVS_SWITCH_CONTEXT switchContext,
 NDIS_SWITCH_PORT_ID sourcePort = 0;
 NDIS_SWITCH_NIC_INDEX sourceIndex = 0;
 PNDIS_SWITCH_FORWARDING_DETAIL_NET_BUFFER_LIST_INFO fwdDetail;
-PNET_BUFFER_LIST curNbl = NULL, nextNbl = NULL;
+PNET_BUFFER_LIST curNbl = NULL, nextNbl = NULL, lastNbl = NULL;
 ULONG sendCompleteFlags;
 UCHAR dispatch;
 LOCK_STATE_EX lockState, dpLockState;
@@ -282,7 +282,7 @@ OvsStartNBLIngress(POVS_SWITCH_CONTEXT switchContext,
 /* Create a NET_BUFFER_LIST for each NET_BUFFER. */
 status = OvsCreateNewNBLsFromMultipleNBs(switchContext,
  ,
- );
+ );
 if (!NT_SUCCESS(status)) {
 RtlInitUnicodeString(,
  L"Cannot allocate NBLs with single 
NB.");
@@ -292,6 +292,10 @@ OvsStartNBLIngress(POVS_SWITCH_CONTEXT switchContext,
 NDIS_STATUS_RESOURCES);
 continue;
 }
+
+lastNbl->Next = nextNbl;
+nextNbl = curNbl->Next;
+curNbl->Next = NULL;
 }
 {
 OvsFlow *flow;
@@ -500,11 +504,10 @@ OvsExtCancelSendNBL(NDIS_HANDLE filterModuleContext,
 static NTSTATUS
 OvsCreateNewNBLsFromMultipleNBs(POVS_SWITCH_CONTEXT switchContext,
 PNET_BUFFER_LIST *curNbl,
-PNET_BUFFER_LIST *nextNbl)
+PNET_BUFFER_LIST *lastNbl)
 {
 NTSTATUS status = STATUS_SUCCESS;
 PNET_BUFFER_LIST newNbls = NULL;
-PNET_BUFFER_LIST lastNbl = NULL;
 PNET_BUFFER_LIST nbl = NULL;
 BOOLEAN error = TRUE;
 
@@ -520,16 +523,15 @@ OvsCreateNewNBLsFromMultipleNBs(POVS_SWITCH_CONTEXT 
switchContext,
 
 nbl = newNbls;
 while (nbl) {
-lastNbl = nbl;
+*lastNbl = nbl;
 nbl = NET_BUFFER_LIST_NEXT_NBL(nbl);
 }
-lastNbl->Next = *nextNbl;
-*nextNbl = newNbls->Next;
+
+(*curNbl)->Next = NULL;
 
 OvsCompleteNBL(switchContext, *curNbl, TRUE);
 
 *curNbl = newNbls;
-(*curNbl)->Next = NULL;
 
 error = FALSE;
 } while (error);
-- 
2.9.3.windows.2

___
dev mailing list
d...@openvswitch.org

Re: [ovs-dev] [PATCH 3/3] ofproto-dpif-rid: Always store tunnel metadata.

[Ben Pfaff]

On Thu, Jul 13, 2017 at 11:30:51PM -0700, Justin Pettit wrote:
> Tunnel metadata was only stored if the tunnel destination was set.  It's
> possible, for example, that a flow could set the tunnel id field before
> recirculation and then set the destination field afterwards.  The
> previous behavior is that the tunnel id would be lost during
> recirculation under such a circumstance.  This changes the behavior to
> always copy the tunnel metadata, regardless of whether the tunnel
> destination is set.  It also adds a unit test.
> 
> Signed-off-by: Justin Pettit 

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

Re: [ovs-dev] [PATCH 2/3] ofproto-dpif-rid: Store tunnel metadata in frozen metadata directly.

[Ben Pfaff]

On Thu, Jul 13, 2017 at 11:30:50PM -0700, Justin Pettit wrote:
> "recirc_id_node" contains a 'state_metadata_tunnel' member field.  The
> "frozen_metadata" structure used by "recird_id_node" had a 'tunnel'
> member that always pointed to 'state_metadata_tunnel".  This commit just
> stores the tunnel information directly in "frozen_metadata" instead of
> accessing it through a pointer.
> 
> Signed-off-by: Justin Pettit 

Would you mind adding something to the commit message to explain why
this is a good thing?  I think I can guess, but rationale is important.

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

Re: [ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Justin Pettit]

> On Jul 27, 2017, at 4:17 PM, Ben Pfaff  wrote:
> 
> On Thu, Jul 27, 2017 at 02:05:22PM -0700, Justin Pettit wrote:
>> 
>>> On Jul 27, 2017, at 1:54 PM, Ben Pfaff  wrote:
>>> 
>>> On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
 Signed-off-by: Justin Pettit 
 ---
 ofproto/ofproto-dpif-rid.c | 1 +
 1 file changed, 1 insertion(+)
 
 diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
 index d546b150b938..26c2357007b2 100644
 --- a/ofproto/ofproto-dpif-rid.c
 +++ b/ofproto/ofproto-dpif-rid.c
 @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, 
 state->action_set),
state->action_set_len, hash);
}
 +hash = hash_int(state->ofpacts_len, hash);
if (state->ofpacts_len) {
hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
state->ofpacts_len, hash);
>>> 
>>> Can you explain the benefit of this change?  hash_bytes64() already uses
>>> the number of bytes hashed as one of the inputs to the hash.
>> 
>> hash_bytes64() is only called if the action length is non-zero.
> 
> Yes, but even so, the hash should still distinguish states with no
> actions from states with actions.

Yes, I agree.

>> However, I was on the fence about making this change, since it wasn't
>> clear if it would be that valuable.  The main reason was just to make
>> it consistent with how "action_set" is handled right above it.
>> 
>> I'm happy to drop the patch if you prefer.
> 
> I'd be more inclined to remove the hash of action_set_len, because it is
> unnecessary for the same reason that hash of ofpacts_len is unnecessary.

I'll go ahead and do that.  I'll send out a v2, since I need to respin the next 
patch in the series due to a build issue on some versions of gcc.

--Justin


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

[ovs-dev] [PATCH] ovsdb-server: Document clarification for some bad wording in RFC 7047.

[Ben Pfaff]

Reported-by: Harish Kanakaraju 
Signed-off-by: Ben Pfaff 
---
 AUTHORS.rst | 1 +
 ovsdb/ovsdb-server.1.in | 8 
 2 files changed, 9 insertions(+)

diff --git a/AUTHORS.rst b/AUTHORS.rst
index 10c84c185262..8ec7db84e9ea 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -424,6 +424,7 @@ Gregor Schaffrath   
gr...@net.t-labs.tu-berlin.de
 Gregory Smith   gasm...@nutanix.com
 Guolin Yang gy...@vmware.com
 Gur Stavi   gst...@mrv.com
+Harish Kanakaraju   hkanakar...@vmware.com
 Hari Sasank Bhamidipallihbham...@cisco.com
 Hassan Khan hassan.k...@seecs.edu.pk
 Hector Oron hector.o...@gmail.com
diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
index c655425e13e7..fd284a0ff0ab 100644
--- a/ovsdb/ovsdb-server.1.in
+++ b/ovsdb/ovsdb-server.1.in
@@ -667,6 +667,14 @@ A condition can be either a 3-element JSON array as 
described in the RFC or a
 boolean value. In case of an empty array an implicit true boolean value will be
 considered.
 .
+.IP "5.2.6. Wait"
+.IQ "5.2.7. Commit"
+.IQ "5.2.9. Comment"
+RFC 7047 says that the \fBwait\fR, \fBcommit\fR, and \fBcomment\fR
+operations have no corresponding result object.  This is not true.
+Instead, when such an operation is successful, it yields a result
+object with no members.
+.
 .SH "BUGS"
 .
 In Open vSwitch before version 2.4, when \fBovsdb\-server\fR sent
-- 
2.10.2

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

Re: [ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Ben Pfaff]

On Thu, Jul 27, 2017 at 02:05:22PM -0700, Justin Pettit wrote:
> 
> > On Jul 27, 2017, at 1:54 PM, Ben Pfaff  wrote:
> > 
> > On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
> >> Signed-off-by: Justin Pettit 
> >> ---
> >> ofproto/ofproto-dpif-rid.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> >> index d546b150b938..26c2357007b2 100644
> >> --- a/ofproto/ofproto-dpif-rid.c
> >> +++ b/ofproto/ofproto-dpif-rid.c
> >> @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
> >> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, 
> >> state->action_set),
> >> state->action_set_len, hash);
> >> }
> >> +hash = hash_int(state->ofpacts_len, hash);
> >> if (state->ofpacts_len) {
> >> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
> >> state->ofpacts_len, hash);
> > 
> > Can you explain the benefit of this change?  hash_bytes64() already uses
> > the number of bytes hashed as one of the inputs to the hash.
> 
> hash_bytes64() is only called if the action length is non-zero.

Yes, but even so, the hash should still distinguish states with no
actions from states with actions.

> However, I was on the fence about making this change, since it wasn't
> clear if it would be that valuable.  The main reason was just to make
> it consistent with how "action_set" is handled right above it.
>
> I'm happy to drop the patch if you prefer.

I'd be more inclined to remove the hash of action_set_len, because it is
unnecessary for the same reason that hash of ofpacts_len is unnecessary.

Thanks,

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

Re: [ovs-dev] [PATCH] ovn: Add support for ACL logging.

[Han Zhou]

On Wed, Jul 26, 2017 at 1:55 PM, Justin Pettit  wrote:
>
> Signed-off-by: Justin Pettit 
> ---
>  NEWS|   1 +
>  include/ovn/actions.h   |  67 ---
>  ovn/controller/ovn-controller.8.xml |   9 +++
>  ovn/controller/pinctrl.c|  38 +++
>  ovn/lib/actions.c   | 130

>  ovn/lib/automake.mk |   2 +
>  ovn/lib/ovn-log.c   |  69 +++
>  ovn/lib/ovn-log.h   |  52 +++
>  ovn/northd/ovn-northd.c |  77 ++---
>  ovn/ovn-nb.ovsschema|  15 -
>  ovn/ovn-nb.xml  |  16 -
>  ovn/ovn-sb.xml  |  32 +
>  ovn/utilities/ovn-nbctl.8.xml   |  35 +++---
>  ovn/utilities/ovn-nbctl.c   |  24 ++-
>  ovn/utilities/ovn-trace.c   |  19 ++
>  tests/ovn.at| 105 +
>  16 files changed, 640 insertions(+), 51 deletions(-)
>  create mode 100644 ovn/lib/ovn-log.c
>  create mode 100644 ovn/lib/ovn-log.h
>

Thanks for the patch!

It seems "invalid" packets won't get logged in this patch. I think it would
be useful: in addition to log packets per-ACL, enable logging for packets
returned as "invalid" state from conntrack. It can be a global
configuration to enable/disable this behavior.

Please see my other comments inlined:


>  static void
> +pinctrl_handle_log(struct dp_packet *packet OVS_UNUSED,

Why need this parameter if it is not useful?

> +   const struct flow *headers,
> +   struct ofpbuf *userdata)
> +{
> +struct log_pin_header *lph = ofpbuf_try_pull(userdata, sizeof *lph);
> +if (!lph) {
> +VLOG_WARN("log data missing");
> +return;
> +}
> +
> +int name_len = ntohs(lph->name_len);
> +char *name = xmalloc(name_len+1);
> +if (name_len) {
> +char *tmp_name = ofpbuf_try_pull(userdata, name_len);
> +if (!name) {

It should be: if (!tmp_name)

> +VLOG_WARN("name missing");
> +free(name);
> +return;
> +}
> +memcpy(name, tmp_name, name_len);
> +}
> +name[name_len] = '\0';
> +
> +char *packet_str = flow_to_string(headers, NULL);
> +VLOG_INFO("ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"",

The log action may be more generic than just for ACL. So would it better to
avoid hardcode "ACL" here in the message? "ACL" could be indicated in
verdict instead, since there could be more use cases that need packet
logging in the future.

> +  name_len ? name : "",
> +  log_verdict_to_string(lph->verdict),
> +  log_severity_to_string(lph->severity), packet_str);
> +free(packet_str);
> +free(name);
> +}


> +
> +enum log_verdict {
> +LOG_VERDICT_ALLOW,
> +LOG_VERDICT_DROP,
> +LOG_VERDICT_REJECT,

Better to be LOG_VERDICT_ACL_ALLOW, LOG_VERDICT_ACL_DROP,
LOG_VERDICT_ACL_REJECT, so that in the future we can support other use
cases for logging.

> +LOG_VERDICT_UNKNOWN = UINT8_MAX
> +};


>  static void
> +build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
> +{
> +if (!acl->log) {
> +return;
> +}
> +
> +ds_put_cstr(actions, "log(");
> +
> +if (acl->name) {
> +ds_put_format(actions, "name=\"%s\", ", acl->name);

Would it be good to use first byte of ACL uuid (which is also used as
stage_hint) when name is empty?


>
> +
>
> -Logging is not yet implemented.
> +When  is true indicates the
> +severity of the ACL.  The severity levels match those of syslog

s/severity of the ACL/severity of the log/ because ACL doesn't have
severity.

> +with the following values (from more to less serious):
> +alert, warning, notice,
> +info, or debug.  If a severity is not
> +specified, the default is info.
>
>  
>


> @@ -332,7 +333,7 @@ Logical switch commands:\n\
>ls-list   print the names of all logical switches\n\
>  \n\
>  ACL commands:\n\
> -  acl-add SWITCH DIRECTION PRIORITY MATCH ACTION [log]\n\
> +  acl-add [--log] SWITCH DIRECTION PRIORITY MATCH ACTION\n\

The optional "name" and "severity" are missing in the help message.

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

Re: [ovs-dev] flow: Refactor flow_compose() API.

[Andy Zhou]

>
> I think, it's better to use 'dp_packet_delete(packet)' instead of two
> lines above. 'delete' is the better pair for 'new'. 'uninit' is mostly
> for packets allocated statically and initialized by 'dp_packet_init()'.
>
> Additionally this will help to avoid issues if 'dp_packet_new' will
> be able to allocate DPDK memory someday.

You are right. I folded the change in. Thanks for the suggestion.
>
> With this change:
> Acked-by: Ilya Maximets 
>
Thanks for the review. Pushed to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] travis: Fail the build if any of the Linux build preparations fail.

[Justin Pettit]

> On Jul 27, 2017, at 1:41 PM, Ben Pfaff  wrote:
> 
> We want the build to fail if we can't prepare properly for it, but
> linux-prepare.sh ignored errors.  This fixes the problem.
> 
> This would have made it more obvious where the problem fixed by the
> previous commit originated.
> 
> (osx-prepare.sh already does the right thing.)
> 
> Signed-off-by: Ben Pfaff 

Acked-by: Justin Pettit 

--Justin




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

Re: [ovs-dev] [PATCH 1/2] travis: Explicitly disable LLVM for sparse build.

[Justin Pettit]

> On Jul 27, 2017, at 1:41 PM, Ben Pfaff  wrote:
> 
> Newer travis environments claim to have LLVM support (llvm-config exists
> and works) but in reality don't, which prevents sparse from building and
> later parts of the build from succeeding.
> 
> Signed-off-by: Ben Pfaff 

Thanks for fixing this!

Acked-by: Justin Pettit 

--Justin


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

Re: [ovs-dev] [PATCH] netlink: Correct comment for nl_msg_put_unspec().

[Justin Pettit]

> On Jul 27, 2017, at 2:11 PM, Joe Stringer  wrote:
> 
> On 27 July 2017 at 13:15, Justin Pettit  wrote:
>> Signed-off-by: Justin Pettit 
>> ---
> 
> Acked-by: Joe Stringer 

Thanks.  Pushed to master.

--Justin


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

Re: [ovs-dev] [PATCH] netlink: Correct comment for nl_msg_put_unspec().

[Joe Stringer]

On 27 July 2017 at 13:15, Justin Pettit  wrote:
> Signed-off-by: Justin Pettit 
> ---

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

Re: [ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Justin Pettit]

> On Jul 27, 2017, at 1:54 PM, Ben Pfaff  wrote:
> 
> On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit 
>> ---
>> ofproto/ofproto-dpif-rid.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
>> index d546b150b938..26c2357007b2 100644
>> --- a/ofproto/ofproto-dpif-rid.c
>> +++ b/ofproto/ofproto-dpif-rid.c
>> @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
>> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, 
>> state->action_set),
>> state->action_set_len, hash);
>> }
>> +hash = hash_int(state->ofpacts_len, hash);
>> if (state->ofpacts_len) {
>> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
>> state->ofpacts_len, hash);
> 
> Can you explain the benefit of this change?  hash_bytes64() already uses
> the number of bytes hashed as one of the inputs to the hash.

hash_bytes64() is only called if the action length is non-zero.  However, I was 
on the fence about making this change, since it wasn't clear if it would be 
that valuable.  The main reason was just to make it consistent with how 
"action_set" is handled right above it. 

I'm happy to drop the patch if you prefer.

--Justin


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

Re: [ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Ben Pfaff]

On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit 
> ---
>  ofproto/ofproto-dpif-rid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> index d546b150b938..26c2357007b2 100644
> --- a/ofproto/ofproto-dpif-rid.c
> +++ b/ofproto/ofproto-dpif-rid.c
> @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
>  hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, 
> state->action_set),
>  state->action_set_len, hash);
>  }
> +hash = hash_int(state->ofpacts_len, hash);
>  if (state->ofpacts_len) {
>  hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
>  state->ofpacts_len, hash);

Can you explain the benefit of this change?  hash_bytes64() already uses
the number of bytes hashed as one of the inputs to the hash.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/2] travis: Explicitly disable LLVM for sparse build.

[Ben Pfaff]

Newer travis environments claim to have LLVM support (llvm-config exists
and works) but in reality don't, which prevents sparse from building and
later parts of the build from succeeding.

Signed-off-by: Ben Pfaff 
---
 .travis/linux-prepare.sh | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/.travis/linux-prepare.sh b/.travis/linux-prepare.sh
index fa30972bdbfd..b73088d4a132 100755
--- a/.travis/linux-prepare.sh
+++ b/.travis/linux-prepare.sh
@@ -1,7 +1,12 @@
 #!/bin/bash
 
+# Build and install sparse.
+# 
+# Explicitly disable sparse support for llvm because some travis
+# environments claim to have LLVM (llvm-config exists and works) but
+# linking against it fails.
 git clone git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git
-cd sparse && make && make install && cd ..
+cd sparse && make HAVE_LLVM= install && cd ..
 
 pip install --disable-pip-version-check --user six flake8 hacking
 pip install --user --upgrade docutils
-- 
2.10.2

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

[ovs-dev] [PATCH] netlink: Correct comment for nl_msg_put_unspec().

[Justin Pettit]

Signed-off-by: Justin Pettit 
---
 lib/netlink.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/netlink.c b/lib/netlink.c
index 4cf1aaca621c..de3ebcd0e792 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -235,8 +235,7 @@ nl_msg_put_unspec_zero(struct ofpbuf *msg, uint16_t type, 
size_t size)
 
 /* Appends a Netlink attribute of the given 'type' and the 'size' bytes of
  * 'data' as its payload, to the tail end of 'msg', reallocating and copying
- * its data if necessary.  Returns a pointer to the first byte of data in the
- * attribute, which is left uninitialized. */
+ * its data if necessary. */
 void
 nl_msg_put_unspec(struct ofpbuf *msg, uint16_t type,
   const void *data, size_t size)
-- 
2.7.4

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

Re: [ovs-dev] [PATCH] dpif-netlink: Fix log level for error message

[Eric Garver]

On Thu, Jul 27, 2017 at 10:17:48AM -0700, Joe Stringer wrote:
> On 27 July 2017 at 05:43, Eric Garver  wrote:
> > On Thu, Jul 27, 2017 at 10:27:18AM +0300, Roi Dayan wrote:
> >> Since it's an error message log it with VLOG_ERR instead of VLOG_INFO.
> >>
> >> Signed-off-by: Roi Dayan 
> >> ---
> >>  lib/dpif-netlink.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> >> index 55effd1..7b53ed5 100644
> >> --- a/lib/dpif-netlink.c
> >> +++ b/lib/dpif-netlink.c
> >> @@ -972,8 +972,8 @@ dpif_netlink_rtnl_port_create_and_add(struct 
> >> dpif_netlink *dpif,
> >>  error = dpif_netlink_rtnl_port_create(netdev);
> >>  if (error) {
> >>  if (error != EOPNOTSUPP) {
> >> -VLOG_INFO_RL(, "Failed to create %s with rtnetlink: %s",
> >> - netdev_get_name(netdev), ovs_strerror(error));
> >> +VLOG_ERR_RL(, "Failed to create %s with rtnetlink: %s",
> >> +netdev_get_name(netdev), ovs_strerror(error));
> >>  }
> >>  return error;
> >>  }
> >> --
> >> 2.7.4
> >
> > I don't think this is appropriate. If creating with rtnetlink fails
> > we'll fall back to using the compat interface. As such, we may still
> > successfully create the tunnel. Maybe WARN would be more appropriate?
> 
> This probably also depends on which kernels we think are most common.
> If the next release we expect to be deployed with kernels that don't
> support rtnetlink+metadata_dst for the tunnel types we want, then
> raising the log level doesn't sound appropriate. But if we expect that
> in most cases people are deploying with either out-of-tree modules, or
> with kernels that support rtnetlink+metadata_dst then we could
> consider raising the log level, for instance to WARN at first and
> maybe ERR later.

COLLECT_METADATA for GENEVE, VXLAN, and GRE was introduced in v4.3,
which was tagged Nov 1, 2015. I would guess many people are using a new
enough kernel or they're using the out-of-tree modules.

As such, I'm okay with making this log a WARN.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] docs: Move restart related note to appropriate place.

[Darrell Ball]

-Original Message-
From:  on behalf of Ilya Maximets 

Date: Thursday, July 27, 2017 at 3:53 AM
To: "ovs-dev@openvswitch.org" , Stephen Finucane 

Cc: Ilya Maximets , Heetae Ahn 

Subject: [ovs-dev] [PATCH] docs: Move restart related note to appropriate   
place.

Currently, 'restart' related note is at the end of 'Setup OVS'
section. This makes the reader think that changing the
'pmd-cpu-mask' requires application restart.

It was mistakenly moved while converting to rST.

Fix that by moving the note closer to options it relates to.

CC: Stephen Finucane 
Fixes: 167703d664fc ("doc: Convert INSTALL.DPDK to rST")
Signed-off-by: Ilya Maximets 
---
 Documentation/intro/install/dpdk.rst | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/intro/install/dpdk.rst 
b/Documentation/intro/install/dpdk.rst
index a05aa1a..d2e5bfc 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -225,6 +225,10 @@ listed below. Defaults will be provided for all values 
not explicitly set.
 ``vhost-sock-dir``
   Option to set the path to the vhost-user unix socket files.
 
+.. note::
+  Changing any of these options requires restarting the ovs-vswitchd
+  application
+

pmd-cpu-mask is also discussed in its own section under the ‘Multiple Poll-Mode 
Driver Threads’ in the same page.
‘Affinity’ in the same page also discusses it.
Maybe a cross references to ‘Multiple Poll-Mode Driver Threads’ from ‘Affinity’ 
is enough and some movement of
sections.
I’ll send a patch.


 If allocating more than one GB hugepage, you can configure the
 amount of memory used from any given NUMA nodes. For example, to use 1GB 
from
 NUMA node 0 and 0GB for all other NUMA nodes, run::
@@ -247,10 +251,6 @@ threads and pin them to cores 1,2, run::
 Refer to ovs-vswitchd.conf.db(5) for additional information on 
configuration
 options.
 
-.. note::
-  Changing any of these options requires restarting the ovs-vswitchd
-  application
-
 Validating
 --
 
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=7TrfYgRUk38uVTBMof_OnOayR8rg3JLAxoRLRY79yhE=LyPyzjpMzmVtQxNsQ2Pe8072qXsbre1FjADyMVcRl18=
 


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

Re: [ovs-dev] Revisit OVN meeting schedule?

[Justin Pettit]

> On Jul 26, 2017, at 8:07 AM, Russell Bryant  wrote:
> 
> It has been difficult for some of our newer contributors in Europe to
> make our weekly OVN IRC meeting, so I wanted to revisit the schedule.
> Roughly we have 2 options to consider:
> 
> 1) Change the time to accommodate more contributors.  My suggestion to
> consider would be 8am Pacific / 11am Eastern (meeting 2 hours earlier
> than currently).  See the following page for an overview of time zone
> overlap of some of our contributors.
> 
> https://www.timeanddate.com/worldclock/meetingtime.html?iso=20170726=1082=54=141=405=224
> 
> 2) Consider a rotating schedule, where we have 2 different meeting
> times that rotate each week.  I can work out a proposal here, but
> let's see what people think of #1 first.
> 
> Please share your thoughts.

Option 1 isn't very convenient for me when school starts for my kids.  Would 
9AM Pacific work?  I know Ben has multiple meetings on Thursday at that time, 
but we could find another day.

I'm not sure how Option 2 works in practice, since I haven't done meetings that 
way in the past.

--Justin


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

Re: [ovs-dev] [PATCH] dpif-netlink: Fix log level for error message

[Joe Stringer]

On 27 July 2017 at 05:43, Eric Garver  wrote:
> On Thu, Jul 27, 2017 at 10:27:18AM +0300, Roi Dayan wrote:
>> Since it's an error message log it with VLOG_ERR instead of VLOG_INFO.
>>
>> Signed-off-by: Roi Dayan 
>> ---
>>  lib/dpif-netlink.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 55effd1..7b53ed5 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -972,8 +972,8 @@ dpif_netlink_rtnl_port_create_and_add(struct 
>> dpif_netlink *dpif,
>>  error = dpif_netlink_rtnl_port_create(netdev);
>>  if (error) {
>>  if (error != EOPNOTSUPP) {
>> -VLOG_INFO_RL(, "Failed to create %s with rtnetlink: %s",
>> - netdev_get_name(netdev), ovs_strerror(error));
>> +VLOG_ERR_RL(, "Failed to create %s with rtnetlink: %s",
>> +netdev_get_name(netdev), ovs_strerror(error));
>>  }
>>  return error;
>>  }
>> --
>> 2.7.4
>
> I don't think this is appropriate. If creating with rtnetlink fails
> we'll fall back to using the compat interface. As such, we may still
> successfully create the tunnel. Maybe WARN would be more appropriate?

This probably also depends on which kernels we think are most common.
If the next release we expect to be deployed with kernels that don't
support rtnetlink+metadata_dst for the tunnel types we want, then
raising the log level doesn't sound appropriate. But if we expect that
in most cases people are deploying with either out-of-tree modules, or
with kernels that support rtnetlink+metadata_dst then we could
consider raising the log level, for instance to WARN at first and
maybe ERR later.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovn: Restrict encap modification to its creating chassis

[Russell Bryant]

On Wed, Jul 26, 2017 at 5:05 PM, Mark Michelson  wrote:
> This patch extends RBAC restrictiveness of the encap table in
> the ovn southbound database by only allowing modification by the
> chassis that created the encap.
>
> Signed-off-by: Mark Michelson 
> Reported-by: Lance Richardson 
> ---
>  ovn/controller/chassis.c  | 1 +
>  ovn/northd/ovn-northd.c   | 2 +-
>  ovn/ovn-sb.ovsschema  | 7 ---
>  ovn/ovn-sb.xml| 3 +++
>  ovn/utilities/ovn-sbctl.c | 1 +
>  5 files changed, 10 insertions(+), 4 deletions(-)
>

This version doesn't seem to build for me:

./ovsdb/ovsdb-idlc.in: syntax
"{"columns":{"chassis-name":{"type":"string"},"ip":{"type":"string"},"options":{"type":{"key":"string","max":"unlimited","min":0,"value":"string"}},"type":{"type":{"key":{"enum":["set",["geneve","stt","vxlan"]],"type":"string"}":
syntax error: name must be a valid id
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] hola

[marria smiith]

how are you doing my dear friend?Wie geht es dir mein lieber freund
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovs-ovctl: Fix "OpenFlow versions" in ovs-ofctl -V

[Timothy Redaelli]

Fix the output of "ovs-ofctl -V" to show OpenFlow 1.4 as max supported
versions since OpenFlow 1.4 was enabled by default in commit
8d3485791188 ("OpenFlow: Enable OpenFlow 1.4 by default.")

CC: Ben Pfaff 
Signed-off-by: Timothy Redaelli 
---
 lib/ofp-version-opt.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ofp-version-opt.h b/lib/ofp-version-opt.h
index 5c8e7b866..74cad5b3f 100644
--- a/lib/ofp-version-opt.h
+++ b/lib/ofp-version-opt.h
@@ -11,7 +11,7 @@
 
 #define OFP_VERSION_OPTION_HANDLERS \
 case 'V':   \
-ovs_print_version(OFP10_VERSION, OFP13_VERSION);\
+ovs_print_version(OFP10_VERSION, OFP14_VERSION);\
 exit(EXIT_SUCCESS); \
 \
 case 'O':   \
-- 
2.13.3

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

Re: [ovs-dev] [PATCH v3 0/2] basic encap/decap

[Zoltán Balogh]

Hi,

As Yi wrote before, we would appreciate if somebody could review the series. I 
won't be available for the next two weeks and Jan is on vacation until 4th 
August. So, none of us will be able to update the patches next week.

Best regards,
Zoltan

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org 
> [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Yang, Yi Y
> Sent: Wednesday, July 26, 2017 12:52 AM
> To: Zoltán Balogh ; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v3 0/2] basic encap/decap
> 
> Hi, folks
> 
> It seems Ben is out of office in these days, can anybody help review or merge 
> this patch series? I heard 2.8 branch
> would be created end of July, but we still have NSH patch series waiting for 
> review except this series.
> 
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org 
> [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Zoltán Balogh
> Sent: Saturday, July 22, 2017 1:08 AM
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH v3 0/2] basic encap/decap
> 
> From: Zoltan Balogh 
> 
> This series is a continuation of other patch series initiated by Jan 
> Scheurich before. These were already applied to
> the master branch:
>  - userspace: Support for L3 tunneling
>https://mail.openvswitch.org/pipermail/ovs-dev/2017-June/87.html
>  - Packet type aware pipeline
>https://mail.openvswitch.org/pipermail/ovs-dev/2017-June/334512.html
> 
> The main purpose of this series is to add support for the OpenFlow actions 
> generic encap and decap (ONF EXT-382) to
> the OVS control plane. It implements a skeleton for translation of generic 
> encap and decap actions in ofproto-dpif
> and provides support to encap and decap an Ethernet header.
> 
> v2->v3
>  - NEWS updated.
>  - fix: drop VLAN tagged packet trying to decap it.
>  - New unit tests for the fix.
>  - Some tests were updated due to change in recirculation on master branch.
> 
> v1->v2
>  - Squash 1/4 and 2/4 commits of v1.
>  - Put unit tests in a separate commit.
>  - Use aligned cast.
>  - Nicira extension numbers for encap/decap action numbers and error codes.
>  - Small fixes according to comments.
> 
> Jan Scheurich (1):
>   OF support and translation of generic encap and decap
> 
> Zoltan Balogh (1):
>   tests: Extend PTAP unit tests with decap action
> 
>  NEWS   |   6 +
>  include/openflow/openflow-common.h |   1 +
>  include/openvswitch/automake.mk|   1 +
>  include/openvswitch/ofp-actions.h  |  31 +++  
> include/openvswitch/ofp-ed-props.h |  69 +++
>  include/openvswitch/ofp-errors.h   |   9 +
>  lib/automake.mk|   1 +
>  lib/odp-util.c |  84 +---
>  lib/odp-util.h |   3 +-
>  lib/ofp-actions.c  | 378 +-
>  lib/ofp-ed-props.c | 150 ++
>  lib/packets.h  |   3 +-
>  ofproto/ofproto-dpif-xlate.c   | 116 ++-
>  tests/packet-type-aware.at | 405 
> +
>  14 files changed, 1215 insertions(+), 42 deletions(-)  create mode 100644 
> include/openvswitch/ofp-ed-props.h
>  create mode 100644 lib/ofp-ed-props.c
> 
> --
> 1.9.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 1/1] ovn: l3ha, CLI for logical router port gateway chassis

[Russell Bryant]

On Tue, Jul 18, 2017 at 2:05 AM, Venkata Anil Kommaddi
 wrote:
> From: Venkata Anil 
>
> This change adds commands to set, get and delete gateway chassis
> for logical router port.
>
> Signed-off-by: Venkata Anil Kommaddi 
> ---
>  ovn/utilities/ovn-nbctl.8.xml |  21 +
>  ovn/utilities/ovn-nbctl.c | 178 
> ++
>  tests/ovn-nbctl.at|  52 
>  3 files changed, 251 insertions(+)

Thanks for the patch!  I applied this to master (with some changes, see below).


As a follow-up patch, I'd like to see gateway chassis listed in
"ovn-nbctl show" output.

At the end of the message, see the output from checkpatch.py.  I fixed
these, but wanted to show you for the future.

Finally, I made the following additional changes to the patch to fix a
couple of issues:


diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 0b4894911..53145daf3 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -2535,7 +2535,7 @@ lr_get_name(const struct nbrec_logical_router
*lr, char uuid_s[UUID_LEN + 1],
 return uuid_s;
 }

-static const struct nbrec_gateway_chassis*
+static const struct nbrec_gateway_chassis *
 gc_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist)
 {
 const struct nbrec_gateway_chassis *gc = NULL;
@@ -2565,7 +2565,7 @@ gc_by_name_or_uuid(struct ctl_context *ctx,
const char *id, bool must_exist)
 static void
 nbctl_lrp_set_gateway_chassis(struct ctl_context *ctx)
 {
-const char *gc_name;
+char *gc_name;
 int64_t priority = 0;
 const char *lrp_name = ctx->argv[1];
 const struct nbrec_logical_router_port *lrp;
@@ -2581,15 +2581,17 @@ nbctl_lrp_set_gateway_chassis(struct ctl_context *ctx)
 }

 gc_name = xasprintf("%s-%s", lrp_name, chassis_name);
-const struct nbrec_gateway_chassis *gc;
-gc = gc_by_name_or_uuid(ctx, gc_name, false);
-if (gc) {
-nbrec_gateway_chassis_set_priority(gc, priority);
+const struct nbrec_gateway_chassis *existing_gc;
+existing_gc = gc_by_name_or_uuid(ctx, gc_name, false);
+if (existing_gc) {
+nbrec_gateway_chassis_set_priority(existing_gc, priority);
+free(gc_name);
 return;
 }

 /* Create the logical gateway chassis. */
-gc = nbrec_gateway_chassis_insert(ctx->txn);
+struct nbrec_gateway_chassis *gc
+= nbrec_gateway_chassis_insert(ctx->txn);
 nbrec_gateway_chassis_set_name(gc, gc_name);
 nbrec_gateway_chassis_set_chassis_name(gc, chassis_name);
 nbrec_gateway_chassis_set_priority(gc, priority);
@@ -2600,11 +2602,11 @@ nbctl_lrp_set_gateway_chassis(struct ctl_context *ctx)
 sizeof *new_gc * (lrp->n_gateway_chassis + 1));
 nullable_memcpy(new_gc, lrp->gateway_chassis,
 sizeof *new_gc * lrp->n_gateway_chassis);
-new_gc[lrp->n_gateway_chassis] = CONST_CAST(
-struct nbrec_gateway_chassis *, gc);
+new_gc[lrp->n_gateway_chassis] = gc;
 nbrec_logical_router_port_set_gateway_chassis(
 lrp, new_gc, lrp->n_gateway_chassis + 1);
 free(new_gc);
+free(gc_name);
 }

 /* Removes logical router port 'lrp->gateway_chassis[idx]'. */
@@ -2612,17 +2614,22 @@ static void
 remove_gc(const struct nbrec_logical_router_port *lrp, size_t idx)
 {
 const struct nbrec_gateway_chassis *gc = lrp->gateway_chassis[idx];
-/* First remove 'gc' from the array of gateway_chassis.  This is what will
- * actually cause the gateway chassis to be deleted when the transaction is
- * sent to the database server (due to garbage collection). */
-struct nbrec_gateway_chassis **new_gc
-= xmemdup(lrp->gateway_chassis,
-  sizeof *new_gc * lrp->n_gateway_chassis);
-new_gc[idx] = new_gc[lrp->n_gateway_chassis - 1];
-nbrec_logical_router_port_verify_gateway_chassis(lrp);
-nbrec_logical_router_port_set_gateway_chassis(
-lrp, new_gc, lrp->n_gateway_chassis - 1);
-free(new_gc);
+
+if (lrp->n_gateway_chassis == 1) {
+nbrec_logical_router_port_set_gateway_chassis(lrp, NULL, 0);
+} else {
+/* First remove 'gc' from the array of gateway_chassis.  This
is what will
+ * actually cause the gateway chassis to be deleted when the
transaction is
+ * sent to the database server (due to garbage collection). */
+struct nbrec_gateway_chassis **new_gc
+= xmemdup(lrp->gateway_chassis,
+  sizeof *new_gc * lrp->n_gateway_chassis);
+new_gc[idx] = new_gc[lrp->n_gateway_chassis - 1];
+nbrec_logical_router_port_verify_gateway_chassis(lrp);
+nbrec_logical_router_port_set_gateway_chassis(
+lrp, new_gc, lrp->n_gateway_chassis - 1);
+free(new_gc);
+}

 /* Delete 'gc' from the IDL.  This won't have a real effect on
  * the database server (the IDL will suppress it in fact) but it
diff --git 

Re: [ovs-dev] [PATCH 0/3] Small refactoring related to tc

[Roi Dayan]

On 27/07/2017 13:19, Roi Dayan wrote:

Hi,

The following patches are small refactoring related to tc.

Thanks,
Roi


Paul Blakey (3):
  tc: Refactor nl_msg_put_flower_options
  tc: Split IPs and transport layer ports unions in flower struct
  netdev-tc-offloads: Parse ip related fields only if eth type is ip

 lib/netdev-tc-offloads.c | 47 --
 lib/tc.c | 85 +---
 lib/tc.h | 25 +++---
 3 files changed, 65 insertions(+), 92 deletions(-)




sorry but we didn't notice that we have some merge issues with this
after the sctp addition. we'll fix and send again.

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

Re: [ovs-dev] [PATCH] dpif-netlink: Fix log level for error message

[Eric Garver]

On Thu, Jul 27, 2017 at 10:27:18AM +0300, Roi Dayan wrote:
> Since it's an error message log it with VLOG_ERR instead of VLOG_INFO.
> 
> Signed-off-by: Roi Dayan 
> ---
>  lib/dpif-netlink.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 55effd1..7b53ed5 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -972,8 +972,8 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink 
> *dpif,
>  error = dpif_netlink_rtnl_port_create(netdev);
>  if (error) {
>  if (error != EOPNOTSUPP) {
> -VLOG_INFO_RL(, "Failed to create %s with rtnetlink: %s",
> - netdev_get_name(netdev), ovs_strerror(error));
> +VLOG_ERR_RL(, "Failed to create %s with rtnetlink: %s",
> +netdev_get_name(netdev), ovs_strerror(error));
>  }
>  return error;
>  }
> -- 
> 2.7.4

I don't think this is appropriate. If creating with rtnetlink fails
we'll fall back to using the compat interface. As such, we may still
successfully create the tunnel. Maybe WARN would be more appropriate?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] dpif-netlink-rtnl: Fix false errors on interfaces without tunnel config

[Roi Dayan]

When we skip adding a port using rtnetlink and not because of an error we
need to return EOPNOTSUPP to avoid logging an error message.

Fixes: 2fd3d5eda508 ("dpif-netlink-rtnl: Support layer3 GRE")
Signed-off-by: Roi Dayan 
Reviewed-by: Paul Blakey 
---
 lib/dpif-netlink-rtnl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
index 98a2f2b..3efa1f6 100644
--- a/lib/dpif-netlink-rtnl.c
+++ b/lib/dpif-netlink-rtnl.c
@@ -349,7 +349,7 @@ dpif_netlink_rtnl_port_create(struct netdev *netdev)
 type = netdev_to_ovs_vport_type(netdev_get_type(netdev));
 tnl_cfg = netdev_get_tunnel_config(netdev);
 if (!tnl_cfg) {
-return EINVAL;
+return EOPNOTSUPP;
 }
 
 kind = vport_type_to_kind(type, tnl_cfg);
-- 
2.7.4

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

Re: [ovs-dev] [PATCH v2] ofproto-dpif-ipfix: add support for per-flow drop counters

[Szczerbik, PrzemyslawX]

Hi Ben,

I did some additional investigation and I finally figured out why I was unable 
to see 'sample' action.
The reason was that I was using 100% sampling probability for my tests. In such 
a case 'sample' action is not created.
Decreasing sampling probability solved the issue, but only partially.

Per your suggestion, my implementation considered a nested 'output' action 
inside 'sample' action as 'true output' only if sampling probability was set to 
100%.
It means that 'output' action in 'sample' action was never consider a 'true 
output', since 'sample' action is not created when probability was set to 100%.

However, there is one additional scenario when 'sample' action is created even 
though probability is set to 100% - when a slow path meter is configured.
I modified my configuration to include a slow path meter and successfully 
triggered a 'sample' action with probability lower than 100%.
Therefore, it looks like you were right and this action should be handled. I'll 
submit v3 soon.

Regards,
Przemek

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Szczerbik, PrzemyslawX
> Sent: Friday, July 14, 2017 1:05 PM
> To: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v2] ofproto-dpif-ipfix: add support for per-flow
> drop counters
> 
> Hi Ben,
> 
> I added code to handle 'clone' action as you suggested.
> I'm still not convinced that it's possible to see 'sample' action during 
> IPFIX upcall,
> so I didn't include it in this patch.
> In every test that I run a flow rule with sample action was translated to
> 'userspace' action in datapath.
> 
> If you are certain that 'sample' action has to be handled I'll add it in v3.
> However, most likely I'd require some assistance on how to trigger this 
> action.
> 
> Let me know what you think.
> 
> Regards,
> Przemek
> 
> > -Original Message-
> > From: Szczerbik, PrzemyslawX
> > Sent: Friday, July 14, 2017 12:42 PM
> > To: d...@openvswitch.org
> > Cc: b...@ovn.org; Szczerbik, PrzemyslawX 
> > Subject: [PATCH v2] ofproto-dpif-ipfix: add support for per-flow drop 
> > counters
> >
> > Patch based on RFC 5102, section 5.10. It implements per-flow drop counters:
> > - droppedPacketDeltaCount
> > - droppedPacketTotalCount
> > - droppedOctetDeltaCount
> > - droppedOctetTotalCount
> >
> > In order to determine if packet is going to be dropped, flow actions 
> > associated
> > with packet are read. If there isn't at least one OVS_ACTION_ATTR_OUTPUT
> > action
> > we assume that packet is going to be dropped. Packet can have direct
> > OVS_ACTION_ATTR_OUTPUT action or one that is nested in
> > OVS_ACTION_ATTR_CLONE
> > action.
> >
> > Signed-off-by: Przemyslaw Szczerbik 
> > ---
> > v1->v2
> > * Prevent segfault when dereferencing ipfix_actions in 
> > ipfix_cache_entry_init
> > * Handle OVS_ACTION_ATTR_CLONE action
> >
> >  ofproto/ofproto-dpif-ipfix.c  | 125
> > +++---
> >  ofproto/ofproto-dpif-ipfix.h  |  16 +-
> >  ofproto/ofproto-dpif-upcall.c |  95 
> >  3 files changed, 202 insertions(+), 34 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index 13ff426..9887da1 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -93,6 +93,8 @@ enum dpif_ipfix_tunnel_type {
> >  typedef struct ofputil_ipfix_stats ofproto_ipfix_stats;
> >
> >  struct dpif_ipfix_global_stats {
> > +uint64_t dropped_packet_total_count;
> > +uint64_t dropped_octet_total_count;
> >  uint64_t packet_total_count;
> >  uint64_t octet_total_count;
> >  uint64_t octet_total_sum_of_squares;
> > @@ -409,6 +411,8 @@ OVS_PACKED(
> >  struct ipfix_data_record_aggregated_common {
> >  ovs_be32 flow_start_delta_microseconds; /*
> > FLOW_START_DELTA_MICROSECONDS */
> >  ovs_be32 flow_end_delta_microseconds; /*
> > FLOW_END_DELTA_MICROSECONDS */
> > +ovs_be64 dropped_packet_delta_count;  /*
> > DROPPED_PACKET_DELTA_COUNT */
> > +ovs_be64 dropped_packet_total_count;  /*
> > DROPPED_PACKET_TOTAL_COUNT */
> >  ovs_be64 packet_delta_count;  /* PACKET_DELTA_COUNT */
> >  ovs_be64 packet_total_count;  /* PACKET_TOTAL_COUNT */
> >  /* INGRESS_UNICAST_PACKET_TOTAL_COUNT */
> > @@ -427,11 +431,13 @@ struct ipfix_data_record_aggregated_common {
> >  ovs_be64 layer2_octet_total_count;  /* LAYER2_OCTET_TOTAL_COUNT */
> >  uint8_t flow_end_reason;  /* FLOW_END_REASON */
> >  });
> > -BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_common)
> ==
> > 97);
> > +BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_common)
> ==
> > 113);
> >
> >  /* Part of data record for IP aggregated elements. */
> >  OVS_PACKED(
> >  struct ipfix_data_record_aggregated_ip {
> > +ovs_be64 dropped_octet_delta_count;  /*
> DROPPED_OCTET_DELTA_COUNT
> 

[ovs-dev] [PATCH] docs: Move restart related note to appropriate place.

[Ilya Maximets]

Currently, 'restart' related note is at the end of 'Setup OVS'
section. This makes the reader think that changing the
'pmd-cpu-mask' requires application restart.

It was mistakenly moved while converting to rST.

Fix that by moving the note closer to options it relates to.

CC: Stephen Finucane 
Fixes: 167703d664fc ("doc: Convert INSTALL.DPDK to rST")
Signed-off-by: Ilya Maximets 
---
 Documentation/intro/install/dpdk.rst | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/intro/install/dpdk.rst 
b/Documentation/intro/install/dpdk.rst
index a05aa1a..d2e5bfc 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -225,6 +225,10 @@ listed below. Defaults will be provided for all values not 
explicitly set.
 ``vhost-sock-dir``
   Option to set the path to the vhost-user unix socket files.
 
+.. note::
+  Changing any of these options requires restarting the ovs-vswitchd
+  application
+
 If allocating more than one GB hugepage, you can configure the
 amount of memory used from any given NUMA nodes. For example, to use 1GB from
 NUMA node 0 and 0GB for all other NUMA nodes, run::
@@ -247,10 +251,6 @@ threads and pin them to cores 1,2, run::
 Refer to ovs-vswitchd.conf.db(5) for additional information on configuration
 options.
 
-.. note::
-  Changing any of these options requires restarting the ovs-vswitchd
-  application
-
 Validating
 --
 
-- 
2.7.4

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

Re: [ovs-dev] Openvswitch crash when bringing down the dpdk bond port using "ovs-ofctl mod-port br-prv dpdk1 down"

[Keshav Gupta]

I think this patch will have below  issue though I don't know how much impact 
it will cause.

packet which are not processed and present in the dpdk port’s rxq  at the time 
of bringing down the port (using mod-port)  they will remain there. And next 
time when
Somebody  bring up the port(using mode-port) that time those old packet will be 
received first. 

Thanks
keshav


-Original Message-
From: Keshav Gupta 
Sent: Thursday, July 27, 2017 3:15 PM
To: 'Ilya Maximets'; ovs-dev@openvswitch.org
Subject: RE: Re: [ovs-dev] Openvswitch crash when bringing down the dpdk bond 
port using "ovs-ofctl mod-port br-prv dpdk1 down"

Thanks Ilya Maximets

It fixes the issue.

Thanks  
Keshav

-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com]
Sent: Wednesday, July 26, 2017 5:58 PM
To: ovs-dev@openvswitch.org; Keshav Gupta
Subject: Re: Re: [ovs-dev] Openvswitch crash when bringing down the dpdk bond 
port using "ovs-ofctl mod-port br-prv dpdk1 down"

Hi.

You need to backport at least following patch:

commit 3b1fb0779b87788968c1a6a9ff295a9883547485
Author: Daniele Di Proietto 
Date:   Tue Nov 15 15:40:49 2016 -0800

netdev-dpdk: Don't call rte_dev_stop() in update_flags().

Calling rte_eth_dev_stop() while the device is running causes a crash.

We could use rte_eth_dev_set_link_down(), but not every PMD implements
that, and I found one NIC where that has no effect.

Instead, this commit checks if the device has the NETDEV_UP flag when
transmitting or receiving (similarly to what we do for vhostuser). I
didn't notice any performance difference with this check in case the
device is up.

An alternative would be to remove the device queues from the pmd threads
tx and receive cache, but that requires reconfiguration and I'd prefer
to avoid it, because the change can come from OpenFlow.

Signed-off-by: Daniele Di Proietto 
Acked-by: Ilya Maximets 

This should fix your issue.
In general, I'm suggesting to use stable 2.7 OVS, there was too many DPDK 
related changes including stability fixes since 2.6.

Best regards, Ilya Maximets.

> Hi
>   We are experiencing a openvswitch crash when bringing down the dpdk bond 
> port using "ovs-ofctl mod-port br-prv dpdk1 down".
> 
> backtrace of core is like below. Is there any issue reported earlier  for 
> this type of crash in openvswitch community.
> 
> (gdb) bt
> #0  ixgbe_rxq_rearm (rxq=0x7fa45061f800) at 
> /home/sdn/new_cloud_sdn_switch_2/cloud-sdn-switch/dpdk/drivers/net/ixg
> be/ixgbe_rxtx_vec_sse.c:98
> #1  _recv_raw_pkts_vec (split_packet=0x0, nb_pkts=32, rx_pkts= out>, rxq=0x7fa45061f800)
> at
> /home/sdn/new_cloud_sdn_switch_2/cloud-sdn-switch/dpdk/drivers/net/ixg
> be/ixgbe_rxtx_vec_sse.c:290
> #2  ixgbe_recv_pkts_vec (rx_queue=0x7fa45061f800, rx_pkts=, 
> nb_pkts=)
> at
> /home/sdn/new_cloud_sdn_switch_2/cloud-sdn-switch/dpdk/drivers/net/ixg
> be/ixgbe_rxtx_vec_sse.c:474
> #3  0x00e500e4 in ?? ()
> #4  0x004600e6 in ?? ()
> #5  0x006a0069 in ?? ()
> #6  0x006c006b in ?? ()
> #7  0x00ec006d in ?? ()
> #8  0x00ee00ed in ?? ()
> #9  0x0001537f5780 in ?? ()
> #10 0x in ?? ()
> (gdb)
> 
> 
> I have analyzed the core and it seems it is a result of device stop 
> and packet receive from the port happening at same time by two thread 
> OVS main thread(device stop) and PMD thread(pkt receive). More precisely main 
> thread cleaning the packet buffer from rxq sw_ring to avoid the packet buffer 
> leak while in parallel PMD thread is filling the packet buffer in 
> sw_ring/descriptor ring as part of ixgbe_recv_pkts_vec.
> 
> version used is: openvswitch (2.6.1) with dpdk (16.11).
> 
> This crash is not every time reproducible but frequency seems to be high.
> 
> I am new to openvswitch community and this is first time I am posting a 
> query. let me know if anything you require from my side.
> 
> Thanks
> Keshav

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

Re: [ovs-dev] OVS-DPDK public meeting

[Kevin Traynor]

26th July 2017

ATTENDEES: Aaron C, Antonio F, Ciara L, Darrell B, Flavio L, Mark M,
Shachar B, Olga A, Sugesh C, Thomas M, Mark K, Michael L, Johan T, Peter
S, Ian S, Kevin T, Yipeng W, Bob D, Jason van A, Billy O'M (may have
missed some)

===
GENERAL
===
- OVS 2.8
-- Probably branch next week
-- Discussion about some OVN patches brewing for OVS 2.8

- DPDK 17.05.1 integration into OVS
-- Looks ready to merge

- DPDK 17.08
-- Should be released after first week in August
-- Some new features like GRO
-- Some pmds have been updated
-- more info http://dpdk.org/doc/guides/rel_notes/release_17_08.html

- Batching patches and merging via pull requests from an OVS-DPDK branch
-- Short term proposal from Darrell to increase merge times
-- Discussion about cadence, patchwork, merge conflicts, testing and
division
-- Darrell will send an update to the ML


PERFORMANCE/FEATURES

- Clean up on delete ports - bugfix
-- Darrell submitted a patch reintroducing an API as safe fix for this
prior to release

- Intermediate Queuing/Tx batching
-- Needs more discussion on the ML. Not clear the preferred implementation
-- Didn't discuss further as authors couldn't attend

- Cuckoo Distributor Patch
- EMC lookup insertions on recirc packets
-- Briefly discussed how these could potentially increase performance


HARDWARE OFFLOAD

- Datapath classifier offloading
-- Shachar B gave us an overview of the recent patches
-- May be able to share some docs/diagrams to illustrate this further
-- Some further discussion to be had with Michael/Finn wrt their
implementation

On 07/25/2017 02:25 PM, Kevin Traynor wrote:
> Hi All,
> 
> The OVS-DPDK public meetings have moved to Wednesday's at the same time.
> Everyone is welcome, it's a chance to share status/plans etc.
> 
> It's scheduled for every 2 weeks starting 26th July. Meeting time is
> currently @ 4pm UTC.
> 
> You can connect through Bluejeans or through dial in:
> 
> https://bluejeans.com/139318596
> 
> US: +1.408.740.7256 
> UK: +44.203.608.5256 
> Germany: +49.32.221.091256 
> Ireland: +353.1.697.1256 
> Other numbers at https://www.bluejeans.com/numbers
> Meeting ID: 139318596
> 
> thanks,
> Kevin.
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 

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

Re: [ovs-dev] [PATCH] tc: Correct convert ticks to msecs on parsing tc TM

[Or Gerlitz]

On 7/27/2017 1:14 PM, Roi Dayan wrote:
the system call is done only once. 


good to know, would be worth to mention that on the change-log, so it's 
clear we're good w.r.t performance.



Or.

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

[ovs-dev] [PATCH 2/3] tc: Split IPs and transport layer ports unions in flower struct

[Roi Dayan]

From: Paul Blakey 

Split dst/src_port and ipv4/ipv6 union so we can
distingush them easily for later features.

Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
---
 lib/netdev-tc-offloads.c | 29 +
 lib/tc.c | 24 
 lib/tc.h | 25 +
 3 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index 6214023..e2aea60 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -319,8 +319,15 @@ parse_tc_flower_to_match(struct tc_flower *flower,
 match_set_ipv6_dst_masked(match,
   >ipv6.ipv6_dst, >ipv6.ipv6_dst);
 
-match_set_tp_dst_masked(match, key->dst_port, mask->dst_port);
-match_set_tp_src_masked(match, key->src_port, mask->src_port);
+if (is_ip_any(>flow)) {
+if (key->ip_proto == IPPROTO_TCP) {
+match_set_tp_dst_masked(match, key->tcp_dst, mask->tcp_dst);
+match_set_tp_src_masked(match, key->tcp_src, mask->tcp_src);
+} else if (key->ip_proto == IPPROTO_UDP) {
+match_set_tp_dst_masked(match, key->udp_dst, mask->udp_dst);
+match_set_tp_src_masked(match, key->udp_src, mask->udp_src);
+}
+}
 
 if (flower->tunnel.tunnel) {
 match_set_tun_id(match, flower->tunnel.id);
@@ -747,12 +754,18 @@ netdev_tc_flow_put(struct netdev *netdev, struct match 
*match,
 if (is_ip_any(key)) {
 flower.key.ip_proto = key->nw_proto;
 flower.mask.ip_proto = mask->nw_proto;
-
-if (key->nw_proto == IPPROTO_TCP || key->nw_proto == IPPROTO_UDP) {
-flower.key.dst_port = key->tp_dst;
-flower.mask.dst_port = mask->tp_dst;
-flower.key.src_port = key->tp_src;
-flower.mask.src_port = mask->tp_src;
+if (key->nw_proto == IPPROTO_TCP) {
+flower.key.tcp_dst = key->tp_dst;
+flower.mask.tcp_dst = mask->tp_dst;
+flower.key.tcp_src = key->tp_src;
+flower.mask.tcp_src = mask->tp_src;
+mask->tp_src = 0;
+mask->tp_dst = 0;
+} else if (key->nw_proto == IPPROTO_UDP) {
+flower.key.udp_dst = key->tp_dst;
+flower.mask.udp_dst = mask->tp_dst;
+flower.key.udp_src = key->tp_src;
+flower.mask.udp_src = mask->tp_src;
 mask->tp_src = 0;
 mask->tp_dst = 0;
 }
diff --git a/lib/tc.c b/lib/tc.c
index 563aba4..d724d8a 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -305,26 +305,26 @@ nl_parse_flower_ip(struct nlattr **attrs, struct 
tc_flower *flower) {
 
 if (ip_proto == IPPROTO_TCP) {
 if (attrs[TCA_FLOWER_KEY_TCP_SRC_MASK]) {
-key->src_port =
+key->tcp_src =
 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_SRC]);
-mask->src_port =
+mask->tcp_src =
 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_SRC_MASK]);
 }
 if (attrs[TCA_FLOWER_KEY_TCP_DST_MASK]) {
-key->dst_port =
+key->tcp_dst =
 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_DST]);
-mask->dst_port =
+mask->tcp_dst =
 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_DST_MASK]);
 }
 } else if (ip_proto == IPPROTO_UDP) {
 if (attrs[TCA_FLOWER_KEY_UDP_SRC_MASK]) {
-key->src_port = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC]);
-mask->src_port =
+key->udp_src = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC]);
+mask->udp_src =
 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC_MASK]);
 }
 if (attrs[TCA_FLOWER_KEY_UDP_DST_MASK]) {
-key->dst_port = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST]);
-mask->dst_port =
+key->udp_dst = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST]);
+mask->udp_dst =
 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST_MASK]);
 }
 }
@@ -994,11 +994,11 @@ nl_msg_put_flower_options(struct ofpbuf *request, struct 
tc_flower *flower)
 }
 
 if (flower->key.ip_proto == IPPROTO_UDP) {
-FLOWER_PUT_MASKED_VALUE(src_port, TCA_FLOWER_KEY_UDP_SRC);
-FLOWER_PUT_MASKED_VALUE(dst_port, TCA_FLOWER_KEY_UDP_DST);
+FLOWER_PUT_MASKED_VALUE(udp_src, TCA_FLOWER_KEY_UDP_SRC);
+FLOWER_PUT_MASKED_VALUE(udp_dst, TCA_FLOWER_KEY_UDP_DST);
 } else if (flower->key.ip_proto == IPPROTO_TCP) {
-FLOWER_PUT_MASKED_VALUE(src_port, TCA_FLOWER_KEY_TCP_SRC);
-FLOWER_PUT_MASKED_VALUE(dst_port, TCA_FLOWER_KEY_TCP_DST);
+FLOWER_PUT_MASKED_VALUE(tcp_src, TCA_FLOWER_KEY_TCP_SRC);
+FLOWER_PUT_MASKED_VALUE(tcp_dst, TCA_FLOWER_KEY_TCP_DST);
 }
 }
 
diff --git a/lib/tc.h b/lib/tc.h

[ovs-dev] [PATCH 0/3] Small refactoring related to tc

[Roi Dayan]

Hi,

The following patches are small refactoring related to tc.

Thanks,
Roi


Paul Blakey (3):
  tc: Refactor nl_msg_put_flower_options
  tc: Split IPs and transport layer ports unions in flower struct
  netdev-tc-offloads: Parse ip related fields only if eth type is ip

 lib/netdev-tc-offloads.c | 47 --
 lib/tc.c | 85 +---
 lib/tc.h | 25 +++---
 3 files changed, 65 insertions(+), 92 deletions(-)

-- 
2.7.4

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

[ovs-dev] [PATCH 3/3] netdev-tc-offloads: Parse ip related fields only if eth type is ip

[Roi Dayan]

From: Paul Blakey 

No need to parse ip related fields otherwise.

Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
---
 lib/netdev-tc-offloads.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index e2aea60..c5f305d 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -307,19 +307,19 @@ parse_tc_flower_to_match(struct tc_flower *flower,
 match_set_dl_type(match, key->eth_type);
 }
 
-if (key->ip_proto && is_ip_any(>flow)) {
-match_set_nw_proto(match, key->ip_proto);
-}
+if (is_ip_any(>flow)) {
+if (key->ip_proto) {
+match_set_nw_proto(match, key->ip_proto);
+}
 
-match_set_nw_src_masked(match, key->ipv4.ipv4_src, mask->ipv4.ipv4_src);
-match_set_nw_dst_masked(match, key->ipv4.ipv4_dst, mask->ipv4.ipv4_dst);
+match_set_nw_src_masked(match, key->ipv4.ipv4_src, 
mask->ipv4.ipv4_src);
+match_set_nw_dst_masked(match, key->ipv4.ipv4_dst, 
mask->ipv4.ipv4_dst);
 
-match_set_ipv6_src_masked(match,
-  >ipv6.ipv6_src, >ipv6.ipv6_src);
-match_set_ipv6_dst_masked(match,
-  >ipv6.ipv6_dst, >ipv6.ipv6_dst);
+match_set_ipv6_src_masked(match,
+  >ipv6.ipv6_src, >ipv6.ipv6_src);
+match_set_ipv6_dst_masked(match,
+  >ipv6.ipv6_dst, >ipv6.ipv6_dst);
 
-if (is_ip_any(>flow)) {
 if (key->ip_proto == IPPROTO_TCP) {
 match_set_tp_dst_masked(match, key->tcp_dst, mask->tcp_dst);
 match_set_tp_src_masked(match, key->tcp_src, mask->tcp_src);
-- 
2.7.4

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

Re: [ovs-dev] [PATCH] tc: Correct convert ticks to msecs on parsing tc TM

[Roi Dayan]

On 27/07/2017 11:37, Or Gerlitz wrote:

On 7/27/2017 11:32 AM, Simon Horman wrote:

On Thu, Jul 27, 2017 at 11:29:10AM +0300, Or Gerlitz wrote:

On 7/27/2017 11:00 AM, Simon Horman wrote:

Use sysconf(_SC_CLK_TCK) to read run time "number of clock ticks per
second" and use that to convert ticks to msecs.
This is how iproute does the conversion when parsing tc filters.

Signed-off-by: Paul Blakey
Reviewed-by: Roi Dayan

This looks good to me

Is this/similar conversion is needed also for the non TC data-path?
if yes,
how they do it there? if not, can you elaborate who does it (the ovs
kernel
dp?)

Hi Or,

the Linux kernel datapath provides an OVS_FLOW_ATTR_USED attribute
whose value is in ms - converted from jiffies in the kernel using
ovs_flow_used_time(). So I don't think the conversion needs
to be done in user-space for that datapath.


I see, so if this patch does it for each dump of each flow, we might
added (say) 100K system calls per second? doesn't sound cool to me... I
guess we either need to make sure the system call is done very rarely or
we can maybe add some flag to tc to get the dump in the units we need.
Hopefully the 1st option (doing the system call rarely) is viable and we
can go that path and avoid kernel UAPI changes and patching.

Or.



the system call is done only once.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Revisit OVN meeting schedule?

[Miguel Angel Ajo Pelayo]

Thank you for bringing this up.

#1 works great for me, but #2 also works.

On Wed, Jul 26, 2017 at 6:14 PM, Daniel Alvarez Sanchez  wrote:

> On Wed, Jul 26, 2017 at 5:13 PM, Ben Pfaff  wrote:
>
> > On July 26, 2017 8:07:57 AM PDT, Russell Bryant  wrote:
> > >It has been difficult for some of our newer contributors in Europe to
> > >make our weekly OVN IRC meeting, so I wanted to revisit the schedule.
> > >Roughly we have 2 options to consider:
> > >
> > >1) Change the time to accommodate more contributors.  My suggestion to
> > >consider would be 8am Pacific / 11am Eastern (meeting 2 hours earlier
> > >than currently).  See the following page for an overview of time zone
> > >overlap of some of our contributors.
> > >
> > >https://www.timeanddate.com/worldclock/meetingtime.html?
> > iso=20170726=1082=54=141=405=224
> > >
> > >2) Consider a rotating schedule, where we have 2 different meeting
> > >times that rotate each week.  I can work out a proposal here, but
> > >let's see what people think of #1 first.
> > >
> > >Please share your thoughts.
> > >
> > >Thanks,
> > >
> > >--
> > >Russell Bryant
> > >___
> > >dev mailing list
> > >d...@openvswitch.org
> > >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > I'd personally favor #1 but #2 also works.
> >
> Same here.
>
>
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tc: Correct convert ticks to msecs on parsing tc TM

[Or Gerlitz]

On 7/27/2017 11:32 AM, Simon Horman wrote:

On Thu, Jul 27, 2017 at 11:29:10AM +0300, Or Gerlitz wrote:

On 7/27/2017 11:00 AM, Simon Horman wrote:

Use sysconf(_SC_CLK_TCK) to read run time "number of clock ticks per
second" and use that to convert ticks to msecs.
This is how iproute does the conversion when parsing tc filters.

Signed-off-by: Paul Blakey
Reviewed-by: Roi Dayan

This looks good to me

Is this/similar conversion is needed also for the non TC data-path? if yes,
how they do it there? if not, can you elaborate who does it (the ovs kernel
dp?)

Hi Or,

the Linux kernel datapath provides an OVS_FLOW_ATTR_USED attribute
whose value is in ms - converted from jiffies in the kernel using
ovs_flow_used_time(). So I don't think the conversion needs
to be done in user-space for that datapath.


I see, so if this patch does it for each dump of each flow, we might 
added (say) 100K system calls per second? doesn't sound cool to me... I 
guess we either need to make sure the system call is done very rarely or 
we can maybe add some flag to tc to get the dump in the units we need.  
Hopefully the 1st option (doing the system call rarely) is viable and we 
can go that path and avoid kernel UAPI changes and patching.


Or.

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

Re: [ovs-dev] [PATCH] tc: Correct convert ticks to msecs on parsing tc TM

[Simon Horman]

On Thu, Jul 27, 2017 at 11:29:10AM +0300, Or Gerlitz wrote:
> On 7/27/2017 11:00 AM, Simon Horman wrote:
> >>Use sysconf(_SC_CLK_TCK) to read run time "number of clock ticks per
> >>second" and use that to convert ticks to msecs.
> >>This is how iproute does the conversion when parsing tc filters.
> >>
> >>Signed-off-by: Paul Blakey
> >>Reviewed-by: Roi Dayan
> >This looks good to me
> 
> Is this/similar conversion is needed also for the non TC data-path? if yes,
> how they do it there? if not, can you elaborate who does it (the ovs kernel
> dp?)

Hi Or,

the Linux kernel datapath provides an OVS_FLOW_ATTR_USED attribute
whose value is in ms - converted from jiffies in the kernel using
ovs_flow_used_time(). So I don't think the conversion needs
to be done in user-space for that datapath.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tc: Correct convert ticks to msecs on parsing tc TM

[Or Gerlitz]

On 7/27/2017 11:00 AM, Simon Horman wrote:

Use sysconf(_SC_CLK_TCK) to read run time "number of clock ticks per
second" and use that to convert ticks to msecs.
This is how iproute does the conversion when parsing tc filters.

Signed-off-by: Paul Blakey
Reviewed-by: Roi Dayan

This looks good to me


Is this/similar conversion is needed also for the non TC data-path? if 
yes, how they do it there? if not, can you elaborate who does it (the 
ovs kernel dp?)


Or.

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

Re: [ovs-dev] [PATCH] dpif: Refactor obj type from void pointer to dpif_class

[Simon Horman]

On Wed, Jul 26, 2017 at 02:35:10PM -0700, Joe Stringer wrote:
> On 24 July 2017 at 22:28, Roi Dayan  wrote:
> > It's basically what is being passed today and passing a specific
> > type adds a compiler type check.
> >
> > Signed-off-by: Roi Dayan 
> > Reviewed-by: Paul Blakey 
> 
> Acked-by: Joe Stringer 

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

Re: [ovs-dev] [PATCH V2] tc: Add SCTP support

[Simon Horman]

On Wed, Jul 26, 2017 at 02:35:51PM -0700, Joe Stringer wrote:
> On 25 July 2017 at 04:39, Roi Dayan  wrote:
> > From: Vlad Buslov 
> >
> > Implement SCTP source and destination ports support for flower.
> >
> > Signed-off-by: Vlad Buslov 
> > Reviewed-by: Paul Blakey 
> > Acked-by: Roi Dayan 
> > ---
> 
> Acked-by: Joe Stringer 

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

Re: [ovs-dev] [PATCH] tc: Correct convert ticks to msecs on parsing tc TM

[Simon Horman]

On Thu, Jul 27, 2017 at 09:14:00AM +0300, Roi Dayan wrote:
> From: Paul Blakey 
> 
> Use sysconf(_SC_CLK_TCK) to read run time "number of clock ticks per
> second" and use that to convert ticks to msecs.
> This is how iproute does the conversion when parsing tc filters.
> 
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 

This looks good to me. I'm happy to apply this if someone could provide
an Acked-by tag. Or alternatively for someone to apply it with:

Acked-by: Simon Horman 

> ---
>  lib/tc.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/tc.c b/lib/tc.c
> index 401690e..b3fe52f 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "byte-order.h"
>  #include "netlink-socket.h"
> @@ -399,12 +400,23 @@ static const struct nl_policy gact_policy[] = {
>.optional = false, },
>  };
>  
> -#define JIFFIES_TO_MS(x) (x * 10)
> +static int
> +get_user_hz(void) {
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +static int user_hz = 100;
> +
> +if (ovsthread_once_start()) {
> +user_hz = sysconf(_SC_CLK_TCK);
> +ovsthread_once_done();
> +}
> +
> +return user_hz;
> +}
>  
>  static void
>  nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
>  {
> -flower->lastused = time_msec() - JIFFIES_TO_MS(tm->lastuse);
> +flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
>  }
>  
>  static int
> -- 
> 2.7.4
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 0/4] Output packet batching.

[Bodireddy, Bhanuprakash]

HI Ilya,

I am OOO and would review and test this patch series shortly(by Monday). 

Bhanuprakash. 

>-Original Message-
>From: Ilya Maximets [mailto:i.maxim...@samsung.com]
>Sent: Wednesday, July 26, 2017 4:21 PM
>To: ovs-dev@openvswitch.org; Bodireddy, Bhanuprakash
>
>Cc: Heetae Ahn ; Ben Pfaff ;
>Fischetti, Antonio ; Eelco Chaudron
>; Loftus, Ciara ; Kevin
>Traynor ; Darrell Ball ; Ilya
>Maximets 
>Subject: [PATCH v2 0/4] Output packet batching.
>
>This patch-set inspired by [1] from Bhanuprakash Bodireddy.
>Implementation of [1] looks very complex and introduces many pitfalls for
>later code modifications like possible packet stucks.
>
>This version targeted to make simple and flexible output packet batching on
>higher level without introducing and even simplifying netdev layer.
>
>Patch set consists of 3 patches. All the functionality introduced in the first
>patch. Two others are just cleanups of netdevs to not do unnecessary things.
>
>Basic testing of 'PVP with OVS bonding on phy ports' scenario shows
>significant performance improvement.
>More accurate and intensive testing required.
>
>[1] [PATCH 0/6] netdev-dpdk: Use intermediate queue during packet
>transmission.
>https://mail.openvswitch.org/pipermail/ovs-dev/2017-June/334762.html
>
>Version 2:
>
>   * Rebased on current master.
>   * Added time based batching RFC patch.
>   * Fixed mixing packets with different sources in same batch.
>
>Ilya Maximets (4):
>  dpif-netdev: Output packet batching.
>  netdev: Remove unused may_steal.
>  netdev: Remove useless cutlen.
>  dpif-netdev: Time based output batching.
>
> lib/dpif-netdev.c | 175
>++
> lib/netdev-bsd.c  |   7 +-
> lib/netdev-dpdk.c |  30 -
> lib/netdev-dummy.c|   6 +-
> lib/netdev-linux.c|   7 +-
> lib/netdev-provider.h |   7 +-
> lib/netdev.c  |  12 ++--
> lib/netdev.h  |   2 +-
> vswitchd/vswitch.xml  |  15 +
> 9 files changed, 189 insertions(+), 72 deletions(-)
>
>--
>2.7.4

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

[ovs-dev] [PATCH] tc: Correct convert ticks to msecs on parsing tc TM

[Roi Dayan]

From: Paul Blakey 

Use sysconf(_SC_CLK_TCK) to read run time "number of clock ticks per
second" and use that to convert ticks to msecs.
This is how iproute does the conversion when parsing tc filters.

Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
---
 lib/tc.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index 401690e..b3fe52f 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "byte-order.h"
 #include "netlink-socket.h"
@@ -399,12 +400,23 @@ static const struct nl_policy gact_policy[] = {
   .optional = false, },
 };
 
-#define JIFFIES_TO_MS(x) (x * 10)
+static int
+get_user_hz(void) {
+static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+static int user_hz = 100;
+
+if (ovsthread_once_start()) {
+user_hz = sysconf(_SC_CLK_TCK);
+ovsthread_once_done();
+}
+
+return user_hz;
+}
 
 static void
 nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
 {
-flower->lastused = time_msec() - JIFFIES_TO_MS(tm->lastuse);
+flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
 }
 
 static int
-- 
2.7.4

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