Re: [ovs-dev] [PATCH v2 1/3] Add multipath static router in OVN northd and north-db

2017-10-25 Thread Gao Zhenyu 
ping.


Thanks
Zhenyu Gao

2017-10-11 9:55 GMT+08:00 Gao Zhenyu :

> Hi Miguel,
>
>Thanks for your suggestion on it. It's very usefull.
>In my point of view, I think no matter we have single router leg or
> multiple router legs on edge router, we still need a way to dispatch
> traffic randomly, right?
>So even we implement multiple legs on a router we can't random seperate
> traffics to those legs easily.(static route only seperates specific
> traffic) Then multipath action is a good candidate to make it.
>
>Currently, gateway chassises are links to single ovn edge logical
> router port, which means those gateways chassises output traffic contain
> same src mac.
>I don't know if we have a good way to implement L3HA A/A in current
> architecture. (Maybe adding  gateway_chassis options field, populate
> "rewrite-mac", "rewrite-ip" to rewrite mac address is a way, but I don't
> think it is a good way and may confuse people)
>So if you already get a idea to make it, it would be great to bring it
> up then we can discuss it and move whole process faster. :)
>
>
> Thanks
> Zhenyu Gao
>
> 2017-10-11 9:50 GMT+08:00 Gao Zhenyu :
>
>> I discussed this multipath stuff with Miguel in other mailling thread and
>> I want to bring this discusstion on ovs mailing list and hope to collect
>> more suggestions from all of you. :)
>>
>> Here is the Miguel's suggestion on it.
>>
>> =
>> Hi Gao,
>>
>>Sorry, I didn't have more time to look at it currently (although it's
>> a topic of my interest.)
>>
>>I'm worried of the replication of concerns inside networking-ovn
>> related routing, and I don't see the advantage of l3gateway mode, beyond
>> legacy usage.
>>
>>I understand the limitation you expressed about the
>> "chassisredirect"/"gatewaychassis" mode only being able to expose a
>> single external router leg.
>>
>>If that's a limitation that doesn't work for you, my opinion is that
>> we should work on fixing that limitation, and keeping all our development
>> efforts in a single place, with distributed E/W routing.
>>
>>In such way we could construct L3HA A/A , by having every
>> gateway_chassis have the same priority, and possible some extra options.
>>
>>But again, please, this is a discussion we may have on the development
>> mailing list, because may be my point of view is too narrow.
>>
>> Can you bring it up on the mailing list, or do you want me to do it?
>>
>>Best regards,
>> =
>>
>> 2017-10-08 17:42 GMT+08:00 Gao Zhenyu :
>>
>>> Comments and suggestions are welcome :)
>>>
>>> Thanks
>>> Zhenyu Gao
>>>
>>> 2017-09-26 17:52 GMT+08:00 Zhenyu Gao :
>>>
 1. ovn-nb.ovsschema was updated output_port field. Change the max entry
 number from 1 to unlimited.
 2. Add multipath feature in ovn-northd part. northd generates multipath
 flows to dispatch traffic by using packet's IP dst address if route's
 output_port contains two or more ports.
 3. Add new table(lr_in_multipath) in ovn-northd's router ingress stages
 to dispatch traffic to ports.
 4. Add multipath flow in Table 5(lr_in_ip_routing) and store hash result
 into reg0. reg9[2] was used to indicate packet which need dispatching.
 5. Add multipath feature description in ovn/northd/ovn-northd.8.xml
 and ovn/ovn-nb.xml
 6. ovn-nbctl.c was updated to handle configuring mulitiple output_port.

 Signed-off-by: Zhenyu Gao 
 ---
  ovn/northd/ovn-northd.8.xml |  67 +++-
  ovn/northd/ovn-northd.c | 257 ++
 +++---
  ovn/ovn-nb.ovsschema|   7 +-
  ovn/ovn-nb.xml  |   4 +
  ovn/utilities/ovn-nbctl.c   |  28 +++--
  5 files changed, 311 insertions(+), 52 deletions(-)

 diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
 index 0d85ec0..b1ce9a9 100644
 --- a/ovn/northd/ovn-northd.8.xml
 +++ b/ovn/northd/ovn-northd.8.xml
 @@ -1598,6 +1598,9 @@ icmp4 {
port (ingress table ARP Request will generate an ARP
request, if needed, with reg0 as the target protocol
address and reg1 as the source protocol address).
 +  A IP route can be configured that it has multipath to next-hop.
 +  If a packet has multipath to destination, OVN assign the port
 +  index into reg[0] to indicate the packet's output port in table
 6.
  

  
 @@ -1617,6 +1620,28 @@ icmp4 {


  
 +  IPv4/IPV6 multipath routing table. For each route to
 IPv4/IPv6
 +  network N with netmask M, on multipath
 port
 +  P with IP address A and Ethernet
 +  address E, a logical flow with match
 +  ip4.dst ==N/M,whose

Re: [ovs-dev] [PATCH] ovn-northd: Do not add lflows in lr_in_arp_resolve stage for disabled logical ports

2017-10-25 Thread Numan Siddique 
On Thu, Oct 26, 2017 at 2:59 AM, Ben Pfaff  wrote:

> On Wed, Sep 27, 2017 at 09:36:18PM +0530, nusid...@redhat.com wrote:
> > From: Numan Siddique 
> >
> > ovn-northd is adding the below logical flow for a disabled logical port
> (with mac M
> > and IP 'A')
> >
> > table=6 (lr_in_arp_resolve  ), match=(outport == "lrp-port" && reg0 ==
> 'A'),
> > action=(eth.dst = 'M'; next;)
> >
> > In the case of openstack load balancer 'octavia' service, it creates
> logical
> > ports 'P1' (M1 IP1) and 'P2' (M2 IP2). It then disables logical port P2
> and
> > adds IP2 to P1 - (M1 IP1 IP2).
> >
> > When another port tries to reach IP2, it doesn't get delivered to port
> P1 because
> > of the above flow.
> >
> > Signed-off-by: Numan Siddique 
>
> Thanks a lot, I applied this to master.
>
> Please let me know if I should backport it to 2.8 (or earlier).
>

Thanks for the review. It would be great if it can be backported to 2.8.

Numan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Do not add lflows in lr_in_arp_resolve stage for disabled logical ports

2017-10-25 Thread Ben Pfaff 
On Wed, Sep 27, 2017 at 09:36:18PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique 
> 
> ovn-northd is adding the below logical flow for a disabled logical port (with 
> mac M
> and IP 'A')
> 
> table=6 (lr_in_arp_resolve  ), match=(outport == "lrp-port" && reg0 == 'A'),
> action=(eth.dst = 'M'; next;)
> 
> In the case of openstack load balancer 'octavia' service, it creates logical
> ports 'P1' (M1 IP1) and 'P2' (M2 IP2). It then disables logical port P2 and
> adds IP2 to P1 - (M1 IP1 IP2).
> 
> When another port tries to reach IP2, it doesn't get delivered to port P1 
> because
> of the above flow.
> 
> Signed-off-by: Numan Siddique 

Thanks a lot, I applied this to master.

Please let me know if I should backport it to 2.8 (or earlier).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] DNS support feature (was: Re: DNS support options)

2017-10-25 Thread Yifeng Sun 
I feel that unbound stands out in the available open source DNS resolver.

Below is the summary for unbound:
* The actual resolving work is done by a background process or thread. A
background process or thread seems unavoidable. Linux's getaddrinfo_a
clones a thread similarly.
* It is ported on Linux, BSD, Windows, MacOS/X and Solaris/SPARC. This is
good because OVS runs on a large range of platforms.
* It complies to the standard, with optional DNSSEC support. Some of its
features may not be needed in our case.
* The unbound context is thread-safe. Its internal locks may bring some
overhead. But since the DNS resolving is not frequent in OVS, I suppose
this small overhead is not an issue.

Unbound looks like a good option. Another option is to create a background
thread which processes DNS resolving requests from the main thread and
sends back the resulting events to the main thread. This method is quite
simple and straightforward.

The above are what I got so far. Please give your thoughts, thanks a lot.

Below is the link for original discussion:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/337038.html



On Wed, Oct 25, 2017 at 2:11 PM, Ben Pfaff  wrote:

> Hello everyone, please allow me to introduce Yifeng Sun (CCed), who
> recently joined VMware's Open vSwitch team.  I've asked Yifeng to start
> out by working on DNS support for Open vSwitch.  Yifeng, can you tell us
> about what you've discovered so far, based on this thread from August
> that I'm reviving, and your tentative conclusions?
>
> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] DNS support feature (was: Re: DNS support options)

2017-10-25 Thread Ben Pfaff 
Hello everyone, please allow me to introduce Yifeng Sun (CCed), who
recently joined VMware's Open vSwitch team.  I've asked Yifeng to start
out by working on DNS support for Open vSwitch.  Yifeng, can you tell us
about what you've discovered so far, based on this thread from August
that I'm reviving, and your tentative conclusions?

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn: Add LB flows for logical router with gateway port

2017-10-25 Thread Ben Pfaff 
On Thu, Sep 21, 2017 at 09:22:16PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique 
> 
> This patch adds support for associating a load balancer to a
> logical router with gateway router port which was missing earlier.
> 
> Signed-off-by: Numan Siddique 

It doesn't look like this patch got any reviews.  It's not my area of
expertise.  Guru, is this something that you could look at?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovn pacemaker: Provide the option to configure inactivity probe value

2017-10-25 Thread Ben Pfaff 
On Mon, Oct 16, 2017 at 03:12:07PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique 
> 
> In the case of OVN HA deployments with openstack, it has been noticed
> that the 5 seconds inactivity probe interval is not enough and ovsdb-servers
> do not get the echo reply back from the IDL clients and disconnects the
> connections.
> 
> This patch
>- providdes an option to configure this value.
>- creates a connection row in NB/SB dbs and sets the target and
>  inactivity_probe values when the node is promoted to master.
> 
> CC: Andy Zhou 
> Signed-off-by: Numan Siddique 

Applied to master, thanks Numan!

This solution isn't ideal so I hope that we continue to evolve it.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Update OvsCompleteNbl argument to match definition

2017-10-25 Thread Shashank Ram 

Update the OvsCompleteNbl to take in a PVOID and explicitly cast to
POVS_SWITCH_CONTEXT. This is useful when finding declarations in Visual
Studio. The mismatch breaks this functionality.

Found by inspection.

Signed-off-by: Sairam Venugopal 
---

Acked-by: Shashank Ram 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Soluciones para aumentar su productividad

2017-10-25 Thread Optimización del uso del tiempo para ejecutivos 
Optimización de procesos y maximización de beneficios

Optimización del uso del tiempo para ejecutivos financieros y personal 
administrativo
10 de noviembre - Lic. Dolores Romero Mora9am-8pm

La productividad se encuentra estrechamente relacionada con la llamada "Ley del 
mínimo esfuerzo", es decir, maximizar resultados empleando el mínimo de costos. 
En general, cuando una persona se queda después de su jornada laboral, trabaja 
durante su hora de almuerzo, se lleva labores para la casa, el fin de semana 
continúa ocupándose de pendientes de la oficina y aun así no avanza en sus 
objetivos, es un indicador de problemas en la distribución de su tiempo en la 
semana laboral.

BENEFICIOS DE ASISTIR: 

- Desarrollará las habilidades fundamentales que debe poseer todo ejecutivo 
eficiente - Identificará sus quita tiempos y revisarán alternativas de solución 
- Recibirá herramientas prácticas para aplicarlas en el trabajo y la vida 
personal - Generará un plan de acción que será revisado por su jefe inmediato 

¿Requiere la información a la Brevedad? responda este email con la palabra: 
Ejecutivo + nombre - teléfono - correo.


centro telefónico:018002120744


 


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] Check flow's dl_type before setting ct_orig_tuple in 'pkt_metadata_from_flow()'

2017-10-25 Thread Ben Pfaff 
On Wed, Oct 25, 2017 at 11:33:03PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique 
> 
> Normally flow's dl_type will be a valid value. However when a packet is sent 
> to
> the controller, dl_type is not stored in the 'ofputil_packet_in_private'. When
> the controller resumes (OFPRAW_NXT_RESUME) the packet, the flow's dl_type 
> will be
> 0. If the flow's ct_state has valid value, then the 'pkt_metadata_from_flow'
> neither sets the ct_orig_tuple from the flow nor resets it. This results in 
> invalid
> value ct_orig_tuple in the pkt_metadata.
> 
> This patch handles this situation by checking the dl_type before setting the
> ct_orig_tuple. If dl_type is 0, it resets it. It also resets ct_orig_tuple if
> dl_type is non zero and other than IPv4 or IPv6.
> 
> Reported-by: Daniel Alvarez Sanchez 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html
> Signed-off-by: Numan Siddique 

Thanks a lot, I applied this to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

2017-10-25 Thread Numan Siddique 
On Wed, Oct 25, 2017 at 10:51 PM, Ben Pfaff  wrote:

> On Wed, Oct 25, 2017 at 06:50:29PM +0530, Numan Siddique wrote:
> > On Wed, Oct 25, 2017 at 3:09 PM, Daniel Alvarez Sanchez <
> dalva...@redhat.com
> > > wrote:
> >
> > >
> > >
> > > On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff  wrote:
> > >
> > >> On Tue, Oct 24, 2017 at 02:27:59PM -0700, Ben Pfaff wrote:
> > >> > On Tue, Oct 24, 2017 at 11:07:58PM +0200, Daniel Alvarez Sanchez
> wrote:
> > >> > > Hi guys,
> > >> > >
> > >> > > Great job Numan!
> > >> > > As we discussed over IRC, the patch below may make more sense.
> > >> > > It essentially sets the dl_type so that when packet comes from the
> > >> > > controller, it matches
> > >> > > a valid type and OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 is not added.
> > >> > > Maybe what Numan proposed and this patch could be a good
> combination
> > >> but I
> > >> > > think
> > >> > > that we definitely need to set the dl_type as it's later checked
> in
> > >> > > pkt_metadata_from_flow()
> > >> > > and it'll be zero there otherwise.
> > >> > > What do you guys think? I have tried the patch below and the
> kernel
> > >> error
> > >> > > is not shown
> > >> > > anymore when issuing DHCP requests.
> > >> > >
> > >> > >
> > >> > > diff --git a/lib/flow.c b/lib/flow.c
> > >> > > index b2b10aa..62b948f 100644
> > >> > > --- a/lib/flow.c
> > >> > > +++ b/lib/flow.c
> > >> > > @@ -1073,6 +1073,7 @@ flow_get_metadata(const struct flow *flow,
> > >> struct
> > >> > > match *flow_metadata)
> > >> > >
> > >> > >  if (flow->ct_state != 0) {
> > >> > >  match_set_ct_state(flow_metadata, flow->ct_state);
> > >> > > +match_set_dl_type(flow_metadata, flow->dl_type);
> > >> > >  if (is_ct_valid(flow, NULL, NULL) && flow->ct_nw_proto
> != 0)
> > >> {
> > >> > >  if (flow->dl_type == htons(ETH_TYPE_IP)) {
> > >> > >  match_set_ct_nw_src(flow_metadata,
> flow->ct_nw_src);
> > >> >
> > >> > This patch seems reasonable too.
> > >> >
> > >> > I recommend adding a comment above it to explain what's going on,
> > >> > because dl_type is not a metadata field and it's confusing to deal
> with
> > >> > it in a context that's supposed to be all about metadata.  I guess
> the
> > >> > comment would essentially say that dl_type is essential to the
> > >> > interpretation of the conntrack metadata.
> > >>
> > >> Oh, and for this patch too I'd welcome a formal patch proposal.
> > >>
> > >
> > > Done. Thanks a lot Ben.
> > > If this get merged, it would be great if we can get it into 2.8 branch
> and
> > > add a new tag (2.8.2).
> > >
> > > Thanks!!
> > >
> >
> > Ben, we have submitted both the patches. The patch from Daniel - (
> > https://patchwork.ozlabs.org/patch/830160/)  will fix the issue.
> > Not sure if this patch  https://patchwork.ozlabs.org/patch/830132/ is
> > needed any more.
> >
> > Request to review these patches if possible as RDO is blocked on these
> > patches before we can  update from OVS 2.7.2 to OVS 2.8(.2)
>
> I've reviewed both.  I wasn't able to immediately apply either one, but
> they're obviously moving in the right direction, so I'd appreciate
> follow-up from both of you so that we can get them in.
>


Thanks for the review.

Numan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Check flow's dl_type before setting ct_orig_tuple in 'pkt_metadata_from_flow()'

2017-10-25 Thread Numan Siddique 
On Wed, Oct 25, 2017 at 10:46 PM, Ben Pfaff  wrote:

> On Wed, Oct 25, 2017 at 01:44:23PM +0530, nusid...@redhat.com wrote:
> > From: Numan Siddique 
> >
> > Normally flow's dl_type will be a valid value. However when a packet is
> sent to
> > the controller, dl_type is not stored in the
> 'ofputil_packet_in_private'. When
> > the controller resumes (OFPRAW_NXT_RESUME) the packet, the flow's
> dl_type will be
> > 0. If the flow's ct_state has valid value, then the
> 'pkt_metadata_from_flow'
> > neither sets the ct_orig_tuple from the flow nor resets it. This results
> in invalid
> > value ct_orig_tuple in the pkt_metadata.
> >
> > This patch handles this situation by checking the dl_type before setting
> the
> > ct_orig_tuple. If dl_type is 0, it resets it.
> >
> > Reported-by: Daniel Alvarez Sanchez 
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> October/339868.html
> > Signed-off-by: Numan Siddique 
> > ---
> >  lib/flow.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/flow.h b/lib/flow.h
> > index 6ae5a674d..8dd1a09a0 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -929,7 +929,7 @@ pkt_metadata_from_flow(struct pkt_metadata *md,
> const struct flow *flow)
> >  md->ct_label = flow->ct_label;
> >
> >  md->ct_orig_tuple_ipv6 = false;
> > -if (is_ct_valid(flow, NULL, NULL)) {
> > +if (flow->dl_type && is_ct_valid(flow, NULL, NULL)) {
> >  if (flow->dl_type == htons(ETH_TYPE_IP)) {
> >  md->ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
> >  flow->ct_nw_src,
>
> Thanks for the patch.  It builds and passes all the tests.
>
> There is still one case where ct_orig_tuple will not be initialized,
> which is the case where dl_type is nonzero but not IPv4 or IPV6; for
> example, if it is ARP.  Should we be careful to handle that case also?
>

Agree. Updated the patch.

Thanks for the review.

Numan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] Check flow's dl_type before setting ct_orig_tuple in 'pkt_metadata_from_flow()'

2017-10-25 Thread nusiddiq 
From: Numan Siddique 

Normally flow's dl_type will be a valid value. However when a packet is sent to
the controller, dl_type is not stored in the 'ofputil_packet_in_private'. When
the controller resumes (OFPRAW_NXT_RESUME) the packet, the flow's dl_type will 
be
0. If the flow's ct_state has valid value, then the 'pkt_metadata_from_flow'
neither sets the ct_orig_tuple from the flow nor resets it. This results in 
invalid
value ct_orig_tuple in the pkt_metadata.

This patch handles this situation by checking the dl_type before setting the
ct_orig_tuple. If dl_type is 0, it resets it. It also resets ct_orig_tuple if
dl_type is non zero and other than IPv4 or IPv6.

Reported-by: Daniel Alvarez Sanchez 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html
Signed-off-by: Numan Siddique 
---
 lib/flow.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

v1 -> v2

Addressed review comments - Reset the ct_orig_tuple if dl_type is not
zero and other than IPv4 or IPv6

diff --git a/lib/flow.h b/lib/flow.h
index 6ae5a674d..b3128dab3 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -929,7 +929,7 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const 
struct flow *flow)
 md->ct_label = flow->ct_label;
 
 md->ct_orig_tuple_ipv6 = false;
-if (is_ct_valid(flow, NULL, NULL)) {
+if (flow->dl_type && is_ct_valid(flow, NULL, NULL)) {
 if (flow->dl_type == htons(ETH_TYPE_IP)) {
 md->ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
 flow->ct_nw_src,
@@ -947,6 +947,9 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const 
struct flow *flow)
 flow->ct_tp_dst,
 flow->ct_nw_proto,
 };
+} else {
+/* Reset ct_orig_tuple for other types. */
+memset(>ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
 }
 } else {
 memset(>ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
-- 
2.13.5

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-10-25 Thread Eric Garver 
On Tue, Oct 24, 2017 at 02:43:31PM -0700, William Tu wrote:

Hi William,
Thanks for taking a look at this.

> When using the out-of-tree (openvswitch compat) geneve module,
> the first time oot tunnel probing returns true (correct).
> Without unloading the geneve module, if the userspace ovs-vswitchd
> restarts, because the 'geneve_sys_6081' still exists, the probing
> incorrectly returns false and loads the in-tree (upstream kernel)
> geneve module.

The reason for this is because rtnl sees the existing device and tries
to call ->changelink, but since the out-of-tree tunnel has no handler it
returns EOPNOTSUPP.

> The patch fixes it by adding NLM_F_EXCL flags, so if geneve already
> exists, the dpif_netlink_rtnl_create return EEXIST and the oot tunnel
> probing returns true.  To reproduce the issue, start the ovs

While this fixes this scenario, it breaks another; an existing device
in-tree/kernel geneve tunnel. We'll end up with the opposite occurring -
it returns true when it should return false.

So in addition to adding NLM_F_EXCL, this needs to also try to delete
the existing device in the case of EEXISTS, then restart the probe.

> > /etc/init.d/openvswitch-switch start
> > creat a bridge and attach a geneve port using out-of-tree geneve
> > /etc/init.d/openvswitch-switch restart
> 
> Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides 
> used interface")
> Signed-off-by: William Tu 
> Cc: Eric Garver 
> Cc: Gurucharan Shetty 
> ---
>  lib/dpif-netlink-rtnl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> index 0c32e7d8ccb4..ab9737b8cc4b 100644
> --- a/lib/dpif-netlink-rtnl.c
> +++ b/lib/dpif-netlink-rtnl.c
> @@ -451,7 +451,7 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>  error = dpif_netlink_rtnl_create(tnl_cfg, name, 
> OVS_VPORT_TYPE_GENEVE,
>   "ovs_geneve",
>   (NLM_F_REQUEST | NLM_F_ACK
> -  | NLM_F_CREATE));
> +  | NLM_F_CREATE | NLM_F_EXCL));
>  if (error != EOPNOTSUPP) {
>  if (!error) {
>  dpif_netlink_rtnl_destroy(name);
> -- 
> 2.7.4
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Openvswitch crash when bringing down the dpdk bond port using "ovs-ofctl mod-port br-prv dpdk1 down"

2017-10-25 Thread Ben Pfaff 
I'm going through old email and patches and trying to figure out whether
we're missing something here.  Ilya, do you want this patch considered
for master?  If so, would you mind sending it formally in the customary
way?

Thanks,

Ben.

On Tue, Aug 01, 2017 at 05:19:32PM +0300, Ilya Maximets wrote:
> On 01.08.2017 15:46, Keshav Gupta wrote:
> > Hi Ben/Ilya
> > Mainly patch do the following
> > 1) Call the dpdk rx api(rte_eth_rx_burst) only when ovs netdev state is UP
> > 2)  Now the rte_eth_dev_stop/ rte_eth_dev_start calls are removed in 
> > updating the flags   
> > 
> > 
> > This  fixes the core dump issue but I see below side effect with this patch:
> > With this change now only ports admin state(ovs netdev level) is changed 
> > (dpdk/nic level state is not changed) so NIC will continue to receive the 
> > packet until its Rxq ring is full
> > Later if somebody bring up the port then he will get the burst of old 
> > packets which are there in the Rxq (typically 4k packet (dpdk Rxq size)).
> > 
> > 
> > Thanks
> > Keshav
> >
> 
> Hi Keshav,
> 
> Yes, you're right. I think it's the same situation right now can be observed 
> on
> current master. We can try to fix this for the HW NICs in the similar way as 
> we
> handle destroy_device event for vhost-user ports. I prepared quick fix (not 
> even
> compile tested) for the reference how this can be fixed. Please check the code
> below. One thing is that we actually can't fix such behaviour for the 
> vhost-user
> because there is no such command to stop the vhost device. But we may try to
> fix it for HW NICs if it's important.
> 
> --8<->8--
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index ea17b97..202678f 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2300,20 +2300,48 @@ netdev_dpdk_update_flags__(struct netdev_dpdk *dev,
> enum netdev_flags *old_flagsp)
>  OVS_REQUIRES(dev->mutex)
>  {
> +enum netdev_flags new_flags = dev->flags;
> +
>  if ((off | on) & ~(NETDEV_UP | NETDEV_PROMISC)) {
>  return EINVAL;
>  }
>  
>  *old_flagsp = dev->flags;
> -dev->flags |= on;
> -dev->flags &= ~off;
> +new_flags |= on;
> +new_flags &= ~off;
>  
> -if (dev->flags == *old_flagsp) {
> +if (new_flags == dev->flags) {
>  return 0;
>  }
>  
>  if (dev->type == DPDK_DEV_ETH) {
> -if (dev->flags & NETDEV_PROMISC) {
> +if (NETDEV_UP & ((*old_flagsp ^ on) | (*old_flagsp ^ off))) {
> +if (NETDEV_UP & off) {
> +bool quiescent = ovsrcu_is_quiescent();
> +
> +dev->flags = new_flags;
> +/* Wait for other threads to quiesce after turning off the
> + * 'NETDEV_UP' before actual stopping the device to be sure
> + * that all threads finished their rx/tx operations. */
> +ovsrcu_synchronize();
> +if (quiescent) {
> +/* As call to ovsrcu_synchronize() will end the quiescent
> + * state, put thread back into quiescent state. */
> +ovsrcu_quiesce_start();
> +}
> +rte_eth_dev_stop(dev->port_id);
> +}  else {
> +int retval = rte_eth_dev_start(dev->port_id);
> +
> +if (retval) {
> +VLOG_ERR("Interface %s start error: %s", dev->up.name,
> + rte_strerror(-diag));
> +return -retval;
> +}
> +}
> +}
> +
> +if (new_flags & NETDEV_PROMISC) {
>  rte_eth_promiscuous_enable(dev->port_id);
>  }
>  
> @@ -2336,6 +2364,7 @@ netdev_dpdk_update_flags__(struct netdev_dpdk *dev,
>  }
>  }
>  
> +dev->flags = new_flags;
>  return 0;
>  }
>  
> --8<->8--
> 
> Also, the diff above made on top of current master. So, you possibly will
> need to port it a little.
> 
> Best regards, Ilya Maximets.
> 
> > 
> > -Original Message-
> > From: Ben Pfaff [mailto:b...@ovn.org] 
> > Sent: Monday, July 31, 2017 9:26 PM
> > To: Ilya Maximets
> > Cc: ovs-dev@openvswitch.org; Keshav Gupta
> > Subject: Re: [ovs-dev] Openvswitch crash when bringing down the dpdk bond 
> > port using "ovs-ofctl mod-port br-prv dpdk1 down"
> > 
> > Thanks.
> > 
> > It doesn't cherry-pick cleanly to branch-2.6, so I think I'll leave it for 
> > now since Darrell made the reasonable point that DPDK was experimental in 
> > 2.6.
> > 
> > On Mon, Jul 31, 2017 at 06:20:28PM +0300, Ilya Maximets wrote:
> >> On 31.07.2017 16:05, Ben Pfaff wrote:
> >>> Ilya, should we apply this patch to branch-2.6?  Are there other 
> >>> patches that should be backported?
> >>
> >> It's definitely a bug and should be backported if someone wants to use 
> >> barnch-2.6 with

Re: [ovs-dev] [PATCH v4 5/7] timeval: Introduce time_usec().

2017-10-25 Thread Ben Pfaff 
On Fri, Oct 13, 2017 at 01:03:18PM +, Bodireddy, Bhanuprakash wrote:
> >This fanction will provide monotonic time in microseconds.
> 
> [BHANU] Typo here with function.
> 
> >
> >Signed-off-by: Ilya Maximets 
> >---
> > lib/timeval.c | 22 ++  lib/timeval.h |  2 ++
> > 2 files changed, 24 insertions(+)
> >
> >diff --git a/lib/timeval.c b/lib/timeval.c index dd63f03..be2eddc 100644
> >--- a/lib/timeval.c
> >+++ b/lib/timeval.c
> >@@ -233,6 +233,22 @@ time_wall_msec(void)
> > return time_msec__(_clock);
> > }
> >
> >+static long long int
> >+time_usec__(struct clock *c)
> >+{
> >+struct timespec ts;
> >+
> >+time_timespec__(c, );
> >+return timespec_to_usec();
> >+}
> >+
> >+/* Returns a monotonic timer, in microseconds. */ long long int
> >+time_usec(void)
> >+{
> >+return time_usec__(_clock); }
> >+
> 
> [BHANU]  As you are introducing the support for microsecond granularity, can 
> you also add time_wall_usec() and time_wall_usec__() here?
> The ipfix code (ipfix_now()) can be the first one to use it for now. May be 
> more in the future! 
> 
> > /* Configures the program to die with SIGALRM 'secs' seconds from now, if
> >  * 'secs' is nonzero, or disables the feature if 'secs' is zero. */  void 
> > @@ -360,6
> >+376,12 @@ timeval_to_msec(const struct timeval *tv)
> > return (long long int) tv->tv_sec * 1000 + tv->tv_usec / 1000;  }
> >
> >+long long int
> >+timespec_to_usec(const struct timespec *ts) {
> >+return (long long int) ts->tv_sec * 1000 * 1000 + ts->tv_nsec /
> >+1000; }
> >+
> 
> [BHANU] how about adding timeval_to_usec()?
> Also it would be nice to have the usec_to_timespec() and  timeval_diff_usec() 
> implemented to make this commit complete.

I'd appreciate those changes too; with those changes, I'd be happy to
apply this, independent of anything else in the series.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

2017-10-25 Thread Ben Pfaff 
On Wed, Oct 25, 2017 at 06:50:29PM +0530, Numan Siddique wrote:
> On Wed, Oct 25, 2017 at 3:09 PM, Daniel Alvarez Sanchez  > wrote:
> 
> >
> >
> > On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff  wrote:
> >
> >> On Tue, Oct 24, 2017 at 02:27:59PM -0700, Ben Pfaff wrote:
> >> > On Tue, Oct 24, 2017 at 11:07:58PM +0200, Daniel Alvarez Sanchez wrote:
> >> > > Hi guys,
> >> > >
> >> > > Great job Numan!
> >> > > As we discussed over IRC, the patch below may make more sense.
> >> > > It essentially sets the dl_type so that when packet comes from the
> >> > > controller, it matches
> >> > > a valid type and OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 is not added.
> >> > > Maybe what Numan proposed and this patch could be a good combination
> >> but I
> >> > > think
> >> > > that we definitely need to set the dl_type as it's later checked in
> >> > > pkt_metadata_from_flow()
> >> > > and it'll be zero there otherwise.
> >> > > What do you guys think? I have tried the patch below and the kernel
> >> error
> >> > > is not shown
> >> > > anymore when issuing DHCP requests.
> >> > >
> >> > >
> >> > > diff --git a/lib/flow.c b/lib/flow.c
> >> > > index b2b10aa..62b948f 100644
> >> > > --- a/lib/flow.c
> >> > > +++ b/lib/flow.c
> >> > > @@ -1073,6 +1073,7 @@ flow_get_metadata(const struct flow *flow,
> >> struct
> >> > > match *flow_metadata)
> >> > >
> >> > >  if (flow->ct_state != 0) {
> >> > >  match_set_ct_state(flow_metadata, flow->ct_state);
> >> > > +match_set_dl_type(flow_metadata, flow->dl_type);
> >> > >  if (is_ct_valid(flow, NULL, NULL) && flow->ct_nw_proto != 0)
> >> {
> >> > >  if (flow->dl_type == htons(ETH_TYPE_IP)) {
> >> > >  match_set_ct_nw_src(flow_metadata, flow->ct_nw_src);
> >> >
> >> > This patch seems reasonable too.
> >> >
> >> > I recommend adding a comment above it to explain what's going on,
> >> > because dl_type is not a metadata field and it's confusing to deal with
> >> > it in a context that's supposed to be all about metadata.  I guess the
> >> > comment would essentially say that dl_type is essential to the
> >> > interpretation of the conntrack metadata.
> >>
> >> Oh, and for this patch too I'd welcome a formal patch proposal.
> >>
> >
> > Done. Thanks a lot Ben.
> > If this get merged, it would be great if we can get it into 2.8 branch and
> > add a new tag (2.8.2).
> >
> > Thanks!!
> >
> 
> Ben, we have submitted both the patches. The patch from Daniel - (
> https://patchwork.ozlabs.org/patch/830160/)  will fix the issue.
> Not sure if this patch  https://patchwork.ozlabs.org/patch/830132/ is
> needed any more.
> 
> Request to review these patches if possible as RDO is blocked on these
> patches before we can  update from OVS 2.7.2 to OVS 2.8(.2)

I've reviewed both.  I wasn't able to immediately apply either one, but
they're obviously moving in the right direction, so I'd appreciate
follow-up from both of you so that we can get them in.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Check flow's dl_type before setting ct_orig_tuple in 'pkt_metadata_from_flow()'

2017-10-25 Thread Ben Pfaff 
On Wed, Oct 25, 2017 at 01:44:23PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique 
> 
> Normally flow's dl_type will be a valid value. However when a packet is sent 
> to
> the controller, dl_type is not stored in the 'ofputil_packet_in_private'. When
> the controller resumes (OFPRAW_NXT_RESUME) the packet, the flow's dl_type 
> will be
> 0. If the flow's ct_state has valid value, then the 'pkt_metadata_from_flow'
> neither sets the ct_orig_tuple from the flow nor resets it. This results in 
> invalid
> value ct_orig_tuple in the pkt_metadata.
> 
> This patch handles this situation by checking the dl_type before setting the
> ct_orig_tuple. If dl_type is 0, it resets it.
> 
> Reported-by: Daniel Alvarez Sanchez 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html
> Signed-off-by: Numan Siddique 
> ---
>  lib/flow.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/flow.h b/lib/flow.h
> index 6ae5a674d..8dd1a09a0 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -929,7 +929,7 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const 
> struct flow *flow)
>  md->ct_label = flow->ct_label;
>  
>  md->ct_orig_tuple_ipv6 = false;
> -if (is_ct_valid(flow, NULL, NULL)) {
> +if (flow->dl_type && is_ct_valid(flow, NULL, NULL)) {
>  if (flow->dl_type == htons(ETH_TYPE_IP)) {
>  md->ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
>  flow->ct_nw_src,

Thanks for the patch.  It builds and passes all the tests.

There is still one case where ct_orig_tuple will not be initialized,
which is the case where dl_type is nonzero but not IPv4 or IPV6; for
example, if it is ARP.  Should we be careful to handle that case also?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Add dl_type to flow metadata for correct interpretation of conntrack metadata

2017-10-25 Thread Ben Pfaff 
On Wed, Oct 25, 2017 at 11:35:52AM +0200, Daniel Alvarez wrote:
> When a packet is sent to the controller, dl_type is not stored in the
> 'ofputil_packet_in_private'. When the packet is resumed, the flow's
> dl_type is 0 and this leads to invalid value in ct_orig_tuple in the
> pkt_metadata.
> 
> This patch adds the dl_type to the metadata so that conntrack
> information can be interpreted correctly when packets are resumed.
> 
> Reported-by: Daniel Alvarez Sanchez 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html
> Signed-off-by: Daniel Alvarez 
> Signed-off-by: Numan Siddique 

Thanks a lot for the patch.  I believe it fixes an important bug.

I might add another paragraph to the commit message to really drive home
what's going, perhaps something like this:

This is a change from the ordinary practice, since flow_get_metadata() is
only supposed to deal with metadata and dl_type is not metadata.  It is
necessary when ct_state is involved, though, because ct_state only applies
in the case of particular Ethertypes (IPv4 and IPv6 currently), so we need
to add it as a kind of prerequisite.  (This isn't ideal; maybe we didn't
think through the ct_state mechanism carefully enough.)

However, this breaks several tests (1211 1213 1215 1216 1217 1218 1219
1220 1221 1223 1226).  Would you mind checking that this makes sense for
those tests and updating them as necessary to pass?

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/3] system-traffic: Add conntrack floating IP test

2017-10-25 Thread Eric Garver 
On Tue, Oct 24, 2017 at 02:10:47PM -0700, William Tu wrote:
> On Tue, Oct 24, 2017 at 1:44 PM, Eric Garver  wrote:
> > On Tue, Oct 24, 2017 at 12:46:46PM -0700, William Tu wrote:
> >> On Fri, Oct 20, 2017 at 11:23 AM, Eric Garver  wrote:
> >> > This test cases uses floating IP (FIP) addresses for each endpoint. If
> >> > the destination is a FIP, the packet will undergo a transformation of
> >> > the form (dst=FIP, src=non-FIP) --> (dst=non-FIP, src=FIP) before
> >> > egress. Otherwise the packet is untouched.
> >> >
> >> > This exercises the ct_clear action in the datapath.
> >> >
> >> > Signed-off-by: Eric Garver 
> >> > ---
> >> >  tests/system-traffic.at | 73 
> >> > +
> >> >  1 file changed, 73 insertions(+)
> >> >
> >> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> >> > index 522eaa615834..7cc1e1e21187 100644
> >> > --- a/tests/system-traffic.at
> >> > +++ b/tests/system-traffic.at
> >> > @@ -3996,6 +3996,79 @@ ovs-ofctl -O OpenFlow15 dump-group-stats br0
> >> >  OVS_TRAFFIC_VSWITCHD_STOP
> >> >  AT_CLEANUP
> >> >
> >> > +AT_SETUP([conntrack - floating IP])
> >> > +AT_SKIP_IF([test $HAVE_NC = no])
> >> > +CHECK_CONNTRACK()
> >> > +OVS_TRAFFIC_VSWITCHD_START()
> >> > +OVS_CHECK_CT_CLEAR()
> >> > +
> >> > +ADD_NAMESPACES(at_ns0, at_ns1)
> >> > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") dnl FIP 
> >> > 10.254.254.1
> >> > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") dnl FIP 
> >> > 10.254.254.2
> >> > +
> >> > +dnl Static ARPs
> >> > +NS_CHECK_EXEC([at_ns0], [ip neigh add 10.1.1.2 lladdr f0:00:00:01:01:02 
> >> > dev p0])
> >> > +NS_CHECK_EXEC([at_ns1], [ip neigh add 10.1.1.1 lladdr f0:00:00:01:01:01 
> >> > dev p1])
> >> > +
> >> > +dnl Static ARP and route entries for the FIP "gateway"
> >> > +NS_CHECK_EXEC([at_ns0], [ip neigh add 10.1.1.254 lladdr 
> >> > f0:00:00:01:01:FE dev p0])
> >> > +NS_CHECK_EXEC([at_ns1], [ip neigh add 10.1.1.254 lladdr 
> >> > f0:00:00:01:01:FE dev p1])
> >> > +NS_CHECK_EXEC([at_ns0], [ip route add default nexthop via 10.1.1.254])
> >> > +NS_CHECK_EXEC([at_ns1], [ip route add default nexthop via 10.1.1.254])
> >> > +
> >> > +NETNS_DAEMONIZE([at_ns0], [nc -l -k > /dev/null], [nc0.pid])
> >> > +
> >> > +AT_DATA([flows.txt], [dnl
> >> > +table=0,priority=10  ip action=ct(table=1)
> >> > +table=0,priority=1   action=drop
> >> > +dnl dst FIP
> >> > +table=1,priority=20  ip,ct_state=+trk+est,nw_dst=10.254.254.0/24 
> >> > action=goto_table:10
> >> > +table=1,priority=20  ip,ct_state=+trk+new,nw_dst=10.254.254.0/24 
> >> > action=ct(commit,table=10)
> >> > +dnl dst local
> >> > +table=1,priority=10  ip,ct_state=+trk+est action=goto_table:20
> >> > +table=1,priority=10  ip,ct_state=+trk+new action=ct(commit,table=20)
> >> > +table=1,priority=1   ip,ct_state=+trk+inv action=drop
> >> > +dnl
> >> > +dnl FIP translation (dst FIP, src local) --> (dst local, src FIP)
> >> > +table=10 ip,nw_dst=10.254.254.1 
> >> > action=set_field:10.1.1.1->nw_dst,goto_table:11
> >> > +table=10 ip,nw_dst=10.254.254.2 
> >> > action=set_field:10.1.1.2->nw_dst,goto_table:11
> >> > +table=11 ip,nw_src=10.1.1.1 
> >> > action=set_field:10.254.254.1->nw_src,goto_table:12
> >> > +table=11 ip,nw_src=10.1.1.2 
> >> > action=set_field:10.254.254.2->nw_src,goto_table:12
> >> > +dnl clear conntrack and do another lookup since we changed the tuple
> >> > +table=12,priority=10 ip action=ct_clear,ct(table=13)
> >> > +table=12,priority=1  action=drop
> >> > +table=13 ip,ct_state=+trk+est action=goto_table:20
> >> > +table=13 ip,ct_state=+trk+new action=ct(commit,table=20)
> >> > +table=13 ip,ct_state=+trk+inv action=drop
> >> > +dnl
> >> > +dnl Output
> >> > +table=20 ip,nw_src=10.1.1.1 
> >> > action=set_field:f0:00:00:01:01:01->eth_src,goto_table:21
> >> > +table=20 ip,nw_src=10.1.1.2 
> >> > action=set_field:f0:00:00:01:01:02->eth_src,goto_table:21
> >> > +table=20 ip,nw_src=10.254.254.0/24 
> >> > action=set_field:f0:00:00:01:01:FE->eth_src,goto_table:21
> >> > +table=21 ip,nw_dst=10.1.1.1 
> >> > action=set_field:f0:00:00:01:01:01->eth_dst,output:ovs-p0
> >> > +table=21 ip,nw_dst=10.1.1.2 
> >> > action=set_field:f0:00:00:01:01:02->eth_dst,output:ovs-p1
> >> > +])
> >> > +
> >> > +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> >> > +
> >> > +dnl non-FIP case
> >> > +NS_CHECK_EXEC([at_ns1], [echo "foobar" |nc $NC_EOF_OPT 10.1.1.1])
> >> > +OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
> >> > 's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' |
> >> > +grep 
> >> > "tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),reply=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),protoinfo=(state=TIME_WAIT)"
> >> > +]])
> >> > +
> >> > +dnl Check that the full session ends as expected (i.e. TIME_WAIT). 
> >> > Otherwise it
>

Re: [ovs-dev] [ovs-discuss] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

2017-10-25 Thread Numan Siddique 
On Wed, Oct 25, 2017 at 3:09 PM, Daniel Alvarez Sanchez  wrote:

>
>
> On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff  wrote:
>
>> On Tue, Oct 24, 2017 at 02:27:59PM -0700, Ben Pfaff wrote:
>> > On Tue, Oct 24, 2017 at 11:07:58PM +0200, Daniel Alvarez Sanchez wrote:
>> > > Hi guys,
>> > >
>> > > Great job Numan!
>> > > As we discussed over IRC, the patch below may make more sense.
>> > > It essentially sets the dl_type so that when packet comes from the
>> > > controller, it matches
>> > > a valid type and OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 is not added.
>> > > Maybe what Numan proposed and this patch could be a good combination
>> but I
>> > > think
>> > > that we definitely need to set the dl_type as it's later checked in
>> > > pkt_metadata_from_flow()
>> > > and it'll be zero there otherwise.
>> > > What do you guys think? I have tried the patch below and the kernel
>> error
>> > > is not shown
>> > > anymore when issuing DHCP requests.
>> > >
>> > >
>> > > diff --git a/lib/flow.c b/lib/flow.c
>> > > index b2b10aa..62b948f 100644
>> > > --- a/lib/flow.c
>> > > +++ b/lib/flow.c
>> > > @@ -1073,6 +1073,7 @@ flow_get_metadata(const struct flow *flow,
>> struct
>> > > match *flow_metadata)
>> > >
>> > >  if (flow->ct_state != 0) {
>> > >  match_set_ct_state(flow_metadata, flow->ct_state);
>> > > +match_set_dl_type(flow_metadata, flow->dl_type);
>> > >  if (is_ct_valid(flow, NULL, NULL) && flow->ct_nw_proto != 0)
>> {
>> > >  if (flow->dl_type == htons(ETH_TYPE_IP)) {
>> > >  match_set_ct_nw_src(flow_metadata, flow->ct_nw_src);
>> >
>> > This patch seems reasonable too.
>> >
>> > I recommend adding a comment above it to explain what's going on,
>> > because dl_type is not a metadata field and it's confusing to deal with
>> > it in a context that's supposed to be all about metadata.  I guess the
>> > comment would essentially say that dl_type is essential to the
>> > interpretation of the conntrack metadata.
>>
>> Oh, and for this patch too I'd welcome a formal patch proposal.
>>
>
> Done. Thanks a lot Ben.
> If this get merged, it would be great if we can get it into 2.8 branch and
> add a new tag (2.8.2).
>
> Thanks!!
>

Ben, we have submitted both the patches. The patch from Daniel - (
https://patchwork.ozlabs.org/patch/830160/)  will fix the issue.
Not sure if this patch  https://patchwork.ozlabs.org/patch/830132/ is
needed any more.

Request to review these patches if possible as RDO is blocked on these
patches before we can  update from OVS 2.7.2 to OVS 2.8(.2)

Thanks
Numan



Daniel
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action

2017-10-25 Thread Roi Dayan 



On 27/09/2017 12:08, Simon Horman wrote:

On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote:



On 18/09/2017 18:01, Simon Horman wrote:

On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote:

From: Paul Blakey 

To be later used to implement ovs action set offloading.

Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
---
  lib/tc.c | 372 ++-
  lib/tc.h |  16 +++
  2 files changed, 385 insertions(+), 3 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index c9cada2..743b2ee 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -21,8 +21,10 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -33,11 +35,14 @@
  #include "netlink-socket.h"
  #include "netlink.h"
  #include "openvswitch/ofpbuf.h"
+#include "openvswitch/util.h"
  #include "openvswitch/vlog.h"
  #include "packets.h"
  #include "timeval.h"
  #include "unaligned.h"
+#define MAX_PEDIT_OFFSETS 8


Why 8?

We don't expect anything more right now (ipv6 src/dst rewrite requires 8
pedits iirc). I can't think of a larger use case, maybe ipv6 + macs if
that's makes sens. do you suggest we increase it? to what?


It seems strange to me to place a somewhat arbitrary small limit
when none exists in the pedit API being used. I would at prefer if
it was at least a bigger, say 16 or 32.



Hi Simon,

Sorry for the late reply due to holidays and vacations.
Me & Paul going to go over this and do the fixes needed and
also rebase over latest master and run tests again.

I'll answer what I'm more familiar with now and Paul will continue.
The 8 here is too low and you right. We used this definition
for allocation of the pedit keys on the stack in
nl_msg_put_flower_rewrite_pedits()

It was for convenience instead of calculating the maximum possible
keys that could exists and allocating it there and freeing it at
the end.

Increasing it to 32 is probably more than enough and wont waste much.





  VLOG_DEFINE_THIS_MODULE(tc);
  static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(60, 5);
@@ -50,6 +55,82 @@ enum tc_offload_policy {
  static enum tc_offload_policy tc_policy = TC_POLICY_NONE;
+struct tc_pedit_key_ex {
+enum pedit_header_type htype;
+enum pedit_cmd cmd;
+};
+
+struct flower_key_to_pedit {
+enum pedit_header_type htype;
+int flower_offset;
+int offset;
+int size;
+};
+
+static struct flower_key_to_pedit flower_pedit_map[] = {
+{
+TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
+12,
+offsetof(struct tc_flower_key, ipv4.ipv4_src),
+MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_src)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
+16,
+offsetof(struct tc_flower_key, ipv4.ipv4_dst),
+MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_dst)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
+8,
+offsetof(struct tc_flower_key, ipv4.rewrite_ttl),
+MEMBER_SIZEOF(struct tc_flower_key, ipv4.rewrite_ttl)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_IP6,
+8,
+offsetof(struct tc_flower_key, ipv6.ipv6_src),
+MEMBER_SIZEOF(struct tc_flower_key, ipv6.ipv6_src)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_IP6,
+24,
+offsetof(struct tc_flower_key, ipv6.ipv6_dst),
+MEMBER_SIZEOF(struct tc_flower_key, ipv6.ipv6_dst)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
+6,
+offsetof(struct tc_flower_key, src_mac),
+MEMBER_SIZEOF(struct tc_flower_key, src_mac)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
+0,
+offsetof(struct tc_flower_key, dst_mac),
+MEMBER_SIZEOF(struct tc_flower_key, dst_mac)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
+12,
+offsetof(struct tc_flower_key, eth_type),
+MEMBER_SIZEOF(struct tc_flower_key, eth_type)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_TCP,
+0,
+offsetof(struct tc_flower_key, tcp_src),
+MEMBER_SIZEOF(struct tc_flower_key, tcp_src)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_TCP,
+2,
+offsetof(struct tc_flower_key, tcp_dst),
+MEMBER_SIZEOF(struct tc_flower_key, tcp_dst)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_UDP,
+0,
+offsetof(struct tc_flower_key, udp_src),
+MEMBER_SIZEOF(struct tc_flower_key, udp_src)
+}, {
+TCA_PEDIT_KEY_EX_HDR_TYPE_UDP,
+2,
+offsetof(struct tc_flower_key, udp_dst),
+MEMBER_SIZEOF(struct tc_flower_key, udp_dst)
+},
+};
+
  struct tcmsg *
  tc_make_request(int ifindex, int type, unsigned int flags,
  struct ofpbuf *request)
@@ -365,6 +446,96 @@ nl_parse_flower_ip(struct nlattr **attrs, struct tc_flower 
*flower) {
  }
  }
+static const struct nl_policy pedit_policy[] = {
+[TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC,
+

Re: [ovs-dev] [PATCH v3 0/2] EMC management fixes.

2017-10-25 Thread Ilya Maximets 
Any other thoughts about this patch-set?
It's in kind of 'undecided' state for a long time.

Best regards, Ilya Maximets.

On 04.08.2017 17:17, Ilya Maximets wrote:
> Version 3:
>   * Added comment to EM_FLOW_INSERT_INV_PROB_SHIFT.
> 
> Ilya Maximets (2):
>   dpif-netdev: Decrease range of values for EMC probability.
>   dpif-netdev: Fix emc replacement policy.
> 
>  lib/dpif-netdev.c| 35 +--
>  vswitchd/vswitch.xml |  3 ++-
>  2 files changed, 27 insertions(+), 11 deletions(-)
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Hi

2017-10-25 Thread Sargt christina 


Hey,
I'm Sargent Christina Hammock , I was born in the year 19/08/1981 - 36yrs old 
female soldier in the American Military , i have very important thing to share 
with you privately .


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

2017-10-25 Thread Daniel Alvarez Sanchez 
On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff  wrote:

> On Tue, Oct 24, 2017 at 02:27:59PM -0700, Ben Pfaff wrote:
> > On Tue, Oct 24, 2017 at 11:07:58PM +0200, Daniel Alvarez Sanchez wrote:
> > > Hi guys,
> > >
> > > Great job Numan!
> > > As we discussed over IRC, the patch below may make more sense.
> > > It essentially sets the dl_type so that when packet comes from the
> > > controller, it matches
> > > a valid type and OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 is not added.
> > > Maybe what Numan proposed and this patch could be a good combination
> but I
> > > think
> > > that we definitely need to set the dl_type as it's later checked in
> > > pkt_metadata_from_flow()
> > > and it'll be zero there otherwise.
> > > What do you guys think? I have tried the patch below and the kernel
> error
> > > is not shown
> > > anymore when issuing DHCP requests.
> > >
> > >
> > > diff --git a/lib/flow.c b/lib/flow.c
> > > index b2b10aa..62b948f 100644
> > > --- a/lib/flow.c
> > > +++ b/lib/flow.c
> > > @@ -1073,6 +1073,7 @@ flow_get_metadata(const struct flow *flow, struct
> > > match *flow_metadata)
> > >
> > >  if (flow->ct_state != 0) {
> > >  match_set_ct_state(flow_metadata, flow->ct_state);
> > > +match_set_dl_type(flow_metadata, flow->dl_type);
> > >  if (is_ct_valid(flow, NULL, NULL) && flow->ct_nw_proto != 0) {
> > >  if (flow->dl_type == htons(ETH_TYPE_IP)) {
> > >  match_set_ct_nw_src(flow_metadata, flow->ct_nw_src);
> >
> > This patch seems reasonable too.
> >
> > I recommend adding a comment above it to explain what's going on,
> > because dl_type is not a metadata field and it's confusing to deal with
> > it in a context that's supposed to be all about metadata.  I guess the
> > comment would essentially say that dl_type is essential to the
> > interpretation of the conntrack metadata.
>
> Oh, and for this patch too I'd welcome a formal patch proposal.
>

Done. Thanks a lot Ben.
If this get merged, it would be great if we can get it into 2.8 branch and
add a new tag (2.8.2).

Thanks!!
Daniel
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] Add dl_type to flow metadata for correct interpretation of conntrack metadata

2017-10-25 Thread Daniel Alvarez 
When a packet is sent to the controller, dl_type is not stored in the
'ofputil_packet_in_private'. When the packet is resumed, the flow's
dl_type is 0 and this leads to invalid value in ct_orig_tuple in the
pkt_metadata.

This patch adds the dl_type to the metadata so that conntrack
information can be interpreted correctly when packets are resumed.

Reported-by: Daniel Alvarez Sanchez 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html
Signed-off-by: Daniel Alvarez 
Signed-off-by: Numan Siddique 
---
 lib/flow.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/flow.c b/lib/flow.c
index b2b10aa48..4d2b7747a 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1073,6 +1073,9 @@ flow_get_metadata(const struct flow *flow, struct match 
*flow_metadata)
 
 if (flow->ct_state != 0) {
 match_set_ct_state(flow_metadata, flow->ct_state);
+/* Match dl_type since it is required for the later interpretation of
+ * the conntrack metadata. */
+match_set_dl_type(flow_metadata, flow->dl_type);
 if (is_ct_valid(flow, NULL, NULL) && flow->ct_nw_proto != 0) {
 if (flow->dl_type == htons(ETH_TYPE_IP)) {
 match_set_ct_nw_src(flow_metadata, flow->ct_nw_src);
-- 
2.13.5

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Populating MIB

2017-10-25 Thread Shreyas Shetty 
Hi,
So I am trying to implement SNMP for OVS mainly for topology discovery and
port statistics.
I was wondering whether this

approach will work here
If it can, how can I go about populating the mapping as mentioned there?
If not, does someone have an idea how I can go about this?
Thank you,
Shreyas
P.S. : I am still learning both OVS and SNMP.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Embroidered patches manufacturer from China

2017-10-25 Thread br...@evergreenbadges.com 






Greetings!

EverGreen Badges supplies custom patches to some of the most recognized 
organizations in the world; our customers include Australia Defence, NBA, Royal 
Canadian Mounted Police. We have built a reputation for consistently providing 
exceptional craftsmanship with both standard and custom patches.

Join EVERGREENBADGES.COM now and start managing your badge requirements at a 
click of a mouse and I'm anticipating the pleasure of hearing from you in the 
near future.

PS: our primary product range includes: embroidered patches, chenille 
patches, sublimation printing patches and epaulettes.

Faithfully,

Bruce
Sales Manager

EVERGREEN BADGES
Ph: +86-769-87036875
Cell: 
+86-15920615551


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] Check flow's dl_type before setting ct_orig_tuple in 'pkt_metadata_from_flow()'

2017-10-25 Thread nusiddiq 
From: Numan Siddique 

Normally flow's dl_type will be a valid value. However when a packet is sent to
the controller, dl_type is not stored in the 'ofputil_packet_in_private'. When
the controller resumes (OFPRAW_NXT_RESUME) the packet, the flow's dl_type will 
be
0. If the flow's ct_state has valid value, then the 'pkt_metadata_from_flow'
neither sets the ct_orig_tuple from the flow nor resets it. This results in 
invalid
value ct_orig_tuple in the pkt_metadata.

This patch handles this situation by checking the dl_type before setting the
ct_orig_tuple. If dl_type is 0, it resets it.

Reported-by: Daniel Alvarez Sanchez 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339868.html
Signed-off-by: Numan Siddique 
---
 lib/flow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/flow.h b/lib/flow.h
index 6ae5a674d..8dd1a09a0 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -929,7 +929,7 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const 
struct flow *flow)
 md->ct_label = flow->ct_label;
 
 md->ct_orig_tuple_ipv6 = false;
-if (is_ct_valid(flow, NULL, NULL)) {
+if (flow->dl_type && is_ct_valid(flow, NULL, NULL)) {
 if (flow->dl_type == htons(ETH_TYPE_IP)) {
 md->ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) {
 flow->ct_nw_src,
-- 
2.13.5

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev