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

2016-10-12 Thread Babu Shanmugam 



On Thursday 13 October 2016 07:26 AM, Andy Zhou wrote:



On Sun, Oct 9, 2016 at 12:02 AM, Babu Shanmugam > wrote:




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


Unfortunately, CRM is not distributed with pacemaker on centos 
anymore.  It took me some time to get it installed.  I think other may 
ran into similar issues, so
it may be worth while do document this, or change the script to use 
"pcs" which is part of the distribution.




I agree. Is INSTALL*.md good enough? In openstack, we are managing the 
resource through puppet manifests.




I adapted the script with my setup.  I have two nodes, 
"h1"(10.33.74.77) and "h2"(10.33.75.158), For Master_IP, I used 
10.33.75.220.


This is the output of crm configure show:

--

 [root@h2 azhou]# crm configure show

node1: h1 \

attributes

node2: h2

primitiveClusterIP IPaddr2 \

paramsip=10.33.75.200cidr_netmask=32\

opstart interval=0stimeout=20s\

opstop interval=0stimeout=20s\

opmonitor interval=30s

primitiveWebSite apache \

paramsconfigfile="/etc/httpd/conf/httpd.conf"statusurl="http://127.0.0.1/server-status"\

opstart interval=0stimeout=40s\

opstop interval=0stimeout=60s\

opmonitor interval=1min\

meta

primitiveovndb ocf:ovn:ovndb-servers \

opstart interval=0stimeout=30s\

opstop interval=0stimeout=20s\

oppromote interval=0stimeout=50s\

opdemote interval=0stimeout=50s\

opmonitor interval=1min\

meta

colocationcolocation-WebSite-ClusterIP-INFINITY inf: WebSiteClusterIP

orderorder-ClusterIP-WebSite-mandatory ClusterIP:start WebSite:start

propertycib-bootstrap-options: \

have-watchdog=false\

dc-version=1.1.13-10.el7_2.4-44eb2dd\

cluster-infrastructure=corosync\

cluster-name=mycluster\

stonith-enabled=false




You seem to have configured ovndb just as a primitive resource and not 
as a master slave resource. And there is no colocation resource 
configured for the ovndb with ClusterIP. Only with the colocation 
resource, ovndb server will be co-located with the ClusterIP resource.  
You will have to include the following lines for crm configure. You can 
configure the same with pcs as well.


ms ovndb-master ovndb meta notify="true"
colocation colocation-ovndb-master-ClusterIP-INFINITY inf: 
ovndb-master:Started ClusterIP:Master
order order-ClusterIP-ovndb-master-mandatory inf: ClusterIP:start 
ovndb-master:start





I have also added firewall rules to allow access to TCP port 6642 and 
port 6641.



At this stage, crm_mon shows:

Last updated: Wed Oct 12 14:49:07 2016  Last change: Wed Oct 
12 13:58:55


 2016 by root via crm_attributeon h2

Stack: corosync

Current DC: h2 (version 1.1.13-10.el7_2.4-44eb2dd) - partition with quorum

2 nodes and 3 resources configured


Online: [ h1 h2 ]


ClusterIP(ocf::heartbeat:IPaddr2):Started h2

WebSite (ocf::heartbeat:apache):Started h2

ovndb (ocf::ovn:ovndb-servers):Started h1


Failed Actions:

* ovndb_start_0 on h2 'unknown error' (1): call=39, status=Timed Out, 
exitreason


='none',

last-rc-change='Wed Oct 12 14:43:03 2016', queued=0ms, exec=30003ms


---

Not sure what the error message on h2 is about, Notice ovndb service 
is now running on h1, while the cluster IP is on h2.




Looks like, the OCF script is not able to start the ovsdb servers in 
'h2' node (we are getting a timed-out status). You

[ovs-dev] [PATCH] netdev-dpdk: Assign value '0' to unsupported netdev features

2016-10-12 Thread Binbin Xu 
When OVS is used, DPDK doesn't support features 'advertised',
'supported' and 'peer'. If a physical port added to bridge, features
descirbed above can't be assigned, and the values are random.

Signed-off-by: Binbin Xu 
---
 lib/netdev-dpdk.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ab8c34f..848734c 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1948,9 +1948,9 @@ out:
 static int
 netdev_dpdk_get_features(const struct netdev *netdev,
  enum netdev_features *current,
- enum netdev_features *advertised OVS_UNUSED,
- enum netdev_features *supported OVS_UNUSED,
- enum netdev_features *peer OVS_UNUSED)
+ enum netdev_features *advertised,
+ enum netdev_features *supported,
+ enum netdev_features *peer)
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 struct rte_eth_link link;
@@ -1988,6 +1988,8 @@ netdev_dpdk_get_features(const struct netdev *netdev,
 *current |= NETDEV_F_AUTONEG;
 }
 
+*advertised = *supported = *peer = 0;
+
 return 0;
 }
 
-- 
2.9.3

___
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

2016-10-12 Thread Andy Zhou 
On Sun, Oct 9, 2016 at 12:02 AM, Babu Shanmugam  wrote:

>
>
> 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
>

Unfortunately, CRM is not distributed with pacemaker on centos anymore.  It
took me some time to get it installed.  I think other may ran into similar
issues, so
it may be worth while do document this, or change the script to use "pcs"
which is part of the distribution.


I adapted the script with my setup.  I have two nodes, "h1"(10.33.74.77)
and "h2"(10.33.75.158), For Master_IP, I used 10.33.75.220.

This is the output of crm configure show:

--

 [root@h2 azhou]# crm configure show

node 1: h1 \

attributes

node 2: h2

primitive ClusterIP IPaddr2 \

params ip=10.33.75.200 cidr_netmask=32 \

op start interval=0s timeout=20s \

op stop interval=0s timeout=20s \

op monitor interval=30s

primitive WebSite apache \

params configfile="/etc/httpd/conf/httpd.conf" statusurl="
http://127.0.0.1/server-status; \

op start interval=0s timeout=40s \

op stop interval=0s timeout=60s \

op monitor interval=1min \

meta

primitive ovndb ocf:ovn:ovndb-servers \

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=1min \

meta

colocation colocation-WebSite-ClusterIP-INFINITY inf: WebSite ClusterIP

order order-ClusterIP-WebSite-mandatory ClusterIP:start WebSite:start

property cib-bootstrap-options: \

have-watchdog=false \

dc-version=1.1.13-10.el7_2.4-44eb2dd \

cluster-infrastructure=corosync \

cluster-name=mycluster \

stonith-enabled=false




I have also added firewall rules to allow access to TCP port 6642 and port
6641.


At this stage, crm_mon shows:

Last updated: Wed Oct 12 14:49:07 2016  Last change: Wed Oct 12
13:58:55

 2016 by root via crm_attribute on h2

Stack: corosync

Current DC: h2 (version 1.1.13-10.el7_2.4-44eb2dd) - partition with quorum

2 nodes and 3 resources configured


Online: [ h1 h2 ]


ClusterIP (ocf::heartbeat:IPaddr2): Started h2

WebSite (ocf::heartbeat:apache):Started h2

ovndb   (ocf::ovn:ovndb-servers): Started h1


Failed Actions:

* ovndb_start_0 on h2 'unknown error' (1): call=39, status=Timed Out,
exitreason

='none',

last-rc-change='Wed Oct 12 14:43:03 2016', queued=0ms, exec=30003ms


---

Not sure what the error message on h2 is about, Notice ovndb service is now
running on h1, while the cluster IP is on h2.

Also, both server are running as a backup server:

[root@h1 azhou]# ovs-appctl -t /run/openvswitch/ovnsb_db.ctl
ovsdb-server/sync-status

state: backup

connecting: tcp:192.0.2.254:6642   // I specified the IP at
/etc/openvswitch/ovnsb-active.conf, But the file was over-written with
192.0.2.254


[root@h2 ovs]# ovs-appctl -t /run/openvswitch/ovnsb_db.ctl
ovsdb-server/sync-status

state: backup

replicating: tcp:10.33.74.77:6642   // The IP address was retained on h2

database: OVN_Southbound

---

Any suggestions on what I did wrong?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

2016-10-12 Thread Daniele Di Proietto 
Looks good to me, could you send this in the next version of the series?

Thanks,

Daniele

2016-10-11 9:14 GMT-07:00 Fischetti, Antonio :

> Thanks Daniele for all your explanations.
> Instead of the proposed changes, is it worth to add a comment to the code
> just to explain why that loop is needed? This way we will avoid other
> people attempting to make the same change in the future.
> Could be something like:
>
> +/* All the flow batches need to be reset before any call to
> +packet_batch_per_flow_execute() as it could potentially
> +trigger recirculation.
> +When a packet matching flow ‘j’ happens to be recirculated,
> +the nested call to dp_netdev_input__() could potentially
> +classify the packet as matching another flow - say 'k'.
> +It could happen that in the previous call to dp_netdev_input__()
> +that same flow 'k' had already its own batches[k] still waiting
> +to be served.  So if its ‘batch’ member is not reset, the
> +recirculated packet would be wrongly appended to batches[k]
> +of the 1st call to dp_netdev_input__(). */
>  for (i = 0; i < n_batches; i++) {
>  batches[i].flow->batch = NULL;
>  }
>
>
> From: Daniele Di Proietto [mailto:diproiet...@ovn.org]
> Sent: Monday, October 10, 2016 1:31 AM
> To: Fischetti, Antonio 
> Cc: Jarno Rajahalme ; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 05/12] dpif-netdev: Clear flow batches
> inside packet_batch_execute.
>
>
>
> 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

[ovs-dev] 答复: Re: [PATCH] netdev: Initialize netdev's features before getting them

2016-10-12 Thread xu . binbin1 
Hi Aaron,

Thanks for your suggestion, I'll resubmit a patch for DPDK, and take a 
look at
lib/netdev-bsd.c to confirm that the additional patch is needed or not.

Thanks,

Aaron Conole  写于 2016/10/12 21:26:14:

> 发件人:  Aaron Conole 
> 收件人:  Binbin Xu , 
> 抄送: dev@openvswitch.org
> 日期:  2016/10/12 21:26
> 主题: Re: [ovs-dev] [PATCH] netdev: Initialize netdev's features 
> before getting them
> 
> Hi Binbin,
> 
> Binbin Xu  writes:
> 
> > When OVS is used, DPDK doesn't support features 'advertised',
> > 'supported' and 'peer'. If a physical port added to bridge, features
> > descirbed above can't be assigned, and the values are random.
> >
> > Signed-off-by: Binbin Xu 
> > ---
> 
> Thanks for reporting this.  I consider this a bug in dpdk, not
> something that requires changing the netdev framework.  A look at other
> netdev classes confirms this.
> 
> Please re-submit something that fixes DPDK, like the following.  If I
> read it correctly, you might also submit an additional patch to cleanup
> the interface in lib/netdev-bsd.c (which seems to be 'mismatched').
> 
> NOTE patch is *completely* untested, not even compile tested.

I have tested the patch in my environment, you means that i didn't make 
the
unit test?

> 
> ---
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 39bf930..6f5ec43 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1978,13 +1978,15 @@ out:
>  static int
>  netdev_dpdk_get_features(const struct netdev *netdev,
>   enum netdev_features *current,
> - enum netdev_features *advertised OVS_UNUSED,
> - enum netdev_features *supported OVS_UNUSED,
> - enum netdev_features *peer OVS_UNUSED)
> + enum netdev_features *advertised,
> + enum netdev_features *supported,
> + enum netdev_features *peer)
>  {
>  struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>  struct rte_eth_link link;
> 
> +*advertised = *peer = *supported = 0;
> +
>  ovs_mutex_lock(>mutex);
>  link = dev->link;
>  ovs_mutex_unlock(>mutex);

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

Re: [ovs-dev] [PATCH 12/13] dpdk: New module with some code from netdev-dpdk.

2016-10-12 Thread Daniele Di Proietto 





On 12/10/2016 13:59, "Aaron Conole"  wrote:

>Daniele Di Proietto  writes:
>
>> There's a lot of code in netdev-dpdk which is not at all related to the
>> netdev interface, mostly the library initialization code.
>>
>> This commit moves it to a new 'dpdk' module, to simplify 'netdev-dpdk'.
>>
>> Also a new module 'dpdk-stub' is introduced to implement some functions
>> when DPDK is not available.  This replaces the old 'netdev-nodpdk'
>> module.
>>
>> Some redundant includes are removed or reorganized as a consequence.
>>
>> No functional change.
>>
>> CC: Aaron Conole 
>> Signed-off-by: Daniele Di Proietto 
>> ---
>
>First - thanks for this.  I like everything about 1-12/13.
>
>You have my ACK for the first 12 of the series (and my Tested-by: as
>well - though I have only tested --with-dpdk).

Thanks for the review, I added your acks and tested-bys and applied this
to master!

>
>Just one thing...
>
>> diff --git a/lib/netdev-nodpdk.c b/lib/dpdk-stub.c
>> similarity index 84%
>> rename from lib/netdev-nodpdk.c
>> rename to lib/dpdk-stub.c
>> index 45564d2..42196c4 100644
>> --- a/lib/netdev-nodpdk.c
>> +++ b/lib/dpdk-stub.c
>> @@ -1,4 +1,5 @@
>>  /*
>> + * Copyright (c) 2014, 2015, 2016 Nicira, Inc.
>>   * Copyright (c) 2016 Red Hat, Inc.
>>   *
>>   * Licensed under the Apache License, Version 2.0 (the "License");
>> @@ -15,7 +16,8 @@
>>   */
>>  
>>  #include 
>> -#include "netdev-dpdk.h"
>> +#include "dpdk.h"
>> +
>>  #include "smap.h"
>>  #include "ovs-thread.h"
>>  #include "openvswitch/vlog.h"
>> @@ -34,3 +36,16 @@ dpdk_init(const struct smap *ovs_other_config)
>>  ovsthread_once_done();
>>  }
>>  }
>> +
>> +void
>> +dpdk_set_lcore_id(unsigned cpu OVS_UNUSED)
>> +{
>> +/* Nothing */
>> +}
>> +
>> +const char *
>> +dpdk_get_vhost_sock_dir(void)
>> +{
>> +return NULL;
>> +}
>> +
>
>When applying, I get one warning from git here, for introducing this
>newline.
>
>   Applying: dpdk: New module with some code from netdev-dpdk.
>   .git/rebase-apply/patch:132: new blank line at EOF.
>   +
>   warning: 1 line adds whitespace errors.
>
>Just thought I'd point it out.  ;-)  It can probably be either ignored
>or fixed when applying to master.

after fixing this, thanks for noticing!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 06/13] netdev-dpdk: Do not abort if out of hugepage memory.

2016-10-12 Thread Daniele Di Proietto 





On 12/10/2016 16:23, "Jarno Rajahalme"  wrote:

>
>> On Oct 4, 2016, at 6:22 PM, Daniele Di Proietto  
>> wrote:
>> 
>> We can run out of hugepage memory coming from rte_*alloc() more easily
>> than heap coming from malloc().
>> 
>> Therefore:
>> 
>> * We should use hugepage memory if we're going to access it only in
>>  the slow path.
>
>
>Should this be “We should NOT use huge page…” instead?

Yep, thanks for noticing!

I fixed it

Daniele

