[ovs-dev] Gentile utente.
Gentile utente La tua email è superato 2 GB, che viene creato dal webmaster, attualmente in esecuzione a 2.30GB, non è possibile inviare o ricevere nuovi messaggi fino a controllare il vostro account. Compila il modulo sottostante per verificare il tuo account. Compila il modulo per confermare il tuo account (1) E-mail: (2) Il nome: (3) Password: (4) Conferma Password: grazie Amministratore di sistem ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3 07/16] ofproto-dpif: Bonding with in_port that isn't present in the datapath
Simon, Thanks for explaining. It is now clear what the test is about. I spent some time today to study this. The root cause seems to be in how packet out are implemented, not with recirculation or bond. A reasonable solution may be treating patch ports as LOCAL ports for packet out. I have prototyped this solution and passed the test. Any thoughts? I will clean it up and send it out soon. Andy On Tue, Apr 29, 2014 at 1:55 AM, Simon Horman wrote: > On Tue, Apr 29, 2014 at 12:40:18AM -0700, Andy Zhou wrote: >> If p7 and p37 are now connected together as patch ports, would any >> traffic injected from p7 bypass (in the user space) the bond >> interfaces all-together? > > I believe what happens is that the normal action forwards > the packet to all ports that are connected to br0 to other than p7. > That is the packet is forwarded to bond0. > > Without this patch, the packet is then executed in the datapath > and an upcall is made as a result of a recirculation action. > In handle_upcalls() the call to xlate_receive() fails because > the in_port does not exist in the datapath. > > > To be honest I'm not particularly concerned if patch-ports work in > conjunction with bonding or recirculation in general. And indeed they don't > even with this series applied. The purpose of this patch was to illustrate > the problem that without other patches in this series recirculation doesn't > work in cases where the in_port doesn't exist in the datapath. And in the > case of bonding the only example I could come up with involved a > patch-port. > > In the case of recirculation for MPLS, which is also added by this series, > a more realistic example can be constructed using packet_out with > CONTROLLER as the in_port. This case I am inclined to care about. > Particularly as it is used extensively by Ryu's test-suite which can be run > using Open vSwitch's make check-ryu target. > > I did not use CONTROLLER as the in_port for the bonding test as for reasons > I don't fully understand packets with the CONTROLLER as the in_port do not > seem to be able to be handled by a normal action. And it it my > understanding that a normal action is needed to forward packets to a bond. > >> >> On Sun, Apr 27, 2014 at 6:24 PM, Simon Horman wrote: >> > On Fri, Apr 25, 2014 at 12:46:16AM -0700, Andy Zhou wrote: >> >> Simon, I thought this test exposed a bug that is fixed with this >> >> patch series. However, when I apply this patch alone against current >> >> master, it passed fine. (I tried many times). So I must >> >> misunderstand the intention of the test, or why current master failed >> >> to address >> >> packet out. Would you please shed some light on this? >> > >> > Hi Andy, >> > >> > thanks for pointing that out. My intention was, as you suggest, >> > to provide a test that fails without previous patches in the series. >> > Unfortunately I managed to create a test that passes regardless >> > of the presence of other patches in the series :( >> > >> > Could you please take a look at the updated test below? >> > It should fail without earlier patches in the series. >> > >> > From: Simon Horman >> > >> > [PATCH] ofproto-dpif: Bonding with in_port that isn't present in the >> > datapath >> > >> > This tests exercises execution of actions in ovs-vswitchd >> > in the case where a packet is processed due to a packet out message >> > with an in_port that doesn't exist in the datapath and translation >> > results in recirc actions due to bonding. >> > >> > Signed-off-by: Simon Horman >> > >> > --- >> > v2 >> > * First Post >> > >> > v2.1 >> > * Use a patch-port instead of NONE as the in_port for packet_out. >> > This has the intended effect of causing a packet with an in_port >> > that does not exist in the datapath to be output to a bonding interface. >> > --- >> > tests/ofproto-dpif.at | 90 >> > +++ >> > 1 file changed, 90 insertions(+) >> > >> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> > index 89c8ad7..2113464 100644 >> > --- a/tests/ofproto-dpif.at >> > +++ b/tests/ofproto-dpif.at >> > @@ -197,6 +197,96 @@ AT_CHECK([test `grep in_port.6 br1_flows.txt |wc -l` >> > -gt 7]) >> > OVS_VSWITCHD_STOP() >> > AT_CLEANUP >> > >> > +AT_SETUP([ofproto-dpif, balance-tcp bonding, packet-out]) >> > +# Create br0 with interfaces bond0(p1, p2, p3), bond1(p4,p5,p6) and p7, >> > and >> > +#br1 with interfaces bond10(p11, p12, p13) and >> > bond11(p14,p15,p16), and >> > +#br2 with interfaces bond20(p21, p22, p23) and bond21(p24,p25,p26) >> > +#br3 with interface p37 >> > +#bond0 <-> bond10 (unix socket backed interfaces) >> > +#bond1 <-> bond20 (unix socket backed interfaces) >> > +#p7 <-> p37 (patch interface) >> > +# Send some traffic, make sure the traffic are spread based on L4 headers. >> > +OVS_VSWITCHD_START( >> > + [add-bond br0 bond0 p1 p2 p3 bond_mode=balance-tcp lacp=active \ >> > +other-config:lacp-tim
Re: [ovs-dev] [PATCH v3 07/16] ofproto-dpif: Bonding with in_port that isn't present in the datapath
On Wed, Apr 30, 2014 at 01:10:02AM -0700, Andy Zhou wrote: > Simon, > > Thanks for explaining. It is now clear what the test is about. > > I spent some time today to study this. The root cause seems to be in > how packet out are > implemented, not with recirculation or bond. A reasonable solution > may be treating patch ports as > LOCAL ports for packet out. I have prototyped this solution and passed > the test. Any thoughts? Will this also handle other ports that are not present in the datapath for packet_out? In particular the CONTROLLER port? > I will clean it up and send it out soon. Thanks. > > Andy > > On Tue, Apr 29, 2014 at 1:55 AM, Simon Horman wrote: > > On Tue, Apr 29, 2014 at 12:40:18AM -0700, Andy Zhou wrote: > >> If p7 and p37 are now connected together as patch ports, would any > >> traffic injected from p7 bypass (in the user space) the bond > >> interfaces all-together? > > > > I believe what happens is that the normal action forwards > > the packet to all ports that are connected to br0 to other than p7. > > That is the packet is forwarded to bond0. > > > > Without this patch, the packet is then executed in the datapath > > and an upcall is made as a result of a recirculation action. > > In handle_upcalls() the call to xlate_receive() fails because > > the in_port does not exist in the datapath. > > > > > > To be honest I'm not particularly concerned if patch-ports work in > > conjunction with bonding or recirculation in general. And indeed they don't > > even with this series applied. The purpose of this patch was to illustrate > > the problem that without other patches in this series recirculation doesn't > > work in cases where the in_port doesn't exist in the datapath. And in the > > case of bonding the only example I could come up with involved a > > patch-port. > > > > In the case of recirculation for MPLS, which is also added by this series, > > a more realistic example can be constructed using packet_out with > > CONTROLLER as the in_port. This case I am inclined to care about. > > Particularly as it is used extensively by Ryu's test-suite which can be run > > using Open vSwitch's make check-ryu target. > > > > I did not use CONTROLLER as the in_port for the bonding test as for reasons > > I don't fully understand packets with the CONTROLLER as the in_port do not > > seem to be able to be handled by a normal action. And it it my > > understanding that a normal action is needed to forward packets to a bond. > > > >> > >> On Sun, Apr 27, 2014 at 6:24 PM, Simon Horman wrote: > >> > On Fri, Apr 25, 2014 at 12:46:16AM -0700, Andy Zhou wrote: > >> >> Simon, I thought this test exposed a bug that is fixed with this > >> >> patch series. However, when I apply this patch alone against current > >> >> master, it passed fine. (I tried many times). So I must > >> >> misunderstand the intention of the test, or why current master failed > >> >> to address > >> >> packet out. Would you please shed some light on this? > >> > > >> > Hi Andy, > >> > > >> > thanks for pointing that out. My intention was, as you suggest, > >> > to provide a test that fails without previous patches in the series. > >> > Unfortunately I managed to create a test that passes regardless > >> > of the presence of other patches in the series :( > >> > > >> > Could you please take a look at the updated test below? > >> > It should fail without earlier patches in the series. > >> > > >> > From: Simon Horman > >> > > >> > [PATCH] ofproto-dpif: Bonding with in_port that isn't present in the > >> > datapath > >> > > >> > This tests exercises execution of actions in ovs-vswitchd > >> > in the case where a packet is processed due to a packet out message > >> > with an in_port that doesn't exist in the datapath and translation > >> > results in recirc actions due to bonding. > >> > > >> > Signed-off-by: Simon Horman > >> > > >> > --- > >> > v2 > >> > * First Post > >> > > >> > v2.1 > >> > * Use a patch-port instead of NONE as the in_port for packet_out. > >> > This has the intended effect of causing a packet with an in_port > >> > that does not exist in the datapath to be output to a bonding > >> > interface. > >> > --- > >> > tests/ofproto-dpif.at | 90 > >> > +++ > >> > 1 file changed, 90 insertions(+) > >> > > >> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > >> > index 89c8ad7..2113464 100644 > >> > --- a/tests/ofproto-dpif.at > >> > +++ b/tests/ofproto-dpif.at > >> > @@ -197,6 +197,96 @@ AT_CHECK([test `grep in_port.6 br1_flows.txt |wc > >> > -l` -gt 7]) > >> > OVS_VSWITCHD_STOP() > >> > AT_CLEANUP > >> > > >> > +AT_SETUP([ofproto-dpif, balance-tcp bonding, packet-out]) > >> > +# Create br0 with interfaces bond0(p1, p2, p3), bond1(p4,p5,p6) and p7, > >> > and > >> > +#br1 with interfaces bond10(p11, p12, p13) and > >> > bond11(p14,p15,p16), and > >> > +#br2 with interfaces bond20(p21, p22, p23) and > >> > bond21(
[ovs-dev] [PATCH] ofproto-dpif: treat patch ports as local port for OFPT_PACKET_OUT
When controller send OFPT_PACKET_OUT message, with its input port set
to a patch port, and the packet handling requires recirculation,
the post recirculation flow is never set up since the xlate layer
rejects the up call triggered by such packet, on the bases those
up calls don't have valid datapath input port. Patch ports don't
have corresponding datapath ports.
A reasonable solution is by injecting the packet into datapath
using LOCAL port's datapath port id. This patch implements
such solution. It works against a test case provided by Simon.
Reported-by: Simon Horman
Signed-off-by: Andy Zhou
---
ofproto/ofproto-dpif.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4cebd77..afc2f21 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -195,6 +195,8 @@ static void vsp_add(struct ofport_dpif *, ofp_port_t
realdev_ofp_port, int vid);
static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
ofp_port_t);
+static odp_port_t packet_out_odp_port(const struct ofproto_dpif *,
+ ofp_port_t);
static ofp_port_t odp_port_to_ofp_port(const struct ofproto_dpif *,
odp_port_t);
@@ -3145,7 +3147,7 @@ ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
execute.md.tunnel = flow->tunnel;
execute.md.skb_priority = flow->skb_priority;
execute.md.pkt_mark = flow->pkt_mark;
-execute.md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port);
+execute.md.in_port.odp_port = packet_out_odp_port(ofproto, in_port);
execute.needs_help = (xout.slow & SLOW_ACTION) != 0;
error = dpif_execute(ofproto->backer->dpif, &execute);
@@ -4745,6 +4747,21 @@ ofp_port_to_odp_port(const struct ofproto_dpif *ofproto,
ofp_port_t ofp_port)
return ofport ? ofport->odp_port : ODPP_NONE;
}
+/* Converts ofp port to odp port, similar to ofp_port_to_odp_port.
+ * This function treats patch ports as LOCAL port, that is safe to send
+ * into the datapatch. */
+static odp_port_t
+packet_out_odp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port)
+{
+const struct ofport_dpif *ofport = get_ofp_port(ofproto, ofp_port);
+if (ofport && netdev_vport_is_patch(ofport->up.netdev)) {
+ofport = get_ofp_port(ofproto, OFPP_LOCAL);
+}
+
+return ofport ? ofport->odp_port : ODPP_NONE;
+}
+
+
struct ofport_dpif *
odp_port_to_ofport(const struct dpif_backer *backer, odp_port_t odp_port)
{
--
1.9.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3 07/16] ofproto-dpif: Bonding with in_port that isn't present in the datapath
On Wed, Apr 30, 2014 at 1:35 AM, Simon Horman wrote: > On Wed, Apr 30, 2014 at 01:10:02AM -0700, Andy Zhou wrote: >> Simon, >> >> Thanks for explaining. It is now clear what the test is about. >> >> I spent some time today to study this. The root cause seems to be in >> how packet out are >> implemented, not with recirculation or bond. A reasonable solution >> may be treating patch ports as >> LOCAL ports for packet out. I have prototyped this solution and passed >> the test. Any thoughts? > > Will this also handle other ports that are not present in the datapath > for packet_out? In particular the CONTROLLER port? This is a good idea, The patch I sent out will only handle patch ports. Let me send a v2 to address CONTROLLER as well. > >> I will clean it up and send it out soon. > > Thanks. > >> >> Andy >> >> On Tue, Apr 29, 2014 at 1:55 AM, Simon Horman wrote: >> > On Tue, Apr 29, 2014 at 12:40:18AM -0700, Andy Zhou wrote: >> >> If p7 and p37 are now connected together as patch ports, would any >> >> traffic injected from p7 bypass (in the user space) the bond >> >> interfaces all-together? >> > >> > I believe what happens is that the normal action forwards >> > the packet to all ports that are connected to br0 to other than p7. >> > That is the packet is forwarded to bond0. >> > >> > Without this patch, the packet is then executed in the datapath >> > and an upcall is made as a result of a recirculation action. >> > In handle_upcalls() the call to xlate_receive() fails because >> > the in_port does not exist in the datapath. >> > >> > >> > To be honest I'm not particularly concerned if patch-ports work in >> > conjunction with bonding or recirculation in general. And indeed they don't >> > even with this series applied. The purpose of this patch was to illustrate >> > the problem that without other patches in this series recirculation doesn't >> > work in cases where the in_port doesn't exist in the datapath. And in the >> > case of bonding the only example I could come up with involved a >> > patch-port. >> > >> > In the case of recirculation for MPLS, which is also added by this series, >> > a more realistic example can be constructed using packet_out with >> > CONTROLLER as the in_port. This case I am inclined to care about. >> > Particularly as it is used extensively by Ryu's test-suite which can be run >> > using Open vSwitch's make check-ryu target. >> > >> > I did not use CONTROLLER as the in_port for the bonding test as for reasons >> > I don't fully understand packets with the CONTROLLER as the in_port do not >> > seem to be able to be handled by a normal action. And it it my >> > understanding that a normal action is needed to forward packets to a bond. >> > >> >> >> >> On Sun, Apr 27, 2014 at 6:24 PM, Simon Horman wrote: >> >> > On Fri, Apr 25, 2014 at 12:46:16AM -0700, Andy Zhou wrote: >> >> >> Simon, I thought this test exposed a bug that is fixed with this >> >> >> patch series. However, when I apply this patch alone against current >> >> >> master, it passed fine. (I tried many times). So I must >> >> >> misunderstand the intention of the test, or why current master failed >> >> >> to address >> >> >> packet out. Would you please shed some light on this? >> >> > >> >> > Hi Andy, >> >> > >> >> > thanks for pointing that out. My intention was, as you suggest, >> >> > to provide a test that fails without previous patches in the series. >> >> > Unfortunately I managed to create a test that passes regardless >> >> > of the presence of other patches in the series :( >> >> > >> >> > Could you please take a look at the updated test below? >> >> > It should fail without earlier patches in the series. >> >> > >> >> > From: Simon Horman >> >> > >> >> > [PATCH] ofproto-dpif: Bonding with in_port that isn't present in the >> >> > datapath >> >> > >> >> > This tests exercises execution of actions in ovs-vswitchd >> >> > in the case where a packet is processed due to a packet out message >> >> > with an in_port that doesn't exist in the datapath and translation >> >> > results in recirc actions due to bonding. >> >> > >> >> > Signed-off-by: Simon Horman >> >> > >> >> > --- >> >> > v2 >> >> > * First Post >> >> > >> >> > v2.1 >> >> > * Use a patch-port instead of NONE as the in_port for packet_out. >> >> > This has the intended effect of causing a packet with an in_port >> >> > that does not exist in the datapath to be output to a bonding >> >> > interface. >> >> > --- >> >> > tests/ofproto-dpif.at | 90 >> >> > +++ >> >> > 1 file changed, 90 insertions(+) >> >> > >> >> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> >> > index 89c8ad7..2113464 100644 >> >> > --- a/tests/ofproto-dpif.at >> >> > +++ b/tests/ofproto-dpif.at >> >> > @@ -197,6 +197,96 @@ AT_CHECK([test `grep in_port.6 br1_flows.txt |wc >> >> > -l` -gt 7]) >> >> > OVS_VSWITCHD_STOP() >> >> > AT_CLEANUP >> >> > >> >> > +AT_SETUP([ofproto-dpif, balance-tcp
[ovs-dev] [PACKET_OUT v2] ofproto-dpif: treat non-datapath ports as local port for OFPT_PACKET_OUT
When controller sends OFPT_PACKET_OUT message with the in_port set
to a patch port or as CONTROLLER, and the message execution requires
recirculation, those packets will be dropped in the datapath.
This is because the post recirculation flow will not be set up by
Xlate layer that rejects up call without a valid datapath input port.
This patch implements a reasonable solution by injecting packets'
in_port does not have a valid datapath port using LOCAL port's
datapath port id.
Reported-by: Simon Horman
Signed-off-by: Andy Zhou
--
v1 -> v2: Also handles CONTROLLER as input port
---
ofproto/ofproto-dpif.c | 32 ++--
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4cebd77..d1b1405 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -193,6 +193,7 @@ struct vlan_splinter {
static void vsp_remove(struct ofport_dpif *);
static void vsp_add(struct ofport_dpif *, ofp_port_t realdev_ofp_port, int
vid);
+static odp_port_t packet_out_odp_port(const struct ofproto_dpif *, ofp_port_t);
static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
ofp_port_t);
@@ -3117,7 +3118,6 @@ ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
struct dpif_flow_stats stats;
struct xlate_out xout;
struct xlate_in xin;
-ofp_port_t in_port;
struct dpif_execute execute;
int error;
@@ -3135,17 +3135,14 @@ ofproto_dpif_execute_actions(struct ofproto_dpif
*ofproto,
xin.resubmit_stats = &stats;
xlate_actions(&xin, &xout);
-in_port = flow->in_port.ofp_port;
-if (in_port == OFPP_NONE) {
-in_port = OFPP_LOCAL;
-}
execute.actions = ofpbuf_data(&xout.odp_actions);
execute.actions_len = ofpbuf_size(&xout.odp_actions);
execute.packet = packet;
execute.md.tunnel = flow->tunnel;
execute.md.skb_priority = flow->skb_priority;
execute.md.pkt_mark = flow->pkt_mark;
-execute.md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port);
+execute.md.in_port.odp_port = packet_out_odp_port(ofproto,
+ flow->in_port.ofp_port);
execute.needs_help = (xout.slow & SLOW_ACTION) != 0;
error = dpif_execute(ofproto->backer->dpif, &execute);
@@ -4745,6 +4742,29 @@ ofp_port_to_odp_port(const struct ofproto_dpif *ofproto,
ofp_port_t ofp_port)
return ofport ? ofport->odp_port : ODPP_NONE;
}
+/* Converts ofp port to odp port, similar to ofp_port_to_odp_port.
+ * This function treats patch ports and CONTROLLER port as LOCAL port.
+ *
+ * In case the packet out execution requires recirculation, the post
+ * recirculation upcall with a valid datapath in_port will not be rejected
+ * by the xlate layer. */
+static odp_port_t
+packet_out_odp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port)
+{
+const struct ofport_dpif *ofport;
+
+if ((ofp_port == OFPP_NONE) || (ofp_port == OFPP_CONTROLLER)) {
+ofp_port = OFPP_LOCAL;
+}
+
+ofport = get_ofp_port(ofproto, ofp_port);
+if (ofport && netdev_vport_is_patch(ofport->up.netdev)) {
+ofport = get_ofp_port(ofproto, OFPP_LOCAL);
+}
+
+return ofport ? ofport->odp_port : ODPP_NONE;
+}
+
struct ofport_dpif *
odp_port_to_ofport(const struct dpif_backer *backer, odp_port_t odp_port)
{
--
1.9.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: treat patch ports as local port for OFPT_PACKET_OUT
please ignore this patch. I've sent a v2 of it.
On Wed, Apr 30, 2014 at 2:09 AM, Andy Zhou wrote:
> When controller send OFPT_PACKET_OUT message, with its input port set
> to a patch port, and the packet handling requires recirculation,
> the post recirculation flow is never set up since the xlate layer
> rejects the up call triggered by such packet, on the bases those
> up calls don't have valid datapath input port. Patch ports don't
> have corresponding datapath ports.
>
> A reasonable solution is by injecting the packet into datapath
> using LOCAL port's datapath port id. This patch implements
> such solution. It works against a test case provided by Simon.
>
> Reported-by: Simon Horman
> Signed-off-by: Andy Zhou
> ---
> ofproto/ofproto-dpif.c | 19 ++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4cebd77..afc2f21 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -195,6 +195,8 @@ static void vsp_add(struct ofport_dpif *, ofp_port_t
> realdev_ofp_port, int vid);
>
> static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
> ofp_port_t);
> +static odp_port_t packet_out_odp_port(const struct ofproto_dpif *,
> + ofp_port_t);
>
> static ofp_port_t odp_port_to_ofp_port(const struct ofproto_dpif *,
> odp_port_t);
> @@ -3145,7 +3147,7 @@ ofproto_dpif_execute_actions(struct ofproto_dpif
> *ofproto,
> execute.md.tunnel = flow->tunnel;
> execute.md.skb_priority = flow->skb_priority;
> execute.md.pkt_mark = flow->pkt_mark;
> -execute.md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port);
> +execute.md.in_port.odp_port = packet_out_odp_port(ofproto, in_port);
> execute.needs_help = (xout.slow & SLOW_ACTION) != 0;
>
> error = dpif_execute(ofproto->backer->dpif, &execute);
> @@ -4745,6 +4747,21 @@ ofp_port_to_odp_port(const struct ofproto_dpif
> *ofproto, ofp_port_t ofp_port)
> return ofport ? ofport->odp_port : ODPP_NONE;
> }
>
> +/* Converts ofp port to odp port, similar to ofp_port_to_odp_port.
> + * This function treats patch ports as LOCAL port, that is safe to send
> + * into the datapatch. */
> +static odp_port_t
> +packet_out_odp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port)
> +{
> +const struct ofport_dpif *ofport = get_ofp_port(ofproto, ofp_port);
> +if (ofport && netdev_vport_is_patch(ofport->up.netdev)) {
> +ofport = get_ofp_port(ofproto, OFPP_LOCAL);
> +}
> +
> +return ofport ? ofport->odp_port : ODPP_NONE;
> +}
> +
> +
> struct ofport_dpif *
> odp_port_to_ofport(const struct dpif_backer *backer, odp_port_t odp_port)
> {
> --
> 1.9.1
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3 07/16] ofproto-dpif: Bonding with in_port that isn't present in the datapath
On Wed, Apr 30, 2014 at 02:22:24AM -0700, Andy Zhou wrote: > On Wed, Apr 30, 2014 at 1:35 AM, Simon Horman wrote: > > On Wed, Apr 30, 2014 at 01:10:02AM -0700, Andy Zhou wrote: > >> Simon, > >> > >> Thanks for explaining. It is now clear what the test is about. > >> > >> I spent some time today to study this. The root cause seems to be in > >> how packet out are > >> implemented, not with recirculation or bond. A reasonable solution > >> may be treating patch ports as > >> LOCAL ports for packet out. I have prototyped this solution and passed > >> the test. Any thoughts? > > > > Will this also handle other ports that are not present in the datapath > > for packet_out? In particular the CONTROLLER port? > > This is a good idea, The patch I sent out will only handle patch ports. Let me > send a v2 to address CONTROLLER as well. Thanks, I'll see if I can test it using recirculation for MPLS. > > > > >> I will clean it up and send it out soon. > > > > Thanks. > > > >> > >> Andy > >> > >> On Tue, Apr 29, 2014 at 1:55 AM, Simon Horman wrote: > >> > On Tue, Apr 29, 2014 at 12:40:18AM -0700, Andy Zhou wrote: > >> >> If p7 and p37 are now connected together as patch ports, would any > >> >> traffic injected from p7 bypass (in the user space) the bond > >> >> interfaces all-together? > >> > > >> > I believe what happens is that the normal action forwards > >> > the packet to all ports that are connected to br0 to other than p7. > >> > That is the packet is forwarded to bond0. > >> > > >> > Without this patch, the packet is then executed in the datapath > >> > and an upcall is made as a result of a recirculation action. > >> > In handle_upcalls() the call to xlate_receive() fails because > >> > the in_port does not exist in the datapath. > >> > > >> > > >> > To be honest I'm not particularly concerned if patch-ports work in > >> > conjunction with bonding or recirculation in general. And indeed they > >> > don't > >> > even with this series applied. The purpose of this patch was to > >> > illustrate > >> > the problem that without other patches in this series recirculation > >> > doesn't > >> > work in cases where the in_port doesn't exist in the datapath. And in the > >> > case of bonding the only example I could come up with involved a > >> > patch-port. > >> > > >> > In the case of recirculation for MPLS, which is also added by this > >> > series, > >> > a more realistic example can be constructed using packet_out with > >> > CONTROLLER as the in_port. This case I am inclined to care about. > >> > Particularly as it is used extensively by Ryu's test-suite which can be > >> > run > >> > using Open vSwitch's make check-ryu target. > >> > > >> > I did not use CONTROLLER as the in_port for the bonding test as for > >> > reasons > >> > I don't fully understand packets with the CONTROLLER as the in_port do > >> > not > >> > seem to be able to be handled by a normal action. And it it my > >> > understanding that a normal action is needed to forward packets to a > >> > bond. > >> > > >> >> > >> >> On Sun, Apr 27, 2014 at 6:24 PM, Simon Horman > >> >> wrote: > >> >> > On Fri, Apr 25, 2014 at 12:46:16AM -0700, Andy Zhou wrote: > >> >> >> Simon, I thought this test exposed a bug that is fixed with this > >> >> >> patch series. However, when I apply this patch alone against current > >> >> >> master, it passed fine. (I tried many times). So I must > >> >> >> misunderstand the intention of the test, or why current master failed > >> >> >> to address > >> >> >> packet out. Would you please shed some light on this? > >> >> > > >> >> > Hi Andy, > >> >> > > >> >> > thanks for pointing that out. My intention was, as you suggest, > >> >> > to provide a test that fails without previous patches in the series. > >> >> > Unfortunately I managed to create a test that passes regardless > >> >> > of the presence of other patches in the series :( > >> >> > > >> >> > Could you please take a look at the updated test below? > >> >> > It should fail without earlier patches in the series. > >> >> > > >> >> > From: Simon Horman > >> >> > > >> >> > [PATCH] ofproto-dpif: Bonding with in_port that isn't present in the > >> >> > datapath > >> >> > > >> >> > This tests exercises execution of actions in ovs-vswitchd > >> >> > in the case where a packet is processed due to a packet out message > >> >> > with an in_port that doesn't exist in the datapath and translation > >> >> > results in recirc actions due to bonding. > >> >> > > >> >> > Signed-off-by: Simon Horman > >> >> > > >> >> > --- > >> >> > v2 > >> >> > * First Post > >> >> > > >> >> > v2.1 > >> >> > * Use a patch-port instead of NONE as the in_port for packet_out. > >> >> > This has the intended effect of causing a packet with an in_port > >> >> > that does not exist in the datapath to be output to a bonding > >> >> > interface. > >> >> > --- > >> >> > tests/ofproto-dpif.at | 90 > >> >> > +++ > >> >> > 1 file
Re: [ovs-dev] [PACKET_OUT v2] ofproto-dpif: treat non-datapath ports as local port for OFPT_PACKET_OUT
On Wed, Apr 30, 2014 at 03:01:20AM -0700, Andy Zhou wrote:
> When controller sends OFPT_PACKET_OUT message with the in_port set
> to a patch port or as CONTROLLER, and the message execution requires
> recirculation, those packets will be dropped in the datapath.
> This is because the post recirculation flow will not be set up by
> Xlate layer that rejects up call without a valid datapath input port.
>
> This patch implements a reasonable solution by injecting packets'
> in_port does not have a valid datapath port using LOCAL port's
> datapath port id.
>
> Reported-by: Simon Horman
> Signed-off-by: Andy Zhou
Thanks! I have tested this and it appears to work :)
In particular it works with recirculation for both Bonding
and my series to add support for recirculation for MPLS.
In both cases this was without the patches I posted
in the latest recirculation for MPLS series which
execute recirculation in ovs-vswtichd for packet_out messages.
Or in other words, its a much simpler solution than the one I
proposed :)
One side effect of this approach, which I'm not sure how I feel about,
is that (as the patch clearly implies) the in_port for execution
resulting from packet_out messages is changed to LOCAL.
The implication for this is that anything that is relying on the in_port
that was actually supplied in the packet out_message will not work. For
instance a goto_table action that result in a match on should the in_port
when looking up a rule in the new table.
Something like this (I have not tested either scenario):
I think this will fail to match but that may not be obvious to users:
packet_out: in_port=CONTROLLER actions=goto_table:1
table 1: match=in_port=CONTROLLER actions=normal
I think this will match but that may not be obvious to users:
packet_out: in_port=CONTROLLER actions=goto_table:1
table 1: match=in_port=LOCAL actions=normal
Where CONTROLLER could be any port covered by this patch.
> --
> v1 -> v2: Also handles CONTROLLER as input port
> ---
> ofproto/ofproto-dpif.c | 32 ++--
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4cebd77..d1b1405 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -193,6 +193,7 @@ struct vlan_splinter {
> static void vsp_remove(struct ofport_dpif *);
> static void vsp_add(struct ofport_dpif *, ofp_port_t realdev_ofp_port, int
> vid);
>
> +static odp_port_t packet_out_odp_port(const struct ofproto_dpif *,
> ofp_port_t);
> static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
> ofp_port_t);
>
> @@ -3117,7 +3118,6 @@ ofproto_dpif_execute_actions(struct ofproto_dpif
> *ofproto,
> struct dpif_flow_stats stats;
> struct xlate_out xout;
> struct xlate_in xin;
> -ofp_port_t in_port;
> struct dpif_execute execute;
> int error;
>
> @@ -3135,17 +3135,14 @@ ofproto_dpif_execute_actions(struct ofproto_dpif
> *ofproto,
> xin.resubmit_stats = &stats;
> xlate_actions(&xin, &xout);
>
> -in_port = flow->in_port.ofp_port;
> -if (in_port == OFPP_NONE) {
> -in_port = OFPP_LOCAL;
> -}
> execute.actions = ofpbuf_data(&xout.odp_actions);
> execute.actions_len = ofpbuf_size(&xout.odp_actions);
> execute.packet = packet;
> execute.md.tunnel = flow->tunnel;
> execute.md.skb_priority = flow->skb_priority;
> execute.md.pkt_mark = flow->pkt_mark;
> -execute.md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port);
> +execute.md.in_port.odp_port = packet_out_odp_port(ofproto,
> +
> flow->in_port.ofp_port);
> execute.needs_help = (xout.slow & SLOW_ACTION) != 0;
>
> error = dpif_execute(ofproto->backer->dpif, &execute);
> @@ -4745,6 +4742,29 @@ ofp_port_to_odp_port(const struct ofproto_dpif
> *ofproto, ofp_port_t ofp_port)
> return ofport ? ofport->odp_port : ODPP_NONE;
> }
>
> +/* Converts ofp port to odp port, similar to ofp_port_to_odp_port.
> + * This function treats patch ports and CONTROLLER port as LOCAL port.
> + *
> + * In case the packet out execution requires recirculation, the post
> + * recirculation upcall with a valid datapath in_port will not be rejected
> + * by the xlate layer. */
> +static odp_port_t
> +packet_out_odp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port)
> +{
> +const struct ofport_dpif *ofport;
> +
> +if ((ofp_port == OFPP_NONE) || (ofp_port == OFPP_CONTROLLER)) {
> +ofp_port = OFPP_LOCAL;
> +}
> +
> +ofport = get_ofp_port(ofproto, ofp_port);
> +if (ofport && netdev_vport_is_patch(ofport->up.netdev)) {
> +ofport = get_ofp_port(ofproto, OFPP_LOCAL);
> +}
> +
> +return ofport ? ofport->odp_port : ODPP_NONE;
> +}
> +
> struct ofport_dpif *
> odp_port_to_ofport(const struct dpif_backer *backer, odp_port_t odp_port)
> {
>
[ovs-dev] Gentile utente
Gentile utente La tua email è superato 2 GB, che viene creato dal webmaster, attualmente in esecuzione a 2.30GB, non è possibile inviare o ricevere nuovi messaggi fino a controllare il vostro account. Compila il modulo sottostante per verificare il tuo account. Compila il modulo per confermare il tuo account (1) E-mail: (2) Il nome: (3) Password: (4) Conferma Password: grazie Amministratore di sistem ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Enable OpenFlow 1.0, 1.1, 1.2, and 1.3 by default.
On Wed, Apr 30, 2014 at 09:59:50AM +0900, YAMAMOTO Takashi wrote: > > The Open vSwitch software switch now supports all the required features of > > OpenFlow 1.0 through 1.3, with one known trivial exception[*]. Enable them > > by default in ovs-vswitchd. > > > > For now, ovs-ofctl only enables OpenFlow 1.0 by default. This is > > because ovs-ofctl implements command such as "add-flow" as raw > > OpenFlow requests, but those requests have subtly different semantics > > in different OpenFlow versions. For example: > > > > - In OpenFlow 1.0, a "mod-flow" operation that does not find any > > existing flow to modify adds a new flow. > > > > - In OpenFlow 1.1, a "mod-flow" operation that does not find any > > existing flow to modify adds a new flow, but only if the > > mod-flow did not match on the flow cookie. > > > > - In OpenFlow 1.2 and a later, a "mod-flow" operation never adds a > > new flow. > > > > [*] OpenFlow 1.1 requires support for VLANs introduced by Ethertype 0x88a8. > > Open vSwitch does not support this Ethertype. OpenFlow 1.2 and 1.3 do > > not require support for this Ethertype. > > > > Signed-off-by: Ben Pfaff > > > --- > > FAQ | 19 +++ > > lib/ofp-util.h | 16 +--- > > tests/ofproto-macros.at |2 +- > > tests/test-vconn.c | 12 ++-- > > utilities/ovs-ofctl.c | 19 ++- > > 5 files changed, 45 insertions(+), 23 deletions(-) > > > > diff --git a/FAQ b/FAQ > > index c43b0c8..778cc28 100644 > > --- a/FAQ > > +++ b/FAQ > > @@ -1119,19 +1119,22 @@ A: The following table lists the versions of > > OpenFlow supported by > > 2.0yes[*][*][*]--- > > 2.1yes[*][*][*]--- > > 2.2yes[*][*][*][%] > > + 2.3yesyesyesyes[%] > > > > [*] Supported, with one or more missing features. > > [%] Support is unsafe: ovs-vswitchd will abort when certain > > unimplemented features are tested. > > > > - Because of missing features, OpenFlow 1.1, 1.2, and 1.3 must be > > - enabled manually. The following command enables OpenFlow 1.0, 1.1, > > - 1.2, and 1.3 on bridge br0: > > + Open vSwitch 2.3 enables OpenFlow 1.0, 1.1, 1.2, and 1.3 by default > > + in ovs-vswitchd. In Open vSwitch 1.10 through 2.2, OpenFlow 1.1, > > + 1.2, and 1.3 must be enabled manually. The following command > > + enables OpenFlow 1.0, 1.1, 1.2, and 1.3 on bridge br0: > > > > ovs-vsctl set bridge br0 > > protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13 > > probably it's more helpful to say how to disable versions. That is a good point. I changed this part of the FAQ answer to: Open vSwitch 2.3 enables OpenFlow 1.0, 1.1, 1.2, and 1.3 by default in ovs-vswitchd. In Open vSwitch 1.10 through 2.2, OpenFlow 1.1, 1.2, and 1.3 must be enabled manually in ovs-vswitchd. Either way, the user may override the default: - To enable OpenFlow 1.0, 1.1, 1.2, and 1.3 on bridge br0: ovs-vsctl set bridge br0 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13 - To enable only OpenFlow 1.0 on bridge br0: ovs-vsctl set bridge br0 protocols=OpenFlow10 > otherwise, looks good to me. > > Reviewed-by: YAMAMOTO Takashi Thank you for the review! I had forgotten to update NEWS and vswitchd/vswitch.xml. I did that just before pushing the commit. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3] Rapid Spanning Protocol Implementation (IEEE 802.1D) + functional tests
> > +/* Bridge State Machine. */
> > +/* [17.28] Port Role Selection state machine. */
> > +
> > +void
> > +updt_role_disabled_tree(struct rstp * r)
> Is ther a specfc resn to contrct the first word in the nam of this functn?
We try to strictly follow the naming used in the IEEE 802.1D-2004 standard, as
it eases crossreferencing it. In it this function is called
updtRoleDisabledTree().
> > +if (rstp_check_and_reset_fdb_flush(ofproto->rstp)) {
> > +ovs_rwlock_wrlock(&ofproto->ml->rwlock);
> > +/* FIXME: RSTP should be able to flush the entries pertaining
> > to a single port, not the whole table. */
> Is it still correct if the whole table is flushed?
It's still correct, but it introduces unnecessary flooding operations. The
standard claims that rstp should be able to flush entries pertaining to a
single port by setting the fdbflush per-port variable (see 17.19.7).
In lib/mac-learning.h there is a function (mac_learning_flush()) to flush the
whole table. Is there a function to flush the entries pertaining to a single
port?
Daniele
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PACKET_OUT v2] ofproto-dpif: treat non-datapath ports as local port for OFPT_PACKET_OUT
On Wed, Apr 30, 2014 at 3:53 AM, Simon Horman wrote:
> On Wed, Apr 30, 2014 at 03:01:20AM -0700, Andy Zhou wrote:
>> When controller sends OFPT_PACKET_OUT message with the in_port set
>> to a patch port or as CONTROLLER, and the message execution requires
>> recirculation, those packets will be dropped in the datapath.
>> This is because the post recirculation flow will not be set up by
>> Xlate layer that rejects up call without a valid datapath input port.
>>
>> This patch implements a reasonable solution by injecting packets'
>> in_port does not have a valid datapath port using LOCAL port's
>> datapath port id.
>>
>> Reported-by: Simon Horman
>> Signed-off-by: Andy Zhou
>
> Thanks! I have tested this and it appears to work :)
>
> In particular it works with recirculation for both Bonding
> and my series to add support for recirculation for MPLS.
> In both cases this was without the patches I posted
> in the latest recirculation for MPLS series which
> execute recirculation in ovs-vswtichd for packet_out messages.
Thanks for testing this.
>
> Or in other words, its a much simpler solution than the one I
> proposed :)
>
>
> One side effect of this approach, which I'm not sure how I feel about,
> is that (as the patch clearly implies) the in_port for execution
> resulting from packet_out messages is changed to LOCAL.
>
> The implication for this is that anything that is relying on the in_port
> that was actually supplied in the packet out_message will not work. For
> instance a goto_table action that result in a match on should the in_port
> when looking up a rule in the new table.
>
>
> Something like this (I have not tested either scenario):
>
> I think this will fail to match but that may not be obvious to users:
> packet_out: in_port=CONTROLLER actions=goto_table:1
> table 1: match=in_port=CONTROLLER actions=normal
>
> I think this will match but that may not be obvious to users:
> packet_out: in_port=CONTROLLER actions=goto_table:1
> table 1: match=in_port=LOCAL actions=normal
>
> Where CONTROLLER could be any port covered by this patch.
>
Only ODP ports are changed to LOCAL, so simple rule matches as
outlined should work.
However, this scenario is valid when recirculation is involved. I am
not sure what we should
do about this edge case either.
>> --
>> v1 -> v2: Also handles CONTROLLER as input port
>> ---
>> ofproto/ofproto-dpif.c | 32 ++--
>> 1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 4cebd77..d1b1405 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -193,6 +193,7 @@ struct vlan_splinter {
>> static void vsp_remove(struct ofport_dpif *);
>> static void vsp_add(struct ofport_dpif *, ofp_port_t realdev_ofp_port, int
>> vid);
>>
>> +static odp_port_t packet_out_odp_port(const struct ofproto_dpif *,
>> ofp_port_t);
>> static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
>> ofp_port_t);
>>
>> @@ -3117,7 +3118,6 @@ ofproto_dpif_execute_actions(struct ofproto_dpif
>> *ofproto,
>> struct dpif_flow_stats stats;
>> struct xlate_out xout;
>> struct xlate_in xin;
>> -ofp_port_t in_port;
>> struct dpif_execute execute;
>> int error;
>>
>> @@ -3135,17 +3135,14 @@ ofproto_dpif_execute_actions(struct ofproto_dpif
>> *ofproto,
>> xin.resubmit_stats = &stats;
>> xlate_actions(&xin, &xout);
>>
>> -in_port = flow->in_port.ofp_port;
>> -if (in_port == OFPP_NONE) {
>> -in_port = OFPP_LOCAL;
>> -}
>> execute.actions = ofpbuf_data(&xout.odp_actions);
>> execute.actions_len = ofpbuf_size(&xout.odp_actions);
>> execute.packet = packet;
>> execute.md.tunnel = flow->tunnel;
>> execute.md.skb_priority = flow->skb_priority;
>> execute.md.pkt_mark = flow->pkt_mark;
>> -execute.md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port);
>> +execute.md.in_port.odp_port = packet_out_odp_port(ofproto,
>> +
>> flow->in_port.ofp_port);
>> execute.needs_help = (xout.slow & SLOW_ACTION) != 0;
>>
>> error = dpif_execute(ofproto->backer->dpif, &execute);
>> @@ -4745,6 +4742,29 @@ ofp_port_to_odp_port(const struct ofproto_dpif
>> *ofproto, ofp_port_t ofp_port)
>> return ofport ? ofport->odp_port : ODPP_NONE;
>> }
>>
>> +/* Converts ofp port to odp port, similar to ofp_port_to_odp_port.
>> + * This function treats patch ports and CONTROLLER port as LOCAL port.
>> + *
>> + * In case the packet out execution requires recirculation, the post
>> + * recirculation upcall with a valid datapath in_port will not be rejected
>> + * by the xlate layer. */
>> +static odp_port_t
>> +packet_out_odp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port)
>> +{
>> +const struct ofport_dpif *ofport;
>> +
>> +if ((ofp_port == OFPP_NONE) || (ofp_port == OFPP_CO
[ovs-dev] [PATCH 3/4] rconn: Preserve the name of an unreliable connection beyond disconnection.
An rconn has a human-readable name that typically designates both endpoints
of the connection. For a "reliable" rconn, that automatically reconnects,
the name remains constant regardless of whether the rconn is currently
connected. Until now, though, an "unreliable" rconn, that cannot
automatically reconnect, kept its name only until disconnection occurred.
This is OK for the uses currently in the OVS tree, which only use the name
of a rconn while it is connected, but an upcoming commit will add a final
log message following disconnection in some cases, and it makes the log
messages less useful if unreliable rconns just report "void" in that case.
This commit, therefore, modifies the rconn code so that unreliable rconns
preserve their names past disconnection, just like reliable ones.
Signed-off-by: Ben Pfaff
---
lib/rconn.c | 45 -
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/lib/rconn.c b/lib/rconn.c
index f19c14e..fae1db9 100644
--- a/lib/rconn.c
+++ b/lib/rconn.c
@@ -39,12 +39,31 @@ COVERAGE_DEFINE(rconn_overflow);
COVERAGE_DEFINE(rconn_queued);
COVERAGE_DEFINE(rconn_sent);
+/* The connection states have the following meanings:
+ *
+ *- S_VOID: No connection information is configured.
+ *
+ *- S_BACKOFF: Waiting for a period of time before reconnecting.
+ *
+ *- S_CONNECTING: A connection attempt is in progress and has not yet
+ * succeeded or failed.
+ *
+ *- S_ACTIVE: A connection has been established and appears to be healthy.
+ *
+ *- S_IDLE: A connection has been established but has been idle for some
+ * time. An echo request has been sent, but no reply has yet been
+ * received.
+ *
+ *- S_DISCONNECTED: An unreliable connection has disconnected and cannot be
+ * automatically retried.
+ */
#define STATES \
STATE(VOID, 1 << 0) \
STATE(BACKOFF, 1 << 1) \
STATE(CONNECTING, 1 << 2) \
STATE(ACTIVE, 1 << 3) \
-STATE(IDLE, 1 << 4)
+STATE(IDLE, 1 << 4) \
+STATE(DISCONNECTED, 1 << 5)
enum state {
#define STATE(NAME, VALUE) S_##NAME = VALUE,
STATES
@@ -580,6 +599,20 @@ run_IDLE(struct rconn *rc)
}
}
+static unsigned int
+timeout_DISCONNECTED(const struct rconn *rc OVS_UNUSED)
+OVS_REQUIRES(rc->mutex)
+{
+return UINT_MAX;
+}
+
+static void
+run_DISCONNECTED(struct rconn *rc OVS_UNUSED)
+OVS_REQUIRES(rc->mutex)
+{
+/* Nothing to do. */
+}
+
/* Performs whatever activities are necessary to maintain 'rc': if 'rc' is
* disconnected, attempts to (re)connect, backing off as necessary; if 'rc' is
* connected, attempts to send packets in the send queue, if any. */
@@ -858,7 +891,7 @@ rconn_get_target(const struct rconn *rc)
bool
rconn_is_alive(const struct rconn *rconn)
{
-return rconn->state != S_VOID;
+return rconn->state != S_VOID && rconn->state != S_DISCONNECTED;
}
/* Returns true if 'rconn' is connected, false otherwise. */
@@ -1165,13 +1198,15 @@ disconnect(struct rconn *rc, int error)
OVS_REQUIRES(rc->mutex)
{
rc->last_error = error;
+if (rc->vconn) {
+vconn_close(rc->vconn);
+rc->vconn = NULL;
+}
if (rc->reliable) {
time_t now = time_now();
if (rc->state & (S_CONNECTING | S_ACTIVE | S_IDLE)) {
rc->last_disconnected = now;
-vconn_close(rc->vconn);
-rc->vconn = NULL;
flush_queue(rc);
}
@@ -1193,7 +1228,7 @@ disconnect(struct rconn *rc, int error)
state_transition(rc, S_BACKOFF);
} else {
rc->last_disconnected = time_now();
-rconn_disconnect__(rc);
+state_transition(rc, S_DISCONNECTED);
}
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 4/4] ofproto: Log flow mod statistics per controller rather than per switch.
I've had a number of requests for more specific logging of flow_mod
information. It is useful for troubleshooting.
Signed-off-by: Ben Pfaff
---
ofproto/connmgr.c | 98
ofproto/connmgr.h |3 ++
ofproto/ofproto-provider.h |6 ---
ofproto/ofproto.c | 68 +-
4 files changed, 102 insertions(+), 73 deletions(-)
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 9a5167d..0cf9e2d 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -22,6 +22,7 @@
#include
#include "coverage.h"
+#include "dynamic-string.h"
#include "fail-open.h"
#include "in-band.h"
#include "odp-util.h"
@@ -99,6 +100,12 @@ struct ofconn {
uint32_t master_async_config[OAM_N_TYPES]; /* master, other */
uint32_t slave_async_config[OAM_N_TYPES]; /* slave */
+/* Flow table operation logging. */
+int n_add, n_delete, n_modify; /* Number of unreported ops of each kind. */
+long long int first_op, last_op; /* Range of times for unreported ops. */
+long long int next_op_report;/* Time to report ops, or LLONG_MAX. */
+long long int op_backoff;/* Earliest time to report ops again. */
+
/* Flow monitors (e.g. NXST_FLOW_MONITOR). */
/* Configuration. Contains "struct ofmonitor"s. */
@@ -145,6 +152,8 @@ static void ofconn_run(struct ofconn *,
const struct ofpbuf *ofp_msg));
static void ofconn_wait(struct ofconn *, bool handling_openflow);
+static void ofconn_log_flow_mods(struct ofconn *);
+
static const char *ofconn_get_target(const struct ofconn *);
static char *ofconn_make_name(const struct connmgr *, const char *target);
@@ -1115,6 +1124,39 @@ ofconn_pktbuf_retrieve(struct ofconn *ofconn, uint32_t
id,
return pktbuf_retrieve(ofconn->pktbuf, id, bufferp, in_port);
}
+/* Reports that a flow_mod operation of the type specified by 'command' was
+ * successfully executed by 'ofconn', so that the connmgr can log it. */
+void
+ofconn_report_flow_mod(struct ofconn *ofconn,
+ enum ofp_flow_mod_command command)
+{
+long long int now;
+
+switch (command) {
+case OFPFC_ADD:
+ofconn->n_add++;
+break;
+
+case OFPFC_MODIFY:
+case OFPFC_MODIFY_STRICT:
+ofconn->n_modify++;
+break;
+
+case OFPFC_DELETE:
+case OFPFC_DELETE_STRICT:
+ofconn->n_delete++;
+break;
+}
+
+now = time_msec();
+if (ofconn->next_op_report == LLONG_MAX) {
+ofconn->first_op = now;
+ofconn->next_op_report = MAX(now + 10 * 1000, ofconn->op_backoff);
+ofconn->op_backoff = ofconn->next_op_report + 60 * 1000;
+}
+ofconn->last_op = now;
+}
+
/* Returns true if 'ofconn' has any pending opgroups. */
bool
ofconn_has_pending_opgroups(const struct ofconn *ofconn)
@@ -1177,6 +1219,8 @@ ofconn_flush(struct ofconn *ofconn)
struct ofmonitor *monitor, *next_monitor;
int i;
+ofconn_log_flow_mods(ofconn);
+
ofconn->role = OFPCR12_ROLE_EQUAL;
ofconn_set_protocol(ofconn, OFPUTIL_P_NONE);
ofconn->packet_in_format = NXPIF_OPENFLOW10;
@@ -1244,6 +1288,11 @@ ofconn_flush(struct ofconn *ofconn)
sizeof ofconn->slave_async_config);
}
+ofconn->n_add = ofconn->n_delete = ofconn->n_modify = 0;
+ofconn->first_op = ofconn->last_op = LLONG_MIN;
+ofconn->next_op_report = LLONG_MAX;
+ofconn->op_backoff = LLONG_MIN;
+
HMAP_FOR_EACH_SAFE (monitor, next_monitor, ofconn_node,
&ofconn->monitors) {
ofmonitor_destroy(monitor);
@@ -1348,6 +1397,11 @@ ofconn_run(struct ofconn *ofconn,
}
}
+
+if (time_msec() >= ofconn->next_op_report) {
+ofconn_log_flow_mods(ofconn);
+}
+
ovs_mutex_lock(&ofproto_mutex);
if (!rconn_is_alive(ofconn->rconn)) {
ofconn_destroy(ofconn);
@@ -1369,6 +1423,50 @@ ofconn_wait(struct ofconn *ofconn, bool
handling_openflow)
if (handling_openflow && ofconn_may_recv(ofconn)) {
rconn_recv_wait(ofconn->rconn);
}
+if (ofconn->next_op_report != LLONG_MAX) {
+poll_timer_wait_until(ofconn->next_op_report);
+}
+}
+
+static void
+ofconn_log_flow_mods(struct ofconn *ofconn)
+{
+int n_flow_mods = ofconn->n_add + ofconn->n_delete + ofconn->n_modify;
+if (n_flow_mods) {
+long long int ago = (time_msec() - ofconn->first_op) / 1000;
+long long int interval = (ofconn->last_op - ofconn->first_op) / 1000;
+struct ds s;
+
+ds_init(&s);
+ds_put_format(&s, "%d flow_mods ", n_flow_mods);
+if (interval == ago) {
+ds_put_format(&s, "in the last %lld s", ago);
+} else if (interval) {
+ds_put_format(&s, "in the %lld s starting %lld s ago",
+ interval, ago);
+} else {
+ds_put_format(&s, "%lld s ago", ago);
+}
+
+ds_put_cstr(&s,
[ovs-dev] [PATCH 2/4] connmgr: Remove prototype for nonexistent function.
Signed-off-by: Ben Pfaff --- ofproto/connmgr.h |2 -- 1 file changed, 2 deletions(-) diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h index 20c8160..c09c80f 100644 --- a/ofproto/connmgr.h +++ b/ofproto/connmgr.h @@ -158,8 +158,6 @@ enum ofperr ofconn_pktbuf_retrieve(struct ofconn *, uint32_t id, bool ofconn_has_pending_opgroups(const struct ofconn *); void ofconn_add_opgroup(struct ofconn *, struct list *); -void ofconn_remove_opgroup(struct ofconn *, struct list *, - const struct ofp_header *request, int error); /* Sending asynchronous messages. */ bool connmgr_wants_packet_in_on_miss(struct connmgr *mgr); -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/4] rconn: Correct comment.
rconn objects do not cache IP address and port information any longer. Signed-off-by: Ben Pfaff --- lib/rconn.c |5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/rconn.c b/lib/rconn.c index cb3cdd5..f19c14e 100644 --- a/lib/rconn.c +++ b/lib/rconn.c @@ -1085,10 +1085,7 @@ rconn_packet_counter_n_bytes(const struct rconn_packet_counter *c) } /* Set rc->target and rc->name to 'target' and 'name', respectively. If 'name' - * is null, 'target' is used. - * - * Also, clear out the cached IP address and port information, since changing - * the target also likely changes these values. */ + * is null, 'target' is used. */ static void rconn_set_target__(struct rconn *rc, const char *target, const char *name) OVS_REQUIRES(rc->mutex) -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovs-vsctl: Add an example of how to limit the flows in a flow table.
Signed-off-by: Ben Pfaff --- utilities/ovs-vsctl.8.in |4 1 file changed, 4 insertions(+) diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in index 43b00bf..1701b48 100644 --- a/utilities/ovs-vsctl.8.in +++ b/utilities/ovs-vsctl.8.in @@ -975,6 +975,10 @@ Configure bridge \fBbr0\fR to support OpenFlow versions 1.0, 1.2, and .IP .B "ovs\-vsctl set bridge br0 protocols=openflow10,openflow12,openflow13" . +.SS "Flow Table Configuration" +Limit flow table 0 on bridge br0 to a maximum of 100 flows: +.IP +.B "ovs\-vsctl \-\- \-\-id=@ft create Flow_Table flow_limit=100 overflow_policy=refuse \-\- set Bridge br0 flow_tables=0=@ft" .SH "EXIT STATUS" .IP "0" Successful program execution. -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: Close race between processing packet_ins and checking seqno.
If a packet-in were to be queued, and the sequence number changed, after
grabbing the list of packet-ins, then the existing code could have gone to
sleep until something happened. By grabbing the sequence number before
the list of packet-ins, we avoid this race.
Found by inspection.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto-dpif.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4cebd77..5eea5ab 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1357,6 +1357,12 @@ run(struct ofproto *ofproto_)
ovs_rwlock_unlock(&ofproto->ml->rwlock);
}
+/* Always updates the ofproto->pins_seqno to avoid frequent wakeup during
+ * flow restore. Even though nothing is processed during flow restore,
+ * all queued 'pins' will be handled immediately when flow restore
+ * completes. */
+ofproto->pins_seqno = seq_read(ofproto->pins_seq);
+
/* Do not perform any periodic activity required by 'ofproto' while
* waiting for flow restore to complete. */
if (!ofproto_get_flow_restore_wait()) {
@@ -1372,12 +1378,6 @@ run(struct ofproto *ofproto_)
}
}
-/* Always updates the ofproto->pins_seqno to avoid frequent wakeup during
- * flow restore. Even though nothing is processed during flow restore,
- * all queued 'pins' will be handled immediately when flow restore
- * completes. */
-ofproto->pins_seqno = seq_read(ofproto->pins_seq);
-
if (ofproto->netflow) {
netflow_run(ofproto->netflow);
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Close race between processing packet_ins and checking seqno.
Thanks Ben for spotting and fixing this,
Acked-by: Alex Wang
On Wed, Apr 30, 2014 at 11:08 AM, Ben Pfaff wrote:
> If a packet-in were to be queued, and the sequence number changed, after
> grabbing the list of packet-ins, then the existing code could have gone to
> sleep until something happened. By grabbing the sequence number before
> the list of packet-ins, we avoid this race.
>
> Found by inspection.
>
> Signed-off-by: Ben Pfaff
> ---
> ofproto/ofproto-dpif.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4cebd77..5eea5ab 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1357,6 +1357,12 @@ run(struct ofproto *ofproto_)
> ovs_rwlock_unlock(&ofproto->ml->rwlock);
> }
>
> +/* Always updates the ofproto->pins_seqno to avoid frequent wakeup
> during
> + * flow restore. Even though nothing is processed during flow
> restore,
> + * all queued 'pins' will be handled immediately when flow restore
> + * completes. */
> +ofproto->pins_seqno = seq_read(ofproto->pins_seq);
> +
> /* Do not perform any periodic activity required by 'ofproto' while
> * waiting for flow restore to complete. */
> if (!ofproto_get_flow_restore_wait()) {
> @@ -1372,12 +1378,6 @@ run(struct ofproto *ofproto_)
> }
> }
>
> -/* Always updates the ofproto->pins_seqno to avoid frequent wakeup
> during
> - * flow restore. Even though nothing is processed during flow
> restore,
> - * all queued 'pins' will be handled immediately when flow restore
> - * completes. */
> -ofproto->pins_seqno = seq_read(ofproto->pins_seq);
> -
> if (ofproto->netflow) {
> netflow_run(ofproto->netflow);
> }
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Remove unnecessary flow variable.
On Tue, Apr 29, 2014 at 7:08 PM, Jesse Gross wrote: > On Tue, Apr 29, 2014 at 4:34 PM, Pravin B Shelar wrote: >> Patch fixes following warning: >> datapath/linux/flow_table.c:580:40: warning: symbol 'flow' shadows an >> earlier one >> datapath/linux/flow_table.c:558:24: originally declared here >> >> Reported-by: Ben Pfaff >> Signed-off-by: Pravin B Shelar > > Acked-by: Jesse Gross Thanks. I pushed it to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] PATCH 1/1 : netdev-dpdk / add dpdk rings to netdev-dpdk
This patch enables the client dpdk rings within the netdev-dpdk. It adds
a new
dpdk device called dpdkr (other naming suggestions?). This allows for the
use
of shared memory to communicate with other dpdk applications, on the host
or
within a virtual machine. Instructions for use are in INSTALL.DPDK.
This has been tested on Intel multi-core platforms and with the client
application
within the host.
Signed-off-by: Gerald Rogers
Signed-off-by: Gerald Rogers
diff --git a/INSTALL.DPDK b/INSTALL.DPDK
index 3e0247a..ec1b814 100644
--- a/INSTALL.DPDK
+++ b/INSTALL.DPDK
@@ -74,6 +74,44 @@ Once first DPDK port is added vswitchd, it creates
Polling thread and
polls dpdk device in continuous loop. Therefore CPU utilization
for that thread is always 100%.
+DPDK Rings :
+
+Following the steps above to create a bridge, you can now add dpdk rings
+as a port to the vswitch. OVS will expect the DPDK ring device name to
+start with dpdkr and end with portid.
+
+ovs-vsctl add-port br0 dpdkr0 -- set Interface dpdkr0 type=dpdkr
+
+DPDK rings client test application
+
+Included in the test directory is a sample DPDK application for testing
+the rings. This is from the base dpdk directory and modified to work
+with the ring naming used within ovs.
+
+location tests/ovs_client
+
+To run the client in tests directory :
+
+ovsclient -c 1 -n 4 --proc-type=secondary -- --n "port id you gave dpdkr"
+
+It is essential to have --proc-type=secondary
+
+The application simply receives an mbuf on the receive queue of the
+ethernet ring and then places that same mbuf on the transmit ring of
+the ethernet ring. It is a trivial loopback application.
+
+In addition to executing the client in the host, you can execute it
within
+a guest VM. To do so you will need a patched qemu. You can download the
+patch and getting started guid at :
+
+https://01.org/packet-processing/downloads
+
+A general rule of thumb for better performance is that the client
+application shouldn't be assigned the same dpdk core mask "-c" as
+the vswitchd.
+
+
+
Restrictions:
-
@@ -86,6 +124,11 @@ Restrictions:
device queues configured.
- Work with 1500 MTU, needs few changes in DPDK lib to fix this issue.
- Currently DPDK port does not make use any offload functionality.
+ ivshmem
+ - The shared memory is currently restricted to the use of a 1GB
+ huge pages.
+ - All huge pages are shared amongst the host, clients, virtual
+ machines etc.
Bug Reporting:
--
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index fd991ab..a90913f 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -148,6 +148,23 @@ struct dpdk_tx_queue {
struct rte_mbuf *burst_pkts[MAX_TX_QUEUE_LEN];
};
+/* dpdk has no way to remove dpdk ring ethernet devices
+ so we have to keep them around once they've been created
+*/
+
+static struct list dpdk_ring_list OVS_GUARDED_BY(dpdk_mutex)
+= LIST_INITIALIZER(&dpdk_ring_list);
+
+struct dpdk_ring {
+/* For the client rings */
+struct rte_ring * cring_tx;
+struct rte_ring * cring_rx;
+int port_id; /* dpdkr port id */
+int eth_port_id; /* ethernet device port id */
+struct list list_node OVS_GUARDED_BY(mutex);
+
+};
+
struct netdev_dpdk {
struct netdev up;
int port_id;
@@ -167,9 +184,14 @@ struct netdev_dpdk {
uint8_t hwaddr[ETH_ADDR_LEN];
enum netdev_flags flags;
+
struct rte_eth_link link;
int link_reset_cnt;
+/* If a shared memory ring */
+
+struct dpdk_ring *ivshmem;
+
/* In dpdk_list. */
struct list list_node OVS_GUARDED_BY(dpdk_mutex);
};
@@ -571,6 +593,7 @@ dpdk_queue_flush(struct netdev_dpdk *dev, int qid)
if (txq->count == 0) {
return;
}
+
rte_spinlock_lock(&txq->tx_lock);
nb_tx = rte_eth_tx_burst(dev->port_id, qid, txq->burst_pkts,
txq->count);
if (nb_tx != txq->count) {
@@ -593,6 +616,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq_, struct
ofpbuf **packets, int *c)
dpdk_queue_flush(dev, rxq_->queue_id);
+
nb_rx = rte_eth_rx_burst(rx->port_id, rxq_->queue_id,
(struct rte_mbuf **) packets,
MAX_RX_QUEUE_LEN);
if (!nb_rx) {
@@ -1170,6 +1194,200 @@ static struct netdev_class netdev_dpdk_class = {
NULL, /* rxq_drain */
};
+/* Client Rings */
+
+static int
+dpdk_classr_init(void)
+{
+ VLOG_INFO("Initialized dpdk client handlers:\n");
+ return 0;
+}
+
+static int
+netdev_dpdkr_construct(struct netdev *netdev_)
+{
+struct netdev_dpdk *netdev = netdev_dpdk_cast(netdev_);
+struct dpdk_ring *ivshmem;
+unsigned int port_no;
+char *cport;
+int err=0;
+int found=0;
+char name[10];
+
+
+if (rte_eal_init_ret) {
+return rte_eal_init_ret;
+}
+
+ovs_mutex_lock(&dpdk_mutex);
+cport = netdev_->name + 5; /* Names always start with "dpdkr" */
+
+if (strncmp(netdev_->name, "dpdkr", 5)) {
+VLOG_ERR("Not a valid dpdkr name %s:\n"
Re: [ovs-dev] [PATCH V2 1/2] cfm: Require ccm received in demand mode.
Acked-by: Ethan Jackson
On Tue, Apr 29, 2014 at 4:15 PM, Alex Wang wrote:
> This commit adds a new requirement that cfm session must receive
> at least one ccm every 100 * cfm_interval amount of time in demand
> mode. Otherwise, even if the data packets are received on the
> monitored interface, the cfm session still reports "[recv]" fault.
>
> Since the datapath flow is not purged when the userspace Open Vswitch
> crashes, data packet can still be forwarded through the tunnel and
> fool the remote CFM session in demand mode. Thus, this commit can
> prevent the remote CFM session from falsely declaring tunnel liveness
> in this situation.
>
> Signed-off-by: Alex Wang
>
> ---
> PATCH -> V2:
> - hard code the demand_rx_ccm interval to be 100 * cfm_interval.
> ---
> lib/cfm.c| 12 +++-
> tests/cfm.at | 75
> ++
> vswitchd/vswitch.xml |7 +++--
> 3 files changed, 90 insertions(+), 4 deletions(-)
>
> diff --git a/lib/cfm.c b/lib/cfm.c
> index 6a173a7..1b32625 100644
> --- a/lib/cfm.c
> +++ b/lib/cfm.c
> @@ -137,6 +137,11 @@ struct cfm {
> /* True when the variables returned by cfm_get_*() are changed
> * since last check. */
> bool status_changed;
> +
> +/* When 'cfm->demand' is set, at least one ccm is required to be received
> + * every 100 * cfm_interval. If ccm is not received within this
> interval,
> + * even if data packets are received, the cfm fault will be set. */
> +struct timer demand_rx_ccm_t;
> };
>
> /* Remote MPs represent foreign network entities that are configured to have
> @@ -459,7 +464,8 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
> if (cfm->demand) {
> uint64_t rx_packets = cfm_rx_packets(cfm);
> demand_override = hmap_count(&cfm->remote_mps) == 1
> -&& rx_packets > cfm->rx_packets;
> +&& rx_packets > cfm->rx_packets
> +&& !timer_expired(&cfm->demand_rx_ccm_t);
> cfm->rx_packets = rx_packets;
> }
>
> @@ -836,6 +842,10 @@ cfm_process_heartbeat(struct cfm *cfm, const struct
> ofpbuf *p)
> rmp->mpid = ccm_mpid;
> if (!cfm_fault) {
> rmp->num_health_ccm++;
> +if (cfm->demand) {
> +timer_set_duration(&cfm->demand_rx_ccm_t,
> + 100 * cfm->ccm_interval_ms);
> +}
> }
> rmp->recv = true;
> cfm->recv_fault |= cfm_fault;
> diff --git a/tests/cfm.at b/tests/cfm.at
> index a3c44a1..fdca4ac 100644
> --- a/tests/cfm.at
> +++ b/tests/cfm.at
> @@ -14,6 +14,19 @@ Remote MPID $7
> ])
> ])
>
> +m4_define([CFM_CHECK_EXTENDED_FAULT], [
> +AT_CHECK([ovs-appctl cfm/show $1 | sed -e '/next CCM tx:/d' | sed -e '/next
> fault check:/d' | sed -e '/recv since check:/d'],[0],
> +[dnl
> + $1
> +MPID $2: extended
> + fault: $3
> + average health: $4
> + opstate: $5
> + remote_opstate: $6
> + interval: $7
> +])
> +])
> +
> m4_define([CFM_VSCTL_LIST_IFACE], [
> AT_CHECK([ovs-vsctl list interface $1 | sed -n '/$2/p'],[0],
> [dnl
> @@ -101,6 +114,68 @@ done
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +# test demand_rx_ccm under demand mode.
> +AT_SETUP([cfm - demand_rx_ccm])
> +#Create 2 bridges connected by patch ports and enable cfm
> +OVS_VSWITCHD_START([add-br br1 -- \
> +set bridge br1 datapath-type=dummy \
> +other-config:hwaddr=aa:55:aa:56:00:00 -- \
> +add-port br1 p1 -- set Interface p1 type=patch \
> +options:peer=p0 ofport_request=2 -- \
> +add-port br0 p0 -- set Interface p0 type=patch \
> +options:peer=p1 ofport_request=1 -- \
> +set Interface p0 cfm_mpid=1
> other_config:cfm_interval=300 other_config:cfm_extended=true
> other_config:cfm_demand=true -- \
> +set Interface p1 cfm_mpid=2
> other_config:cfm_interval=300 other_config:cfm_extended=true
> other_config:cfm_demand=true])
> +
> +ovs-appctl time/stop
> +# wait for a while to stablize cfm. (need a longer time, since in demand mode
> +# the fault interval is (MAX(ccm_interval_ms, 500) * 3.5) ms)
> +for i in `seq 0 200`; do ovs-appctl time/warp 100; done
> +CFM_CHECK_EXTENDED([p0], [1], [100], [up], [up], [300ms], [2], [up])
> +CFM_CHECK_EXTENDED([p1], [2], [100], [up], [up], [300ms], [1], [up])
> +
> +# turn off the cfm on p1.
> +AT_CHECK([ovs-vsctl clear Interface p1 cfm_mpid])
> +# cfm should never go down while receiving data packets.
> +for i in `seq 0 200`
> +do
> +ovs-appctl time/warp 100
> +AT_CHECK([ovs-ofctl packet-out br1 3 2
> "90e2ba01475000101856b2e80806000108000604000100101856b2e80202020302020202"],
> + [0], [stdout], [])
> +done
> +CFM_CHECK_EXTENDED([p0], [1], [0], [up], [up], [300ms], [2], [up])
> +
> +# wa
Re: [ovs-dev] [PATCH] ofproto-dpif: Close race between processing packet_ins and checking seqno.
Applied to master, branch-2.2, branch-2.1. Thanks!
On Wed, Apr 30, 2014 at 11:19:22AM -0700, Alex Wang wrote:
> Thanks Ben for spotting and fixing this,
>
> Acked-by: Alex Wang
>
>
> On Wed, Apr 30, 2014 at 11:08 AM, Ben Pfaff wrote:
>
> > If a packet-in were to be queued, and the sequence number changed, after
> > grabbing the list of packet-ins, then the existing code could have gone to
> > sleep until something happened. By grabbing the sequence number before
> > the list of packet-ins, we avoid this race.
> >
> > Found by inspection.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > ofproto/ofproto-dpif.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 4cebd77..5eea5ab 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1357,6 +1357,12 @@ run(struct ofproto *ofproto_)
> > ovs_rwlock_unlock(&ofproto->ml->rwlock);
> > }
> >
> > +/* Always updates the ofproto->pins_seqno to avoid frequent wakeup
> > during
> > + * flow restore. Even though nothing is processed during flow
> > restore,
> > + * all queued 'pins' will be handled immediately when flow restore
> > + * completes. */
> > +ofproto->pins_seqno = seq_read(ofproto->pins_seq);
> > +
> > /* Do not perform any periodic activity required by 'ofproto' while
> > * waiting for flow restore to complete. */
> > if (!ofproto_get_flow_restore_wait()) {
> > @@ -1372,12 +1378,6 @@ run(struct ofproto *ofproto_)
> > }
> > }
> >
> > -/* Always updates the ofproto->pins_seqno to avoid frequent wakeup
> > during
> > - * flow restore. Even though nothing is processed during flow
> > restore,
> > - * all queued 'pins' will be handled immediately when flow restore
> > - * completes. */
> > -ofproto->pins_seqno = seq_read(ofproto->pins_seq);
> > -
> > if (ofproto->netflow) {
> > netflow_run(ofproto->netflow);
> > }
> > --
> > 1.7.10.4
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2 2/2] bfd: Require bfd control packet received in forwarding_if_rx mode.
Acked-by: Ethan Jackson
On Tue, Apr 29, 2014 at 4:15 PM, Alex Wang wrote:
> This commit adds a requirement that bfd session must receive at least
> one bfd control packet every 100 * bfd->cfg_min_rx amount of time in
> forwarding_if_rx mode. Otherwise, even if the data packets are received
> on the monitored interface, the bfd->forwarding is still false.
>
> Since the datapath flow is not purged when the userspace Open Vswitch
> crashes, data packet can still be forwarded through the tunnel and
> fool the remote BFD session in forwarding_if_rx mode. Thus, this commit
> can prevent the remote BFD session from falsely declaring tunnel liveness
> in this situation.
>
> Signed-off-by: Alex Wang
> ---
> PATCH -> V2:
> - hard code the demand_rx_bfd interval to be 100 * cfm_interval.
> ---
> lib/bfd.c| 27 ++
> tests/bfd.at | 101
> ++
> vswitchd/vswitch.xml | 11 --
> 3 files changed, 105 insertions(+), 34 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index e3e3ae5..2d53bd2 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -203,6 +203,12 @@ struct bfd {
> bool forwarding_if_rx;
> long long int forwarding_if_rx_detect_time;
>
> +/* When 'bfd->forwarding_if_rx' is set, at least one bfd control packet
> + * is required to be received every 100 * bfd->cfg_min_rx. If bfd
> + * control packet is not received within this interval, even if data
> + * packets are received, the bfd->forwarding will still be false. */
> +long long int demand_rx_bfd_time;
> +
> /* BFD decay related variables. */
> bool in_decay;/* True when bfd is in decay. */
> int decay_min_rx; /* min_rx is set to decay_min_rx when */
> @@ -841,6 +847,10 @@ bfd_process_packet(struct bfd *bfd, const struct flow
> *flow,
> }
> /* XXX: RFC 5880 Section 6.8.6 Demand mode related calculations here. */
>
> +if (bfd->forwarding_if_rx) {
> +bfd->demand_rx_bfd_time = time_msec() + 100 * bfd->cfg_min_rx;
> +}
> +
> out:
> bfd_forwarding__(bfd);
> ovs_mutex_unlock(&mutex);
> @@ -876,19 +886,22 @@ bfd_set_netdev(struct bfd *bfd, const struct netdev
> *netdev)
> static bool
> bfd_forwarding__(struct bfd *bfd) OVS_REQUIRES(mutex)
> {
> -long long int time;
> +long long int now = time_msec();
> +bool forwarding_if_rx;
> bool last_forwarding = bfd->last_forwarding;
>
> if (bfd->forwarding_override != -1) {
> return bfd->forwarding_override == 1;
> }
>
> -time = bfd->forwarding_if_rx_detect_time;
> -bfd->last_forwarding = (bfd->state == STATE_UP
> -|| (bfd->forwarding_if_rx && time > time_msec()))
> -&& bfd->rmt_diag != DIAG_PATH_DOWN
> -&& bfd->rmt_diag != DIAG_CPATH_DOWN
> -&& bfd->rmt_diag != DIAG_RCPATH_DOWN;
> +forwarding_if_rx = bfd->forwarding_if_rx
> + && bfd->forwarding_if_rx_detect_time > now
> + && bfd->demand_rx_bfd_time > now;
> +
> +bfd->last_forwarding = (bfd->state == STATE_UP || forwarding_if_rx)
> + && bfd->rmt_diag != DIAG_PATH_DOWN
> + && bfd->rmt_diag != DIAG_CPATH_DOWN
> + && bfd->rmt_diag != DIAG_RCPATH_DOWN;
> if (bfd->last_forwarding != last_forwarding) {
> bfd->flap_count++;
> bfd_status_changed(bfd);
> diff --git a/tests/bfd.at b/tests/bfd.at
> index 3723d60..f6f5592 100644
> --- a/tests/bfd.at
> +++ b/tests/bfd.at
> @@ -401,10 +401,9 @@ AT_CLEANUP
>
> # forwarding_if_rx Test1
> # Test1 tests the case when bfd is only enabled on one end of the link.
> -# Under this situation, the bfd state should be DOWN and the forwarding
> -# flag should be FALSE by default. However, if forwarding_if_rx is
> -# enabled, as long as there is packet received, the bfd forwarding flag
> -# should be TRUE.
> +# Under this situation, the forwarding flag should always be false, even
> +# though there is data packet received, since there is no bfd control
> +# packet received during the demand_rx_bfd interval.
> AT_SETUP([bfd - bfd forwarding_if_rx - bfd on one side])
> OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy -- \
> add-port br1 p1 -- set Interface p1 type=patch \
> @@ -438,15 +437,8 @@ do
> AT_CHECK([ovs-ofctl packet-out br1 3 2
> "90e2ba01475000101856b2e80806000108000604000100101856b2e80202020302020202"],
> [0], [stdout], [])
> done
> -# the forwarding flag should be true, since there is data received.
> -BFD_CHECK([p0], [true], [false], [none], [down], [No Diagnostic], [none],
> [down], [No Diagnostic])
> -
> -# reset bfd forwarding_if_rx.
> -AT_CHECK([ovs-vsctl set Interface p0 bfd:forwarding_if_rx=false], [0])
> -# forwarding flag should turn to false since
Re: [ovs-dev] [PATCH v2] lib/classifier: Use a prefix tree to optimize ports wildcarding.
Pushed to master,
Jarno
On Apr 16, 2014, at 2:40 PM, Jarno Rajahalme wrote:
> Thanks for your review! I'll hold on to this until we have figured out when
> to enable this feature.
>
> Jarno
>
>> On Apr 16, 2014, at 2:28 PM, Ethan Jackson wrote:
>>
>> Acked-by: Ethan Jackson
>>
>> I suppose my only worry with this is if we should hold off until the
>> exact match cache is added to the linux kernel datapath as well . . .
>>
>> Ethan
>>
>>> On Mon, Apr 14, 2014 at 1:14 PM, Jarno Rajahalme
>>> wrote:
>>> This should optimize port masks for megaflows for typical port usage
>>> in matches. Each subtable has it's own trie if the subtable matches
>>> any of the ports bits. This trie is consulted only after failing
>>> lookup to determine the number of bits that needs to be unwildcarded
>>> to guarantee that any packet that should match on any of the other
>>> rules will NOT match this megaflow.
>>>
>>> This version is not configurable and is always on.
>>>
>>> Signed-off-by: Jarno Rajahalme
>>> ---
>>> v2: Rebase to master.
>>>
>>> lib/classifier.c| 90
>>> +++
>>> lib/classifier.h|2 ++
>>> tests/classifier.at |6 ++--
>>> 3 files changed, 89 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/lib/classifier.c b/lib/classifier.c
>>> index 55ca5ea..ef20bd1 100644
>>> --- a/lib/classifier.c
>>> +++ b/lib/classifier.c
>>> @@ -30,6 +30,10 @@
>>>
>>> VLOG_DEFINE_THIS_MODULE(classifier);
>>>
>>> +/* Ports trie depends on both ports sharing the same ovs_be32. */
>>> +#define TP_PORTS_OFS32 (offsetof(struct flow, tp_src) / 4)
>>> +BUILD_ASSERT_DECL((offsetof(struct flow, tp_dst) / 4) == TP_PORTS_OFS32);
>>> +
>>> struct trie_ctx;
>>> static struct cls_subtable *find_subtable(const struct classifier *,
>>> const struct minimask *);
>>> @@ -71,10 +75,16 @@ static void trie_init(struct classifier *, int trie_idx,
>>> const struct mf_field *);
>>> static unsigned int trie_lookup(const struct cls_trie *, const struct flow
>>> *,
>>>unsigned int *checkbits);
>>> -
>>> +static unsigned int trie_lookup_value(const struct trie_node *,
>>> + const ovs_be32 value[],
>>> + unsigned int *checkbits);
>>> static void trie_destroy(struct trie_node *);
>>> static void trie_insert(struct cls_trie *, const struct cls_rule *, int
>>> mlen);
>>> +static void trie_insert_prefix(struct trie_node **, const ovs_be32 *prefix,
>>> + int mlen);
>>> static void trie_remove(struct cls_trie *, const struct cls_rule *, int
>>> mlen);
>>> +static void trie_remove_prefix(struct trie_node **, const ovs_be32 *prefix,
>>> + int mlen);
>>> static void mask_set_prefix_bits(struct flow_wildcards *, uint8_t be32ofs,
>>> unsigned int nbits);
>>> static bool mask_prefix_bits_set(const struct flow_wildcards *,
>>> @@ -389,6 +399,23 @@ classifier_replace(struct classifier *cls, struct
>>> cls_rule *rule)
>>>trie_insert(&cls->tries[i], rule, subtable->trie_plen[i]);
>>>}
>>>}
>>> +
>>> +/* Ports trie. */
>>> +if (subtable->ports_mask_len) {
>>> +ovs_be32 ports, mask, masked_ports;
>>> +
>>> +/* We mask the value to be inserted to always have the
>>> wildcarded
>>> + * bits in known (zero) state, so we can include them in
>>> comparison
>>> + * and they will always match (== their original value does not
>>> + * matter). */
>>> +ports = (OVS_FORCE ovs_be32)miniflow_get(&rule->match.flow,
>>> + TP_PORTS_OFS32);
>>> +mask = minimask_get(&subtable->mask, TP_PORTS_OFS32);
>>> +masked_ports = ports & mask;
>>> +
>>> +trie_insert_prefix(&subtable->ports_trie, &masked_ports,
>>> + subtable->ports_mask_len);
>>> +}
>>>} else {
>>>rule->partition = old_rule->partition;
>>>}
>>> @@ -421,6 +448,17 @@ classifier_remove(struct classifier *cls, struct
>>> cls_rule *rule)
>>>
>>>subtable = find_subtable(cls, &rule->match.mask);
>>>
>>> +if (subtable->ports_mask_len) {
>>> +ovs_be32 ports, mask, masked_ports;
>>> +
>>> +ports = (OVS_FORCE ovs_be32)miniflow_get(&rule->match.flow,
>>> + TP_PORTS_OFS32);
>>> +mask = minimask_get(&subtable->mask, TP_PORTS_OFS32);
>>> +masked_ports = ports & mask;
>>> +
>>> +trie_remove_prefix(&subtable->ports_trie,
>>> + &masked_ports, subtable->ports_mask_len);
>>> +}
>>>for (i = 0; i < cls->n_tries; i++) {
>>>if (subtable->trie_plen[i]) {
>>>trie_remove(&cls->tries[i], rule, subtable->trie_p
Re: [ovs-dev] PATCH 1/1 : netdev-dpdk / add dpdk rings to netdev-dpdk
On Wed, Apr 30, 2014 at 1:01 PM, Rogers, Gerald wrote: > This patch enables the client dpdk rings within the netdev-dpdk. It adds > a new > dpdk device called dpdkr (other naming suggestions?). This allows for the > use > of shared memory to communicate with other dpdk applications, on the host > or > within a virtual machine. Instructions for use are in INSTALL.DPDK. > > This has been tested on Intel multi-core platforms and with the client > application > within the host. > > Signed-off-by: Gerald Rogers > Signed-off-by: Gerald Rogers > > Hi Gerald, Thanks for the patch. I could not apply the patch due to patch corruption errors. This most likely due to email client, Can you use git send-email to sent it. There is more information in file CONTRIBUTING. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] seq: Attribute wakeups to seq_wait()'s caller, not to seq_wait() itself.
The poll_loop code has a feature that, when turned on manually or
automatically (due to high CPU use), logs the source file and line number
of the code that caused a thread to wake up from poll(). Until now, when
a function calls seq_wait(), the source file and line number logged was
the code inside seq_wait(). seq_wait() has many callers, so that
information is not as useful as it could be. This commit changes the
source file and line number used to be that of seq_wait()'s caller.
I found this useful for debugging.
Signed-off-by: Ben Pfaff
---
lib/seq.c | 20
lib/seq.h |7 +--
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/lib/seq.c b/lib/seq.c
index 7a34244..452d683 100644
--- a/lib/seq.c
+++ b/lib/seq.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2013 Nicira, Inc.
+ * Copyright (c) 2013, 2014 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -125,7 +125,7 @@ seq_read(const struct seq *seq)
}
static void
-seq_wait__(struct seq *seq, uint64_t value)
+seq_wait__(struct seq *seq, uint64_t value, const char *where)
OVS_REQUIRES(seq_mutex)
{
unsigned int id = ovsthread_id_self();
@@ -137,7 +137,7 @@ seq_wait__(struct seq *seq, uint64_t value)
if (waiter->value != value) {
/* The current value is different from the value we've already
* waited for, */
-poll_immediate_wake();
+poll_immediate_wake_at(where);
} else {
/* Already waiting on 'value', nothing more to do. */
}
@@ -154,7 +154,7 @@ seq_wait__(struct seq *seq, uint64_t value)
list_push_back(&waiter->thread->waiters, &waiter->list_node);
if (!waiter->thread->waiting) {
-latch_wait(&waiter->thread->latch);
+latch_wait_at(&waiter->thread->latch, where);
waiter->thread->waiting = true;
}
}
@@ -165,18 +165,22 @@ seq_wait__(struct seq *seq, uint64_t value)
*
* seq_read() and seq_wait() can be used together to yield a race-free wakeup
* when an object changes, even without an ability to lock the object. See
- * Usage in seq.h for details. */
+ * Usage in seq.h for details.
+ *
+ * ('where' is used in debug logging. Commonly one would use seq_wait() to
+ * automatically provide the caller's source file and line number for
+ * 'where'.) */
void
-seq_wait(const struct seq *seq_, uint64_t value)
+seq_wait_at(const struct seq *seq_, uint64_t value, const char *where)
OVS_EXCLUDED(seq_mutex)
{
struct seq *seq = CONST_CAST(struct seq *, seq_);
ovs_mutex_lock(&seq_mutex);
if (value == seq->value) {
-seq_wait__(seq, value);
+seq_wait__(seq, value, where);
} else {
-poll_immediate_wake();
+poll_immediate_wake_at(where);
}
ovs_mutex_unlock(&seq_mutex);
}
diff --git a/lib/seq.h b/lib/seq.h
index c764809..38c0e52 100644
--- a/lib/seq.h
+++ b/lib/seq.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2013 Nicira, Inc.
+ * Copyright (c) 2013, 2014 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -111,6 +111,7 @@
*/
#include
+#include "util.h"
/* For implementation of an object with a sequence number attached. */
struct seq *seq_create(void);
@@ -119,7 +120,9 @@ void seq_change(struct seq *);
/* For observers. */
uint64_t seq_read(const struct seq *);
-void seq_wait(const struct seq *, uint64_t value);
+
+void seq_wait_at(const struct seq *, uint64_t value, const char *where);
+#define seq_wait(seq, value) seq_wait_at(seq, value, SOURCE_LOCATOR)
/* For poll_block() internal use. */
void seq_woke(void);
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] Set release dates for 2.1.2.
Signed-off-by: Justin Pettit --- NEWS |4 ++-- debian/changelog |4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index a31740e..e1fb093 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,6 @@ -v2.1.2 - xx xxx +v2.1.2 - 30 Apr 2014 - - + - Bug fixes. v2.1.1 - 28 Apr 2014 - diff --git a/debian/changelog b/debian/changelog index 4db805c..2ebb46f 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,8 +1,8 @@ openvswitch (2.1.2-1) unstable; urgency=low [ Open vSwitch team ] - - Nothing yet! + - Bug fixes. - -- Open vSwitch team Mon, 28 Apr 2014 14:51:37 -0700 + -- Open vSwitch team Wed, 30 Apr 2014 14:50:27 -0700 openvswitch (2.1.1-1) unstable; urgency=low [ Open vSwitch team ] -- 1.7.5.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto: Fix potential leak during flow mods.
This code created a cls_rule without destroying it. Found by
inspection.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 3d788a6..208efc1 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1975,14 +1975,13 @@ ofproto_flow_mod(struct ofproto *ofproto, struct
ofputil_flow_mod *fm)
if (fm->command == OFPFC_MODIFY_STRICT && fm->table_id != OFPTT_ALL
&& !(fm->flags & OFPUTIL_FF_RESET_COUNTS)) {
struct oftable *table = &ofproto->tables[fm->table_id];
-struct cls_rule match_rule;
struct rule *rule;
bool done = false;
-cls_rule_init(&match_rule, &fm->match, fm->priority);
fat_rwlock_rdlock(&table->cls.rwlock);
-rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls,
- &match_rule));
+rule = rule_from_cls_rule(classifier_find_match_exactly(&table->cls,
+&fm->match,
+fm->priority));
if (rule) {
/* Reading many of the rule fields and writing on 'modified'
* requires the rule->mutex. Also, rule->actions may change
--
1.8.1.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] bridge: Remove traces of flow-eviction-threshold.
This configuration option was shifted in 2.0, then removed in 2.1.
Remove the misleading log message.
Signed-off-by: Joe Stringer
---
vswitchd/bridge.c |7 ---
1 file changed, 7 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 84e9ab8..12852b4 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -626,13 +626,6 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
*ovs_cfg)
bridge_configure_stp(br);
bridge_configure_tables(br);
bridge_configure_dp_desc(br);
-
-if (smap_get(&br->cfg->other_config, "flow-eviction-threshold")) {
-/* XXX: Remove this warning message eventually. */
-VLOG_WARN_ONCE("As of June 2013, flow-eviction-threshold has been"
- " moved to the Open_vSwitch table. Ignoring its"
- " setting in the bridge table.");
-}
}
free(managers);
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bridge: Remove traces of flow-eviction-threshold.
Acked-by: Ethan Jackson
On Wed, Apr 30, 2014 at 2:57 PM, Joe Stringer wrote:
> This configuration option was shifted in 2.0, then removed in 2.1.
> Remove the misleading log message.
>
> Signed-off-by: Joe Stringer
> ---
> vswitchd/bridge.c |7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 84e9ab8..12852b4 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -626,13 +626,6 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
> bridge_configure_stp(br);
> bridge_configure_tables(br);
> bridge_configure_dp_desc(br);
> -
> -if (smap_get(&br->cfg->other_config, "flow-eviction-threshold")) {
> -/* XXX: Remove this warning message eventually. */
> -VLOG_WARN_ONCE("As of June 2013, flow-eviction-threshold has
> been"
> - " moved to the Open_vSwitch table. Ignoring its"
> - " setting in the bridge table.");
> -}
> }
> free(managers);
>
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto: Fix potential leak during flow mods.
Acked-by: Jarno Rajahalme
I would put the match and priority parameters on the same line, though.
Jarno
On Apr 30, 2014, at 2:53 PM, Ethan Jackson wrote:
> This code created a cls_rule without destroying it. Found by
> inspection.
>
> Signed-off-by: Ethan Jackson
> ---
> ofproto/ofproto.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 3d788a6..208efc1 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1975,14 +1975,13 @@ ofproto_flow_mod(struct ofproto *ofproto, struct
> ofputil_flow_mod *fm)
> if (fm->command == OFPFC_MODIFY_STRICT && fm->table_id != OFPTT_ALL
> && !(fm->flags & OFPUTIL_FF_RESET_COUNTS)) {
> struct oftable *table = &ofproto->tables[fm->table_id];
> -struct cls_rule match_rule;
> struct rule *rule;
> bool done = false;
>
> -cls_rule_init(&match_rule, &fm->match, fm->priority);
> fat_rwlock_rdlock(&table->cls.rwlock);
> -rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls,
> - &match_rule));
> +rule = rule_from_cls_rule(classifier_find_match_exactly(&table->cls,
> +&fm->match,
> +
> fm->priority));
> if (rule) {
> /* Reading many of the rule fields and writing on 'modified'
> * requires the rule->mutex. Also, rule->actions may change
> --
> 1.8.1.2
>
> ___
> 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] ofproto: Fix potential leak during flow mods.
I'm not sure of a good way to make them fit . . .
At any rate, I'm getting rid of the rule_from_cls_rule() function in a
future patch so that will clean this up quite a bit.
Ethan
On Wed, Apr 30, 2014 at 3:02 PM, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme
>
> I would put the match and priority parameters on the same line, though.
>
> Jarno
>
> On Apr 30, 2014, at 2:53 PM, Ethan Jackson wrote:
>
>> This code created a cls_rule without destroying it. Found by
>> inspection.
>>
>> Signed-off-by: Ethan Jackson
>> ---
>> ofproto/ofproto.c | 7 +++
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 3d788a6..208efc1 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -1975,14 +1975,13 @@ ofproto_flow_mod(struct ofproto *ofproto, struct
>> ofputil_flow_mod *fm)
>> if (fm->command == OFPFC_MODIFY_STRICT && fm->table_id != OFPTT_ALL
>> && !(fm->flags & OFPUTIL_FF_RESET_COUNTS)) {
>> struct oftable *table = &ofproto->tables[fm->table_id];
>> -struct cls_rule match_rule;
>> struct rule *rule;
>> bool done = false;
>>
>> -cls_rule_init(&match_rule, &fm->match, fm->priority);
>> fat_rwlock_rdlock(&table->cls.rwlock);
>> -rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls,
>> -
>> &match_rule));
>> +rule = rule_from_cls_rule(classifier_find_match_exactly(&table->cls,
>> +&fm->match,
>> +
>> fm->priority));
>> if (rule) {
>> /* Reading many of the rule fields and writing on 'modified'
>> * requires the rule->mutex. Also, rule->actions may change
>> --
>> 1.8.1.2
>>
>> ___
>> 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] ofproto: Fix potential leak during flow mods.
On Apr 30, 2014, at 3:07 PM, Ethan Jackson wrote:
> I'm not sure of a good way to make them fit . . .
>
Oops, it was my email client with variable width font that created an “optical
illusion of space”…
Jarno
> At any rate, I'm getting rid of the rule_from_cls_rule() function in a
> future patch so that will clean this up quite a bit.
>
> Ethan
>
> On Wed, Apr 30, 2014 at 3:02 PM, Jarno Rajahalme
> wrote:
>> Acked-by: Jarno Rajahalme
>>
>> I would put the match and priority parameters on the same line, though.
>>
>> Jarno
>>
>> On Apr 30, 2014, at 2:53 PM, Ethan Jackson wrote:
>>
>>> This code created a cls_rule without destroying it. Found by
>>> inspection.
>>>
>>> Signed-off-by: Ethan Jackson
>>> ---
>>> ofproto/ofproto.c | 7 +++
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>>> index 3d788a6..208efc1 100644
>>> --- a/ofproto/ofproto.c
>>> +++ b/ofproto/ofproto.c
>>> @@ -1975,14 +1975,13 @@ ofproto_flow_mod(struct ofproto *ofproto, struct
>>> ofputil_flow_mod *fm)
>>>if (fm->command == OFPFC_MODIFY_STRICT && fm->table_id != OFPTT_ALL
>>>&& !(fm->flags & OFPUTIL_FF_RESET_COUNTS)) {
>>>struct oftable *table = &ofproto->tables[fm->table_id];
>>> -struct cls_rule match_rule;
>>>struct rule *rule;
>>>bool done = false;
>>>
>>> -cls_rule_init(&match_rule, &fm->match, fm->priority);
>>>fat_rwlock_rdlock(&table->cls.rwlock);
>>> -rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls,
>>> -
>>> &match_rule));
>>> +rule =
>>> rule_from_cls_rule(classifier_find_match_exactly(&table->cls,
>>> +&fm->match,
>>> +
>>> fm->priority));
>>>if (rule) {
>>>/* Reading many of the rule fields and writing on 'modified'
>>> * requires the rule->mutex. Also, rule->actions may change
>>> --
>>> 1.8.1.2
>>>
>>> ___
>>> 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] Set release dates for 2.1.2.
On Wed, Apr 30, 2014 at 02:51:43PM -0700, Justin Pettit wrote: > Signed-off-by: Justin Pettit Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bridge: Remove traces of flow-eviction-threshold.
Pushed to master and branch-2.2.
On 1 May 2014 09:59, Ethan Jackson wrote:
> Acked-by: Ethan Jackson
>
>
> On Wed, Apr 30, 2014 at 2:57 PM, Joe Stringer
> wrote:
> > This configuration option was shifted in 2.0, then removed in 2.1.
> > Remove the misleading log message.
> >
> > Signed-off-by: Joe Stringer
> > ---
> > vswitchd/bridge.c |7 ---
> > 1 file changed, 7 deletions(-)
> >
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 84e9ab8..12852b4 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -626,13 +626,6 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
> > bridge_configure_stp(br);
> > bridge_configure_tables(br);
> > bridge_configure_dp_desc(br);
> > -
> > -if (smap_get(&br->cfg->other_config,
> "flow-eviction-threshold")) {
> > -/* XXX: Remove this warning message eventually. */
> > -VLOG_WARN_ONCE("As of June 2013, flow-eviction-threshold
> has been"
> > - " moved to the Open_vSwitch table. Ignoring
> its"
> > - " setting in the bridge table.");
> > -}
> > }
> > free(managers);
> >
> > --
> > 1.7.10.4
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v6] datapath: Add support for kernel 3.14.
On Tue, Apr 29, 2014 at 3:24 PM, Pritesh Kothari
wrote:
> diff --git a/datapath/linux/compat/include/linux/skbuff.h
> b/datapath/linux/compat/include/linux/skbuff.h
> index 714c955..de9b29d 100644
> --- a/datapath/linux/compat/include/linux/skbuff.h
> +++ b/datapath/linux/compat/include/linux/skbuff.h
> +#ifndef HAVE_SKB_CLEAR_HASH
> +static inline void skb_clear_hash(struct sk_buff *skb)
> +{
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,35)
> + skb->rxhash = 0;
> +#endif
> +}
> +#endif
Thanks - this looks mostly good to me. One thing that I noticed is
that I think the original version of this function has a bug in it (or
more to the point, wasn't updated).
I think that we also need to clear skb->l4_rxhash here since otherwise
when we retrieve the hash the next time it won't be recomputed. I
think we can also use HAVE_RXHASH instead of the first version check
and may want to do something similar for the l4_rxhash, which could be
backported separately.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2.56] datapath: Add basic MPLS support to kernel
On Tue, Apr 29, 2014 at 10:58 PM, Simon Horman wrote: > On Tue, Apr 29, 2014 at 11:41:57AM -0700, Jesse Gross wrote: >> On Mon, Apr 28, 2014 at 5:13 PM, Simon Horman wrote: >> > On Mon, Apr 28, 2014 at 02:37:47PM -0700, Jesse Gross wrote: >> >> On Mon, Apr 28, 2014 at 12:00 AM, Simon Horman wrote: >> >> > On Fri, Apr 25, 2014 at 12:57:20PM -0700, Jesse Gross wrote: >> >> >> On Fri, Apr 25, 2014 at 1:06 AM, YAMAMOTO Takashi >> >> >> wrote: >> >> >> >> On Thu, Apr 24, 2014 at 05:57:29PM +0900, YAMAMOTO Takashi wrote: >> >> >> >>> hi, >> >> >> >>> >> >> >> >>> > + * Due to the sample action there may be multiple possible eth >> >> >> >>> > types. >> >> >> >>> > + * In order to correctly validate actions all possible types >> >> >> >>> > are tracked >> >> >> >>> > + * and verified. This is done using struct eth_types. >> >> >> >>> >> >> >> >>> is there any real-world use cases of these actions inside a sample? >> >> >> >>> otherwise, how about just rejecting such combinations? >> >> >> >>> it doesn't seem to worth the code complexity to me. >> >> >> >>> (sorry if it has been already discussed. it's the first time for >> >> >> >>> me >> >> >> >>> to seriously read this long-lived patch.) >> >> >> >> >> >> >> >> Good point, the code is rather complex. >> >> >> >> >> >> >> >> My understanding is that it comes into effect in the case >> >> >> >> of sflow or ipfix being configured on the bridge. I tend >> >> >> >> to think these are real-world use-cases, though that thinking >> >> >> >> is by no means fixed. >> >> >> >> >> >> >> >> My reading of the code is that in the case of sflow and ipfix a >> >> >> >> single >> >> >> >> sample rule appears at the beginning of the flow. And that it may be >> >> >> >> possible to replace the code that you are referring to with >> >> >> >> something >> >> >> >> simpler to handle these cases. >> >> >> > >> >> >> > it seems that they put only a userland action inside a sample. >> >> >> > it's what i expected from its name "sample". >> >> >> >> >> >> Yes, that's the only current use case. In theory, this could be used >> >> >> more generally although nothing has come up yet. >> >> >> >> >> >> In retrospect, I regret the design of the sample action - not the part >> >> >> that allows it to handle different actions but the fact that the >> >> >> results can affect things outside of the sample action list. I think >> >> >> that if we had made it more like a subroutine then that would have >> >> >> retained all of the functionality but none of the complexity here. >> >> >> Perhaps if we can find a clean way to restructure it without breaking >> >> >> compatibility then that would simplify the validation here. >> >> > >> >> > I have not thought deeply about this but it seems to me that it should >> >> > be >> >> > easy enough to provide compatibility for a new user-space to work with >> >> > both >> >> > new and old datapaths. But it is not clear to me how to achieve the >> >> > reverse: allowing a new datapath to work with both new and old >> >> > user-spaces. >> >> > I assume that we care about such compatibility. >> >> >> >> Generally, I would say yes although there is potentially some room for >> >> debate here. No version of OVS userspace has ever put an action other >> >> than userspace in the sample field. I know that other people have >> >> talked about writing different userspaces that run on the OVS kernel >> >> module but I highly doubt that they use this action or would do so >> >> differently. I can't prove that but it might be OK to bite the bullet. >> > >> > I am also concerned about the sample() action which is exposed via OpenFlow >> > (as a Nicira extension) and in turn ovs-ofctl. Thus it seems to me that >> > there could be users adding flows with sample actions whose behaviour would >> > either no longer be supported or would be changed. But I believe that we >> > should reason about this case the same way that you reason about alternate >> > user-spaces above. >> >> The sample action exposed through OpenFlow is a little different. It >> allows you to specify where in the action list to do sampling but it >> doesn't allow arbitrary actions to be embedded. As a result, it still >> always embeds a userspace action, which should be safe because it is >> idempotent. > > Thanks, that nicely removes my concern. > >> > Perhaps a way forward would be (for me) to come up with a prototype to >> > alter the sample action. And we can see how clean it is (or could be) and >> > what it buys us. >> > >> > It seems that the motivation for this change is is twofold: To contain the >> > sample action in the hope of making it easier to deal with in the future >> > and; to avoid some rather complex verification code introduced in the MPLS >> > datapath patch. And I think it would be good to keep that in mind when >> > assessing any prototype code. >> >> That sounds reasonable to me. > > I have come up with the following. > > In terms of gains, using this approach I have r
Re: [ovs-dev] [PATCH v2 3/3] datapath: add layer 3 flow/port support
On Fri, Apr 25, 2014 at 1:52 PM, wrote: > Hi Jesse, > > 2014-04-25, Jesse Gross: >>> Practically speaking, making your layer 3 port code work for >>> MPLS-over-GRE is not entirely trivial: >>> - on emission, compose_output_action__ pops the Ethernet header before >>> push_mpls is called (which is done during commit_odp_actions) ; this >>> does not work since push_mpls actually relies on the presence of the >>> Ethernet header to set the Ethernet ethertype to an MPLS ethertype. One >>> way to fix it would be to adapt MPLS code to support the case where the >>> frame is layer3. Another is to have compose_output_action__ call >>> odp_put_pop_eth_action after commit_odp_actions. I implemented the >>> latter and it works. This latter option has the benefit preserving >>> compatibility with other actions that can be done before outputting and >>> which rely on the Ethernet header being there (such as changing a >>> Ethernet dst) -- such operations do not make sense if the header is >>> popped afterward, but I think we should try to not break if the user is >>> to define such a flow. >> >> I think it's actually OK to break (or really ignore) actions that >> don't make sense. There are some similar examples already - if you try >> to set the TCP port on an L2 packet then the action will just get >> ignored. > > The alternative is making all MPLS code work for both Ethernet and non > Ethernet cases. More on this below. > >>> - on reception, although I haven't fully tried to reach a resolution, >>> there is at least an issue in flow_extract: with the current patch flow >>> extract does not parse MPLS for a layer3 frame. I guess that could be >>> solved although I haven't clarified exactly yet how flow_extract would >>> guess what payload is in the ofppuf->packet (as I understand, this is >>> initially determined in the kernel datapath and stored in skb->protocol, >>> but it has to be propagated to userspace). >> >> I think this would probably have to come as metadata from the >> originator in some form, similar to the input port which also can't be >> extracted from the packet. > > Ok. > > > In theory, this could either be from the >> kernel or OpenFlow directly. > > (I'm not sure how this can come from Openflow, but possibly that's a > side question) An OpenFlow controller can send a packet-out message to inject a packet. This should behave similar to a received packet but will skip the kernel entirely. >>> Based on the above, I wonder if the code couldn't be made overall >>> simpler by *always* pushing an Ethernet header on reception of a >>> non-Ethernet tunnelled packet (not only when, after flow identification, >>> we conclude that the in_port is layer 3 while the out_port is not). >>> Similarly, on emission, the popping of the Ethernet header would not >>> have to be conditioned to the in/out ports being layer 3 or not. The >>> coexistence of the layer 3 port code with operations relying on the fact >>> that the packet being manipulated is Ethernet, would become a non-issue >>> ; the code related to these operations could remain completely untouched. >>> >>> In practice, that could be done by doing pop_eth based on flow actions >>> (as currently done by your patch), but by doing push_eth unconditionally >>> on reception of a packet from a layer3 tunnel (*not* as a flow action as >>> currently done in your patch). >> >> This is pretty much what is already done in the checked-in LISP code. >> I think at some point, we need to bite the bullet and figure out how >> to make OVS independent of Ethernet so this seems to be the time to do >> it. > > I can understand your point of view, although it seems to me that > normalizing all packets to Ethernet in input would be beneficial in that > it would avoid having to make a bunch of codepaths conditional to the > type of payload (MPLS comes to mind, there might be others). It would be > similarly beneficial in terms of testsuite coverage, and in terms of > avoiding the introduction of regressions in the kernel datapath or in > kernel/user space exchanges. In the past, support for Infiniband has come up a few times. I think if we started adding fake Ethernet headers that would drive us even further away from the possibility of being truly multi-protocol. (I'm still thinking about the earlier messages because I think there are some significant differences between the kernel netlink protocol and OpenFlow). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2 2/2] bfd: Require bfd control packet received in forwarding_if_rx mode.
Thanks for the review, pushed both patches to master and branch-2.2
On Wed, Apr 30, 2014 at 1:38 PM, Ethan Jackson wrote:
> Acked-by: Ethan Jackson
>
>
> On Tue, Apr 29, 2014 at 4:15 PM, Alex Wang wrote:
> > This commit adds a requirement that bfd session must receive at least
> > one bfd control packet every 100 * bfd->cfg_min_rx amount of time in
> > forwarding_if_rx mode. Otherwise, even if the data packets are received
> > on the monitored interface, the bfd->forwarding is still false.
> >
> > Since the datapath flow is not purged when the userspace Open Vswitch
> > crashes, data packet can still be forwarded through the tunnel and
> > fool the remote BFD session in forwarding_if_rx mode. Thus, this commit
> > can prevent the remote BFD session from falsely declaring tunnel liveness
> > in this situation.
> >
> > Signed-off-by: Alex Wang
> > ---
> > PATCH -> V2:
> > - hard code the demand_rx_bfd interval to be 100 * cfm_interval.
> > ---
> > lib/bfd.c| 27 ++
> > tests/bfd.at | 101
> ++
> > vswitchd/vswitch.xml | 11 --
> > 3 files changed, 105 insertions(+), 34 deletions(-)
> >
> > diff --git a/lib/bfd.c b/lib/bfd.c
> > index e3e3ae5..2d53bd2 100644
> > --- a/lib/bfd.c
> > +++ b/lib/bfd.c
> > @@ -203,6 +203,12 @@ struct bfd {
> > bool forwarding_if_rx;
> > long long int forwarding_if_rx_detect_time;
> >
> > +/* When 'bfd->forwarding_if_rx' is set, at least one bfd control
> packet
> > + * is required to be received every 100 * bfd->cfg_min_rx. If bfd
> > + * control packet is not received within this interval, even if data
> > + * packets are received, the bfd->forwarding will still be false. */
> > +long long int demand_rx_bfd_time;
> > +
> > /* BFD decay related variables. */
> > bool in_decay;/* True when bfd is in decay. */
> > int decay_min_rx; /* min_rx is set to decay_min_rx when
> */
> > @@ -841,6 +847,10 @@ bfd_process_packet(struct bfd *bfd, const struct
> flow *flow,
> > }
> > /* XXX: RFC 5880 Section 6.8.6 Demand mode related calculations
> here. */
> >
> > +if (bfd->forwarding_if_rx) {
> > +bfd->demand_rx_bfd_time = time_msec() + 100 * bfd->cfg_min_rx;
> > +}
> > +
> > out:
> > bfd_forwarding__(bfd);
> > ovs_mutex_unlock(&mutex);
> > @@ -876,19 +886,22 @@ bfd_set_netdev(struct bfd *bfd, const struct
> netdev *netdev)
> > static bool
> > bfd_forwarding__(struct bfd *bfd) OVS_REQUIRES(mutex)
> > {
> > -long long int time;
> > +long long int now = time_msec();
> > +bool forwarding_if_rx;
> > bool last_forwarding = bfd->last_forwarding;
> >
> > if (bfd->forwarding_override != -1) {
> > return bfd->forwarding_override == 1;
> > }
> >
> > -time = bfd->forwarding_if_rx_detect_time;
> > -bfd->last_forwarding = (bfd->state == STATE_UP
> > -|| (bfd->forwarding_if_rx && time >
> time_msec()))
> > -&& bfd->rmt_diag != DIAG_PATH_DOWN
> > -&& bfd->rmt_diag != DIAG_CPATH_DOWN
> > -&& bfd->rmt_diag != DIAG_RCPATH_DOWN;
> > +forwarding_if_rx = bfd->forwarding_if_rx
> > + && bfd->forwarding_if_rx_detect_time > now
> > + && bfd->demand_rx_bfd_time > now;
> > +
> > +bfd->last_forwarding = (bfd->state == STATE_UP || forwarding_if_rx)
> > + && bfd->rmt_diag != DIAG_PATH_DOWN
> > + && bfd->rmt_diag != DIAG_CPATH_DOWN
> > + && bfd->rmt_diag != DIAG_RCPATH_DOWN;
> > if (bfd->last_forwarding != last_forwarding) {
> > bfd->flap_count++;
> > bfd_status_changed(bfd);
> > diff --git a/tests/bfd.at b/tests/bfd.at
> > index 3723d60..f6f5592 100644
> > --- a/tests/bfd.at
> > +++ b/tests/bfd.at
> > @@ -401,10 +401,9 @@ AT_CLEANUP
> >
> > # forwarding_if_rx Test1
> > # Test1 tests the case when bfd is only enabled on one end of the link.
> > -# Under this situation, the bfd state should be DOWN and the forwarding
> > -# flag should be FALSE by default. However, if forwarding_if_rx is
> > -# enabled, as long as there is packet received, the bfd forwarding flag
> > -# should be TRUE.
> > +# Under this situation, the forwarding flag should always be false, even
> > +# though there is data packet received, since there is no bfd control
> > +# packet received during the demand_rx_bfd interval.
> > AT_SETUP([bfd - bfd forwarding_if_rx - bfd on one side])
> > OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy --
> \
> > add-port br1 p1 -- set Interface p1 type=patch \
> > @@ -438,15 +437,8 @@ do
> > AT_CHECK([ovs-ofctl packet-out br1 3 2
>
> "90e2ba01475000101856b2e80806000108000604000100101856b2e80202020302020202"],
> > [0], [stdout], [])
>
[ovs-dev] [PATCH] datapath: handle recirculation loop detection
Current datapath allows the same packet to be executed up to 5 times to
avoid blowing out the kernel stack. Recirculation is currently also
counted as one execution, but does not use same amount of stack compare
to other services, such as IPsec.
This patch introduces the concept of stack cost. Recirculation has a
stack cost of 1 while other services have stack cost of 4. Datapath
packet process accommodates packets that need both services and
recirculation as long as the total stack cost does not exceed 16.
Packets exceed stack cost of 16 will be treated as looped packets
and dropped.
The behavior of packets do not recirculate does not change.
Signed-off-by: Andy Zhou
---
datapath/actions.c | 28 ++--
datapath/datapath.c | 9 +
datapath/datapath.h | 4 ++--
3 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index 5871d82..5556a0c 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -536,7 +536,7 @@ static int execute_recirc(struct datapath *dp, struct
sk_buff *skb,
recirc_key.ovs_flow_hash = hash;
recirc_key.recirc_id = nla_get_u32(a);
- ovs_dp_process_packet_with_key(skb, &recirc_key);
+ ovs_dp_process_packet_with_key(skb, &recirc_key, true);
return 0;
}
@@ -631,11 +631,16 @@ static int do_execute_actions(struct datapath *dp, struct
sk_buff *skb,
}
/* We limit the number of times that we pass into execute_actions()
- * to avoid blowing out the stack in the event that we have a loop. */
-#define MAX_LOOPS 4
+ * to avoid blowing out the stack in the event that we have a loop.
+ *
+ * Each loop adds some (estimated) cost to the kernel stack.
+ * The loop terminates When max cost is reached. */
+#define MAX_STACK_COST 16
+#define RECIRC_STACK_COST 1
+#define DEFAULT_STACK_COST 4
struct loop_counter {
- u8 count; /* Count. */
+ u8 stack_cost; /* loop stack cost. */
bool looping; /* Loop detected? */
};
@@ -644,22 +649,24 @@ static DEFINE_PER_CPU(struct loop_counter, loop_counters);
static int loop_suppress(struct datapath *dp, struct sw_flow_actions *actions)
{
if (net_ratelimit())
- pr_warn("%s: flow looped %d times, dropping\n",
- ovs_dp_name(dp), MAX_LOOPS);
+ pr_warn("%s: flow looped detected, dropping\n",
+ ovs_dp_name(dp));
actions->actions_len = 0;
return -ELOOP;
}
/* Execute a list of actions against 'skb'. */
-int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
+int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, bool recirc)
{
struct sw_flow_actions *acts =
rcu_dereference(OVS_CB(skb)->flow->sf_acts);
+ const u8 stack_cost = recirc ? RECIRC_STACK_COST : DEFAULT_STACK_COST;
struct loop_counter *loop;
int error;
/* Check whether we've looped too much. */
loop = &__get_cpu_var(loop_counters);
- if (unlikely(++loop->count > MAX_LOOPS))
+ loop->stack_cost += stack_cost;
+ if (unlikely(loop->stack_cost > MAX_STACK_COST))
loop->looping = true;
if (unlikely(loop->looping)) {
error = loop_suppress(dp, acts);
@@ -676,8 +683,9 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff
*skb)
error = loop_suppress(dp, acts);
out_loop:
- /* Decrement loop counter. */
- if (!--loop->count)
+ /* Decrement loop stack cost. */
+ loop->stack_cost -= stack_cost;
+ if (!loop->stack_cost)
loop->looping = false;
return error;
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 10706f5..0611dad 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -241,7 +241,8 @@ void ovs_dp_detach_port(struct vport *p)
}
void ovs_dp_process_packet_with_key(struct sk_buff *skb,
- struct sw_flow_key *pkt_key)
+ struct sw_flow_key *pkt_key,
+ bool recirc)
{
const struct vport *p = OVS_CB(skb)->input_vport;
struct datapath *dp = p->dp;
@@ -272,7 +273,7 @@ void ovs_dp_process_packet_with_key(struct sk_buff *skb,
OVS_CB(skb)->flow = flow;
ovs_flow_stats_update(OVS_CB(skb)->flow, pkt_key->tp.flags, skb);
- ovs_execute_actions(dp, skb);
+ ovs_execute_actions(dp, skb, recirc);
stats_counter = &stats->n_hit;
out:
@@ -298,7 +299,7 @@ void ovs_dp_process_received_packet(struct vport *p, struct
sk_buff *skb)
return;
}
- ovs_dp_process_packet_with_key(skb, &key);
+ ovs_dp_process_packet_with_key(skb, &key, false);
}
int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb,
@@ -601,7 +602,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
struct genl_info *info)
OVS_CB(packet)->input_vport = input_vport
[ovs-dev] [PATCH] ofproto: Fix is_flow_deletion_pending() false positive.
If one tries to install a rule that's identical to another rule in
another OpenFlow table which is being deleted, it's possible that
is_flow_deletion_pending() might confuse them and block the
installation. This is such an edge case I doubt it has ever actually
happened.
Found by inspection.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 208efc1..eb1f3be 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3953,7 +3953,8 @@ is_flow_deletion_pending(const struct ofproto *ofproto,
HMAP_FOR_EACH_WITH_HASH (op, hmap_node,
cls_rule_hash(cls_rule, table_id),
&ofproto->deletions) {
-if (cls_rule_equal(cls_rule, &op->rule->cr)) {
+if (op->rule->table_id == table_id
+&& cls_rule_equal(cls_rule, &op->rule->cr)) {
return true;
}
}
--
1.8.1.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix in_port=controller case for NORMAL action
> On Wed, Apr 30, 2014 at 10:24:46AM +0900, YAMAMOTO Takashi wrote: >> The problem mentioned by Simon Horman in the following mail. >> http://openvswitch.org/pipermail/dev/2014-April/039492.html >> >> Cc: Simon Horman >> Signed-off-by: YAMAMOTO Takashi > > One of the most pointless differences between OF1.0 and later versions. > > Acked-by: Ben Pfaff applied to master. thanks. which release branches are "live" at this point? 1.9 and 2.2? YAMAMOTO Takashi ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
