[ovs-dev] Returned mail: see transcript for details

[Mail Administrator]

The original message was received at Wed, 12 Aug 2015 15:58:59 +0800 from 
[77.200.136.166]

- The following addresses had permanent fatal errors -
dev@openvswitch.org

- Transcript of session follows -
... while talking to openvswitch.org.:
>>> MAIL FROM:"Mail Administrator" 
<<< 508 "Mail Administrator" ... User blacklisted

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

Re: [ovs-dev] [PATCH 1/2] dpif-netdev: Purge all ukeys when reconfigure pmd.

[Ilya Maximets]

Ok, but why don't just put dp->dp_purge_cb(dp->dp_purge_aux) right after
dp_netdev_destroy_all_pmds(dp) in dpif_netdev_pmd_set() ? In that case,
disabling of upcalls will not be necessary.

P.S. The second letter of my name is 'L'.

On 11.08.2015 21:06, Alex Wang wrote:
> Thx, IIya for the review, please see my reply below,
> 
> 
> 
> On Tue, Aug 11, 2015 at 4:41 AM, Ilya Maximets  > wrote:
> 
> On 11.08.2015 05:47, Alex Wang wrote:
> > When dpdk configuration changes, all pmd threads are recreated
> > and rx queues of each port are reloaded.  After this process,
> > rx queue could be mapped to a different pmd thread other than
> > the one before reconfiguration.  However, this is totally
> > transparent to ofproto layer modules.  So, if the ofproto-dpif-upcall
> > module still holds ukeys generated before pmd thread recreation,
> > this old ukey will collide with the ukey for the new upcalls
> > from same traffic flow, causing flow installation failure.
> >
> > To fix the bug, this commit adds a new call-back function
> > in dpif layer for notifying upper layer the purging of datapath
> > (e.g. pmd threads recreation in dpif-netdev).  So, the
> > ofproto-dpif-upcall module can react properly with deleting
> > all the ukeys and with collecting all flows' last stats.
> >
> > Reported-by: Ilya Maximets  >
> > Signed-off-by: Alex Wang mailto:al...@nicira.com>>
> > ---
> >  lib/dpif-netdev.c |   25 +
> >  lib/dpif-netlink.c|1 +
> >  lib/dpif-provider.h   |   11 ++-
> >  lib/dpif.c|8 
> >  lib/dpif.h|   12 
> >  ofproto/ofproto-dpif-upcall.c |   16 
> >  6 files changed, 72 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index c144352..a54853f 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -203,6 +203,11 @@ struct dp_netdev {
> >  upcall_callback *upcall_cb;  /* Callback function for executing 
> upcalls. */
> >  void *upcall_aux;
> >
> > +/* Callback function for notifying the purging of dp flows (during
> > + * reseting pmd threads). */
> > +dp_purge_callback *dp_purge_cb;
> > +void *dp_purge_aux;
> > +
> >  /* Stores all 'struct dp_netdev_pmd_thread's. */
> >  struct cmap poll_threads;
> >
> > @@ -223,6 +228,8 @@ struct dp_netdev {
> >
> >  static struct dp_netdev_port *dp_netdev_lookup_port(const struct 
> dp_netdev *dp,
> >  odp_port_t);
> > +static void dp_netdev_disable_upcall(struct dp_netdev *);
> > +static void dp_netdev_enable_upcall(struct dp_netdev *);
> >
> >  enum dp_stat_type {
> >  DP_STAT_EXACT_HIT,  /* Packets that had an exact match 
> (emc). */
> > @@ -2440,10 +2447,18 @@ dpif_netdev_pmd_set(struct dpif *dpif, unsigned 
> int n_rxqs, const char *cmask)
> >  free(dp->pmd_cmask);
> >  dp->pmd_cmask = cmask ? xstrdup(cmask) : NULL;
> >
> > +/* Disables upcall and notifies higher layer about the incoming
> > + * datapath purging. */
> > +dp_netdev_disable_upcall(dp);
> > +dp->dp_purge_cb(dp->dp_purge_aux);
> > +
> >  /* Restores the non-pmd. */
> >  dp_netdev_set_nonpmd(dp);
> >  /* Restores all pmd threads. */
> >  dp_netdev_reset_pmd_threads(dp);
> > +
> > +/* Enables upcall after pmd reconfiguration. */
> > +dp_netdev_enable_upcall(dp);
> >  }
> 
> It is better to enable upcalls before restoring PMD threads to prevent 
> dropping of packets.
> 
> 
> 
> Sorry, the comments are misleading, the "dp_netdev_reset_pmd_threads()"
> is for recreating all pmd threads (not exactly restoring).  If we move the
> "dp_netdev_enable_upcall()" before the function, then upcall will be enabled
> before destroying all existing pmd threads, and we will have the same issue
> again (the existing pmd threads will try installing flows/creating ukeys 
> before
> getting destroyed).
> 
> Also, since recreating pmd threads is considered a major configuration
> change and is disruptive, I think it should be okay to resume upcall handling
> right after all new pmd threads are created.
> 
> 
> >
> >  return 0;
> > @@ -3419,6 +3434,15 @@ struct dp_netdev_execute_aux {
> >  };
> >
> >  static void
> > +dpif_netdev_register_dp_purge_cb(struct dpif *dpif, dp_purge_callback 
> *cb,
> > + void *aux)
> > +{
> > +struct dp_netdev *dp = get_dp_netdev(dpif);
> > +dp->dp_purge_aux = aux;
> > +dp->dp

[ovs-dev] [PATCH V1 Resend 10/10] net: Drop unlikely before IS_ERR(_OR_NULL)

[Viresh Kumar]

IS_ERR(_OR_NULL) already contain an 'unlikely' compiler flag and there
is no need to do that again from its callers. Drop it.

Acked-by: Neil Horman 
Signed-off-by: Viresh Kumar 
---
 net/openvswitch/datapath.c | 2 +-
 net/sctp/socket.c  | 2 +-
 net/socket.c   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index ffe984f5b95c..a515e338cade 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1143,7 +1143,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct 
genl_info *info)
info, OVS_FLOW_CMD_NEW, false,
ufid_flags);
 
-   if (unlikely(IS_ERR(reply))) {
+   if (IS_ERR(reply)) {
error = PTR_ERR(reply);
goto err_unlock_ovs;
}
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 17bef01b9aa3..897c01c029ca 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4475,7 +4475,7 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int 
len, char __user *optval
}
 
newfile = sock_alloc_file(newsock, 0, NULL);
-   if (unlikely(IS_ERR(newfile))) {
+   if (IS_ERR(newfile)) {
put_unused_fd(retval);
sock_release(newsock);
return PTR_ERR(newfile);
diff --git a/net/socket.c b/net/socket.c
index 9963a0b53a64..dd2c247c99e3 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -373,7 +373,7 @@ struct file *sock_alloc_file(struct socket *sock, int 
flags, const char *dname)
 
file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
  &socket_file_ops);
-   if (unlikely(IS_ERR(file))) {
+   if (IS_ERR(file)) {
/* drop dentry, keep inode */
ihold(d_inode(path.dentry));
path_put(&path);
@@ -1303,7 +1303,7 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, 
protocol,
}
 
newfile1 = sock_alloc_file(sock1, flags, NULL);
-   if (unlikely(IS_ERR(newfile1))) {
+   if (IS_ERR(newfile1)) {
err = PTR_ERR(newfile1);
goto out_put_unused_both;
}
@@ -1467,7 +1467,7 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user 
*, upeer_sockaddr,
goto out_put;
}
newfile = sock_alloc_file(newsock, flags, 
sock->sk->sk_prot_creator->name);
-   if (unlikely(IS_ERR(newfile))) {
+   if (IS_ERR(newfile)) {
err = PTR_ERR(newfile);
put_unused_fd(newfd);
sock_release(newsock);
-- 
2.4.0

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

Re: [ovs-dev] [PATCH RFC] coverage: fix coverage accounting for pmd threads

[Ilya Maximets]

I agree, that this solution is much more simple, but I don't like this race for
coverage_mutex. 
Why this all done so?
I mean, why we should run coverage_clear every second? What if we change type of
thread_local unsigned int counter_##COUNTER from unsigned int to unsigned long 
long
and will call coverage_clear() in coverage_read() ? (May be possible with my 
patch
applied) Also, we may collect stats for all threads separately.

On 12.08.2015 08:01, Alex Wang wrote:
> Just trying share an alternative, could we still do the coverage clear inside 
> 'pmd_thread_main()'.  However,
> instead of contending for the 'coverage_mutex', use 'ovs_mutex_trylock()'.  
> Since pmd thread runs in infinite
> loop, it can constantly try grabbing the lock and will eventually acquire it.
> 
> Something like this:
> 
> diff --git a/lib/coverage.c b/lib/coverage.c
> index 6121956..9f23f99 100644
> --- a/lib/coverage.c
> +++ b/lib/coverage.c
> @@ -272,6 +272,37 @@ coverage_clear(void)
>  }
>  }
> 
> +/* Runs approximately every COVERAGE_CLEAR_INTERVAL amount of time to
> + * synchronize per-thread counters with global counters.  Exits immediately
> + * without updating the 'thread_time' if cannot acquire the 'coverage_mutex'.
> + * This is to avoid lock contention for some performance-critical threads.
> + */
> +void
> +coverage_try_clear(void)
> +{
> +long long int now, *thread_time;
> +
> +now = time_msec();
> +thread_time = coverage_clear_time_get();
> +
> +/* Initialize the coverage_clear_time. */
> +if (*thread_time == LLONG_MIN) {
> +*thread_time = now + COVERAGE_CLEAR_INTERVAL;
> +}
> +
> +if (now >= *thread_time) {
> +if (ovs_mutex_trylock(&coverage_mutex)) {
> +size_t i;
> +for (i = 0; i < n_coverage_counters; i++) {
> +struct coverage_counter *c = coverage_counters[i];
> +c->total += c->count();
> +}
> +ovs_mutex_unlock(&coverage_mutex);
> +*thread_time = now + COVERAGE_CLEAR_INTERVAL;
> +}
> +}
> +}
> +
>  /* Runs approximately every COVERAGE_RUN_INTERVAL amount of time to update 
> the
>   * coverage counters' 'min' and 'hr' array.  'min' array is for cumulating
>   * per second counts into per minute count.  'hr' array is for cumulating per
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c144352..96884ec 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2695,6 +2695,7 @@ reload:
> 
>  lc = 0;
> 
> +coverage_try_clear();
>  emc_cache_slow_sweep(&pmd->flow_cache);
>  ovsrcu_quiesce();
> 
> 
> 
> On Tue, Aug 11, 2015 at 6:24 PM, Alex Wang  > wrote:
> 
> Hi IIya,
> 
> Thx a lot for writing up this fix,
> 
> The reason that you did not see the pmd related coverage counter stats
> (in "ovs-appctl coverage/show") is that we do not call coverage_clear()
> anywhere in pmd_thread_main().  This is in that the coverage_clear()
> requires holding the global mutex in the coverage module which can
> be super expensive for dpdk packet processing.
> 
> Want to admit that your fix is more complicated than I expect.  
> Especially,
> changing the global (extern) struct 'counter_##COUNTER' to per-thread
> and expanding the array size by number of threads times can also be 
> expensive for main thread...
> 
> Do you think there could be a simpler way to achieve this?  I'd like to 
> think
> about other possible solutions more,
> 
> Thanks,
> Alex Wang,
> 
> 
> On Mon, Aug 10, 2015 at 7:45 AM, Ilya Maximets  > wrote:
> 
> Currently coverage_counter_register() is called only from
> constructor functions at program initialization time by
> main thread. Coverage counter values are static and
> thread local.
> 
> This means, that all COVERAGE_INC() calls from pmd threads
> leads to increment of thread local variables of that threads
> that can't be accessed by anyone else. And they are not used
> in final coverage accounting.
> 
> Fix that by adding ability to add/remove coverage counters
> in runtime for newly created threads and counting coverage
> using counters from all threads.
> 
> Signed-off-by: Ilya Maximets  >
> ---
>  lib/coverage.c| 83 
> +--
>  lib/coverage.h| 27 --
>  lib/dpif-netdev.c |  4 ++-
>  3 files changed, 91 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/coverage.c b/lib/coverage.c
> index 6121956..2ae9e7a 100644
> --- a/lib/coverage.c
> +++ b/lib/coverage.c
> @@ -30,14 +30,22 @@ VLOG_DEFINE_THIS_MODULE(coverage);
> 
>  /* The coverage counters. */
>  static struct coverage_cou

Re: [ovs-dev] [PATCH] netdev-dpdk: Add some missing statistics.

[Puha, TimoX]

Hi,

From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Puha, TimoX
> From: Weglicki, MichalX
> > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Daniele Di
> Proietto
> > >Do you know how much it is going to impact the vhost throughput? Other than
> > this the change looks good to me.
> >
> > I compared the results based on (OVS head) vs (OVS head + patch) and seems
> > there is very small performance drop:
> > 4,763,016 (HEAD) vs 4,699,571(Patch).
> 
> This is also in line with my testing with various options. In general the 
> throughput
> drop has been a bit more than 1% on average in our physical port -> VM ->
> physical port test setup.

Any update on the fate of this patch? 

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

Re: [ovs-dev] [PATCH 1/2] dpif-netdev: Purge all ukeys when reconfigure pmd.

[Alex Wang]

On Wed, Aug 12, 2015 at 1:10 AM, Ilya Maximets 
wrote:

> Ok, but why don't just put dp->dp_purge_cb(dp->dp_purge_aux) right after
> dp_netdev_destroy_all_pmds(dp) in dpif_netdev_pmd_set() ? In that case,
> disabling of upcalls will not be necessary.
>
>
Hey Ilya,

Calling it before 'dp_netdev_destroy_all_pmds()' can also allow the update
of 'dpif-netdev' flows' stats before they are gone.

Also, since the pmd thread recreation change is deemed big configuration
change and enable/disable upcall is only one locking operation each, so I
think it will not matter very much.  Would like to hear your concerns,

Thanks,
Alex Wang,




> P.S. The second letter of my name is 'L'.
>
>
Apology~  for keeping using it,





> On 11.08.2015 21:06, Alex Wang wrote:
> > Thx, IIya for the review, please see my reply below,
> >
> >
> >
> > On Tue, Aug 11, 2015 at 4:41 AM, Ilya Maximets  > wrote:
> >
> > On 11.08.2015 05:47, Alex Wang wrote:
> > > When dpdk configuration changes, all pmd threads are recreated
> > > and rx queues of each port are reloaded.  After this process,
> > > rx queue could be mapped to a different pmd thread other than
> > > the one before reconfiguration.  However, this is totally
> > > transparent to ofproto layer modules.  So, if the
> ofproto-dpif-upcall
> > > module still holds ukeys generated before pmd thread recreation,
> > > this old ukey will collide with the ukey for the new upcalls
> > > from same traffic flow, causing flow installation failure.
> > >
> > > To fix the bug, this commit adds a new call-back function
> > > in dpif layer for notifying upper layer the purging of datapath
> > > (e.g. pmd threads recreation in dpif-netdev).  So, the
> > > ofproto-dpif-upcall module can react properly with deleting
> > > all the ukeys and with collecting all flows' last stats.
> > >
> > > Reported-by: Ilya Maximets  i.maxim...@samsung.com>>
> > > Signed-off-by: Alex Wang  al...@nicira.com>>
> > > ---
> > >  lib/dpif-netdev.c |   25 +
> > >  lib/dpif-netlink.c|1 +
> > >  lib/dpif-provider.h   |   11 ++-
> > >  lib/dpif.c|8 
> > >  lib/dpif.h|   12 
> > >  ofproto/ofproto-dpif-upcall.c |   16 
> > >  6 files changed, 72 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > index c144352..a54853f 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -203,6 +203,11 @@ struct dp_netdev {
> > >  upcall_callback *upcall_cb;  /* Callback function for
> executing upcalls. */
> > >  void *upcall_aux;
> > >
> > > +/* Callback function for notifying the purging of dp flows
> (during
> > > + * reseting pmd threads). */
> > > +dp_purge_callback *dp_purge_cb;
> > > +void *dp_purge_aux;
> > > +
> > >  /* Stores all 'struct dp_netdev_pmd_thread's. */
> > >  struct cmap poll_threads;
> > >
> > > @@ -223,6 +228,8 @@ struct dp_netdev {
> > >
> > >  static struct dp_netdev_port *dp_netdev_lookup_port(const struct
> dp_netdev *dp,
> > >  odp_port_t);
> > > +static void dp_netdev_disable_upcall(struct dp_netdev *);
> > > +static void dp_netdev_enable_upcall(struct dp_netdev *);
> > >
> > >  enum dp_stat_type {
> > >  DP_STAT_EXACT_HIT,  /* Packets that had an exact
> match (emc). */
> > > @@ -2440,10 +2447,18 @@ dpif_netdev_pmd_set(struct dpif *dpif,
> unsigned int n_rxqs, const char *cmask)
> > >  free(dp->pmd_cmask);
> > >  dp->pmd_cmask = cmask ? xstrdup(cmask) : NULL;
> > >
> > > +/* Disables upcall and notifies higher layer about the
> incoming
> > > + * datapath purging. */
> > > +dp_netdev_disable_upcall(dp);
> > > +dp->dp_purge_cb(dp->dp_purge_aux);
> > > +
> > >  /* Restores the non-pmd. */
> > >  dp_netdev_set_nonpmd(dp);
> > >  /* Restores all pmd threads. */
> > >  dp_netdev_reset_pmd_threads(dp);
> > > +
> > > +/* Enables upcall after pmd reconfiguration. */
> > > +dp_netdev_enable_upcall(dp);
> > >  }
> >
> > It is better to enable upcalls before restoring PMD threads to
> prevent dropping of packets.
> >
> >
> >
> > Sorry, the comments are misleading, the "dp_netdev_reset_pmd_threads()"
> > is for recreating all pmd threads (not exactly restoring).  If we move
> the
> > "dp_netdev_enable_upcall()" before the function, then upcall will be
> enabled
> > before destroying all existing pmd threads, and we will have the same
> issue
> > again (the existing pmd threads will try installing flows/creating 

Re: [ovs-dev] [PATCH RFC] coverage: fix coverage accounting for pmd threads

[Alex Wang]

Hey Ilya,

Thanks for the reply please see my comments inline,

Thanks,
Alex Wang,

On Wed, Aug 12, 2015 at 4:19 AM, Ilya Maximets 
wrote:

> I agree, that this solution is much more simple, but I don't like this
> race for
> coverage_mutex.
> Why this all done so?
>

Before implementing pmd threads, waiting on the ‘coverage_mutex’ is
affordable for all other threads.




> I mean, why we should run coverage_clear every second?


Because, we (the main thread) need to calculate the per second/minute/hour
rate..



> What if we change type of
> thread_local unsigned int counter_##COUNTER from unsigned int to unsigned
> long long
> and will call coverage_clear() in coverage_read() ? (May be possible with
> my patch
> applied) Also, we may collect stats for all threads separately.
>
>
There is one major issue with you change to 'COVERAGE_DEFNE()'

>  /* Defines COUNTER.  There must be exactly one such definition
at file scope
>   * within a program. */
>  #define COVERAGE_DEFINE(COUNTER)
\
>  DEFINE_STATIC_PER_THREAD_DATA(unsigned int,
   \
>counter_##COUNTER, 0);
\
> -static unsigned int COUNTER##_count(void)
   \
> +static unsigned int COUNTER##_count(unsigned int i)
   \
>  {
   \
> -unsigned int *countp = counter_##COUNTER##_get();
   \
> +volatile unsigned int *countp = coverage_val[i];
\
>  unsigned int count = *countp;
   \
>  *countp = 0;
\
>  return count;
   \
> @@ -72,11 +77,17 @@ void coverage_counter_register(struct
coverage_counter*);
>  {
   \
>  *counter_##COUNTER##_get() += n;
\
>  }
   \
> -extern struct coverage_counter counter_##COUNTER;
   \
> -struct coverage_counter counter_##COUNTER
   \
> +extern thread_local struct coverage_counter
counter_##COUNTER;  \
> +thread_local struct coverage_counter counter_##COUNTER
\
>  = { #COUNTER, COUNTER##_count, 0, 0, {0}, {0} };



Here a minor issue is that 'thread_local' is not portable.  You need to
define
it using the ovs wrappers documented in lib/ovs-thread.h



> -OVS_CONSTRUCTOR(COUNTER##_init) {
   \
> +static void COUNTER##_initializer(void) {
   \
>  coverage_counter_register(&counter_##COUNTER);
\
> +coverage_val[n_coverage_counters - 1] =
   \
> +
counter_##COUNTER##_get();   \
> +}
   \
> +OVS_CONSTRUCTOR(COUNTER##_init) {
   \
> +coverage_initializer_register(&COUNTER##_initializer);
\
> +COUNTER##_initializer();
\
>  }
>

We really should not have main thread accessing the per thread data of other
thread.  As mentioned in the comments in lib/ovs-thread.h,

"""
 * - The thread_local feature newly defined in C11  works
with
 *   any data type and initializer, and it is fast.  thread_local does
not
 *   require once-only initialization like pthread_key_t. * C11 does
not*
* *   define what happens if one attempts to access a thread_local
object*
* *   from a thread other than the one to which that object belongs.*
 There
 *   is no provision to call a user-specified destructor when a thread
 *   ends.  Typical implementations allow for an arbitrary amount of
 *   thread_local storage, but statically allocated only.
"""

We really should not play with any undefined behavior.  (since has no way
to guarantee the portability).  In other words, we really should not use
thread local variables, if we ever want to re-architect the coverage module
to have main thread taking care of collecting the stats.

So, we think the try lock solution should do good now,

What do you think?

Thanks,
Alex Wang,



> On 12.08.2015 08:01, Alex Wang wrote:
> > Just trying share an alternative, could we still do the coverage clear
> inside 'pmd_thread_main()'.  However,
> > instead of contending for the 'coverage_mutex', use
> 'ovs_mutex_trylock()'.  Since pmd thread runs in infinite
> > loop, it can constantly try grabbing the lock and will eventually
> acquire it.
> >
> > Something like this:
> >
> > diff --git a/lib/coverage.c b/lib/coverage.c
> > index 6121956..9f23f99 100644
> > --- a/lib/coverage.c
> > +++ b/lib/coverage.c
> > @@ -272,6 +272,37 @@ coverage_clear(void)
> >  }
> >  }
> >
> > +/* Runs approximately every COVERAGE_CLEAR_INTERVAL amount of time to
> > + * synchronize per-thread counters with global counters.  Exits
> immediately
> > + * without updating the 'thread_time' if cannot acquire the
> 'coverage_mutex'.
> > + * This is to avoid lock

[ovs-dev] [PATCHv2 0/5] Extend system tests to include tunnel test.

[Joe Stringer]

Add various minor fixups and improvements to the system tests, and a basic
tunnel sanity test for vxlan.

v2: Applied one patch
Tested series on Ubuntu 14.04.2
Better detection of tool support for tunnels

Joe Stringer (5):
  system-common-macros: Allow quotes in NS_EXEC().
  system-traffic: Check ping-by-ping output.
  system-macros: Create ADD_BR variant.
  system-macros: Don't explicitly remove bridge.
  kmod-traffic: Add basic vxlan tunnel sanity test.

 tests/system-common-macros.at| 54 ++-
 tests/system-kmod-macros.at  | 19 ++
 tests/system-traffic.at  | 80 
 tests/system-userspace-macros.at |  9 ++---
 4 files changed, 126 insertions(+), 36 deletions(-)

-- 
2.1.4

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

[ovs-dev] [PATCHv2 1/5] system-common-macros: Allow quotes in NS_EXEC().

[Joe Stringer]

This allows arbitrary commands to be passed into the NS_EXEC macro to be
executed within a namespace, including commands that have quotes and
commands chained together.

Signed-off-by: Joe Stringer 
---
Daniele, thanks for the suggestion, this seems to work pretty well. My
only complaint is that the test logs seem to end up with a bunch of
extra whitespaces printed after these commands. Do you have any insight
into why this might be?
---
 tests/system-common-macros.at | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 1321e58..198592c 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -25,7 +25,9 @@ m4_define([ADD_NAMESPACES],
 #
 # Execute 'command' in 'namespace'
 m4_define([NS_EXEC],
-[ip netns exec $1 bash -c "$2"]
+[ip netns exec $1 sh << NS_EXEC_HEREDOC
+$2
+NS_EXEC_HEREDOC]
 )
 
 # NS_CHECK_EXEC([namespace], [command], other_params...)
-- 
2.1.4

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

[ovs-dev] [PATCHv2 4/5] system-macros: Don't explicitly remove bridge.

[Joe Stringer]

Depending on the kernel in use, manually removing a bridge during
OVS_VSWITCHD_STOP can cause the kernel to send a route update that
refers to the bridge device which is being deleted. OVS can't make sense
of these messages, resulting in logs like the following:

route_table|DBG|Could not find interface name[702]: No such device or
address
netlink_notifier|WARN|received bad netlink message

One such example kernel is the Ubuntu 3.16.0-33-generic linux package.

The bridge doesn't need to be explicitly removed, as the OVSDB
configuration for each test will be cleared before executing the next.
Drop this extra step.

Signed-off-by: Joe Stringer 
---
An alternative would be to force test writers to add additional
boilerplate to ensure that they delete addresses from bridges before
calling OVS_VSWITCHD_STOP. This approach seems to be lower maintenance.
---
 tests/system-kmod-macros.at  | 3 +--
 tests/system-userspace-macros.at | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index a5aa5db..2e2bf4e 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -34,7 +34,6 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
 # invoked. They can be used to perform additional cleanups such as name space
 # removal.
 m4_define([OVS_TRAFFIC_VSWITCHD_STOP],
-  [AT_CHECK([ovs-vsctl del-br br0])
-   OVS_VSWITCHD_STOP([$1])
+  [OVS_VSWITCHD_STOP([$1])
AT_CHECK([:; $2])
   ])
diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index adc40c4..fca26f7 100644
--- a/tests/system-userspace-macros.at
+++ b/tests/system-userspace-macros.at
@@ -32,8 +32,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
 # invoked. They can be used to perform additional cleanups such as name space
 # removal.
 m4_define([OVS_TRAFFIC_VSWITCHD_STOP],
-  [AT_CHECK([ovs-vsctl del-br br0])
-   OVS_VSWITCHD_STOP([dnl
+  [OVS_VSWITCHD_STOP([dnl
 "/netdev_linux.*obtaining netdev stats via vport failed/d
 /dpif_netlink.*Generic Netlink family 'ovs_datapath' does not exist. The Open 
vSwitch kernel module is probably not loaded./d"])
AT_CHECK([:; $2])
-- 
2.1.4

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

[ovs-dev] [PATCHv2 3/5] system-macros: Create ADD_BR variant.

[Joe Stringer]

This patch splits ADD_BR into two commands, so they can be used from
different contexts:

ADD_BR(...) is a standalone command to add a bridge to OVS, and allows
additional ovs-vsctl arguments to be passed. It uses _ADD_BR().
_ADD_BR(...) is the implementation-specific ovs-vsctl arguments to
set up the correct datapath type for userspace or kmod tests.

Signed-off-by: Joe Stringer 
Acked-by: Daniele Di Proietto 
---
 tests/system-common-macros.at| 6 ++
 tests/system-kmod-macros.at  | 6 +++---
 tests/system-userspace-macros.at | 6 +++---
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 0e82a91..11b29fc 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -38,6 +38,12 @@ m4_define([NS_CHECK_EXEC],
 [ AT_CHECK([NS_EXEC([$1], [$2])], m4_shift(m4_shift($@))) ]
 )
 
+# ADD_BR([name], [vsctl-args])
+#
+# Expands into the proper ovs-vsctl commands to create a bridge with the
+# appropriate type, and allows additional arguments to be passed.
+m4_define([ADD_BR], [ovs-vsctl _ADD_BR([$1]) -- $2])
+
 # ADD_VETH([port], [namespace], [ovs-br], [ip_addr])
 #
 # Add a pair of veth ports. 'port' will be added to name space 'namespace',
diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index 3f94504..a5aa5db 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -1,8 +1,8 @@
-# ADD_BR([name])
+# _ADD_BR([name])
 #
 # Expands into the proper ovs-vsctl commands to create a bridge with the
 # appropriate type
-m4_define([ADD_BR], [[add-br $1]])
+m4_define([_ADD_BR], [[add-br $1]])
 
 # OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override])
 #
@@ -18,7 +18,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
 ON_EXIT([modprobe -r openvswitch])
_OVS_VSWITCHD_START([])
dnl Add bridges, ports, etc.
-   AT_CHECK([ovs-vsctl -- ADD_BR([br0]) -- set bridge br0 
protocols=[[OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15]] 
fail-mode=secure -- $1 m4_if([$2], [], [], [| ${PERL} $srcdir/uuidfilt.pl])], 
[0], [$2])
+   AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- set bridge br0 
protocols=[[OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15]] 
fail-mode=secure -- $1 m4_if([$2], [], [], [| ${PERL} $srcdir/uuidfilt.pl])], 
[0], [$2])
 ])
 
 # OVS_TRAFFIC_VSWITCHD_STOP([WHITELIST], [extra_cmds])
diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index b273576..adc40c4 100644
--- a/tests/system-userspace-macros.at
+++ b/tests/system-userspace-macros.at
@@ -1,8 +1,8 @@
-# ADD_BR([name])
+# _ADD_BR([name])
 #
 # Expands into the proper ovs-vsctl commands to create a bridge with the
 # appropriate type
-m4_define([ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" ]])
+m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" ]])
 
 # OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override])
 #
@@ -16,7 +16,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
   [
_OVS_VSWITCHD_START([--disable-system])
dnl Add bridges, ports, etc.
-   AT_CHECK([ovs-vsctl -- ADD_BR([br0]) -- set bridge br0 
protocols=[[OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15]] 
fail-mode=secure -- $1 m4_if([$2], [], [], [| ${PERL} $srcdir/uuidfilt.pl])], 
[0], [$2])
+   AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- set bridge br0 
protocols=[[OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15]] 
fail-mode=secure -- $1 m4_if([$2], [], [], [| ${PERL} $srcdir/uuidfilt.pl])], 
[0], [$2])
 ])
 
 # OVS_TRAFFIC_VSWITCHD_STOP([WHITELIST], [extra_cmds])
-- 
2.1.4

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

[ovs-dev] [PATCHv2 5/5] kmod-traffic: Add basic vxlan tunnel sanity test.

[Joe Stringer]

This test is skipped if the 'ip' command cannot interpret the vxlan 'dstport'
option; this is used as a proxy for detecting native kernel support for this
tunnel type.

Signed-off-by: Joe Stringer 
---
This initial sanity test uses a linux device tunnel in combination with an
OVS-based tunnel, so I think it is reasonable to limit its operation to newer
kernels and distributions with newer iptools. In future it would also be good
to have tests which use two OVS tunnel endpoints which could be run on any
kernel version and with the userspace datapath as well.
---
 tests/system-common-macros.at | 38 ++
 tests/system-kmod-macros.at   | 10 --
 tests/system-traffic.at   | 38 ++
 3 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 11b29fc..91792af 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -76,6 +76,44 @@ m4_define([ADD_VLAN],
 ]
 )
 
+# ADD_OVS_TUNNEL([type], [bridge], [port], [remote-addr], [overlay-addr])
+#
+# Add an ovs-based tunnel device in the root namespace, with name 'port' and
+# type 'type'. The tunnel device will be configured as point-to-point with the
+# 'remote-addr' as the underlay address of the remote tunnel endpoint.
+#
+# 'port will be configured with the address 'overlay-addr'.
+#
+m4_define([ADD_OVS_TUNNEL],
+   [AT_CHECK([ovs-vsctl add-port $2 $3 -- \
+  set int $3 type=$1 options:remote_ip=$4])
+AT_CHECK([ip addr add dev $2 $5])
+AT_CHECK([ip link set dev $2 up])
+AT_CHECK([ip link set dev $2 mtu 1450])
+ON_EXIT([ip addr del dev $2 $5])
+   ]
+)
+
+# ADD_NATIVE_TUNNEL([type], [port], [namespace], [remote-addr], [overlay-addr],
+#   [link-args])
+#
+# Add a native tunnel device within 'namespace', with name 'port' and type
+# 'type'. The tunnel device will be configured as point-to-point with the
+# 'remote-addr' as the underlay address of the remote tunnel endpoint (as
+# viewed from the perspective of that namespace).
+#
+# 'port' will be configured with the address 'overlay-addr'. 'link-args' is
+# made available so that additional arguments can be passed to "ip link",
+# for instance to configure the vxlan destination port.
+#
+m4_define([ADD_NATIVE_TUNNEL],
+   [NS_CHECK_EXEC([$3], [ip link add dev $2 type $1 remote $4 $6])
+NS_CHECK_EXEC([$3], [ip addr add dev $2 $5])
+NS_CHECK_EXEC([$3], [ip link set dev $2 up])
+NS_CHECK_EXEC([$3], [ip link set dev $2 mtu 1450])
+   ]
+)
+
 # FORMAT_PING([])
 #
 # Strip variant pieces from ping output so the output can be reliably compared.
diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index 2e2bf4e..5fab7b7 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -13,9 +13,15 @@ m4_define([_ADD_BR], [[add-br $1]])
 # output (e.g. because it includes "create" commands) then 'vsctl-output'
 # specifies the expected output after filtering through uuidfilt.pl.
 #
+# Best-effort loading of all available vport modules is performed.
+#
 m4_define([OVS_TRAFFIC_VSWITCHD_START],
-  [ AT_CHECK([modprobe openvswitch])
-ON_EXIT([modprobe -r openvswitch])
+  [AT_CHECK([modprobe openvswitch])
+   ON_EXIT([modprobe -r openvswitch])
+   m4_foreach([mod], [[vport_geneve], [vport_gre], [vport_lisp], [vport_stt], 
[vport_vxlan]],
+  [modprobe -q mod || echo "Module mod not loaded."
+   ON_EXIT([modprobe -q -r mod])])
+   ON_EXIT([ovs-dpctl del-dp ovs-system])
_OVS_VSWITCHD_START([])
dnl Add bridges, ports, etc.
AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- set bridge br0 
protocols=[[OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15]] 
fail-mode=secure -- $1 m4_if([$2], [], [], [| ${PERL} $srcdir/uuidfilt.pl])], 
[0], [$2])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 8324480..7dbed68 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -101,3 +101,41 @@ NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 
fc00:1::2 | FORMAT_PI
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([datapath - ping over vxlan tunnel])
+AT_SKIP_IF([! ip link add foo type vxlan help 2>&1 | grep dstport >/dev/null])
+
+OVS_TRAFFIC_VSWITCHD_START(
+   [set-fail-mode br0 standalone -- ])
+ADD_BR([br-underlay], [set-fail-mode br-underlay standalone])
+ADD_NAMESPACES(at_ns0)
+
+dnl Set up underlay link from host into the namespace using veth pair.
+ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24")
+AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
+AT_CHECK([ip link set dev br-underlay up])
+
+dnl Set up tunnel endpoints on OVS outside the namespace and with a native
+dnl linux device inside the namespace.
+ADD_OVS_TUNNEL([vxlan], [br0], [at_vxlan0], [172.31.1.1], [10.1.1.100/24])
+ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], 
[10.1.1.1/24],
+  [i

[ovs-dev] [PATCHv2 2/5] system-traffic: Check ping-by-ping output.

[Joe Stringer]

Rather than saving all of the ping output to a file then checking at the
end, check each ping and fail as soon as there is a connectivity
failure.

Signed-off-by: Joe Stringer 
---
 tests/system-common-macros.at |  6 ++
 tests/system-traffic.at   | 44 ---
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 198592c..0e82a91 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -69,3 +69,9 @@ m4_define([ADD_VLAN],
   NS_CHECK_EXEC([$2], [ip addr add dev $1.$3 $4])
 ]
 )
+
+# FORMAT_PING([])
+#
+# Strip variant pieces from ping output so the output can be reliably compared.
+#
+m4_define([FORMAT_PING], [grep "transmitted" | sed 's/time.*ms$/time 0ms/'])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 1ff2286..8324480 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -9,14 +9,13 @@ ADD_NAMESPACES(at_ns0, at_ns1)
 ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
 ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
 
-AT_CAPTURE_FILE([ping.output])
-NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 > ping.output])
-NS_CHECK_EXEC([at_ns0], [ping -s 1600 -q -c 3 -i 0.3 -w 2 10.1.1.2 >> 
ping.output])
-NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.1.1.2 >> 
ping.output])
-
-AT_CHECK([cat ping.output | grep "transmitted" | sed 's/time.*ms$/time 0ms/'], 
[0], [dnl
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], 
[0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping -s 1600 -q -c 3 -i 0.3 -w 2 10.1.1.2 | 
FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.1.1.2 | 
FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])
 
@@ -35,14 +34,13 @@ ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
 ADD_VLAN(p0, at_ns0, 100, "10.2.2.1/24")
 ADD_VLAN(p1, at_ns1, 100, "10.2.2.2/24")
 
-AT_CAPTURE_FILE([ping.output])
-NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.2.2.2 > ping.output])
-NS_CHECK_EXEC([at_ns0], [ping -s 1600 -q -c 3 -i 0.3 -w 2 10.2.2.2 >> 
ping.output])
-NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.2.2.2 >> 
ping.output])
-
-AT_CHECK([cat ping.output | grep "transmitted" | sed 's/time.*ms$/time 0ms/'], 
[0], [dnl
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.2.2.2 | FORMAT_PING], 
[0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping -s 1600 -q -c 3 -i 0.3 -w 2 10.2.2.2 | 
FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.2.2.2 | 
FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])
 
@@ -62,14 +60,13 @@ dnl Without this sleep, we get occasional failures due to 
the following error:
 dnl "connect: Cannot assign requested address"
 sleep 2;
 
-AT_CAPTURE_FILE([ping.output])
-NS_CHECK_EXEC([at_ns0], [ping6 -q -c 3 -i 0.3 -w 2 fc00::2 > ping.output])
-NS_CHECK_EXEC([at_ns0], [ping6 -s 1600 -q -c 3 -i 0.3 -w 2 fc00::2 >> 
ping.output])
-NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 fc00::2 >> 
ping.output])
-
-AT_CHECK([cat ping.output | grep "transmitted" | sed 's/time.*ms$/time 0ms/'], 
[0], [dnl
+NS_CHECK_EXEC([at_ns0], [ping6 -q -c 3 -i 0.3 -w 2 fc00::2 | FORMAT_PING], 
[0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping6 -s 1600 -q -c 3 -i 0.3 -w 2 fc00::2 | 
FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 fc00::2 | 
FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])
 
@@ -92,14 +89,13 @@ dnl Without this sleep, we get occasional failures due to 
the following error:
 dnl "connect: Cannot assign requested address"
 sleep 2;
 
-AT_CAPTURE_FILE([ping.output])
-NS_CHECK_EXEC([at_ns0], [ping6 -q -c 3 -i 0.3 -w 2 fc00:1::2 > ping.output])
-NS_CHECK_EXEC([at_ns0], [ping6 -s 1600 -q -c 3 -i 0.3 -w 2 fc00:1::2 >> 
ping.output])
-NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 fc00:1::2 >> 
ping.output])
-
-AT_CHECK([cat ping.output | grep "transmitted" | sed 's/time.*ms$/time 0ms/'], 
[0], [dnl
+NS_CHECK_EXEC([at_ns0], [ping6 -q -c 3 -i 0.3 -w 2 fc00:1::2 | FORMAT_PING], 
[0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping6 -s 1600 -q -c 3 -i 0.3 -w 2 fc00:1::2 | 
FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 fc00:1::2 | 
FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])
 
-- 
2.1.4

Re: [ovs-dev] [PATCH v4 5/9] classifier: Remove unused hash functions.

[Joe Stringer]

On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
> Remove unused cls_rule_hash() and minimatch_hash() functions.
>
> Signed-off-by: Jarno Rajahalme 

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

Re: [ovs-dev] [PATCH v4 6/9] classifier: Do not use mf_value.

[Joe Stringer]

On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
> mf_value has grown bigger than needed for storing the biggest
> supported prefix (IPv6 address length).  Define a new type to be used
> instead of mf_value.
>
> This makes classifier lookups a bit faster.
>
> Signed-off-by: Jarno Rajahalme 
> ---
>  lib/classifier.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 4adee2d..040d04f 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -138,12 +138,18 @@ next_visible_rule_in_list(const struct cls_match *rule, 
> cls_version_t version)
>  return rule;
>  }
>
> +/* Type with maximum supported prefix length. */
> +typedef union {
> +struct in6_addr ipv6;  /* For sizing. */
> +ovs_be32 be32; /* For access. */
> +} trie_prefix_t;

Typically I believe that we try to avoid typedefs, instead using the
expanded "union trie_prefix" so that the actual type can be determined
at a glance (See CodingStyle.md for more explanation).

Other than that, this change seems to make sense.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 7/9] classifier: Simplify minimask_hash().

[Joe Stringer]

On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
> minimask_hash() can be simplified as each value is known to be non-zero.
>
> Move miniflow_hash() into test-classifier.c as miniflow_hash__() as it
> is no longer needed elsewhere.
>
> Signed-off-by: Jarno Rajahalme 

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

Re: [ovs-dev] [PATCH v4 1/9] classifier: Fix comment.

[Joe Stringer]

On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
> Signed-off-by: Jarno Rajahalme 

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

[ovs-dev] Status of Open vSwitch with DPDK

[Daniele Di Proietto]

There has been some discussion lately about the status of the Open vSwitch
port to DPDK.  While part of the code has been tested for quite some time,
I think we can agree that there are a few rough spots that prevent it from
being easily deployed and used.

I was hoping to get some feedback from the community about those rough
spots,
i.e. areas where OVS+DPDK can/needs to improve to become more "production
ready" and user-friendly.

- PMD threads and queues management: the code has shown several bugs and
the
  netdev interfaces don't seem up to the job anymore.

  There's a lot of margin of improvement: we could factor out the code from
  dpif-netdev, add configuration parameters for advanced users, and figure
out
  a way to add unit tests.

  Related to this, the system should be as fast as possible out-of-the-box,
  without requiring too much tuning.

- Userspace tunneling: while the code has been there for quite some time it
  hasn't received the level of testing that the Linux kernel datapath
tunneling
  has.

- Documentation: other than a step by step tutorial,  it cannot be said
that
  DPDK is a first class citizen in the OVS documentation.  Manpages could
be
  improved.

- Vhost: the code has not received the level of testing of the kernel
vhost.
  Another doubt shared by some developers is whether we should keep
  vhost-cuse, given its relatively low ease of use and the overlapping with
  the far more standard vhost-user.

- Interface management and naming: interfaces must be manually removed from
  the kernel drivers.

  We still don't have an easy way to identify them. Ideas are welcome: how
can
  we make this user friendly?  Is there a better solution on the DPDK side?

  How are DPDK interfaces handled by linux distributions? I've heard about
  ongoing work for RHEL and Ubuntu, it would be interesting to coordinate.


- Insight into the system and debuggability: nothing beats tcpdump for the
  kernel datapath.  Can something similar be done for the userspace
datapath?

- Consistency of the tools: some commands are slightly different for the
  userspace/kernel datapath.  Ideally there shouldn't be any difference.

- Packaging: how should the distributions package DPDK and OVS? Should
there
  only be a single build to handle both the kernel and the userspace
datapath,
  eventually dynamically linked to DPDK?

- Benchmarks: we often rely on extremely simple flow tables with single
flow
  traffic to evaluate the effect of a change.  That may be ok during
  development, but OVS with the kernel datapath has been tested in
different
  scenarios with more complicated flow tables and even with hostile traffic
  patterns.

  Efforts in this sense are being made, like the vsperf project, or even
the
  simple ovs-pipeline.py

I would appreciate feedback on the above points, not (only) in terms of
solutions, but in terms of requirements that you feel are important for our
system to be considered ready.

Cheers,

Daniele

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

Re: [ovs-dev] [PATCH v4 2/9] flow: Avoid compile errors.

[Joe Stringer]

On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
> GCC (4.7) sees too wide shifts when there are none, refactor to
> circumvent the false error.
>
> Signed-off-by: Jarno Rajahalme 
> ---
>  lib/flow.c | 35 ++-
>  1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/lib/flow.c b/lib/flow.c
> index af51aac..61d9bdf 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -133,25 +133,34 @@ BUILD_MESSAGE("FLOW_WC_SEQ changed: miniflow_extract() 
> will have runtime "
>  #endif
>
>  #define miniflow_set_map(MF, OFS)   \
> -if ((OFS) < FLOW_TNL_U64S) {\
> -MINIFLOW_ASSERT(!(MF.maps.tnl_map & (UINT64_MAX << (OFS)))  \
> +{   \
> +size_t ofs = (OFS); \
> +\

Being a bit pedantic here, but don't we usually use 'size_t' to
indicate a length in bytes? Seems like 'unsigned int' would be more
appropriate for bit-shifting.

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

[ovs-dev] [PATCH v2] ovn: Add lflow-list to ovn-sbctl.

[Russell Bryant]

I frequently view the contents of the Logical_Flow table while working
on OVN.  Add a command that can output the contents of this table in a
sorted way that makes it easier to read through.  It's sorted by
logical datapath, pipeline, table id, priority, and match.

Signed-off-by: Russell Bryant 
---
 ovn/utilities/ovn-sbctl.8.in |   6 +++
 ovn/utilities/ovn-sbctl.c| 116 +++
 2 files changed, 122 insertions(+)


v1->v2
 - Address Alex's review feedback
 - Add a friendly "dump-flows" alias

Example output:

$ ./ovn-2port-setup.sh 
+ ovn-nbctl lswitch-add sw0
+ ovn-nbctl lport-add sw0 sw0-port1
+ ovn-nbctl lport-add sw0 sw0-port2
+ ovn-nbctl lport-set-macs sw0-port1 00:00:00:00:00:01
+ ovn-nbctl lport-set-macs sw0-port2 00:00:00:00:00:02
+ ovn-nbctl lport-set-port-security sw0-port1 00:00:00:00:00:01
+ ovn-nbctl lport-set-port-security sw0-port2 00:00:00:00:00:02
+ ovs-vsctl add-port br-int lport1 -- set Interface lport1 
external_ids:iface-id=sw0-port1
+ ovs-vsctl add-port br-int lport2 -- set Interface lport2 
external_ids:iface-id=sw0-port2
$ ovn-sbctl lflow-list
Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: ingress
  table_id=0, priority=100, match=(eth.src[40]), action=(drop;)
  table_id=0, priority=100, match=(vlan.present), action=(drop;)
  table_id=0, priority= 50, match=(inport == "sw0-port1" && eth.src == 
{00:00:00:00:00:01}), action=(next;)
  table_id=0, priority= 50, match=(inport == "sw0-port2" && eth.src == 
{00:00:00:00:00:02}), action=(next;)
  table_id=0, priority=  0, match=(1), action=(drop;)
  table_id=1, priority=100, match=(eth.dst[40]), action=(outport = "_MC_flood"; 
output;)
  table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:01), 
action=(outport = "sw0-port1"; output;)
  table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:02), 
action=(outport = "sw0-port2"; output;)
Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: egress
  table_id=0, priority=  0, match=(1), action=(next;)
  table_id=1, priority=100, match=(eth.dst[40]), action=(output;)
  table_id=1, priority= 50, match=(outport == "sw0-port1" && eth.dst == 
{00:00:00:00:00:01}), action=(output;)
  table_id=1, priority= 50, match=(outport == "sw0-port2" && eth.dst == 
{00:00:00:00:00:02}), action=(output;)



diff --git a/ovn/utilities/ovn-sbctl.8.in b/ovn/utilities/ovn-sbctl.8.in
index b5c796e..9f9168e 100644
--- a/ovn/utilities/ovn-sbctl.8.in
+++ b/ovn/utilities/ovn-sbctl.8.in
@@ -146,6 +146,12 @@ Without \fB\-\-if\-exists\fR, attempting to unbind a 
logical port
 that is not bound is an error.  With \fB\-\-if\-exists\fR, attempting
 to unbind logical port that is not bound has no effect.
 .
+.SS "Logical Flow Commands"
+.
+.IP "\fBlflow\-list\fR [\fIlogical\-datapath\fR]"
+List logical flows. If \fIlogical\-datapath\fR is specified, only list flows 
for
+that logical datapath.
+.
 .so lib/db-ctl-base.man
 .SH "EXIT STATUS"
 .IP "0"
diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index cbde60a..5d988f5 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -300,6 +300,9 @@ Port binding commands:\n\
   lport-bind LPORT CHASSISbind logical port LPORT to CHASSIS\n\
   lport-unbind LPORT  reset the port binding of logical port LPORT\n\
 \n\
+Logical flow commands:\n\
+  lflow-list [DATAPATH]   List logical flows for all or a single 
datapath\n\
+\n\
 %s\
 \n\
 Options:\n\
@@ -470,6 +473,13 @@ pre_get_info(struct ctl_context *ctx)
 
 ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_logical_port);
 ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);
+
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_logical_datapath);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_pipeline);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_actions);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_priority);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_table_id);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_match);
 }
 
 static struct cmd_show_table cmd_show_tables[] = {
@@ -584,6 +594,106 @@ cmd_lport_unbind(struct ctl_context *ctx)
 }
 }
 
+enum {
+PL_INGRESS,
+PL_EGRESS,
+};
+
+/* Help ensure we catch any future pipeline values */
+static int
+pipeline_encode(const char *pl)
+{
+if (!strcmp(pl, "ingress")) {
+return PL_INGRESS;
+} else if (!strcmp(pl, "egress")) {
+return PL_EGRESS;
+}
+
+OVS_NOT_REACHED();
+}
+
+static int
+lflow_cmp(const void *lf1_, const void *lf2_)
+{
+const struct sbrec_logical_flow *lf1, *lf2;
+
+lf1 = *((struct sbrec_logical_flow **) lf1_);
+lf2 = *((struct sbrec_logical_flow **) lf2_);
+
+int pl1 = pipeline_encode(lf1->pipeline);
+int pl2 = pipeline_encode(lf2->pipeline);
+
+#define CMP(expr) \
+do { \
+int res; \
+res = (expr); \
+if (res) { \
+return res; \
+} \
+} while

Re: [ovs-dev] [PATCH v2 1/2] upcall: Fix minor race when deleting ukeys.

[Ethan Jackson]

Ah, that's a good point I had missed.  I think the logic is fine as is
then.  I think I'll add a comment to that affect at the beginning of
the function.

Ethan

On Tue, Aug 11, 2015 at 1:51 PM, Joe Stringer  wrote:
> FWIW the "slice" logic in the for loop is meant to ensure that no
> revalidator threads concurrently clean the same map.
>
> There's two cases where the main thread might call into this as well
> though, one is when reconfiguring threads (which shouldn't conflict,
> because it stops all revalidators first), and the ovs-appctl
> revalidator/purge case, which could hit something like this, but I
> don't think there is a case where we use this functionality while OVS
> is under load, so the likelyhood of problems is low.
>
> On 11 August 2015 at 12:18, Ethan Jackson  wrote:
>> Yeah sorry this patch is not great.  That second problem is solved in
>> the follow on.  I'd be somewhat inclined to drop this one all
>> together.
>>
>> Ethan
>>
>> On Tue, Aug 11, 2015 at 11:15 AM, Jarno Rajahalme  
>> wrote:
>>>
 On Aug 11, 2015, at 10:42 AM, Jarno Rajahalme  
 wrote:


> On Aug 10, 2015, at 6:46 PM, Ethan J. Jackson  wrote:
>
> From: Ethan Jackson 
>
> Since revalidator_sweep() doesn't hold the ukey mutex for each full
> loop iteration, it's theoretically possible that two threads may
> call ukey_delete() on the same ukey.  If this happens, they both will
> attempt to remove the ukey from he cmap, causing the loser of the race
> to fail.
>
> Signed-off-by: Ethan J. Jackson 
> ---
> ofproto/ofproto-dpif-upcall.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 0f2e186..fddb535 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2076,7 +2076,6 @@ revalidator_sweep__(struct revalidator 
> *revalidator, bool purge)
>flow_exists = ukey->flow_exists;
>seq_mismatch = (ukey->dump_seq != dump_seq
>&& ukey->reval_seq != reval_seq);
> -ovs_mutex_unlock(&ukey->mutex);
>
>if (flow_exists
>&& (purge

 There is a call to push_ukey_ops() between these blocks. It calls 
 push_ukey_ops__(), which will take a lock on the ukeys, including the last 
 one added and still locked, so this would result in double locking.

>>>
>>> I missed the fact that handle_missed_revalidation() in here also takes the 
>>> ukey lock.
>>>
>>>   Jarno
>>>
 This could be resolved by moving the “ if (n_ops == REVALIDATE_MAX_BATCH)” 
 block after the (moved) ukey unlock, similarly to the remainder ops 
 handling at the end. However, I’m not sure if this would defeat the 
 purpose of this patch.

 Also, it seems that generally the umap lock is taken first and ukey locks 
 after, so there is a possibility of a deadlock if the locks are taken in 
 the reverse order, like in this patch.

 It would be really great if we could employ clang thread safety analysis 
 more than we do in this file currently, but I’m not sure if that is 
 possible.

  Jarno

> @@ -2095,6 +2094,7 @@ revalidator_sweep__(struct revalidator 
> *revalidator, bool purge)
>ukey_delete(umap, ukey);
>ovs_mutex_unlock(&umap->mutex);
>}
> +ovs_mutex_unlock(&ukey->mutex);
>}
>
>if (n_ops) {
> --
> 2.1.0
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

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

Re: [ovs-dev] [PATCH v2] ovn: Add lflow-list to ovn-sbctl.

[Alex Wang]

Thx a lot for the refinement,

Minor comments below,


On Wed, Aug 12, 2015 at 3:04 PM, Russell Bryant  wrote:

> I frequently view the contents of the Logical_Flow table while working
> on OVN.  Add a command that can output the contents of this table in a
> sorted way that makes it easier to read through.  It's sorted by
> logical datapath, pipeline, table id, priority, and match.
>
> Signed-off-by: Russell Bryant 
> ---
>  ovn/utilities/ovn-sbctl.8.in |   6 +++
>  ovn/utilities/ovn-sbctl.c| 116
> +++
>  2 files changed, 122 insertions(+)
>
>
> v1->v2
>  - Address Alex's review feedback
>  - Add a friendly "dump-flows" alias
>
> Example output:
>
> $ ./ovn-2port-setup.sh
> + ovn-nbctl lswitch-add sw0
> + ovn-nbctl lport-add sw0 sw0-port1
> + ovn-nbctl lport-add sw0 sw0-port2
> + ovn-nbctl lport-set-macs sw0-port1 00:00:00:00:00:01
> + ovn-nbctl lport-set-macs sw0-port2 00:00:00:00:00:02
> + ovn-nbctl lport-set-port-security sw0-port1 00:00:00:00:00:01
> + ovn-nbctl lport-set-port-security sw0-port2 00:00:00:00:00:02
> + ovs-vsctl add-port br-int lport1 -- set Interface lport1
> external_ids:iface-id=sw0-port1
> + ovs-vsctl add-port br-int lport2 -- set Interface lport2
> external_ids:iface-id=sw0-port2
> $ ovn-sbctl lflow-list
> Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: ingress
>   table_id=0, priority=100, match=(eth.src[40]), action=(drop;)
>   table_id=0, priority=100, match=(vlan.present), action=(drop;)
>   table_id=0, priority= 50, match=(inport == "sw0-port1" && eth.src ==
> {00:00:00:00:00:01}), action=(next;)
>   table_id=0, priority= 50, match=(inport == "sw0-port2" && eth.src ==
> {00:00:00:00:00:02}), action=(next;)
>   table_id=0, priority=  0, match=(1), action=(drop;)
>   table_id=1, priority=100, match=(eth.dst[40]), action=(outport =
> "_MC_flood"; output;)
>   table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:01),
> action=(outport = "sw0-port1"; output;)
>   table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:02),
> action=(outport = "sw0-port2"; output;)
> Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: egress
>   table_id=0, priority=  0, match=(1), action=(next;)
>   table_id=1, priority=100, match=(eth.dst[40]), action=(output;)
>   table_id=1, priority= 50, match=(outport == "sw0-port1" && eth.dst ==
> {00:00:00:00:00:01}), action=(output;)
>   table_id=1, priority= 50, match=(outport == "sw0-port2" && eth.dst ==
> {00:00:00:00:00:02}), action=(output;)
>
>
>
> diff --git a/ovn/utilities/ovn-sbctl.8.in b/ovn/utilities/ovn-sbctl.8.in
> index b5c796e..9f9168e 100644
> --- a/ovn/utilities/ovn-sbctl.8.in
> +++ b/ovn/utilities/ovn-sbctl.8.in
> @@ -146,6 +146,12 @@ Without \fB\-\-if\-exists\fR, attempting to unbind a
> logical port
>  that is not bound is an error.  With \fB\-\-if\-exists\fR, attempting
>  to unbind logical port that is not bound has no effect.
>  .
> +.SS "Logical Flow Commands"
> +.
> +.IP "\fBlflow\-list\fR [\fIlogical\-datapath\fR]"
> +List logical flows. If \fIlogical\-datapath\fR is specified, only list
> flows for
> +that logical datapath.
> +.
>  .so lib/db-ctl-base.man
>  .SH "EXIT STATUS"
>  .IP "0"
> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> index cbde60a..5d988f5 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> @@ -300,6 +300,9 @@ Port binding commands:\n\
>lport-bind LPORT CHASSISbind logical port LPORT to CHASSIS\n\
>lport-unbind LPORT  reset the port binding of logical port
> LPORT\n\
>  \n\
> +Logical flow commands:\n\
> +  lflow-list [DATAPATH]   List logical flows for all or a single
> datapath\n\
> +\n\
>


Could we also add usage for the alias 'dump-flows'



>  %s\
>  \n\
>  Options:\n\
> @@ -470,6 +473,13 @@ pre_get_info(struct ctl_context *ctx)
>
>  ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_logical_port);
>  ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);
> +
> +ovsdb_idl_add_column(ctx->idl,
> &sbrec_logical_flow_col_logical_datapath);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_pipeline);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_actions);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_priority);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_table_id);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_match);
>  }
>
>  static struct cmd_show_table cmd_show_tables[] = {
> @@ -584,6 +594,106 @@ cmd_lport_unbind(struct ctl_context *ctx)
>  }
>  }
>
> +enum {
> +PL_INGRESS,
> +PL_EGRESS,
> +};
> +
> +/* Help ensure we catch any future pipeline values */
> +static int
> +pipeline_encode(const char *pl)
> +{
> +if (!strcmp(pl, "ingress")) {
> +return PL_INGRESS;
> +} else if (!strcmp(pl, "egress")) {
> +return PL_EGRESS;
> +}
> +
> +OVS_NOT_REACHED();
> +}
> +
> +static int
> +lflow_cmp(const void *lf1_, const void *lf2_)
> +

[ovs-dev] [PATCH v3 1/2] ofproto-dpif-upcall: Make ukey actions modifiable with RCU.

[Ethan J. Jackson]

From: Ethan Jackson 

Future patches will need to modify ukey actions in some instances.
This patch makes this possible by protecting them with RCU.  It also
adds thread safety checks to enforce the new protection mechanism.

Signed-off-by: Ethan J. Jackson 
---
 ofproto/ofproto-dpif-upcall.c | 47 +++
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 0f2e186..c57cebd 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -215,7 +215,6 @@ struct udpif_key {
 size_t key_len;/* Length of 'key'. */
 const struct nlattr *mask; /* Datapath flow mask. */
 size_t mask_len;   /* Length of 'mask'. */
-struct ofpbuf *actions;/* Datapath flow actions as nlattrs. */
 ovs_u128 ufid; /* Unique flow identifier. */
 bool ufid_present; /* True if 'ufid' is in datapath. */
 uint32_t hash; /* Pre-computed hash for 'key'. */
@@ -228,6 +227,10 @@ struct udpif_key {
 uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. */
 bool flow_exists OVS_GUARDED; /* Ensures flows are only deleted
  once. */
+/* Datapath flow actions as nlattrs.  Protected by RCU.  Lockless reads can
+ * be made with ukey_get_actions(), otherwise a lock is required. Updates
+ * should be made with the ukey_set_actions() function. */
+const struct ofpbuf *actions OVS_GUARDED;
 
 struct xlate_cache *xcache OVS_GUARDED;   /* Cache for xlate entries that
* are affected by this ukey.
@@ -287,6 +290,8 @@ static struct udpif_key *ukey_create_from_upcall(struct 
upcall *,
 static int ukey_create_from_dpif_flow(const struct udpif *,
   const struct dpif_flow *,
   struct udpif_key **);
+static void ukey_get_actions(struct udpif_key *, const struct nlattr **actions,
+ size_t *size);
 static bool ukey_install_start(struct udpif *, struct udpif_key *ukey);
 static bool ukey_install_finish(struct udpif_key *ukey, int error);
 static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
@@ -1127,7 +1132,7 @@ process_upcall(struct udpif *udpif, struct upcall *upcall,
 if (upcall->sflow) {
 union user_action_cookie cookie;
 const struct nlattr *actions;
-int actions_len = 0;
+size_t actions_len = 0;
 struct dpif_sflow_actions sflow_actions;
 memset(&sflow_actions, 0, sizeof sflow_actions);
 memset(&cookie, 0, sizeof cookie);
@@ -1145,8 +1150,7 @@ process_upcall(struct udpif *udpif, struct upcall *upcall,
 /* Lookup actions in userspace cache. */
 struct udpif_key *ukey = ukey_lookup(udpif, upcall->ufid);
 if (ukey) {
-actions = ukey->actions->data;
-actions_len = ukey->actions->size;
+ukey_get_actions(ukey, &actions, &actions_len);
 dpif_sflow_read_actions(flow, actions, actions_len,
 &sflow_actions);
 }
@@ -1273,8 +1277,8 @@ handle_upcalls(struct udpif *udpif, struct upcall 
*upcalls,
 op->dop.u.flow_put.mask_len = ukey->mask_len;
 op->dop.u.flow_put.ufid = upcall->ufid;
 op->dop.u.flow_put.stats = NULL;
-op->dop.u.flow_put.actions = ukey->actions->data;
-op->dop.u.flow_put.actions_len = ukey->actions->size;
+ukey_get_actions(ukey, &op->dop.u.flow_put.actions,
+ &op->dop.u.flow_put.actions_len);
 }
 
 if (upcall->odp_actions.size) {
@@ -1340,6 +1344,31 @@ ukey_lookup(struct udpif *udpif, const ovs_u128 *ufid)
 return NULL;
 }
 
+/* Provides safe lockless access of RCU protected 'ukey->actions'.  Callers may
+ * alternatively access the field directly if they take 'ukey->mutex'. */
+static void
+ukey_get_actions(struct udpif_key *ukey, const struct nlattr **actions, size_t 
*size)
+OVS_NO_THREAD_SAFETY_ANALYSIS
+{
+const struct ofpbuf *buf = ukey->actions;
+*actions = buf->data;
+*size = buf->size;
+}
+
+static void
+ukey_set_actions(struct udpif_key *ukey, const struct ofpbuf *actions)
+OVS_REQUIRES(ukey->mutex)
+{
+const struct ofpbuf *old_actions = ukey->actions;
+
+ukey->actions = ofpbuf_clone(actions);
+
+if (old_actions) {
+ovsrcu_postpone(ofpbuf_delete, CONST_CAST(struct ofpbuf *,
+  old_actions));
+}
+}
+
 static struct udpif_key *
 ukey_create__(const struct nlattr *key, size_t key_len,
   const struct nlattr *mask, size_t mask_len,
@@ -1363,7 +1392,9 @@ u

[ovs-dev] [PATCH v3 2/2] ofproto: Allow in-place modifications of datapath flows.

[Ethan J. Jackson]

From: Ethan Jackson 

There are certain use cases (such as bond rebalancing) where a
datapath flow's actions may change, while it's wildcard pattern
remains the same.  Before this patch, revalidators would note the
change, delete the flow, and wait for the handlers to install an
updated version.  This is inefficient, as many packets could get
punted to userspace before the new flow is finally installed.

To improve the situation, this patch implements in place modification
of datapath flows.  If the revalidators detect the only change to a
given ukey is its actions, instead of deleting it, it does a put with
the MODIFY flag set.

Signed-off-by: Ethan J. Jackson 
---
 ofproto/ofproto-dpif-upcall.c | 164 +++---
 tests/ofproto-dpif.at |  40 +++
 2 files changed, 146 insertions(+), 58 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index c57cebd..f22fb05 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -150,6 +150,12 @@ enum upcall_type {
 IPFIX_UPCALL/* Per-bridge sampling. */
 };
 
+enum reval_result {
+UKEY_KEEP,
+UKEY_DELETE,
+UKEY_MODIFY
+};
+
 struct upcall {
 struct ofproto_dpif *ofproto;  /* Parent ofproto. */
 const struct recirc_id_node *recirc; /* Recirculation context. */
@@ -1694,33 +1700,41 @@ should_revalidate(const struct udpif *udpif, uint64_t 
packets,
 return false;
 }
 
-static bool
+/* Verifies that the datapath actions of 'ukey' are still correct, and pushes
+ * 'stats' for it.
+ *
+ * Returns a recommended action for 'ukey', options include:
+ *  UKEY_DELETE The ukey should be deleted.
+ *  UKEY_KEEP   The ukey is fine as is.
+ *  UKEY_MODIFY The ukey's actions should be changed but is otherwise
+ *  fine.  Callers should change the actions to those found
+ *  in the caller supplied 'odp_actions' buffer. */
+static enum reval_result
 revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
-const struct dpif_flow_stats *stats, uint64_t reval_seq)
+const struct dpif_flow_stats *stats,
+struct ofpbuf *odp_actions, uint64_t reval_seq)
 OVS_REQUIRES(ukey->mutex)
 {
-uint64_t odp_actions_stub[1024 / 8];
-struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
-
 struct xlate_out xout, *xoutp;
 struct netflow *netflow;
 struct ofproto_dpif *ofproto;
 struct dpif_flow_stats push;
 struct flow flow, dp_mask;
 struct flow_wildcards wc;
+enum reval_result result;
 uint64_t *dp64, *xout64;
 ofp_port_t ofp_in_port;
 struct xlate_in xin;
 long long int last_used;
 int error;
 size_t i;
-bool ok;
 bool need_revalidate;
 
-ok = false;
+result = UKEY_DELETE;
 xoutp = NULL;
 netflow = NULL;
 
+ofpbuf_clear(odp_actions);
 need_revalidate = (ukey->reval_seq != reval_seq);
 last_used = ukey->stats.used;
 push.used = stats->used;
@@ -1734,20 +1748,19 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 
 if (need_revalidate && last_used
 && !should_revalidate(udpif, push.n_packets, last_used)) {
-ok = false;
 goto exit;
 }
 
 /* We will push the stats, so update the ukey stats cache. */
 ukey->stats = *stats;
 if (!push.n_packets && !need_revalidate) {
-ok = true;
+result = UKEY_KEEP;
 goto exit;
 }
 
 if (ukey->xcache && !need_revalidate) {
 xlate_push_stats(ukey->xcache, &push);
-ok = true;
+result = UKEY_KEEP;
 goto exit;
 }
 
@@ -1770,7 +1783,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 }
 
 xlate_in_init(&xin, ofproto, &flow, ofp_in_port, NULL, push.tcp_flags,
-  NULL, need_revalidate ? &wc : NULL, &odp_actions);
+  NULL, need_revalidate ? &wc : NULL, odp_actions);
 if (push.n_packets) {
 xin.resubmit_stats = &push;
 xin.may_learn = true;
@@ -1780,18 +1793,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 xoutp = &xout;
 
 if (!need_revalidate) {
-ok = true;
+result = UKEY_KEEP;
 goto exit;
 }
 
 if (xout.slow) {
-ofpbuf_clear(&odp_actions);
+ofpbuf_clear(odp_actions);
 compose_slow_path(udpif, &xout, &flow, flow.in_port.odp_port,
-  &odp_actions);
-}
-
-if (!ofpbuf_equal(&odp_actions, ukey->actions)) {
-goto exit;
+  odp_actions);
 }
 
 if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, ukey->key,
@@ -1812,18 +1821,24 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 }
 }
 
-ok = true;
+if (!ofpbuf_equal(odp_actions, ukey->actions)) {
+/* The datapath mask was OK, but the actions seem to have changed.
+ * Let's modi

Re: [ovs-dev] [PATCH v2] ovn: Add lflow-list to ovn-sbctl.

[Russell Bryant]

On 08/12/2015 06:51 PM, Alex Wang wrote:
> Thx a lot for the refinement,
> 
> Minor comments below,
> 
> 
> On Wed, Aug 12, 2015 at 3:04 PM, Russell Bryant  > wrote:
> 
> I frequently view the contents of the Logical_Flow table while working
> on OVN.  Add a command that can output the contents of this table in a
> sorted way that makes it easier to read through.  It's sorted by
> logical datapath, pipeline, table id, priority, and match.
> 
> Signed-off-by: Russell Bryant  >
> ---
>  ovn/utilities/ovn-sbctl.8.in  |   6 +++
>  ovn/utilities/ovn-sbctl.c| 116
> +++
>  2 files changed, 122 insertions(+)
> 
> 
> v1->v2
>  - Address Alex's review feedback
>  - Add a friendly "dump-flows" alias
> 
> Example output:
> 
> $ ./ovn-2port-setup.sh
> + ovn-nbctl lswitch-add sw0
> + ovn-nbctl lport-add sw0 sw0-port1
> + ovn-nbctl lport-add sw0 sw0-port2
> + ovn-nbctl lport-set-macs sw0-port1 00:00:00:00:00:01
> + ovn-nbctl lport-set-macs sw0-port2 00:00:00:00:00:02
> + ovn-nbctl lport-set-port-security sw0-port1 00:00:00:00:00:01
> + ovn-nbctl lport-set-port-security sw0-port2 00:00:00:00:00:02
> + ovs-vsctl add-port br-int lport1 -- set Interface lport1
> external_ids:iface-id=sw0-port1
> + ovs-vsctl add-port br-int lport2 -- set Interface lport2
> external_ids:iface-id=sw0-port2
> $ ovn-sbctl lflow-list
> Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: ingress
>   table_id=0, priority=100, match=(eth.src[40]), action=(drop;)
>   table_id=0, priority=100, match=(vlan.present), action=(drop;)
>   table_id=0, priority= 50, match=(inport == "sw0-port1" && eth.src
> == {00:00:00:00:00:01}), action=(next;)
>   table_id=0, priority= 50, match=(inport == "sw0-port2" && eth.src
> == {00:00:00:00:00:02}), action=(next;)
>   table_id=0, priority=  0, match=(1), action=(drop;)
>   table_id=1, priority=100, match=(eth.dst[40]), action=(outport =
> "_MC_flood"; output;)
>   table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:01),
> action=(outport = "sw0-port1"; output;)
>   table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:02),
> action=(outport = "sw0-port2"; output;)
> Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: egress
>   table_id=0, priority=  0, match=(1), action=(next;)
>   table_id=1, priority=100, match=(eth.dst[40]), action=(output;)
>   table_id=1, priority= 50, match=(outport == "sw0-port1" && eth.dst
> == {00:00:00:00:00:01}), action=(output;)
>   table_id=1, priority= 50, match=(outport == "sw0-port2" && eth.dst
> == {00:00:00:00:00:02}), action=(output;)
> 
> 
> 
> diff --git a/ovn/utilities/ovn-sbctl.8.in 
> b/ovn/utilities/ovn-sbctl.8.in 
> index b5c796e..9f9168e 100644
> --- a/ovn/utilities/ovn-sbctl.8.in 
> +++ b/ovn/utilities/ovn-sbctl.8.in 
> @@ -146,6 +146,12 @@ Without \fB\-\-if\-exists\fR, attempting to
> unbind a logical port
>  that is not bound is an error.  With \fB\-\-if\-exists\fR, attempting
>  to unbind logical port that is not bound has no effect.
>  .
> +.SS "Logical Flow Commands"
> +.
> +.IP "\fBlflow\-list\fR [\fIlogical\-datapath\fR]"
> +List logical flows. If \fIlogical\-datapath\fR is specified, only
> list flows for
> +that logical datapath.
> +.
>  .so lib/db-ctl-base.man
>  .SH "EXIT STATUS"
>  .IP "0"
> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> index cbde60a..5d988f5 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> @@ -300,6 +300,9 @@ Port binding commands:\n\
>lport-bind LPORT CHASSISbind logical port LPORT to CHASSIS\n\
>lport-unbind LPORT  reset the port binding of logical
> port LPORT\n\
>  \n\
> +Logical flow commands:\n\
> +  lflow-list [DATAPATH]   List logical flows for all or a
> single datapath\n\
> +\n\
> 
> 
> 
> Could we also add usage for the alias 'dump-flows'

I excluded on purpose because I thought we might want to keep it hidden.
 It doesn't matter to me though.  I'll add it.

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

[ovs-dev] [PATCH v3] ovn: Add lflow-list to ovn-sbctl.

[Russell Bryant]

I frequently view the contents of the Logical_Flow table while working
on OVN.  Add a command that can output the contents of this table in a
sorted way that makes it easier to read through.  It's sorted by
logical datapath, pipeline, table id, priority, and match.

Signed-off-by: Russell Bryant 
---
 ovn/utilities/ovn-sbctl.8.in |   9 
 ovn/utilities/ovn-sbctl.c| 116 +++
 2 files changed, 125 insertions(+)


v2->v3
 - Address additional feedbcak from Alex
v1->v2
 - Address Alex's review feedback
 - Add a friendly "dump-flows" alias

Example output:

$ ./ovn-2port-setup.sh 
+ ovn-nbctl lswitch-add sw0
+ ovn-nbctl lport-add sw0 sw0-port1
+ ovn-nbctl lport-add sw0 sw0-port2
+ ovn-nbctl lport-set-macs sw0-port1 00:00:00:00:00:01
+ ovn-nbctl lport-set-macs sw0-port2 00:00:00:00:00:02
+ ovn-nbctl lport-set-port-security sw0-port1 00:00:00:00:00:01
+ ovn-nbctl lport-set-port-security sw0-port2 00:00:00:00:00:02
+ ovs-vsctl add-port br-int lport1 -- set Interface lport1 
external_ids:iface-id=sw0-port1
+ ovs-vsctl add-port br-int lport2 -- set Interface lport2 
external_ids:iface-id=sw0-port2
$ ovn-sbctl lflow-list
Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: ingress
  table_id=0, priority=100, match=(eth.src[40]), action=(drop;)
  table_id=0, priority=100, match=(vlan.present), action=(drop;)
  table_id=0, priority= 50, match=(inport == "sw0-port1" && eth.src == 
{00:00:00:00:00:01}), action=(next;)
  table_id=0, priority= 50, match=(inport == "sw0-port2" && eth.src == 
{00:00:00:00:00:02}), action=(next;)
  table_id=0, priority=  0, match=(1), action=(drop;)
  table_id=1, priority=100, match=(eth.dst[40]), action=(outport = "_MC_flood"; 
output;)
  table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:01), 
action=(outport = "sw0-port1"; output;)
  table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:02), 
action=(outport = "sw0-port2"; output;)
Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: egress
  table_id=0, priority=  0, match=(1), action=(next;)
  table_id=1, priority=100, match=(eth.dst[40]), action=(output;)
  table_id=1, priority= 50, match=(outport == "sw0-port1" && eth.dst == 
{00:00:00:00:00:01}), action=(output;)
  table_id=1, priority= 50, match=(outport == "sw0-port2" && eth.dst == 
{00:00:00:00:00:02}), action=(output;)




diff --git a/ovn/utilities/ovn-sbctl.8.in b/ovn/utilities/ovn-sbctl.8.in
index b5c796e..7e68d68 100644
--- a/ovn/utilities/ovn-sbctl.8.in
+++ b/ovn/utilities/ovn-sbctl.8.in
@@ -146,6 +146,15 @@ Without \fB\-\-if\-exists\fR, attempting to unbind a 
logical port
 that is not bound is an error.  With \fB\-\-if\-exists\fR, attempting
 to unbind logical port that is not bound has no effect.
 .
+.SS "Logical Flow Commands"
+.
+.IP "\fBlflow\-list\fR [\fIlogical\-datapath\fR]"
+List logical flows. If \fIlogical\-datapath\fR is specified, only list flows 
for
+that logical datapath.
+.
+.IP "\fBdump\-flows\fR [\fIlogical\-datapath\fR]"
+Alias for \fBlflow\-list\fB.
+.
 .so lib/db-ctl-base.man
 .SH "EXIT STATUS"
 .IP "0"
diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index cbde60a..45b2afe 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -300,6 +300,10 @@ Port binding commands:\n\
   lport-bind LPORT CHASSISbind logical port LPORT to CHASSIS\n\
   lport-unbind LPORT  reset the port binding of logical port LPORT\n\
 \n\
+Logical flow commands:\n\
+  lflow-list [DATAPATH]   List logical flows for all or a single 
datapath\n\
+  dump-flows [DATAPATH]   Alias for lflow-list\n\
+\n\
 %s\
 \n\
 Options:\n\
@@ -470,6 +474,13 @@ pre_get_info(struct ctl_context *ctx)
 
 ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_logical_port);
 ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);
+
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_logical_datapath);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_pipeline);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_actions);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_priority);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_table_id);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_match);
 }
 
 static struct cmd_show_table cmd_show_tables[] = {
@@ -584,6 +595,105 @@ cmd_lport_unbind(struct ctl_context *ctx)
 }
 }
 
+enum {
+PL_INGRESS,
+PL_EGRESS,
+};
+
+/* Help ensure we catch any future pipeline values */
+static int
+pipeline_encode(const char *pl)
+{
+if (!strcmp(pl, "ingress")) {
+return PL_INGRESS;
+} else if (!strcmp(pl, "egress")) {
+return PL_EGRESS;
+}
+
+OVS_NOT_REACHED();
+}
+
+static int
+lflow_cmp(const void *lf1_, const void *lf2_)
+{
+const struct sbrec_logical_flow *lf1, *lf2;
+
+lf1 = *((struct sbrec_logical_flow **) lf1_);
+lf2 = *((struct sbrec_logical_flow **) lf2_);
+
+int pl1 = pipeline_encode(lf1->pipeline);
+int pl

Re: [ovs-dev] [PATCH v4 1/9] classifier: Fix comment.

[Jarno Rajahalme]

Pushed to master,

  Jarno

> On Aug 12, 2015, at 2:33 PM, Joe Stringer  wrote:
> 
> On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Joe Stringer 

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

Re: [ovs-dev] [PATCH v4 2/9] flow: Avoid compile errors.

[Jarno Rajahalme]

> On Aug 12, 2015, at 2:38 PM, Joe Stringer  wrote:
> 
> On 7 August 2015 at 16:57, Jarno Rajahalme  > wrote:
>> GCC (4.7) sees too wide shifts when there are none, refactor to
>> circumvent the false error.
>> 
>> Signed-off-by: Jarno Rajahalme 
>> ---
>> lib/flow.c | 35 ++-
>> 1 file changed, 22 insertions(+), 13 deletions(-)
>> 
>> diff --git a/lib/flow.c b/lib/flow.c
>> index af51aac..61d9bdf 100644
>> --- a/lib/flow.c
>> +++ b/lib/flow.c
>> @@ -133,25 +133,34 @@ BUILD_MESSAGE("FLOW_WC_SEQ changed: miniflow_extract() 
>> will have runtime "
>> #endif
>> 
>> #define miniflow_set_map(MF, OFS)   \
>> -if ((OFS) < FLOW_TNL_U64S) {\
>> -MINIFLOW_ASSERT(!(MF.maps.tnl_map & (UINT64_MAX << (OFS)))  \
>> +{   \
>> +size_t ofs = (OFS); \
>> +\
> 
> Being a bit pedantic here, but don't we usually use 'size_t' to
> indicate a length in bytes? Seems like 'unsigned int' would be more
> appropriate for bit-shifting.
> 

Changed, thanks for the review!

Pushed to master,

  Jarno

> Acked-by: Joe Stringer  >

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

Re: [ovs-dev] [PATCH v4 7/9] classifier: Simplify minimask_hash().

[Jarno Rajahalme]

Thanks for the review!

Pushed to master,

  Jarno

> On Aug 12, 2015, at 2:32 PM, Joe Stringer  wrote:
> 
> On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
>> minimask_hash() can be simplified as each value is known to be non-zero.
>> 
>> Move miniflow_hash() into test-classifier.c as miniflow_hash__() as it
>> is no longer needed elsewhere.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Joe Stringer 

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

Re: [ovs-dev] [PATCH v4 5/9] classifier: Remove unused hash functions.

[Jarno Rajahalme]

Thanks for the review, pushed to master,

  Jarno

> On Aug 12, 2015, at 2:12 PM, Joe Stringer  wrote:
> 
> On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
>> Remove unused cls_rule_hash() and minimatch_hash() functions.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Joe Stringer 

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

Re: [ovs-dev] [PATCH v4 6/9] classifier: Do not use mf_value.

[Jarno Rajahalme]

> On Aug 12, 2015, at 2:18 PM, Joe Stringer  wrote:
> 
> On 7 August 2015 at 16:57, Jarno Rajahalme  > wrote:
>> mf_value has grown bigger than needed for storing the biggest
>> supported prefix (IPv6 address length).  Define a new type to be used
>> instead of mf_value.
>> 
>> This makes classifier lookups a bit faster.
>> 
>> Signed-off-by: Jarno Rajahalme 
>> ---
>> lib/classifier.c | 14 ++
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>> 
>> diff --git a/lib/classifier.c b/lib/classifier.c
>> index 4adee2d..040d04f 100644
>> --- a/lib/classifier.c
>> +++ b/lib/classifier.c
>> @@ -138,12 +138,18 @@ next_visible_rule_in_list(const struct cls_match 
>> *rule, cls_version_t version)
>> return rule;
>> }
>> 
>> +/* Type with maximum supported prefix length. */
>> +typedef union {
>> +struct in6_addr ipv6;  /* For sizing. */
>> +ovs_be32 be32; /* For access. */
>> +} trie_prefix_t;
> 
> Typically I believe that we try to avoid typedefs, instead using the
> expanded "union trie_prefix" so that the actual type can be determined
> at a glance (See CodingStyle.md for more explanation).
> 

OK, changed to “union trie_prefix”.

> Other than that, this change seems to make sense.

Is this an Acked-by?

  Jarno

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

Re: [ovs-dev] [PATCH v4 6/9] classifier: Do not use mf_value.

[Joe Stringer]

On 12 August 2015 at 16:48, Jarno Rajahalme  wrote:
>
> On Aug 12, 2015, at 2:18 PM, Joe Stringer  wrote:
>
> On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
>
> mf_value has grown bigger than needed for storing the biggest
> supported prefix (IPv6 address length).  Define a new type to be used
> instead of mf_value.
>
> This makes classifier lookups a bit faster.
>
> Signed-off-by: Jarno Rajahalme 
> ---
> lib/classifier.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 4adee2d..040d04f 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -138,12 +138,18 @@ next_visible_rule_in_list(const struct cls_match
> *rule, cls_version_t version)
> return rule;
> }
>
> +/* Type with maximum supported prefix length. */
> +typedef union {
> +struct in6_addr ipv6;  /* For sizing. */
> +ovs_be32 be32; /* For access. */
> +} trie_prefix_t;
>
>
> Typically I believe that we try to avoid typedefs, instead using the
> expanded "union trie_prefix" so that the actual type can be determined
> at a glance (See CodingStyle.md for more explanation).
>
>
> OK, changed to “union trie_prefix”.
>
> Other than that, this change seems to make sense.
>
>
> Is this an Acked-by?

Sorry, yeah this is fine. I thought it might be dependent on an
earlier change, but it seems like it's good by itself.

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

Re: [ovs-dev] [PATCH v3] ovn: Add lflow-list to ovn-sbctl.

[Alex Wang]

Thx, applied to master~

On Wed, Aug 12, 2015 at 4:20 PM, Russell Bryant  wrote:

> I frequently view the contents of the Logical_Flow table while working
> on OVN.  Add a command that can output the contents of this table in a
> sorted way that makes it easier to read through.  It's sorted by
> logical datapath, pipeline, table id, priority, and match.
>
> Signed-off-by: Russell Bryant 
> ---
>  ovn/utilities/ovn-sbctl.8.in |   9 
>  ovn/utilities/ovn-sbctl.c| 116
> +++
>  2 files changed, 125 insertions(+)
>
>
> v2->v3
>  - Address additional feedbcak from Alex
> v1->v2
>  - Address Alex's review feedback
>  - Add a friendly "dump-flows" alias
>
> Example output:
>
> $ ./ovn-2port-setup.sh
> + ovn-nbctl lswitch-add sw0
> + ovn-nbctl lport-add sw0 sw0-port1
> + ovn-nbctl lport-add sw0 sw0-port2
> + ovn-nbctl lport-set-macs sw0-port1 00:00:00:00:00:01
> + ovn-nbctl lport-set-macs sw0-port2 00:00:00:00:00:02
> + ovn-nbctl lport-set-port-security sw0-port1 00:00:00:00:00:01
> + ovn-nbctl lport-set-port-security sw0-port2 00:00:00:00:00:02
> + ovs-vsctl add-port br-int lport1 -- set Interface lport1
> external_ids:iface-id=sw0-port1
> + ovs-vsctl add-port br-int lport2 -- set Interface lport2
> external_ids:iface-id=sw0-port2
> $ ovn-sbctl lflow-list
> Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: ingress
>   table_id=0, priority=100, match=(eth.src[40]), action=(drop;)
>   table_id=0, priority=100, match=(vlan.present), action=(drop;)
>   table_id=0, priority= 50, match=(inport == "sw0-port1" && eth.src ==
> {00:00:00:00:00:01}), action=(next;)
>   table_id=0, priority= 50, match=(inport == "sw0-port2" && eth.src ==
> {00:00:00:00:00:02}), action=(next;)
>   table_id=0, priority=  0, match=(1), action=(drop;)
>   table_id=1, priority=100, match=(eth.dst[40]), action=(outport =
> "_MC_flood"; output;)
>   table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:01),
> action=(outport = "sw0-port1"; output;)
>   table_id=1, priority= 50, match=(eth.dst == 00:00:00:00:00:02),
> action=(outport = "sw0-port2"; output;)
> Datapath: 0f9a9e4e-0ef0-4afb-bed4-09887387fd10  Pipeline: egress
>   table_id=0, priority=  0, match=(1), action=(next;)
>   table_id=1, priority=100, match=(eth.dst[40]), action=(output;)
>   table_id=1, priority= 50, match=(outport == "sw0-port1" && eth.dst ==
> {00:00:00:00:00:01}), action=(output;)
>   table_id=1, priority= 50, match=(outport == "sw0-port2" && eth.dst ==
> {00:00:00:00:00:02}), action=(output;)
>
>
>
>
> diff --git a/ovn/utilities/ovn-sbctl.8.in b/ovn/utilities/ovn-sbctl.8.in
> index b5c796e..7e68d68 100644
> --- a/ovn/utilities/ovn-sbctl.8.in
> +++ b/ovn/utilities/ovn-sbctl.8.in
> @@ -146,6 +146,15 @@ Without \fB\-\-if\-exists\fR, attempting to unbind a
> logical port
>  that is not bound is an error.  With \fB\-\-if\-exists\fR, attempting
>  to unbind logical port that is not bound has no effect.
>  .
> +.SS "Logical Flow Commands"
> +.
> +.IP "\fBlflow\-list\fR [\fIlogical\-datapath\fR]"
> +List logical flows. If \fIlogical\-datapath\fR is specified, only list
> flows for
> +that logical datapath.
> +.
> +.IP "\fBdump\-flows\fR [\fIlogical\-datapath\fR]"
> +Alias for \fBlflow\-list\fB.
> +.
>  .so lib/db-ctl-base.man
>  .SH "EXIT STATUS"
>  .IP "0"
> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> index cbde60a..45b2afe 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> @@ -300,6 +300,10 @@ Port binding commands:\n\
>lport-bind LPORT CHASSISbind logical port LPORT to CHASSIS\n\
>lport-unbind LPORT  reset the port binding of logical port
> LPORT\n\
>  \n\
> +Logical flow commands:\n\
> +  lflow-list [DATAPATH]   List logical flows for all or a single
> datapath\n\
> +  dump-flows [DATAPATH]   Alias for lflow-list\n\
> +\n\
>  %s\
>  \n\
>  Options:\n\
> @@ -470,6 +474,13 @@ pre_get_info(struct ctl_context *ctx)
>
>  ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_logical_port);
>  ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);
> +
> +ovsdb_idl_add_column(ctx->idl,
> &sbrec_logical_flow_col_logical_datapath);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_pipeline);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_actions);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_priority);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_table_id);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_match);
>  }
>
>  static struct cmd_show_table cmd_show_tables[] = {
> @@ -584,6 +595,105 @@ cmd_lport_unbind(struct ctl_context *ctx)
>  }
>  }
>
> +enum {
> +PL_INGRESS,
> +PL_EGRESS,
> +};
> +
> +/* Help ensure we catch any future pipeline values */
> +static int
> +pipeline_encode(const char *pl)
> +{
> +if (!strcmp(pl, "ingress")) {
> +return PL_INGRESS;
> +} else if (!strcmp(pl, "egress")) {
> +return PL_EGRESS;

Re: [ovs-dev] [PATCH v3 1/2] ofproto-dpif-upcall: Make ukey actions modifiable with RCU.

[Jarno Rajahalme]

> On Aug 12, 2015, at 4:13 PM, Ethan J. Jackson  wrote:
> 
> From: Ethan Jackson 
> 
> Future patches will need to modify ukey actions in some instances.
> This patch makes this possible by protecting them with RCU.  It also
> adds thread safety checks to enforce the new protection mechanism.
> 
> Signed-off-by: Ethan J. Jackson 
> ---
> ofproto/ofproto-dpif-upcall.c | 47 +++
> 1 file changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 0f2e186..c57cebd 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -215,7 +215,6 @@ struct udpif_key {
> size_t key_len;/* Length of 'key'. */
> const struct nlattr *mask; /* Datapath flow mask. */
> size_t mask_len;   /* Length of 'mask'. */
> -struct ofpbuf *actions;/* Datapath flow actions as nlattrs. */
> ovs_u128 ufid; /* Unique flow identifier. */
> bool ufid_present; /* True if 'ufid' is in datapath. */
> uint32_t hash; /* Pre-computed hash for 'key'. */
> @@ -228,6 +227,10 @@ struct udpif_key {
> uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. */
> bool flow_exists OVS_GUARDED; /* Ensures flows are only 
> deleted
>  once. */
> +/* Datapath flow actions as nlattrs.  Protected by RCU.  Lockless reads 
> can
> + * be made with ukey_get_actions(), otherwise a lock is required. Updates
> + * should be made with the ukey_set_actions() function. */
> +const struct ofpbuf *actions OVS_GUARDED;
> 

IMO it would be prudent to define this as:

OVSRCU_TYPE(const struct ofpbuf *) actions;

and then always use ovsrcu_get() to access it, and ovsrcu_set() to set it. This 
makes sure the memory barriers are there, i.e., the compiler will not rearrange 
any initialization of the new actions after the point in which other threads 
can see it.

With this the OVS_GUARDED would not be needed here.

  Jarno

> struct xlate_cache *xcache OVS_GUARDED;   /* Cache for xlate entries that
>* are affected by this ukey.
> @@ -287,6 +290,8 @@ static struct udpif_key *ukey_create_from_upcall(struct 
> upcall *,
> static int ukey_create_from_dpif_flow(const struct udpif *,
>   const struct dpif_flow *,
>   struct udpif_key **);
> +static void ukey_get_actions(struct udpif_key *, const struct nlattr 
> **actions,
> + size_t *size);
> static bool ukey_install_start(struct udpif *, struct udpif_key *ukey);
> static bool ukey_install_finish(struct udpif_key *ukey, int error);
> static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
> @@ -1127,7 +1132,7 @@ process_upcall(struct udpif *udpif, struct upcall 
> *upcall,
> if (upcall->sflow) {
> union user_action_cookie cookie;
> const struct nlattr *actions;
> -int actions_len = 0;
> +size_t actions_len = 0;
> struct dpif_sflow_actions sflow_actions;
> memset(&sflow_actions, 0, sizeof sflow_actions);
> memset(&cookie, 0, sizeof cookie);
> @@ -1145,8 +1150,7 @@ process_upcall(struct udpif *udpif, struct upcall 
> *upcall,
> /* Lookup actions in userspace cache. */
> struct udpif_key *ukey = ukey_lookup(udpif, upcall->ufid);
> if (ukey) {
> -actions = ukey->actions->data;
> -actions_len = ukey->actions->size;
> +ukey_get_actions(ukey, &actions, &actions_len);
> dpif_sflow_read_actions(flow, actions, actions_len,
> &sflow_actions);
> }
> @@ -1273,8 +1277,8 @@ handle_upcalls(struct udpif *udpif, struct upcall 
> *upcalls,
> op->dop.u.flow_put.mask_len = ukey->mask_len;
> op->dop.u.flow_put.ufid = upcall->ufid;
> op->dop.u.flow_put.stats = NULL;
> -op->dop.u.flow_put.actions = ukey->actions->data;
> -op->dop.u.flow_put.actions_len = ukey->actions->size;
> +ukey_get_actions(ukey, &op->dop.u.flow_put.actions,
> + &op->dop.u.flow_put.actions_len);
> }
> 
> if (upcall->odp_actions.size) {
> @@ -1340,6 +1344,31 @@ ukey_lookup(struct udpif *udpif, const ovs_u128 *ufid)
> return NULL;
> }
> 
> +/* Provides safe lockless access of RCU protected 'ukey->actions'.  Callers 
> may
> + * alternatively access the field directly if they take 'ukey->mutex'. */
> +static void
> +ukey_get_actions(struct udpif_key *ukey, const struct nlattr **actions, 
> size_t *size)
> +OVS_NO_THREAD_SAFETY_ANALYSIS
> +{
> +const struct ofpbuf *buf = ukey->actions

Re: [ovs-dev] [PATCH v4 6/9] classifier: Do not use mf_value.

[Jarno Rajahalme]

> On Aug 12, 2015, at 4:51 PM, Joe Stringer  wrote:
> 
> On 12 August 2015 at 16:48, Jarno Rajahalme  > wrote:
>> 
>> On Aug 12, 2015, at 2:18 PM, Joe Stringer  wrote:
>> 
>> On 7 August 2015 at 16:57, Jarno Rajahalme  wrote:
>> 
>> mf_value has grown bigger than needed for storing the biggest
>> supported prefix (IPv6 address length).  Define a new type to be used
>> instead of mf_value.
>> 
>> This makes classifier lookups a bit faster.
>> 
>> Signed-off-by: Jarno Rajahalme 
>> ---
>> lib/classifier.c | 14 ++
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>> 
>> diff --git a/lib/classifier.c b/lib/classifier.c
>> index 4adee2d..040d04f 100644
>> --- a/lib/classifier.c
>> +++ b/lib/classifier.c
>> @@ -138,12 +138,18 @@ next_visible_rule_in_list(const struct cls_match
>> *rule, cls_version_t version)
>>return rule;
>> }
>> 
>> +/* Type with maximum supported prefix length. */
>> +typedef union {
>> +struct in6_addr ipv6;  /* For sizing. */
>> +ovs_be32 be32; /* For access. */
>> +} trie_prefix_t;
>> 
>> 
>> Typically I believe that we try to avoid typedefs, instead using the
>> expanded "union trie_prefix" so that the actual type can be determined
>> at a glance (See CodingStyle.md for more explanation).
>> 
>> 
>> OK, changed to “union trie_prefix”.
>> 
>> Other than that, this change seems to make sense.
>> 
>> 
>> Is this an Acked-by?
> 
> Sorry, yeah this is fine. I thought it might be dependent on an
> earlier change, but it seems like it's good by itself.
> 
> Acked-by: Joe Stringer  >

Pushed to master,

  Jarno

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

Re: [ovs-dev] [PATCH v3 2/2] ofproto: Allow in-place modifications of datapath flows.

[Jarno Rajahalme]

This seems good to go for me,

  Jarno

Acked-by: Jarno Rajahalme 

> On Aug 12, 2015, at 4:13 PM, Ethan J. Jackson  wrote:
> 
> From: Ethan Jackson 
> 
> There are certain use cases (such as bond rebalancing) where a
> datapath flow's actions may change, while it's wildcard pattern
> remains the same.  Before this patch, revalidators would note the
> change, delete the flow, and wait for the handlers to install an
> updated version.  This is inefficient, as many packets could get
> punted to userspace before the new flow is finally installed.
> 
> To improve the situation, this patch implements in place modification
> of datapath flows.  If the revalidators detect the only change to a
> given ukey is its actions, instead of deleting it, it does a put with
> the MODIFY flag set.
> 
> Signed-off-by: Ethan J. Jackson 
> ---
> ofproto/ofproto-dpif-upcall.c | 164 +++---
> tests/ofproto-dpif.at |  40 +++
> 2 files changed, 146 insertions(+), 58 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index c57cebd..f22fb05 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -150,6 +150,12 @@ enum upcall_type {
> IPFIX_UPCALL/* Per-bridge sampling. */
> };
> 
> +enum reval_result {
> +UKEY_KEEP,
> +UKEY_DELETE,
> +UKEY_MODIFY
> +};
> +
> struct upcall {
> struct ofproto_dpif *ofproto;  /* Parent ofproto. */
> const struct recirc_id_node *recirc; /* Recirculation context. */
> @@ -1694,33 +1700,41 @@ should_revalidate(const struct udpif *udpif, uint64_t 
> packets,
> return false;
> }
> 
> -static bool
> +/* Verifies that the datapath actions of 'ukey' are still correct, and pushes
> + * 'stats' for it.
> + *
> + * Returns a recommended action for 'ukey', options include:
> + *  UKEY_DELETE The ukey should be deleted.
> + *  UKEY_KEEP   The ukey is fine as is.
> + *  UKEY_MODIFY The ukey's actions should be changed but is otherwise
> + *  fine.  Callers should change the actions to those found
> + *  in the caller supplied 'odp_actions' buffer. */
> +static enum reval_result
> revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
> -const struct dpif_flow_stats *stats, uint64_t reval_seq)
> +const struct dpif_flow_stats *stats,
> +struct ofpbuf *odp_actions, uint64_t reval_seq)
> OVS_REQUIRES(ukey->mutex)
> {
> -uint64_t odp_actions_stub[1024 / 8];
> -struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
> -
> struct xlate_out xout, *xoutp;
> struct netflow *netflow;
> struct ofproto_dpif *ofproto;
> struct dpif_flow_stats push;
> struct flow flow, dp_mask;
> struct flow_wildcards wc;
> +enum reval_result result;
> uint64_t *dp64, *xout64;
> ofp_port_t ofp_in_port;
> struct xlate_in xin;
> long long int last_used;
> int error;
> size_t i;
> -bool ok;
> bool need_revalidate;
> 
> -ok = false;
> +result = UKEY_DELETE;
> xoutp = NULL;
> netflow = NULL;
> 
> +ofpbuf_clear(odp_actions);
> need_revalidate = (ukey->reval_seq != reval_seq);
> last_used = ukey->stats.used;
> push.used = stats->used;
> @@ -1734,20 +1748,19 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
> *ukey,
> 
> if (need_revalidate && last_used
> && !should_revalidate(udpif, push.n_packets, last_used)) {
> -ok = false;
> goto exit;
> }
> 
> /* We will push the stats, so update the ukey stats cache. */
> ukey->stats = *stats;
> if (!push.n_packets && !need_revalidate) {
> -ok = true;
> +result = UKEY_KEEP;
> goto exit;
> }
> 
> if (ukey->xcache && !need_revalidate) {
> xlate_push_stats(ukey->xcache, &push);
> -ok = true;
> +result = UKEY_KEEP;
> goto exit;
> }
> 
> @@ -1770,7 +1783,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
> *ukey,
> }
> 
> xlate_in_init(&xin, ofproto, &flow, ofp_in_port, NULL, push.tcp_flags,
> -  NULL, need_revalidate ? &wc : NULL, &odp_actions);
> +  NULL, need_revalidate ? &wc : NULL, odp_actions);
> if (push.n_packets) {
> xin.resubmit_stats = &push;
> xin.may_learn = true;
> @@ -1780,18 +1793,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
> *ukey,
> xoutp = &xout;
> 
> if (!need_revalidate) {
> -ok = true;
> +result = UKEY_KEEP;
> goto exit;
> }
> 
> if (xout.slow) {
> -ofpbuf_clear(&odp_actions);
> +ofpbuf_clear(odp_actions);
> compose_slow_path(udpif, &xout, &flow, flow.in_port.odp_port,
> -  &odp_actions);
> -}
> -
> -if (!ofpbuf_equal(&odp_actions, ukey->actions)) {
> -goto exit;
> +  odp_actions);
> }
>

[ovs-dev] [PATCH 2/2] ovn-sbctl: Print stage name in addition to table number.

[Justin Pettit]

Signed-off-by: Justin Pettit 
---
 ovn/utilities/ovn-sbctl.c |   11 +++
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index 9976215..06bc0ca 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -481,6 +481,7 @@ pre_get_info(struct ctl_context *ctx)
 ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_priority);
 ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_table_id);
 ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_match);
+ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_external_ids);
 }
 
 static struct cmd_show_table cmd_show_tables[] = {
@@ -686,10 +687,12 @@ cmd_lflow_list(struct ctl_context *ctx)
 lflow->pipeline);
 cur_pipeline = lflow->pipeline;
 }
-printf("  table_id=%" PRId64 ", priority=%3" PRId64 ", "
-   "match=(%s), action=(%s)\n",
-   lflow->table_id, lflow->priority,
-   lflow->match, lflow->actions);
+
+const char *table_name = smap_get(&lflow->external_ids, "stage-name");
+printf("  table=%" PRId64 "(%8s), priority=%3" PRId64
+   ", match=(%s), action=(%s)\n",
+   lflow->table_id, table_name ? table_name : "",
+   lflow->priority, lflow->match, lflow->actions);
 }
 
 free(lflows);
-- 
1.7.5.4

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

[ovs-dev] [PATCH 1/2] ovn-northd: Store name of the logical flow stage in external-ids.

[Justin Pettit]

This will be useful in a future commit.

It also introduces #define's for logical stages instead of in-place
constants.

Signed-off-by: Justin Pettit 
---
 ovn/northd/ovn-northd.c |   86 --
 ovn/ovn-sb.ovsschema|5 ++-
 ovn/ovn-sb.xml  |   11 ++
 3 files changed, 89 insertions(+), 13 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 796070f..d70ba39 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -54,6 +54,35 @@ static const char *ovnsb_db;
 
 static const char *default_db(void);
 
+
+/* Ingress pipeline stages.
+ *
+ * These must be listed in the order that the stages will be executed. */
+#define INGRESS_STAGES \
+INGRESS_STAGE(PORT_SEC, port_sec)  \
+INGRESS_STAGE(L2_LKUP, l2_lkup)
+
+enum ingress_stage {
+#define INGRESS_STAGE(NAME, STR) S_IN_##NAME,
+INGRESS_STAGES
+#undef INGRESS_STAGE
+INGRESS_N_STAGES
+};
+
+/* Egress pipeline stages.
+ *
+ * These must be listed in the order that the stages will be executed. */
+#define EGRESS_STAGES \
+EGRESS_STAGE(ACL, acl)\
+EGRESS_STAGE(PORT_SEC, port_sec)
+
+enum egress_stage {
+#define EGRESS_STAGE(NAME, STR) S_OUT_##NAME,
+EGRESS_STAGES
+#undef EGRESS_STAGE
+EGRESS_N_STAGES
+};
+
 static void
 usage(void)
 {
@@ -596,6 +625,26 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct 
ovn_datapath *od,
 lflow->actions = actions;
 }
 
+static const char *
+ingress_stage_to_str(int stage) {
+switch (stage) {
+#define INGRESS_STAGE(NAME, STR) case S_IN_##NAME: return #STR;
+INGRESS_STAGES
+#undef INGRESS_STAGE
+default: return "";
+}
+}
+
+static const char *
+egress_stage_to_str(int stage) {
+switch (stage) {
+#define EGRESS_STAGE(NAME, STR) case S_OUT_##NAME: return #STR;
+EGRESS_STAGES
+#undef EGRESS_STAGE
+default: return "";
+}
+}
+
 /* Adds a row with the specified contents to the Logical_Flow table. */
 static void
 ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,
@@ -687,16 +736,18 @@ build_lflows(struct northd_context *ctx, struct hmap 
*datapaths,
 struct ovn_datapath *od;
 HMAP_FOR_EACH (od, key_node, datapaths) {
 /* Logical VLANs not supported. */
-ovn_lflow_add(&lflows, od, P_IN, 0, 100, "vlan.present", "drop;");
+ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 100, "vlan.present",
+  "drop;");
 
 /* Broadcast/multicast source address is invalid. */
-ovn_lflow_add(&lflows, od, P_IN, 0, 100, "eth.src[40]", "drop;");
+ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 100, "eth.src[40]",
+  "drop;");
 
 /* Port security flows have priority 50 (see below) and will continue
  * to the next table if packet source is acceptable. */
 
 /* Otherwise drop the packet. */
-ovn_lflow_add(&lflows, od, P_IN, 0, 0, "1", "drop;");
+ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 0, "1", "drop;");
 }
 
 /* Ingress table 0: Ingress port security (priority 50). */
@@ -708,7 +759,7 @@ build_lflows(struct northd_context *ctx, struct hmap 
*datapaths,
 build_port_security("eth.src",
 op->nb->port_security, op->nb->n_port_security,
 &match);
-ovn_lflow_add(&lflows, op->od, P_IN, 0, 50, ds_cstr(&match),
+ovn_lflow_add(&lflows, op->od, P_IN, S_IN_PORT_SEC, 50, 
ds_cstr(&match),
   lport_is_enabled(op->nb) ? "next;" : "drop;");
 ds_destroy(&match);
 }
@@ -721,7 +772,7 @@ build_lflows(struct northd_context *ctx, struct hmap 
*datapaths,
 }
 }
 HMAP_FOR_EACH (od, key_node, datapaths) {
-ovn_lflow_add(&lflows, od, P_IN, 1, 100, "eth.dst[40]",
+ovn_lflow_add(&lflows, od, P_IN, S_IN_L2_LKUP, 100, "eth.dst[40]",
   "outport = \""MC_FLOOD"\"; output;");
 }
 
@@ -740,7 +791,7 @@ build_lflows(struct northd_context *ctx, struct hmap 
*datapaths,
 ds_put_cstr(&actions, "outport = ");
 json_string_escape(op->nb->name, &actions);
 ds_put_cstr(&actions, "; output;");
-ovn_lflow_add(&lflows, op->od, P_IN, 1, 50,
+ovn_lflow_add(&lflows, op->od, P_IN, S_IN_L2_LKUP, 50,
   ds_cstr(&match), ds_cstr(&actions));
 ds_destroy(&actions);
 ds_destroy(&match);
@@ -759,7 +810,7 @@ build_lflows(struct northd_context *ctx, struct hmap 
*datapaths,
 /* Ingress table 1: Destination lookup for unknown MACs (priority 0). */
 HMAP_FOR_EACH (od, key_node, datapaths) {
 if (od->has_unknown) {
-ovn_lflow_add(&lflows, od, P_IN, 1, 0, "1",
+ovn_lflow_add(&lflows, od, P_IN, S_IN_L2_LKUP, 0, "1",
   "outport = \""MC_UNKNOWN"\"; output;");

[ovs-dev] [PATCH v4 1/2] ofproto-dpif-upcall: Make ukey actions modifiable with RCU.

[Ethan J. Jackson]

From: Ethan Jackson 

Future patches will need to modify ukey actions in some instances.
This patch makes this possible by protecting them with RCU.  It also
adds thread safety checks to enforce the new protection mechanism.

Signed-off-by: Ethan J. Jackson 
---
 ofproto/ofproto-dpif-upcall.c | 42 +-
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index e604aa3..2151831 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -215,7 +215,6 @@ struct udpif_key {
 size_t key_len;/* Length of 'key'. */
 const struct nlattr *mask; /* Datapath flow mask. */
 size_t mask_len;   /* Length of 'mask'. */
-struct ofpbuf *actions;/* Datapath flow actions as nlattrs. */
 ovs_u128 ufid; /* Unique flow identifier. */
 bool ufid_present; /* True if 'ufid' is in datapath. */
 uint32_t hash; /* Pre-computed hash for 'key'. */
@@ -228,6 +227,9 @@ struct udpif_key {
 uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. */
 bool flow_exists OVS_GUARDED; /* Ensures flows are only deleted
  once. */
+/* Datapath flow actions as nlattrs.  Protected by RCU.  Read with
+ * ukey_get_actions(), and write with ukey_set_actions(). */
+OVSRCU_TYPE(struct ofpbuf *) actions;
 
 struct xlate_cache *xcache OVS_GUARDED;   /* Cache for xlate entries that
* are affected by this ukey.
@@ -287,6 +289,8 @@ static struct udpif_key *ukey_create_from_upcall(struct 
upcall *,
 static int ukey_create_from_dpif_flow(const struct udpif *,
   const struct dpif_flow *,
   struct udpif_key **);
+static void ukey_get_actions(struct udpif_key *, const struct nlattr **actions,
+ size_t *size);
 static bool ukey_install_start(struct udpif *, struct udpif_key *ukey);
 static bool ukey_install_finish(struct udpif_key *ukey, int error);
 static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
@@ -1131,7 +1135,7 @@ process_upcall(struct udpif *udpif, struct upcall *upcall,
 if (upcall->sflow) {
 union user_action_cookie cookie;
 const struct nlattr *actions;
-int actions_len = 0;
+size_t actions_len = 0;
 struct dpif_sflow_actions sflow_actions;
 memset(&sflow_actions, 0, sizeof sflow_actions);
 memset(&cookie, 0, sizeof cookie);
@@ -1149,8 +1153,7 @@ process_upcall(struct udpif *udpif, struct upcall *upcall,
 /* Lookup actions in userspace cache. */
 struct udpif_key *ukey = ukey_lookup(udpif, upcall->ufid);
 if (ukey) {
-actions = ukey->actions->data;
-actions_len = ukey->actions->size;
+ukey_get_actions(ukey, &actions, &actions_len);
 dpif_sflow_read_actions(flow, actions, actions_len,
 &sflow_actions);
 }
@@ -1277,8 +1280,8 @@ handle_upcalls(struct udpif *udpif, struct upcall 
*upcalls,
 op->dop.u.flow_put.mask_len = ukey->mask_len;
 op->dop.u.flow_put.ufid = upcall->ufid;
 op->dop.u.flow_put.stats = NULL;
-op->dop.u.flow_put.actions = ukey->actions->data;
-op->dop.u.flow_put.actions_len = ukey->actions->size;
+ukey_get_actions(ukey, &op->dop.u.flow_put.actions,
+ &op->dop.u.flow_put.actions_len);
 }
 
 if (upcall->odp_actions.size) {
@@ -1344,6 +1347,24 @@ ukey_lookup(struct udpif *udpif, const ovs_u128 *ufid)
 return NULL;
 }
 
+/* Provides safe lockless access of RCU protected 'ukey->actions'.  Callers may
+ * alternatively access the field directly if they take 'ukey->mutex'. */
+static void
+ukey_get_actions(struct udpif_key *ukey, const struct nlattr **actions, size_t 
*size)
+{
+const struct ofpbuf *buf = ovsrcu_get(struct ofpbuf *, &ukey->actions);
+*actions = buf->data;
+*size = buf->size;
+}
+
+static void
+ukey_set_actions(struct udpif_key *ukey, const struct ofpbuf *actions)
+{
+ovsrcu_postpone(ofpbuf_delete,
+ovsrcu_get_protected(struct ofpbuf *, &ukey->actions));
+ovsrcu_set(&ukey->actions, ofpbuf_clone(actions));
+}
+
 static struct udpif_key *
 ukey_create__(const struct nlattr *key, size_t key_len,
   const struct nlattr *mask, size_t mask_len,
@@ -1367,7 +1388,9 @@ ukey_create__(const struct nlattr *key, size_t key_len,
 ukey->ufid = *ufid;
 ukey->pmd_id = pmd_id;
 ukey->hash = get_ufid_hash(&ukey->ufid);
-ukey->actions = ofpbuf_clone(actions);
+
+ovsrcu_init(&ukey->act

Re: [ovs-dev] [PATCH v3 1/2] ofproto-dpif-upcall: Make ukey actions modifiable with RCU.

[Ethan Jackson]

agreed, sent a new version.

Ethan

On Wed, Aug 12, 2015 at 5:01 PM, Jarno Rajahalme  wrote:
>
>> On Aug 12, 2015, at 4:13 PM, Ethan J. Jackson  wrote:
>>
>> From: Ethan Jackson 
>>
>> Future patches will need to modify ukey actions in some instances.
>> This patch makes this possible by protecting them with RCU.  It also
>> adds thread safety checks to enforce the new protection mechanism.
>>
>> Signed-off-by: Ethan J. Jackson 
>> ---
>> ofproto/ofproto-dpif-upcall.c | 47 
>> +++
>> 1 file changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 0f2e186..c57cebd 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -215,7 +215,6 @@ struct udpif_key {
>> size_t key_len;/* Length of 'key'. */
>> const struct nlattr *mask; /* Datapath flow mask. */
>> size_t mask_len;   /* Length of 'mask'. */
>> -struct ofpbuf *actions;/* Datapath flow actions as nlattrs. */
>> ovs_u128 ufid; /* Unique flow identifier. */
>> bool ufid_present; /* True if 'ufid' is in datapath. */
>> uint32_t hash; /* Pre-computed hash for 'key'. */
>> @@ -228,6 +227,10 @@ struct udpif_key {
>> uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. */
>> bool flow_exists OVS_GUARDED; /* Ensures flows are only 
>> deleted
>>  once. */
>> +/* Datapath flow actions as nlattrs.  Protected by RCU.  Lockless reads 
>> can
>> + * be made with ukey_get_actions(), otherwise a lock is required. 
>> Updates
>> + * should be made with the ukey_set_actions() function. */
>> +const struct ofpbuf *actions OVS_GUARDED;
>>
>
> IMO it would be prudent to define this as:
>
> OVSRCU_TYPE(const struct ofpbuf *) actions;
>
> and then always use ovsrcu_get() to access it, and ovsrcu_set() to set it. 
> This makes sure the memory barriers are there, i.e., the compiler will not 
> rearrange any initialization of the new actions after the point in which 
> other threads can see it.
>
> With this the OVS_GUARDED would not be needed here.
>
>   Jarno
>
>> struct xlate_cache *xcache OVS_GUARDED;   /* Cache for xlate entries that
>>* are affected by this ukey.
>> @@ -287,6 +290,8 @@ static struct udpif_key *ukey_create_from_upcall(struct 
>> upcall *,
>> static int ukey_create_from_dpif_flow(const struct udpif *,
>>   const struct dpif_flow *,
>>   struct udpif_key **);
>> +static void ukey_get_actions(struct udpif_key *, const struct nlattr 
>> **actions,
>> + size_t *size);
>> static bool ukey_install_start(struct udpif *, struct udpif_key *ukey);
>> static bool ukey_install_finish(struct udpif_key *ukey, int error);
>> static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
>> @@ -1127,7 +1132,7 @@ process_upcall(struct udpif *udpif, struct upcall 
>> *upcall,
>> if (upcall->sflow) {
>> union user_action_cookie cookie;
>> const struct nlattr *actions;
>> -int actions_len = 0;
>> +size_t actions_len = 0;
>> struct dpif_sflow_actions sflow_actions;
>> memset(&sflow_actions, 0, sizeof sflow_actions);
>> memset(&cookie, 0, sizeof cookie);
>> @@ -1145,8 +1150,7 @@ process_upcall(struct udpif *udpif, struct upcall 
>> *upcall,
>> /* Lookup actions in userspace cache. */
>> struct udpif_key *ukey = ukey_lookup(udpif, upcall->ufid);
>> if (ukey) {
>> -actions = ukey->actions->data;
>> -actions_len = ukey->actions->size;
>> +ukey_get_actions(ukey, &actions, &actions_len);
>> dpif_sflow_read_actions(flow, actions, actions_len,
>> &sflow_actions);
>> }
>> @@ -1273,8 +1277,8 @@ handle_upcalls(struct udpif *udpif, struct upcall 
>> *upcalls,
>> op->dop.u.flow_put.mask_len = ukey->mask_len;
>> op->dop.u.flow_put.ufid = upcall->ufid;
>> op->dop.u.flow_put.stats = NULL;
>> -op->dop.u.flow_put.actions = ukey->actions->data;
>> -op->dop.u.flow_put.actions_len = ukey->actions->size;
>> +ukey_get_actions(ukey, &op->dop.u.flow_put.actions,
>> + &op->dop.u.flow_put.actions_len);
>> }
>>
>> if (upcall->odp_actions.size) {
>> @@ -1340,6 +1344,31 @@ ukey_lookup(struct udpif *udpif, const ovs_u128 *ufid)
>> return NULL;
>> }
>>
>> +/* Provides safe lockless access of RCU protected 'ukey->actions'.  Callers 
>> may
>> + * alternatively access the field directly if they take 'ukey->m

[ovs-dev] [PATCH v4 2/2] ofproto: Allow in-place modifications of datapath flows.

[Ethan J. Jackson]

From: Ethan Jackson 

There are certain use cases (such as bond rebalancing) where a
datapath flow's actions may change, while it's wildcard pattern
remains the same.  Before this patch, revalidators would note the
change, delete the flow, and wait for the handlers to install an
updated version.  This is inefficient, as many packets could get
punted to userspace before the new flow is finally installed.

To improve the situation, this patch implements in place modification
of datapath flows.  If the revalidators detect the only change to a
given ukey is its actions, instead of deleting it, it does a put with
the MODIFY flag set.

Signed-off-by: Ethan J. Jackson 
Acked-by: Jarno Rajahalme 
---
 ofproto/ofproto-dpif-upcall.c | 165 +++---
 tests/ofproto-dpif.at |  40 ++
 2 files changed, 146 insertions(+), 59 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 2151831..a0994a2 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -150,6 +150,12 @@ enum upcall_type {
 IPFIX_UPCALL/* Per-bridge sampling. */
 };
 
+enum reval_result {
+UKEY_KEEP,
+UKEY_DELETE,
+UKEY_MODIFY
+};
+
 struct upcall {
 struct ofproto_dpif *ofproto;  /* Parent ofproto. */
 const struct recirc_id_node *recirc; /* Recirculation context. */
@@ -1690,33 +1696,41 @@ should_revalidate(const struct udpif *udpif, uint64_t 
packets,
 return false;
 }
 
-static bool
+/* Verifies that the datapath actions of 'ukey' are still correct, and pushes
+ * 'stats' for it.
+ *
+ * Returns a recommended action for 'ukey', options include:
+ *  UKEY_DELETE The ukey should be deleted.
+ *  UKEY_KEEP   The ukey is fine as is.
+ *  UKEY_MODIFY The ukey's actions should be changed but is otherwise
+ *  fine.  Callers should change the actions to those found
+ *  in the caller supplied 'odp_actions' buffer. */
+static enum reval_result
 revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
-const struct dpif_flow_stats *stats, uint64_t reval_seq)
+const struct dpif_flow_stats *stats,
+struct ofpbuf *odp_actions, uint64_t reval_seq)
 OVS_REQUIRES(ukey->mutex)
 {
-uint64_t odp_actions_stub[1024 / 8];
-struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
-
 struct xlate_out xout, *xoutp;
 struct netflow *netflow;
 struct ofproto_dpif *ofproto;
 struct dpif_flow_stats push;
 struct flow flow, dp_mask;
 struct flow_wildcards wc;
+enum reval_result result;
 uint64_t *dp64, *xout64;
 ofp_port_t ofp_in_port;
 struct xlate_in xin;
 long long int last_used;
 int error;
 size_t i;
-bool ok;
 bool need_revalidate;
 
-ok = false;
+result = UKEY_DELETE;
 xoutp = NULL;
 netflow = NULL;
 
+ofpbuf_clear(odp_actions);
 need_revalidate = (ukey->reval_seq != reval_seq);
 last_used = ukey->stats.used;
 push.used = stats->used;
@@ -1730,20 +1744,19 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 
 if (need_revalidate && last_used
 && !should_revalidate(udpif, push.n_packets, last_used)) {
-ok = false;
 goto exit;
 }
 
 /* We will push the stats, so update the ukey stats cache. */
 ukey->stats = *stats;
 if (!push.n_packets && !need_revalidate) {
-ok = true;
+result = UKEY_KEEP;
 goto exit;
 }
 
 if (ukey->xcache && !need_revalidate) {
 xlate_push_stats(ukey->xcache, &push);
-ok = true;
+result = UKEY_KEEP;
 goto exit;
 }
 
@@ -1766,7 +1779,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 }
 
 xlate_in_init(&xin, ofproto, &flow, ofp_in_port, NULL, push.tcp_flags,
-  NULL, need_revalidate ? &wc : NULL, &odp_actions);
+  NULL, need_revalidate ? &wc : NULL, odp_actions);
 if (push.n_packets) {
 xin.resubmit_stats = &push;
 xin.may_learn = true;
@@ -1776,19 +1789,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 xoutp = &xout;
 
 if (!need_revalidate) {
-ok = true;
+result = UKEY_KEEP;
 goto exit;
 }
 
 if (xout.slow) {
-ofpbuf_clear(&odp_actions);
+ofpbuf_clear(odp_actions);
 compose_slow_path(udpif, &xout, &flow, flow.in_port.odp_port,
-  &odp_actions);
-}
-
-if (!ofpbuf_equal(&odp_actions,
-  ovsrcu_get(struct ofpbuf *, &ukey->actions))) {
-goto exit;
+  odp_actions);
 }
 
 if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, ukey->key,
@@ -1809,18 +1817,25 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 }
 }
 
-ok = true;
+if (!ofpbuf_equal(odp_actions,
+  ovsrcu_get(str

[ovs-dev] Mail System Error - Returned Mail

[Mail Delivery Subsystem]

Dear user dev@openvswitch.org, administration of openvswitch.org would like to 
inform you

We have found that your account has been used to send a large amount of junk 
e-mail during this week.
Most likely your computer had been infected by a recent virus and now contains 
a hidden proxy server.

Please follow the instruction 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

[ovs-dev] [PATCH] db-ctl-base: Allow print rows that weak reference to table in 'cmd_show_table'.

[Alex Wang]

Sometimes, it is desirable to print the table with weak reference to
the table specified in 'struct cmd_show_table'.  For example the
Port_Binding table rows in OVN_Southbound database that refer to the
same Chassis table row can be printed under the same chassis entry
in 'ovn-sbctl show' output.

To achieve it, this commit adds a new struct in 'struct cmd_show_table'
that allows users to print a table with weak reference to 'table'
specified in 'struct cmd_show_table'.  The 'ovn-sbctl' which now prints
the Port_Binding entries with Chassis table, is the first user of this
new feature.

Requested-by: Justin Pettit 
Signed-off-by: Alex Wang 
---
 lib/db-ctl-base.c |   45 +
 lib/db-ctl-base.h |   16 
 ovn/utilities/ovn-sbctl.c |   10 +++---
 tests/ovn-sbctl.at|   27 +++
 utilities/ovs-vsctl.c |   20 +---
 vtep/vtep-ctl.c   |   17 +++--
 6 files changed, 119 insertions(+), 16 deletions(-)

diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index 3028c09..cbafff1 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -1620,6 +1620,12 @@ pre_cmd_show(struct ctl_context *ctx)
 ovsdb_idl_add_column(ctx->idl, column);
 }
 }
+if (show->wref_table.name_column) {
+ovsdb_idl_add_column(ctx->idl, show->wref_table.name_column);
+}
+if (show->wref_table.wref_column) {
+ovsdb_idl_add_column(ctx->idl, show->wref_table.wref_column);
+}
 }
 }
 
@@ -1649,6 +1655,44 @@ cmd_show_find_table_by_name(const char *name)
 return NULL;
 }
 
+/*  Prints table entries that weak reference the 'cur_row'. */
+static void
+cmd_show_weak_ref(struct ctl_context *ctx, const struct cmd_show_table *show,
+  const struct ovsdb_idl_row *cur_row, int level)
+{
+const struct ovsdb_idl_row *row_wref;
+const struct ovsdb_idl_table_class *table = show->wref_table.table;
+const struct ovsdb_idl_column *name_column
+= show->wref_table.name_column;
+const struct ovsdb_idl_column *wref_column
+= show->wref_table.wref_column;
+
+if (!table || !name_column || !wref_column) {
+return;
+}
+
+ds_put_char_multiple(&ctx->output, ' ', (level + 1) * 4);
+ds_put_format(&ctx->output, "%s", table->name);
+ds_put_char(&ctx->output, '\n');
+
+for (row_wref = ovsdb_idl_first_row(ctx->idl, table); row_wref;
+ row_wref = ovsdb_idl_next_row(row_wref)) {
+const struct ovsdb_datum *wref_datum
+= ovsdb_idl_read(row_wref, wref_column);
+/* If weak reference refers to the 'cur_row', prints it. */
+if (wref_datum->n
+&& uuid_equals(&cur_row->uuid, &wref_datum->keys[0].uuid)) {
+const struct ovsdb_datum *name_datum
+= ovsdb_idl_read(row_wref, name_column);
+
+ds_put_char_multiple(&ctx->output, ' ', (level + 2) * 4);
+ds_put_format(&ctx->output, "%s: ", name_column->name);
+ovsdb_datum_to_string(name_datum, &name_column->type, 
&ctx->output);
+ds_put_char(&ctx->output, '\n');
+}
+}
+}
+
 /* 'shown' records the tables that has been displayed by the current
  * command to avoid duplicated prints.
  */
@@ -1753,6 +1797,7 @@ cmd_show_row(struct ctl_context *ctx, const struct 
ovsdb_idl_row *row,
 ds_put_char(&ctx->output, '\n');
 }
 }
+cmd_show_weak_ref(ctx, show, row, level);
 sset_find_and_delete_assert(shown, show->table->name);
 }
 
diff --git a/lib/db-ctl-base.h b/lib/db-ctl-base.h
index 00e86f8..209419a 100644
--- a/lib/db-ctl-base.h
+++ b/lib/db-ctl-base.h
@@ -148,6 +148,17 @@ struct ctl_command *ctl_parse_commands(int argc, char 
*argv[],
struct shash *local_options,
size_t *n_commandsp);
 
+/* Sometimes, it is desirable to print the table with weak reference to
+ * rows in a 'cmd_show_table' table.  In that case, the 'weak_ref_table'
+ * should be used and user must define all variables. */
+struct weak_ref_table {
+const struct ovsdb_idl_table_class *table;
+const struct ovsdb_idl_column *name_column;
+/* This colum must be a weak reference to the owning
+ * 'struct cmd_show_table''s table row. */
+const struct ovsdb_idl_column *wref_column;
+};
+
 /* This struct is for organizing the 'show' command output where:
  *
  * - 'table' is the table to show.
@@ -157,11 +168,16 @@ struct ctl_command *ctl_parse_commands(int argc, char 
*argv[],
  *
  * - 'columns[]' allows user to specify the print of additional columns
  *   in 'table'.
+ *
+ * - if 'wref_table' is filled, print the 'wref_table.table' rows that refer
+ *   to the 'table'.
+ *
  * */
 struct cmd_show_table {
 const struct ovsdb_idl_table_class *table;
 const struct ovsdb_idl_column *name_column;
 const str

Re: [ovs-dev] [PATCH 2/2] ovn-sbctl: Print stage name in addition to table number.

[Alex Wang]

Acked-by: Alex Wang 

On Wed, Aug 12, 2015 at 6:02 PM, Justin Pettit  wrote:

> Signed-off-by: Justin Pettit 
> ---
>  ovn/utilities/ovn-sbctl.c |   11 +++
>  1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> index 9976215..06bc0ca 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> @@ -481,6 +481,7 @@ pre_get_info(struct ctl_context *ctx)
>  ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_priority);
>  ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_table_id);
>  ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_match);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_external_ids);
>  }
>
>  static struct cmd_show_table cmd_show_tables[] = {
> @@ -686,10 +687,12 @@ cmd_lflow_list(struct ctl_context *ctx)
>  lflow->pipeline);
>  cur_pipeline = lflow->pipeline;
>  }
> -printf("  table_id=%" PRId64 ", priority=%3" PRId64 ", "
> -   "match=(%s), action=(%s)\n",
> -   lflow->table_id, lflow->priority,
> -   lflow->match, lflow->actions);
> +
> +const char *table_name = smap_get(&lflow->external_ids,
> "stage-name");
> +printf("  table=%" PRId64 "(%8s), priority=%3" PRId64
> +   ", match=(%s), action=(%s)\n",
> +   lflow->table_id, table_name ? table_name : "",
> +   lflow->priority, lflow->match, lflow->actions);
>  }
>
>  free(lflows);
> --
> 1.7.5.4
>
> ___
> 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 1/2] ovn-northd: Store name of the logical flow stage in external-ids.

[Alex Wang]

Acked-by: Alex Wang 

On Wed, Aug 12, 2015 at 6:02 PM, Justin Pettit  wrote:

> This will be useful in a future commit.
>
> It also introduces #define's for logical stages instead of in-place
> constants.
>
> Signed-off-by: Justin Pettit 
> ---
>  ovn/northd/ovn-northd.c |   86
> --
>  ovn/ovn-sb.ovsschema|5 ++-
>  ovn/ovn-sb.xml  |   11 ++
>  3 files changed, 89 insertions(+), 13 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 796070f..d70ba39 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -54,6 +54,35 @@ static const char *ovnsb_db;
>
>  static const char *default_db(void);
>
> +
> +/* Ingress pipeline stages.
> + *
> + * These must be listed in the order that the stages will be executed. */
> +#define INGRESS_STAGES \
> +INGRESS_STAGE(PORT_SEC, port_sec)  \
> +INGRESS_STAGE(L2_LKUP, l2_lkup)
> +
> +enum ingress_stage {
> +#define INGRESS_STAGE(NAME, STR) S_IN_##NAME,
> +INGRESS_STAGES
> +#undef INGRESS_STAGE
> +INGRESS_N_STAGES
> +};
> +
> +/* Egress pipeline stages.
> + *
> + * These must be listed in the order that the stages will be executed. */
> +#define EGRESS_STAGES \
> +EGRESS_STAGE(ACL, acl)\
> +EGRESS_STAGE(PORT_SEC, port_sec)
> +
> +enum egress_stage {
> +#define EGRESS_STAGE(NAME, STR) S_OUT_##NAME,
> +EGRESS_STAGES
> +#undef EGRESS_STAGE
> +EGRESS_N_STAGES
> +};
> +
>  static void
>  usage(void)
>  {
> @@ -596,6 +625,26 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> ovn_datapath *od,
>  lflow->actions = actions;
>  }
>
> +static const char *
> +ingress_stage_to_str(int stage) {
> +switch (stage) {
> +#define INGRESS_STAGE(NAME, STR) case S_IN_##NAME: return #STR;
> +INGRESS_STAGES
> +#undef INGRESS_STAGE
> +default: return "";
> +}
> +}
> +
> +static const char *
> +egress_stage_to_str(int stage) {
> +switch (stage) {
> +#define EGRESS_STAGE(NAME, STR) case S_OUT_##NAME: return #STR;
> +EGRESS_STAGES
> +#undef EGRESS_STAGE
> +default: return "";
> +}
> +}
> +
>  /* Adds a row with the specified contents to the Logical_Flow table. */
>  static void
>  ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,
> @@ -687,16 +736,18 @@ build_lflows(struct northd_context *ctx, struct hmap
> *datapaths,
>  struct ovn_datapath *od;
>  HMAP_FOR_EACH (od, key_node, datapaths) {
>  /* Logical VLANs not supported. */
> -ovn_lflow_add(&lflows, od, P_IN, 0, 100, "vlan.present", "drop;");
> +ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 100,
> "vlan.present",
> +  "drop;");
>
>  /* Broadcast/multicast source address is invalid. */
> -ovn_lflow_add(&lflows, od, P_IN, 0, 100, "eth.src[40]", "drop;");
> +ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 100,
> "eth.src[40]",
> +  "drop;");
>
>  /* Port security flows have priority 50 (see below) and will
> continue
>   * to the next table if packet source is acceptable. */
>
>  /* Otherwise drop the packet. */
> -ovn_lflow_add(&lflows, od, P_IN, 0, 0, "1", "drop;");
> +ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 0, "1", "drop;");
>  }
>
>  /* Ingress table 0: Ingress port security (priority 50). */
> @@ -708,7 +759,7 @@ build_lflows(struct northd_context *ctx, struct hmap
> *datapaths,
>  build_port_security("eth.src",
>  op->nb->port_security,
> op->nb->n_port_security,
>  &match);
> -ovn_lflow_add(&lflows, op->od, P_IN, 0, 50, ds_cstr(&match),
> +ovn_lflow_add(&lflows, op->od, P_IN, S_IN_PORT_SEC, 50,
> ds_cstr(&match),
>lport_is_enabled(op->nb) ? "next;" : "drop;");
>  ds_destroy(&match);
>  }
> @@ -721,7 +772,7 @@ build_lflows(struct northd_context *ctx, struct hmap
> *datapaths,
>  }
>  }
>  HMAP_FOR_EACH (od, key_node, datapaths) {
> -ovn_lflow_add(&lflows, od, P_IN, 1, 100, "eth.dst[40]",
> +ovn_lflow_add(&lflows, od, P_IN, S_IN_L2_LKUP, 100, "eth.dst[40]",
>"outport = \""MC_FLOOD"\"; output;");
>  }
>
> @@ -740,7 +791,7 @@ build_lflows(struct northd_context *ctx, struct hmap
> *datapaths,
>  ds_put_cstr(&actions, "outport = ");
>  json_string_escape(op->nb->name, &actions);
>  ds_put_cstr(&actions, "; output;");
> -ovn_lflow_add(&lflows, op->od, P_IN, 1, 50,
> +ovn_lflow_add(&lflows, op->od, P_IN, S_IN_L2_LKUP, 50,
>ds_cstr(&match), ds_cstr(&actions));
>  ds_destroy(&actions);
>  ds_destroy(&match);
> @@ -759,7 +810,7 @@ build_lflows(struct northd_context *ctx, struct hmap
> *datapaths,
>  /* Ingre

Re: [ovs-dev] [PATCH] db-ctl-base: Allow print rows that weak reference to table in 'cmd_show_table'.

[Justin Pettit]

> On Aug 12, 2015, at 9:49 PM, Alex Wang  wrote:
> 
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index 3028c09..cbafff1 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -1620,6 +1620,12 @@ pre_cmd_show(struct ctl_context *ctx)
> ovsdb_idl_add_column(ctx->idl, column);
> }
> }
> +if (show->wref_table.name_column) {
> +ovsdb_idl_add_column(ctx->idl, show->wref_table.name_column);
> +}
> +if (show->wref_table.wref_column) {
> +ovsdb_idl_add_column(ctx->idl, show->wref_table.wref_column);
> +}

Do you need to do a ovsdb_idl_add_table() on 'wref_table.table'?

> +/*  Prints table entries that weak reference the 'cur_row'. */
> +static void
> +cmd_show_weak_ref(struct ctl_context *ctx, const struct cmd_show_table *show,
> +  const struct ovsdb_idl_row *cur_row, int level)
> +{
> +const struct ovsdb_idl_row *row_wref;
> +const struct ovsdb_idl_table_class *table = show->wref_table.table;
> +const struct ovsdb_idl_column *name_column
> += show->wref_table.name_column;
> +const struct ovsdb_idl_column *wref_column
> += show->wref_table.wref_column;
> +
> +if (!table || !name_column || !wref_column) {
> +return;
> +}
> +
> +ds_put_char_multiple(&ctx->output, ' ', (level + 1) * 4);
> +ds_put_format(&ctx->output, "%s", table->name);
> +ds_put_char(&ctx->output, '\n');
> +
> +for (row_wref = ovsdb_idl_first_row(ctx->idl, table); row_wref;
> + row_wref = ovsdb_idl_next_row(row_wref)) {
> +const struct ovsdb_datum *wref_datum
> += ovsdb_idl_read(row_wref, wref_column);
> +/* If weak reference refers to the 'cur_row', prints it. */
> +if (wref_datum->n
> +&& uuid_equals(&cur_row->uuid, &wref_datum->keys[0].uuid)) {
> +const struct ovsdb_datum *name_datum
> += ovsdb_idl_read(row_wref, name_column);
> +
> +ds_put_char_multiple(&ctx->output, ' ', (level + 2) * 4);
> +ds_put_format(&ctx->output, "%s: ", name_column->name);
> +ovsdb_datum_to_string(name_datum, &name_column->type, 
> &ctx->output);
> +ds_put_char(&ctx->output, '\n');
> +}
> +}
> +}

I think this causes the "ovn-controller-vtep" test case to fail due to the text 
printing before the "for" loop; that gets printed regardless of whether there 
are any matches or not.  I originally wrote a patch to just not print that 
preamble if there were no matches.  However, I think the attached patch might 
be more consistent with commands like "ovs-vsctl show" which print the root 
name for a row on the same line as the table name.  It also passes the unit 
tests.

For example, here's the output of the original patch:

-=-=-=-=-=-=-=-=-=-=-
Chassis "56b18105-5706-46ef-80c4-ff20979ab068"
Encap geneve
ip: "127.0.0.1"
Port_Binding
logical_port: "sw0-port0"
logical_port: "sw0-port1"
-=-=-=-=-=-=-=-=-=-=-

Here it is with the attached patch:

-=-=-=-=-=-=-=-=-=-=-
Chassis "56b18105-5706-46ef-80c4-ff20979ab068"
Encap geneve
ip: "127.0.0.1"
Port_Binding "sw0-port0"
Port_Binding "sw0-port1"
-=-=-=-=-=-=-=-=-=-=-

Does that seem reasonable?  If we later want to add other columns from 
Port_Binding, they can then be placed under each entry.

> @@ -157,11 +168,16 @@ struct ctl_command *ctl_parse_commands(int argc, char 
> *argv[],
>  *
>  * - 'columns[]' allows user to specify the print of additional columns
>  *   in 'table'.
> + *
> + * - if 'wref_table' is filled, print the 'wref_table.table' rows that refer
> + *   to the 'table'.

What do you think of this phrasing instead?  I think it's a bit more 
descriptive.

 * - if 'wref_table' is populated, print 'wref_table.name_column' for
 *   each row in table 'wref_table.table' that has a reference to 'table'
 *   in 'wref_table.wref_column'.  Every field must be populated.

Thanks a lot for doing this on short notice!

Acked-by: Justin Pettit 

--Justin


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

diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index cbafff1..142428e 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -1671,10 +1671,6 @@ cmd_show_weak_ref(struct ctl_context *ctx, const struct c
 return;
 }
 
-ds_put_char_multiple(&ctx->output, ' ', (level + 1) * 4);
-ds_put_format(&ctx->output, "%s", table->name);
-ds_put_char(&ctx->output, '\n');
-
 for (row_wref = ovsdb_idl_first_row(ctx->idl, table); row_wref;
  row_wref = ovsdb_idl_next_row(row_wref)) {
 const struct ovsdb_datum *wref_datum
@@ -1685,8 +1681,8 @@ cmd_show_weak_ref(struct ctl_context *ctx, const struct cm
 const struct ovsdb_datum *name_datum
 = ovsdb_idl_read(row_wref, name_column);
 
-ds_put_char_multiple(&ctx->output, ' ', (level + 2) * 4);
-ds_put_format(&ctx->output, "%s: ", name_column->

Re: [ovs-dev] [PATCH 2/2] ovn-sbctl: Print stage name in addition to table number.

[Justin Pettit]

Thanks, Alex.  I pushed the series.

--Justin


> On Aug 12, 2015, at 10:12 PM, Alex Wang  wrote:
> 
> Acked-by: Alex Wang 
> 
> On Wed, Aug 12, 2015 at 6:02 PM, Justin Pettit  wrote:
> Signed-off-by: Justin Pettit 
> ---
>  ovn/utilities/ovn-sbctl.c |   11 +++
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> index 9976215..06bc0ca 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> @@ -481,6 +481,7 @@ pre_get_info(struct ctl_context *ctx)
>  ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_priority);
>  ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_table_id);
>  ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_match);
> +ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_external_ids);
>  }
> 
>  static struct cmd_show_table cmd_show_tables[] = {
> @@ -686,10 +687,12 @@ cmd_lflow_list(struct ctl_context *ctx)
>  lflow->pipeline);
>  cur_pipeline = lflow->pipeline;
>  }
> -printf("  table_id=%" PRId64 ", priority=%3" PRId64 ", "
> -   "match=(%s), action=(%s)\n",
> -   lflow->table_id, lflow->priority,
> -   lflow->match, lflow->actions);
> +
> +const char *table_name = smap_get(&lflow->external_ids, 
> "stage-name");
> +printf("  table=%" PRId64 "(%8s), priority=%3" PRId64
> +   ", match=(%s), action=(%s)\n",
> +   lflow->table_id, table_name ? table_name : "",
> +   lflow->priority, lflow->match, lflow->actions);
>  }
> 
>  free(lflows);
> --
> 1.7.5.4
> 
> ___
> 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] db-ctl-base: Allow print rows that weak reference to table in 'cmd_show_table'.

[Justin Pettit]

I should have added that if you use the attached patch, you'll need to update 
the ovn-sbctl unit test you were nice enough to include in this patch.

--Justin


> On Aug 12, 2015, at 11:52 PM, Justin Pettit  wrote:
> 
> 
>> On Aug 12, 2015, at 9:49 PM, Alex Wang  wrote:
>> 
>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
>> index 3028c09..cbafff1 100644
>> --- a/lib/db-ctl-base.c
>> +++ b/lib/db-ctl-base.c
>> @@ -1620,6 +1620,12 @@ pre_cmd_show(struct ctl_context *ctx)
>>ovsdb_idl_add_column(ctx->idl, column);
>>}
>>}
>> +if (show->wref_table.name_column) {
>> +ovsdb_idl_add_column(ctx->idl, show->wref_table.name_column);
>> +}
>> +if (show->wref_table.wref_column) {
>> +ovsdb_idl_add_column(ctx->idl, show->wref_table.wref_column);
>> +}
> 
> Do you need to do a ovsdb_idl_add_table() on 'wref_table.table'?
> 
>> +/*  Prints table entries that weak reference the 'cur_row'. */
>> +static void
>> +cmd_show_weak_ref(struct ctl_context *ctx, const struct cmd_show_table 
>> *show,
>> +  const struct ovsdb_idl_row *cur_row, int level)
>> +{
>> +const struct ovsdb_idl_row *row_wref;
>> +const struct ovsdb_idl_table_class *table = show->wref_table.table;
>> +const struct ovsdb_idl_column *name_column
>> += show->wref_table.name_column;
>> +const struct ovsdb_idl_column *wref_column
>> += show->wref_table.wref_column;
>> +
>> +if (!table || !name_column || !wref_column) {
>> +return;
>> +}
>> +
>> +ds_put_char_multiple(&ctx->output, ' ', (level + 1) * 4);
>> +ds_put_format(&ctx->output, "%s", table->name);
>> +ds_put_char(&ctx->output, '\n');
>> +
>> +for (row_wref = ovsdb_idl_first_row(ctx->idl, table); row_wref;
>> + row_wref = ovsdb_idl_next_row(row_wref)) {
>> +const struct ovsdb_datum *wref_datum
>> += ovsdb_idl_read(row_wref, wref_column);
>> +/* If weak reference refers to the 'cur_row', prints it. */
>> +if (wref_datum->n
>> +&& uuid_equals(&cur_row->uuid, &wref_datum->keys[0].uuid)) {
>> +const struct ovsdb_datum *name_datum
>> += ovsdb_idl_read(row_wref, name_column);
>> +
>> +ds_put_char_multiple(&ctx->output, ' ', (level + 2) * 4);
>> +ds_put_format(&ctx->output, "%s: ", name_column->name);
>> +ovsdb_datum_to_string(name_datum, &name_column->type, 
>> &ctx->output);
>> +ds_put_char(&ctx->output, '\n');
>> +}
>> +}
>> +}
> 
> I think this causes the "ovn-controller-vtep" test case to fail due to the 
> text printing before the "for" loop; that gets printed regardless of whether 
> there are any matches or not.  I originally wrote a patch to just not print 
> that preamble if there were no matches.  However, I think the attached patch 
> might be more consistent with commands like "ovs-vsctl show" which print the 
> root name for a row on the same line as the table name.  It also passes the 
> unit tests.
> 
> For example, here's the output of the original patch:
> 
> -=-=-=-=-=-=-=-=-=-=-
> Chassis "56b18105-5706-46ef-80c4-ff20979ab068"
>Encap geneve
>ip: "127.0.0.1"
>Port_Binding
>logical_port: "sw0-port0"
>logical_port: "sw0-port1"
> -=-=-=-=-=-=-=-=-=-=-
> 
> Here it is with the attached patch:
> 
> -=-=-=-=-=-=-=-=-=-=-
> Chassis "56b18105-5706-46ef-80c4-ff20979ab068"
>Encap geneve
>ip: "127.0.0.1"
>Port_Binding "sw0-port0"
>Port_Binding "sw0-port1"
> -=-=-=-=-=-=-=-=-=-=-
> 
> Does that seem reasonable?  If we later want to add other columns from 
> Port_Binding, they can then be placed under each entry.
> 
>> @@ -157,11 +168,16 @@ struct ctl_command *ctl_parse_commands(int argc, char 
>> *argv[],
>> *
>> * - 'columns[]' allows user to specify the print of additional columns
>> *   in 'table'.
>> + *
>> + * - if 'wref_table' is filled, print the 'wref_table.table' rows that refer
>> + *   to the 'table'.
> 
> What do you think of this phrasing instead?  I think it's a bit more 
> descriptive.
> 
> * - if 'wref_table' is populated, print 'wref_table.name_column' for
> *   each row in table 'wref_table.table' that has a reference to 'table'
> *   in 'wref_table.wref_column'.  Every field must be populated.
> 
> Thanks a lot for doing this on short notice!
> 
> Acked-by: Justin Pettit 
> 
> --Justin
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-=-
> 
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index cbafff1..142428e 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -1671,10 +1671,6 @@ cmd_show_weak_ref(struct ctl_context *ctx, const 
> struct c
> return;
> }
> 
> -ds_put_char_multiple(&ctx->output, ' ', (level + 1) * 4);
> -ds_put_format(&ctx->output, "%s", table->name);
> -ds_put_char(&ctx->output, '\n');
> -
> for (row_wref = ovsdb_idl_first_row(ctx->idl, table); row_wref;
>  row_wref = o