>
>  Jarno
>
>> * We shouldn't abort if we're out of hugepage memory.
>> * We should gracefully handle out of memory conditions.
>> 
>> Signed-off-by: Daniele Di Proietto 
>> ---
>> lib/netdev-dpdk.c | 62 
>> ++-
>> 1 file changed, 38 insertions(+), 24 deletions(-)
>> 
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index fec3e68..f5523a9 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -436,19 +436,13 @@ dpdk_buf_size(int mtu)
>>  NETDEV_DPDK_MBUF_ALIGN);
>> }
>> 
>> -/* XXX: use dpdk malloc for entire OVS. in fact huge page should be used
>> - * for all other segments data, bss and text. */
>> -
>> +/* Allocates an area of 'sz' bytes from DPDK.  The memory is zero'ed.
>> + *
>> + * Unlike xmalloc(), this function can return NULL on failure. */
>> static void *
>> dpdk_rte_mzalloc(size_t sz)
>> {
>> -void *ptr;
>> -
>> -ptr = rte_zmalloc(OVS_VPORT_DPDK, sz, OVS_CACHE_LINE_SIZE);
>> -if (ptr == NULL) {
>> -out_of_memory();
>> -}
>> -return ptr;
>> +return rte_zmalloc(OVS_VPORT_DPDK, sz, OVS_CACHE_LINE_SIZE);
>> }
>> 
>> /* XXX this function should be called only by pmd threads (or by non pmd
>> @@ -483,6 +477,9 @@ dpdk_mp_create(int socket_id, int mtu)
>> unsigned mp_size;
>> 
>> dmp = dpdk_rte_mzalloc(sizeof *dmp);
>> +if (!dmp) {
>> +return NULL;
>> +}
>> dmp->socket_id = socket_id;
>> dmp->mtu = mtu;
>> dmp->refcount = 1;
>> @@ -801,17 +798,22 @@ netdev_dpdk_alloc(void)
>> return NULL;
>> }
>> 
>> -static void
>> -netdev_dpdk_alloc_txq(struct netdev_dpdk *dev, unsigned int n_txqs)
>> +static struct dpdk_tx_queue *
>> +netdev_dpdk_alloc_txq(unsigned int n_txqs)
>> {
>> +struct dpdk_tx_queue *txqs;
>> unsigned i;
>> 
>> -dev->tx_q = dpdk_rte_mzalloc(n_txqs * sizeof *dev->tx_q);
>> -for (i = 0; i < n_txqs; i++) {
>> -/* Initialize map for vhost devices. */
>> -dev->tx_q[i].map = OVS_VHOST_QUEUE_MAP_UNKNOWN;
>> -rte_spinlock_init(>tx_q[i].tx_lock);
>> +txqs = dpdk_rte_mzalloc(n_txqs * sizeof *txqs);
>> +if (txqs) {
>> +for (i = 0; i < n_txqs; i++) {
>> +/* Initialize map for vhost devices. */
>> +txqs[i].map = OVS_VHOST_QUEUE_MAP_UNKNOWN;
>> +rte_spinlock_init([i].tx_lock);
>> +}
>> }
>> +
>> +return txqs;
>> }
>> 
>> static int
>> @@ -874,13 +876,18 @@ netdev_dpdk_init(struct netdev *netdev, unsigned int 
>> port_no,
>> if (err) {
>> goto unlock;
>> }
>> -netdev_dpdk_alloc_txq(dev, netdev->n_txq);
>> +dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
>> } else {
>> -netdev_dpdk_alloc_txq(dev, OVS_VHOST_MAX_QUEUE_NUM);
>> +dev->tx_q = netdev_dpdk_alloc_txq(OVS_VHOST_MAX_QUEUE_NUM);
>> /* Enable DPDK_DEV_VHOST device and set promiscuous mode flag. */
>> dev->flags = NETDEV_UP | NETDEV_PROMISC;
>> }
>> 
>> +if (!dev->tx_q) {
>> +err = ENOMEM;
>> +goto unlock;
>> +}
>> +
>> ovs_list_push_back(_list, >list_node);
>> 
>> unlock:
>> @@ -1226,7 +1233,11 @@ netdev_dpdk_rxq_alloc(void)
>> {
>> struct netdev_rxq_dpdk *rx = dpdk_rte_mzalloc(sizeof *rx);
>> 
>> -return >up;
>> +if (rx) {
>> +return >up;
>> +}
>> +
>> +return NULL;
>> }
>> 
>> static struct netdev_rxq_dpdk *
>> @@ -2352,7 +2363,7 @@ netdev_dpdk_remap_txqs(struct netdev_dpdk *dev)
>> int *enabled_queues, n_enabled = 0;
>> int i, k, total_txqs = dev->up.n_txq;
>> 
>> -enabled_queues = dpdk_rte_mzalloc(total_txqs * sizeof *enabled_queues);
>> +enabled_queues = xcalloc(total_txqs, sizeof *enabled_queues);
>> 
>> for (i = 0; i < total_txqs; i++) {
>> /* Enabled queues always mapped to themselves. */
>> @@ -2379,7 +2390,7 @@ netdev_dpdk_remap_txqs(struct netdev_dpdk *dev)
>> VLOG_DBG("%2d --> %2d", i, dev->tx_q[i].map);
>> }
>> 
>> -rte_free(enabled_queues);
>> +free(enabled_queues);
>> }
>> 
>> /*
>> @@ -2620,7 +2631,7 @@ dpdk_ring_create(const char dev_name[], unsigned int 
>> port_no,
>> int err;
>> 
>> ivshmem = dpdk_rte_mzalloc(sizeof *ivshmem);
>> -if (ivshmem == NULL) {
>> +if (!ivshmem) {
>> return ENOMEM;
>> }
>> 
>> @@ -2977,7 +2988,10 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>> 
>> rte_free(dev->tx_q);
>> err = dpdk_eth_dev_init(dev);
>> -

Re: [ovs-dev] [PATCH 11/13] netdev-dpdk: Change vlog module name to 'netdev_dpdk'.

2016-10-12 Thread Daniele Di Proietto 





On 12/10/2016 13:27, "Ben Pfaff"  wrote:

>On Tue, Oct 04, 2016 at 06:22:22PM -0700, Daniele Di Proietto wrote:
>> It is customary to have the vlog module name similar to the filename.
>> Plus a following commit will introduce a 'dpdk' module.
>> 
>> Signed-off-by: Daniele Di Proietto 
>
>Acked-by: Ben Pfaff 

Thanks for all the reviews!

I applied the series to master until patch 12
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 03/13] netdev-dpdk: Refactor dpdk_mp_get().

2016-10-12 Thread Daniele Di Proietto 





On 12/10/2016 12:45, "Ben Pfaff"  wrote:

>On Tue, Oct 04, 2016 at 06:22:14PM -0700, Daniele Di Proietto wrote:
>> The error handling path in dpdk_mp_get() is getting complicated, it
>> even requires a boolean variable.
>> 
>> Simplify it by extracting the function dpdk_mp_create().
>> 
>> CC: Ilya Maximets 
>> Signed-off-by: Daniele Di Proietto 
>
>Here's a suggested additional incremental to fold in.  Completely
>untested.
>
>Acked-by: Ben Pfaff 

That seems better, thanks!

I put the incremental on patch 5 and applied this to master

>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index 15250dc..6af3ee3 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -504,15 +504,10 @@ dpdk_mp_create(int socket_id, int mtu)
>  mp_name, mp_size);
> }
> free(mp_name);
>-} while (!dmp->mp && rte_errno == ENOMEM && (mp_size /= 2) >= 
>MIN_NB_MBUF);
>-
>-if (dmp->mp == NULL) {
>-goto out;
>-}
>-
>-return dmp;
>-
>-out:
>+if (dmp->mp) {
>+return dmp;
>+}
>+} while (rte_errno == ENOMEM && (mp_size /= 2) >= MIN_NB_MBUF);
> rte_free(dmp);
> return NULL;
> }
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 04/13] netdev-dpdk: Use RCU for egress QoS.

2016-10-12 Thread Daniele Di Proietto 





On 12/10/2016 13:04, "Ben Pfaff"  wrote:

>On Tue, Oct 04, 2016 at 06:22:15PM -0700, Daniele Di Proietto wrote:
>> I think it's clearer to use RCU than to check for a pointer twice in the
>> fast path (before and after taking the spinlock). Now the spinlock is
>> integrated into 'qos_conf'.
>> 
>> 'qos_conf' objects cannot be modified, so, instead of having
>> 'qos_set()', we now have 'qos_is_equal()', which tells us if an object
>> must be destroyed and recreated.
>> 
>> With this patch we also avoid passing the netdev parameter to qos ops,
>> since it was unused most of the times.
>> 
>> Lastly, some duplication is removed.
>> 
>> CC: Ian Stokes 
>> Signed-off-by: Daniele Di Proietto 
>
>The use of rte_strerror(-error) in netdev_dpdk_set_qos() looks a little
>confusing to me.  Is ->qos_construct() supposed to return a negative
>number for errors?  But that doesn't seem to mesh with the assignment
>"error = EOPNOTSUPP;" earlier in netdev_dpdk_set_qos().  But maybe I'm
>not familiar enough with how DPDK reports errors.

You're right, there is a problem.  qos_construct() only returns positive
error codes, because it takes care of inverting the return value of
qos_construct().  I removed the minus sign


>
>Acked-by: Ben Pfaff 

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix crash in dpif_netdev_execute().

2016-10-12 Thread Daniele Di Proietto 





On 12/10/2016 12:37, "Ben Pfaff"  wrote:

>On Tue, Oct 04, 2016 at 04:25:35PM -0700, Daniele Di Proietto wrote:
>> dp_netdev_get_pmd() is allowed to return NULL (even if we call it with
>> NON_PMD_CORE_ID) for different reasons:
>> 
>> * Since we use RCU to protect pmd threads, it is possible that
>>   ovs_refcount_try_ref_rcu() has failed.
>> * During reconfiguration we destroy every thread.
>
>I'd recommend making the comment on dp_netdev_get_pmd() clearer, by
>mentioning that it can always return NULL, even with NON_PMD_CORE_ID as
>an argument.

Good point, done

>
>Acked-by: Ben Pfaff 

Thanks, I applied this to master and branch-2.{6,5,4}
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 02/13] netdev-nodpdk.c: Add missing copyright.

2016-10-12 Thread Daniele Di Proietto 





On 12/10/2016 12:43, "Ben Pfaff"  wrote:

>On Tue, Oct 04, 2016 at 06:22:13PM -0700, Daniele Di Proietto wrote:
>> Looks like we forgot to add the copyright headers to netdev-dpdk.h.
>> Looking at the contribution history of the file, this commit adds the
>> header with Red Hat copyright.
>> 
>> CC: Aaron Conole 
>> Signed-off-by: Daniele Di Proietto 
>
>I'm glad to see Aaron's ack here.
>
>Acked-by: Ben Pfaff 

Thanks, I applied this to master and branch-2.6
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 01/13] netdev-dpdk.h: Add missing copyright.

2016-10-12 Thread Daniele Di Proietto 





On 12/10/2016 12:42, "Ben Pfaff"  wrote:

>On Tue, Oct 04, 2016 at 06:22:12PM -0700, Daniele Di Proietto wrote:
>> Looks like we forgot to add the copyright headers to netdev-dpdk.h.
>> Looking at the contribution history of the file, this commit adds the
>> header with Nicira copyright.
>> 
>> Signed-off-by: Daniele Di Proietto 
>
>Acked-by: Ben Pfaff 

Thanks, I applied this to master and to branch-2.{6,5,4,3}
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 06/13] netdev-dpdk: Do not abort if out of hugepage memory.

2016-10-12 Thread Jarno Rajahalme 

> On Oct 4, 2016, at 6:22 PM, Daniele Di Proietto  
> wrote:
> 
> We can run out of hugepage memory coming from rte_*alloc() more easily
> than heap coming from malloc().
> 
> Therefore:
> 
> * We should use hugepage memory if we're going to access it only in
>  the slow path.


Should this be “We should NOT use huge page…” instead?

  Jarno

> * We shouldn't abort if we're out of hugepage memory.
> * We should gracefully handle out of memory conditions.
> 
> Signed-off-by: Daniele Di Proietto 
> ---
> lib/netdev-dpdk.c | 62 ++-
> 1 file changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index fec3e68..f5523a9 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -436,19 +436,13 @@ dpdk_buf_size(int mtu)
>  NETDEV_DPDK_MBUF_ALIGN);
> }
> 
> -/* XXX: use dpdk malloc for entire OVS. in fact huge page should be used
> - * for all other segments data, bss and text. */
> -
> +/* Allocates an area of 'sz' bytes from DPDK.  The memory is zero'ed.
> + *
> + * Unlike xmalloc(), this function can return NULL on failure. */
> static void *
> dpdk_rte_mzalloc(size_t sz)
> {
> -void *ptr;
> -
> -ptr = rte_zmalloc(OVS_VPORT_DPDK, sz, OVS_CACHE_LINE_SIZE);
> -if (ptr == NULL) {
> -out_of_memory();
> -}
> -return ptr;
> +return rte_zmalloc(OVS_VPORT_DPDK, sz, OVS_CACHE_LINE_SIZE);
> }
> 
> /* XXX this function should be called only by pmd threads (or by non pmd
> @@ -483,6 +477,9 @@ dpdk_mp_create(int socket_id, int mtu)
> unsigned mp_size;
> 
> dmp = dpdk_rte_mzalloc(sizeof *dmp);
> +if (!dmp) {
> +return NULL;
> +}
> dmp->socket_id = socket_id;
> dmp->mtu = mtu;
> dmp->refcount = 1;
> @@ -801,17 +798,22 @@ netdev_dpdk_alloc(void)
> return NULL;
> }
> 
> -static void
> -netdev_dpdk_alloc_txq(struct netdev_dpdk *dev, unsigned int n_txqs)
> +static struct dpdk_tx_queue *
> +netdev_dpdk_alloc_txq(unsigned int n_txqs)
> {
> +struct dpdk_tx_queue *txqs;
> unsigned i;
> 
> -dev->tx_q = dpdk_rte_mzalloc(n_txqs * sizeof *dev->tx_q);
> -for (i = 0; i < n_txqs; i++) {
> -/* Initialize map for vhost devices. */
> -dev->tx_q[i].map = OVS_VHOST_QUEUE_MAP_UNKNOWN;
> -rte_spinlock_init(>tx_q[i].tx_lock);
> +txqs = dpdk_rte_mzalloc(n_txqs * sizeof *txqs);
> +if (txqs) {
> +for (i = 0; i < n_txqs; i++) {
> +/* Initialize map for vhost devices. */
> +txqs[i].map = OVS_VHOST_QUEUE_MAP_UNKNOWN;
> +rte_spinlock_init([i].tx_lock);
> +}
> }
> +
> +return txqs;
> }
> 
> static int
> @@ -874,13 +876,18 @@ netdev_dpdk_init(struct netdev *netdev, unsigned int 
> port_no,
> if (err) {
> goto unlock;
> }
> -netdev_dpdk_alloc_txq(dev, netdev->n_txq);
> +dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
> } else {
> -netdev_dpdk_alloc_txq(dev, OVS_VHOST_MAX_QUEUE_NUM);
> +dev->tx_q = netdev_dpdk_alloc_txq(OVS_VHOST_MAX_QUEUE_NUM);
> /* Enable DPDK_DEV_VHOST device and set promiscuous mode flag. */
> dev->flags = NETDEV_UP | NETDEV_PROMISC;
> }
> 
> +if (!dev->tx_q) {
> +err = ENOMEM;
> +goto unlock;
> +}
> +
> ovs_list_push_back(_list, >list_node);
> 
> unlock:
> @@ -1226,7 +1233,11 @@ netdev_dpdk_rxq_alloc(void)
> {
> struct netdev_rxq_dpdk *rx = dpdk_rte_mzalloc(sizeof *rx);
> 
> -return >up;
> +if (rx) {
> +return >up;
> +}
> +
> +return NULL;
> }
> 
> static struct netdev_rxq_dpdk *
> @@ -2352,7 +2363,7 @@ netdev_dpdk_remap_txqs(struct netdev_dpdk *dev)
> int *enabled_queues, n_enabled = 0;
> int i, k, total_txqs = dev->up.n_txq;
> 
> -enabled_queues = dpdk_rte_mzalloc(total_txqs * sizeof *enabled_queues);
> +enabled_queues = xcalloc(total_txqs, sizeof *enabled_queues);
> 
> for (i = 0; i < total_txqs; i++) {
> /* Enabled queues always mapped to themselves. */
> @@ -2379,7 +2390,7 @@ netdev_dpdk_remap_txqs(struct netdev_dpdk *dev)
> VLOG_DBG("%2d --> %2d", i, dev->tx_q[i].map);
> }
> 
> -rte_free(enabled_queues);
> +free(enabled_queues);
> }
> 
> /*
> @@ -2620,7 +2631,7 @@ dpdk_ring_create(const char dev_name[], unsigned int 
> port_no,
> int err;
> 
> ivshmem = dpdk_rte_mzalloc(sizeof *ivshmem);
> -if (ivshmem == NULL) {
> +if (!ivshmem) {
> return ENOMEM;
> }
> 
> @@ -2977,7 +2988,10 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
> 
> rte_free(dev->tx_q);
> err = dpdk_eth_dev_init(dev);
> -netdev_dpdk_alloc_txq(dev, netdev->n_txq);
> +dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
> +if (!dev->tx_q) {
> +err = ENOMEM;
> +}
> 
> netdev_change_seq_changed(netdev);
> 
> -- 
> 2.9.3
> 
> ___
> dev

[ovs-dev] [PATCH] ofproto: Return the OFPC_BUNDLES bit in switch features reply.

2016-10-12 Thread Jarno Rajahalme 
Add definitions for the OpenFlow 1.5 specific capabilities bits
OFPC15_BUNDLES and OFPC15_FLOW_MONITORING.  Return the bundles
capability bit in switch features reply.

Reported-by: Andrej Leitner 
Signed-off-by: Jarno Rajahalme 
---
 include/openflow/openflow-1.5.h |  7 +++
 include/openvswitch/ofp-util.h  | 11 ---
 lib/ofp-print.c |  2 ++
 lib/ofp-util.c  |  8 ++--
 ofproto/ofproto.c   |  2 +-
 tests/ofp-print.at  | 12 
 6 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/include/openflow/openflow-1.5.h b/include/openflow/openflow-1.5.h
index 3649e6c..e3c7c6f 100644
--- a/include/openflow/openflow-1.5.h
+++ b/include/openflow/openflow-1.5.h
@@ -39,6 +39,13 @@
 
 #include 
 
+/* OpenFlow 1.5 specific capabilities
+ * (struct ofp_switch_features, member capabilities). */
+enum ofp15_capabilities {
+OFPC15_BUNDLES= 1 << 9,/* Switch supports bundles. */
+OFPC15_FLOW_MONITORING = 1 << 10,  /* Switch supports flow monitoring. */
+};
+
 /* Body for ofp15_multipart_request of type OFPMP_PORT_DESC. */
 struct ofp15_port_desc_request {
 ovs_be32 port_no; /* All ports if OFPP_ANY. */
diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
index f3cb624..e55d7b5 100644
--- a/include/openvswitch/ofp-util.h
+++ b/include/openvswitch/ofp-util.h
@@ -613,7 +613,7 @@ struct ofputil_phy_port {
 };
 
 enum ofputil_capabilities {
-/* OpenFlow 1.0, 1.1, 1.2, and 1.3 share these capability values. */
+/* All OpenFlow versions share these capability values. */
 OFPUTIL_C_FLOW_STATS = 1 << 0,  /* Flow statistics. */
 OFPUTIL_C_TABLE_STATS= 1 << 1,  /* Table statistics. */
 OFPUTIL_C_PORT_STATS = 1 << 2,  /* Port statistics. */
@@ -626,11 +626,16 @@ enum ofputil_capabilities {
 /* OpenFlow 1.0 only. */
 OFPUTIL_C_STP= 1 << 3,  /* 802.1d spanning tree. */
 
-/* OpenFlow 1.1, 1.2, and 1.3 share this capability. */
+/* OpenFlow 1.1+ only.  Note that this bit value does not match the one
+ * in the OpenFlow message. */
 OFPUTIL_C_GROUP_STATS= 1 << 4,  /* Group statistics. */
 
-/* OpenFlow 1.2 and 1.3 share this capability */
+/* OpenFlow 1.2+ only. */
 OFPUTIL_C_PORT_BLOCKED   = 1 << 8,  /* Switch will block looping ports */
+
+/* OpenFlow 1.5+ only. */
+OFPUTIL_C_BUNDLES = 1 << 9,  /* Switch supports bundles. */
+OFPUTIL_C_FLOW_MONITORING = 1 << 10, /* Switch supports flow monitoring. */
 };
 
 /* Abstract ofp_switch_features. */
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 0a68551..b7b5290 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -506,6 +506,8 @@ ofputil_capabilities_to_name(uint32_t bit)
 case OFPUTIL_C_STP:  return "STP";
 case OFPUTIL_C_GROUP_STATS:  return "GROUP_STATS";
 case OFPUTIL_C_PORT_BLOCKED: return "PORT_BLOCKED";
+case OFPUTIL_C_BUNDLES:  return "BUNDLES";
+case OFPUTIL_C_FLOW_MONITORING: return "FLOW_MONITORING";
 }
 
 return NULL;
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 0445968..84109d0 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4644,6 +4644,9 @@ BUILD_ASSERT_DECL((int) OFPUTIL_C_PORT_STATS == 
OFPC_PORT_STATS);
 BUILD_ASSERT_DECL((int) OFPUTIL_C_IP_REASM == OFPC_IP_REASM);
 BUILD_ASSERT_DECL((int) OFPUTIL_C_QUEUE_STATS == OFPC_QUEUE_STATS);
 BUILD_ASSERT_DECL((int) OFPUTIL_C_ARP_MATCH_IP == OFPC_ARP_MATCH_IP);
+BUILD_ASSERT_DECL((int) OFPUTIL_C_PORT_BLOCKED == OFPC12_PORT_BLOCKED);
+BUILD_ASSERT_DECL((int) OFPUTIL_C_BUNDLES == OFPC15_BUNDLES);
+BUILD_ASSERT_DECL((int) OFPUTIL_C_FLOW_MONITORING == OFPC15_FLOW_MONITORING);
 
 static uint32_t
 ofputil_capabilities_mask(enum ofp_version ofp_version)
@@ -4656,9 +4659,11 @@ ofputil_capabilities_mask(enum ofp_version ofp_version)
 case OFP12_VERSION:
 case OFP13_VERSION:
 case OFP14_VERSION:
+return OFPC_COMMON | OFPC12_PORT_BLOCKED;
 case OFP15_VERSION:
 case OFP16_VERSION:
-return OFPC_COMMON | OFPC12_PORT_BLOCKED;
+return OFPC_COMMON | OFPC12_PORT_BLOCKED | OFPC15_BUNDLES
+| OFPC15_FLOW_MONITORING;
 default:
 /* Caller needs to check osf->header.version itself */
 return 0;
@@ -4784,7 +4789,6 @@ ofputil_encode_switch_features(const struct 
ofputil_switch_features *features,
 osf->n_buffers = htonl(features->n_buffers);
 osf->n_tables = features->n_tables;
 
-osf->capabilities = htonl(features->capabilities & OFPC_COMMON);
 osf->capabilities = htonl(features->capabilities &
   ofputil_capabilities_mask(version));
 switch (version) {
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index de1c469..53b7226 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3295,7 +3295,7 @@ handle_features_request(struct ofconn *ofconn, const 
struct ofp_header *oh)

Re: [ovs-dev] [PATCH v1] Python-IDL: getattr after mutate fix

2016-10-12 Thread Amitabha Biswas 
Thanks! I reposted the patch with the update to the commit message.

Amitabha

> On Oct 12, 2016, at 1:16 PM, Russell Bryant  wrote:
> 
> 
> 
> On Wed, Oct 12, 2016 at 10:32 AM, Amitabha Biswas  > wrote:
> This commit returns the updated column value when getattr is done
> after a mutate operation is performed (but before the commit). It
> addresses the bug reported in
> http://openvswitch.org/pipermail/dev/2016-September/080120.html 
> 
> 
> You can also record this with a "Reported-at" header:
> 
> Reported-at: http://openvswitch.org/pipermail/dev/2016-September/080120.html 
> 
> 
> It would also be good to record the related commit that this fixes.
> 
> Fixes: a59912a0ee8e ("python: Add support for partial map and partial set 
> updates")
> 
> Signed-off-by: Amitabha Biswas  >
> 
> Your Signed-off-by doesn't match the commit author (your gmail address).  Can 
> you re-submit with the two matching?
>  
> Reported-by: Richard Theis >
> 
> Other than the Signed-off-by issue, I think this is ready to apply.
> 
> Thanks!
> 
> -- 
> Russell Bryant 

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

[ovs-dev] [PATCH v1] Python-IDL: getattr after mutate fix

2016-10-12 Thread Amitabha Biswas 
This commit returns the updated column value when getattr is done
after a mutate operation is performed (but before the commit).

Signed-off-by: Amitabha Biswas 

Reported-by: Richard Theis 

Reported-at: http://openvswitch.org/pipermail/dev/2016-September/080120.html

Fixes: a59912a0ee8e ("python: Add support for partial map and set updates")
---
 python/ovs/db/idl.py | 38 --
 tests/ovsdb-idl.at   | 19 ---
 tests/test-ovsdb.py  | 36 
 3 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 187e902..0178050 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -770,6 +770,7 @@ class Row(object):
 assert self._changes is not None
 assert self._mutations is not None
 
+column = self._table.columns[column_name]
 datum = self._changes.get(column_name)
 inserts = None
 if '_inserts' in self._mutations.keys():
@@ -784,24 +785,33 @@ class Row(object):
  (self.__class__.__name__,
   column_name))
 else:
-datum = inserts
-if column_name in self._data:
+datum = data.Datum.from_python(column.type,
+   inserts,
+   _row_to_uuid)
+elif column_name in self._data:
 datum = self._data[column_name]
-try:
+if column.type.is_set():
+dlist = datum.as_list()
 if inserts is not None:
-datum.extend(inserts)
+dlist.extend(list(inserts))
 if removes is not None:
-datum = [x for x in datum if x not in removes]
-except error.Error:
-pass
-try:
+removes_datum = data.Datum.from_python(column.type,
+  removes,
+  _row_to_uuid)
+removes_list = removes_datum.as_list()
+dlist = [x for x in dlist if x not in removes_list]
+datum = data.Datum.from_python(column.type, dlist,
+   _row_to_uuid)
+elif column.type.is_map():
+dmap = datum.to_python(_uuid_to_row)
 if inserts is not None:
-datum.merge(inserts)
+dmap.update(inserts)
 if removes is not None:
-for key in removes.keys():
-del datum[key]
-except error.Error:
-pass
+for key in removes:
+if key not in (inserts or {}):
+del dmap[key]
+datum = data.Datum.from_python(column.type, dmap,
+   _row_to_uuid)
 else:
 if inserts is None:
 raise AttributeError("%s instance has no attribute '%s'" %
@@ -870,7 +880,7 @@ class Row(object):
 vlog.err("attempting to write bad value to column %s (%s)"
  % (column_name, e))
 return
-if column_name in self._data:
+if self._data and column_name in self._data:
 # Remove existing key/value before updating.
 removes = self._mutations.setdefault('_removes', {})
 column_value = removes.setdefault(column_name, set())
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index e57a3a4..6326d32 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -1112,15 +1112,18 @@ OVSDB_CHECK_IDL_PY([partial-map idl],
 [['["idltest", {"op":"insert", "table":"simple2",
 
"row":{"name":"myString1","smap":["map",[["key1","value1"],["key2","value2"]]]} 
}]']
 ],
-  [?simple2:name,smap,imap 'partialmapinsertelement' 
'partialmapinsertmultipleelements' 'partialmapdelelements'],
+  [?simple2:name,smap,imap 'partialmapinsertelement' 
'partialmapinsertmultipleelements' 'partialmapdelelements' 
'partialmapmutatenew'],
 [[000: name=myString1 smap=[(key1 value1) (key2 value2)] imap=[]
 001: commit, status=success
 002: name=String2 smap=[(key1 myList1) (key2 value2)] imap=[(3 myids2)]
 003: commit, status=success
-004: name=String2 smap=[(key1 myList1) (key2 myList2) (key3 myList3)] imap=[(3 
myids2)]
+004: name=String2 smap=[(key1 myList1) (key2 myList2) (key3 myList3) (key4 
myList4)] imap=[(3 myids2)]
 005: commit, status=success
 006: name=String2 smap=[(key2 myList2)] imap=[(3 myids2)]
-007:

Re: [ovs-dev] [PATCH 01/12] dpcls: Use 32 packet batches for lookups.

2016-10-12 Thread Pravin Shelar 
On Tue, Oct 11, 2016 at 8:33 AM, Fischetti, Antonio
 wrote:
> Comments inline.
>
> Thanks,
> 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 01/12] dpcls: Use 32 packet batches for
>> lookups.
>>
>>
>> > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
>>  wrote:
>> >
>> > This patch increases the number of packets processed in a batch during a
>> > lookup from 16 to 32. Processing batches of 32 packets improves
>> > performance and also one of the internal loops can be avoided here.
>> >
>>
>> Can you provide some qualification of the performance test conditions? Do
>> you believe the performance difference applies universally?
>>
> [Antonio F] We saw a performance improvement in EMC disabled PHY2PHY loopback 
> testcase with 2 physical ports and few tens of active IXIA streams. With 1 
> subtable - flow: from port1 to port2 - we got a throughput increment of +150 
> kpps. With 4 subtables the increment was +80 kpps.
> Also we spent some time profiling OVS with VTune. When comparing stock ovs 
> with the patched one, it’s been observed that for dpcls_lookup(), there is 
> significant reduction in the overall retired instructions (reduced by 
> 729,800,000), CPI rate improved from 0.471 to 0.427 and Front-end bound 
> cycles reduced from 19.1% to 8.5%.
>
>
>> > Signed-off-by: Antonio Fischetti 
>> > Signed-off-by: Bhanuprakash Bodireddy 
>> > ---
>> > lib/dpif-netdev.c | 109 +++-
>> --
>> > 1 file changed, 46 insertions(+), 63 deletions(-)
>> >
>> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> > index 6e09e44..c002dd3 100644
>> > --- a/lib/dpif-netdev.c
>> > +++ b/lib/dpif-netdev.c
>> > @@ -4975,23 +4975,20 @@ dpcls_lookup(struct dpcls *cls, const struct
>> netdev_flow_key keys[],
>> >  int *num_lookups_p)
>> > {
>> > /* The received 'cnt' miniflows are the search-keys that will be
>> processed
>> > - * in batches of 16 elements.  N_MAPS will contain the number of
>> these
>> > - * 16-elements batches.  i.e. for 'cnt' = 32, N_MAPS will be 2.
>> The batch
>> > - * size 16 was experimentally found faster than 8 or 32. */
>> > -typedef uint16_t map_type;
>> > + * to find a matching entry into the available subtables.
>> > + * The number of bits in map_type is equal to NETDEV_MAX_BURST. */
>>
>> This needs a build time assertion to catch any future change in
>> NETDEV_MAX_BURST.
>>
>> Preferably, if you can verify that the compiler is able to remove the
>> unnecessary loop in this case, you should consider not removing the extra
>> loop that would kick in only if NETDEV_MAX_BURST becomes larger than 32.
>>
> [Antonio F] Is that ok if we add the following line after the MAP_BITS define?
> BUILD_ASSERT_DECL(MAP_BITS >= NETDEV_MAX_BURST);
>
> It seems NETDEV_MAX_BURST was reduced to 32 from 256 a while ago, and one of 
> the reasons
> being the slow VMs and packet drops at VMs. Can we safely assume that 
> NETDEV_MAX_BURST is
> unlikely to be increased as it has wider consequence on the system stability?
>
>
I think it is safe to assume it for now. Looking at the code I think
it can be easily increased to 64. So it is fine to add the assertion.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 12/13] dpdk: New module with some code from netdev-dpdk.

2016-10-12 Thread Aaron Conole 
Daniele Di Proietto  writes:

> There's a lot of code in netdev-dpdk which is not at all related to the
> netdev interface, mostly the library initialization code.
>
> This commit moves it to a new 'dpdk' module, to simplify 'netdev-dpdk'.
>
> Also a new module 'dpdk-stub' is introduced to implement some functions
> when DPDK is not available.  This replaces the old 'netdev-nodpdk'
> module.
>
> Some redundant includes are removed or reorganized as a consequence.
>
> No functional change.
>
> CC: Aaron Conole 
> Signed-off-by: Daniele Di Proietto 
> ---

First - thanks for this.  I like everything about 1-12/13.

You have my ACK for the first 12 of the series (and my Tested-by: as
well - though I have only tested --with-dpdk).

Just one thing...

> diff --git a/lib/netdev-nodpdk.c b/lib/dpdk-stub.c
> similarity index 84%
> rename from lib/netdev-nodpdk.c
> rename to lib/dpdk-stub.c
> index 45564d2..42196c4 100644
> --- a/lib/netdev-nodpdk.c
> +++ b/lib/dpdk-stub.c
> @@ -1,4 +1,5 @@
>  /*
> + * Copyright (c) 2014, 2015, 2016 Nicira, Inc.
>   * Copyright (c) 2016 Red Hat, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
> @@ -15,7 +16,8 @@
>   */
>  
>  #include 
> -#include "netdev-dpdk.h"
> +#include "dpdk.h"
> +
>  #include "smap.h"
>  #include "ovs-thread.h"
>  #include "openvswitch/vlog.h"
> @@ -34,3 +36,16 @@ dpdk_init(const struct smap *ovs_other_config)
>  ovsthread_once_done();
>  }
>  }
> +
> +void
> +dpdk_set_lcore_id(unsigned cpu OVS_UNUSED)
> +{
> +/* Nothing */
> +}
> +
> +const char *
> +dpdk_get_vhost_sock_dir(void)
> +{
> +return NULL;
> +}
> +

When applying, I get one warning from git here, for introducing this
newline.

   Applying: dpdk: New module with some code from netdev-dpdk.
   .git/rebase-apply/patch:132: new blank line at EOF.
   +
   warning: 1 line adds whitespace errors.

Just thought I'd point it out.  ;-)  It can probably be either ignored
or fixed when applying to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH ovs RFC 0/9] Introducing HW offload support for openvswitch

2016-10-12 Thread Pravin Shelar 
Sorry for jumping in a bit late. I have couple of high level comments below.

On Thu, Oct 6, 2016 at 10:10 AM, Rony Efraim  wrote:
> From: Joe Stringer [mailto:j...@ovn.org]  Sent: Thursday, October 06, 2016 
> 5:06 AM
>>
>> Subject: Re: [ovs-dev] [PATCH ovs RFC 0/9] Introducing HW offload support for
>> openvswitch
>>
>> On 27 September 2016 at 21:45, Paul Blakey  wrote:
>> > Openvswitch currently configures the kerenel datapath via netlink over an
>> internal ovs protocol.
>> >
>> > This patch series offers a new provider: dpif-netlink-tc that uses the
>> > tc flower protocol to offload ovs rules into HW data-path through 
>> > netdevices
>> that e.g represent NIC e-switch ports.
>> >
>> > The user can create a bridge with type: datapath_type=dpif-hw-netlink in
>> order to use this provider.
>> > This provider can be used to pass the tc flower rules to the HW for HW
>> offloads.
>> >
>> > Also introducing in this patch series a policy module in which the
>> > user can program a HW-offload policy. The policy module accept a ovs
>> > flow and returns a policy decision for each flow:NO_OFFLOAD or HW_ONLY --
>> currently the policy is to HW offload all rules.
>> >
>> > If the HW_OFFLOAD rule assignment fails the provider will fallback to the
>> system datapath.
>> >
>> > Flower was chosen b/c its sort of natural to state OVS DP rules for
>> > this classifier. However, the code can be extended to support other
>> > classifiers such as U32, eBPF, etc which have HW offloads as well.
>> >
>> > The use-case we are currently addressing is the newly introduced SRIOV
>> > switchdev mode in the Linux kernel which is introduced in version 4.8
>> > [1][2]. This series was tested against SRIOV VFs vports representors of the
>> Mellanox 100G ConnectX-4 series exposed by the mlx5 kernel driver.
>> >
>> > Paul and Shahar.
>> >
>> > [1]
>> > http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=5
>> > 13334e18a74f70c0be58c2eb73af1715325b870
>> > [2]
>> > http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=5
>> > 3d94892e27409bb2b48140207c0273b2ba65f61
>>
>> Thanks for submitting the series. Clearly this is a topic of interest for 
>> multiple
>> parties, and it's a good starting point to discuss.
>>
>> A few of us also discussed this topic today at netdev, so I'll list a few 
>> points that
>> we talked about and hopefully others can fill in the bits I miss.
> Thanks for summarize our meeting today.
> Attached a link to the pdf pic that show the idea (picture <= 1,000 words)
> https://drive.google.com/file/d/0B2Yjm5a810FsZEoxOUJHU0l3c01OODUwMzVseXBFOE5MSGxr/view?usp=sharing
>
>>
>> Positives
>> * Hardware offload decision is made in a module in userspace
>> * Layered dpif approach means that the tc-based hardware offload could sit in
>> front of kernel or userspace datapaths
>> * Separate dpif means that if you don't enable it, it doesn't affect you. 
>> Doesn't
>> litter another dpif implementation with offload logic.
>>
Because of better modularity and usage of existing kernel interfaces
for flow offload, I like this approach.

>> Drawbacks
>> * Additional dpif to maintain. Another implementation to change when
>> modifying dpif interface. Maybe this doesn't change too often, but there has
>> been some discussions recently about whether the flow_{put,get,del} should be
>> converted to use internal flow structures rather than OVS netlink
>> representation. This is one example of potential impact on development.
> [RONY] you are right, but I don't think we can add it outher way. I think 
> that the approach of use dpif_netlink will saved us a lot of maintenance.
>> * Fairly limited support for OVS matches and actions. For instance, it is 
>> not yet
>> useful for OVN-style pipeline. But that's not a limitation of the design, 
>> just the
>> current implementation.
> [RONY] sure, we intend to support OVN and connection tracking, we start with 
> the simple case.
>>
>> Other considerations
>> * Is tc flower filter setup rate and stats dump fast enough? How does it
>> compare to existing kernel datapath flow setup rate? Multiple threads 
>> inserting
>> at once? How many filters can be dumped per second?
>> etc.
> [RONY] we will test it, and will try to improve the TC if it will be needed
>
I think there are two part in flow offloading.
1. Time spent to Add the flow to TC.
2. Time spent on pushing the flow to hardware.

It would be interesting to know which one is dominant in this case.

>> * Currently for a given flow, it will exist in either the offloaded 
>> implementation
>> or the kernel datapath. Statistics are only drawn from one location. This is
>> consistent with how ofproto-dpif-upcall will insert flows - one flow_put
>> operation and one flow is inserted into the datapath. Correspondingly there 
>> is
>> one udpif_key which reflects the most recently used stats for this datapath
>> flow. There may be situations where flows need to be in both

Re: [ovs-dev] [PATCH 11/13] netdev-dpdk: Change vlog module name to 'netdev_dpdk'.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:22PM -0700, Daniele Di Proietto wrote:
> It is customary to have the vlog module name similar to the filename.
> Plus a following commit will introduce a 'dpdk' module.
> 
> Signed-off-by: Daniele Di Proietto 

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

Re: [ovs-dev] [ovs-dev, 10/13] netdev-dpdk: Use init() function to initialize classes.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:21PM -0700, Daniele Di Proietto wrote:
> It's better to use the classes init() functions to perform
> initialization required for classes.
> 
> This will make it easier to move dpdk_init__() to a separate module in a
> future commit.
> 
> No functional change.
> 
> Signed-off-by: Daniele Di Proietto 

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

[ovs-dev] Ich brauche Ihre Unterstützung zu investieren !!

2016-10-12 Thread Brownson Marbella 
Sehr geehrte Damen und Herren,

Ich brauche Ihre Unterstützung in Ihrem Land zu verlagern und zu investieren. 
Ich bitte Sie um Hilfe, weil ich nicht das Wissen über Geschäft und die Regeln, 
die ihr Land für eine sichere Investition führen.
Werden Sie versprechen, mit mir aufrichtig zu sein?
Bitte kontaktieren Sie mich für weitere Informationen Details!

Freundliche Grüße,
Ms Brownson Marbella.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 0/9 md->rst] Convert most INSTALL guides

2016-10-12 Thread Russell Bryant 
On Sat, Oct 8, 2016 at 12:30 PM, Stephen Finucane  wrote:

> I'm going to dripfeed the conversion patches to avoid overloading
> peoples (there are ~25 of them so far). This is the first batch.
>
> Stephen Finucane (9):
>   dist-docs: Add support for rST
>   doc: Convert INSTALL to rST
>   doc: Convert INSTALL.DPDK to rST
>   doc: Convert INSTALL.Debian to rST
>   doc: Convert INSTALL.Docker to rST
>   doc: Convert INSTALL.Windows to rST
>   doc: Convert INSTALL.userspace to rST
>   doc: Convert INSTALL.XenServer to rST
>   doc: Convert INSTALL.KVM to rST
>

Do you have the end result published anywhere?  It would be nice to see the
before and after before diving into the detailed reviews.

Thanks,

-- 
Russell Bryant


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

Re: [ovs-dev] [ovs-dev, 09/13] netdev-dpdk: Remove useless 'rte_eal_init_ret'.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:20PM -0700, Daniele Di Proietto wrote:
> If rte_eal_init() fails, we do not register the DPDK netdev classes,
> therefore it's impossible to reach the classes construct functions.
> 
> Signed-off-by: Daniele Di Proietto 

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

Re: [ovs-dev] [PATCH v1] Python-IDL: getattr after mutate fix

2016-10-12 Thread Russell Bryant 
On Wed, Oct 12, 2016 at 10:32 AM, Amitabha Biswas 
wrote:

> This commit returns the updated column value when getattr is done
> after a mutate operation is performed (but before the commit). It
> addresses the bug reported in
> http://openvswitch.org/pipermail/dev/2016-September/080120.html


You can also record this with a "Reported-at" header:

Reported-at: http://openvswitch.org/pipermail/dev/2016-September/080120.html

It would also be good to record the related commit that this fixes.

Fixes: a59912a0ee8e ("python: Add support for partial map and partial set
updates")

Signed-off-by: Amitabha Biswas 
>

Your Signed-off-by doesn't match the commit author (your gmail address).
Can you re-submit with the two matching?


> Reported-by: Richard Theis 


Other than the Signed-off-by issue, I think this is ready to apply.

Thanks!

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

Re: [ovs-dev] [PATCH 08/13] netdev-dpdk: Remove useless nonpmd_mempool_mutex.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:19PM -0700, Daniele Di Proietto wrote:
> Since DPDK commit 30e639989227("mempool: support non-EAL thread"),
> non-EAL threads can use the mempool API safely.  Plus, nonpmd threads
> access to netdev is already serialized with 'non_pmd_mutex' in
> dpif-netdev.
> 
> Signed-off-by: Daniele Di Proietto 

I don't really understand the commit message for that DPDK commit, but
if you're interpreting it correctly then I agree that this is correct
and an improvement.

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

Re: [ovs-dev] [PATCH 07/13] netdev-dpdk: Use xasprintf() when possible.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:18PM -0700, Daniele Di Proietto wrote:
> We're in the slowpath.  I find it easier to allocate and free memory,
> than to handle snprintf() error condition.
> 
> Signed-off-by: Daniele Di Proietto 

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

Re: [ovs-dev] [PATCH 06/13] netdev-dpdk: Do not abort if out of hugepage memory.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:17PM -0700, Daniele Di Proietto wrote:
> We can run out of hugepage memory coming from rte_*alloc() more easily
> than heap coming from malloc().
> 
> Therefore:
> 
> * We should use hugepage memory if we're going to access it only in
>   the slowpath.
> * We shouldn't abort if we're out of hugepage memory.
> * We should gracefully handle out of memory conditions.
> 
> Signed-off-by: Daniele Di Proietto 

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

Re: [ovs-dev] [PATCH 05/13] netdev-dpdk: Acquire dev->stats_lock only once.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:16PM -0700, Daniele Di Proietto wrote:
> Signed-off-by: Daniele Di Proietto 

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

Re: [ovs-dev] [PATCH 04/13] netdev-dpdk: Use RCU for egress QoS.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:15PM -0700, Daniele Di Proietto wrote:
> I think it's clearer to use RCU than to check for a pointer twice in the
> fast path (before and after taking the spinlock). Now the spinlock is
> integrated into 'qos_conf'.
> 
> 'qos_conf' objects cannot be modified, so, instead of having
> 'qos_set()', we now have 'qos_is_equal()', which tells us if an object
> must be destroyed and recreated.
> 
> With this patch we also avoid passing the netdev parameter to qos ops,
> since it was unused most of the times.
> 
> Lastly, some duplication is removed.
> 
> CC: Ian Stokes 
> Signed-off-by: Daniele Di Proietto 

The use of rte_strerror(-error) in netdev_dpdk_set_qos() looks a little
confusing to me.  Is ->qos_construct() supposed to return a negative
number for errors?  But that doesn't seem to mesh with the assignment
"error = EOPNOTSUPP;" earlier in netdev_dpdk_set_qos().  But maybe I'm
not familiar enough with how DPDK reports errors.

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

Re: [ovs-dev] [PATCH 02/13] netdev-nodpdk.c: Add missing copyright.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:13PM -0700, Daniele Di Proietto wrote:
> Looks like we forgot to add the copyright headers to netdev-dpdk.h.
> Looking at the contribution history of the file, this commit adds the
> header with Red Hat copyright.
> 
> CC: Aaron Conole 
> Signed-off-by: Daniele Di Proietto 

I'm glad to see Aaron's ack here.

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

Re: [ovs-dev] [PATCH 03/13] netdev-dpdk: Refactor dpdk_mp_get().

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:14PM -0700, Daniele Di Proietto wrote:
> The error handling path in dpdk_mp_get() is getting complicated, it
> even requires a boolean variable.
> 
> Simplify it by extracting the function dpdk_mp_create().
> 
> CC: Ilya Maximets 
> Signed-off-by: Daniele Di Proietto 

Here's a suggested additional incremental to fold in.  Completely
untested.

Acked-by: Ben Pfaff 

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 15250dc..6af3ee3 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -504,15 +504,10 @@ dpdk_mp_create(int socket_id, int mtu)
  mp_name, mp_size);
 }
 free(mp_name);
-} while (!dmp->mp && rte_errno == ENOMEM && (mp_size /= 2) >= MIN_NB_MBUF);
-
-if (dmp->mp == NULL) {
-goto out;
-}
-
-return dmp;
-
-out:
+if (dmp->mp) {
+return dmp;
+}
+} while (rte_errno == ENOMEM && (mp_size /= 2) >= MIN_NB_MBUF);
 rte_free(dmp);
 return NULL;
 }

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

Re: [ovs-dev] [PATCH 01/13] netdev-dpdk.h: Add missing copyright.

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 06:22:12PM -0700, Daniele Di Proietto wrote:
> Looks like we forgot to add the copyright headers to netdev-dpdk.h.
> Looking at the contribution history of the file, this commit adds the
> header with Nicira copyright.
> 
> Signed-off-by: Daniele Di Proietto 

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

Re: [ovs-dev] [PATCH] vagrant: add CentOS 7.2 support

2016-10-12 Thread Ben Pfaff 
On Tue, Sep 27, 2016 at 04:13:11PM -0300, Thadeu Lima de Souza Cascardo wrote:
> Signed-off-by: Thadeu Lima de Souza Cascardo 

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix crash in dpif_netdev_execute().

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 04, 2016 at 04:25:35PM -0700, Daniele Di Proietto wrote:
> dp_netdev_get_pmd() is allowed to return NULL (even if we call it with
> NON_PMD_CORE_ID) for different reasons:
> 
> * Since we use RCU to protect pmd threads, it is possible that
>   ovs_refcount_try_ref_rcu() has failed.
> * During reconfiguration we destroy every thread.

I'd recommend making the comment on dp_netdev_get_pmd() clearer, by
mentioning that it can always return NULL, even with NON_PMD_CORE_ID as
an argument.

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

Re: [ovs-dev] [PATCH] tests: Get rid of overly specific --pidfile and --unixctl options.

2016-10-12 Thread Ben Pfaff 
On Wed, Oct 12, 2016 at 12:00:00AM -0700, Andy Zhou wrote:
> On Wed, Oct 5, 2016 at 8:11 PM, Ben Pfaff  wrote:
> 
> > At an early point in OVS development, OVS was built with fixed default
> > directories for pidfiles and sockets.  This meant that it was necessary to
> > use lots of --pidfile and --unixctl options in the testsuite, to point the
> > daemons to where they should put these files (since the testsuite cannot
> > and generally should not touch the real system /var/run).  Later on,
> > the environment variables OVS_RUNDIR, OVS_LOGDIR, etc. were introduced
> > to override these defaults, and even later the testsuite was changed to
> > always set these variables correctly in every test.  Thus, these days it
> > isn't usually necessary to specify a filename on --pidfile or to specify
> > --unixctl at all.  However, many of the tests are built by cut-and-paste,
> > so they tended to keep appearing anyhow.  This commit drops most of them,
> > making the testsuite easier to read and understand.
> >
> > This commit also sweeps away some other historical detritus.  In
> > particular, in early days of the testsuite there was no way to
> > automatically kill daemons when a test failed (or otherwise ended).  This
> > meant that some tests were littered with calls to "kill `cat pidfile`" on
> > almost every line (or m4 macros that expanded to the same thing) so that if
> > a test failed partway through the testsuite would not hang waiting for a
> > daemon to die that was never going to die without manual intervention.
> > However, a long time ago we introduced the "on_exit" mechanism that
> > obsoletes this.  This commit eliminates a lot of the old litter of kill
> > invocations, which also makes those tests easier to read.
> >
> > Signed-off-by: Ben Pfaff 
> 
> 
> Acked-by: Andy Zhou 
> 
> Thanks for making those changes! One of the replication tests bothered me,
> so I made similar changes to cleanup
> that test. It did not occur to me that many tests had similar issues and
> can be cleaned up as well, until I saw this patch.

Thanks.  This commit makes me happy too.

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

Re: [ovs-dev] [PATCH] tests: Fix double-rebuild of testsuite for "check-valgrind".

2016-10-12 Thread Ben Pfaff 
On Wed, Oct 12, 2016 at 10:42:36AM -0700, Ben Pfaff wrote:
> On Tue, Oct 11, 2016 at 04:58:26PM -0700, Andy Zhou wrote:
> > On Wed, Oct 5, 2016 at 6:28 PM, Ben Pfaff  wrote:
> > 
> > > When I ran "make check-valgrind -j10" and the testsuite needed to be
> > > rebuilt, two copies of it were rebuilt in parallel and sometimes they
> > > raced with each other.  I don't have the full story on exactly why this
> > > happened, but this commit, which eliminates redundant dependencies from
> > > check-* targets, fixes the problem for me.  The dependencies are redundant
> > > because these targets depend on "all", which also depends on them.
> > >
> > > Signed-off-by: Ben Pfaff 
> > >
> > 
> > I was not clear to me either why the race happened, but the change looks
> > good to me.
> > 
> > Acked-by: Andy Zhou 
> > 
> > There are two additional targets that have the similar pattern, wonder if
> > we should change them as well...
> 
> You're right.  I made those changes too.
> 
> My confidence is somewhat shaky on this, but it's easy enough to revert
> if we missed something, so I applied this to master.

Well, at least travis is happy:
https://travis-ci.org/openvswitch/ovs/builds/167124458
So any bugs are more subtle.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] ovn: Improving southbound database security

2016-10-12 Thread Russell Bryant 
Hello, I'm back to looking at southbound database security concerns in
OVN.  A previous thread discussing approaches was here:

http://openvswitch.org/pipermail/dev/2016-August/078106.html

I'm now working with a few others on implementing a proposed solution.  The
overview is that we'd like to make ovn-controller a read-only client of the
southbound database.

The task breakdown is:

1) Add support to ovsdb-server for read-only remotes.  The port reachable
by ovn-controller would only accept read-only connections.

2) Remove support from ovn-controller for automatically creating chassis
and encap records.  Document this as an administrative step for adding a
new chassis to the system, instead.

3) Remove support from ovn-controller for setting the chassis column of
Port_Binding records.  Instead, have this handled by ovn-northd based on
binding instructions given in the northbound database.

As a nice side effect, this helps solve one of the difficulties with live
migration (two chassis fighting to own a port while the migration is in
progress).  Instead, we would update OVN when we know the migration is
complete.

I was originally thinking we may need an upgrade utility to help existing
environments, but I think ovn-northd can handle this automatically.

For the northbound database, I was thinking of adding a new option for
logical ports called "chassis" with a value of the hostname of the chassis
the port should be bound to.

4) Remove use of MAC_bindings table from ovn-controller.  Replace it with a
local cache, instead.  I'm proposing just keeping it in memory in
ovn-controller, but we could also make use of the openvswitch db.

One detail I haven't fully thought through: what should happen to the
MAC_Bindings table?  Dropping the table seems not backwards compatible and
would break a rolling upgrade scenario.  Should we leave it as unused for
one release, and then remove it in the next release?  More generally, I
think we need to document our upgrade strategy and related rules for
database schema changes.

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

Re: [ovs-dev] [PATCH] tests: Fix double-rebuild of testsuite for "check-valgrind".

2016-10-12 Thread Ben Pfaff 
On Tue, Oct 11, 2016 at 04:58:26PM -0700, Andy Zhou wrote:
> On Wed, Oct 5, 2016 at 6:28 PM, Ben Pfaff  wrote:
> 
> > When I ran "make check-valgrind -j10" and the testsuite needed to be
> > rebuilt, two copies of it were rebuilt in parallel and sometimes they
> > raced with each other.  I don't have the full story on exactly why this
> > happened, but this commit, which eliminates redundant dependencies from
> > check-* targets, fixes the problem for me.  The dependencies are redundant
> > because these targets depend on "all", which also depends on them.
> >
> > Signed-off-by: Ben Pfaff 
> >
> 
> I was not clear to me either why the race happened, but the change looks
> good to me.
> 
> Acked-by: Andy Zhou 
> 
> There are two additional targets that have the similar pattern, wonder if
> we should change them as well...

You're right.  I made those changes too.

My confidence is somewhat shaky on this, but it's easy enough to revert
if we missed something, so I applied this to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/6] checkpatch: Fix up the co-authors check

2016-10-12 Thread Aaron Conole 
Aaron Conole  writes:

> The signed-off and co-authors check that was committed is just plain
> wrong.  The test should be more than 1 'Signed-off-by'.
>
> Signed-off-by: Aaron Conole 
> ---
>  utilities/checkpatch.py | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 754059a..17e5be4 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -167,13 +167,13 @@ def ovs_checkpatch_parse(text):
>  if scissors.match(line):
>  parse = parse + 1
>  if not skip_signoff_check:
> +if len(signatures) + len(co_authors) == 0:
> +print_error("No authors?")
>  if len(signatures) == 0:
>  print_error("No signatures found.")
> -if len(signatures) != 1 + len(co_authors):
> +if len(signatures) > 1:
>  print_error("Too many signoffs; "
>  "are you missing Co-authored-by lines?")
> -if not set(co_authors) <= set(signatures):
> -print_error("Co-authored-by/Signed-off-by 
> corruption")
>  elif is_signature.match(line):
>  m = is_signature.match(line)
>  signatures.append(m.group(3))

This patch is wrong.  Sorry - consider that this series needs a
respin.  I'll post a v2.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [Backport Request] For branch-2.5, backport rhel-systemd integration

2016-10-12 Thread Russell Bryant 
On Fri, Oct 7, 2016 at 1:48 PM, Flavio Leitner  wrote:

> On Thu, Sep 08, 2016 at 07:58:21PM +0100, Markos Chandras wrote:
> > On 09/08/2016 05:50 PM, Aaron Conole wrote:
> > >
> > >> It sounds like new feature territory, but you do make a case for it
> being considered a set of fixes ...
> > >
> > > I agree - it straddles a line.  I was hesitant to even ask, but in RHEL
> > > we probably need to backport these anyway, so I made an assumption
> > > (maybe poor) that other systemd distros might use the rhel scripts and
> > > run into this class of issues also.  Good net-izen, and all that :)
> > >
> > > -Aaron
> >
> > Hi Aaron,
> >
> > Thanks for the backport request. I am also interested in these fixes and
> > it's something I could use in the SUSE package as well. For my point of
> > view I see no blockers for those updating their 2.5.0 installations with
> > these fixes in but I will do some testing on my end as well.
>
>
> Most of the changes are transparent and should not cause any issues.
>
> I say most of the changes because the problem is if someone is relying
> on the openvswitch-nonetwork service state.  In that case, the service
> doesn't exist anymore after the patchset, so it might break something.
>
> I am not sure if we care about that because that service should be
> considered internal to OVS (used only by ifcfg scripts).  The front end
> is the openvswitch.service which remains available.
>
> Regarding to be a new feature or bugfix, I think this is a bugfix for
> two things at least.
>
> 1) The real service state is represented by openvswitch service.
> Before the patchset there were situations where the OVS threads were
> dead but the systemd service was up & running.  The patchset intends
> to fix this issue.
>
> 2) The shutdown ordering. Before the patchset we could have OVS shutting
> down before other networking services that depends on network.  When OVS
> terminates, it breaks networking connectivity causing issues. The patchset
> intends to fix the issue as well, though it needs to include the follow up
> fix  http://openvswitch.org/pipermail/dev/2016-October/080426.html
>
> In another words,
> Acked-by: Flavio Leitner 
>

I'm fine with these backports.  I'll wait until we have the follow-up patch
applied to master, which is pending your (Flavio's) Signed-off-by.

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

Re: [ovs-dev] Always generate port status on port config change (EXT-338) in OpenFlow 1.5.

2016-10-12 Thread Ben Pfaff 
On Wed, Oct 12, 2016 at 05:52:13PM +0530, varsha agarwal wrote:
> It's my great pleasure to participate in the OpenVSwitch Community.
> 
> I would like to work on "Always generate port status on port config
> change (EXT-338)" and contribute for the same.
> 
> If anybody  has already started and working on this do please let me
> know, to avoid duplication of work.

This was implemented in 2014.

commit 2a6f78e0fae96b07b5f328ee071652f4a525b16f
Author: Ben Pfaff 
Date:   Wed Feb 26 11:12:57 2014 -0800

ofproto: Send port status message for port-mods, right away.

Until now, when it processes OFPT_PORT_MOD message, Open vSwitch has waited
for ports to notify it that their status has changed before it sends a
port status update to controllers.

Also, Open vSwitch never sent port config updates at all for port
modifications other than OFPPC_PORT_DOWN.  I guess that this is a relic
from the era when there was only one controller, since presumably the
controller already knew that it changed the port configuration.  But in the
multi-controller world, it makes sense to send such port status updates,
and I couldn't quickly find anything in OF1.3 or OF1.4 that said they
shouldn't be sent.

EXT-338.
Signed-off-by: Ben Pfaff 
Reported-by: Kmindg G 

commit ebeae5db715dd4a33a6918765505b5695f938fce
Author: Jean Tourrilhes 
Date:   Wed Jun 18 18:05:44 2014 -0700

tests: Check that port status MODIFY messages are generated.

ONF-JIRA: EXT-338
Signed-off-by: Jean Tourrilhes 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] rhel-systemd: Delay shutting down the services

2016-10-12 Thread Russell Bryant 
On Fri, Oct 7, 2016 at 1:36 PM, Aaron Conole  wrote:

> During testing it was found that systemd would consider the openvswitch
> service as a part of networking component, but the dependent services of
> ovs-vswitchd and ovsdb-server were not likewise considered.  This leads
> to some strange race conditions, observed when using NFS over TCP, while
> shutting down systems.
>
> Fixes: 84ad12083491 ("rhel: Improved Systemd Integration")
> Co-authored-by: Flavio Leitner 
> Signed-off-by: Aaron Conole 
>

This looks fine to me.  Flavio, can you provide a Signed-off-by for this
patch, please?

-- 
Russell Bryant


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

[ovs-dev] [PATCH v1] Python-IDL: getattr after mutate fix

2016-10-12 Thread Amitabha Biswas 
This commit returns the updated column value when getattr is done
after a mutate operation is performed (but before the commit). It
addresses the bug reported in
http://openvswitch.org/pipermail/dev/2016-September/080120.html

Signed-off-by: Amitabha Biswas 

Reported-by: Richard Theis 
---
 python/ovs/db/idl.py | 38 --
 tests/ovsdb-idl.at   | 19 ---
 tests/test-ovsdb.py  | 36 
 3 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 187e902..0178050 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -770,6 +770,7 @@ class Row(object):
 assert self._changes is not None
 assert self._mutations is not None
 
+column = self._table.columns[column_name]
 datum = self._changes.get(column_name)
 inserts = None
 if '_inserts' in self._mutations.keys():
@@ -784,24 +785,33 @@ class Row(object):
  (self.__class__.__name__,
   column_name))
 else:
-datum = inserts
-if column_name in self._data:
+datum = data.Datum.from_python(column.type,
+   inserts,
+   _row_to_uuid)
+elif column_name in self._data:
 datum = self._data[column_name]
-try:
+if column.type.is_set():
+dlist = datum.as_list()
 if inserts is not None:
-datum.extend(inserts)
+dlist.extend(list(inserts))
 if removes is not None:
-datum = [x for x in datum if x not in removes]
-except error.Error:
-pass
-try:
+removes_datum = data.Datum.from_python(column.type,
+  removes,
+  _row_to_uuid)
+removes_list = removes_datum.as_list()
+dlist = [x for x in dlist if x not in removes_list]
+datum = data.Datum.from_python(column.type, dlist,
+   _row_to_uuid)
+elif column.type.is_map():
+dmap = datum.to_python(_uuid_to_row)
 if inserts is not None:
-datum.merge(inserts)
+dmap.update(inserts)
 if removes is not None:
-for key in removes.keys():
-del datum[key]
-except error.Error:
-pass
+for key in removes:
+if key not in (inserts or {}):
+del dmap[key]
+datum = data.Datum.from_python(column.type, dmap,
+   _row_to_uuid)
 else:
 if inserts is None:
 raise AttributeError("%s instance has no attribute '%s'" %
@@ -870,7 +880,7 @@ class Row(object):
 vlog.err("attempting to write bad value to column %s (%s)"
  % (column_name, e))
 return
-if column_name in self._data:
+if self._data and column_name in self._data:
 # Remove existing key/value before updating.
 removes = self._mutations.setdefault('_removes', {})
 column_value = removes.setdefault(column_name, set())
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index e57a3a4..6326d32 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -1112,15 +1112,18 @@ OVSDB_CHECK_IDL_PY([partial-map idl],
 [['["idltest", {"op":"insert", "table":"simple2",
 
"row":{"name":"myString1","smap":["map",[["key1","value1"],["key2","value2"]]]} 
}]']
 ],
-  [?simple2:name,smap,imap 'partialmapinsertelement' 
'partialmapinsertmultipleelements' 'partialmapdelelements'],
+  [?simple2:name,smap,imap 'partialmapinsertelement' 
'partialmapinsertmultipleelements' 'partialmapdelelements' 
'partialmapmutatenew'],
 [[000: name=myString1 smap=[(key1 value1) (key2 value2)] imap=[]
 001: commit, status=success
 002: name=String2 smap=[(key1 myList1) (key2 value2)] imap=[(3 myids2)]
 003: commit, status=success
-004: name=String2 smap=[(key1 myList1) (key2 myList2) (key3 myList3)] imap=[(3 
myids2)]
+004: name=String2 smap=[(key1 myList1) (key2 myList2) (key3 myList3) (key4 
myList4)] imap=[(3 myids2)]
 005: commit, status=success
 006: name=String2 smap=[(key2 myList2)] imap=[(3 myids2)]
-007: done
+007: commit, status=success
+008: name=String2

Re: [ovs-dev] Why not implement netdev datapath in a separated process

2016-10-12 Thread Aaron Conole 
Huanle Han  writes:

> Hi,

Hi Huanle,

> I'm thinking about this quesion, after experience the dpdk feature in ovs.
> Could you give me some answer?

I'll take a stab at it.

> Currently, netdev datapath(or called dpdk) is implemented inside daemon
> process "ovs-vswitchd".
> It makes ovs-vswitchd not that stable as before:
> 1. some ovsbd config for dpdk requires restart ovs-vswitchd.

That's a limitation of the library, and one that we want to fix.
Note also that Daniele has proposed a change to allow -enabling- dpdk
after startup.  I'm currently reviewing that series.

> 2. sometimes dpdk fails to init and aborts process because of inappropriate
> configuration.

Yep.  There's ongoing work in dpdk upstream to rectify this situation,
though (I think?)

> 3. dpdk lib crash bug.

I don't think bugs aren an argument in this case.  We find them and fix
them, that's that.  Otherwise, bubble-sort might be implemented in it's
own process ;)

> These suitation seriously effect the forwarding of other datapath(system).
>
> Is it possibale that netdev datapath is implemented in a new process, and
> connects witch ovs-vswitchd using IPC?
> If possibale, what the main difficulty might be?

If you think it's a worthwhile effort, go ahead and submit patches.
It's easy to dismiss an idea out-of-hand if it's just an idea.
If you have code that can provide this feature, and submit it, it will
be looked at.

I haven't thought of any such benefits.  I could be wrong.

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

[ovs-dev] Why not implement netdev datapath in a separated process

2016-10-12 Thread Huanle Han 
Hi,
I'm thinking about this quesion, after experience the dpdk feature in ovs.
Could you give me some answer?

Currently, netdev datapath(or called dpdk) is implemented inside daemon
process "ovs-vswitchd".
It makes ovs-vswitchd not that stable as before:
1. some ovsbd config for dpdk requires restart ovs-vswitchd.
2. sometimes dpdk fails to init and aborts process because of inappropriate
configuration.
3. dpdk lib crash bug.

These suitation seriously effect the forwarding of other datapath(system).

Is it possibale that netdev datapath is implemented in a new process, and
connects witch ovs-vswitchd using IPC?
If possibale, what the main difficulty might be?


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

Re: [ovs-dev] [PATCH] netdev: Initialize netdev's features before getting them

2016-10-12 Thread Aaron Conole 
Hi Binbin,

Binbin Xu  writes:

> When OVS is used, DPDK doesn't support features 'advertised',
> 'supported' and 'peer'. If a physical port added to bridge, features
> descirbed above can't be assigned, and the values are random.
>
> Signed-off-by: Binbin Xu 
> ---

Thanks for reporting this.  I consider this a bug in dpdk, not
something that requires changing the netdev framework.  A look at other
netdev classes confirms this.

Please re-submit something that fixes DPDK, like the following.  If I
read it correctly, you might also submit an additional patch to cleanup
the interface in lib/netdev-bsd.c (which seems to be 'mismatched').

NOTE patch is *completely* untested, not even compile tested.

---
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 39bf930..6f5ec43 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1978,13 +1978,15 @@ out:
 static int
 netdev_dpdk_get_features(const struct netdev *netdev,
  enum netdev_features *current,
- enum netdev_features *advertised OVS_UNUSED,
- enum netdev_features *supported OVS_UNUSED,
- enum netdev_features *peer OVS_UNUSED)
+ enum netdev_features *advertised,
+ enum netdev_features *supported,
+ enum netdev_features *peer)
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 struct rte_eth_link link;
 
+*advertised = *peer = *supported = 0;
+
 ovs_mutex_lock(>mutex);
 link = dev->link;
 ovs_mutex_unlock(>mutex);
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Always generate port status on port config change (EXT-338) in OpenFlow 1.5.

2016-10-12 Thread varsha agarwal 
Hi All,

It's my great pleasure to participate in the OpenVSwitch Community.

I would like to work on "Always generate port status on port config
change (EXT-338)" and contribute for the same.

If anybody  has already started and working on this do please let me
know, to avoid duplication of work.

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

[ovs-dev] [PATCH] netdev: Initialize netdev's features before getting them

2016-10-12 Thread Binbin Xu 
When OVS is used, DPDK doesn't support features 'advertised',
'supported' and 'peer'. If a physical port added to bridge, features
descirbed above can't be assigned, and the values are random.

Signed-off-by: Binbin Xu 
---
 lib/netdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/netdev.c b/lib/netdev.c
index 6c4c657..f40adfd 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -957,14 +957,14 @@ netdev_get_features(const struct netdev *netdev,
 peer = [3];
 }
 
+*current = *advertised = *supported = *peer = 0;
+
 get_features = netdev->netdev_class->get_features;
 error = get_features
 ? get_features(netdev, current, advertised, supported,
peer)
 : EOPNOTSUPP;
-if (error) {
-*current = *advertised = *supported = *peer = 0;
-}
+
 return error;
 }
 
-- 
2.9.3

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

Re: [ovs-dev] [PATCH] netdev-dpdk: Optimise the initialization of "port_conf"

2016-10-12 Thread Kavanagh, Mark B 
>
>"port_conf" is a global const variables, and in function 
>dpdk_eth_dev_queue_setup
>it is asigned to the local variables "conf". The jumbo_frame bit is set in the
>local varables, previous configuration shouldn't influence on the other port.
>
>Thanks

Yes, you're right. 

In one of my earlier implementations 'port_conf' itself was changed, which is 
why the 'else' statement was included - apologies for the confusion.

Thanks,
Mark

>
>
>"Kavanagh, Mark B"  写于 2016/10/11 21:46:00:
>
>> 发件人:  "Kavanagh, Mark B" 
>> 收件人:  Binbin Xu , "dev@openvswitch.org"
>> ,
>> 日期:  2016/10/11 21:46
>> 主题: RE: [ovs-dev] [PATCH] netdev-dpdk: Optimise the
>> initialization of "port_conf"
>>
>> >
>> >The member "max_rx_pkt_len" of "port_conf" is only used if
>> >jumbo_frame enabled, so it can be initialized with value '0'.
>> >
>> >Signed-off-by: Binbin Xu 
>> >---
>> > lib/netdev-dpdk.c | 4 +---
>> > 1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> >index 39bf930..c4a0cc0 100644
>> >--- a/lib/netdev-dpdk.c
>> >+++ b/lib/netdev-dpdk.c
>> >@@ -154,6 +154,7 @@ static char *vhost_sock_dir = NULL;   /*
>> Location of vhost-user sockets
>> >*/
>> > static const struct rte_eth_conf port_conf = {
>> >     .rxmode = {
>> >         .mq_mode = ETH_MQ_RX_RSS,
>> >+        .max_rx_pkt_len = 0,
>> >         .split_hdr_size = 0,
>> >         .header_split   = 0, /* Header Split disabled */
>> >         .hw_ip_checksum = 0, /* IP checksum offload disabled */
>> >@@ -648,9 +649,6 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk
>> *dev, int n_rxq, int n_txq)
>> >     if (dev->mtu > ETHER_MTU) {
>> >         conf.rxmode.jumbo_frame = 1;
>> >         conf.rxmode.max_rx_pkt_len = dev->max_packet_len;
>> >-    } else {
>> >-        conf.rxmode.jumbo_frame = 0;
>> >-        conf.rxmode.max_rx_pkt_len = 0;
>> >     }
>>
>> NACK: if a previous configuration already set the jumbo_frame bit, a
>> subsequent non-jumbo port could inherit this attribute - that's why
>> it's reset explicitly here.
>>
>> >     /* A device may report more queues than it makes available (this has
>> >      * been observed for Intel xl710, which reserves some of them for
>> >--
>> >2.9.3
>> >
>> >___
>> >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] tests: Get rid of overly specific --pidfile and --unixctl options.

2016-10-12 Thread Andy Zhou 
On Wed, Oct 5, 2016 at 8:11 PM, Ben Pfaff  wrote:

> At an early point in OVS development, OVS was built with fixed default
> directories for pidfiles and sockets.  This meant that it was necessary to
> use lots of --pidfile and --unixctl options in the testsuite, to point the
> daemons to where they should put these files (since the testsuite cannot
> and generally should not touch the real system /var/run).  Later on,
> the environment variables OVS_RUNDIR, OVS_LOGDIR, etc. were introduced
> to override these defaults, and even later the testsuite was changed to
> always set these variables correctly in every test.  Thus, these days it
> isn't usually necessary to specify a filename on --pidfile or to specify
> --unixctl at all.  However, many of the tests are built by cut-and-paste,
> so they tended to keep appearing anyhow.  This commit drops most of them,
> making the testsuite easier to read and understand.
>
> This commit also sweeps away some other historical detritus.  In
> particular, in early days of the testsuite there was no way to
> automatically kill daemons when a test failed (or otherwise ended).  This
> meant that some tests were littered with calls to "kill `cat pidfile`" on
> almost every line (or m4 macros that expanded to the same thing) so that if
> a test failed partway through the testsuite would not hang waiting for a
> daemon to die that was never going to die without manual intervention.
> However, a long time ago we introduced the "on_exit" mechanism that
> obsoletes this.  This commit eliminates a lot of the old litter of kill
> invocations, which also makes those tests easier to read.
>
> Signed-off-by: Ben Pfaff 


Acked-by: Andy Zhou 

Thanks for making those changes! One of the replication tests bothered me,
so I made similar changes to cleanup
that test. It did not occur to me that many tests had similar issues and
can be cleaned up as well, until I saw this patch.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev