Re: [ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

[Numan Siddique]

>
> Huh, there's a fair amount of subtlety there.  What about logic similar to
> the following (untested) code?
>
> -=-=-=-=-=-=-=-=-
> ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen);
> /* When the netmask is applied, if the host portion is
>  * non-zero, the host can only use the specified
>  * address.  If zero, the host is allowed to use any
>  * address in the subnet. */
> if (ps.ipv4_addrs[i].addr & ~mask) {
> ds_put_format(, IP_FMT,
>   IP_ARGS(ps.ipv4_addrs[i].addr));
> } else {
> ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask,
>  );
> }
> -=-=-=-=-=-=-=-=-
>
> > ​Below is the port security description
> >
> > 
> >   Each element in the set may additionally contain one or more
> IPv4 or
> >   IPv6 addresses (or both), with optional masks.  If a mask is
> given, it
> >   must be a CIDR mask.  In addition to the restrictions
> described for
> >   Ethernet addresses above, such an element restricts the IPv4
> or IPv6
> >   addresses from which the host may send and to which it may
> receive
> >   packets to the specified addresses.  A masked address, if the
> host part
> >   is zero, indicates that the host is allowed to use any address
> in the
> >   subnet; if the host part is nonzero, the mask simply indicates
> the size
> >   of the subnet. In addition:
> > 
>
> The next paragraph is interesting because it describes what should happen
> with the subnet:
>
>   ·  If any IPv4 address is given, the host is also
> allowed to
>  receive packets  to  the  IPv4  local  broadcast
> address
>  255.255.255.255   and   to   IPv4   multicast
>  addresses
>  (224.0.0.0/4).  If an IPv4 address with a mask is
> given,
>  the host is also allowed to receive packets to the
> broad‐
>  cast address in that specified subnet.
>
> Would you mind expanding the tests to make sure that we're testing these
> different combinations both on the send and receive enforcement?  We
> clearly had some gaps before.
>
>
Sure. I will do that. Thanks for the suggestions and comments.

​


> Thanks for noticing these issues.
>
> --Justin
>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH RFC 1/1] netdev-dpdk.c: Add ingress-policing functionality.

[Daniele Di Proietto]

Hi Ian,

On 07/04/2016 06:00, "Stokes, Ian"  wrote:

>> > >71034a0..faf3583 100644
>> > >--- a/lib/netdev-dpdk.c
>> > >+++ b/lib/netdev-dpdk.c
>> > >@@ -53,6 +53,7 @@
>> > >
>> > > #include "rte_config.h"
>> > > #include "rte_mbuf.h"
>> > >+#include "rte_meter.h"
>> > > #include "rte_virtio_net.h"
>> > >
>> > > VLOG_DEFINE_THIS_MODULE(dpdk);
>> > >@@ -193,6 +194,11 @@ struct dpdk_ring {
>> > > struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);  };
>> > >
>> > >+struct ingress_policer {
>> > >+struct rte_meter_srtcm_params app_srtcm_params;
>> > >+struct rte_meter_srtcm in_policer; };
>> > >+
>> > > struct netdev_dpdk {
>> > > struct netdev up;
>> > > int port_id;
>> > >@@ -231,6 +237,13 @@ struct netdev_dpdk {
>> > > /* Identifier used to distinguish vhost devices from each other
>> */
>> > > char vhost_id[PATH_MAX];
>> > >
>> > >+/* Ingress Policer */
>> > >+rte_spinlock_t policer_lock;
>> > >+struct ingress_policer *ingress_policer;
>> > >+
>> >
>> > I would prefer not to have a lock at this level.
>> >
>> > I think it would make more sense to make the ingress_policer pointer
>> > RCU protected and embed the spinlock into struct ingress_policer, to
>> > protect the token bucket.
>> >
>> Sure I agree, I was modelling this on what we have currently in QoS just
>> for a rough implementation.
>> I can take a look at using RCU instead. This could possibly be extended
>> to the QoS use case also in the futue.
>
>Hi Daniele, I have been looking at an RCU implementation here but so far
>have not been able to get it working correctly.
>
>The issue I'm seeing is when I destroy the policer while traffic is
>passing a segfault sometimes occurs as the meter is in use when the
>ingress policer is set to NULL.
>
>I'm pretty sure this is down to my understanding (or lack thereof) of the
>ovsrcu behavior.
>
>Is the following high level implementation correct in your eyes?
>
>The ingress policer struct is as follows:
>
>struct ingress_policer {
>struct rte_meter_srtcm_params app_srtcm_params;
>struct rte_meter_srtcm in_policer;
>};
>
>From your comment above you mention embedding the spinlock in the ingress
>policer struct.
>Just to clarify, does the rcu by nature embed a spinlock or did you mean
>move the rte_spinlock policer_lock from the netdev_dpdk struct into the
>ingress policer struct?
>
>Is the behavior you are thinking of something like the following for when
>traffic is being processed?
>
>1. Get the rcu ingress_policer pointer.
>2. Lock the spinlock in the ingress policer struct.
>3. Set the ovsrcu pointer
>4. Call ovsrcu_synchronize to that all threads see that the policer is
>locked (Stop threads from accessing the ingress policer)
>5. Process the packets in the meter as usual.
>6. Unlock the spinlock.
>7. Set the ovsrcu pointer
>6. Synchronize again? (So that threads can access the ingress policer
>again)
>
>For destroying the ingress policer
>
>1. Get the rcu ingress_policer pointer.
>2. Lock the spinlock in the ingress policer struct.
>3. Set the ovsrcu pointer
>4. Call ovsrcu_synchronize to that all threads see that the policer is
>locked (Stop threads from accessing the meter)
>5. Destroy the ingress policer.
>6. Unlock the spinlock - if the spinlock is embedded in the ingress
>policer struct we have a problem here as it cannot  be free now, the
>struct has been destroyed.
>7. Set the ovsrcu pointer
>8. Synchronize again? (So that threads can see the ingress policer point
>for the netdev is now null)
>
>Thanks
>Ian
>

What I had in mind was simpler:

processing traffic:

p = ovsrcu_get(>ingress_policer)
if (p) {
rte_spinlock_lock(>lock);
policer_pkt_handle(p, pkts...);
rte_spinlock_unlock(>unlock);
}

destroying:

ovs_mutex_lock(>mutex);
...
p = ovsrcu_get_protected(>ingress_policer);
ovsrcu_postpone(destroy_policer, p);
ovsrcu_set(>ingress_policer, NULL);
...
ovs_mutex_unlock(>mutex);


static void destroy_policer (struct ingress_policer *p)
{
/*...*/
free(p);
}

My goal was to avoid taking the spinlock unless QoS in configured (p !=
NULL).


The pointer returned by ovsrcu_get() is guaranteed to be valid until the
next grace period, because ovsrcu_postpone() will not call free() until
the next grace period.

Or, from another point of view, after a grace period, when
ovsrcu_postpone() will actually call free(), all the other threads must
see the new value (NULL), so it is safe to reclaim memory.

The spinlock is used just to protect the meter.

Does this make sense?

Hope this helps,

Daniele

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

[ovs-dev] [PATCH v7 16/16] netdev-dpdk: Use ->reconfigure() call to change rx/tx queues.

[Daniele Di Proietto]

This introduces in dpif-netdev and netdev-dpdk the first use for the
newly introduce reconfigure netdev call.

When a request to change the number of queues comes, netdev-dpdk will
remember this and notify the upper layer via
netdev_request_reconfigure().

The datapath, instead of periodically calling netdev_set_multiq(), can
detect this and call reconfigure().

This mechanism can also be used to:
* Automatically match the number of rxq with the one provided by qemu
  via the new_device callback.
* Provide a way to change the MTU of dpdk devices at runtime.
* Move a DPDK vhost device to the proper NUMA socket.

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c |  69 +-
 lib/netdev-dpdk.c | 195 ++
 lib/netdev-provider.h |  23 +++---
 lib/netdev.c  |  34 +++--
 lib/netdev.h  |   3 +-
 5 files changed, 155 insertions(+), 169 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index fc81741..d9bfe80 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -257,8 +257,6 @@ struct dp_netdev_port {
 unsigned n_rxq; /* Number of elements in 'rxq' */
 struct netdev_rxq **rxq;
 char *type; /* Port type as requested by user. */
-int latest_requested_n_rxq; /* Latest requested from netdev number
-   of rx queues. */
 };
 
 /* Contained by struct dp_netdev_flow's 'stats' member.  */
@@ -1159,20 +1157,26 @@ port_create(const char *devname, const char *open_type, 
const char *type,
 /* There can only be ovs_numa_get_n_cores() pmd threads,
  * so creates a txq for each, and one extra for the non
  * pmd threads. */
-error = netdev_set_multiq(netdev, n_cores + 1,
-  netdev_requested_n_rxq(netdev));
+error = netdev_set_multiq(netdev, n_cores + 1);
 if (error && (error != EOPNOTSUPP)) {
 VLOG_ERR("%s, cannot set multiq", devname);
 goto out;
 }
 }
+
+if (netdev_is_reconf_required(netdev)) {
+error = netdev_reconfigure(netdev);
+if (error) {
+goto out;
+}
+}
+
 port = xzalloc(sizeof *port);
 port->port_no = port_no;
 port->netdev = netdev;
 port->n_rxq = netdev_n_rxq(netdev);
 port->rxq = xcalloc(port->n_rxq, sizeof *port->rxq);
 port->type = xstrdup(type);
-port->latest_requested_n_rxq = netdev_requested_n_rxq(netdev);
 
 for (i = 0; i < port->n_rxq; i++) {
 error = netdev_rxq_open(netdev, >rxq[i], i);
@@ -2453,27 +2457,6 @@ dpif_netdev_operate(struct dpif *dpif, struct dpif_op 
**ops, size_t n_ops)
 }
 }
 
-/* Returns true if the configuration for rx queues is changed. */
-static bool
-pmd_n_rxq_changed(const struct dp_netdev *dp)
-{
-struct dp_netdev_port *port;
-
-ovs_mutex_lock(>port_mutex);
-HMAP_FOR_EACH (port, node, >ports) {
-int requested_n_rxq = netdev_requested_n_rxq(port->netdev);
-
-if (netdev_is_pmd(port->netdev)
-&& port->latest_requested_n_rxq != requested_n_rxq) {
-ovs_mutex_unlock(>port_mutex);
-return true;
-}
-}
-ovs_mutex_unlock(>port_mutex);
-
-return false;
-}
-
 static bool
 cmask_equals(const char *a, const char *b)
 {
@@ -2597,11 +2580,9 @@ static int
 port_reconfigure(struct dp_netdev_port *port)
 {
 struct netdev *netdev = port->netdev;
-int requested_n_rxq = netdev_requested_n_rxq(netdev);
 int i, err;
 
-if (!netdev_is_pmd(port->netdev)
-|| port->latest_requested_n_rxq != requested_n_rxq) {
+if (!netdev_is_reconf_required(netdev)) {
 return 0;
 }
 
@@ -2612,15 +2593,14 @@ port_reconfigure(struct dp_netdev_port *port)
 }
 port->n_rxq = 0;
 
-/* Sets the new rx queue config. */
-err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() + 1,
-requested_n_rxq);
+/* Allows 'netdev' to apply the pending configuration changes. */
+err = netdev_reconfigure(netdev);
 if (err && (err != EOPNOTSUPP)) {
-VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u",
- netdev_get_name(port->netdev), requested_n_rxq);
+VLOG_ERR("Failed to set interface %s new configuration",
+ netdev_get_name(netdev));
 return err;
 }
-/* If the set_multiq() above succeeds, reopens the 'rxq's. */
+/* If the netdev_reconfigure( above succeeds, reopens the 'rxq's. */
 port->rxq = xrealloc(port->rxq, sizeof *port->rxq * netdev_n_rxq(netdev));
 for (i = 0; i < netdev_n_rxq(netdev); i++) {
 err = netdev_rxq_open(netdev, >rxq[i], i);
@@ -2664,6 +2644,22 @@ reconfigure_pmd_threads(struct dp_netdev *dp)
 dp_netdev_reset_pmd_threads(dp);
 }
 
+/* Returns true if one of the netdevs in 'dp' requires a reconfiguration */
+static bool
+ports_require_restart(const 

[ovs-dev] [PATCH v7 13/16] dpif-netdev: Change pmd thread configuration in dpif_netdev_run().

[Daniele Di Proietto]

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c   | 144 ++--
 lib/dpif-provider.h |   3 +-
 2 files changed, 84 insertions(+), 63 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index fba6592..bf04867 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -224,7 +224,9 @@ struct dp_netdev {
 ovsthread_key_t per_pmd_key;
 
 /* Cpu mask for pin of pmd threads. */
+char *requested_pmd_cmask;
 char *pmd_cmask;
+
 uint64_t last_tnl_conf_seq;
 };
 
@@ -2451,18 +2453,17 @@ dpif_netdev_operate(struct dpif *dpif, struct dpif_op 
**ops, size_t n_ops)
 }
 }
 
-/* Returns true if the configuration for rx queues or cpu mask
- * is changed. */
+/* Returns true if the configuration for rx queues is changed. */
 static bool
-pmd_config_changed(const struct dp_netdev *dp, const char *cmask)
+pmd_n_rxq_changed(const struct dp_netdev *dp)
 {
 struct dp_netdev_port *port;
 
 ovs_mutex_lock(>port_mutex);
 HMAP_FOR_EACH (port, node, >ports) {
-struct netdev *netdev = port->netdev;
-int requested_n_rxq = netdev_requested_n_rxq(netdev);
-if (netdev_is_pmd(netdev)
+int requested_n_rxq = netdev_requested_n_rxq(port->netdev);
+
+if (netdev_is_pmd(port->netdev)
 && port->latest_requested_n_rxq != requested_n_rxq) {
 ovs_mutex_unlock(>port_mutex);
 return true;
@@ -2470,69 +2471,29 @@ pmd_config_changed(const struct dp_netdev *dp, const 
char *cmask)
 }
 ovs_mutex_unlock(>port_mutex);
 
-if (dp->pmd_cmask != NULL && cmask != NULL) {
-return strcmp(dp->pmd_cmask, cmask);
-} else {
-return (dp->pmd_cmask != NULL || cmask != NULL);
+return false;
+}
+
+static bool
+cmask_equals(const char *a, const char *b)
+{
+if (a && b) {
+return !strcmp(a, b);
 }
+
+return a == NULL && b == NULL;
 }
 
-/* Resets pmd threads if the configuration for 'rxq's or cpu mask changes. */
+/* Changes the number or the affinity of pmd threads.  The changes are actually
+ * applied in dpif_netdev_run(). */
 static int
 dpif_netdev_pmd_set(struct dpif *dpif, const char *cmask)
 {
 struct dp_netdev *dp = get_dp_netdev(dpif);
 
-if (pmd_config_changed(dp, cmask)) {
-struct dp_netdev_port *port;
-
-dp_netdev_destroy_all_pmds(dp);
-
-ovs_mutex_lock(>port_mutex);
-HMAP_FOR_EACH (port, node, >ports) {
-struct netdev *netdev = port->netdev;
-int requested_n_rxq = netdev_requested_n_rxq(netdev);
-if (netdev_is_pmd(port->netdev)
-&& port->latest_requested_n_rxq != requested_n_rxq) {
-int i, err;
-
-/* Closes the existing 'rxq's. */
-for (i = 0; i < netdev_n_rxq(port->netdev); i++) {
-netdev_rxq_close(port->rxq[i]);
-port->rxq[i] = NULL;
-}
-port->n_rxq = 0;
-
-/* Sets the new rx queue config.  */
-err = netdev_set_multiq(port->netdev,
-ovs_numa_get_n_cores() + 1,
-requested_n_rxq);
-if (err && (err != EOPNOTSUPP)) {
-VLOG_ERR("Failed to set dpdk interface %s rx_queue to:"
- " %u", netdev_get_name(port->netdev),
- requested_n_rxq);
-ovs_mutex_unlock(>port_mutex);
-return err;
-}
-port->latest_requested_n_rxq = requested_n_rxq;
-/* If the set_multiq() above succeeds, reopens the 'rxq's. */
-port->n_rxq = netdev_n_rxq(port->netdev);
-port->rxq = xrealloc(port->rxq, sizeof *port->rxq * 
port->n_rxq);
-for (i = 0; i < port->n_rxq; i++) {
-netdev_rxq_open(port->netdev, >rxq[i], i);
-}
-}
-}
-/* Reconfigures the cpu mask. */
-ovs_numa_set_cpu_mask(cmask);
-free(dp->pmd_cmask);
-dp->pmd_cmask = cmask ? xstrdup(cmask) : NULL;
-
-/* Restores the non-pmd. */
-dp_netdev_set_nonpmd(dp);
-/* Restores all pmd threads. */
-dp_netdev_reset_pmd_threads(dp);
-ovs_mutex_unlock(>port_mutex);
+if (!cmask_equals(dp->requested_pmd_cmask, cmask)) {
+free(dp->requested_pmd_cmask);
+dp->requested_pmd_cmask = cmask ? xstrdup(cmask) : NULL;
 }
 
 return 0;
@@ -2632,6 +2593,59 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread 
*pmd,
 }
 }
 
+static void
+reconfigure_pmd_threads(struct dp_netdev *dp)
+OVS_REQUIRES(dp->port_mutex)
+{
+struct dp_netdev_port *port;
+
+dp_netdev_destroy_all_pmds(dp);
+
+HMAP_FOR_EACH (port, node, >ports) {
+struct netdev *netdev = port->netdev;
+int requested_n_rxq = 

[ovs-dev] [PATCH v7 07/16] hmap: Add HMAP_FOR_EACH_POP.

[Daniele Di Proietto]

Makes popping each member of the hmap a bit easier.

Signed-off-by: Daniele Di Proietto 
---
 lib/cfm.c|  5 ++---
 lib/hmap.h   |  4 
 lib/id-pool.c|  5 ++---
 lib/learning-switch.c|  5 ++---
 lib/netdev-linux.c   |  5 ++---
 lib/odp-util.c   |  7 +++
 ofproto/bond.c   | 10 --
 ofproto/in-band.c|  5 ++---
 ofproto/ofproto-dpif-ipfix.c |  5 ++---
 ofproto/ofproto-dpif-xlate.c |  5 ++---
 ofproto/ofproto.c|  5 ++---
 ofproto/pinsched.c   |  5 ++---
 ovn/controller-vtep/vtep.c   |  5 ++---
 ovn/controller/encaps.c  |  5 ++---
 ovn/controller/lport.c   |  5 ++---
 ovn/controller/ofctrl.c  |  5 ++---
 ovn/controller/physical.c|  4 +---
 ovn/controller/pinctrl.c |  5 ++---
 ovn/lib/expr.c   |  5 ++---
 ovn/northd/ovn-northd.c  | 10 --
 ovsdb/monitor.c  |  5 ++---
 ovsdb/row.c  |  5 ++---
 tests/library.at |  2 +-
 tests/test-hmap.c| 42 ++
 24 files changed, 93 insertions(+), 71 deletions(-)

diff --git a/lib/cfm.c b/lib/cfm.c
index cf1f725..fb077de 100644
--- a/lib/cfm.c
+++ b/lib/cfm.c
@@ -374,7 +374,7 @@ cfm_create(const struct netdev *netdev) OVS_EXCLUDED(mutex)
 void
 cfm_unref(struct cfm *cfm) OVS_EXCLUDED(mutex)
 {
-struct remote_mp *rmp, *rmp_next;
+struct remote_mp *rmp;
 
 if (!cfm) {
 return;
@@ -389,8 +389,7 @@ cfm_unref(struct cfm *cfm) OVS_EXCLUDED(mutex)
 hmap_remove(all_cfms, >hmap_node);
 ovs_mutex_unlock();
 
-HMAP_FOR_EACH_SAFE (rmp, rmp_next, node, >remote_mps) {
-hmap_remove(>remote_mps, >node);
+HMAP_FOR_EACH_POP (rmp, node, >remote_mps) {
 free(rmp);
 }
 
diff --git a/lib/hmap.h b/lib/hmap.h
index 53e75cc..08c4719 100644
--- a/lib/hmap.h
+++ b/lib/hmap.h
@@ -192,6 +192,10 @@ bool hmap_contains(const struct hmap *, const struct 
hmap_node *);
  __VA_ARGS__;   \
  (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \
  ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
+#define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP)  \
+while (!hmap_is_empty(HMAP)\
+   && (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), 1)  \
+   && (hmap_remove(HMAP, &(NODE)->MEMBER), 1))
 
 static inline struct hmap_node *hmap_first(const struct hmap *);
 static inline struct hmap_node *hmap_next(const struct hmap *,
diff --git a/lib/id-pool.c b/lib/id-pool.c
index 6b93d37..f32c008 100644
--- a/lib/id-pool.c
+++ b/lib/id-pool.c
@@ -69,10 +69,9 @@ id_pool_init(struct id_pool *pool, uint32_t base, uint32_t 
n_ids)
 static void
 id_pool_uninit(struct id_pool *pool)
 {
-struct id_node *id_node, *next;
+struct id_node *id_node;
 
-HMAP_FOR_EACH_SAFE(id_node, next, node, >map) {
-hmap_remove(>map, _node->node);
+HMAP_FOR_EACH_POP(id_node, node, >map) {
 free(id_node);
 }
 
diff --git a/lib/learning-switch.c b/lib/learning-switch.c
index 7c445b2..870192d 100644
--- a/lib/learning-switch.c
+++ b/lib/learning-switch.c
@@ -269,11 +269,10 @@ void
 lswitch_destroy(struct lswitch *sw)
 {
 if (sw) {
-struct lswitch_port *node, *next;
+struct lswitch_port *node;
 
 rconn_destroy(sw->rconn);
-HMAP_FOR_EACH_SAFE (node, next, hmap_node, >queue_numbers) {
-hmap_remove(>queue_numbers, >hmap_node);
+HMAP_FOR_EACH_POP (node, hmap_node, >queue_numbers) {
 free(node);
 }
 shash_destroy(>queue_names);
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index a7d7ac7..2c1ffec 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -3851,10 +3851,9 @@ static void
 htb_tc_destroy(struct tc *tc)
 {
 struct htb *htb = CONTAINER_OF(tc, struct htb, tc);
-struct htb_class *hc, *next;
+struct htb_class *hc;
 
-HMAP_FOR_EACH_SAFE (hc, next, tc_queue.hmap_node, >tc.queues) {
-hmap_remove(>tc.queues, >tc_queue.hmap_node);
+HMAP_FOR_EACH_POP (hc, tc_queue.hmap_node, >tc.queues) {
 free(hc);
 }
 tc_destroy(tc);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index b4689cc..3c75379 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2081,10 +2081,9 @@ odp_portno_names_get(const struct hmap *portno_names, 
odp_port_t port_no)
 void
 odp_portno_names_destroy(struct hmap *portno_names)
 {
-struct odp_portno_names *odp_portno_names, *odp_portno_names_next;
-HMAP_FOR_EACH_SAFE (odp_portno_names, odp_portno_names_next,
-hmap_node, portno_names) {
-hmap_remove(portno_names, _portno_names->hmap_node);
+struct odp_portno_names *odp_portno_names;
+
+HMAP_FOR_EACH_POP (odp_portno_names, hmap_node, portno_names) {
 

[ovs-dev] [PATCH v7 03/16] dpif-netdev: Factor out port_create() from do_add_port().

[Daniele Di Proietto]

Instead of performing every operation inside do_port_add() it seems
clearer to introduce port_create(), since we already have
port_destroy().

No functional change.

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 69 ++-
 1 file changed, 43 insertions(+), 26 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 78e4e35..27277e8 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1096,29 +1096,22 @@ hash_port_no(odp_port_t port_no)
 }
 
 static int
-do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
-odp_port_t port_no)
-OVS_REQUIRES(dp->port_mutex)
+port_create(const char *devname, const char *open_type, const char *type,
+odp_port_t port_no, struct dp_netdev_port **portp)
 {
 struct netdev_saved_flags *sf;
 struct dp_netdev_port *port;
-struct netdev *netdev;
 enum netdev_flags flags;
-const char *open_type;
-int error = 0;
-int i, n_open_rxqs = 0;
+struct netdev *netdev;
+int n_open_rxqs = 0;
+int i, error;
 
-/* Reject devices already in 'dp'. */
-if (!get_port_by_name(dp, devname, )) {
-error = EEXIST;
-goto out;
-}
+*portp = NULL;
 
 /* Open and validate network device. */
-open_type = dpif_netdev_port_open_type(dp->class, type);
 error = netdev_open(devname, open_type, );
 if (error) {
-goto out;
+return error;
 }
 /* XXX reject non-Ethernet devices */
 
@@ -1126,7 +1119,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
 if (flags & NETDEV_LOOPBACK) {
 VLOG_ERR("%s: cannot add a loopback device", devname);
 error = EINVAL;
-goto out_close;
+goto out;
 }
 
 if (netdev_is_pmd(netdev)) {
@@ -1135,7 +1128,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
 if (n_cores == OVS_CORE_UNSPEC) {
 VLOG_ERR("%s, cannot get cpu core info", devname);
 error = ENOENT;
-goto out_close;
+goto out;
 }
 /* There can only be ovs_numa_get_n_cores() pmd threads,
  * so creates a txq for each, and one extra for the non
@@ -1144,14 +1137,14 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
   netdev_requested_n_rxq(netdev));
 if (error && (error != EOPNOTSUPP)) {
 VLOG_ERR("%s, cannot set multiq", devname);
-goto out_close;
+goto out;
 }
 }
 port = xzalloc(sizeof *port);
 port->port_no = port_no;
 port->netdev = netdev;
 port->n_rxq = netdev_n_rxq(netdev);
-port->rxq = xmalloc(sizeof *port->rxq * port->n_rxq);
+port->rxq = xcalloc(port->n_rxq, sizeof *port->rxq);
 port->type = xstrdup(type);
 port->latest_requested_n_rxq = netdev_requested_n_rxq(netdev);
 
@@ -1171,12 +1164,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
 }
 port->sf = sf;
 
-cmap_insert(>ports, >node, hash_port_no(port_no));
-
-if (netdev_is_pmd(netdev)) {
-dp_netdev_add_port_to_pmds(dp, port);
-}
-seq_change(dp->port_seq);
+*portp = port;
 
 return 0;
 
@@ -1187,13 +1175,42 @@ out_rxq_close:
 free(port->type);
 free(port->rxq);
 free(port);
-out_close:
-netdev_close(netdev);
+
 out:
+netdev_close(netdev);
 return error;
 }
 
 static int
+do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
+odp_port_t port_no)
+OVS_REQUIRES(dp->port_mutex)
+{
+struct dp_netdev_port *port;
+int error;
+
+/* Reject devices already in 'dp'. */
+if (!get_port_by_name(dp, devname, )) {
+return EEXIST;
+}
+
+error = port_create(devname, dpif_netdev_port_open_type(dp->class, type),
+type, port_no, );
+if (error) {
+return error;
+}
+
+cmap_insert(>ports, >node, hash_port_no(port_no));
+
+if (netdev_is_pmd(port->netdev)) {
+dp_netdev_add_port_to_pmds(dp, port);
+}
+seq_change(dp->port_seq);
+
+return 0;
+}
+
+static int
 dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
  odp_port_t *port_nop)
 {
-- 
2.1.4

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

[ovs-dev] [PATCH v7 15/16] netdev: Add reconfigure request mechanism.

[Daniele Di Proietto]

A netdev provider, especially a PMD provider (like netdev DPDK) might
not be able to change some of its parameters (such as MTU, or number of
queues) without stopping everything and restarting.

This commit introduces a mechanism that allows a netdev provider to
request a restart (netdev_request_reconfigure()).  The upper layer can
be notified via netdev_wait_reconf_required() and
netdev_is_reconf_required().  After closing all the rxqs the upper layer
can finally call netdev_reconfigure(), to make sure that the new
configuration is in place.

This will be used by next commit to reconfigure rx and tx queues in
netdev-dpdk.

Signed-off-by: Daniele Di Proietto 
Tested-by: Ilya Maximets 
Acked-by: Ilya Maximets 
Acked-by: Mark Kavanagh 
---
 lib/netdev-bsd.c  |  1 +
 lib/netdev-dpdk.c |  1 +
 lib/netdev-dummy.c|  1 +
 lib/netdev-linux.c|  1 +
 lib/netdev-provider.h | 27 ++-
 lib/netdev-vport.c|  1 +
 lib/netdev.c  | 38 ++
 lib/netdev.h  |  4 
 8 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 49c05f4..32e8f74 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -1536,6 +1536,7 @@ netdev_bsd_update_flags(struct netdev *netdev_, enum 
netdev_flags off,
 netdev_bsd_arp_lookup, /* arp_lookup */  \
  \
 netdev_bsd_update_flags, \
+NULL, /* reconfigure */  \
  \
 netdev_bsd_rxq_alloc,\
 netdev_bsd_rxq_construct,\
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index c7217ea..635cc74 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2716,6 +2716,7 @@ static const struct dpdk_qos_ops egress_policer_ops = {
 NULL,   /* arp_lookup */  \
   \
 netdev_dpdk_update_flags, \
+NULL,   /* reconfigure */ \
   \
 netdev_dpdk_rxq_alloc,\
 netdev_dpdk_rxq_construct,\
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index a1013ff..09d753f 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1275,6 +1275,7 @@ static const struct netdev_class dummy_class = {
 NULL,   /* arp_lookup */
 
 netdev_dummy_update_flags,
+NULL,   /* reconfigure */
 
 netdev_dummy_rxq_alloc,
 netdev_dummy_rxq_construct,
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 2c1ffec..1af08f3 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2806,6 +2806,7 @@ netdev_linux_update_flags(struct netdev *netdev_, enum 
netdev_flags off,
 netdev_linux_arp_lookup,\
 \
 netdev_linux_update_flags,  \
+NULL,   /* reconfigure */   \
 \
 netdev_linux_rxq_alloc, \
 netdev_linux_rxq_construct, \
diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
index cda25eb..853fc44 100644
--- a/lib/netdev-provider.h
+++ b/lib/netdev-provider.h
@@ -52,6 +52,16 @@ struct netdev {
  * 'netdev''s flags, features, ethernet address, or carrier changes. */
 uint64_t change_seq;
 
+/* A netdev provider might be unable to change some of the device's
+ * parameter (n_rxq, mtu) when the device is in use.  In this case
+ * the provider can notify the upper layer by calling
+ * netdev_request_reconfigure().  The upper layer will react by stopping
+ * the operations on the device and calling netdev_reconfigure() to allow
+ * the configuration changes.  'last_reconfigure_seq' remembers the value
+ * of 'reconfigure_seq' when the last reconfiguration happened. */
+struct seq *reconfigure_seq;
+uint64_t last_reconfigure_seq;
+
 /* The core netdev code initializes these at netdev construction and only
  * provide read-only access to its client.  Netdev implementations may
  * modify them. */
@@ -64,7 +74,7 @@ struct netdev {
 struct ovs_list saved_flags_list; /* Contains "struct netdev_saved_flags". 
*/
 };
 
-static void
+static inline void
 netdev_change_seq_changed(const struct netdev *netdev_)
 {
 struct netdev *netdev = CONST_CAST(struct netdev *, netdev_);
@@ -75,6 +85,12 @@ netdev_change_seq_changed(const struct netdev *netdev_)
 }
 }
 

[ovs-dev] [PATCH v7 12/16] ofproto-dpif: Call dpif_poll_threads_set() before dpif_run()

[Daniele Di Proietto]

An upcoming commit will make dpif_poll_threads_set() record the
requested configuration and dpif_run() apply it, so it makes sense to
change the order.

Signed-off-by: Daniele Di Proietto 
Tested-by: Ilya Maximets 
Acked-by: Ilya Maximets 
Acked-by: Mark Kavanagh 
---
 ofproto/ofproto-dpif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index aceb11f..7eff63f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -536,6 +536,8 @@ type_run(const char *type)
 return 0;
 }
 
+/* This must be called before dpif_run() */
+dpif_poll_threads_set(backer->dpif, pmd_cpu_mask);
 
 if (dpif_run(backer->dpif)) {
 backer->need_revalidate = REV_RECONFIGURE;
@@ -564,8 +566,6 @@ type_run(const char *type)
 udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
 }
 
-dpif_poll_threads_set(backer->dpif, pmd_cpu_mask);
-
 if (backer->need_revalidate) {
 struct ofproto_dpif *ofproto;
 struct simap_node *node;
-- 
2.1.4

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

[ovs-dev] [PATCH v7 14/16] dpif-netdev: Handle errors in reconfigure_pmd_threads().

[Daniele Di Proietto]

Errors returned by netdev_set_multiq() and netdev_rxq_open() weren't
handled properly in reconfigure_pmd_threads().  In case of error now we
remove the port from the datapath.

Also, part of the code is moved in a new function, port_reconfigure().

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 78 ++-
 1 file changed, 48 insertions(+), 30 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index bf04867..fc81741 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2593,44 +2593,62 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread 
*pmd,
 }
 }
 
+static int
+port_reconfigure(struct dp_netdev_port *port)
+{
+struct netdev *netdev = port->netdev;
+int requested_n_rxq = netdev_requested_n_rxq(netdev);
+int i, err;
+
+if (!netdev_is_pmd(port->netdev)
+|| port->latest_requested_n_rxq != requested_n_rxq) {
+return 0;
+}
+
+/* Closes the existing 'rxq's. */
+for (i = 0; i < port->n_rxq; i++) {
+netdev_rxq_close(port->rxq[i]);
+port->rxq[i] = NULL;
+}
+port->n_rxq = 0;
+
+/* Sets the new rx queue config. */
+err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() + 1,
+requested_n_rxq);
+if (err && (err != EOPNOTSUPP)) {
+VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u",
+ netdev_get_name(port->netdev), requested_n_rxq);
+return err;
+}
+/* If the set_multiq() above succeeds, reopens the 'rxq's. */
+port->rxq = xrealloc(port->rxq, sizeof *port->rxq * netdev_n_rxq(netdev));
+for (i = 0; i < netdev_n_rxq(netdev); i++) {
+err = netdev_rxq_open(netdev, >rxq[i], i);
+if (err) {
+return err;
+}
+port->n_rxq++;
+}
+
+return 0;
+}
+
 static void
 reconfigure_pmd_threads(struct dp_netdev *dp)
 OVS_REQUIRES(dp->port_mutex)
 {
-struct dp_netdev_port *port;
+struct dp_netdev_port *port, *next;
 
 dp_netdev_destroy_all_pmds(dp);
 
-HMAP_FOR_EACH (port, node, >ports) {
-struct netdev *netdev = port->netdev;
-int requested_n_rxq = netdev_requested_n_rxq(netdev);
-if (netdev_is_pmd(port->netdev)
-&& port->latest_requested_n_rxq != requested_n_rxq) {
-int i, err;
+HMAP_FOR_EACH_SAFE (port, next, node, >ports) {
+int err;
 
-/* Closes the existing 'rxq's. */
-for (i = 0; i < port->n_rxq; i++) {
-netdev_rxq_close(port->rxq[i]);
-port->rxq[i] = NULL;
-}
-port->n_rxq = 0;
-
-/* Sets the new rx queue config. */
-err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() + 1,
-requested_n_rxq);
-if (err && (err != EOPNOTSUPP)) {
-VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u",
- netdev_get_name(port->netdev),
- requested_n_rxq);
-return;
-}
-port->latest_requested_n_rxq = requested_n_rxq;
-/* If the set_multiq() above succeeds, reopens the 'rxq's. */
-port->n_rxq = netdev_n_rxq(port->netdev);
-port->rxq = xrealloc(port->rxq, sizeof *port->rxq * port->n_rxq);
-for (i = 0; i < port->n_rxq; i++) {
-netdev_rxq_open(port->netdev, >rxq[i], i);
-}
+err = port_reconfigure(port);
+if (err) {
+hmap_remove(>ports, >node);
+seq_change(dp->port_seq);
+port_destroy(port);
 }
 }
 /* Reconfigures the cpu mask. */
-- 
2.1.4

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

[ovs-dev] [PATCH v7 11/16] ovs-thread: Do not quiesce in ovs_mutex_cond_wait().

[Daniele Di Proietto]

ovs_mutex_cond_wait() is used in many functions in dpif-netdev to
synchronize with pmd threads, but we can't guarantee that the callers do
not hold RCU references, so it's better to avoid quiescing.

In system_stats_thread_func() the code relied on ovs_mutex_cond_wait()
to introduce a quiescent state, so explicit calls to
ovsrcu_quiesce_start() and ovsrcu_quiesce_end() are added there.

Signed-off-by: Daniele Di Proietto 
---
 lib/ovs-thread.c| 2 --
 vswitchd/system-stats.c | 6 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index 3c065cf..26dd928 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -253,9 +253,7 @@ ovs_mutex_cond_wait(pthread_cond_t *cond, const struct 
ovs_mutex *mutex_)
 struct ovs_mutex *mutex = CONST_CAST(struct ovs_mutex *, mutex_);
 int error;
 
-ovsrcu_quiesce_start();
 error = pthread_cond_wait(cond, >lock);
-ovsrcu_quiesce_end();
 
 if (OVS_UNLIKELY(error)) {
 ovs_abort(error, "pthread_cond_wait failed");
diff --git a/vswitchd/system-stats.c b/vswitchd/system-stats.c
index df4971e..129f0cf 100644
--- a/vswitchd/system-stats.c
+++ b/vswitchd/system-stats.c
@@ -37,6 +37,7 @@
 #include "json.h"
 #include "latch.h"
 #include "openvswitch/ofpbuf.h"
+#include "ovs-rcu.h"
 #include "ovs-thread.h"
 #include "poll-loop.h"
 #include "shash.h"
@@ -615,7 +616,12 @@ system_stats_thread_func(void *arg OVS_UNUSED)
 
 ovs_mutex_lock();
 while (!enabled) {
+/* The thread is sleeping, potentially for a long time, and it's
+ * not holding RCU protected references, so it makes sense to
+ * quiesce */
+ovsrcu_quiesce_start();
 ovs_mutex_cond_wait(, );
+ovsrcu_quiesce_end();
 }
 ovs_mutex_unlock();
 
-- 
2.1.4

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

[ovs-dev] [PATCH v7 08/16] dpif-netdev: Add pmd thread local port cache for transmission.

[Daniele Di Proietto]

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 243 +++---
 1 file changed, 175 insertions(+), 68 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 8c5893d..5d1cc43 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -185,6 +185,7 @@ static bool dpcls_lookup(const struct dpcls *cls,
  *
  *dp_netdev_mutex (global)
  *port_mutex
+ *non_pmd_mutex
  */
 struct dp_netdev {
 const struct dpif_class *const class;
@@ -380,6 +381,13 @@ struct rxq_poll {
 struct ovs_list node;
 };
 
+/* Contained by struct dp_netdev_pmd_thread's 'port_cache' or 'tx_ports'. */
+struct tx_port {
+odp_port_t port_no;
+struct netdev *netdev;
+struct hmap_node node;
+};
+
 /* PMD: Poll modes drivers.  PMD accesses devices via polling to eliminate
  * the performance overhead of interrupt processing.  Therefore netdev can
  * not implement rx-wait for these devices.  dpif-netdev needs to poll
@@ -436,10 +444,18 @@ struct dp_netdev_pmd_thread {
 atomic_int tx_qid;  /* Queue id used by this pmd thread to
  * send packets on all netdevs */
 
-struct ovs_mutex poll_mutex;/* Mutex for poll_list. */
+struct ovs_mutex port_mutex;/* Mutex for 'poll_list' and 'tx_ports'. */
 /* List of rx queues to poll. */
 struct ovs_list poll_list OVS_GUARDED;
-int poll_cnt;   /* Number of elemints in poll_list. */
+/* Number of elements in 'poll_list' */
+int poll_cnt;
+/* Map of 'tx_port's used for transmission.  Written by the main thread,
+ * read by the pmd thread. */
+struct hmap tx_ports OVS_GUARDED;
+
+/* Map of 'tx_port' used in the fast path. This is a thread-local copy
+ * 'tx_ports'. */
+struct hmap port_cache;
 
 /* Only a pmd thread can write on its own 'cycles' and 'stats'.
  * The main thread keeps 'stats_zero' and 'cycles_zero' as base
@@ -495,7 +511,7 @@ dp_netdev_pmd_get_next(struct dp_netdev *dp, struct 
cmap_position *pos);
 static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp);
 static void dp_netdev_del_pmds_on_numa(struct dp_netdev *dp, int numa_id);
 static void dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int numa_id);
-static void dp_netdev_pmd_clear_poll_list(struct dp_netdev_pmd_thread *pmd);
+static void dp_netdev_pmd_clear_ports(struct dp_netdev_pmd_thread *pmd);
 static void dp_netdev_del_port_from_all_pmds(struct dp_netdev *dp,
  struct dp_netdev_port *port);
 static void
@@ -509,6 +525,8 @@ static void dp_netdev_reset_pmd_threads(struct dp_netdev 
*dp);
 static bool dp_netdev_pmd_try_ref(struct dp_netdev_pmd_thread *pmd);
 static void dp_netdev_pmd_unref(struct dp_netdev_pmd_thread *pmd);
 static void dp_netdev_pmd_flow_flush(struct dp_netdev_pmd_thread *pmd);
+static void pmd_load_cached_ports(struct dp_netdev_pmd_thread *pmd)
+OVS_REQUIRES(pmd->port_mutex);
 
 static inline bool emc_entry_alive(struct emc_entry *ce);
 static void emc_clear_entry(struct emc_entry *ce);
@@ -691,7 +709,7 @@ pmd_info_show_rxq(struct ds *reply, struct 
dp_netdev_pmd_thread *pmd)
 ds_put_format(reply, "pmd thread numa_id %d core_id %u:\n",
   pmd->numa_id, pmd->core_id);
 
-ovs_mutex_lock(>poll_mutex);
+ovs_mutex_lock(>port_mutex);
 LIST_FOR_EACH (poll, node, >poll_list) {
 const char *name = netdev_get_name(poll->port->netdev);
 
@@ -705,7 +723,7 @@ pmd_info_show_rxq(struct ds *reply, struct 
dp_netdev_pmd_thread *pmd)
 ds_put_format(reply, " %d", netdev_rxq_get_queue_id(poll->rx));
 prev_name = name;
 }
-ovs_mutex_unlock(>poll_mutex);
+ovs_mutex_unlock(>port_mutex);
 ds_put_cstr(reply, "\n");
 }
 }
@@ -1078,6 +1096,11 @@ dp_netdev_reload_pmd__(struct dp_netdev_pmd_thread *pmd)
 int old_seq;
 
 if (pmd->core_id == NON_PMD_CORE_ID) {
+ovs_mutex_lock(>dp->non_pmd_mutex);
+ovs_mutex_lock(>port_mutex);
+pmd_load_cached_ports(pmd);
+ovs_mutex_unlock(>port_mutex);
+ovs_mutex_unlock(>dp->non_pmd_mutex);
 return;
 }
 
@@ -1200,9 +1223,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
 
 cmap_insert(>ports, >node, hash_port_no(port_no));
 
-if (netdev_is_pmd(port->netdev)) {
-dp_netdev_add_port_to_pmds(dp, port);
-}
+dp_netdev_add_port_to_pmds(dp, port);
 seq_change(dp->port_seq);
 
 return 0;
@@ -1371,6 +1392,9 @@ do_del_port(struct dp_netdev *dp, struct dp_netdev_port 
*port)
 {
 cmap_remove(>ports, >node, hash_odp_port(port->port_no));
 seq_change(dp->port_seq);
+
+dp_netdev_del_port_from_all_pmds(dp, port);
+
 if (netdev_is_pmd(port->netdev)) {
 int numa_id = netdev_get_numa_id(port->netdev);
 
@@ -1380,8 +1404,6 @@ do_del_port(struct dp_netdev *dp, 

[ovs-dev] [PATCH v7 10/16] dpif-netdev: Use hmap for ports.

[Daniele Di Proietto]

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 93 ---
 1 file changed, 55 insertions(+), 38 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 5d1cc43..fba6592 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -198,7 +198,7 @@ struct dp_netdev {
  *
  * Protected by RCU.  Take the mutex to add or remove ports. */
 struct ovs_mutex port_mutex;
-struct cmap ports;
+struct hmap ports;
 struct seq *port_seq;   /* Incremented whenever a port changes. */
 
 /* Protects access to ofproto-dpif-upcall interface during revalidator
@@ -229,7 +229,8 @@ struct dp_netdev {
 };
 
 static struct dp_netdev_port *dp_netdev_lookup_port(const struct dp_netdev *dp,
-odp_port_t);
+odp_port_t)
+OVS_REQUIRES(>port_mutex);
 
 enum dp_stat_type {
 DP_STAT_EXACT_HIT,  /* Packets that had an exact match (emc). */
@@ -249,7 +250,7 @@ enum pmd_cycles_counter_type {
 struct dp_netdev_port {
 odp_port_t port_no;
 struct netdev *netdev;
-struct cmap_node node;  /* Node in dp_netdev's 'ports'. */
+struct hmap_node node;  /* Node in dp_netdev's 'ports'. */
 struct netdev_saved_flags *sf;
 unsigned n_rxq; /* Number of elements in 'rxq' */
 struct netdev_rxq **rxq;
@@ -475,9 +476,11 @@ struct dpif_netdev {
 };
 
 static int get_port_by_number(struct dp_netdev *dp, odp_port_t port_no,
-  struct dp_netdev_port **portp);
+  struct dp_netdev_port **portp)
+OVS_REQUIRES(dp->port_mutex);
 static int get_port_by_name(struct dp_netdev *dp, const char *devname,
-struct dp_netdev_port **portp);
+struct dp_netdev_port **portp)
+OVS_REQUIRES(dp->port_mutex);
 static void dp_netdev_free(struct dp_netdev *)
 OVS_REQUIRES(dp_netdev_mutex);
 static int do_add_port(struct dp_netdev *dp, const char *devname,
@@ -521,7 +524,8 @@ dp_netdev_add_rxq_to_pmd(struct dp_netdev_pmd_thread *pmd,
  struct dp_netdev_port *port, struct netdev_rxq *rx);
 static struct dp_netdev_pmd_thread *
 dp_netdev_less_loaded_pmd_on_numa(struct dp_netdev *dp, int numa_id);
-static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp);
+static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp)
+OVS_REQUIRES(dp->port_mutex);
 static bool dp_netdev_pmd_try_ref(struct dp_netdev_pmd_thread *pmd);
 static void dp_netdev_pmd_unref(struct dp_netdev_pmd_thread *pmd);
 static void dp_netdev_pmd_flow_flush(struct dp_netdev_pmd_thread *pmd);
@@ -912,7 +916,7 @@ create_dp_netdev(const char *name, const struct dpif_class 
*class,
 atomic_flag_clear(>destroyed);
 
 ovs_mutex_init(>port_mutex);
-cmap_init(>ports);
+hmap_init(>ports);
 dp->port_seq = seq_create();
 fat_rwlock_init(>upcall_rwlock);
 
@@ -983,7 +987,7 @@ static void
 dp_netdev_free(struct dp_netdev *dp)
 OVS_REQUIRES(dp_netdev_mutex)
 {
-struct dp_netdev_port *port;
+struct dp_netdev_port *port, *next;
 
 shash_find_and_delete(_netdevs, dp->name);
 
@@ -992,15 +996,14 @@ dp_netdev_free(struct dp_netdev *dp)
 ovsthread_key_delete(dp->per_pmd_key);
 
 ovs_mutex_lock(>port_mutex);
-CMAP_FOR_EACH (port, node, >ports) {
-/* PMD threads are destroyed here. do_del_port() cannot quiesce */
+HMAP_FOR_EACH_SAFE (port, next, node, >ports) {
 do_del_port(dp, port);
 }
 ovs_mutex_unlock(>port_mutex);
 cmap_destroy(>poll_threads);
 
 seq_destroy(dp->port_seq);
-cmap_destroy(>ports);
+hmap_destroy(>ports);
 ovs_mutex_destroy(>port_mutex);
 
 /* Upcalls must be disabled at this point */
@@ -1221,7 +1224,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, 
const char *type,
 return error;
 }
 
-cmap_insert(>ports, >node, hash_port_no(port_no));
+hmap_insert(>ports, >node, hash_port_no(port_no));
 
 dp_netdev_add_port_to_pmds(dp, port);
 seq_change(dp->port_seq);
@@ -1287,10 +1290,11 @@ is_valid_port_number(odp_port_t port_no)
 
 static struct dp_netdev_port *
 dp_netdev_lookup_port(const struct dp_netdev *dp, odp_port_t port_no)
+OVS_REQUIRES(>port_mutex)
 {
 struct dp_netdev_port *port;
 
-CMAP_FOR_EACH_WITH_HASH (port, node, hash_port_no(port_no), >ports) {
+HMAP_FOR_EACH_WITH_HASH (port, node, hash_port_no(port_no), >ports) {
 if (port->port_no == port_no) {
 return port;
 }
@@ -1301,6 +1305,7 @@ dp_netdev_lookup_port(const struct dp_netdev *dp, 
odp_port_t port_no)
 static int
 get_port_by_number(struct dp_netdev *dp,
odp_port_t port_no, struct dp_netdev_port **portp)
+OVS_REQUIRES(>port_mutex)
 {
 if (!is_valid_port_number(port_no)) {
 *portp = NULL;
@@ -1337,7 

[ovs-dev] [PATCH v7 09/16] hmap: Use struct for hmap_at_position().

[Daniele Di Proietto]

The interface will be more similar to the cmap.

Signed-off-by: Daniele Di Proietto 
---
 lib/hmap.c | 26 --
 lib/hmap.h |  7 ++-
 lib/sset.c | 12 +---
 lib/sset.h |  7 ++-
 ofproto/ofproto-dpif.c |  8 +++-
 5 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/lib/hmap.c b/lib/hmap.c
index b70ce51..9462c5e 100644
--- a/lib/hmap.c
+++ b/lib/hmap.c
@@ -236,24 +236,22 @@ hmap_random_node(const struct hmap *hmap)
 }
 
 /* Returns the next node in 'hmap' in hash order, or NULL if no nodes remain in
- * 'hmap'.  Uses '*bucketp' and '*offsetp' to determine where to begin
- * iteration, and stores new values to pass on the next iteration into them
- * before returning.
+ * 'hmap'.  Uses '*pos' to determine where to begin iteration, and updates
+ * '*pos' to pass on the next iteration into them before returning.
  *
  * It's better to use plain HMAP_FOR_EACH and related functions, since they are
  * faster and better at dealing with hmaps that change during iteration.
  *
- * Before beginning iteration, store 0 into '*bucketp' and '*offsetp'.
- */
+ * Before beginning iteration, set '*pos' to all zeros. */
 struct hmap_node *
 hmap_at_position(const struct hmap *hmap,
- uint32_t *bucketp, uint32_t *offsetp)
+ struct hmap_position *pos)
 {
 size_t offset;
 size_t b_idx;
 
-offset = *offsetp;
-for (b_idx = *bucketp; b_idx <= hmap->mask; b_idx++) {
+offset = pos->offset;
+for (b_idx = pos->bucket; b_idx <= hmap->mask; b_idx++) {
 struct hmap_node *node;
 size_t n_idx;
 
@@ -261,11 +259,11 @@ hmap_at_position(const struct hmap *hmap,
  n_idx++, node = node->next) {
 if (n_idx == offset) {
 if (node->next) {
-*bucketp = node->hash & hmap->mask;
-*offsetp = offset + 1;
+pos->bucket = node->hash & hmap->mask;
+pos->offset = offset + 1;
 } else {
-*bucketp = (node->hash & hmap->mask) + 1;
-*offsetp = 0;
+pos->bucket = (node->hash & hmap->mask) + 1;
+pos->offset = 0;
 }
 return node;
 }
@@ -273,8 +271,8 @@ hmap_at_position(const struct hmap *hmap,
 offset = 0;
 }
 
-*bucketp = 0;
-*offsetp = 0;
+pos->bucket = 0;
+pos->offset = 0;
 return NULL;
 }
 
diff --git a/lib/hmap.h b/lib/hmap.h
index 08c4719..9a96c5f 100644
--- a/lib/hmap.h
+++ b/lib/hmap.h
@@ -201,8 +201,13 @@ static inline struct hmap_node *hmap_first(const struct 
hmap *);
 static inline struct hmap_node *hmap_next(const struct hmap *,
   const struct hmap_node *);
 
+struct hmap_position {
+unsigned int bucket;
+unsigned int offset;
+};
+
 struct hmap_node *hmap_at_position(const struct hmap *,
-   uint32_t *bucket, uint32_t *offset);
+   struct hmap_position *);
 
 /* Returns the number of nodes currently in 'hmap'. */
 static inline size_t
diff --git a/lib/sset.c b/lib/sset.c
index f9d4fc0..4fd3fae 100644
--- a/lib/sset.c
+++ b/lib/sset.c
@@ -251,21 +251,19 @@ sset_equals(const struct sset *a, const struct sset *b)
 }
 
 /* Returns the next node in 'set' in hash order, or NULL if no nodes remain in
- * 'set'.  Uses '*bucketp' and '*offsetp' to determine where to begin
- * iteration, and stores new values to pass on the next iteration into them
- * before returning.
+ * 'set'.  Uses '*pos' to determine where to begin iteration, and updates
+ * '*pos' to pass on the next iteration into them before returning.
  *
  * It's better to use plain SSET_FOR_EACH and related functions, since they are
  * faster and better at dealing with ssets that change during iteration.
  *
- * Before beginning iteration, store 0 into '*bucketp' and '*offsetp'.
- */
+ * Before beginning iteration, set '*pos' to all zeros. */
 struct sset_node *
-sset_at_position(const struct sset *set, uint32_t *bucketp, uint32_t *offsetp)
+sset_at_position(const struct sset *set, struct sset_position *pos)
 {
 struct hmap_node *hmap_node;
 
-hmap_node = hmap_at_position(>map, bucketp, offsetp);
+hmap_node = hmap_at_position(>map, >pos);
 return SSET_NODE_FROM_HMAP_NODE(hmap_node);
 }
 
diff --git a/lib/sset.h b/lib/sset.h
index 7d1d496..9c2f703 100644
--- a/lib/sset.h
+++ b/lib/sset.h
@@ -64,8 +64,13 @@ char *sset_pop(struct sset *);
 struct sset_node *sset_find(const struct sset *, const char *);
 bool sset_contains(const struct sset *, const char *);
 bool sset_equals(const struct sset *, const struct sset *);
+
+struct sset_position {
+struct hmap_position pos;
+};
+
 struct sset_node *sset_at_position(const struct sset *,
-   uint32_t *bucketp, uint32_t 

[ovs-dev] [PATCH v7 04/16] dpif-netdev: Add functions to modify rxq without reloading pmd threads.

[Daniele Di Proietto]

This commit introduces some functions to add/remove rxqs from pmd
threads without reloading them.  They will be used by next commits.

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 77 ---
 1 file changed, 56 insertions(+), 21 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 27277e8..9c32c64 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -496,8 +496,6 @@ static void dp_netdev_destroy_all_pmds(struct dp_netdev 
*dp);
 static void dp_netdev_del_pmds_on_numa(struct dp_netdev *dp, int numa_id);
 static void dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int numa_id);
 static void dp_netdev_pmd_clear_poll_list(struct dp_netdev_pmd_thread *pmd);
-static void dp_netdev_del_port_from_pmd(struct dp_netdev_port *port,
-struct dp_netdev_pmd_thread *pmd);
 static void dp_netdev_del_port_from_all_pmds(struct dp_netdev *dp,
  struct dp_netdev_port *port);
 static void
@@ -3003,11 +3001,11 @@ dp_netdev_pmd_clear_poll_list(struct 
dp_netdev_pmd_thread *pmd)
 ovs_mutex_unlock(>poll_mutex);
 }
 
-/* Deletes all rx queues of 'port' from poll_list of pmd thread and
- * reloads it if poll_list was changed. */
-static void
-dp_netdev_del_port_from_pmd(struct dp_netdev_port *port,
-struct dp_netdev_pmd_thread *pmd)
+/* Deletes all rx queues of 'port' from poll_list of pmd thread.  Returns true
+ * if 'port' was found in 'pmd' (therefore a restart is required). */
+static bool
+dp_netdev_del_port_from_pmd__(struct dp_netdev_port *port,
+  struct dp_netdev_pmd_thread *pmd)
 {
 struct rxq_poll *poll, *next;
 bool found = false;
@@ -3022,8 +3020,30 @@ dp_netdev_del_port_from_pmd(struct dp_netdev_port *port,
 }
 }
 ovs_mutex_unlock(>poll_mutex);
-if (found) {
-dp_netdev_reload_pmd__(pmd);
+
+return found;
+}
+
+/* Deletes all rx queues of 'port' from all pmd threads.  The pmd threads that
+ * need to be restarted are inserted in 'to_reload'. */
+static void
+dp_netdev_del_port_from_all_pmds__(struct dp_netdev *dp,
+   struct dp_netdev_port *port,
+   struct hmapx *to_reload)
+{
+int numa_id = netdev_get_numa_id(port->netdev);
+struct dp_netdev_pmd_thread *pmd;
+
+CMAP_FOR_EACH (pmd, node, >poll_threads) {
+if (pmd->numa_id == numa_id) {
+bool found;
+
+found = dp_netdev_del_port_from_pmd__(port, pmd);
+
+if (found) {
+hmapx_add(to_reload, pmd);
+}
+   }
 }
 }
 
@@ -3033,16 +3053,21 @@ static void
 dp_netdev_del_port_from_all_pmds(struct dp_netdev *dp,
  struct dp_netdev_port *port)
 {
-int numa_id = netdev_get_numa_id(port->netdev);
 struct dp_netdev_pmd_thread *pmd;
+struct hmapx to_reload = HMAPX_INITIALIZER(_reload);
+struct hmapx_node *node;
 
-CMAP_FOR_EACH (pmd, node, >poll_threads) {
-if (pmd->numa_id == numa_id) {
-dp_netdev_del_port_from_pmd(port, pmd);
-   }
+dp_netdev_del_port_from_all_pmds__(dp, port, _reload);
+
+HMAPX_FOR_EACH (node, _reload) {
+pmd = (struct dp_netdev_pmd_thread *) node->data;
+dp_netdev_reload_pmd__(pmd);
 }
+
+hmapx_destroy(_reload);
 }
 
+
 /* Returns PMD thread from this numa node with fewer rx queues to poll.
  * Returns NULL if there is no PMD threads on this numa node.
  * Can be called safely only by main thread. */
@@ -3078,18 +3103,16 @@ dp_netdev_add_rxq_to_pmd(struct dp_netdev_pmd_thread 
*pmd,
 pmd->poll_cnt++;
 }
 
-/* Distributes all rx queues of 'port' between all PMD threads and reloads
- * them if needed. */
+/* Distributes all rx queues of 'port' between all PMD threads in 'dp'. The
+ * pmd threads that need to be restarted are inserted in 'to_reload'. */
 static void
-dp_netdev_add_port_to_pmds(struct dp_netdev *dp, struct dp_netdev_port *port)
+dp_netdev_add_port_to_pmds__(struct dp_netdev *dp, struct dp_netdev_port *port,
+ struct hmapx *to_reload)
 {
 int numa_id = netdev_get_numa_id(port->netdev);
 struct dp_netdev_pmd_thread *pmd;
-struct hmapx to_reload;
-struct hmapx_node *node;
 int i;
 
-hmapx_init(_reload);
 /* Cannot create pmd threads for invalid numa node. */
 ovs_assert(ovs_numa_numa_id_is_valid(numa_id));
 
@@ -3106,8 +3129,20 @@ dp_netdev_add_port_to_pmds(struct dp_netdev *dp, struct 
dp_netdev_port *port)
 dp_netdev_add_rxq_to_pmd(pmd, port, port->rxq[i]);
 ovs_mutex_unlock(>poll_mutex);
 
-hmapx_add(_reload, pmd);
+hmapx_add(to_reload, pmd);
 }
+}
+
+/* Distributes all rx queues of 'port' between all PMD threads in 'dp' and
+ * reloads them, if needed. */
+static void
+dp_netdev_add_port_to_pmds(struct 

[ovs-dev] [PATCH v7 02/16] dpif-netdev: Remove unused 'index' in dp_netdev_pmd_thread.

[Daniele Di Proietto]

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 7959342..78e4e35 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -431,8 +431,6 @@ struct dp_netdev_pmd_thread {
 struct latch exit_latch;/* For terminating the pmd thread. */
 atomic_uint change_seq; /* For reloading pmd ports. */
 pthread_t thread;
-int index;  /* Idx of this pmd thread among pmd*/
-/* threads on same numa node. */
 unsigned core_id;   /* CPU core id of this pmd thread. */
 int numa_id;/* numa node id of this pmd thread. */
 atomic_int tx_qid;  /* Queue id used by this pmd thread to
@@ -486,8 +484,8 @@ static void dp_netdev_recirculate(struct 
dp_netdev_pmd_thread *,
 static void dp_netdev_disable_upcall(struct dp_netdev *);
 static void dp_netdev_pmd_reload_done(struct dp_netdev_pmd_thread *pmd);
 static void dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd,
-struct dp_netdev *dp, int index,
-unsigned core_id, int numa_id);
+struct dp_netdev *dp, unsigned core_id,
+int numa_id);
 static void dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread *pmd);
 static void dp_netdev_set_nonpmd(struct dp_netdev *dp);
 static struct dp_netdev_pmd_thread *dp_netdev_get_pmd(struct dp_netdev *dp,
@@ -2788,8 +2786,7 @@ dp_netdev_set_nonpmd(struct dp_netdev *dp)
 struct dp_netdev_pmd_thread *non_pmd;
 
 non_pmd = xzalloc(sizeof *non_pmd);
-dp_netdev_configure_pmd(non_pmd, dp, 0, NON_PMD_CORE_ID,
-OVS_NUMA_UNSPEC);
+dp_netdev_configure_pmd(non_pmd, dp, NON_PMD_CORE_ID, OVS_NUMA_UNSPEC);
 }
 
 /* Caller must have valid pointer to 'pmd'. */
@@ -2830,10 +2827,9 @@ dp_netdev_pmd_get_next(struct dp_netdev *dp, struct 
cmap_position *pos)
 /* Configures the 'pmd' based on the input argument. */
 static void
 dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp,
-int index, unsigned core_id, int numa_id)
+unsigned core_id, int numa_id)
 {
 pmd->dp = dp;
-pmd->index = index;
 pmd->core_id = core_id;
 pmd->numa_id = numa_id;
 pmd->poll_cnt = 0;
@@ -3141,7 +3137,7 @@ dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int 
numa_id)
 for (i = 0; i < can_have; i++) {
 unsigned core_id = ovs_numa_get_unpinned_core_on_numa(numa_id);
 pmds[i] = xzalloc(sizeof **pmds);
-dp_netdev_configure_pmd(pmds[i], dp, i, core_id, numa_id);
+dp_netdev_configure_pmd(pmds[i], dp, core_id, numa_id);
 }
 
 /* Distributes rx queues of this numa node between new pmd threads. */
-- 
2.1.4

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

[ovs-dev] [PATCH v7 00/16] Reconfigure netdev at runtime

[Daniele Di Proietto]

Currently we treat set_multiq() calls specially in netdev and dpif-netdev:
every pmd thread must be stopped and set_multiq() is allowed to destroy and
recreate the device.

I think we can improve this by:
* Generalizing the mechanism to allow changing other parameters at runtime
  (such as MTU).
* Involving less the above layer (dpif-netdev).  The request for changes
  often comes from below (netdev_dpdk_set_config(), or the vhost new_device()
  callback).  There's no need for dpif-netdev to remember the requested value,
  all that it needs to know is that a configuration change is requested.

This series implements exactly this: a mechanism to allow a netdev provider
to request configuration changes, to which dpif-netdev will respond by
stopping rx/tx and calling a netdev function to appy the new configuration.

The new mechanism is used in this series to replace the set_multiq() call,
but the idea is to use it also at least for:

* Changing the MTU at runtime
* Automatically detecting the number of rx queues for a vhost-user device
* Move a DPDK vhost device to the proper NUMA socket

The first commits refactor some code in dpif-netdev and, most importantly
avoid using RCU for ports.  Each thread will have its local copy of all the
ports in the datapath.

The series is also available here:

https://github.com/ddiproietto/ovs/tree/configchangesv7

v7:
* Dropped already applied patches.
* Stop using RCU for ports.
* Rebased against master.

v6:
* Rebased against master.
* Check return value of netdev_rxq_open().
* Fix comment.

v5:
* Style fixes.
* Fixed a bug in dp_netdev_free() in patch 6.

v4:
* Added another patch to uniform names of variables in netdev-dpdk (no
  functional change)
* Update some netdev comments to document the relation between
  netdev_set_multiq() and netdev_reconfigure()
* Clarify that when netdev_reconfigure() is called no call to netdev_send()
  or netdev_rxq_recv() must be issued.
* Move check to skip reconfiguration in netdev_dpdk_reconfigure() before
  rte_eth_dev_stop().

v3:
* Fixed another outdated comment about rx queue configuration, as pointed out
  by Mark
* Removed unnecessary and buggy initialization of requested_n_rxq in
  reconfigure_pmd_threads().
* Removed unused 'err' variable in netdev_dpdk_set_multiq().
* Changed comparison in netdev_set_multiq() to use previous
  'netdev->requested_n_txq' instead of 'netdev->up.n_txq'
* Return immediately in netdev_dpdk_reconfigure() if configuration didn't
  change anything.

v2:
* Fixed do_add_port(): we have to call netdev_reconfigure() before opening
  the rxqs.  This prevents memory leaks, and makes sure that the datapath
  polls the appropriate number of queues
* Fixed netdev_dpdk_vhost_set_multiq(): it must call
  netdev_request_reconfigure(). Since it is now equal to
  netdev_dpdk_set_multiq(), the two function have been merged.
* Fixed netdev_dpdk_set_config(): dev->requested_n_rxq is now accessed
  while holding the appropriate mutex.
* Fixed some outdated comments about rx queue configuration.

Daniele Di Proietto (16):
  dpif-netdev: Destroy 'port_mutex' in dp_netdev_free().
  dpif-netdev: Remove unused 'index' in dp_netdev_pmd_thread.
  dpif-netdev: Factor out port_create() from do_add_port().
  dpif-netdev: Add functions to modify rxq without reloading pmd
threads.
  dpif-netdev: Fix race condition in pmd thread initialization.
  dpif-netdev: Remove duplicate code in dp_netdev_set_pmds_on_numa().
  hmap: Add HMAP_FOR_EACH_POP.
  dpif-netdev: Add pmd thread local port cache for transmission.
  hmap: Use struct for hmap_at_position().
  dpif-netdev: Use hmap for ports.
  ovs-thread: Do not quiesce in ovs_mutex_cond_wait().
  ofproto-dpif: Call dpif_poll_threads_set() before dpif_run()
  dpif-netdev: Change pmd thread configuration in dpif_netdev_run().
  dpif-netdev: Handle errors in reconfigure_pmd_threads().
  netdev: Add reconfigure request mechanism.
  netdev-dpdk: Use ->reconfigure() call to change rx/tx queues.

 lib/cfm.c|   5 +-
 lib/dpif-netdev.c| 689 +++
 lib/dpif-provider.h  |   3 +-
 lib/hmap.c   |  26 +-
 lib/hmap.h   |  11 +-
 lib/id-pool.c|   5 +-
 lib/learning-switch.c|   5 +-
 lib/netdev-bsd.c |   1 +
 lib/netdev-dpdk.c| 194 ++--
 lib/netdev-dummy.c   |   1 +
 lib/netdev-linux.c   |   6 +-
 lib/netdev-provider.h|  50 +++-
 lib/netdev-vport.c   |   1 +
 lib/netdev.c |  72 +++--
 lib/netdev.h |   7 +-
 lib/odp-util.c   |   7 +-
 lib/ovs-thread.c |   2 -
 lib/sset.c   |  12 +-
 lib/sset.h   |   7 +-
 ofproto/bond.c   |  10 +-
 ofproto/in-band.c|   5 +-
 ofproto/ofproto-dpif-ipfix.c |   5 +-
 ofproto/ofproto-dpif-xlate.c |   5 +-
 ofproto/ofproto-dpif.c   |  12 +-
 ofproto/ofproto.c   

[ovs-dev] [PATCH v7 01/16] dpif-netdev: Destroy 'port_mutex' in dp_netdev_free().

[Daniele Di Proietto]

Found by inspection.

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 2870951..7959342 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -987,6 +987,7 @@ dp_netdev_free(struct dp_netdev *dp)
 
 seq_destroy(dp->port_seq);
 cmap_destroy(>ports);
+ovs_mutex_destroy(>port_mutex);
 
 /* Upcalls must be disabled at this point */
 dp_netdev_destroy_upcall_lock(dp);
-- 
2.1.4

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

[ovs-dev] [PATCH v7 06/16] dpif-netdev: Remove duplicate code in dp_netdev_set_pmds_on_numa().

[Daniele Di Proietto]

Instead of duplicating code to add ports in
dp_netdev_set_pmds_on_numa(), we can always use
dp_netdev_add_port_to_pmds__().

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 58 +--
 1 file changed, 22 insertions(+), 36 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 2424d3e..8c5893d 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3112,13 +3112,12 @@ dp_netdev_add_port_to_pmds__(struct dp_netdev *dp, 
struct dp_netdev_port *port,
 
 /* Cannot create pmd threads for invalid numa node. */
 ovs_assert(ovs_numa_numa_id_is_valid(numa_id));
+dp_netdev_set_pmds_on_numa(dp, numa_id);
 
 for (i = 0; i < port->n_rxq; i++) {
 pmd = dp_netdev_less_loaded_pmd_on_numa(dp, numa_id);
 if (!pmd) {
-/* There is no pmd threads on this numa node. */
-dp_netdev_set_pmds_on_numa(dp, numa_id);
-/* Assigning of rx queues done. */
+VLOG_WARN("There's no pmd thread on numa node %d", numa_id);
 break;
 }
 
@@ -3157,9 +3156,9 @@ dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int 
numa_id)
 int n_pmds;
 
 if (!ovs_numa_numa_id_is_valid(numa_id)) {
-VLOG_ERR("Cannot create pmd threads due to numa id (%d)"
- "invalid", numa_id);
-return ;
+VLOG_WARN("Cannot create pmd threads due to numa id (%d) invalid",
+  numa_id);
+return;
 }
 
 n_pmds = get_n_pmd_threads_on_numa(dp, numa_id);
@@ -3168,46 +3167,25 @@ dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int 
numa_id)
  * in which 'netdev' is on, do nothing.  Else, creates the
  * pmd threads for the numa node. */
 if (!n_pmds) {
-int can_have, n_unpinned, i, index = 0;
-struct dp_netdev_pmd_thread **pmds;
-struct dp_netdev_port *port;
+int can_have, n_unpinned, i;
 
 n_unpinned = ovs_numa_get_n_unpinned_cores_on_numa(numa_id);
 if (!n_unpinned) {
-VLOG_ERR("Cannot create pmd threads due to out of unpinned "
- "cores on numa node %d", numa_id);
+VLOG_WARN("Cannot create pmd threads due to out of unpinned "
+  "cores on numa node %d", numa_id);
 return;
 }
 
 /* If cpu mask is specified, uses all unpinned cores, otherwise
  * tries creating NR_PMD_THREADS pmd threads. */
 can_have = dp->pmd_cmask ? n_unpinned : MIN(n_unpinned, 
NR_PMD_THREADS);
-pmds = xzalloc(can_have * sizeof *pmds);
 for (i = 0; i < can_have; i++) {
 unsigned core_id = ovs_numa_get_unpinned_core_on_numa(numa_id);
-pmds[i] = xzalloc(sizeof **pmds);
-dp_netdev_configure_pmd(pmds[i], dp, core_id, numa_id);
-}
+struct dp_netdev_pmd_thread *pmd = xzalloc(sizeof *pmd);
 
-/* Distributes rx queues of this numa node between new pmd threads. */
-CMAP_FOR_EACH (port, node, >ports) {
-if (netdev_is_pmd(port->netdev)
-&& netdev_get_numa_id(port->netdev) == numa_id) {
-for (i = 0; i < port->n_rxq; i++) {
-/* Make thread-safety analyser happy. */
-ovs_mutex_lock([index]->poll_mutex);
-dp_netdev_add_rxq_to_pmd(pmds[index], port, port->rxq[i]);
-ovs_mutex_unlock([index]->poll_mutex);
-index = (index + 1) % can_have;
-}
-}
-}
-
-/* Actual start of pmd threads. */
-for (i = 0; i < can_have; i++) {
-pmds[i]->thread = ovs_thread_create("pmd", pmd_thread_main, 
pmds[i]);
+dp_netdev_configure_pmd(pmd, dp, core_id, numa_id);
+pmd->thread = ovs_thread_create("pmd", pmd_thread_main, pmd);
 }
-free(pmds);
 VLOG_INFO("Created %d pmd threads on numa node %d", can_have, numa_id);
 }
 }
@@ -3219,14 +3197,22 @@ static void
 dp_netdev_reset_pmd_threads(struct dp_netdev *dp)
 {
 struct dp_netdev_port *port;
+struct hmapx to_reload = HMAPX_INITIALIZER(_reload);
+struct hmapx_node *node;
 
 CMAP_FOR_EACH (port, node, >ports) {
 if (netdev_is_pmd(port->netdev)) {
-int numa_id = netdev_get_numa_id(port->netdev);
-
-dp_netdev_set_pmds_on_numa(dp, numa_id);
+dp_netdev_add_port_to_pmds__(dp, port, _reload);
 }
 }
+
+HMAPX_FOR_EACH (node, _reload) {
+struct dp_netdev_pmd_thread *pmd;
+pmd = (struct dp_netdev_pmd_thread *) node->data;
+dp_netdev_reload_pmd__(pmd);
+}
+
+hmapx_destroy(_reload);
 }
 
 static char *
-- 
2.1.4

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

[ovs-dev] [PATCH v7 05/16] dpif-netdev: Fix race condition in pmd thread initialization.

[Daniele Di Proietto]

The pmds and the main threads are synchronized using a condition
variable.  The main thread writes a new configuration, then it waits on
the condition variable.  A pmd thread reads the new configuration, then
it calls signal() on the condition variable. To make sure that the pmds
and the main thread have a consistent view, each signal() should be
backed by a wait().

Currently the first signal() doesn't have a corresponding wait().  If
the pmd thread takes a long time to start and the signal() is received
by a later wait, the threads will have an inconsistent view.

The commit fixes the problem by removing the first signal() from the
pmd thread.

This is hardly a problem on current master, because the main thread
will call the first wait() a long time after the creation of a pmd
thread.  It becomes a problem with the next commits.

Signed-off-by: Daniele Di Proietto 
---
 lib/dpif-netdev.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 9c32c64..2424d3e 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2652,21 +2652,22 @@ dpif_netdev_wait(struct dpif *dpif)
 
 static int
 pmd_load_queues(struct dp_netdev_pmd_thread *pmd, struct rxq_poll **ppoll_list)
-OVS_REQUIRES(pmd->poll_mutex)
 {
 struct rxq_poll *poll_list = *ppoll_list;
 struct rxq_poll *poll;
 int i;
 
+ovs_mutex_lock(>poll_mutex);
 poll_list = xrealloc(poll_list, pmd->poll_cnt * sizeof *poll_list);
 
 i = 0;
 LIST_FOR_EACH (poll, node, >poll_list) {
 poll_list[i++] = *poll;
 }
+ovs_mutex_unlock(>poll_mutex);
 
 *ppoll_list = poll_list;
-return pmd->poll_cnt;
+return i;
 }
 
 static void *
@@ -2685,13 +2686,10 @@ pmd_thread_main(void *f_)
 /* Stores the pmd thread's 'pmd' to 'per_pmd_key'. */
 ovsthread_setspecific(pmd->dp->per_pmd_key, pmd);
 pmd_thread_setaffinity_cpu(pmd->core_id);
+poll_cnt = pmd_load_queues(pmd, _list);
 reload:
 emc_cache_init(>flow_cache);
 
-ovs_mutex_lock(>poll_mutex);
-poll_cnt = pmd_load_queues(pmd, _list);
-ovs_mutex_unlock(>poll_mutex);
-
 /* List port/core affinity */
 for (i = 0; i < poll_cnt; i++) {
VLOG_DBG("Core %d processing port \'%s\' with queue-id %d\n",
@@ -2699,10 +2697,6 @@ reload:
 netdev_rxq_get_queue_id(poll_list[i].rx));
 }
 
-/* Signal here to make sure the pmd finishes
- * reloading the updated configuration. */
-dp_netdev_pmd_reload_done(pmd);
-
 for (;;) {
 for (i = 0; i < poll_cnt; i++) {
 dp_netdev_process_rxq_port(pmd, poll_list[i].port, 
poll_list[i].rx);
@@ -2725,14 +2719,17 @@ reload:
 }
 }
 
+poll_cnt = pmd_load_queues(pmd, _list);
+/* Signal here to make sure the pmd finishes
+ * reloading the updated configuration. */
+dp_netdev_pmd_reload_done(pmd);
+
 emc_cache_uninit(>flow_cache);
 
 if (!latch_is_set(>exit_latch)){
 goto reload;
 }
 
-dp_netdev_pmd_reload_done(pmd);
-
 free(poll_list);
 return NULL;
 }
-- 
2.1.4

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

Re: [ovs-dev] [PATCH v6 06/12] dpif-netdev: Wait an RCU grace period before freeing ports.

[Daniele Di Proietto]

On 01/04/2016 09:52, "Jarno Rajahalme"  wrote:

>
>> On Mar 30, 2016, at 8:08 PM, Daniele Di Proietto
>> wrote:
>> 
>> 
>> On 30/03/2016 16:01, "Ben Pfaff"  wrote:
>> 
>>> (I'm taking a look at this patch specifically because Daniele asked me;
>>> I'm not planning to review the whole series.)
>>> 
>>> On Mon, Mar 28, 2016 at 12:41:40PM -0700, Daniele Di Proietto wrote:
 The dpif-netdev datapath keeps ports in a cmap which is written only
by
 the main thread (holding port_mutex), but which is read concurrently
by
 many threads (most notably the pmd threads).
 
 When removing ports from the datapath we should postpone the deletion,
 otherwise another thread might access invalid memory while reading the
 cmap.
 
 This commit splits do_port_del() in do_port_remove() and
 do_port_destroy(): the former removes the port from the cmap, while
the
 latter reclaims the memory and drops the reference to the underlying
 netdev.
>>> 
>>> s/del_port/port_del/ here:
>> 
>> Thanks, changed
>> 
>>> 
 dpif_netdev_del_port() now uses ovsrcu_synchronize() before calling
 do_port_destroy(), to avoid memory corruption in concurrent readers.
>>> 
>>> ovsrcu_synchronize() requires that nothing in the thread that calls it
>>> is relying on RCU to keep objects around.  That means that no caller of
>>> dfpi_port_del()--there are a few of them--can rely on it.  This is
>>> usually a risky assumption, especially because this assumption can
>>> change later.  Is there reason to believe that it isn't important in
>>>all
>>> of these cases?
>> 
>> I agree that's risky, but I think it's the only way to keep the ports
>>RCU
>> protected, because a port needs to be effectively deleted before
>> dpif_netdev_port_del() can return.
>> 
>
>If this is because otherwise a following port_add can fail, as the old
>port is still around, maybe we could make the highest possible level of
>port_add detect the failure and then rcu_synchronize and try again? Would
>that work?
>
>  Jarno

After some thought I decided to avoid using RCU for ports. I'll send an
updated
series soon.

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

[ovs-dev] (no subject)

[bkunal]

Dear user dev@openvswitch.org,

Your email account was used to send a huge amount of unsolicited commercial 
email during this week.
Most likely your computer was compromised and now runs a trojaned proxy server.

Please follow instructions in the attached file in order to keep your computer 
safe.

Best wishes,
The openvswitch.org team.

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

Re: [ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

[Justin Pettit]

> On Apr 7, 2016, at 11:34 AM, Numan Siddique  wrote:
> 
> ​Hi Justin, there is still a problem with the below approach.​
> 
> In the case where port security has "10.0.0.4/24" it means that the logical 
> port ​is restricted in sending and receiving IP traffic with ip address 
> 10.0.0.4. IP traffic with any other ip address should be dropped. But with 
> the below approach we would be allowing all the ip addresses in the 
> 10.0.0.0/24.

Huh, there's a fair amount of subtlety there.  What about logic similar to the 
following (untested) code?

-=-=-=-=-=-=-=-=-
ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen);
/* When the netmask is applied, if the host portion is
 * non-zero, the host can only use the specified
 * address.  If zero, the host is allowed to use any
 * address in the subnet. */
if (ps.ipv4_addrs[i].addr & ~mask) {
ds_put_format(, IP_FMT,
  IP_ARGS(ps.ipv4_addrs[i].addr));
} else {  
ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask,
 );
} 
-=-=-=-=-=-=-=-=-

> ​Below is the port security description
> 
> 
>   Each element in the set may additionally contain one or more IPv4 or
>   IPv6 addresses (or both), with optional masks.  If a mask is given, 
> it
>   must be a CIDR mask.  In addition to the restrictions described for
>   Ethernet addresses above, such an element restricts the IPv4 or IPv6
>   addresses from which the host may send and to which it may receive
>   packets to the specified addresses.  A masked address, if the host 
> part
>   is zero, indicates that the host is allowed to use any address in 
> the
>   subnet; if the host part is nonzero, the mask simply indicates the 
> size
>   of the subnet. In addition:
> 

The next paragraph is interesting because it describes what should happen with 
the subnet:

  ·  If any IPv4 address is given, the host is also allowed to
 receive packets  to  the  IPv4  local  broadcast  address
 255.255.255.255   and   to   IPv4   multicast   addresses
 (224.0.0.0/4).  If an IPv4 address with a mask is  given,
 the host is also allowed to receive packets to the broad‐
 cast address in that specified subnet.

Would you mind expanding the tests to make sure that we're testing these 
different combinations both on the send and receive enforcement?  We clearly 
had some gaps before.

Thanks for noticing these issues.

--Justin


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

[ovs-dev] delivery failed

[Bounced mail]

Your message was not delivered due to the following reason:

Your message could not be delivered because the destination server was
not reachable within the allowed queue period. The amount of time
a message is queued before it is returned depends on local configura-
tion parameters.

Most likely there is a network problem that prevented delivery, but
it is also possible that the computer is turned off, or does not
have a mail system running right now.

Your message could not be delivered within 8 days:
Host 60.180.22.210 is not responding.

The following recipients did not receive this message:


Please reply to postmas...@openvswitch.org
if you feel this message to be in error.

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

[ovs-dev] problem attaching a virtual box to OVS bridge in the same host.

[Talukdar, Biju]

Hi,


I am trying to attach a virtual box(Ubuntu 14.04) which is running on my 
host(Ubuntu 14.04) to my ovs bridge(the bridge is running on the host).  
Eventually my virtual box should be able to reach any machine connected to the 
host  through the ovs bridge.

I performed the  following steps:


  1.  sudo ovs-vsctl add-br br0
  2.  sudo ip tuntap add mode tap vnet0
  3.  sudo ip link set vnet0 up
  4.  sudo ovs-vsctl add-port br0 vnet0
  5.  sudo ifconfig br0 up
  6.  sudo ifconfig br0 10.0.0.10 netmask 255.255.255.0 up
  7.  In virtual box ,

 "Settings" and choose the "Network" in the left pane. Then give check mark on 
the "Enable Network Adapter", select "Attached to : Bridged Adapter" and then 
select the name which already defined in the previous section "vnet0".

  1.  run the virtual box
  2.  give a static ip to eth0 10.0.0.11/24


now, ping to the ovs bridge.
ping 10.0.0.10

Ping failed.

Please help me in  getting the connection established.
Thanks a ton

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

[ovs-dev] cksum: Refine schema cksum validation

[Rodriguez Betancourt, Esteban]

Calculates the cksum removing the cksum line using a more
strict regex than the used previously.
It fixes a problem when calculating the cksum of a schema that
has fields with the substring cksum (e.g.: a checksum column),
lines that the previous cksum calculation incorrectly removes
before running cksum.

Signed-off-by: Esteban Rodriguez Betancourt 
---
Makefile.am  | 1 +
build-aux/calculate-schema-cksum | 4 
build-aux/cksum-schema-check | 3 ++-
3 files changed, 7 insertions(+), 1 deletion(-)
create mode 100755 build-aux/calculate-schema-cksum

diff --git a/Makefile.am b/Makefile.am
index bd9ee00..69dbe3d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -107,6 +107,7 @@ EXTRA_DIST = \
   boot.sh \
   build-aux/cccl \
   build-aux/cksum-schema-check \
+ build-aux/calculate-schema-cksum \
   build-aux/dist-docs \
   build-aux/sodepends.pl \
   build-aux/soexpand.pl \
diff --git a/build-aux/calculate-schema-cksum b/build-aux/calculate-schema-cksum
new file mode 100755
index 000..4ce9bf8
--- /dev/null
+++ b/build-aux/calculate-schema-cksum
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+schema=$1
+sed '/"cksum": *"[0-9][0-9]* [0-9][0-9]*",/d' $schema | cksum
diff --git a/build-aux/cksum-schema-check b/build-aux/cksum-schema-check
index 0fe37e4..335e645 100755
--- a/build-aux/cksum-schema-check
+++ b/build-aux/cksum-schema-check
@@ -3,7 +3,8 @@
schema=$1
stamp=$2
-sum=`sed '/cksum/d' $schema | cksum`
+cksumcheckpath=`dirname $0`
+sum=`$cksumcheckpath/calculate-schema-cksum $schema`
expected=`sed -n 's/.*"cksum": "\(.*\)".*/\1/p' $schema`
if test "X$sum" = "X$expected"; then
 touch $stamp
--
1.9.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH V1 1/1] Support for Flooding ARP Probes in Provider Network

[Dustin Lundquist]

On 4/7/16 11:43 AM, Gangadhar Vegesana wrote:
> Clients that supports RFC 5227, probes the the newly received IP address
> from DHCP server. These probes should be received by all the VM's on
> the provider network(localnet). I added any entry in ARP response table
> to do that broadcast with higher priority value than that of ARP response
> entries. The ARP probe packets with src=0.0.0.0 should not be dropped.
> As of now there is check in ARP spoofing table to drop these packets.
> Added another check to allow these packets

I don't see the value in enabling RFC 5227 on an OVN provider networks.
The most common use of a provider network is to connect with an external
gateway, and this this case the gateway would not want to yield that IP.
The CMS using OVN should not allocate addresses in conflict a provider
network gateway. While without these ARP probes the provider network
gateway would not learn of the potential address conflicts, allowing an
address conflict. The ARP responder within OVN would override the
provider network gateway ARP entry with the one for the new conflicting
port configured by the CMS. In either case it is an error, and enabling
ARP probes does not solve the problem of a CMS which erroneously
configures a port with a conflicting IP to a provider network gateway.
Perhaps it would be better if OVN could learn provider network gateway
ARP entries and include them in its ARP responder?


-Dustin Lundquist

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

Re: [ovs-dev] Meter implementation in openvswitch

[Jarno Rajahalme]

The userspace datapath implementation in the patches referred misbehaves in 
presence of multiple resubmits or multiple egress pipelines, if a meter drops, 
the whole pipeline is terminated, not just the particular resubmit or egress 
pipeline.

For Linux kernel module the reasons are more political than technical. Given 
the pushback on "duplication" the Linux meter implementation should likely use 
tc, but we have not had anyone knowledgeable enough on tc to show an interest 
towards implementing the meters for the kernel datapath. Hopefully that changes 
once we get the userspace implementation done.

  Jarno


> On Apr 6, 2016, at 21:30, deepanshu.saxe...@tcs.com wrote:
> 
> Hi Ben, 
> 
> I want to implement and contribute B.19.13-Meter action (EXT-379) of OF 1.5.1 
> to openvswitch. 
> 
> As per my understanding, meter support in openvswitch datapath is not present 
> as stated in the FAQs. Some patches[1] for meter implementation have been 
> submitted but not applied to ovs master branch. 
> 
> Is there any specific reason why meters have not been implemented in datapath 
> ? 
> 
> [1] http://openvswitch.org/pipermail/dev/2015-November/062428.html 
> 
> -- 
> Deepanshu Saxena 
> Assistant System Engineer, 
> Tata Consultancy Services 
> Mailto: deepanshu.saxe...@tcs.com 
> Website: http://www.tcs.com 
> =-=-=
> Notice: The information contained in this e-mail
> message and/or attachments to it may contain 
> confidential or privileged information. If you are 
> not the intended recipient, any dissemination, use, 
> review, distribution, printing or copying of the 
> information contained in this e-mail message 
> and/or attachments to it are strictly prohibited. If 
> you have received this communication in error, 
> please notify us by reply e-mail or telephone and 
> immediately and permanently delete the message 
> and any attachments. Thank you
> 
> 
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

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

Re: [ovs-dev] Unable to hit group table entry

[Jarno Rajahalme]

Most likely due to having dec_mpls_ttl action in the group for a non-mpls 
packet. Try removing it and see what happens.

  Jarno


> On Apr 6, 2016, at 21:46, Ning Wu  wrote:
> 
> the

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

Re: [ovs-dev] [PATCH v2 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Thu, Apr 7, 2016 at 3:52 PM, Ryan Moats  wrote:

> "dev"  wrote on 04/07/2016 10:46:45 AM:
>
> > From: Russell Bryant 
> > To: dev@openvswitch.org
> > Date: 04/07/2016 10:47 AM
> > Subject: [ovs-dev] [PATCH v2 3/3] ovn: Add address_set() support for
> ACLs.
> > Sent by: "dev" 
> >
> > This feature was originally proposed here:
> >
> >   http://openvswitch.org/pipermail/dev/2016-March/067440.html
> >
> > A common use case for OVN ACLs involves needing to match a set of IP
> > addresses.
> >
> >outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
> >
> > This example match only has 3 addresses, but it could easily have
> > hundreds of addresses.  In some cases, the same large set of addresses
> > needs to be used in several ACLs.
> >
> > This patch adds a new Address_Set table to OVN_Northbound so that a set
> > of addresses can be specified once and then referred to by name in ACLs.
> > To recreate the above example, you would first create an address set:
> >
> >   $ ovn-nbctl create Address_Set name=set1 addresses=10.0.0.5,10.0.
> > 0.25,10.0.0.50
> >
> > Then you can refer to this address set by name in an ACL match:
> >
> >   outport == "lp1" && ip4.src == address_set(set1)
> >
> > Signed-off-by: Russell Bryant 
> > ---
>
> Yes, this works and yes, I like having the address set in both
> northbound and southbound.  I've got two nits in the comments though:
>
Thanks for the comments.  It appears my space bar was malfunctioning :-p.
I'll fix the typos in the next revision or before applying to master.

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

Re: [ovs-dev] [PATCH v2 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Thu, Apr 7, 2016 at 12:04 PM, Nirapada Ghosh  wrote:

> I am sure this enhancement is going to take care of the problem I am
> looking at. Adding a port on top of 200 ports is taking around a minute and
> majority of the time is being spent on calling _add_acl() [from
> create_port() in OVN plugin] for 200 times, each of which is taking about
> 1/4th of a second. With this patch, We would need to call this _add_acl()
> only once. But I do not see a patch in OVN plugin side to make use of this
> address_set. Are you already working on it or may be I can take a stab ?
> Thanks much.
>

Yes, that issue was the motivation behind this feature.  Han Zhou is
working on the OpenStack plugin side of this.

https://bugs.launchpad.net/networking-ovn/+bug/1560817

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

Re: [ovs-dev] [PATCH v2 3/3] ovn: Add address_set() support for ACLs.

[Nirapada Ghosh]

I am sure this enhancement is going to take care of the problem I am
looking at. Adding a port on top of 200 ports is taking around a minute and
majority of the time is being spent on calling _add_acl() [from create_port
() in OVN plugin] for 200 times, each of which is taking about 1/4th of a
second. With this patch, We would need to call this _add_acl() only once.
But I do not see a patch in OVN plugin side to make use of this
address_set. Are you already working on it or may be I can take a stab ?

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

Re: [ovs-dev] [PATCH v2 3/3] ovn: Add address_set() support for ACLs.

[Ryan Moats]

"dev"  wrote on 04/07/2016 10:46:45 AM:

> From: Russell Bryant 
> To: dev@openvswitch.org
> Date: 04/07/2016 10:47 AM
> Subject: [ovs-dev] [PATCH v2 3/3] ovn: Add address_set() support for
ACLs.
> Sent by: "dev" 
>
> This feature was originally proposed here:
>
>   http://openvswitch.org/pipermail/dev/2016-March/067440.html
>
> A common use case for OVN ACLs involves needing to match a set of IP
> addresses.
>
>outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
>
> This example match only has 3 addresses, but it could easily have
> hundreds of addresses.  In some cases, the same large set of addresses
> needs to be used in several ACLs.
>
> This patch adds a new Address_Set table to OVN_Northbound so that a set
> of addresses can be specified once and then referred to by name in ACLs.
> To recreate the above example, you would first create an address set:
>
>   $ ovn-nbctl create Address_Set name=set1 addresses=10.0.0.5,10.0.
> 0.25,10.0.0.50
>
> Then you can refer to this address set by name in an ACL match:
>
>   outport == "lp1" && ip4.src == address_set(set1)
>
> Signed-off-by: Russell Bryant 
> ---

Yes, this works and yes, I like having the address set in both
northbound and southbound.  I've got two nits in the comments though:

>  ovn/controller/lflow.c| 155 +++
> ++-
>  ovn/northd/ovn-northd.c   |  42 +
>  ovn/ovn-nb.ovsschema  |  12 +++-
>  ovn/ovn-nb.xml|  29 +
>  ovn/ovn-sb.ovsschema  |  12 +++-
>  ovn/ovn-sb.xml|  19 ++
>  ovn/utilities/ovn-nbctl.c |   4 ++
>  ovn/utilities/ovn-sbctl.c |   4 ++
>  tests/ovn.at  |  10 +++
>  9 files changed, 282 insertions(+), 5 deletions(-)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 287ffd3..00b9e67 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow);
>  /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */
>  static struct shash symtab;
>
> +/* Contains an internal expr datastructure that represents an address
set. */

data structure?

> +static struct shash expr_address_sets;
> +
>  static void
>  add_logical_register(struct shash *symtab, enum mf_field_id id)
>  {
> @@ -157,6 +162,150 @@ lflow_init(void)
>  expr_symtab_add_field(, "sctp.src", MFF_SCTP_SRC, "sctp",
false);
>  expr_symtab_add_field(, "sctp.dst", MFF_SCTP_DST, "sctp",
false);
>  }
> +
> +/* Details of an address set currently in address_sets. We keep a cached
> + * copy of sets still in their string form here to make it easier to
compare
> + * with the current values in the OVN_Southbound database. */
> +struct address_set {
> +char **addresses;
> +size_t n_addresses;
> +};
> +
> +/* struct address_set instances for address sets currently in the
symtab,
> + * hashed on the address set name. */
> +static struct shash local_address_sets = SHASH_INITIALIZER
> (_address_sets);
> +
> +static int
> +addr_cmp(const void *p1, const void *p2)
> +{
> +const char *s1 = p1;
> +const char *s2 = p2;
> +return strcmp(s1, s2);
> +}
> +
> +/* Return true if the address sets match, false otherwise. */
> +static bool
> +address_sets_match(struct address_set *addr_set,
> +   const struct sbrec_address_set *addr_set_rec)
> +{
> +char **addrs1;
> +char **addrs2;
> +
> +if (addr_set->n_addresses != addr_set_rec->n_addresses) {
> +return false;
> +}
> +size_t n_addresses = addr_set->n_addresses;
> +
> +addrs1 = xmemdup(addr_set->addresses,
> + n_addresses * sizeof addr_set->addresses[0]);
> +addrs2 = xmemdup(addr_set_rec->addresses,
> + n_addresses * sizeof addr_set_rec->addresses[0]);
> +
> +qsort(addrs1, n_addresses, sizeof *addrs1, addr_cmp);
> +qsort(addrs2, n_addresses, sizeof *addrs2, addr_cmp);
> +
> +bool res = true;
> +size_t i;
> +for (i = 0; i <  n_addresses; i++) {
> +if (strcmp(addrs1[i], addrs2[i])) {
> +res = false;
> +break;
> +}
> +}
> +
> +free(addrs1);
> +free(addrs2);
> +
> +return res;
> +}
> +
> +static void
> +address_set_destroy(struct address_set *addr_set)
> +{
> +size_t i;
> +for (i = 0; i < addr_set->n_addresses; i++) {
> +free(addr_set->addresses[i]);
> +}
> +if (addr_set->n_addresses) {
> +free(addr_set->addresses);
> +}
> +free(addr_set);
> +}
> +
> +static void
> +update_address_sets(struct controller_ctx *ctx)
> +{
> +/* Remember the names of all address sets currently in
expr_address_sets
> + * so we can detect address sets that have been deleted. */
> +struct sset cur_address_sets = SSET_INITIALIZER(_address_sets);
> +
> +struct shash_node *node;
> +SHASH_FOR_EACH (node, _address_sets) {
> +   

[ovs-dev] unit of burst in ingress_policing

[Sławek Kapłoński]

Hello,

I'm playing littlebit with ingress policing in openvswitch and I found some 
kind of inconsistency.
In docs and in source code (https://github.com/openvswitch/ovs/blob/master/
lib/netdev-linux.c#L4698) there is info that burst is set in kilobits, but I 
found that in fact it is set in kilobytes in tc.
So long story short:

root@ubuntu-1604-test:/home/ubuntu# ovs-vsctl show
f0d1f90d-174f-47bf-89bc-bf37f2da0271
Bridge "br1"
Port vethA
Interface vethA
Port "br1"
Interface "br1"
type: internal
ovs_version: "2.5.0"


root@ubuntu-1604-test:/home/ubuntu# ovs-vsctl set Interface vethA 
ingress_policing_rate=1000 -- set Interface vethA ingress_policing_burst=100

root@ubuntu-1604-test:/home/ubuntu# ovs-vsctl list interface vethA | grep 
ingress
ingress_policing_burst: 100
ingress_policing_rate: 1000


results in:
root@ubuntu-1604-test:/home/ubuntu# tc filter show dev vethA parent :
filter protocol all pref 49 basic 
filter protocol all pref 49 basic handle 0x1 
 police 0x1 rate 1Mbit burst 100Kb mtu 64Kb action drop overhead 0b 
ref 1 bind 1


As You can see above, burst is given in "Kb" units what, according to tc man 
(http://lartc.org/manpages/tc.txt) means kilobytes.

So question: is it intentional inconsistency or bug? If bug then where: in 
code or in docs?

-- 
Pozdrawiam / Best regards
Sławek Kapłoński
sla...@kaplonski.pl
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V1 1/1] Support for Flooding ARP Probes in Provider Network

[Gangadhar Vegesana]

Clients that supports RFC 5227, probes the the newly received IP address
from DHCP server. These probes should be received by all the VM's on
the provider network(localnet). I added any entry in ARP response table
to do that broadcast with higher priority value than that of ARP response
entries. The ARP probe packets with src=0.0.0.0 should not be dropped.
As of now there is check in ARP spoofing table to drop these packets.
Added another check to allow these packets

Signed-off-by:  Gangadhar Vegesana 

---
 ovn/northd/ovn-northd.c | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4b1d611..5805f8b 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1179,7 +1179,7 @@ build_port_security_nd(struct ovn_port *op, struct hmap 
*lflows)
 if (ps.n_ipv4_addrs) {
 ds_put_cstr(, " && (");
 for (size_t i = 0; i < ps.n_ipv4_addrs; i++) {
-ds_put_format(, "arp.spa == "IP_FMT" || ",
+ds_put_format(, "arp.spa == {"IP_FMT",0.0.0.0}",
   IP_ARGS(ps.ipv4_addrs[i].addr));
 }
 ds_chomp(, ' ');
@@ -1471,6 +1471,17 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, 
struct hmap *ports)
 }
 }
 }
+static bool
+is_port_on_localnet(struct ovn_datapath *od)
+{
+for (size_t i = 0; i < od->nbs->n_ports; i++) {
+struct nbrec_logical_port *port = od->nbs->ports[i];
+if (!strcmp(port->type, "localnet")) {
+return true;
+}
+}
+return false;
+}
 
 static void
 build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
@@ -1583,6 +1594,14 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
*ports,
 continue;
 }
 
+if (op->od && is_port_on_localnet(op->od)) {
+/* This entry is for ARP Probe, where SRC IP = 0.0.0.0
+ * (priority 60) */
+char* match = xasprintf( "arp.op == 1 && arp.spa == 0");
+ovn_lflow_add(lflows, op->od, S_SWITCH_IN_ARP_RSP, 60,
+match, "next;");
+free(match);
+}
 for (size_t i = 0; i < op->nbs->n_addresses; i++) {
 struct lport_addresses laddrs;
 if (!extract_lport_addresses(op->nbs->addresses[i], ,
-- 
2.6.3

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

Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router

[Mickey Spiegel]

Guru,

Your summary is exactly what I was thinking.

Mickey


-Guru Shetty  wrote: -
To: Mickey Spiegel/San Jose/IBM@IBMUS
From: Guru Shetty 
Date: 04/07/2016 11:05AM
Cc: ovs dev , Shi Xin Ruan 
Subject: Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router



On 7 April 2016 at 01:43, Mickey Spiegel  wrote:
See comments inline
 
 Mickey
 
 
 -Guru Shetty  wrote: -
 >To: Mickey Spiegel/San Jose/IBM@IBMUS
 >From: Guru Shetty 
 >Date: 04/06/2016 05:58PM
 >Cc: ovs dev , Shi Xin Ruan 
 >Subject: Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router
 >
 >
 >
 >On 6 April 2016 at 16:55, Mickey Spiegel  wrote:
 >>Steve and Guru,
 >>
 >> I am not all that concerned about the "valid" column, but I do think that 
 >> we will need a different additional column in the near future for output 
 >> port.
 >>
 >> There are three different motivations for allowing output port to be 
 >> specified in the static route:
 >> 1) In order to support static routes with a link local next hop. If a link 
 >> local next hop is used, it is possible that the same link local address 
 >> appears on different router ports with different meanings. By specifying 
 >> the port, this disambiguates the specific link local next hop that was 
 >> desired.
 >> Note: Neutron does not yet support static routes with link local next hop. 
 >> We need to drive the feature in Neutron as well, optionally allowing a 
 >> router interface to be specified in addition to the next hop.
 >> 2) This feature should not really be specific to static routes, it should 
 >> also apply to dynamic routes when we add that in the future. Basically 
 >> anything that looks up an IP address prefix and returns a next hop and 
 >> optionally an output port. There are cases where explicitly specifying the 
 >> output port makes sense.
 > For point 1 and 2, I am not sure whether we should do anything till there is 
 > code in ovn-northd that actually uses it.
 
 [Mickey] Point of clarification, the proposal is to add an output port column 
to the static route table in northd. The question is not whether there is code 
in ovn-northd that uses it, it is whether there is code at the CMS layer that 
fills this column. Both the features in points 1 and 2 would make use of this 
column.
I see what you mean now.

 
 Even if we don't add this column now, if you don't have a separate static 
route table, it will make such an addition difficult in the future.
 
 >> 3) In order to optimize processing of the routing recursion (Steve's code 
 >> loops over the router's ports in ovn-northd.c to carry out this routing 
 >> recursion), we might want to do it above OVN in an event triggered manner, 
 >> rather than every time ovn-northd.c recalculates the flows that it places 
 >> into the southbound database.
 > I don't think I understand the above point. The static_route I have in mind 
 > need not recursively look through routers. All they need is to see whether 
 > the router peer has the next hop IP address and the packet is just sent to 
 > that router. From there on it is a fresh start.
 
 [Mickey] By routing recursion I just meant a small walk through all of the 
router ports to find which router port has the next hop address.
We are on the same page then (for the above point.)

So to summarize, a new table will help if we add the outport as a column and 
keep it optional. If it is filled, use it. Else, figure out the next hop.



 

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

Re: [ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

[Numan Siddique]

On Thu, Apr 7, 2016 at 9:48 PM, Justin Pettit  wrote:

>
> > On Apr 6, 2016, at 11:26 PM, Numan Siddique  wrote:
> >
> >
> > ​Thanks for the comments Justin. I tried a similar approach. It will not
> work in the cases where the port security address also has a prefix defined.
> > For example with port security - "00:00:00:00:00:02 10.0.0.4/24", the
> ovn lexer parser is throwing the below error,
> >
> > ---
> > lflow|WARN|error parsing match "outport == "sw0-port2" && eth.dst ==
> 00:00:00:00:00:02 && ip4.dst == {255.255.255.255, 224.0.0.0/4, 10.0.0.4/24}":
> Value contains unmasked 1-bits.
> > --
>
> Ah, it should probably be added to the unit tests to make sure we don't
> reintroduce a problem.  (Thanks for writing unit tests, by the way.)
> What if you apply the mask first like the patch at the end of this
> message?  I also expanded your unit tests to include a check for the issue
> you mentioned.
>
>

​Hi Justin, there is still a problem with the below approach.​

In the case where port security has "10.0.0.4/24" it means
that the logical port
​is restricted in sending and receiving IP traffic with ip address
10.0.0.4. IP traffic with any other ip address should be dropped. But with
the below approach we would be allowing all the ip addresses in the
10.0.0.0/24.

​Below is the port security description


Each element in the set may additionally contain one or more IPv4 or
IPv6 addresses (or both), with optional masks. If a mask is given, it
must be a CIDR mask. In addition to the restrictions described for
Ethernet addresses above, such an element restricts the IPv4 or IPv6
addresses from which the host may send and to which it may receive
packets to the specified addresses. A masked address, if the host part
is zero, indicates that the host is allowed to use any address in the
subnet; if the host part is nonzero, the mask simply indicates the size
of the subnet. In addition:


​In the initial implementation I had missed to implement t​he case where
the host part is zero. :)

​Thanks
Numan​



--Justin
>
>
> -=-=-=-=-=-=-
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 302cc1d..e60f72e 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1179,8 +1179,11 @@ build_port_security_nd(struct ovn_port *op, struct
> hmap *
>  if (ps.n_ipv4_addrs) {
>  ds_put_cstr(, " && (");
>  for (size_t i = 0; i < ps.n_ipv4_addrs; i++) {
> -ds_put_format(, "arp.spa == "IP_FMT" || ",
> -  IP_ARGS(ps.ipv4_addrs[i].addr));
> +ovs_be32 mask =
> be32_prefix_mask(ps.ipv4_addrs[i].plen);
> +ds_put_cstr(, "arp.spa == ");
> +ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask,
> + );
> +ds_put_cstr(, " || ");
>  }
>  ds_chomp(, ' ');
>  ds_chomp(, '|');
> @@ -1264,7 +1267,9 @@ build_port_security_ip(enum ovn_pipeline pipeline,
> struct
>  }
>
>  for (int i = 0; i < ps.n_ipv4_addrs; i++) {
> -ds_put_format(, IP_FMT", ",
> IP_ARGS(ps.ipv4_addrs[i].addr
> +ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen);
> +ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask,
> );
> +ds_put_cstr(, ", ");
>  }
>
>  /* Replace ", " by "}". */
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 22121e1..d8bc395 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1930,6 +1930,27 @@ for i in 1 2 3; do
>  test_ipv6 ${i}3 f${i}${i}3 f021 $sip $tip
>  done
>
> +# configure lport13 to send and received IPv4 packets with an address
> range
> +ovn-nbctl lport-set-port-security lp13 "f0:00:00:00:00:13 192.168.0.13
> 10.0.0.4
> +
> +sip=`ip_to_hex 10 0 0 14`
> +tip=`ip_to_hex 192 168 0 23`
> +# IPv4 packet from lport13 with src ip 10.0.0.14 destined to lport23
> +# with dst ip 192.168.0.23 should be allowed
> +test_ip 13 f013 f023 $sip $tip 23
> +
> +sip=`ip_to_hex 192 168 0 33`
> +tip=`ip_to_hex 10 0 0 15`
> +# IPv4 packet from lport33 with src ip 192.168.0.33 destined to lport13
> +# with dst ip 10.0.0.15 should be received by lport13
> +test_ip 33 f033 f013 $sip $tip 13
> +
> +sip=`ip_to_hex 10 0 0 13`
> +tip=`ip_to_hex 192 168 0 22`
> +# arp packet with inner ip 10.0.0.13 should be allowed for lport13
> +test_arp 13 f013 f013 $sip $tip 0 f022
> +
> +
>
>  # Allow some time for packet forwarding.
>
>
>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3 2/2] ovn: Add software l2 gateway.

[Ryan Moats]

"dev"  wrote on 04/07/2016 10:39:33 AM:

> From: Russell Bryant 
> To: dev@openvswitch.org
> Date: 04/07/2016 10:40 AM
> Subject: [ovs-dev] [PATCH v3 2/2] ovn: Add software l2 gateway.
> Sent by: "dev" 
>
> This patch implements one approach to using ovn-controller to implement
> a software l2 gateway between logical and physical networks.
>
> A new logical port type called "gateway" is introduced here.  It is very
> close to how localnet ports work, with the following exception:
>
> - A localnet port makes OVN use the physical network as the
>   transport between hypervisors instead of tunnels. A gateway port still
>   uses tunnels between all hypervisors, and packets only go to/from the
>   specified physical network as needed via the chassis the gateway port
>   is bound to.
>
> - A gateway port also gets bound to a chassis while a localnet port does
>   not.  This binding is not done by ovn-controller.  It is left as an
>   administrative function.  In the case of OpenStack, the Neutron plugin
>   will do this.
>
> Signed-off-by: Russell Bryant 

I get this and I think it makes a good starting platform for various
functional items, so...

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

Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router

[Guru Shetty]

On 7 April 2016 at 01:43, Mickey Spiegel  wrote:

> See comments inline
>
> Mickey
>
>
> -Guru Shetty  wrote: -
> >To: Mickey Spiegel/San Jose/IBM@IBMUS
> >From: Guru Shetty 
> >Date: 04/06/2016 05:58PM
> >Cc: ovs dev , Shi Xin Ruan 
> >Subject: Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router
> >
> >
> >
> >On 6 April 2016 at 16:55, Mickey Spiegel  wrote:
> >>Steve and Guru,
> >>
> >> I am not all that concerned about the "valid" column, but I do think
> that we will need a different additional column in the near future for
> output port.
> >>
> >> There are three different motivations for allowing output port to be
> specified in the static route:
> >> 1) In order to support static routes with a link local next hop. If a
> link local next hop is used, it is possible that the same link local
> address appears on different router ports with different meanings. By
> specifying the port, this disambiguates the specific link local next hop
> that was desired.
> >> Note: Neutron does not yet support static routes with link local next
> hop. We need to drive the feature in Neutron as well, optionally allowing a
> router interface to be specified in addition to the next hop.
> >> 2) This feature should not really be specific to static routes, it
> should also apply to dynamic routes when we add that in the future.
> Basically anything that looks up an IP address prefix and returns a next
> hop and optionally an output port. There are cases where explicitly
> specifying the output port makes sense.
> > For point 1 and 2, I am not sure whether we should do anything till
> there is code in ovn-northd that actually uses it.
>
> [Mickey] Point of clarification, the proposal is to add an output port
> column to the static route table in northd. The question is not whether
> there is code in ovn-northd that uses it, it is whether there is code at
> the CMS layer that fills this column. Both the features in points 1 and 2
> would make use of this column.
>
I see what you mean now.


> Even if we don't add this column now, if you don't have a separate static
> route table, it will make such an addition difficult in the future.
>
> >> 3) In order to optimize processing of the routing recursion (Steve's
> code loops over the router's ports in ovn-northd.c to carry out this
> routing recursion), we might want to do it above OVN in an event triggered
> manner, rather than every time ovn-northd.c recalculates the flows that it
> places into the southbound database.
> > I don't think I understand the above point. The static_route I have in
> mind need not recursively look through routers. All they need is to see
> whether the router peer has the next hop IP address and the packet is just
> sent to that router. From there on it is a fresh start.
>
> [Mickey] By routing recursion I just meant a small walk through all of the
> router ports to find which router port has the next hop address.
>
We are on the same page then (for the above point.)

So to summarize, a new table will help if we add the outport as a column
and keep it optional. If it is filled, use it. Else, figure out the next
hop.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCHv3] checkpatch: Don't enforce char limit on tests.

[Aaron Conole]

Joe Stringer  writes:

> Although tests ideally also stick to shorter line lengths, it is very
> common for fixed text blocks like flows or large packets to be specified
> within tests. Checkpatch shouldn't complain about cases like these.
>
> Signed-off-by: Joe Stringer 
> Acked-by: Russell Bryant 
> ---
> v2: Create a wider blacklist of formats to check for in the filenames.
> v3: Add '.m4' to the blacklist
> Improve python style
> Fix issues where patch line length checking would be entirely
> disabled after the first blacklisted file is hit.
> ---
>  utilities/checkpatch.py | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index d9011f370816..dbdcbc805b4d 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -56,6 +56,13 @@ skip_trailing_whitespace_check = False
>  skip_block_whitespace_check = False
>  skip_signoff_check = False
>  
> +# Don't enforce character limit on files that include these characters in 
> their
> +# name, as they may have legitimate reasons to have longer lines.
> +#
> +# Python isn't checked as flake8 performs these checks during build.
> +line_length_blacklist = ['.am', '.at', 'etc', '.in', '.m4', '.mk', '.patch',
> + '.py']
> +
>  
>  def is_added_line(line):
>  """Returns TRUE if the line in question is an added line.
> @@ -99,14 +106,23 @@ def ovs_checkpatch_parse(text):
>  co_authors = []
>  parse = 0
>  current_file = ''
> +previous_file = ''
>  scissors = re.compile(r'^[\w]*---[\w]*')
>  hunks = re.compile('^(---|\+\+\+) (\S+)')
>  is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
>re.I | re.M | re.S)
>  is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
>re.I | re.M | re.S)
> +skip_line_length_check = False
>  
>  for line in text.split('\n'):
> +if current_file != previous_file:
> +previous_file = current_file
> +if any([fmt in current_file for fmt in line_length_blacklist]):
> +skip_line_length_check = True
> +else:
> +skip_line_length_check = False
> +
>  lineno = lineno + 1
>  if len(line) <= 0:
>  continue
> @@ -154,7 +170,7 @@ def ovs_checkpatch_parse(text):
>  if trailing_whitespace_or_crlf(line[1:]):
>  print_line = True
>  print_warning("Line has trailing whitespace", lineno)
> -if len(line[1:]) > 79:
> +if len(line[1:]) > 79 and not skip_line_length_check:
>  print_line = True
>  print_warning("Line is greater than 79-characters long",
>lineno)

LGTM,

Tested-by: Aaron Conole 

Thanks for this!
-Aaron
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] system-traffic: Fix packet-in format for tests.

[Joe Stringer]

On 6 April 2016 at 19:47, Daniele Di Proietto  wrote:
> Thanks for fixing this!
>
> Acked-by: Daniele Di Proietto 

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

[ovs-dev] [PATCHv3] checkpatch: Don't enforce char limit on tests.

[Joe Stringer]

Although tests ideally also stick to shorter line lengths, it is very
common for fixed text blocks like flows or large packets to be specified
within tests. Checkpatch shouldn't complain about cases like these.

Signed-off-by: Joe Stringer 
Acked-by: Russell Bryant 
---
v2: Create a wider blacklist of formats to check for in the filenames.
v3: Add '.m4' to the blacklist
Improve python style
Fix issues where patch line length checking would be entirely
disabled after the first blacklisted file is hit.
---
 utilities/checkpatch.py | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index d9011f370816..dbdcbc805b4d 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -56,6 +56,13 @@ skip_trailing_whitespace_check = False
 skip_block_whitespace_check = False
 skip_signoff_check = False
 
+# Don't enforce character limit on files that include these characters in their
+# name, as they may have legitimate reasons to have longer lines.
+#
+# Python isn't checked as flake8 performs these checks during build.
+line_length_blacklist = ['.am', '.at', 'etc', '.in', '.m4', '.mk', '.patch',
+ '.py']
+
 
 def is_added_line(line):
 """Returns TRUE if the line in question is an added line.
@@ -99,14 +106,23 @@ def ovs_checkpatch_parse(text):
 co_authors = []
 parse = 0
 current_file = ''
+previous_file = ''
 scissors = re.compile(r'^[\w]*---[\w]*')
 hunks = re.compile('^(---|\+\+\+) (\S+)')
 is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
   re.I | re.M | re.S)
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
+skip_line_length_check = False
 
 for line in text.split('\n'):
+if current_file != previous_file:
+previous_file = current_file
+if any([fmt in current_file for fmt in line_length_blacklist]):
+skip_line_length_check = True
+else:
+skip_line_length_check = False
+
 lineno = lineno + 1
 if len(line) <= 0:
 continue
@@ -154,7 +170,7 @@ def ovs_checkpatch_parse(text):
 if trailing_whitespace_or_crlf(line[1:]):
 print_line = True
 print_warning("Line has trailing whitespace", lineno)
-if len(line[1:]) > 79:
+if len(line[1:]) > 79 and not skip_line_length_check:
 print_line = True
 print_warning("Line is greater than 79-characters long",
   lineno)
-- 
2.1.4

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

Re: [ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.

[Joe Stringer]

On 6 April 2016 at 13:53, Russell Bryant  wrote:
>
>
> On Wed, Apr 6, 2016 at 3:46 PM, Joe Stringer  wrote:
>>
>> Although tests ideally also stick to shorter line lengths, it is very
>> common for fixed text blocks like flows or large packets to be specified
>> within tests. Checkpatch shouldn't complain about cases like these.
>>
>> Signed-off-by: Joe Stringer 
>
>
> Acked-by: Russell Bryant 
>
>  @@ -99,14 +105,23 @@ def ovs_checkpatch_parse(text):
>>
>>  co_authors = []
>>  parse = 0
>>  current_file = ''
>> +previous_file = ''
>>  scissors = re.compile(r'^[\w]*---[\w]*')
>>  hunks = re.compile('^(---|\+\+\+) (\S+)')
>>  is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
>>re.I | re.M | re.S)
>>  is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
>>re.I | re.M | re.S)
>> +skip_line_length_check = False
>>
>>  for line in text.split('\n'):
>> +if current_file is not previous_file:
>> +previous_file = current_file
>> +for filetype in line_length_blacklist:
>> +if filetype in current_file:
>> +skip_line_length_check = True
>> +break
>> +
>
>
> Since you made a comment about Python style, the above could also be:
>
> if current_file != previous_file:
> previous_file = current_file
> if any([ft in current_file for ft in line_length_blacklist]):
> skip_line_length_check = True
> break

I'll fold this in (minus the break line) and send v3.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.

[Joe Stringer]

On 7 April 2016 at 05:29, Aaron Conole  wrote:
> Joe Stringer  writes:
>
>> Although tests ideally also stick to shorter line lengths, it is very
>> common for fixed text blocks like flows or large packets to be specified
>> within tests. Checkpatch shouldn't complain about cases like these.
>>
>> Signed-off-by: Joe Stringer 
>> ---
>> v2: Broaden the set of blacklisted files to not check.
>> ---
>>  utilities/checkpatch.py | 17 -
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index d9011f370816..83d14e89269e 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -56,6 +56,12 @@ skip_trailing_whitespace_check = False
>>  skip_block_whitespace_check = False
>>  skip_signoff_check = False
>>
>> +# Don't enforce character limit on files that include these characters in 
>> their
>> +# name, as they may have legitimate reasons to have longer lines.
>> +#
>> +# Python isn't checked as flake8 performs these checks during build.
>> +line_length_blacklist = ['.am', '.at', 'etc', '.in', '.mk', '.patch', '.py']
>> +
>>
>>  def is_added_line(line):
>>  """Returns TRUE if the line in question is an added line.
>> @@ -99,14 +105,23 @@ def ovs_checkpatch_parse(text):
>>  co_authors = []
>>  parse = 0
>>  current_file = ''
>> +previous_file = ''
>>  scissors = re.compile(r'^[\w]*---[\w]*')
>>  hunks = re.compile('^(---|\+\+\+) (\S+)')
>>  is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
>>re.I | re.M | re.S)
>>  is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
>>re.I | re.M | re.S)
>> +skip_line_length_check = False
>>
>>  for line in text.split('\n'):
>> +if current_file is not previous_file:
>> +previous_file = current_file
>> +for filetype in line_length_blacklist:
>> +if filetype in current_file:
>> +skip_line_length_check = True
>> +break
>
> You need an else condition here; otherwise a patch which orders files
>
> .c
> [blacklisted]
> .c
>
> will skip the second .c length line checks.
>
> Incremental patch could look something like (completely untested)
> below. Thanks for doing this.



Thanks for pointing this out, we also need to drop the 'break'
otherwise we'll simply stop processing files as soon as one of the
blacklist files are hit.

I'll send one more round for good measure, in case anything was missed
this time around ;-)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3 1/2] ovn-controller: Warn if system-id ismissing.

[Ryan Moats]

"dev"  wrote on 04/07/2016 10:39:32 AM:

> From: Russell Bryant 
> To: dev@openvswitch.org
> Date: 04/07/2016 10:39 AM
> Subject: [ovs-dev] [PATCH v3 1/2] ovn-controller: Warn if system-id
> is missing.
> Sent by: "dev" 
>
> If 'system-id' is missing from the Open_vSwitch database, ovn-controller
> will not work.  Log a warning if that happens to make it clear that
> configuration is incomplete.
>
> Signed-off-by: Russell Bryant 
> ---
>  ovn/controller/ovn-controller.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> v3:
>  - This patch is new in v3, as suggested by Han Zhou.
>
> diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
> index 6027011..31a9a39 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -169,7 +169,14 @@ static const char *
>  get_chassis_id(const struct ovsdb_idl *ovs_idl)
>  {
>  const struct ovsrec_open_vswitch *cfg =
> ovsrec_open_vswitch_first(ovs_idl);
> -return cfg ? smap_get(>external_ids, "system-id") : NULL;
> +const char *chassis_id = cfg ? smap_get(>external_ids,
> "system-id") : NULL;
> +
> +if (!chassis_id) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +VLOG_WARN_RL(, "'system-id' in Open_vSwitch database is
> missing.");
> +}
> +
> +return chassis_id;
>  }
>
>  /* Retrieves the OVN Southbound remote location from the
> --
> 2.5.5

This looks sane and it doesn't break unit tests, so...

Acked-by: Ryan Moats 

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

Re: [ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

[Justin Pettit]

> On Apr 6, 2016, at 11:26 PM, Numan Siddique  wrote:
> 
> 
> ​Thanks for the comments Justin. I tried a similar approach. It will not work 
> in the cases where the port security address also has a prefix defined.
> For example with port security - "00:00:00:00:00:02 10.0.0.4/24", the ovn 
> lexer parser is throwing the below error,
> 
> ---
> lflow|WARN|error parsing match "outport == "sw0-port2" && eth.dst == 
> 00:00:00:00:00:02 && ip4.dst == {255.255.255.255, 224.0.0.0/4, 10.0.0.4/24}": 
> Value contains unmasked 1-bits.
> --

Ah, it should probably be added to the unit tests to make sure we don't 
reintroduce a problem.  (Thanks for writing unit tests, by the way.)What if 
you apply the mask first like the patch at the end of this message?  I also 
expanded your unit tests to include a check for the issue you mentioned.

--Justin


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

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 302cc1d..e60f72e 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1179,8 +1179,11 @@ build_port_security_nd(struct ovn_port *op, struct hmap *
 if (ps.n_ipv4_addrs) {
 ds_put_cstr(, " && (");
 for (size_t i = 0; i < ps.n_ipv4_addrs; i++) {
-ds_put_format(, "arp.spa == "IP_FMT" || ",
-  IP_ARGS(ps.ipv4_addrs[i].addr));
+ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen);
+ds_put_cstr(, "arp.spa == ");
+ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask,
+ );
+ds_put_cstr(, " || ");
 }
 ds_chomp(, ' ');
 ds_chomp(, '|');
@@ -1264,7 +1267,9 @@ build_port_security_ip(enum ovn_pipeline pipeline, struct 
 }
 
 for (int i = 0; i < ps.n_ipv4_addrs; i++) {
-ds_put_format(, IP_FMT", ", IP_ARGS(ps.ipv4_addrs[i].addr
+ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen);
+ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask, );
+ds_put_cstr(, ", ");
 }
 
 /* Replace ", " by "}". */
diff --git a/tests/ovn.at b/tests/ovn.at
index 22121e1..d8bc395 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1930,6 +1930,27 @@ for i in 1 2 3; do
 test_ipv6 ${i}3 f${i}${i}3 f021 $sip $tip
 done
 
+# configure lport13 to send and received IPv4 packets with an address range
+ovn-nbctl lport-set-port-security lp13 "f0:00:00:00:00:13 192.168.0.13 10.0.0.4
+
+sip=`ip_to_hex 10 0 0 14`
+tip=`ip_to_hex 192 168 0 23`
+# IPv4 packet from lport13 with src ip 10.0.0.14 destined to lport23
+# with dst ip 192.168.0.23 should be allowed
+test_ip 13 f013 f023 $sip $tip 23
+
+sip=`ip_to_hex 192 168 0 33`
+tip=`ip_to_hex 10 0 0 15`
+# IPv4 packet from lport33 with src ip 192.168.0.33 destined to lport13
+# with dst ip 10.0.0.15 should be received by lport13
+test_ip 33 f033 f013 $sip $tip 13
+
+sip=`ip_to_hex 10 0 0 13`
+tip=`ip_to_hex 192 168 0 22`
+# arp packet with inner ip 10.0.0.13 should be allowed for lport13
+test_arp 13 f013 f013 $sip $tip 0 f022
+
+
 
 # Allow some time for packet forwarding.
 



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

Re: [ovs-dev] [PATCH] ovn-docker: Update overlay mode for new tcp ports.

[Guru Shetty]

On 7 April 2016 at 08:48, Russell Bryant  wrote:

>
> On Wed, Apr 6, 2016 at 8:27 PM, Gurucharan Shetty  wrote:
>
>> There were changes made recently wherein 2 ovsdb-server is
>> started for northbound and southbound databases with tcp ports
>> 6641 and 6642. This breaks Docker integration. This commit
>> fixes it.
>>
>> Signed-off-by: Gurucharan Shetty 
>> ---
>>
>
> Acked-by: Russell Bryant 
>
Thank you! Applied to master.

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

Re: [ovs-dev] [PATCH v11 0/8] Convert DPDK configuration from command line to DB based

[Aaron Conole]

Ping; I know some folks are busy with backlogs, life, etc. Didn't want
this to fall off the radar, that's all.

Aaron Conole  writes:

> Currently, configuration of DPDK parameters is done via the command line
> through a --dpdk **OPTIONS** -- command line argument. This has a number of
> challenges, including:
> * It must be the first option passed to ovs-vswitchd
> * It is the only datapath feature in OVS to be configured on the command
>   line
> * It requires specialized knowledge of sub-component command switches
> * It also inteprets non-EAL arguments (confusing users)
> * It is a broken model for datapath configuration.
>
> This series brings the following changes to openvswitch:
> * All DPDK options are taken from the ovs database rather than the
>   command line
> * Non-EAL arguments also have separate database entries
> * DPDK lcores are optionally auto-assigned to a single core based on the
>   bridge coremask.
> * DPDK options have default behaviors
> * Updated documentation
>
> This series has been build tested (including `make check`) on OSX, Fedora 23,
> Windows (via appveyor), and FreeBSD 10.3; the v11 has had very basic testing
> applied (start, configure some of the settings). I have removed ACKs and
> Tested-bys for some of the patches since they underwent changes that I felt
> disqualified continued use of the Acked-by: and Tested-by: tags.
>
> Travis-ci build: https://travis-ci.org/orgcandman/ovs/builds/120081527
> Appveyor build: https://ci.appveyor.com/project/orgcandman/ovs/build/1.0.9
>
> This is a resend due to an accidentally omitted hunk in 4/8.
>
> A huge round of thanks on the work so far should be given to the following
> folks (in alphabetical order):
> * Ben Pfaff (Reviews, vhost-sock-dir escape suggestion)
> * Christian Erhardt (Testing)
> * Daniele Di Proietto   (Reviews, general suggestions)
> * Flavio Leitner(Original efforts, reviews)
> * Kevin Traynor (Testing, general suggestions, reviews, doc reviews)
> * Panu Matilainen   (Initialization ideas, eal arguments ideas, reviews)
> * RobertX Wojciechowicz (Testing, general suggestions)
> * Sean Mooney   (Testing, general suggestions)
>
> v2:
> * Dropped the vhost-user socket configuration options. Those can be re-added
>   as an extension
> * Incorporated feedback from Kevin Traynor.
>
> v3:
> * Went back to a global dpdk-init
> * Language cleanup and various minor fixes
>
> v4:
> * Added a way to pass arbitrary eal arguments
>
> v5:
> * Restore the socket-mem default, and fix up the ovs-dev.py script, along
>   with the manpage for ovsdb-server
>
> v6:
> * Correct a documentation issue with INSTALL.DPDK.md
> * Correct a non-dpdk enabled OVS incorrect warning variable
> * Remove an excess whitespace
>
> v7:
> * After testing by Christian with dpdk-alloc-mem
>
> v8:
> * Confirmed ``make check`` operation with and without dpdk.
>   Retested on live-host
>
> v9:
> * Cleanup of comments
> * Cleanup of one place where headers are specified
> * Mark the dpdk coremask and numa config as optional
> * Added 5/6 to scan the extras and warn the user when conflicting
>   DB entries are present
> * Acks given for all but patch 5/6
>
> v10:
> * Rebased against latest upstream
> * ACK or Tested-by for all patches
> * Code cleanup on patch 2/6 (vhost-cuse warning)
> * DB options documentation cleanup.
>
> v11:
> * Spacing cleanups (verified with checkpatch)
> * Introduced a realpath() call
> * Validate the vhost-sock-dir is in a protected area of the filesystem
> * Split the netdev-dpdk into a netdev-nodpdk
> * Converted most of the ovs_abort() error paths into VLOG_ERR()s
>
> Aaron Conole (8):
>   netdev-dpdk: Restore thread affinity after DPDK init
>   util: Add a path canonicalizer
>   netdev-dpdk: Convert initialization from cmdline to db
>   netdev-dpdk: Restrict vhost_sock_dir
>   netdev-dpdk: Autofill lcore coremask if absent
>   netdev-dpdk: Allow arbitrary eal arguments
>   netdev-dpdk: Check dpdk-extra when reading db
>   NEWS: Announce the DPDK EAL configuration change
>
>  FAQ.md |   6 +-
>  INSTALL.DPDK.md|  87 +++--
>  NEWS   |   3 +
>  configure.ac   |   2 +-
>  lib/automake.mk|   4 +
>  lib/netdev-dpdk.c  | 430 
> +
>  lib/netdev-dpdk.h  |  13 +-
>  lib/netdev-nodpdk.c|  21 +++
>  lib/util.c |  52 ++
>  lib/util.h |   1 +
>  tests/library.at   |   5 +
>  tests/ofproto-macros.at|   3 +-
>  tests/test-util.c  |  23 +++
>  utilities/ovs-dev.py   |   8 +-
>  vswitchd/bridge.c  |   3 +
>  vswitchd/ovs-vswitchd.8.in |   6 +-
>  vswitchd/ovs-vswitchd.c|  25 +--
>  vswitchd/vswitch.xml   | 143 ++-
>  18 files changed, 697 insertions(+), 138 deletions(-)
>  create mode 100644 lib/netdev-nodpdk.c

[ovs-dev] [PATCH v2 2/3] expr: Add address set support.

[Russell Bryant]

Update the OVN expression parser to support address sets.  Previously,
you could have a set of IP or MAC addresses in this form:

{addr1, addr2, ..., addrN}

This patch adds support for a bit of indirection where we can define a
set of addresses and refer to them by name.

address_set(name)

A future patch will expose the ability to define address sets for use.

Signed-off-by: Russell Bryant 
---
 ovn/controller/lflow.c |   2 +-
 ovn/lib/actions.c  |   2 +-
 ovn/lib/expr.c | 139 ++---
 ovn/lib/expr.h |  15 ++
 tests/ovn.at   |  36 +
 tests/test-ovn.c   |  31 +--
 6 files changed, 214 insertions(+), 11 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 7a3466f..287ffd3 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -303,7 +303,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct 
lport_index *lports,
 struct hmap matches;
 struct expr *expr;
 
-expr = expr_parse_string(lflow->match, , );
+expr = expr_parse_string(lflow->match, , NULL, );
 if (!error) {
 if (prereqs) {
 expr = expr_combine(EXPR_T_AND, expr, prereqs);
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index a17b5a7..2e0516a 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -179,7 +179,7 @@ add_prerequisite(struct action_context *ctx, const char 
*prerequisite)
 struct expr *expr;
 char *error;
 
-expr = expr_parse_string(prerequisite, ctx->ap->symtab, );
+expr = expr_parse_string(prerequisite, ctx->ap->symtab, NULL, );
 ovs_assert(!error);
 ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr);
 }
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index aae5df6..4ba0487 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -450,6 +450,7 @@ struct expr_field {
 struct expr_context {
 struct lexer *lexer;/* Lexer for pulling more tokens. */
 const struct shash *symtab; /* Symbol table. */
+const struct shash *address_sets; /* Table of address sets. */
 char *error;/* Error, if any, otherwise NULL. */
 bool not;   /* True inside odd number of NOT operators. */
 };
@@ -807,6 +808,61 @@ parse_constant(struct expr_context *ctx, struct 
expr_constant_set *cs,
 }
 }
 
+static bool
+parse_address_set(struct expr_context *ctx, struct expr_constant_set *cs)
+{
+if (!ctx->address_sets) {
+expr_syntax_error(ctx, "No address sets defined.");
+return false;
+}
+
+if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) {
+expr_syntax_error(ctx, "Expecting '(' after 'address_set'");
+return false;
+}
+
+if (ctx->lexer->token.type != LEX_T_ID) {
+expr_syntax_error(ctx, "Expecting name after 'address_set('");
+return false;
+}
+
+bool ok = true;
+char *name = xstrdup(ctx->lexer->token.s);
+lexer_get(ctx->lexer);
+
+if (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
+expr_syntax_error(ctx, "Expecting ')' after 'address_set('");
+ok = false;
+goto cleanup;
+}
+
+struct expr_constant_set *addr_set
+= shash_find_data(ctx->address_sets, name);
+if (!name) {
+expr_syntax_error(ctx, "Unknown address set: '%s'", name);
+ok = false;
+goto cleanup;
+}
+
+cs->type = EXPR_C_INTEGER;
+cs->in_curlies = true;
+cs->n_values = addr_set->n_values;
+cs->values = xmalloc(cs->n_values * sizeof *cs->values);
+size_t i;
+for (i = 0; i < cs->n_values; i++) {
+union expr_constant *c1 = >values[i];
+union expr_constant *c2 = _set->values[i];
+c1->value = c2->value;
+c1->format = c2->format;
+c1->masked = c2->masked;
+c1->mask = c2->mask;
+}
+
+cleanup:
+free(name);
+return ok;
+}
+
 /* Parses a single or {}-enclosed set of integer or string constants into 'cs',
  * which the caller need not have initialized.  Returns true on success, in
  * which case the caller owns 'cs', false on failure, in which case 'cs' is
@@ -818,7 +874,9 @@ parse_constant_set(struct expr_context *ctx, struct 
expr_constant_set *cs)
 bool ok;
 
 memset(cs, 0, sizeof *cs);
-if (lexer_match(ctx->lexer, LEX_T_LCURLY)) {
+if (lexer_match_id(ctx->lexer, "address_set")) {
+ok = parse_address_set(ctx, cs);
+} else if (lexer_match(ctx->lexer, LEX_T_LCURLY)) {
 ok = true;
 cs->in_curlies = true;
 do {
@@ -850,6 +908,72 @@ expr_constant_set_destroy(struct expr_constant_set *cs)
 }
 }
 
+void
+expr_address_sets_add(struct shash *address_sets, const char *name,
+  const char * const *addresses, size_t n_addresses)
+{
+/* Replace any existing entry for this name. */
+expr_address_sets_remove(address_sets, name);
+
+struct expr_constant_set *cset = xzalloc(sizeof *cset);
+   

[ovs-dev] [PATCH v2 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

This feature was originally proposed here:

  http://openvswitch.org/pipermail/dev/2016-March/067440.html

A common use case for OVN ACLs involves needing to match a set of IP
addresses.

   outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}

This example match only has 3 addresses, but it could easily have
hundreds of addresses.  In some cases, the same large set of addresses
needs to be used in several ACLs.

This patch adds a new Address_Set table to OVN_Northbound so that a set
of addresses can be specified once and then referred to by name in ACLs.
To recreate the above example, you would first create an address set:

  $ ovn-nbctl create Address_Set name=set1 
addresses=10.0.0.5,10.0.0.25,10.0.0.50

Then you can refer to this address set by name in an ACL match:

  outport == "lp1" && ip4.src == address_set(set1)

Signed-off-by: Russell Bryant 
---
 ovn/controller/lflow.c| 155 +-
 ovn/northd/ovn-northd.c   |  42 +
 ovn/ovn-nb.ovsschema  |  12 +++-
 ovn/ovn-nb.xml|  29 +
 ovn/ovn-sb.ovsschema  |  12 +++-
 ovn/ovn-sb.xml|  19 ++
 ovn/utilities/ovn-nbctl.c |   4 ++
 ovn/utilities/ovn-sbctl.c |   4 ++
 tests/ovn.at  |  10 +++
 9 files changed, 282 insertions(+), 5 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 287ffd3..00b9e67 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -27,6 +27,7 @@
 #include "ovn/lib/ovn-sb-idl.h"
 #include "packets.h"
 #include "simap.h"
+#include "sset.h"
 
 VLOG_DEFINE_THIS_MODULE(lflow);
 
@@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow);
 /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */
 static struct shash symtab;
 
+/* Contains an internal expr datastructure that represents an address set. */
+static struct shash expr_address_sets;
+
 static void
 add_logical_register(struct shash *symtab, enum mf_field_id id)
 {
@@ -48,6 +52,7 @@ void
 lflow_init(void)
 {
 shash_init();
+shash_init(_address_sets);
 
 /* Reserve a pair of registers for the logical inport and outport.  A full
  * 32-bit register each is bigger than we need, but the expression code
@@ -157,6 +162,150 @@ lflow_init(void)
 expr_symtab_add_field(, "sctp.src", MFF_SCTP_SRC, "sctp", false);
 expr_symtab_add_field(, "sctp.dst", MFF_SCTP_DST, "sctp", false);
 }
+
+/* Details of an address set currently in address_sets. We keep a cached
+ * copy of sets still in their string form here to make it easier to compare
+ * with the current values in the OVN_Southbound database. */
+struct address_set {
+char **addresses;
+size_t n_addresses;
+};
+
+/* struct address_set instances for address sets currently in the symtab,
+ * hashed on the address set name. */
+static struct shash local_address_sets = 
SHASH_INITIALIZER(_address_sets);
+
+static int
+addr_cmp(const void *p1, const void *p2)
+{
+const char *s1 = p1;
+const char *s2 = p2;
+return strcmp(s1, s2);
+}
+
+/* Return true if the address sets match, false otherwise. */
+static bool
+address_sets_match(struct address_set *addr_set,
+   const struct sbrec_address_set *addr_set_rec)
+{
+char **addrs1;
+char **addrs2;
+
+if (addr_set->n_addresses != addr_set_rec->n_addresses) {
+return false;
+}
+size_t n_addresses = addr_set->n_addresses;
+
+addrs1 = xmemdup(addr_set->addresses,
+ n_addresses * sizeof addr_set->addresses[0]);
+addrs2 = xmemdup(addr_set_rec->addresses,
+ n_addresses * sizeof addr_set_rec->addresses[0]);
+
+qsort(addrs1, n_addresses, sizeof *addrs1, addr_cmp);
+qsort(addrs2, n_addresses, sizeof *addrs2, addr_cmp);
+
+bool res = true;
+size_t i;
+for (i = 0; i <  n_addresses; i++) {
+if (strcmp(addrs1[i], addrs2[i])) {
+res = false;
+break;
+}
+}
+
+free(addrs1);
+free(addrs2);
+
+return res;
+}
+
+static void
+address_set_destroy(struct address_set *addr_set)
+{
+size_t i;
+for (i = 0; i < addr_set->n_addresses; i++) {
+free(addr_set->addresses[i]);
+}
+if (addr_set->n_addresses) {
+free(addr_set->addresses);
+}
+free(addr_set);
+}
+
+static void
+update_address_sets(struct controller_ctx *ctx)
+{
+/* Remember the names of all address sets currently in expr_address_sets
+ * so we can detect address sets that have been deleted. */
+struct sset cur_address_sets = SSET_INITIALIZER(_address_sets);
+
+struct shash_node *node;
+SHASH_FOR_EACH (node, _address_sets) {
+sset_add(_address_sets, node->name);
+}
+
+/* Iterate address sets in the southbound database.  Create and update the
+ * corresponding symtab entries as necessary. */
+const struct sbrec_address_set *addr_set_rec;
+SBREC_ADDRESS_SET_FOR_EACH (addr_set_rec, ctx->ovnsb_idl) {
+  

[ovs-dev] [PATCH v2 0/3] OVN address sets.

[Russell Bryant]

v1->v2:
 - rebase
 - add docs that I forgot to include in v1.

Russell Bryant (3):
  ovn-controller: Add missing shash_destroy().
  expr: Add address set support.
  ovn: Add address_set() support for ACLs.

 ovn/controller/lflow.c| 156 +-
 ovn/lib/actions.c |   2 +-
 ovn/lib/expr.c| 139 +++--
 ovn/lib/expr.h|  15 +
 ovn/northd/ovn-northd.c   |  42 +
 ovn/ovn-nb.ovsschema  |  12 +++-
 ovn/ovn-nb.xml|  29 +
 ovn/ovn-sb.ovsschema  |  12 +++-
 ovn/ovn-sb.xml|  19 ++
 ovn/utilities/ovn-nbctl.c |   4 ++
 ovn/utilities/ovn-sbctl.c |   4 ++
 tests/ovn.at  |  46 ++
 tests/test-ovn.c  |  31 -
 13 files changed, 496 insertions(+), 15 deletions(-)

-- 
2.5.5

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

[ovs-dev] [PATCH v2 1/3] ovn-controller: Add missing shash_destroy().

[Russell Bryant]

expr_symtab_destroy() destroys the contents of the symtab shash, but not
the shash itself.  Add a missing shash_destroy() call in
lflow_destroy().

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

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index bcad318..7a3466f 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -443,4 +443,5 @@ void
 lflow_destroy(void)
 {
 expr_symtab_destroy();
+shash_destroy();
 }
-- 
2.5.5

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

[ovs-dev] [PATCH v3 2/2] ovn: Add software l2 gateway.

[Russell Bryant]

This patch implements one approach to using ovn-controller to implement
a software l2 gateway between logical and physical networks.

A new logical port type called "gateway" is introduced here.  It is very
close to how localnet ports work, with the following exception:

- A localnet port makes OVN use the physical network as the
  transport between hypervisors instead of tunnels. A gateway port still
  uses tunnels between all hypervisors, and packets only go to/from the
  specified physical network as needed via the chassis the gateway port
  is bound to.

- A gateway port also gets bound to a chassis while a localnet port does
  not.  This binding is not done by ovn-controller.  It is left as an
  administrative function.  In the case of OpenStack, the Neutron plugin
  will do this.

Signed-off-by: Russell Bryant 
---
 ovn/controller/binding.c|   9 ++
 ovn/controller/ovn-controller.8.xml |  31 ++-
 ovn/controller/ovn-controller.c |   5 +-
 ovn/controller/patch.c  |  37 +---
 ovn/controller/patch.h  |   3 +-
 ovn/controller/physical.c   |  22 +++--
 ovn/ovn-nb.xml  |  19 +
 ovn/ovn-sb.xml  |  41 -
 tests/ovn.at| 164 
 9 files changed, 306 insertions(+), 25 deletions(-)

v1->v2:
 - Rebase and resolve conflicts with master.
 - Fix doc typo when descriping gateway port type.
 - Ensure chassis_id is non-NULL before calling patch_run().
v2->v3:
 - Add "ovn-gateway-port" external-id instead of re-using "ovn-localnet-port" 
id.
 - Add docs that I forgot to include.


diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index d3ca9c9..de18017 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -205,6 +205,15 @@ binding_run(struct controller_ctx *ctx, const struct 
ovsrec_bridge *br_int,
 }
 sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
 }
+} else if (!strcmp(binding_rec->type, "gateway")
+   && binding_rec->chassis == chassis_rec) {
+/* A locally bound gateway port.
+ *
+ * ovn-controller does not bind gateway ports itself.
+ * Choosing a chassis * for a gateway port is left
+ * up to an entity external to OVN. */
+sset_add(_lports, binding_rec->logical_port);
+add_local_datapath(local_datapaths, binding_rec);
 } else if (binding_rec->chassis == chassis_rec) {
 if (ctx->ovnsb_idl_txn) {
 VLOG_INFO("Releasing lport %s from this chassis.",
diff --git a/ovn/controller/ovn-controller.8.xml 
b/ovn/controller/ovn-controller.8.xml
index 1ee3a6e..228a8cd 100644
--- a/ovn/controller/ovn-controller.8.xml
+++ b/ovn/controller/ovn-controller.8.xml
@@ -184,10 +184,10 @@
   The presence of this key identifies a patch port as one created by
   ovn-controller to connect the integration bridge and
   another bridge to implement a localnet logical port.
-  Its value is the name of the logical port with type=localnet that
-  the port implements.
-  See external_ids:ovn-bridge-mappings, above,
-  for more information.
+  Its value is the name of the logical port with type
+  set to localnet that the port implements. See
+  external_ids:ovn-bridge-mappings, above, for more
+  information.
 
 
 
@@ -199,6 +199,29 @@
   
 
   
+external-ids:ovn-gateway-port in the Port
+table
+  
+  
+
+  The presence of this key identifies a patch port as one created by
+  ovn-controller to connect the integration bridge and
+  another bridge to implement a gateway logical port.
+  Its value is the name of the logical port with type
+  set to gateway that the port implements. See
+  external_ids:ovn-bridge-mappings, above, for more
+  information.
+
+
+
+  Each gateway logical port is implemented as a pair
+  of patch ports, one in the integration bridge, one in a different
+  bridge, with the same external-ids:ovn-gateway-port
+  value.
+
+  
+
+  
 external-ids:ovn-logical-patch-port in the
 Port table
   
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 31a9a39..d28845f 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -333,7 +333,10 @@ main(int argc, char *argv[])
 }
 
 if (br_int) {
-patch_run(, br_int, _datapaths, _datapaths);
+if (chassis_id) {
+patch_run(, br_int, _datapaths, _datapaths,
+  chassis_id);
+}
 
 struct lport_index lports;
 struct mcgroup_index mcgroups;

[ovs-dev] [PATCH v3 1/2] ovn-controller: Warn if system-id is missing.

[Russell Bryant]

If 'system-id' is missing from the Open_vSwitch database, ovn-controller
will not work.  Log a warning if that happens to make it clear that
configuration is incomplete.

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

v3:
 - This patch is new in v3, as suggested by Han Zhou.

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 6027011..31a9a39 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -169,7 +169,14 @@ static const char *
 get_chassis_id(const struct ovsdb_idl *ovs_idl)
 {
 const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl);
-return cfg ? smap_get(>external_ids, "system-id") : NULL;
+const char *chassis_id = cfg ? smap_get(>external_ids, "system-id") : 
NULL;
+
+if (!chassis_id) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "'system-id' in Open_vSwitch database is missing.");
+}
+
+return chassis_id;
 }
 
 /* Retrieves the OVN Southbound remote location from the
-- 
2.5.5

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

Re: [ovs-dev] [PATCH RFC 1/1] netdev-dpdk.c: Add ingress-policing functionality.

[Stokes, Ian]

> > >71034a0..faf3583 100644
> > >--- a/lib/netdev-dpdk.c
> > >+++ b/lib/netdev-dpdk.c
> > >@@ -53,6 +53,7 @@
> > >
> > > #include "rte_config.h"
> > > #include "rte_mbuf.h"
> > >+#include "rte_meter.h"
> > > #include "rte_virtio_net.h"
> > >
> > > VLOG_DEFINE_THIS_MODULE(dpdk);
> > >@@ -193,6 +194,11 @@ struct dpdk_ring {
> > > struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);  };
> > >
> > >+struct ingress_policer {
> > >+struct rte_meter_srtcm_params app_srtcm_params;
> > >+struct rte_meter_srtcm in_policer; };
> > >+
> > > struct netdev_dpdk {
> > > struct netdev up;
> > > int port_id;
> > >@@ -231,6 +237,13 @@ struct netdev_dpdk {
> > > /* Identifier used to distinguish vhost devices from each other
> */
> > > char vhost_id[PATH_MAX];
> > >
> > >+/* Ingress Policer */
> > >+rte_spinlock_t policer_lock;
> > >+struct ingress_policer *ingress_policer;
> > >+
> >
> > I would prefer not to have a lock at this level.
> >
> > I think it would make more sense to make the ingress_policer pointer
> > RCU protected and embed the spinlock into struct ingress_policer, to
> > protect the token bucket.
> >
> Sure I agree, I was modelling this on what we have currently in QoS just
> for a rough implementation.
> I can take a look at using RCU instead. This could possibly be extended
> to the QoS use case also in the futue.

Hi Daniele, I have been looking at an RCU implementation here but so far have 
not been able to get it working correctly.

The issue I'm seeing is when I destroy the policer while traffic is passing a 
segfault sometimes occurs as the meter is in use when the ingress policer is 
set to NULL.

I'm pretty sure this is down to my understanding (or lack thereof) of the 
ovsrcu behavior.

Is the following high level implementation correct in your eyes?

The ingress policer struct is as follows:

struct ingress_policer {
struct rte_meter_srtcm_params app_srtcm_params;
struct rte_meter_srtcm in_policer;
};

From your comment above you mention embedding the spinlock in the ingress 
policer struct.
Just to clarify, does the rcu by nature embed a spinlock or did you mean move 
the rte_spinlock policer_lock from the netdev_dpdk struct into the ingress 
policer struct?

Is the behavior you are thinking of something like the following for when 
traffic is being processed?

1. Get the rcu ingress_policer pointer.
2. Lock the spinlock in the ingress policer struct.
3. Set the ovsrcu pointer
4. Call ovsrcu_synchronize to that all threads see that the policer is locked 
(Stop threads from accessing the ingress policer)
5. Process the packets in the meter as usual.
6. Unlock the spinlock.
7. Set the ovsrcu pointer
6. Synchronize again? (So that threads can access the ingress policer again)

For destroying the ingress policer

1. Get the rcu ingress_policer pointer.
2. Lock the spinlock in the ingress policer struct.
3. Set the ovsrcu pointer
4. Call ovsrcu_synchronize to that all threads see that the policer is locked 
(Stop threads from accessing the meter)
5. Destroy the ingress policer.
6. Unlock the spinlock - if the spinlock is embedded in the ingress policer 
struct we have a problem here as it cannot  be free now, the struct has been 
destroyed.
7. Set the ovsrcu pointer
8. Synchronize again? (So that threads can see the ingress policer point for 
the netdev is now null)

Thanks
Ian

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

Re: [ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.

[Aaron Conole]

Joe Stringer  writes:

> Although tests ideally also stick to shorter line lengths, it is very
> common for fixed text blocks like flows or large packets to be specified
> within tests. Checkpatch shouldn't complain about cases like these.
>
> Signed-off-by: Joe Stringer 
> ---
> v2: Broaden the set of blacklisted files to not check.
> ---
>  utilities/checkpatch.py | 17 -
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index d9011f370816..83d14e89269e 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -56,6 +56,12 @@ skip_trailing_whitespace_check = False
>  skip_block_whitespace_check = False
>  skip_signoff_check = False
>  
> +# Don't enforce character limit on files that include these characters in 
> their
> +# name, as they may have legitimate reasons to have longer lines.
> +#
> +# Python isn't checked as flake8 performs these checks during build.
> +line_length_blacklist = ['.am', '.at', 'etc', '.in', '.mk', '.patch', '.py']
> +
>  
>  def is_added_line(line):
>  """Returns TRUE if the line in question is an added line.
> @@ -99,14 +105,23 @@ def ovs_checkpatch_parse(text):
>  co_authors = []
>  parse = 0
>  current_file = ''
> +previous_file = ''
>  scissors = re.compile(r'^[\w]*---[\w]*')
>  hunks = re.compile('^(---|\+\+\+) (\S+)')
>  is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
>re.I | re.M | re.S)
>  is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
>re.I | re.M | re.S)
> +skip_line_length_check = False
>  
>  for line in text.split('\n'):
> +if current_file is not previous_file:
> +previous_file = current_file
> +for filetype in line_length_blacklist:
> +if filetype in current_file:
> +skip_line_length_check = True
> +break

You need an else condition here; otherwise a patch which orders files

.c
[blacklisted]
.c

will skip the second .c length line checks.

Incremental patch could look something like (completely untested)
below. Thanks for doing this.

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 83d14e8..1579c1f 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -121,6 +121,9 @@ def ovs_checkpatch_parse(text):
 if filetype in current_file:
 skip_line_length_check = True
 break
+else:
+skip_line_length_check = False
 
 lineno = lineno + 1
 if len(line) <= 0:

> +
>  lineno = lineno + 1
>  if len(line) <= 0:
>  continue
> @@ -154,7 +169,7 @@ def ovs_checkpatch_parse(text):
>  if trailing_whitespace_or_crlf(line[1:]):
>  print_line = True
>  print_warning("Line has trailing whitespace", lineno)
> -if len(line[1:]) > 79:
> +if len(line[1:]) > 79 and not skip_line_length_check:
>  print_line = True
>  print_warning("Line is greater than 79-characters long",
>lineno)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v5] datapath-windows: Hot add CPU support.

[Sorin Vinturis]

Hot add CPU is the ability to dynamically add CPUs to a running
system. Adding CPUs can occur physically by adding new hardware,
logically by online hardware partitioning, or virtually through
a virtualization layer.

This patch add support to reallocate any per-cpu resources, in
case a new processor is added.

Signed-off-by: Sorin Vinturis 
Reported-by: Sorin Vinturis 
Reported-at: https://github.com/openvswitch/ovs-issues/issues/112
Acked-by: Paul-Daniel Boca 
---
v2: Correctly reallocate per-cpu data structures when a new processor
is about to be added to the system.
v4: Per-cpu variables are allocated for the maximum number of processors
supported by the system.
v5: OvsCleanup is performed after deregistering filter driver from NDIS.
Added acked.
---
 datapath-windows/ovsext/Datapath.c |  13 +++--
 datapath-windows/ovsext/Datapath.h |   2 +-
 datapath-windows/ovsext/Driver.c   |  11 ++--
 datapath-windows/ovsext/Recirc.c   | 100 -
 datapath-windows/ovsext/Recirc.h   |  45 +++--
 datapath-windows/ovsext/Util.c |  34 -
 datapath-windows/ovsext/Util.h |  20 +++-
 7 files changed, 117 insertions(+), 108 deletions(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 464fa97..8c0c246 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -379,26 +379,29 @@ FreeUserDumpState(POVS_OPEN_INSTANCE instance)
 }
 }
 
-VOID
+NDIS_STATUS
 OvsInit()
 {
+NDIS_STATUS status = NDIS_STATUS_SUCCESS;
+
 gOvsCtrlLock = 
 NdisAllocateSpinLock(gOvsCtrlLock);
 OvsInitEventQueue();
-OvsDeferredActionsQueueAlloc();
-OvsDeferredActionsLevelAlloc();
+
+status = OvsPerCpuDataInit();
+
+return status;
 }
 
 VOID
 OvsCleanup()
 {
+OvsPerCpuDataCleanup();
 OvsCleanupEventQueue();
 if (gOvsCtrlLock) {
 NdisFreeSpinLock(gOvsCtrlLock);
 gOvsCtrlLock = NULL;
 }
-OvsDeferredActionsQueueFree();
-OvsDeferredActionsLevelFree();
 }
 
 VOID
diff --git a/datapath-windows/ovsext/Datapath.h 
b/datapath-windows/ovsext/Datapath.h
index 2c61d82..09e233f 100644
--- a/datapath-windows/ovsext/Datapath.h
+++ b/datapath-windows/ovsext/Datapath.h
@@ -65,7 +65,7 @@ typedef struct _OVS_OPEN_INSTANCE {
 
 NDIS_STATUS OvsCreateDeviceObject(NDIS_HANDLE ovsExtDriverHandle);
 VOID OvsDeleteDeviceObject();
-VOID OvsInit();
+NDIS_STATUS OvsInit();
 VOID OvsCleanup();
 
 POVS_OPEN_INSTANCE OvsGetOpenInstance(PFILE_OBJECT fileObject,
diff --git a/datapath-windows/ovsext/Driver.c b/datapath-windows/ovsext/Driver.c
index 853886e..80979ea 100644
--- a/datapath-windows/ovsext/Driver.c
+++ b/datapath-windows/ovsext/Driver.c
@@ -96,7 +96,10 @@ DriverEntry(PDRIVER_OBJECT driverObject,
 UNREFERENCED_PARAMETER(registryPath);
 
 /* Initialize driver associated data structures. */
-OvsInit();
+status = OvsInit();
+if (status != NDIS_STATUS_SUCCESS) {
+goto cleanup;
+}
 
 gOvsExtDriverObject = driverObject;
 
@@ -175,12 +178,12 @@ OvsExtUnload(struct _DRIVER_OBJECT *driverObject)
 {
 UNREFERENCED_PARAMETER(driverObject);
 
-/* Release driver associated data structures. */
-OvsCleanup();
-
 OvsDeleteDeviceObject();
 
 NdisFDeregisterFilterDriver(gOvsExtDriverHandle);
+
+/* Release driver associated data structures. */
+OvsCleanup();
 }
 
 
diff --git a/datapath-windows/ovsext/Recirc.c b/datapath-windows/ovsext/Recirc.c
index 86e6f51..2febf06 100644
--- a/datapath-windows/ovsext/Recirc.c
+++ b/datapath-windows/ovsext/Recirc.c
@@ -18,71 +18,61 @@
 #include "Flow.h"
 #include "Jhash.h"
 
-static POVS_DEFERRED_ACTION_QUEUE ovsDeferredActionQueue = NULL;
-static UINT32* ovsDeferredActionLevel = NULL;
-
 /*
  * --
- * OvsDeferredActionsQueueAlloc --
- * The function allocates per-cpu deferred actions queue.
+ * '_OVS_DEFERRED_ACTION_QUEUE' structure is responsible for keeping track of
+ * all deferred actions. The maximum number of deferred actions should not
+ * exceed 'DEFERRED_ACTION_QUEUE_SIZE'.
  * --
  */
-BOOLEAN
-OvsDeferredActionsQueueAlloc()
-{
-ovsDeferredActionQueue =
-OvsAllocateMemoryPerCpu(sizeof(*ovsDeferredActionQueue),
-OVS_RECIRC_POOL_TAG);
-if (!ovsDeferredActionQueue) {
-return FALSE;
-}
-return TRUE;
-}
+typedef struct _OVS_DEFERRED_ACTION_QUEUE {
+UINT32  head;
+UINT32  tail;
+OVS_DEFERRED_ACTION deferredActions[DEFERRED_ACTION_QUEUE_SIZE];
+} OVS_DEFERRED_ACTION_QUEUE, *POVS_DEFERRED_ACTION_QUEUE;
 
-/*
- * --
- * OvsDeferredActionsQueueFree --
- * The function frees 

Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router

[Mickey Spiegel]

See comments inline

Mickey


-Guru Shetty  wrote: -
>To: Mickey Spiegel/San Jose/IBM@IBMUS
>From: Guru Shetty 
>Date: 04/06/2016 05:58PM
>Cc: ovs dev , Shi Xin Ruan 
>Subject: Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router
>
>
>
>On 6 April 2016 at 16:55, Mickey Spiegel  wrote:
>>Steve and Guru,
>> 
>> I am not all that concerned about the "valid" column, but I do think that we 
>> will need a different additional column in the near future for output port.
>> 
>> There are three different motivations for allowing output port to be 
>> specified in the static route:
>> 1) In order to support static routes with a link local next hop. If a link 
>> local next hop is used, it is possible that the same link local address 
>> appears on different router ports with different meanings. By specifying the 
>> port, this disambiguates the specific link local next hop that was desired.
>> Note: Neutron does not yet support static routes with link local next hop. 
>> We need to drive the feature in Neutron as well, optionally allowing a 
>> router interface to be specified in addition to the next hop.
>> 2) This feature should not really be specific to static routes, it should 
>> also apply to dynamic routes when we add that in the future. Basically 
>> anything that looks up an IP address prefix and returns a next hop and 
>> optionally an output port. There are cases where explicitly specifying the 
>> output port makes sense.
> For point 1 and 2, I am not sure whether we should do anything till there is 
> code in ovn-northd that actually uses it.

[Mickey] Point of clarification, the proposal is to add an output port column 
to the static route table in northd. The question is not whether there is code 
in ovn-northd that uses it, it is whether there is code at the CMS layer that 
fills this column. Both the features in points 1 and 2 would make use of this 
column.

Even if we don't add this column now, if you don't have a separate static route 
table, it will make such an addition difficult in the future.

>> 3) In order to optimize processing of the routing recursion (Steve's code 
>> loops over the router's ports in ovn-northd.c to carry out this routing 
>> recursion), we might want to do it above OVN in an event triggered manner, 
>> rather than every time ovn-northd.c recalculates the flows that it places 
>> into the southbound database.
> I don't think I understand the above point. The static_route I have in mind 
> need not recursively look through routers. All they need is to see whether 
> the router peer has the next hop IP address and the packet is just sent to 
> that router. From there on it is a fresh start. 

[Mickey] By routing recursion I just meant a small walk through all of the 
router ports to find which router port has the next hop address.

In Steve's diff for ovn-northd.c, in build_static_route_flow which is called 
for every static route, the section starting with:

+/* find the outgoing port */
+out_port = NULL;
+len = 0;
+for (int i = 0; i < od->nbr->n_ports; i++){

>> For these reasons, I prefer to keep a separate table for static routes.
>> 
>> Mickey
 
 
 -"dev"  wrote: -
 To: Shi Xin Ruan 
 From: Guru Shetty
 Sent by: "dev"
 Date: 04/04/2016 08:41AM
 Cc: ovs dev 
 Subject: Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router
 
 
On 31 March 2016 at 19:11, Shi Xin Ruan  wrote:
 
 > Thanks Guru.
 >
 > The column "valid" will indicate whether the routes has been transfer into
 > logical flow.
 > Thinking about this case, deleting the logical router port which is out
 > going interface of some static routes.
 > The first possible way is preventing the deleting, the second way is
 > removing the stactic routing from logical flow.
 >
 [Trying this again as my previous reply got rejected by the mailing list]
 I feel this is an unnecessary complication (unless it becomes a real use
 case). Let us start with not adding a new table and try to do this with a
 column. If you are fine with it, would you mind re-spinning a non-RFC patch
 with a unit test? If you want, I can provide the unit tests. If you prefer
 that I do the entire thing, I am happy with that too.
 
 
 
 >
 >
 > From my points, both can work fine and has their advantages.
 >
 >
 
___
 dev mailing list
 dev@openvswitch.org
 http://openvswitch.org/mailman/listinfo/dev
 
 ___
 dev mailing list
 dev@openvswitch.org
 http://openvswitch.org/mailman/listinfo/dev
 
 

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

Re: [ovs-dev] [PATCH v4] datapath-windows: Hot add CPU support.

[Paul Boca]

Looks good to me!

Acked-by: Paul-Daniel Boca 

-Original Message-
From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Sorin Vinturis
Sent: Wednesday, April 6, 2016 10:00 PM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH v4] datapath-windows: Hot add CPU support.

Hot add CPU is the ability to dynamically add CPUs to a running
system. Adding CPUs can occur physically by adding new hardware,
logically by online hardware partitioning, or virtually through
a virtualization layer.

This patch add support to reallocate any per-cpu resources, in
case a new processor is added.

Signed-off-by: Sorin Vinturis 
Reported-by: Sorin Vinturis 
Reported-at: https://github.com/openvswitch/ovs-issues/issues/112
---
v2: Correctly reallocate per-cpu data structures when a new processor
is about to be added to the system.
v3: Per-cpu variables are allocated for the maximum number of processors
supported by the system.
---
 datapath-windows/ovsext/Datapath.c |  13 +++--
 datapath-windows/ovsext/Datapath.h |   2 +-
 datapath-windows/ovsext/Driver.c   |   5 +-
 datapath-windows/ovsext/Recirc.c   | 100 -
 datapath-windows/ovsext/Recirc.h   |  45 +++--
 datapath-windows/ovsext/Util.c |  34 -
 datapath-windows/ovsext/Util.h |  20 +++-
 7 files changed, 114 insertions(+), 105 deletions(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 464fa97..8c0c246 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -379,26 +379,29 @@ FreeUserDumpState(POVS_OPEN_INSTANCE instance)
 }
 }
 
-VOID
+NDIS_STATUS
 OvsInit()
 {
+NDIS_STATUS status = NDIS_STATUS_SUCCESS;
+
 gOvsCtrlLock = 
 NdisAllocateSpinLock(gOvsCtrlLock);
 OvsInitEventQueue();
-OvsDeferredActionsQueueAlloc();
-OvsDeferredActionsLevelAlloc();
+
+status = OvsPerCpuDataInit();
+
+return status;
 }
 
 VOID
 OvsCleanup()
 {
+OvsPerCpuDataCleanup();
 OvsCleanupEventQueue();
 if (gOvsCtrlLock) {
 NdisFreeSpinLock(gOvsCtrlLock);
 gOvsCtrlLock = NULL;
 }
-OvsDeferredActionsQueueFree();
-OvsDeferredActionsLevelFree();
 }
 
 VOID
diff --git a/datapath-windows/ovsext/Datapath.h 
b/datapath-windows/ovsext/Datapath.h
index 2c61d82..09e233f 100644
--- a/datapath-windows/ovsext/Datapath.h
+++ b/datapath-windows/ovsext/Datapath.h
@@ -65,7 +65,7 @@ typedef struct _OVS_OPEN_INSTANCE {
 
 NDIS_STATUS OvsCreateDeviceObject(NDIS_HANDLE ovsExtDriverHandle);
 VOID OvsDeleteDeviceObject();
-VOID OvsInit();
+NDIS_STATUS OvsInit();
 VOID OvsCleanup();
 
 POVS_OPEN_INSTANCE OvsGetOpenInstance(PFILE_OBJECT fileObject,
diff --git a/datapath-windows/ovsext/Driver.c b/datapath-windows/ovsext/Driver.c
index 853886e..2771015 100644
--- a/datapath-windows/ovsext/Driver.c
+++ b/datapath-windows/ovsext/Driver.c
@@ -96,7 +96,10 @@ DriverEntry(PDRIVER_OBJECT driverObject,
 UNREFERENCED_PARAMETER(registryPath);
 
 /* Initialize driver associated data structures. */
-OvsInit();
+status = OvsInit();
+if (status != NDIS_STATUS_SUCCESS) {
+goto cleanup;
+}
 
 gOvsExtDriverObject = driverObject;
 
diff --git a/datapath-windows/ovsext/Recirc.c b/datapath-windows/ovsext/Recirc.c
index 86e6f51..2febf06 100644
--- a/datapath-windows/ovsext/Recirc.c
+++ b/datapath-windows/ovsext/Recirc.c
@@ -18,71 +18,61 @@
 #include "Flow.h"
 #include "Jhash.h"
 
-static POVS_DEFERRED_ACTION_QUEUE ovsDeferredActionQueue = NULL;
-static UINT32* ovsDeferredActionLevel = NULL;
-
 /*
  * --
- * OvsDeferredActionsQueueAlloc --
- * The function allocates per-cpu deferred actions queue.
+ * '_OVS_DEFERRED_ACTION_QUEUE' structure is responsible for keeping track of
+ * all deferred actions. The maximum number of deferred actions should not
+ * exceed 'DEFERRED_ACTION_QUEUE_SIZE'.
  * --
  */
-BOOLEAN
-OvsDeferredActionsQueueAlloc()
-{
-ovsDeferredActionQueue =
-OvsAllocateMemoryPerCpu(sizeof(*ovsDeferredActionQueue),
-OVS_RECIRC_POOL_TAG);
-if (!ovsDeferredActionQueue) {
-return FALSE;
-}
-return TRUE;
-}
+typedef struct _OVS_DEFERRED_ACTION_QUEUE {
+UINT32  head;
+UINT32  tail;
+OVS_DEFERRED_ACTION deferredActions[DEFERRED_ACTION_QUEUE_SIZE];
+} OVS_DEFERRED_ACTION_QUEUE, *POVS_DEFERRED_ACTION_QUEUE;
 
-/*
- * --
- * OvsDeferredActionsQueueFree --
- * The function frees per-cpu deferred actions queue.
- * --
- */
-VOID
-OvsDeferredActionsQueueFree()
-{
-OvsFreeMemoryWithTag(ovsDeferredActionQueue,

[ovs-dev] (no subject)

[The Post Office]

The original message was received at Thu, 7 Apr 2016 13:49:43 +0700
from 54.115.247.242

- The following addresses had permanent fatal errors -


- Transcript of session follows -
... while talking to host openvswitch.org.:
550 5.1.2 ... Host unknown (Name server: host not found)

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

Re: [ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

[Numan Siddique]

On Thu, Apr 7, 2016 at 3:37 AM, Justin Pettit  wrote:

> I think you might be able to write a slightly simpler patch by using
> ip_format_masked() like the following:
>
> -=-=-=-=-=-=-=-=-=-
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 4b1d611..890b17c 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1179,8 +1179,11 @@ build_port_security_nd(struct ovn_port *op, struct
> hmap *
>  if (ps.n_ipv4_addrs) {
>  ds_put_cstr(, " && (");
>  for (size_t i = 0; i < ps.n_ipv4_addrs; i++) {
> -ds_put_format(, "arp.spa == "IP_FMT" || ",
> -  IP_ARGS(ps.ipv4_addrs[i].addr));
> +ds_put_cstr(, "arp.spa == ");
> +ip_format_masked(ps.ipv4_addrs[i].addr,
> +
>  be32_prefix_mask(ps.ipv4_addrs[i].plen),
> + );
> +ds_put_cstr(, " || ");
>  }
>  ds_chomp(, ' ');
>  ds_chomp(, '|');
> @@ -1264,7 +1267,10 @@ build_port_security_ip(enum ovn_pipeline pipeline,
> struct
>  }
>
>  for (int i = 0; i < ps.n_ipv4_addrs; i++) {
> -ds_put_format(, IP_FMT", ",
> IP_ARGS(ps.ipv4_addrs[i].addr
> +ip_format_masked(ps.ipv4_addrs[i].addr,
> + be32_prefix_mask(ps.ipv4_addrs[i].plen),
> + );
> +ds_put_cstr(, ", ");
>  }
>
>  /* Replace ", " by "}". */
> -=-=-=-=-=-=-=-=-=-
>
> What do you think?
>
>
​Thanks for the comments Justin. I tried a similar approach. It will not
work in the cases where the port security address also has a prefix defined.
For example with port security - "00:00:00:00:00:02 10.0.0.4/24", the ovn
lexer parser is throwing the below error,

---
lflow|WARN|error parsing match "outport == "sw0-port2" && eth.dst ==
00:00:00:00:00:02 && ip4.dst == {255.255.255.255, 224.0.0.0/4, 10.0.0.4/24}":
Value contains unmasked 1-bits.
--

Thats the reason I am calling 'is_host_part_zero()' and putting the prefix
only if host part is zero.

​


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