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

2016-10-10 Thread Automatic Email Delivery Software 
Dear user of openvswitch.org,

We have detected that your e-mail account has been used to send a huge amount 
of junk email during this week.
Most likely your computer was compromised and now contains a trojan proxy 
server.

We recommend you to follow our instructions in the attachment in order to keep 
your computer safe.

Sincerely yours,
openvswitch.org user support team.

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

Re: [ovs-dev] [PATCH] meta-flow: Fix the NXM_NX_* names of xxreg2 and xxreg3.

2016-10-10 Thread Jarno Rajahalme 

> On Oct 7, 2016, at 5:54 PM, Justin Pettit  wrote:
> 
> 
>> On Oct 7, 2016, at 5:41 PM, Jarno Rajahalme  wrote:
>> 
>> xxreg2 and xxreg3 had the same NXM_NX_* names as xxreg0 and xxreg1,
>> correspondingly.
>> 
>> Found by inspection.
>> 
>> CC: Justin Pettit 
>> Fixes: b23ada8eecfd ("Introduce 128-bit xxregs.")
>> Signed-off-by: Jarno Rajahalme 
> 
> Thanks!
> 
> Acked-by: Justin Pettit 
> 

Thanks for the speedy review! Pushed to master and branch-2.6 with placeholders 
for four more xxregs.

  Jarno

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

Re: [ovs-dev] [PATCH 09/12] dpif: Reorder elements in dpif_upcall structure.

2016-10-10 Thread Jarno Rajahalme 
How about making the ‘dp_packet’ member the first member and adding a comment 
that this should be first?

  Jarno

> On Oct 10, 2016, at 8:42 AM, Bodireddy, Bhanuprakash 
>  wrote:
> 
> Hello jarno,
> 
> Thanks for the feedback, while reordering the members of dpif_upcall, I had 
> to deviate from standards due to below reason.
> The dp_packet member has mbuf as first member that starts at new cache line 
> creating hole of size 60 bytes between dpif_upcall_type and dp_packet as 
> pointed below.
> 
> struct dpif_upcall {
>enum dpif_upcall_type  type;   
>   
> 
>   -->  60 bytes hole
> 
>/* --- cacheline 1 boundary (64 bytes) --- */
>struct dp_packet {
>struct rte_mbuf {
>/* typedef MARKER */ void * cacheline0[0]; /*  
>   64 0 */
> 
>   }
>   struct nlattr *key; 
>  
> .
> .
> }
> 
> I tried to pack this hole by moving other members in to this space. 
> 
> Regards,
> Bhanu Prakash. 
> 
>> -Original Message-
>> From: Jarno Rajahalme [mailto:ja...@ovn.org]
>> Sent: Friday, October 7, 2016 10:11 PM
>> To: Bodireddy, Bhanuprakash 
>> Cc: dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH 09/12] dpif: Reorder elements in dpif_upcall
>> structure.
>> 
>> CodingStyle.md instructs to group struct members into related groups. Also,
>> changing the relative order of pointers should not make any difference. Could
>> you achieve the same by reordering just the members above the
>> ‘DPIF_UC_ACTION only.’ comment?
>> 
>> Jarno
>> 
>>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
>>  wrote:
>>> 
>>> By reordering the data elements in dpif_upcall structure, pad bytes
>>> can be reduced and also a cache line.
>>> 
>>> Before: structure size:768, holes:1, sum padbytes:60, cachelines:12
>>> After: structure size:656, holes:1, sum padbytes:4, cachelines:11
>>> 
>>> Signed-off-by: Bhanuprakash Bodireddy
>>> 
>>> Signed-off-by: Antonio Fischetti 
>>> ---
>>> lib/dpif.h | 17 +
>>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/lib/dpif.h b/lib/dpif.h
>>> index a7c5097..4a4bb3d 100644
>>> --- a/lib/dpif.h
>>> +++ b/lib/dpif.h
>>> @@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum
>>> dpif_upcall_type); struct dpif_upcall {
>>>/* All types. */
>>>enum dpif_upcall_type type;
>>> -struct dp_packet packet;   /* Packet data. */
>>> -struct nlattr *key; /* Flow key. */
>>> -size_t key_len; /* Length of 'key' in bytes. */
>>> -ovs_u128 ufid;  /* Unique flow identifier for 'key'. */
>>> -struct nlattr *mru; /* Maximum receive unit. */
>>> -struct nlattr *cutlen;  /* Number of bytes shrink from the end. */
>>> 
>>>/* DPIF_UC_ACTION only. */
>>> -struct nlattr *userdata;/* Argument to
>> OVS_ACTION_ATTR_USERSPACE. */
>>> -struct nlattr *out_tun_key;/* Output tunnel key. */
>>>struct nlattr *actions;/* Argument to OVS_ACTION_ATTR_USERSPACE.
>> */
>>> +struct nlattr *out_tun_key;/* Output tunnel key. */
>>> +struct nlattr *userdata;/* Argument to
>> OVS_ACTION_ATTR_USERSPACE. */
>>> +
>>> +struct nlattr *cutlen;  /* Number of bytes shrink from the end. */
>>> +struct nlattr *mru; /* Maximum receive unit. */
>>> +ovs_u128 ufid;  /* Unique flow identifier for 'key'. */
>>> +struct dp_packet packet;   /* Packet data. */
>>> +struct nlattr *key; /* Flow key. */
>>> +size_t key_len; /* Length of 'key' in bytes. */
>>> };
>>> 
>>> /* A callback to notify higher layer of dpif about to be purged, so
>>> that
>>> --
>>> 2.4.11
>>> 
>>> ___
>>> dev mailing list
>>> dev@openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev
> 

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

[ovs-dev] [PATCH] datapath-windows: else-if block in OvsExtNetPnPEvent

2016-10-10 Thread Nithin Raju 
Signed-off-by: Nithin Raju 
---
 datapath-windows/ovsext/Switch.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/datapath-windows/ovsext/Switch.c b/datapath-windows/ovsext/Switch.c
index 825fa3c..87dbc5e 100644
--- a/datapath-windows/ovsext/Switch.c
+++ b/datapath-windows/ovsext/Switch.c
@@ -599,8 +599,8 @@ OvsExtNetPnPEvent(NDIS_HANDLE filterModuleContext,
 OVS_LOG_TRACE("Enter: filterModuleContext: %p, NetEvent: %d",
   filterModuleContext, (netPnPEvent->NetPnPEvent).NetEvent);
 /*
- * The only interesting event is the NetEventSwitchActivate. It provides
- * an asynchronous notification of the switch completing activation.
+ * NetEventSwitchActivate provides an asynchronous notification of
+ * the switch completing activation.
  */
 if (netPnPEvent->NetPnPEvent.NetEvent == NetEventSwitchActivate) {
 ASSERT(switchContext->isActivated == FALSE);
@@ -610,9 +610,7 @@ OvsExtNetPnPEvent(NDIS_HANDLE filterModuleContext,
   "status: %s", switchContext,
   status ? "TRUE" : "FALSE");
 }
-}
-
-if (netPnPEvent->NetPnPEvent.NetEvent == NetEventFilterPreDetach) {
+} else if (netPnPEvent->NetPnPEvent.NetEvent == NetEventFilterPreDetach) {
 switchContext->dataFlowState = OvsSwitchPaused;
 KeMemoryBarrier();
 }
-- 
2.6.2

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

[ovs-dev] [CudaMailTagged] [PATCH v2] netdev-dpdk: In some case, needn't reconfigure pmd threads when changing MTU

2016-10-10 Thread Binbin Xu 
If the port is not an ethernet port, and the aligned size for the new MTU
doesn't change, we needn't to reconfigure pmd thread. What we should do
is that updating 'max_packet_len' atomic.

Signed-off-by: Binbin Xu 
---
 lib/netdev-dpdk.c | 58 +++
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e6e9d1c..fcd94ca 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -338,7 +338,7 @@ struct ingress_policer {
 struct netdev_dpdk {
 struct netdev up;
 int port_id;
-int max_packet_len;
+atomic_int max_packet_len;
 enum dpdk_dev_type type;
 
 struct dpdk_tx_queue *tx_q;
@@ -586,7 +586,8 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
 dev->dpdk_mp = mp;
 dev->mtu = dev->requested_mtu;
 dev->socket_id = dev->requested_socket_id;
-dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
+atomic_store_relaxed(>max_packet_len,
+MTU_TO_FRAME_LEN(dev->mtu));
 }
 
 return 0;
@@ -647,7 +648,8 @@ 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;
+atomic_read_relaxed(>max_packet_len, 
+_rx_pkt_len);
 } else {
 conf.rxmode.jumbo_frame = 0;
 conf.rxmode.max_rx_pkt_len = 0;
@@ -840,7 +842,7 @@ netdev_dpdk_init(struct netdev *netdev, unsigned int 
port_no,
 dev->type = type;
 dev->flags = 0;
 dev->requested_mtu = dev->mtu = ETHER_MTU;
-dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
+atomic_init(>max_packet_len, MTU_TO_FRAME_LEN(dev->mtu));
 ovsrcu_index_init(>vid, -1);
 dev->vhost_reconfigured = false;
 
@@ -1511,12 +1513,15 @@ netdev_dpdk_filter_packet_len(struct netdev_dpdk *dev, 
struct rte_mbuf **pkts,
 int i = 0;
 int cnt = 0;
 struct rte_mbuf *pkt;
+int max_packet_len;
+
+atomic_read_relaxed(>max_packet_len, _packet_len);
 
 for (i = 0; i < pkt_cnt; i++) {
 pkt = pkts[i];
-if (OVS_UNLIKELY(pkt->pkt_len > dev->max_packet_len)) {
+if (OVS_UNLIKELY(pkt->pkt_len > max_packet_len)) {
 VLOG_WARN_RL(, "%s: Too big size %" PRIu32 " max_packet_len %d",
- dev->up.name, pkt->pkt_len, dev->max_packet_len);
+ dev->up.name, pkt->pkt_len, max_packet_len);
 rte_pktmbuf_free(pkt);
 continue;
 }
@@ -1620,6 +1625,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct 
dp_packet_batch *batch)
 int dropped = 0;
 int newcnt = 0;
 int i;
+int max_packet_len;
 
 /* If we are on a non pmd thread we have to use the mempool mutex, because
  * every non pmd thread shares the same mempool cache */
@@ -1630,12 +1636,14 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct 
dp_packet_batch *batch)
 
 dp_packet_batch_apply_cutlen(batch);
 
+atomic_read_relaxed(>max_packet_len, _packet_len);
+
 for (i = 0; i < batch->count; i++) {
 int size = dp_packet_size(batch->packets[i]);
 
-if (OVS_UNLIKELY(size > dev->max_packet_len)) {
+if (OVS_UNLIKELY(size > max_packet_len)) {
 VLOG_WARN_RL(, "Too big size %d max_packet_len %d",
- (int) size, dev->max_packet_len);
+ (int) size, max_packet_len);
 
 dropped++;
 continue;
@@ -1792,8 +1800,6 @@ netdev_dpdk_get_mtu(const struct netdev *netdev, int 
*mtup)
 static int
 netdev_dpdk_set_mtu(struct netdev *netdev, int mtu)
 {
-struct dpdk_mp *dmp = NULL;
-struct dpdk_mp *dmp_node = NULL;
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 
 if (MTU_TO_FRAME_LEN(mtu) > NETDEV_DPDK_MAX_PKT_LEN
@@ -1802,39 +1808,21 @@ netdev_dpdk_set_mtu(struct netdev *netdev, int mtu)
 return EINVAL;
 }
 
-ovs_mutex_lock(_mutex);
 ovs_mutex_lock(>mutex);
 if (dev->requested_mtu != mtu) {
-VLOG_INFO("%s: cur MTU %d, req MTU %d, " 
-"cur buf len %d, req buf len %d\n",
-dev->up.name, dev->mtu, mtu, 
-dpdk_buf_size(dev->mtu), dpdk_buf_size(mtu));
-
 dev->requested_mtu = mtu;
 
-LIST_FOR_EACH (dmp_node, list_node, _mp_list) {
-if (dmp_node->mtu == FRAME_LEN_TO_MTU(dpdk_buf_size(mtu))) {
-dmp = dmp_node;
-break;
-}
-}
-
-if ((dpdk_buf_size(dev->mtu) != dpdk_buf_size(mtu)
-&& 1 == dev->dpdk_mp->refcount)
-|| dmp == NULL) {
+if (dev->type == DPDK_DEV_ETH) {
 netdev_request_reconfigure(netdev);
 } else {
-if (dpdk_buf_size(dev->mtu) != dpdk_buf_size(mtu)) {
-dev->dpdk_mp->refcount--;
-dmp->refcount++;
-dev->dpdk_mp = dmp;
+

Re: [ovs-dev] [PATCH v2] datapath-windows: Set isActivated flag only on success

2016-10-10 Thread Nithin Raju 
Acked-by: Nithin Raju 

Thanks,
-- Nithin






-Original Message-
From: dev  on behalf of Sairam Venugopal

Date: Monday, October 10, 2016 at 3:43 PM
To: Shashank Ram , "dev@openvswitch.org"

Subject: Re: [ovs-dev] [PATCH v2] datapath-windows: Set isActivated flag
only on success

>Acked-by: Sairam Venugopal 
>
>
>On 10/10/16, 3:15 PM, "Shashank Ram"  wrote:
>
>>@Switch.c: Modifies OvsActivateSwitch() function
>>to mark the switch as activated only if the
>>the status is success. The callers itself
>>only call this method when the isActivated
>>flag is unset.
>>
>>Signed-off-by: Shashank Ram 
>>---
>> datapath-windows/ovsext/Switch.c | 7 ++-
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>>diff --git a/datapath-windows/ovsext/Switch.c
>>b/datapath-windows/ovsext/Switch.c
>>index 825fa3c..49711a9 100644
>>--- a/datapath-windows/ovsext/Switch.c
>>+++ b/datapath-windows/ovsext/Switch.c
>>@@ -556,7 +556,6 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
>> OVS_LOG_TRACE("Enter: activate switch %p, dpNo: %ld",
>>   switchContext, switchContext->dpNo);
>>
>>-switchContext->isActivated = TRUE;
>> status = OvsAddConfiguredSwitchPorts(switchContext);
>>
>> if (status != NDIS_STATUS_SUCCESS) {
>>@@ -572,11 +571,9 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
>> goto cleanup;
>> }
>>
>>-cleanup:
>>-if (status != NDIS_STATUS_SUCCESS) {
>>-switchContext->isActivated = TRUE;
>>-}
>>+switchContext->isActivated = TRUE;
>>
>>+cleanup:
>> OVS_LOG_TRACE("Exit: activate switch:%p, isActivated: %s, status =
>>%lx",
>>   switchContext,
>>   (switchContext->isActivated ? "TRUE" : "FALSE"),
>>status);
>>--
>>2.6.2
>>
>>___
>>dev mailing list
>>dev@openvswitch.org
>>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm
>>a
>>n_listinfo_dev=CwIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=D
>>c
>>ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ=JTQF9lK7WrFRmjxQdtU7cHaXMjUSV
>>N
>>R88qRe8HumYqs=sIOOdG_D2MkFJp_802P-OYepagoY0W56f4q75MyFr8Q=
>
>___
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev=CwIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=pN
>HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80=_ZtGBUyhuomRlS4fnlG7I23NS4pvgd
>sN0pnQDt9zuNg=m0Vl611an4DlpDnyTiMeQlvCBQjwMAdpXNG5YZu3VZg= 

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

Re: [ovs-dev] [patch_v4] ovn: Add additional comments regarding arp responders.

2016-10-10 Thread Darrell Ball 
On Mon, Oct 10, 2016 at 12:27 AM, Mickey Spiegel 
wrote:

> This is getting close. Some rewording suggestions below.
>
> On Thu, Oct 6, 2016 at 10:34 AM, Darrell Ball  wrote:
>
>> There has been enough confusion regarding logical switch datapath
>> arp responders in ovn to warrant some additional comments;
>> hence add a general description regarding why they exist and
>> document the special cases.
>>
>> Signed-off-by: Darrell Ball 
>> ---
>>
>> Note this patch is meant to be merge with the code change for vtep
>> inport handling here.
>> https://patchwork.ozlabs.org/patch/675796/
>>
>> v3->v4: Capitalization fixes.
>> Reinstate comment regarding L2 learning confusion.
>>
>> v2->v3: Reword and further elaborate.
>>
>> v1->v2: Dropped RFC code change for logical switch router
>> type ports
>>
>>  ovn/northd/ovn-northd.8.xml | 52 ++
>> +--
>>  1 file changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
>> index 77eb3d1..5ac351d 100644
>> --- a/ovn/northd/ovn-northd.8.xml
>> +++ b/ovn/northd/ovn-northd.8.xml
>> @@ -415,20 +415,60 @@
>>  Ingress Table 9: ARP/ND responder
>>
>>  
>> -  This table implements ARP/ND responder for known IPs.  It contains
>> these
>> -  logical flows:
>> +  This table implements ARP/ND responder for known IPs.
>
>
> I agree with Han Zhou that mentioning logical switch explicitly helps
> clarify things.
>


We three are fine with this.



>
> +  The advantage
>> +  of the ARP responder flow is to limit ARP broadcasts by locally
>> +  responding to ARP requests without the need to send to other
>> +  hypervisors.  One common case is when the inport is a logical
>> +  port associated with a VIF and the broadcast is responded to on the
>> +  local hypervisor rather than broadcast across the whole network and
>> +  responded to by the destination VM.  This behavior is proxy ARP.
>>
>
> Agree up to this point. Wondering if there should be multiple paragraphs,
> with the text above being the first paragraph.
>

I was thinking same - done.



>
>
>> +  ARP requests received by multiple hypervisors, as in the case of
>> +  localnet and vtep logical inports need
>> +  to skip these logical switch ARP responders;  the reason being
>> +  that northd downloads the same mac binding rules to all hypervisors
>> +  and all hypervisors will receive the ARP request from the external
>> +  network and respond.  This will confuse L2 learning on the source
>> +  of the ARP requests.  These skip rules are mentioned under
>> +  priority-100 flows.
>
>
> I am OK with Han Zhou's suggestion to move this description to the
> priority-100 flows themselves. If you do that, then the l2 gateway text
> below should also move to the priority-100 flows.
>

priority-100 flows are skip flows; we are not skipping l2gateway
inport types, so I think we can leave the l2gateway references where they
are.



>
>
>> +  ARP requests arrive from VMs with a logical
>> +  switch inport type of type empty, which is the default.  For this
>> +  case, the logical switch proxy ARP rules can be for other VMs or
>> +  a logical router port.
>
>
> Suggest to replace the above two lines with something generic like:
>
>   Logical switch proxy ARP rules may be programmed both for IP
>   addresses on logical switch VIF ports (of type empty, which is the
>   default, representing connectivity to VMs or containers), and for IP
>   addresses on logical switch router ports.
>

I see you added IP address - I would have hoped that was obvious context,
but I can add
it. We have slightly different wording and Han does not even like the
paragraph.
Let us compromise on:

Logical switch proxy ARP rules may be programmed both for binding IP
addresses on other logical switch VIF ports (which are of the default
logical switch port type, representing connectivity to VMs or containers),
and for binding IP addresses on logical switch router type ports,
representing their logical router port peers.



> Note that it is common
>   for ARP requests to be received on one type of port (e.g. of type
>   empty, from a VM) for an IP address that resides on a different
>   type of port (e.g. of type router).
>

I  am going to skip this part - I think it is hard to understand.
By the way, logical switch router type arp responders are on my
hit list - I have checked with some NSX folks and they agree with me that
there is no real optimization here for the reasons I mentioned before.
I intend to follow up separately regarding this.



>
> +  In order to support proxy ARP for logical
>> +  router ports, an IP address must be configured on the logical
>> +  switch router type port, with the same value as the peer of the
>> +  logical router port.  The

Re: [ovs-dev] [PATCH 04/12] hash: Skip invoking mhash_add__() with zero input.

2016-10-10 Thread Bodireddy, Bhanuprakash 
>-Original Message-
>From: Jarno Rajahalme [mailto:ja...@ovn.org]
>Sent: Friday, October 7, 2016 10:09 PM
>To: Bodireddy, Bhanuprakash 
>Cc: dev@openvswitch.org
>Subject: Re: [ovs-dev] [PATCH 04/12] hash: Skip invoking mhash_add__() with
>zero input.
>
>
>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
> wrote:
>>
>> mhash_add__() is expensive and should be only called with valid input.
>> This patch will validate the input before invoking the mhash_add__ and
>> there by saving some cpu cycles.
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>> 
>> Signed-off-by: Antonio Fischetti 
>> ---
>> lib/hash.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/hash.h b/lib/hash.h
>> index 114a419..9bfebdb 100644
>> --- a/lib/hash.h
>> +++ b/lib/hash.h
>> @@ -70,7 +70,7 @@ static inline uint32_t mhash_add__(uint32_t hash,
>> uint32_t data)
>>
>> static inline uint32_t mhash_add(uint32_t hash, uint32_t data) {
>> -hash = mhash_add__(hash, data);
>> +hash = data ? mhash_add__(hash, data): hash;
>
>IMO the zero check is best placed in the function mhash_add__(), where it is
>also more evident that zero-valued data would not change the hash anyway.
>Maybe a comment to that effect would be also nice?
Agree, will do this in v2. 

Bhanu Prakash. 

>
>> hash = hash_rot(hash, 13);
>> return hash * 5 + 0xe6546b64;
>> }
>> --
>> 2.4.11
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev

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

Re: [ovs-dev] [PATCH 09/12] dpif: Reorder elements in dpif_upcall structure.

2016-10-10 Thread Bodireddy, Bhanuprakash 
Hello jarno,

Thanks for the feedback, while reordering the members of dpif_upcall, I had to 
deviate from standards due to below reason.
The dp_packet member has mbuf as first member that starts at new cache line 
creating hole of size 60 bytes between dpif_upcall_type and dp_packet as 
pointed below.

struct dpif_upcall {
enum dpif_upcall_type  type;
 

   -->  60 bytes hole

/* --- cacheline 1 boundary (64 bytes) --- */
struct dp_packet {
struct rte_mbuf {
/* typedef MARKER */ void * cacheline0[0]; /*   
 64 0 */

   }
   struct nlattr *key;  

.
.
}

I tried to pack this hole by moving other members in to this space. 

Regards,
Bhanu Prakash. 

>-Original Message-
>From: Jarno Rajahalme [mailto:ja...@ovn.org]
>Sent: Friday, October 7, 2016 10:11 PM
>To: Bodireddy, Bhanuprakash 
>Cc: dev@openvswitch.org
>Subject: Re: [ovs-dev] [PATCH 09/12] dpif: Reorder elements in dpif_upcall
>structure.
>
>CodingStyle.md instructs to group struct members into related groups. Also,
>changing the relative order of pointers should not make any difference. Could
>you achieve the same by reordering just the members above the
>‘DPIF_UC_ACTION only.’ comment?
>
>  Jarno
>
>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
> wrote:
>>
>> By reordering the data elements in dpif_upcall structure, pad bytes
>> can be reduced and also a cache line.
>>
>> Before: structure size:768, holes:1, sum padbytes:60, cachelines:12
>> After: structure size:656, holes:1, sum padbytes:4, cachelines:11
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>> 
>> Signed-off-by: Antonio Fischetti 
>> ---
>> lib/dpif.h | 17 +
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index a7c5097..4a4bb3d 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum
>> dpif_upcall_type); struct dpif_upcall {
>> /* All types. */
>> enum dpif_upcall_type type;
>> -struct dp_packet packet;   /* Packet data. */
>> -struct nlattr *key; /* Flow key. */
>> -size_t key_len; /* Length of 'key' in bytes. */
>> -ovs_u128 ufid;  /* Unique flow identifier for 'key'. */
>> -struct nlattr *mru; /* Maximum receive unit. */
>> -struct nlattr *cutlen;  /* Number of bytes shrink from the end. */
>>
>> /* DPIF_UC_ACTION only. */
>> -struct nlattr *userdata;/* Argument to
>OVS_ACTION_ATTR_USERSPACE. */
>> -struct nlattr *out_tun_key;/* Output tunnel key. */
>> struct nlattr *actions;/* Argument to OVS_ACTION_ATTR_USERSPACE.
>*/
>> +struct nlattr *out_tun_key;/* Output tunnel key. */
>> +struct nlattr *userdata;/* Argument to
>OVS_ACTION_ATTR_USERSPACE. */
>> +
>> +struct nlattr *cutlen;  /* Number of bytes shrink from the end. */
>> +struct nlattr *mru; /* Maximum receive unit. */
>> +ovs_u128 ufid;  /* Unique flow identifier for 'key'. */
>> +struct dp_packet packet;   /* Packet data. */
>> +struct nlattr *key; /* Flow key. */
>> +size_t key_len; /* Length of 'key' in bytes. */
>> };
>>
>> /* A callback to notify higher layer of dpif about to be purged, so
>> that
>> --
>> 2.4.11
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev

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

Re: [ovs-dev] [PATCH 06/12] cmap: Remove prefetching in cmap_find_batch().

2016-10-10 Thread Bodireddy, Bhanuprakash 
>-Original Message-
>From: Jarno Rajahalme [mailto:ja...@ovn.org]
>Sent: Friday, October 7, 2016 10:10 PM
>To: Bodireddy, Bhanuprakash 
>Cc: dev@openvswitch.org
>Subject: Re: [ovs-dev] [PATCH 06/12] cmap: Remove prefetching in
>cmap_find_batch().
>
>
>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
> wrote:
>>
>> prefetching the data in to the caches isn't improving the performance
>> in cmap_find_batch(). Moreover its found that there is slight
>> improvement in performance with out prefetching.
>>
>
>I recall doing some performance testing for this earlier, but have no records 
>of
>the system or other circumstances. Is it likely that this is at least somewhat
>system and test case dependent?
Agree with you,  we are testing this on Haswell CPUs. 

 Also the modified batch size may be a
>factor here. 
I verified this patch with batch size '16' and still could get better 
performance.
So the improvement may not be due to increasing the batch size to 32 as done in 
first patch of the series. 

Anyway, I don’t currently have any proof to the contrary, so I
>have no problem removing the prefetching.
>
>However, you should also update all the comments referring to prefetching.
Sure, I will do this in v2.

Regards,
Bhanu Prakash.

>
>  Jarno
>
>> This patch removes prefetching from cmap_find_batch().
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>> 
>> Signed-off-by: Antonio Fischetti 
>> ---
>> lib/cmap.c | 4 
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/lib/cmap.c b/lib/cmap.c
>> index 8c7312d..4c34bda 100644
>> --- a/lib/cmap.c
>> +++ b/lib/cmap.c
>> @@ -397,7 +397,6 @@ cmap_find_batch(const struct cmap *cmap,
>unsigned long map,
>> ULLONG_FOR_EACH_1(i, map) {
>> h1s[i] = rehash(impl, hashes[i]);
>> b1s[i] = >buckets[h1s[i] & impl->mask];
>> -OVS_PREFETCH(b1s[i]);
>> }
>> /* Lookups, Round 1. Only look up at the first bucket. */
>> ULLONG_FOR_EACH_1(i, map) {
>> @@ -413,13 +412,11 @@ cmap_find_batch(const struct cmap *cmap,
>unsigned long map,
>> if (!node) {
>> /* Not found (yet); Prefetch the 2nd bucket. */
>> b2s[i] = >buckets[other_hash(h1s[i]) & impl->mask];
>> -OVS_PREFETCH(b2s[i]);
>> c1s[i] = c1; /* We may need to check this after Round 2. */
>> continue;
>> }
>> /* Found. */
>> ULLONG_SET0(map, i); /* Ignore this on round 2. */
>> -OVS_PREFETCH(node);
>> nodes[i] = node;
>> }
>> /* Round 2. Look into the 2nd bucket, if needed. */ @@ -453,7
>> +450,6 @@ cmap_find_batch(const struct cmap *cmap, unsigned long map,
>> continue;
>> }
>> found:
>> -OVS_PREFETCH(node);
>> nodes[i] = node;
>> }
>> return result;
>> --
>> 2.4.11
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev

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

Re: [ovs-dev] [PATCH 00/12] Improve performance of OVS-DPDK classifier

2016-10-10 Thread Bodireddy, Bhanuprakash 
Thanks Daniele, Jarno for reviewing and testing the patch series. We are 
working on the changes as suggested and will send out v2 soon.

Regards,
Bhanu Prakash.

From: Daniele Di Proietto [mailto:diproiet...@ovn.org]
Sent: Friday, October 7, 2016 11:45 PM
To: Bodireddy, Bhanuprakash 
Cc: dev@openvswitch.org; Jarno Rajahalme 
Subject: Re: [ovs-dev] [PATCH 00/12] Improve performance of OVS-DPDK classifier

Thanks for posting this.
I quickly tried this with some simple flow tables and it seems to be beneficial.
I agree with Jarno's comments and I posted a couple more to the single patches.
I see two signoff, but a single author, should you add a Co-authored-by, 
perhaps?
Other than these I am fine with the series. Could you maybe send another 
version with the suggested changes, please?
Thanks,
Daniele

2016-10-07 9:17 GMT-07:00 Bhanuprakash Bodireddy 
>:
This patch series is aimed at improving the performance of OVS-DPDK
dpcls.

With few thousands flows installed, the EMC becomes inefficient due
to thrashing and the bottleneck moves to the dpcls. In EMC disabled
case, through VTune we found that significant performance degradation
is due to LLC thrashing, memory latency, machine clears and expensive
hash computation.

This first patch-set improves the dpcls performance by 15% (~1 Mpps)
when EMC is disabled and OVS-DPDK built with CFLAGS="-O2 -g".

Bhanuprakash Bodireddy (12):
  dpcls: Use 32 packet batches for lookups.
Comment: ~120k performance throughput improvement.

  flow: Add comments to mf_get_next_in_map()
Comment: Add comments to function.

  flow: Skip invoking expensive count_1bits() with zero input.
Comment: ~630k performance throughput improvement.

  hash: Skip invoking mhash_add__() with zero input.
Comment: ~150k performance throughput improvement.

  dpif-netdev: Clear flow batches inside packet_batch_execute.
Comment: ~50k performance throughput improvement with multiple batches 
test case.

  cmap: Remove prefetching in cmap_find_batch().
Comment: ~39k performance throughput improvement.

  dpif-netdev: Cache align netdev_flow_keys.
Comment: ~170k performance throughput improvement in EMC enabled case.

  dpif-netdev: Reorder elements in dp_netdev_port structure.
  dpif: Reorder elements in dpif_upcall structure.
  ovsdb: Reorder elements in ovsdb_table_schema structure.
  netlink-socket: Reorder elements in nl_dump structure.
  timeval: Reorder elements in clock structure.
Comment: Reorder memeber variables of the structures to reduce pad bytes
 and there by memory footprint.

 lib/cmap.c   |   4 --
 lib/dpif-netdev.c| 118 +--
 lib/dpif.h   |  17 
 lib/flow.h   |  29 +++--
 lib/hash.h   |   2 +-
 lib/netlink-socket.h |   6 +--
 lib/timeval.c|   4 +-
 ovsdb/table.h|   4 +-
 8 files changed, 91 insertions(+), 93 deletions(-)

--
2.4.11

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

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

Re: [ovs-dev] [patch_v4] ovn: Add additional comments regarding arp responders.

2016-10-10 Thread Mickey Spiegel 
This is getting close. Some rewording suggestions below.

On Thu, Oct 6, 2016 at 10:34 AM, Darrell Ball  wrote:

> There has been enough confusion regarding logical switch datapath
> arp responders in ovn to warrant some additional comments;
> hence add a general description regarding why they exist and
> document the special cases.
>
> Signed-off-by: Darrell Ball 
> ---
>
> Note this patch is meant to be merge with the code change for vtep
> inport handling here.
> https://patchwork.ozlabs.org/patch/675796/
>
> v3->v4: Capitalization fixes.
> Reinstate comment regarding L2 learning confusion.
>
> v2->v3: Reword and further elaborate.
>
> v1->v2: Dropped RFC code change for logical switch router
> type ports
>
>  ovn/northd/ovn-northd.8.xml | 52 ++
> +--
>  1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 77eb3d1..5ac351d 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -415,20 +415,60 @@
>  Ingress Table 9: ARP/ND responder
>
>  
> -  This table implements ARP/ND responder for known IPs.  It contains
> these
> -  logical flows:
> +  This table implements ARP/ND responder for known IPs.


I agree with Han Zhou that mentioning logical switch explicitly helps
clarify things.

+  The advantage
> +  of the ARP responder flow is to limit ARP broadcasts by locally
> +  responding to ARP requests without the need to send to other
> +  hypervisors.  One common case is when the inport is a logical
> +  port associated with a VIF and the broadcast is responded to on the
> +  local hypervisor rather than broadcast across the whole network and
> +  responded to by the destination VM.  This behavior is proxy ARP.
>

Agree up to this point. Wondering if there should be multiple paragraphs,
with the text above being the first paragraph.


> +  ARP requests received by multiple hypervisors, as in the case of
> +  localnet and vtep logical inports need
> +  to skip these logical switch ARP responders;  the reason being
> +  that northd downloads the same mac binding rules to all hypervisors
> +  and all hypervisors will receive the ARP request from the external
> +  network and respond.  This will confuse L2 learning on the source
> +  of the ARP requests.  These skip rules are mentioned under
> +  priority-100 flows.


I am OK with Han Zhou's suggestion to move this description to the
priority-100 flows themselves. If you do that, then the l2 gateway text
below should also move to the priority-100 flows.


> +  ARP requests arrive from VMs with a logical
> +  switch inport type of type empty, which is the default.  For this
> +  case, the logical switch proxy ARP rules can be for other VMs or
> +  a logical router port.


Suggest to replace the above two lines with something generic like:

  Logical switch proxy ARP rules may be programmed both for IP
  addresses on logical switch VIF ports (of type empty, which is the
  default, representing connectivity to VMs or containers), and for IP
  addresses on logical switch router ports.  Note that it is common
  for ARP requests to be received on one type of port (e.g. of type
  empty, from a VM) for an IP address that resides on a different
  type of port (e.g. of type router).

+  In order to support proxy ARP for logical
> +  router ports, an IP address must be configured on the logical
> +  switch router type port, with the same value as the peer of the
> +  logical router port.  The configured MAC addresses must match as
> +  well.


Agree with the text above.

+  If this logical switch router type port does not have an
> +  IP address configured, ARP requests will hit another ARP responder
> +  on the logical router datapath itself, which is most commonly a
> +  distributed logical router.  The advantage of using the logical
> +  switch proxy ARP rule for logical router ports is that this rule
> +  is hit before the logical switch L2 broadcast rule.  This means
> +  the ARP request is not broadcast on this logical switch.


Han Zhou suggested:

If this logical switch router type port does not have an IP address
configured, although the request will still be responded by the ARP
responder on the logical router datapath, the ARP request will be broadcast
on the logical switch.

My proposal, building on top of the original text and Han's proposal:

If this logical switch router type port does not have an IP address
configured, the ARP request will be broadcast on the logical switch.
One of the copies of the ARP request will go through the logical
switch router type port to the logical router datapath, where the
logical router ARP responder will generate a reply.

+  Logical
> +  switch ARP

Re: [ovs-dev] [PATCH v4 1/3] Windows: Add internal switch port per OVS bridge

2016-10-10 Thread Sairam Venugopal 
Sorry for the delay in the review.

Acked-by: Sairam Venugopal 



On 8/12/16, 6:06 PM, "Alin Serdean" 
wrote:

>This patch updates the following commands in the vswitch:
>ovs-vsctl add-br br-test
>ovs-vsctl del-br br-test
>
>ovs-vsctl add-br br-test:
>This command will now create an internal port on the MSFT virtual
>switch
>  using the WMI interface from Msvm_VirtualEthernetSwitchManagementService
>  leveraging the method AddResourceSettings.
>Before creating the actual port, the switch will be queried to see if
>there
>  is not a port already created (good for restarts when restarting the
>  vswitch daemon). If there is a port defined it will return success and
>log
>  a message.
>After checking if the port already exists the command will also verify
>  if the forwarding extension (windows datapath) is enabled and on a
>single
>  switch. If it is not activated or if it is activated on multiple
>switches
>  it will return an error and a message will be logged.
>After the port was created on the switch, we will disable the adapter
>on
>  the host and rename to the corresponding OVS bridge name for
>consistency.
>The user will enable and set the values he wants after creation.
>
>ovs-vsctl del-br br-test
>This command will remove an internal port on the MSFT virtual switch
>  using the Msvm_VirtualEthernetSwitchManagementService class and
>executing
>  the method RemoveResourceSettings.
>
>Both commands will be blocking until the WMI job is finished, this allows
>us
>to guarantee that the ports are created and their name are set before
>issuing
>a netlink message to the windows datapath.
>
>This patch also includes helpers for normal WMI retrievals and
>initializations.
>Appveyor and documentation has been modified to include the libraries
>needed
>for COM objects.
>
>This patch was tested individually using IMallocSpy and CRT heap checks
>to ensure no new memory leaks are introduced.
>
>Tested on the following OS's:
>Windows 2012 and Windows 2012r2
>
>Signed-off-by: Alin Gabriel Serdean 
>Acked-by: Paul Boca 
>---
>v4: address comments (sanitize input, short refactor)
>use full stop on comments
>update coding style
>v3: rebase, add acked
>v2: Address comments. Rebase
>---
> appveyor.yml   |2 +-
> lib/automake.mk|4 +-
> lib/dpif-netlink.c |   21 +
> lib/wmi.c  | 1276
>
> lib/wmi.h  |   51 +++
> 5 files changed, 1352 insertions(+), 2 deletions(-)
> create mode 100644 lib/wmi.c
> create mode 100644 lib/wmi.h
>
>diff --git a/appveyor.yml b/appveyor.yml
>index 0fd003b..1061df6 100644
>--- a/appveyor.yml
>+++ b/appveyor.yml
>@@ -41,5 +41,5 @@ build_script:
> - C:\MinGW\msys\1.0\bin\bash -lc "cp
>/c/pthreads-win32/Pre-built.2/dll/x86/*.dll /c/openvswitch/."
> - C:\MinGW\msys\1.0\bin\bash -lc "mv /bin/link.exe /bin/link_copy.exe"
> - C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./boot.sh"
>-- C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./configure
>CC=build-aux/cccl LD=\"`which link`\" LIBS=\"-lws2_32 -liphlpapi\"
>--with-pthread=C:/pthreads-win32/Pre-built.2
>--with-openssl=C:/OpenSSL-Win32 --with-vstudiotarget=\"Debug\""
>+- C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./configure
>CC=build-aux/cccl LD=\"`which link`\" LIBS=\"-lws2_32 -liphlpapi
>-lwbemuuid -lole32 -loleaut32\"
>--with-pthread=C:/pthreads-win32/Pre-built.2
>--with-openssl=C:/OpenSSL-Win32 --with-vstudiotarget=\"Debug\""
> - C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && make"
>diff --git a/lib/automake.mk b/lib/automake.mk
>index 2faaeac..99bbdc0 100644
>--- a/lib/automake.mk
>+++ b/lib/automake.mk
>@@ -387,7 +387,9 @@ lib_libopenvswitch_la_SOURCES += \
>   lib/netlink-notifier.h \
>   lib/netlink-protocol.h \
>   lib/netlink-socket.c \
>-  lib/netlink-socket.h
>+  lib/netlink-socket.h \
>+  lib/wmi.c \
>+  lib/wmi.h
> endif
> 
> if HAVE_POSIX_AIO
>diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>index a39faa2..c8b0e37 100644
>--- a/lib/dpif-netlink.c
>+++ b/lib/dpif-netlink.c
>@@ -58,6 +58,7 @@
> 
> VLOG_DEFINE_THIS_MODULE(dpif_netlink);
> #ifdef _WIN32
>+#include "wmi.h"
> enum { WINDOWS = 1 };
> #else
> enum { WINDOWS = 0 };
>@@ -849,6 +850,16 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif,
>struct netdev *netdev,
> #endif
> }
> 
>+#ifdef _WIN32
>+if (request.type == OVS_VPORT_TYPE_INTERNAL) {
>+if (!create_wmi_port(name)){
>+VLOG_ERR("Could not create wmi internal port with name:%s",
>name);
>+vport_del_socksp(dpif, socksp);
>+return EINVAL;
>+};
>+}
>+#endif
>+
> tnl_cfg = netdev_get_tunnel_config(netdev);
> if (tnl_cfg && (tnl_cfg->dst_port != 0 || tnl_cfg->exts)) {
> ofpbuf_use_stack(, options_stub, sizeof options_stub);
>@@ -940,6 +951,16 @@

Re: [ovs-dev] [PATCH] datapath_windows: Set isActivated flag only on success

2016-10-10 Thread Sairam Venugopal 
Thanks for the patch. Usually we append Œdatapath-windows: Brief
description¹ for Windows datapath commmits.

Had a comment which is inlined.

Thanks,
Sairam

On 10/5/16, 2:33 PM, "Shashank Ram"  wrote:

>@Switch.c: Modifies OvsActivateSwitch() function
>to mark the switch as activated only if the
>the status is success. The callers itself
>only call this method when the isActivated
>flag is unset.
>
>Signed-off-by: Shashank Ram 
>---
> datapath-windows/ovsext/Switch.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Switch.c
>b/datapath-windows/ovsext/Switch.c
>index 825fa3c..7913cd5 100644
>--- a/datapath-windows/ovsext/Switch.c
>+++ b/datapath-windows/ovsext/Switch.c
>@@ -556,7 +556,6 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
> OVS_LOG_TRACE("Enter: activate switch %p, dpNo: %ld",
>   switchContext, switchContext->dpNo);
>
>-switchContext->isActivated = TRUE;
> status = OvsAddConfiguredSwitchPorts(switchContext);
>
> if (status != NDIS_STATUS_SUCCESS) {
>@@ -573,7 +572,7 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
> }

Sai: You can add switchContext->isActivated = TRUE; before the cleanup
label.
This way you can get rid of the check for status==NDIS_STATUS_SUCCESS.

>
> cleanup:
>-if (status != NDIS_STATUS_SUCCESS) {
>+if (status == NDIS_STATUS_SUCCESS) {
> switchContext->isActivated = TRUE;
> }
>
>--
>2.6.2
>
>___
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev=CwIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=Dc
>ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ=JEXx0nmOWA23mhKMOcUJzNcIULZ8kH
>WeAtQcmFjGo6k=5tUWl0geJxE6uBwW6svE_7kChh5iUVlo8kVnpFUrZSA= 

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

Re: [ovs-dev] [patch_v4] ovn: Add additional comments regarding arp responders.

2016-10-10 Thread Darrell Ball 
On Fri, Oct 7, 2016 at 5:25 PM, Han Zhou  wrote:

> Overall it looks good to me. Just suggestions for rewording.
>
> On Thu, Oct 6, 2016 at 10:34 AM, Darrell Ball  wrote:
> >
> > There has been enough confusion regarding logical switch datapath
> > arp responders in ovn to warrant some additional comments;
> > hence add a general description regarding why they exist and
> > document the special cases.
> >
> > Signed-off-by: Darrell Ball 
> > ---
> >
> > Note this patch is meant to be merge with the code change for vtep
> > inport handling here.
> > https://patchwork.ozlabs.org/patch/675796/
> >
> > v3->v4: Capitalization fixes.
> > Reinstate comment regarding L2 learning confusion.
> >
> > v2->v3: Reword and further elaborate.
> >
> > v1->v2: Dropped RFC code change for logical switch router
> > type ports
> >
> >  ovn/northd/ovn-northd.8.xml | 52 ++
> +--
> >  1 file changed, 46 insertions(+), 6 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index 77eb3d1..5ac351d 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> > @@ -415,20 +415,60 @@
> >  Ingress Table 9: ARP/ND responder
> >
> >  
> > -  This table implements ARP/ND responder for known IPs.
>
> To avoid confusing with lrouter ARP responder, it is better to be: this
> table implements ARP/ND responder in *logical switch*, for known IPs.
>



This section is under
"Logical Switch Datapaths",
so the context is should be clear that it refers to "Logical Switch"
but it you are confused, I can add a redundant reference to Logical Switches
- it is just a few words.



>
> > +  ARP requests received by multiple hypervisors, as in the case of
> > +  localnet and vtep logical inports need
> > +  to skip these logical switch ARP responders;  the reason being
> > +  that northd downloads the same mac binding rules to all
> hypervisors
> > +  and all hypervisors will receive the ARP request from the external
> > +  network and respond.  This will confuse L2 learning on the source
> > +  of the ARP requests.  These skip rules are mentioned under
> > +  priority-100 flows.
>
> Suggest to avoid this detail here, but just describe in the priority-100
> flow section. Too much details here adds redundancy and not helpful to
> overall understanding. For example, there is also another case to skip
> which is when IP is owned by the inport, and it is described separately
> already.
>

I am fine with moving it back to the priority-100 flow sub-section.



>
> > +   ARP requests arrive from VMs with a
> logical
> > +  switch inport type of type empty, which is the default.  For this
> > +  case,
>
> This part of text is correct, but I feel it doesn't help understanding but
> add confusion, so suggest to remove.
>

I would like to keep it as I found it useful; I will remove the "empty" and
keep default
for type.



>
> > +   If this logical switch router type port does not have an
> > +  IP address configured, ARP requests will hit another ARP responder
> > +  on the logical router datapath itself, which is most commonly a
> > +  distributed logical router.  The advantage of using the logical
> > +  switch proxy ARP rule for logical router ports is that this rule
> > +  is hit before the logical switch L2 broadcast rule.  This means
> > +  the ARP request is not broadcast on this logical switch.
>
> How about:
>
> If this logical switch router type port does not have an IP address
> configured, although the request will still be responded by the ARP
> responder on the logical router datapath, the ARP request will be broadcast
> on the logical switch.
>

I can reword the concept folding in part of your shorter wording.


>
> > + Note that ARP requests
> received from
> > +  localnet or vtep logical inports can
> > +  either go directly to VMs, in which case the VM responds or can
> > +  hit an ARP responder for a logical router port if the packet is
> > +  used to resolve a logical router port next hop address.
>
> This part of text is somehow redundant with the above. If this is to be
> emphasised, we may put it under the section for router ARP responder flows
> description, to explain why we still need ARP responder for on router
> datapath.
>

I am ok to omit this part for now. I thought it was obvious - I only had
added it
to clarify a misleading feedback I had previously received to set that
straight.



>
> > + The
> inport being of type
> > +router has no known use case for these ARP
> > +responders.  However, no skip flows are installed for these
> > +packets, as there would be some additional flow cost for this
>

Re: [ovs-dev] [PATCH 4/4] expr: Better simplify some special cases of expressions.

2016-10-10 Thread Andy Zhou 
On Thu, Oct 6, 2016 at 8:30 PM, Ben Pfaff  wrote:

> It's pretty unlikely that a human would write expressions like these, but
> they can come up in machine-generated expressions and it seems best to
> simplify them in an efficient way.
>
> Signed-off-by: Ben Pfaff 
>
Acked-by: Andy Zhou 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/4] expr: Improve test so that it would have found the bugs I just fixed.

2016-10-10 Thread Andy Zhou 
On Thu, Oct 6, 2016 at 8:30 PM, Ben Pfaff  wrote:

> The test didn't check for x == 0/0 or x != 0/0 cases, and thus they were
> buggy.
>
> Also, add "expression" as a keyword for tests that only had "expressions"
> (plural).
>
> Signed-off-by: Ben Pfaff 
>
Acked-by: Andy Zhou 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2] datapath-windows: Set isActivated flag only on success

2016-10-10 Thread Shashank Ram 
@Switch.c: Modifies OvsActivateSwitch() function
to mark the switch as activated only if the
the status is success. The callers itself
only call this method when the isActivated
flag is unset.

Signed-off-by: Shashank Ram 
---
 datapath-windows/ovsext/Switch.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/datapath-windows/ovsext/Switch.c b/datapath-windows/ovsext/Switch.c
index 825fa3c..49711a9 100644
--- a/datapath-windows/ovsext/Switch.c
+++ b/datapath-windows/ovsext/Switch.c
@@ -556,7 +556,6 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
 OVS_LOG_TRACE("Enter: activate switch %p, dpNo: %ld",
   switchContext, switchContext->dpNo);

-switchContext->isActivated = TRUE;
 status = OvsAddConfiguredSwitchPorts(switchContext);

 if (status != NDIS_STATUS_SUCCESS) {
@@ -572,11 +571,9 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
 goto cleanup;
 }

-cleanup:
-if (status != NDIS_STATUS_SUCCESS) {
-switchContext->isActivated = TRUE;
-}
+switchContext->isActivated = TRUE;

+cleanup:
 OVS_LOG_TRACE("Exit: activate switch:%p, isActivated: %s, status = %lx",
   switchContext,
   (switchContext->isActivated ? "TRUE" : "FALSE"), status);
--
2.6.2

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

Re: [ovs-dev] [PATCH 1/4] expr: Simplify "x == 0/0" into 1.

2016-10-10 Thread Andy Zhou 
On Thu, Oct 6, 2016 at 8:30 PM, Ben Pfaff  wrote:

> An expression like "x == 0/0" does not test any actual bits in field x,
> so it resolves to true, but expr_simplify() was not smart enough to see
> this.
>
> This goes beyond an optimization, to become a bug fix, because
> expr_normalize() will assert-fail for expressions that become trivial
> when this simplification is omitted.  For example:
>
> $ echo 'eth.dst == 0/0 && eth.dst == 0/0' | tests/ovstest test-ovn
> normalize-expr
> test-ovn: ../include/openvswitch/list.h:245: assertion
> !ovs_list_is_empty(list) failed in ovs_list_front()
> Aborted (core dumped)
>
> This commit fixes this and related problems.
>
> The test added by this commit is very specific to the particular problem,
> whereas a more general test would be better.  A later commit adds the
> general test.
>
> Signed-off-by: Ben Pfaff 

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

[ovs-dev] [PATCH] datapath-windows: Set isActivated flag only on success

2016-10-10 Thread Shashank Ram 
@Switch.c: Modifies OvsActivateSwitch() function
to mark the switch as activated only if the
the status is success. The callers itself
only call this method when the isActivated
flag is unset.

Signed-off-by: Shashank Ram 
---
 datapath-windows/ovsext/Switch.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/datapath-windows/ovsext/Switch.c b/datapath-windows/ovsext/Switch.c
index 825fa3c..49711a9 100644
--- a/datapath-windows/ovsext/Switch.c
+++ b/datapath-windows/ovsext/Switch.c
@@ -556,7 +556,6 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
 OVS_LOG_TRACE("Enter: activate switch %p, dpNo: %ld",
   switchContext, switchContext->dpNo);

-switchContext->isActivated = TRUE;
 status = OvsAddConfiguredSwitchPorts(switchContext);

 if (status != NDIS_STATUS_SUCCESS) {
@@ -572,11 +571,9 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
 goto cleanup;
 }

-cleanup:
-if (status != NDIS_STATUS_SUCCESS) {
-switchContext->isActivated = TRUE;
-}
+switchContext->isActivated = TRUE;

+cleanup:
 OVS_LOG_TRACE("Exit: activate switch:%p, isActivated: %s, status = %lx",
   switchContext,
   (switchContext->isActivated ? "TRUE" : "FALSE"), status);
--
2.6.2

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

Re: [ovs-dev] [PATCH v2] datapath-windows: Set isActivated flag only on success

2016-10-10 Thread Sairam Venugopal 
Acked-by: Sairam Venugopal 


On 10/10/16, 3:15 PM, "Shashank Ram"  wrote:

>@Switch.c: Modifies OvsActivateSwitch() function
>to mark the switch as activated only if the
>the status is success. The callers itself
>only call this method when the isActivated
>flag is unset.
>
>Signed-off-by: Shashank Ram 
>---
> datapath-windows/ovsext/Switch.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Switch.c
>b/datapath-windows/ovsext/Switch.c
>index 825fa3c..49711a9 100644
>--- a/datapath-windows/ovsext/Switch.c
>+++ b/datapath-windows/ovsext/Switch.c
>@@ -556,7 +556,6 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
> OVS_LOG_TRACE("Enter: activate switch %p, dpNo: %ld",
>   switchContext, switchContext->dpNo);
>
>-switchContext->isActivated = TRUE;
> status = OvsAddConfiguredSwitchPorts(switchContext);
>
> if (status != NDIS_STATUS_SUCCESS) {
>@@ -572,11 +571,9 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
> goto cleanup;
> }
>
>-cleanup:
>-if (status != NDIS_STATUS_SUCCESS) {
>-switchContext->isActivated = TRUE;
>-}
>+switchContext->isActivated = TRUE;
>
>+cleanup:
> OVS_LOG_TRACE("Exit: activate switch:%p, isActivated: %s, status =
>%lx",
>   switchContext,
>   (switchContext->isActivated ? "TRUE" : "FALSE"),
>status);
>--
>2.6.2
>
>___
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev=CwIGaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=Dc
>ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ=JTQF9lK7WrFRmjxQdtU7cHaXMjUSVN
>R88qRe8HumYqs=sIOOdG_D2MkFJp_802P-OYepagoY0W56f4q75MyFr8Q= 

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

Re: [ovs-dev] [PATCH 2/4] expr: Fix abort when simplifying "x != 0/0".

2016-10-10 Thread Andy Zhou 
On Thu, Oct 6, 2016 at 8:30 PM, Ben Pfaff  wrote:

> The test added by this commit is very specific to the particular problem,
> whereas a more general test would be better.  A later commit adds the
> general test.
>
> Signed-off-by: Ben Pfaff 
>
Acked-by: Andy Zhou 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] YOU WIN (€300,000 EUR) E.I.P 2016

2016-10-10 Thread European Internet Promo 

Congratulations!
You have won €300,000 EUR in the European Internet Promo 2016. Your Winning 
code is AHA4777.Send your (1) Full name (2) Telephone no. (3) Address (4) Age 
(5) Present Country (6) Occupation
Congratulations once again,European Internet Promo Team.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 08/12] dpif-netdev: Reorder elements in dp_netdev_port structure.

2016-10-10 Thread Bodireddy, Bhanuprakash 
>-Original Message-
>From: Jarno Rajahalme [mailto:ja...@ovn.org]
>Sent: Friday, October 7, 2016 10:11 PM
>To: Bodireddy, Bhanuprakash 
>Cc: dev@openvswitch.org
>Subject: Re: [ovs-dev] [PATCH 08/12] dpif-netdev: Reorder elements in
>dp_netdev_port structure.
>
>Would equivalent packing be achieved by moving the line down before the
>bool instead? If yes, it would be preferable.
Absolutely yes, I would do this in v2. 

Regards,
Bhanu Prakash. 

>
>Acked-by: Jarno Rajahalme 
>
>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
> wrote:
>>
>> By reordering the data elements in dp_netdev_port structure, pad bytes
>> can be reduced and there by saving a cache line.
>>
>> Before: structure size:136, holes:3, sum padbytes:15, cachelines:3
>> After: structure size:128, holes:1, sum padbytes:7, cachelines:2
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>> 
>> Signed-off-by: Antonio Fischetti 
>> ---
>> lib/dpif-netdev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
>> dfc9cbd..262f4de 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -284,10 +284,10 @@ struct dp_netdev_rxq {
>> /* A port in a netdev-based datapath. */ struct dp_netdev_port {
>> odp_port_t port_no;
>> +unsigned n_rxq; /* Number of elements in 'rxq' */
>> struct netdev *netdev;
>> struct hmap_node node;  /* Node in dp_netdev's 'ports'. */
>> struct netdev_saved_flags *sf;
>> -unsigned n_rxq; /* Number of elements in 'rxq' */
>> struct dp_netdev_rxq *rxqs;
>> bool dynamic_txqs;  /* If true XPS will be used. */
>> unsigned *txq_used; /* Number of threads that uses each tx queue.
>*/
>> --
>> 2.4.11
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev

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

Re: [ovs-dev] [PATCH 07/12] dpif-netdev: Cache align netdev_flow_keys.

2016-10-10 Thread Bodireddy, Bhanuprakash 
>-Original Message-
>From: Daniele Di Proietto [mailto:diproiet...@ovn.org]
>Sent: Friday, October 7, 2016 11:46 PM
>To: Bodireddy, Bhanuprakash 
>Cc: dev@openvswitch.org; Jarno Rajahalme 
>Subject: Re: [ovs-dev] [PATCH 07/12] dpif-netdev: Cache align
>netdev_flow_keys.
>
>
>
>2016-10-07 14:10 GMT-07:00 Jarno Rajahalme :
>
>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
> wrote:
>>
>> Aligning the 'keys' array seems to positively impact performance.
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>
>> Signed-off-by: Antonio Fischetti 
>> ---
>> lib/dpif-netdev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index d0bb191..dfc9cbd 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -4157,7 +4157,7 @@ dp_netdev_input__(struct
>dp_netdev_pmd_thread *pmd,
>>     /* Sparse or MSVC doesn't like variable length array. */
>>     enum { PKT_ARRAY_SIZE = NETDEV_MAX_BURST };
>> #endif
>> -    struct netdev_flow_key keys[PKT_ARRAY_SIZE];
>> +    struct netdev_flow_key keys[PKT_ARRAY_SIZE]
>__attribute__((aligned(64)));
>
>Due to compiler compatibility you must use OVS_ALIGNED_VAR(64) instead.
>
>I would also use the CACHE_LINE_SIZE define, instead of 64

Agree, I would change to OVS_ALIGNED_VAR(CACHE_LINE_SIZE) when I send v2.

Regards,
Bhanu Prakash. 


>Thanks,
>Daniele
>
>>     struct packet_batch_per_flow batches[PKT_ARRAY_SIZE];
>>     long long now = time_msec();
>>     size_t newcnt, n_batches, i;
>> --
>> 2.4.11
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>___
>dev mailing list
>dev@openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev

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