Re: [ovs-dev] [PATCH 05/12] dpif-netdev: Clear flow batches inside packet_batch_execute.

[Daniele Di Proietto]

2016-10-09 0:54 GMT-07:00 Fischetti, Antonio :

> Thanks Daniele,
> in this patch we moved the reset of batches[i].flow->batch
> from dp_netdev_input__() into packet_batch_per_flow_execute().
> So before calling dp_netdev_execute_actions() the flow->batch is already
> NULL.
>

batches[0]->flow->batch is already NULL, but batches[1]->flow->batch is not
NULL with this patch.  The lookup after recirculation could find
batches[1]->flow


> I think this should prevent recirculation, or am I missing some detail?
>

Suppose the datapath has the following flow table (I'm not sure something
like this is ever generated by the current translation code, but it's a
valid datapath flow table):

in_port(1),recird_id(0),...,tcp(src=80)
actions:set(tcp(src=443)),recirc(0)   #flow 0
in_port(1),recird_id(0),...,tcp(src=443) actions:output:2
 #flow 1

Suppose the following packets enter the datapath on port 1, in the same
input batch, in this order:

in_port(1),...,tcp(src=80)   #packet 0
in_port(1),...,tcp(src=443) #packet 1

Here's how the datapath behaves:

The batch is received and classified. We create two batches_per_flow

packet_batch_per_flow[0] = { .flow = flow1, .array = [packet 1] } #batch 0
packet_batch_per_flow[1] = { .flow = flow2, .array = [packet 2] } #batch 1

Flow 0 points to batch_per_flow[0] and flow 1 points to batch_per_flow[1].

Now the datapath starts to execute the batch 0. The recirculation will
execute immediately and it will find flow 1.  If we didn't clear flow
1->batch before executing batch 0 (with this patch we don't), the datapath
will append #packet 0 (after recirculation) to batch 1.  As I said, I
believe this to be wrong because we should have created a new batch.

Every time I look at that code again I need some time to re understand
this, and I actually got this wrong when I introduced the batch pointer
into struct dp_netdev_flow, so I agree it's quite complicated.  Please,
feel free to ask more if my reasoning seems wrong or if I'm making wrong
assumptions.

Thanks,

Daniele


> Antonio
>
> > -Original Message-
> > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Daniele Di
> > Proietto
> > Sent: Friday, October 7, 2016 11:46 PM
> > To: Jarno Rajahalme 
> > Cc: dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH 05/12] dpif-netdev: Clear flow batches
> > inside packet_batch_execute.
> >
> > This patch basically reverts 603f2ce04d00("dpif-netdev: Clear flow
> batches
> > before execute.")
> >
> > As explained in that commit message the problem is that
> > packet_batch_per_flow_execute() can trigger recirculation.  This means
> > that
> > we will call recursively dp_netdev_input__().  Here's a stack frame:
> >
> > dp_netdev_input__()
> > {
> > struct packet_batch_per_flow batches[PKT_ARRAY_SIZE];
> > /*...*/
> > packet_batch_per_flow_execute()
> > {
> > dp_execute_cb()
> > {
> > case OVS_ACTION_ATTR_RECIRC:
> > dp_netdev_input__() {
> > struct packet_batch_per_flow
> > batches[PKT_ARRAY_SIZE];
> > /*...*/
> > }
> > }
> >
> > }
> >
> > In the inner dp_netdev_input__() a lookup could theoretically find the
> > same
> > flows that it finds in the outer.
> > If we don't clear _all_ the batch pointers before calling the inner
> > dp_netdev_input__() we could find a flow that still has a pointer to a
> > batch in the outer dp_netdev_input__(). Then we will append packets to
> the
> > outer batch, which is clearly wrong.
> >
> > 2016-10-07 14:09 GMT-07:00 Jarno Rajahalme :
> >
> > > Daniele had a comment on this, I believe?
> > >
> > > > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy <
> > > bhanuprakash.bodire...@intel.com> wrote:
> > > >
> > > > There is a slight negative performance impact, by zeroing out the
> flow
> > > > batch pointers in dp_netdev_input__ ahead of packet_batch_execute().
> > The
> > > > issue has been observed with multiple batches test scenario.
> > > >
> > > > This patch fixes the problem by removing the extra for loop and clear
> > > > the flow batches inside the packet_batch_per_flow_execute(). Also
> the
> > > > vtune analysis showed that the overall no. of instructions retired
> for
> > > > dp_netdev_input__ reduced by 1,273,800,000 with this patch.
> > > >
> > > > Fixes: 603f2ce04d00 ("dpif-netdev: Clear flow batches before
> > execute.")
> > > > Signed-off-by: Bhanuprakash Bodireddy
> > 
> > > > Signed-off-by: Antonio Fischetti 
> > > > ---
> > > > lib/dpif-netdev.c | 5 +
> > > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > > index c002dd3..d0bb191 100644
> > > > --- a/lib/dpif-netdev.c
> > > > +++ b/lib/dpif-netdev.c
> > > > @@ -3878,6 +3878,7 @@ 

Re: [ovs-dev] Hyphens in address set names

[Kevin Lin]

Got it, thanks Ben.

On Thu, Oct 6, 2016 at 3:09 PM, Ben Pfaff  wrote:

> On Thu, Oct 06, 2016 at 02:56:44PM -0700, Kevin Lin wrote:
> > I’m a developer under Ethan Jackson on Quilt. We’re using address sets
> in our ACL match strings,
> > but they don’t seem to parse properly for names with hypens:
> >
> > error parsing match "ip4.src == $foo-bar": Syntax error at `$foo'
> expecting address set name.
> >
> > Is this expected?
>
> Address set names begin with $ followed by a letter or underscore or
> '.'; subsequent characters may also be digits.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] vagrant: cleanup before building

[Ben Pfaff]

On Sat, Oct 08, 2016 at 07:27:17AM -0300, Thadeu Lima de Souza Cascardo wrote:
> Clean the source directory before building, otherwise, build might fail if it
> has been configured already.
> 
> Only do it if there is a Makefile present, as suggested by Ben Pfaff.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo 
> Cc: Ben Pfaff 

Thanks for humoring me.  I applied this to master.  Please let me know
if you want me to backport it to branch-2.6 also.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 02/12] flow: Add comments to mf_get_next_in_map()

[Fischetti, Antonio]

Thanks Jarno, will follow your suggestions. That's really a good improvement 
and will help a lot to explain the function details.

Antonio

> -Original Message-
> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Jarno
> Rajahalme
> Sent: Friday, October 7, 2016 10:08 PM
> To: Bodireddy, Bhanuprakash 
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 02/12] flow: Add comments to
> mf_get_next_in_map()
> 
> 
> > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
>  wrote:
> >
> 
> A brief commit message should be included here. E.g.:
> 
> This patch adds comments to mf)get_next_in_map() to make it more
> comprehensible.
> 
> > Signed-off-by: Bhanuprakash Bodireddy 
> > Signed-off-by: Antonio Fischetti 
> > ---
> > lib/flow.h | 23 +--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/flow.h b/lib/flow.h
> > index ea24e28..4eb19ae 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -570,15 +570,27 @@ struct mf_for_each_in_map_aux {
> > const uint64_t *values;
> > };
> >
> > +/*
> > + * Parse the bitmap mask of the subtable and output the next value
> > + * from the search-key.
> > + */
> 
> While it is helpful to refer to higher level concepts such as subtable and
> search-key, I’d prefer the comment to rather state what the function does
> in terms of the abstractions at hand and then provide an example of use
> referring to subtables and such. Like this for example:
> 
> /* Get the data from ‘aux->values’ corresponding to the next lowest 1-bit
> in
>  * ‘aux->map’, given that ‘aux->values’ points to an array of 64-bit words
>  * corresponding to the 1-bits in ‘aux->fmap’, starting from the rightmost
> 1-bit.
>  *
>  * Returns ’true’ if the traversal is incomplete, ‘false’ otherwise. ‘aux’
> is prepared
>  * for the next iteration after each call.
>  *
>  * This is used to traverse through, for example, the values in a miniflow
>  * representation of a flow key selected by non-zero 64-bit words in a
>  * corresponding subtable mask. */
> 
> > static inline bool
> > mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
> >uint64_t *value)
> > {
> > -map_t *map, *fmap;
> > +map_t *map;/* map refers to the bitmap mask of the subtable. */
> > +map_t *fmap;   /* fmap refers to the bitmap of the search-key. */
> 
> Again, given that the example use was referred in the main comment above,
> these comments should stick to the abstractions at hand. Better yet, these
> comments should instead be placed into the aux struct definition above,
> e.g.:
> 
> struct mf_for_each_in_map_aux {
> size_t unit;  /* Current 64-bit unit of the
> flowmaps being processed. */
> struct flowmap fmap;  /* Remaining 1-bits corresponding to the 64-
> bit words in ‘values’
> struct flowmap map;   /* Remaining 1-bits corresponding to the 64-
> bit words of interest.
> const uint64_t *values;   /* 64-bit words corresponding to the 1-bits
> in ‘fmap’. */
> };
> 
> > map_t rm1bit;
> >
> > +/* The bitmap mask selects which value must be considered from the
> > + * search-key. We process the corresponding 'unit' of size 64 bits.
> > + * 'aux->unit' is an index to the current unit of 64 bits. */
> 
> Given the above comments this could be changed to:
> 
> /* Skip empty map units. */
> 
> > while (OVS_UNLIKELY(!*(map = >map.bits[aux->unit]))) {
> > -/* Skip remaining data in the previous unit. */
> > +/* No bits are enabled, so no search-key value will be output.
> > + * In case some of the corresponding data chunks in the search-
> key
> > + * bitmap are set, the data chunks must be skipped, as they are
> not
> > + * considered by this mask. This is handled by incrementing
> aux->values
> > + * accordingly. */
> 
> This comment is incorrect, as the determination of the iteration
> termination is done later (the ‘false’ return case).
> 
> How about this:
> 
> /* Skip remaining data in the current unit before advancing to the next.
> */
> 
> > aux->values += count_1bits(aux->fmap.bits[aux->unit]);
> > if (++aux->unit == FLOWMAP_UNITS) {
> > return false;
> > @@ -589,7 +601,12 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux
> *aux,
> > *map -= rm1bit;
> > fmap = >fmap.bits[aux->unit];
> >
> > +/* If the 8-byte value referred by 'rm1bit' is present in the
> > + * search-key return 'value', otherwise return 0. */
> 
> How about this instead:
> 
> /* If the rightmost 1-bit found from the current unit in ‘aux->map’
>  * (‘rm1bit’) is also present in ‘aux->fmap’, store the corresponding
>  * value from ‘aux->values’ to ‘*value', otherwise store 0. */
> 
> > if (OVS_LIKELY(*fmap & rm1bit)) {
> > +/* The value is in the search-key, if the 

Re: [ovs-dev] [PATCH 05/12] dpif-netdev: Clear flow batches inside packet_batch_execute.

[Fischetti, Antonio]

Thanks Daniele,
in this patch we moved the reset of batches[i].flow->batch 
from dp_netdev_input__() into packet_batch_per_flow_execute().
So before calling dp_netdev_execute_actions() the flow->batch is already NULL.

I think this should prevent recirculation, or am I missing some detail?

Antonio 

> -Original Message-
> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Daniele Di
> Proietto
> Sent: Friday, October 7, 2016 11:46 PM
> To: Jarno Rajahalme 
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 05/12] dpif-netdev: Clear flow batches
> inside packet_batch_execute.
> 
> This patch basically reverts 603f2ce04d00("dpif-netdev: Clear flow batches
> before execute.")
> 
> As explained in that commit message the problem is that
> packet_batch_per_flow_execute() can trigger recirculation.  This means
> that
> we will call recursively dp_netdev_input__().  Here's a stack frame:
> 
> dp_netdev_input__()
> {
> struct packet_batch_per_flow batches[PKT_ARRAY_SIZE];
> /*...*/
> packet_batch_per_flow_execute()
> {
> dp_execute_cb()
> {
> case OVS_ACTION_ATTR_RECIRC:
> dp_netdev_input__() {
> struct packet_batch_per_flow
> batches[PKT_ARRAY_SIZE];
> /*...*/
> }
> }
> 
> }
> 
> In the inner dp_netdev_input__() a lookup could theoretically find the
> same
> flows that it finds in the outer.
> If we don't clear _all_ the batch pointers before calling the inner
> dp_netdev_input__() we could find a flow that still has a pointer to a
> batch in the outer dp_netdev_input__(). Then we will append packets to the
> outer batch, which is clearly wrong.
> 
> 2016-10-07 14:09 GMT-07:00 Jarno Rajahalme :
> 
> > Daniele had a comment on this, I believe?
> >
> > > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy <
> > bhanuprakash.bodire...@intel.com> wrote:
> > >
> > > There is a slight negative performance impact, by zeroing out the flow
> > > batch pointers in dp_netdev_input__ ahead of packet_batch_execute().
> The
> > > issue has been observed with multiple batches test scenario.
> > >
> > > This patch fixes the problem by removing the extra for loop and clear
> > > the flow batches inside the packet_batch_per_flow_execute(). Also the
> > > vtune analysis showed that the overall no. of instructions retired for
> > > dp_netdev_input__ reduced by 1,273,800,000 with this patch.
> > >
> > > Fixes: 603f2ce04d00 ("dpif-netdev: Clear flow batches before
> execute.")
> > > Signed-off-by: Bhanuprakash Bodireddy
> 
> > > Signed-off-by: Antonio Fischetti 
> > > ---
> > > lib/dpif-netdev.c | 5 +
> > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > index c002dd3..d0bb191 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -3878,6 +3878,7 @@ packet_batch_per_flow_execute(struct
> > packet_batch_per_flow *batch,
> > > {
> > > struct dp_netdev_actions *actions;
> > > struct dp_netdev_flow *flow = batch->flow;
> > > +flow->batch = NULL;
> > >
> > > dp_netdev_flow_used(flow, batch->array.count, batch->byte_count,
> > > batch->tcp_flags, now);
> > > @@ -4173,10 +4174,6 @@ dp_netdev_input__(struct dp_netdev_pmd_thread
> > *pmd,
> > > }
> > >
> > > for (i = 0; i < n_batches; i++) {
> > > -batches[i].flow->batch = NULL;
> > > -}
> > > -
> > > -for (i = 0; i < n_batches; i++) {
> > > packet_batch_per_flow_execute([i], pmd, now);
> > > }
> > > }
> > > --
> > > 2.4.11
> > >
> > > ___
> > > 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
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3 1/4] ovn: ovn-ctl support for HA ovn DB servers

[Babu Shanmugam]

On Friday 07 October 2016 05:33 AM, Andy Zhou wrote:
Babu,  Thank you for working on this.  At a high level, it is not 
clear to me the boundary between ocf scripts and the ovn-ctl script -- 
i.e. which aspect is managed by which entity.  For example, 
1) which scripts are responsible for starting the ovsdb servers.
ovsdb servers are started by the pacemaker. It uses the OCF script and 
the OCF script uses ovn-ctl.


2) Which script should manage the fail-over -- I tried to shut down a 
cluster node using the "pcs" command, and fail-over did not happen.
The OCF script for OVN DB servers is capable of understanding the 
promote and demote calls. So, pacemaker will use this script to run 
ovsdb server in all the nodes and promote one node as the master(active 
server). If the node in which the master instance is running fails, 
pacemaker automatically promotes another node as the master. OCF script 
is an agent for the pacemaker for the OVN db resource.
The above behavior depends on the way you are configuring the resource 
that uses this OCF script. I am attaching a simple set of commands to 
configure the ovsdb server. You can create the resources after creating 
the cluster with the following command


crm configure < ovndb.pcmk

Please note, you have to replace the macros VM1_NAME, VM2_NAME, VM3_NAME 
and MASTER_IP with the respective values before using ovndb.pcmk. This 
script works with a 3 node cluster. I am assuming the node ids as 101, 
102, and 103. Please replace them as well to work with your cluster.



--
Babu
node $id="101" VM1_NAME
node $id="102" VM2_NAME
node $id="103" VM3_NAME
primitive ovndb_servers ocf:ovn:ovndb-servers params master_ip="MASTER_IP" op 
start interval="0s" timeout="30s" op stop interval="0s" timeout="20s" op 
promote interval="0s" timeout="50s" op demote interval="0s" timeout="50s" op 
monitor interval="10s" timeout="20s"
primitive ovnip ocf:heartbeat:IPaddr2 params ip="MASTER_IP" cidr_netmask="24" 
op start interval="0s" timeout="20s" op stop interval="0s" timeout="20s" op 
monitor interval="10s" timeout="20s"
ms ovndb_servers-master ovndb_servers meta notify="true"
colocation colocation-ovndb_servers-master-ovnip-INFINITY inf: 
ovndb_servers-master:Started ovnip:Master
order order-ovnip-ovndb_servers-master-mandatory inf: ovnip:start 
ovndb_servers-master:start
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev