Re: [ovs-dev] [PATCH] datapath: Fix compatibility with older kernel (<2.6.26).
Hi all, the patches solved the compilation errors. But when I insert the kernel module (insmod openvswitch.ko) the module doesn't load. In dmesg I get: *openvswitch: Unknown symbol ksize* Thanks, Andi On 6 March 2013 01:34, Jesse Gross wrote: > On Tue, Mar 5, 2013 at 3:44 PM, Pravin B Shelar > wrote: > > diff --git a/datapath/tunnel.c b/datapath/tunnel.c > > index 83d2c41..dbe4ebc 100644 > > --- a/datapath/tunnel.c > > +++ b/datapath/tunnel.c > > @@ -27,6 +27,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > > > @@ -203,6 +204,7 @@ error: > > return ERR_PTR(err); > > } > > > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,26) > > I'm somewhat inclined to provide a backport for > inet_get_local_port_range() for kernels older than 2.6.24 (which is > when it was actually introduced). If there isn't an easy alternate > way to get those values, then it can just statically return some > defaults. > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > -- - *University of Rome "La Sapienza"* *Ing Andi Palo* *PhD student* *Department of Computer, Control and Management Engineering (DIAG)* Via Ariosto, 25 - 00185 Rome - Italy Tel: +39 06 77274034 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Fix compatibility with older kernel (<2.6.26).
Hi, I solved even this error. That particular version of linux kernel did not export ksize symbol. I re-build the kernel with the patch to mm/slab.c. Practically I added EXPORT_SYMBOL(ksize); after the function definition in mm/slab.c. Andi On 6 March 2013 12:51, Palo Andi wrote: > Hi all, > the patches solved the compilation errors. > > But when I insert the kernel module (insmod openvswitch.ko) the module > doesn't load. > In dmesg I get: > *openvswitch: Unknown symbol ksize* > > Thanks, > Andi > > On 6 March 2013 01:34, Jesse Gross wrote: > >> On Tue, Mar 5, 2013 at 3:44 PM, Pravin B Shelar >> wrote: >> > diff --git a/datapath/tunnel.c b/datapath/tunnel.c >> > index 83d2c41..dbe4ebc 100644 >> > --- a/datapath/tunnel.c >> > +++ b/datapath/tunnel.c >> > @@ -27,6 +27,7 @@ >> > #include >> > #include >> > #include >> > +#include >> > #include >> > #include >> > >> > @@ -203,6 +204,7 @@ error: >> > return ERR_PTR(err); >> > } >> > >> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,26) >> >> I'm somewhat inclined to provide a backport for >> inet_get_local_port_range() for kernels older than 2.6.24 (which is >> when it was actually introduced). If there isn't an easy alternate >> way to get those values, then it can just statically return some >> defaults. >> ___ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >> > > > > -- > - > *University of Rome "La Sapienza"* > > *Ing Andi Palo* > *PhD student* > *Department of Computer, Control and Management Engineering (DIAG)* > > Via Ariosto, 25 - 00185 Rome - Italy > Tel: +39 06 77274034 > -- - *University of Rome "La Sapienza"* *Ing Andi Palo* *PhD student* *Department of Computer, Control and Management Engineering (DIAG)* Via Ariosto, 25 - 00185 Rome - Italy Tel: +39 06 77274034 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH 1/2] add igmp snooping support
On Tue, 2013-03-05 at 09:34 -0800, Ben Pfaff wrote: > On Fri, Mar 01, 2013 at 09:03:44PM +0800, Cong Wang wrote: > > WARNING: This patch *only* compiles! NOT tested yet! > > > > This patch adds the initial IGMP snooping support. > > I sent out this patch early to get some early reviews, > > especially the design of the code. > > > > Any comments are welcome! > > > > Cc: Ben Pfaff > > Cc: Jesse Gross > > Not-Yet-Signed-off-by: Cong Wang > > Hi Cong. IGMP snooping is a nice feature to have. The code looks OK at > first skim. Great, thanks for your positive feedback! > > One thing we've been trying to do as we introduce new features is to > try to make them useful apart from the "normal" action where we can. Do > you have an idea how that can be done for IGMP snooping? Not yet, currently I have no idea on how to integrate it with openflow tables. > > I'd prefer to avoid adding a new struct flow member for this case. I > hope we can reuse an existing member. > Yeah, makes sense, I will dig which members can be reused. Thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] OVS-VSWITCHD crashes when packet received from datapath
Hi all, I managed to cross-compile OVS 1.10 and install it on the mipsel router running dd-wrt kernel 2.6.23. After the patches OVS side and kernel side([1] [2]) it runs. But the error from [3] remains. This time it manifest as a Bus error. Gdb connected to gdbserver in target machine segfaults also in host machine(with no reason), so no backtrace. The only available thing is the log. 1970-01-01T01:20:00Z|00345|rconn|INFO|br0<->tcp:192.168.6.145:6633: connection timed out 1970-01-01T01:20:00Z|00346|rconn|INFO|br0<->tcp:192.168.6.145:6633: waiting 4 seconds before reconnect 1970-01-01T01:20:00Z|00347|rconn|DBG|br0<->tcp:192.168.6.145:6633: entering BACKOFF 1970-01-01T01:20:01Z|00348|poll_loop|DBG|wakeup due to [POLLIN] on fd 16 (unknown anon_inode:[eventpoll]) at lib/dpif-linux.c:1364 (3% CPU usage) 1970-01-01T01:20:01Z|2|poll_loop(worker)|DBG|wakeup due to [POLLIN][POLLHUP] on fd 12 (<->) at lib/worker.c:385 1970-01-01T01:20:01Z|3|worker(worker)|INFO|worker process exiting Bus error Any idea? Thanks, Andi [1] http://openvswitch.org/pipermail/dev/2013-March/025815.html [2] http://openvswitch.org/pipermail/dev/2013-March/025828.html [3] http://openvswitch.org/pipermail/dev/2013-March/025729.html -- - *University of Rome "La Sapienza"* *Ing Andi Palo* *PhD student* *Department of Computer, Control and Management Engineering (DIAG)* Via Ariosto, 25 - 00185 Rome - Italy Tel: +39 06 77274034 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OpenVSwitch and libvirt integration problem at shutdown/reboot
Sorry for the late response. On Mon, Mar 4, 2013 at 7:06 PM, Ansis Atteka wrote: > On Mon, Mar 4, 2013 at 12:08 PM, Ernesto Domato wrote: > > If you do not block on interface creation and libvirt/Open vSwitch > init.d dependencies are not right, then I think you might end up with > another race condition, where VM automatic start-up would fail. > Imagine that: > 1. Neither Open vSwitch or libvirt are running > 2. libvirt starts up > 3. libvirt tries to spin up VM and executes "ovs-vsctl --no-wait > --timeout=5 -- del-port ... -- add-port ..." command. After 5 seconds > this command times out, because Open vSwitch wasn't running > 4. After 6 seconds Open vSwitch starts up, but VM still remains down. > > This means that you will have to manually start the VM one more time. > Ok, so I guess that to solve this problem, the right solution would be that libvirt wait till OVS is up by not timing out, right? I think that could be a good idea, I'll write a new patch for libvirt then and send it to them for comments. >> >> I also added "--no-wait --timeout 5" when libvirt goes down so it can >> timeout if ovs-switch is down. > Just curious, but wasn't this part already solved with libvirt commit > 98e732fc34a47ad9dfdb64aa4207623ee4c1ebcd (network: prevent infinite > hang if ovs-vswitchd isn't running)? Are you using libvirt that has > this patch? > Ok, I'm using the stable version of libvirt and that fix is in experimental package. Anyway, this fix only adds the timeout flag when deleting the interface from OVS which does that libvirt don't hang trying to delete the interface if OVS is down. But it doesn't fix the issue that the OVS-DB still have the reference to the virtual interface and so, when you bring the virtual machine up again, it doesn't response because of this. As you (Ansis Atteka I guess) recommended before, my patch adds that libvirt try to delete (with the --if-exists flag) the interface before adding it again so that problem is resolved. > By the way I tried to execute the same commands as libvirt would have > executed. Except I ran them directly in the shell. What I observed is > that ovs-vsctl did not block indefinitely for this corner case: > > root@ubuntu:~# service openvswitch-switch stop > * ovs-brcompatd is not running > * Killing ovs-vswitchd (13202) > * Killing ovsdb-server (13193) > root@ubuntu:~# ovs-vsctl --timeout=5 -- --if-exists del-port p0 > Mar 04 13:39:14|2|stream_unix|ERR|/tmp/stream-unix.13222.0: > connection to /var/run/openvswitch/db.sock failed: No such file or > directory > Mar 04 13:39:14|3|reconnect|WARN|unix:/var/run/openvswitch/db.sock: > connection attempt failed (No such file or directory) > Mar 04 13:39:15|4|stream_unix|ERR|/tmp/stream-unix.13222.1: > connection to /var/run/openvswitch/db.sock failed: No such file or > directory > Mar 04 13:39:15|5|reconnect|WARN|unix:/var/run/openvswitch/db.sock: > connection attempt failed (No such file or directory) > Mar 04 13:39:17|6|stream_unix|ERR|/tmp/stream-unix.13222.2: > connection to /var/run/openvswitch/db.sock failed: No such file or > directory > Mar 04 13:39:17|7|reconnect|WARN|unix:/var/run/openvswitch/db.sock: > connection attempt failed (No such file or directory) > Alarm clock > root@ubuntu:~# > > I tried this with Open vSwitch: 1.4.3-0ubuntu2.1. Are you seeing the > same effect with your Open vSwitch? Yes, I see the same behavior but after applying the patch that adds the --timeout flag to the older version of libvirt that is currently in Debian as stable but is incorporated in experimental package. So, summarizing, what I'll do is to download the GIT version of libvirt, make patch that just tries to delete the interface before adding it again when the virtual machine is going up and do it in a separate call to ovs-vsctl so when adding the interface, it wait for OVS to be up. Is that the way to solve this problem?, what do you think? :-) Thanks Ernesto ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OVS-VSWITCHD crashes when packet received from datapath
On Wed, Mar 06, 2013 at 04:05:01PM +0100, Palo Andi wrote: > I managed to cross-compile OVS 1.10 and install it on the mipsel router > running dd-wrt kernel 2.6.23. > After the patches OVS side and kernel side([1] [2]) it runs. > But the error from [3] remains. This time it manifest as a Bus error. Gdb > connected to gdbserver in target machine segfaults also in host > machine(with no reason), so no backtrace. A bus error on a RISC system usually means a misaligned memory access. We don't get a lot of testing on RISC systems, so it's possible that such an access (or more than one) has crept in. If you do get a backtrace, let us know so that we can fix the problem. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] Add support for dec_mpls_ttl action
On Tue, Mar 5, 2013 at 11:08 PM, Simon Horman wrote:
> [ Cc Pravin B Shelar ]
>
> On Tue, Mar 05, 2013 at 06:11:27PM -0800, Ben Pfaff wrote:
>> On Wed, Mar 06, 2013 at 10:42:02AM +0900, Simon Horman wrote:
>> > On Tue, Mar 05, 2013 at 10:14:44AM -0800, Ben Pfaff wrote:
>> > > On Thu, Feb 28, 2013 at 06:15:07PM +0900, Simon Horman wrote:
>> > > > This adds support for the OpenFlow 1.1+ dec_mpls_ttl action.
>> > > > And also adds an NX dec_mpls_ttl action.
>> > > >
>> > > > The handling of the TTL modification is entirely handled in userspace.
>> > > >
>> > > > Reviewed-by: Isaku Yamahata
>> > > > Signed-off-by: Simon Horman
>> > >
>> > > The only issue I see with this is that it seems uncertain about what is
>> > > an invalid MPLS TTL. The code says that, pre-decrement, values 0 and 1
>> > > are invalid:
>> > >
>> > > +if (ttl > 1) {
>> > > +ttl--;
>> > > +set_mpls_lse_ttl(&ctx->flow.mpls_lse, ttl);
>> > > +return false;
>> > > +} else {
>> > > +execute_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, 0);
>> > >
>> > > The documentation says that, pre-decrement, value 0 is invalid:
>> > >
>> > > +.IP \fBdec_mpls_ttl\fR
>> > > +Decrement TTL of the outer MPLS label stack entry of a packet. If the
>> > > TTL
>> > > +is initially zero, no decrement occurs. Instead, a ``packet-in''
>> > > message
>> > >
>> > > I don't know MPLS, so I don't know which definition is correct.
>> >
>> > I am not sure and to be honest I had just followed dec_ttl implementation
>> > when adding the code. Reading up I think that section 2.3 of RFC3443
>> > implies this is the desired behaviour.
>> >
>> > I'm not sure why the documentation conflicts with the code, most likely
>> > it documents a previous incantation of the code. In any case I propose
>> > updating it rather than the code.
>>
>> OK, that makes sense, thank you.
>
> It seems to me that dec_ttl has the same problem.
>
> But it also seems to me that it is the code rather than the documentation
> that should be updated.
>
> The commit log for the addition of the TTL decrement action states:
>
> commit f0fd1a1772665ea57662281d9cccadb0f0146196
> Author: Pravin B Shelar
> Date: Fri Jan 13 17:54:04 2012 -0800
>
> ofproto: New action TTL decrement.
>
> Following patch implements dec_ttl as vendor action with similar
> semantics as OpenFlow 1.2. If TTL reaches zero while procession
> actions in current table, the remaining actions in previous tables
> are processed. A configuration parameter is added to make TTL
> decrement to zero generate packet in.
>
> Feature #8758
> Signed-off-by: Pravin B Shelar
>
> But the code features "if (ttl > 1)".
>
> That now seems like a logic bug to me (though perhaps the afternoon
> air is affecting my brain adversely).
>
>
> I have revised the dec_mpls_ttl patch to use "(ttl > 0)".
> Should I provide a patch to update dec_ttl?
I think you were right the first time - if a decrement causes the TTL
to hit zero then it should go to the exception case (in theory we
should never receive a packet with TTL zero here, so it's always an
exception). I believe this applies equally to IP and MPLS.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] Add support for dec_mpls_ttl action
On Wed, Mar 06, 2013 at 07:46:16AM -0800, Jesse Gross wrote:
> On Tue, Mar 5, 2013 at 11:08 PM, Simon Horman wrote:
> > [ Cc Pravin B Shelar ]
> >
> > On Tue, Mar 05, 2013 at 06:11:27PM -0800, Ben Pfaff wrote:
> >> On Wed, Mar 06, 2013 at 10:42:02AM +0900, Simon Horman wrote:
> >> > On Tue, Mar 05, 2013 at 10:14:44AM -0800, Ben Pfaff wrote:
> >> > > On Thu, Feb 28, 2013 at 06:15:07PM +0900, Simon Horman wrote:
> >> > > > This adds support for the OpenFlow 1.1+ dec_mpls_ttl action.
> >> > > > And also adds an NX dec_mpls_ttl action.
> >> > > >
> >> > > > The handling of the TTL modification is entirely handled in
> >> > > > userspace.
> >> > > >
> >> > > > Reviewed-by: Isaku Yamahata
> >> > > > Signed-off-by: Simon Horman
> >> > >
> >> > > The only issue I see with this is that it seems uncertain about what is
> >> > > an invalid MPLS TTL. The code says that, pre-decrement, values 0 and 1
> >> > > are invalid:
> >> > >
> >> > > +if (ttl > 1) {
> >> > > +ttl--;
> >> > > +set_mpls_lse_ttl(&ctx->flow.mpls_lse, ttl);
> >> > > +return false;
> >> > > +} else {
> >> > > +execute_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
> >> > > 0);
> >> > >
> >> > > The documentation says that, pre-decrement, value 0 is invalid:
> >> > >
> >> > > +.IP \fBdec_mpls_ttl\fR
> >> > > +Decrement TTL of the outer MPLS label stack entry of a packet. If
> >> > > the TTL
> >> > > +is initially zero, no decrement occurs. Instead, a ``packet-in''
> >> > > message
> >> > >
> >> > > I don't know MPLS, so I don't know which definition is correct.
> >> >
> >> > I am not sure and to be honest I had just followed dec_ttl implementation
> >> > when adding the code. Reading up I think that section 2.3 of RFC3443
> >> > implies this is the desired behaviour.
> >> >
> >> > I'm not sure why the documentation conflicts with the code, most likely
> >> > it documents a previous incantation of the code. In any case I propose
> >> > updating it rather than the code.
> >>
> >> OK, that makes sense, thank you.
> >
> > It seems to me that dec_ttl has the same problem.
> >
> > But it also seems to me that it is the code rather than the documentation
> > that should be updated.
> >
> > The commit log for the addition of the TTL decrement action states:
> >
> > commit f0fd1a1772665ea57662281d9cccadb0f0146196
> > Author: Pravin B Shelar
> > Date: Fri Jan 13 17:54:04 2012 -0800
> >
> > ofproto: New action TTL decrement.
> >
> > Following patch implements dec_ttl as vendor action with similar
> > semantics as OpenFlow 1.2. If TTL reaches zero while procession
> > actions in current table, the remaining actions in previous tables
> > are processed. A configuration parameter is added to make TTL
> > decrement to zero generate packet in.
> >
> > Feature #8758
> > Signed-off-by: Pravin B Shelar
> >
> > But the code features "if (ttl > 1)".
> >
> > That now seems like a logic bug to me (though perhaps the afternoon
> > air is affecting my brain adversely).
> >
> >
> > I have revised the dec_mpls_ttl patch to use "(ttl > 0)".
> > Should I provide a patch to update dec_ttl?
>
> I think you were right the first time - if a decrement causes the TTL
> to hit zero then it should go to the exception case (in theory we
> should never receive a packet with TTL zero here, so it's always an
> exception). I believe this applies equally to IP and MPLS.
My understanding is that, in IP, a host accepts packets with TTL=0, but
a router discards them. If that is correct, then decrementing a TTL to
0 should not discard the packet, only decrementing a TTL that is
initially zero.
I think that this isn't a very important question for the purpose of
accepting these patches. I'm going to do my best to adjust them to do
what I think is correct. If there is a bug, then we can fix it.
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] Add support for dec_mpls_ttl action
On Wed, Mar 6, 2013 at 8:21 AM, Ben Pfaff wrote:
> On Wed, Mar 06, 2013 at 07:46:16AM -0800, Jesse Gross wrote:
>> On Tue, Mar 5, 2013 at 11:08 PM, Simon Horman wrote:
>> > [ Cc Pravin B Shelar ]
>> >
>> > On Tue, Mar 05, 2013 at 06:11:27PM -0800, Ben Pfaff wrote:
>> >> On Wed, Mar 06, 2013 at 10:42:02AM +0900, Simon Horman wrote:
>> >> > On Tue, Mar 05, 2013 at 10:14:44AM -0800, Ben Pfaff wrote:
>> >> > > On Thu, Feb 28, 2013 at 06:15:07PM +0900, Simon Horman wrote:
>> >> > > > This adds support for the OpenFlow 1.1+ dec_mpls_ttl action.
>> >> > > > And also adds an NX dec_mpls_ttl action.
>> >> > > >
>> >> > > > The handling of the TTL modification is entirely handled in
>> >> > > > userspace.
>> >> > > >
>> >> > > > Reviewed-by: Isaku Yamahata
>> >> > > > Signed-off-by: Simon Horman
>> >> > >
>> >> > > The only issue I see with this is that it seems uncertain about what
>> >> > > is
>> >> > > an invalid MPLS TTL. The code says that, pre-decrement, values 0 and
>> >> > > 1
>> >> > > are invalid:
>> >> > >
>> >> > > +if (ttl > 1) {
>> >> > > +ttl--;
>> >> > > +set_mpls_lse_ttl(&ctx->flow.mpls_lse, ttl);
>> >> > > +return false;
>> >> > > +} else {
>> >> > > +execute_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
>> >> > > 0);
>> >> > >
>> >> > > The documentation says that, pre-decrement, value 0 is invalid:
>> >> > >
>> >> > > +.IP \fBdec_mpls_ttl\fR
>> >> > > +Decrement TTL of the outer MPLS label stack entry of a packet. If
>> >> > > the TTL
>> >> > > +is initially zero, no decrement occurs. Instead, a ``packet-in''
>> >> > > message
>> >> > >
>> >> > > I don't know MPLS, so I don't know which definition is correct.
>> >> >
>> >> > I am not sure and to be honest I had just followed dec_ttl
>> >> > implementation
>> >> > when adding the code. Reading up I think that section 2.3 of RFC3443
>> >> > implies this is the desired behaviour.
>> >> >
>> >> > I'm not sure why the documentation conflicts with the code, most likely
>> >> > it documents a previous incantation of the code. In any case I propose
>> >> > updating it rather than the code.
>> >>
>> >> OK, that makes sense, thank you.
>> >
>> > It seems to me that dec_ttl has the same problem.
>> >
>> > But it also seems to me that it is the code rather than the documentation
>> > that should be updated.
>> >
>> > The commit log for the addition of the TTL decrement action states:
>> >
>> > commit f0fd1a1772665ea57662281d9cccadb0f0146196
>> > Author: Pravin B Shelar
>> > Date: Fri Jan 13 17:54:04 2012 -0800
>> >
>> > ofproto: New action TTL decrement.
>> >
>> > Following patch implements dec_ttl as vendor action with similar
>> > semantics as OpenFlow 1.2. If TTL reaches zero while procession
>> > actions in current table, the remaining actions in previous tables
>> > are processed. A configuration parameter is added to make TTL
>> > decrement to zero generate packet in.
>> >
>> > Feature #8758
>> > Signed-off-by: Pravin B Shelar
>> >
>> > But the code features "if (ttl > 1)".
>> >
>> > That now seems like a logic bug to me (though perhaps the afternoon
>> > air is affecting my brain adversely).
>> >
>> >
>> > I have revised the dec_mpls_ttl patch to use "(ttl > 0)".
>> > Should I provide a patch to update dec_ttl?
>>
>> I think you were right the first time - if a decrement causes the TTL
>> to hit zero then it should go to the exception case (in theory we
>> should never receive a packet with TTL zero here, so it's always an
>> exception). I believe this applies equally to IP and MPLS.
>
> My understanding is that, in IP, a host accepts packets with TTL=0, but
> a router discards them. If that is correct, then decrementing a TTL to
> 0 should not discard the packet, only decrementing a TTL that is
> initially zero.
>From the IPv6 RFC (since the meaning has somewhat changed over time):
Hop Limit8-bit unsigned integer. Decremented by 1 by
each node that forwards the packet. The packet
is discarded if Hop Limit is decremented to
zero.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] Add support for dec_mpls_ttl action
On Wed, Mar 06, 2013 at 08:29:13AM -0800, Jesse Gross wrote:
> On Wed, Mar 6, 2013 at 8:21 AM, Ben Pfaff wrote:
> > On Wed, Mar 06, 2013 at 07:46:16AM -0800, Jesse Gross wrote:
> >> On Tue, Mar 5, 2013 at 11:08 PM, Simon Horman wrote:
> >> > [ Cc Pravin B Shelar ]
> >> >
> >> > On Tue, Mar 05, 2013 at 06:11:27PM -0800, Ben Pfaff wrote:
> >> >> On Wed, Mar 06, 2013 at 10:42:02AM +0900, Simon Horman wrote:
> >> >> > On Tue, Mar 05, 2013 at 10:14:44AM -0800, Ben Pfaff wrote:
> >> >> > > On Thu, Feb 28, 2013 at 06:15:07PM +0900, Simon Horman wrote:
> >> >> > > > This adds support for the OpenFlow 1.1+ dec_mpls_ttl action.
> >> >> > > > And also adds an NX dec_mpls_ttl action.
> >> >> > > >
> >> >> > > > The handling of the TTL modification is entirely handled in
> >> >> > > > userspace.
> >> >> > > >
> >> >> > > > Reviewed-by: Isaku Yamahata
> >> >> > > > Signed-off-by: Simon Horman
> >> >> > >
> >> >> > > The only issue I see with this is that it seems uncertain about
> >> >> > > what is
> >> >> > > an invalid MPLS TTL. The code says that, pre-decrement, values 0
> >> >> > > and 1
> >> >> > > are invalid:
> >> >> > >
> >> >> > > +if (ttl > 1) {
> >> >> > > +ttl--;
> >> >> > > +set_mpls_lse_ttl(&ctx->flow.mpls_lse, ttl);
> >> >> > > +return false;
> >> >> > > +} else {
> >> >> > > +execute_controller_action(ctx, UINT16_MAX,
> >> >> > > OFPR_INVALID_TTL, 0);
> >> >> > >
> >> >> > > The documentation says that, pre-decrement, value 0 is invalid:
> >> >> > >
> >> >> > > +.IP \fBdec_mpls_ttl\fR
> >> >> > > +Decrement TTL of the outer MPLS label stack entry of a packet. If
> >> >> > > the TTL
> >> >> > > +is initially zero, no decrement occurs. Instead, a ``packet-in''
> >> >> > > message
> >> >> > >
> >> >> > > I don't know MPLS, so I don't know which definition is correct.
> >> >> >
> >> >> > I am not sure and to be honest I had just followed dec_ttl
> >> >> > implementation
> >> >> > when adding the code. Reading up I think that section 2.3 of RFC3443
> >> >> > implies this is the desired behaviour.
> >> >> >
> >> >> > I'm not sure why the documentation conflicts with the code, most
> >> >> > likely
> >> >> > it documents a previous incantation of the code. In any case I propose
> >> >> > updating it rather than the code.
> >> >>
> >> >> OK, that makes sense, thank you.
> >> >
> >> > It seems to me that dec_ttl has the same problem.
> >> >
> >> > But it also seems to me that it is the code rather than the documentation
> >> > that should be updated.
> >> >
> >> > The commit log for the addition of the TTL decrement action states:
> >> >
> >> > commit f0fd1a1772665ea57662281d9cccadb0f0146196
> >> > Author: Pravin B Shelar
> >> > Date: Fri Jan 13 17:54:04 2012 -0800
> >> >
> >> > ofproto: New action TTL decrement.
> >> >
> >> > Following patch implements dec_ttl as vendor action with similar
> >> > semantics as OpenFlow 1.2. If TTL reaches zero while procession
> >> > actions in current table, the remaining actions in previous tables
> >> > are processed. A configuration parameter is added to make TTL
> >> > decrement to zero generate packet in.
> >> >
> >> > Feature #8758
> >> > Signed-off-by: Pravin B Shelar
> >> >
> >> > But the code features "if (ttl > 1)".
> >> >
> >> > That now seems like a logic bug to me (though perhaps the afternoon
> >> > air is affecting my brain adversely).
> >> >
> >> >
> >> > I have revised the dec_mpls_ttl patch to use "(ttl > 0)".
> >> > Should I provide a patch to update dec_ttl?
> >>
> >> I think you were right the first time - if a decrement causes the TTL
> >> to hit zero then it should go to the exception case (in theory we
> >> should never receive a packet with TTL zero here, so it's always an
> >> exception). I believe this applies equally to IP and MPLS.
> >
> > My understanding is that, in IP, a host accepts packets with TTL=0, but
> > a router discards them. If that is correct, then decrementing a TTL to
> > 0 should not discard the packet, only decrementing a TTL that is
> > initially zero.
>
> From the IPv6 RFC (since the meaning has somewhat changed over time):
>
>Hop Limit8-bit unsigned integer. Decremented by 1 by
> each node that forwards the packet. The packet
> is discarded if Hop Limit is decremented to
> zero.
The requirements are a little confusing. For example, RFC 1122 states:
A host MUST NOT discard a datagram just because it was
received with TTL less than 2.
However, RFC 1009 is very clear:
If the TTL is reduced to zero, the datagram must be discarded, and
the gateway may send an ICMP Time Exceeded message to the source.
A datagram should never be received with a TTL of zero.
so I agree with you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listi
Re: [ovs-dev] [PATCH 1/3] Add support for dec_mpls_ttl action
On Wed, Mar 06, 2013 at 08:40:38AM -0800, Ben Pfaff wrote:
> On Wed, Mar 06, 2013 at 08:29:13AM -0800, Jesse Gross wrote:
> > On Wed, Mar 6, 2013 at 8:21 AM, Ben Pfaff wrote:
> > > On Wed, Mar 06, 2013 at 07:46:16AM -0800, Jesse Gross wrote:
> > >> On Tue, Mar 5, 2013 at 11:08 PM, Simon Horman wrote:
> > >> > [ Cc Pravin B Shelar ]
> > >> >
> > >> > On Tue, Mar 05, 2013 at 06:11:27PM -0800, Ben Pfaff wrote:
> > >> >> On Wed, Mar 06, 2013 at 10:42:02AM +0900, Simon Horman wrote:
> > >> >> > On Tue, Mar 05, 2013 at 10:14:44AM -0800, Ben Pfaff wrote:
> > >> >> > > On Thu, Feb 28, 2013 at 06:15:07PM +0900, Simon Horman wrote:
> > >> >> > > > This adds support for the OpenFlow 1.1+ dec_mpls_ttl action.
> > >> >> > > > And also adds an NX dec_mpls_ttl action.
> > >> >> > > >
> > >> >> > > > The handling of the TTL modification is entirely handled in
> > >> >> > > > userspace.
> > >> >> > > >
> > >> >> > > > Reviewed-by: Isaku Yamahata
> > >> >> > > > Signed-off-by: Simon Horman
> > >> >> > >
> > >> >> > > The only issue I see with this is that it seems uncertain about
> > >> >> > > what is
> > >> >> > > an invalid MPLS TTL. The code says that, pre-decrement, values 0
> > >> >> > > and 1
> > >> >> > > are invalid:
> > >> >> > >
> > >> >> > > +if (ttl > 1) {
> > >> >> > > +ttl--;
> > >> >> > > +set_mpls_lse_ttl(&ctx->flow.mpls_lse, ttl);
> > >> >> > > +return false;
> > >> >> > > +} else {
> > >> >> > > +execute_controller_action(ctx, UINT16_MAX,
> > >> >> > > OFPR_INVALID_TTL, 0);
> > >> >> > >
> > >> >> > > The documentation says that, pre-decrement, value 0 is invalid:
> > >> >> > >
> > >> >> > > +.IP \fBdec_mpls_ttl\fR
> > >> >> > > +Decrement TTL of the outer MPLS label stack entry of a packet.
> > >> >> > > If the TTL
> > >> >> > > +is initially zero, no decrement occurs. Instead, a
> > >> >> > > ``packet-in'' message
> > >> >> > >
> > >> >> > > I don't know MPLS, so I don't know which definition is correct.
> > >> >> >
> > >> >> > I am not sure and to be honest I had just followed dec_ttl
> > >> >> > implementation
> > >> >> > when adding the code. Reading up I think that section 2.3 of RFC3443
> > >> >> > implies this is the desired behaviour.
> > >> >> >
> > >> >> > I'm not sure why the documentation conflicts with the code, most
> > >> >> > likely
> > >> >> > it documents a previous incantation of the code. In any case I
> > >> >> > propose
> > >> >> > updating it rather than the code.
> > >> >>
> > >> >> OK, that makes sense, thank you.
> > >> >
> > >> > It seems to me that dec_ttl has the same problem.
> > >> >
> > >> > But it also seems to me that it is the code rather than the
> > >> > documentation
> > >> > that should be updated.
> > >> >
> > >> > The commit log for the addition of the TTL decrement action states:
> > >> >
> > >> > commit f0fd1a1772665ea57662281d9cccadb0f0146196
> > >> > Author: Pravin B Shelar
> > >> > Date: Fri Jan 13 17:54:04 2012 -0800
> > >> >
> > >> > ofproto: New action TTL decrement.
> > >> >
> > >> > Following patch implements dec_ttl as vendor action with similar
> > >> > semantics as OpenFlow 1.2. If TTL reaches zero while procession
> > >> > actions in current table, the remaining actions in previous tables
> > >> > are processed. A configuration parameter is added to make TTL
> > >> > decrement to zero generate packet in.
> > >> >
> > >> > Feature #8758
> > >> > Signed-off-by: Pravin B Shelar
> > >> >
> > >> > But the code features "if (ttl > 1)".
> > >> >
> > >> > That now seems like a logic bug to me (though perhaps the afternoon
> > >> > air is affecting my brain adversely).
> > >> >
> > >> >
> > >> > I have revised the dec_mpls_ttl patch to use "(ttl > 0)".
> > >> > Should I provide a patch to update dec_ttl?
> > >>
> > >> I think you were right the first time - if a decrement causes the TTL
> > >> to hit zero then it should go to the exception case (in theory we
> > >> should never receive a packet with TTL zero here, so it's always an
> > >> exception). I believe this applies equally to IP and MPLS.
> > >
> > > My understanding is that, in IP, a host accepts packets with TTL=0, but
> > > a router discards them. If that is correct, then decrementing a TTL to
> > > 0 should not discard the packet, only decrementing a TTL that is
> > > initially zero.
> >
> > From the IPv6 RFC (since the meaning has somewhat changed over time):
> >
> >Hop Limit8-bit unsigned integer. Decremented by 1 by
> > each node that forwards the packet. The packet
> > is discarded if Hop Limit is decremented to
> > zero.
>
> The requirements are a little confusing. For example, RFC 1122 states:
>
> A host MUST NOT discard a datagram just because it was
> received with TTL less than 2.
>
> However, RFC 1009 is very clear:
>
> If the TTL is reduced to zero, the datag
Re: [ovs-dev] [PATCH 2/3] Add support for set_mpls_ttl action
On Wed, Mar 06, 2013 at 04:08:12PM +0900, Simon Horman wrote: > On Wed, Mar 06, 2013 at 10:43:45AM +0900, Simon Horman wrote: > > On Tue, Mar 05, 2013 at 10:23:10AM -0800, Ben Pfaff wrote: > > > On Thu, Feb 28, 2013 at 06:15:08PM +0900, Simon Horman wrote: > > > > This adds support for the OpenFlow 1.1+ set_mpls_ttl action. > > > > And also adds an NX set_mpls_ttl action. > > > > > > > > The handling of the TTL modification is entirely handled in userspace. > > > > > > > > Reviewed-by: Isaku Yamahata > > > > Signed-off-by: Simon Horman > > > > > > I don't think that OpenFlow says that we have to send a packet to the > > > controller for set_mpls_ttl(0). I think it would be a bit surprising if > > > we did. > > > > Thanks, I will fix that. > > Updated patch is below. I applied patches 1 and 2 to master without any changes except a NEWS update. When Bruce gets back to us about the semantics of TTL in MPLS, we can fine-tune that (or not, as necessary). Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 3/3] datapath: Add basic MPLS support to kernel
On Tue, Mar 5, 2013 at 10:03 PM, Simon Horman wrote: > On Tue, Mar 05, 2013 at 11:35:41AM -0800, Jesse Gross wrote: >> On Tue, Mar 5, 2013 at 10:23 AM, Ben Pfaff wrote: >> > On Thu, Feb 28, 2013 at 06:15:09PM +0900, Simon Horman wrote: >> >> Allow datapath to recognize and extract MPLS labels into flow keys >> >> and execute actions which push, pop, and set labels on packets. >> >> >> >> Based heavily on work by Leo Alterman and Ravi K. >> >> >> >> Cc: Ravi K >> >> Cc: Leo Alterman >> >> Reviewed-by: Isaku Yamahata >> >> Signed-off-by: Simon Horman >> > >> > Jesse needs to review this. >> >> I was hoping that Simon would update the patch based on the discussion >> in the thread before I take another look at it. > > Hi Jesse, > > I'm not quite sure which issues you would like me to look at. > > There are number of changes in v2.20 (this version) of the patch > which address the very helpful review you supplied. > > On my todo list I have the following: > > * Exercise checksum code in pop_mpls(), push_mpls() and set_mpls() > I am unsure how to pass through or simulate skbs with CHECKSUM_COMPLETE. > * Enhance core kernel code to handle GSO for MPLS or > otherwise deal with accelerations. (Linux network core) > * Add ETH_TYPE_MIN or similar to Linux network core > * Enhance ovs-vswitchd to output actions in order to > allow use of network_header in the datapath to track the top > of the MPLS stack. > > I'd appreciate some guidance on how to achieve the first item on the list. I wouldn't worry that much about it. There are a few NICs that will generate packets like this but they are fairly rare at this point and simulating it seems like more trouble than it is worth. I'll review it and that should be good enough. > And the rest of the items seem like future work to me. > > Am I missing some items on my list? The main one that I was referring to is the last one on the list since I think that it will allow us to simplify things somewhat. GSO and other offloads are also somewhat problematic since it means that until we have that core kernel support OVS will create invalid packets in some situations. The exact same situation happened previously with vlans, so there is some GSO emulation code in vport-netdev.c. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] Add table_id to NXM flow_removed messages.
Feature #15466.
Requested-by: Ronghua Zhang
Signed-off-by: Ben Pfaff
---
NEWS |2 ++
include/openflow/nicira-ext.h | 10 --
lib/ofp-util.c|3 ++-
tests/ofp-print.at|4 ++--
4 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/NEWS b/NEWS
index 7fb0932..a33c14a 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,8 @@ post-v1.10.0
- OpenFlow:
* The "dec_mpls_ttl" and "set_mpls_ttl" actions from OpenFlow
1.1 and later are now implemented.
+ * The NXM flow_removed message now reports the OpenFlow table ID
+from which the flow was removed.
v1.10.0 - xx xxx
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index cdb1952..a1e5b58 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -1770,12 +1770,18 @@ struct nx_flow_mod {
};
OFP_ASSERT(sizeof(struct nx_flow_mod) == 32);
-/* NXT_FLOW_REMOVED (analogous to OFPT_FLOW_REMOVED). */
+/* NXT_FLOW_REMOVED (analogous to OFPT_FLOW_REMOVED).
+ *
+ * 'table_id' is present only in Open vSwitch 1.11 and later. In earlier
+ * versions of Open vSwitch, this is a padding byte that is always zeroed.
+ * Therefore, a 'table_id' value of 0 indicates that the table ID is not known,
+ * and other values may be interpreted as one more than the flow's former table
+ * ID. */
struct nx_flow_removed {
ovs_be64 cookie; /* Opaque controller-issued identifier. */
ovs_be16 priority;/* Priority level of flow entry. */
uint8_t reason; /* One of OFPRR_*. */
-uint8_t pad[1]; /* Align to 32-bits. */
+uint8_t table_id; /* Flow's former table ID, plus one. */
ovs_be32 duration_sec;/* Time flow was alive in seconds. */
ovs_be32 duration_nsec; /* Time flow was alive in nanoseconds beyond
duration_sec. */
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index dd6eed8..06e149a 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -2345,7 +2345,7 @@ ofputil_decode_flow_removed(struct ofputil_flow_removed
*fr,
fr->priority = ntohs(nfr->priority);
fr->cookie = nfr->cookie;
fr->reason = nfr->reason;
-fr->table_id = 255;
+fr->table_id = nfr->table_id ? nfr->table_id - 1 : 255;
fr->duration_sec = ntohl(nfr->duration_sec);
fr->duration_nsec = ntohl(nfr->duration_nsec);
fr->idle_timeout = ntohs(nfr->idle_timeout);
@@ -2424,6 +2424,7 @@ ofputil_encode_flow_removed(const struct
ofputil_flow_removed *fr,
nfr->cookie = fr->cookie;
nfr->priority = htons(fr->priority);
nfr->reason = fr->reason;
+nfr->table_id = fr->table_id + 1;
nfr->duration_sec = htonl(fr->duration_sec);
nfr->duration_nsec = htonl(fr->duration_nsec);
nfr->idle_timeout = htons(fr->idle_timeout);
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index 2939125..35f599c 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -1699,7 +1699,7 @@ AT_SETUP([NXT_FLOW_REMOVED])
AT_KEYWORDS([ofp-print])
AT_CHECK([ovs-ofctl ofp-print "\
01 04 00 78 00 00 00 00 00 00 23 20 00 00 00 0e \
-00 00 00 00 00 00 00 00 ff ff 00 00 00 00 00 06 \
+00 00 00 00 00 00 00 00 ff ff 00 02 00 00 00 06 \
01 6e 36 00 00 05 00 3c 00 00 00 00 00 00 00 01 \
00 00 00 00 00 00 00 3c 00 00 00 02 00 03 00 00 \
02 06 50 54 00 00 00 06 00 00 04 06 50 54 00 00 \
@@ -1707,7 +1707,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
1e 02 00 02 00 00 20 04 c0 a8 00 01 00 00 22 04 \
c0 a8 00 02 00 00 00 00 \
"], [0], [dnl
-NXT_FLOW_REMOVED (xid=0x0):
priority=65535,arp,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2
reason=idle duration6.024s idle5 pkts1 bytes60
+NXT_FLOW_REMOVED (xid=0x0):
priority=65535,arp,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2
reason=idle table_id=1 duration6.024s idle5 pkts1 bytes60
])
AT_CLEANUP
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] Add support for dec_mpls_ttl action
The processing for MPLS TTL is somewhat complex compared to IP, because there
are a few different options for what is acceptable when pushing and popping
labels. However, one thing that DOES carry forward straightforwardly from IP is
that if the TTL reaches zero, the packet MUST be discarded. This is documented
in RFC 1812, Section 5.3.
If you want to see all the gorey details about MPLS TTL processing, you need to
read RFC 3443.
Bruce
On Mar 6, 2013, at 8:54 AM, Ben Pfaff wrote:
> On Wed, Mar 06, 2013 at 08:40:38AM -0800, Ben Pfaff wrote:
>> On Wed, Mar 06, 2013 at 08:29:13AM -0800, Jesse Gross wrote:
>>> On Wed, Mar 6, 2013 at 8:21 AM, Ben Pfaff wrote:
On Wed, Mar 06, 2013 at 07:46:16AM -0800, Jesse Gross wrote:
> On Tue, Mar 5, 2013 at 11:08 PM, Simon Horman wrote:
>> [ Cc Pravin B Shelar ]
>>
>> On Tue, Mar 05, 2013 at 06:11:27PM -0800, Ben Pfaff wrote:
>>> On Wed, Mar 06, 2013 at 10:42:02AM +0900, Simon Horman wrote:
On Tue, Mar 05, 2013 at 10:14:44AM -0800, Ben Pfaff wrote:
> On Thu, Feb 28, 2013 at 06:15:07PM +0900, Simon Horman wrote:
>> This adds support for the OpenFlow 1.1+ dec_mpls_ttl action.
>> And also adds an NX dec_mpls_ttl action.
>>
>> The handling of the TTL modification is entirely handled in
>> userspace.
>>
>> Reviewed-by: Isaku Yamahata
>> Signed-off-by: Simon Horman
>
> The only issue I see with this is that it seems uncertain about what
> is
> an invalid MPLS TTL. The code says that, pre-decrement, values 0 and
> 1
> are invalid:
>
> +if (ttl > 1) {
> +ttl--;
> +set_mpls_lse_ttl(&ctx->flow.mpls_lse, ttl);
> +return false;
> +} else {
> +execute_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
> 0);
>
> The documentation says that, pre-decrement, value 0 is invalid:
>
> +.IP \fBdec_mpls_ttl\fR
> +Decrement TTL of the outer MPLS label stack entry of a packet. If
> the TTL
> +is initially zero, no decrement occurs. Instead, a ``packet-in''
> message
>
> I don't know MPLS, so I don't know which definition is correct.
I am not sure and to be honest I had just followed dec_ttl
implementation
when adding the code. Reading up I think that section 2.3 of RFC3443
implies this is the desired behaviour.
I'm not sure why the documentation conflicts with the code, most likely
it documents a previous incantation of the code. In any case I propose
updating it rather than the code.
>>>
>>> OK, that makes sense, thank you.
>>
>> It seems to me that dec_ttl has the same problem.
>>
>> But it also seems to me that it is the code rather than the documentation
>> that should be updated.
>>
>> The commit log for the addition of the TTL decrement action states:
>>
>>commit f0fd1a1772665ea57662281d9cccadb0f0146196
>>Author: Pravin B Shelar
>>Date: Fri Jan 13 17:54:04 2012 -0800
>>
>>ofproto: New action TTL decrement.
>>
>>Following patch implements dec_ttl as vendor action with similar
>>semantics as OpenFlow 1.2. If TTL reaches zero while procession
>>actions in current table, the remaining actions in previous tables
>>are processed. A configuration parameter is added to make TTL
>>decrement to zero generate packet in.
>>
>>Feature #8758
>>Signed-off-by: Pravin B Shelar
>>
>> But the code features "if (ttl > 1)".
>>
>> That now seems like a logic bug to me (though perhaps the afternoon
>> air is affecting my brain adversely).
>>
>>
>> I have revised the dec_mpls_ttl patch to use "(ttl > 0)".
>> Should I provide a patch to update dec_ttl?
>
> I think you were right the first time - if a decrement causes the TTL
> to hit zero then it should go to the exception case (in theory we
> should never receive a packet with TTL zero here, so it's always an
> exception). I believe this applies equally to IP and MPLS.
My understanding is that, in IP, a host accepts packets with TTL=0, but
a router discards them. If that is correct, then decrementing a TTL to
0 should not discard the packet, only decrementing a TTL that is
initially zero.
>>>
>>> From the IPv6 RFC (since the meaning has somewhat changed over time):
>>>
>>> Hop Limit8-bit unsigned integer. Decremented by 1 by
>>>each node that forwards the packet. The packet
>>>is discarded if Hop Limit is decremented to
>>>zero.
>>
>> The requirements are a little confusing. For example, RFC 1
Re: [ovs-dev] [bug15171 1/4] ovsdb-idlc: Make no-op writes to write-only columns cheaper.
On Tue, Mar 05, 2013 at 10:44:48PM -0800, Ethan Jackson wrote: > > The underscores style is what we've adopted for new code in the python > > directory. I think I would rather go back and change the old code to > > conform, than to write new code to fit in with whatever code it is near. > > Fine with me, I don't think it's worth the time to go back and change > things, but feel free if you'd like to. Since you're not pushing on it I doubt I'll get to it soon. It does slightly bug me. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [bug15171 4/4] ofproto: Merge all the CFM query functions into one.
On Tue, Mar 05, 2013 at 09:40:18PM -0800, Ben Pfaff wrote:
> On Tue, Mar 05, 2013 at 05:02:25PM -0800, Ethan Jackson wrote:
> > I think cfm_get_status() deserves a comment in ofproto-provider.h and
> > ofproto.c, more out of convention than anything else.
>
> Oops. I overlooked that. I'll take care of that in the morning and
> push this to master. Thanks for the reviews.
I applied this incremental and I'll push it soon:
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 3b17a73..d8db3ae 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1120,6 +1120,13 @@ struct ofproto_class {
* support CFM, as does a null pointer. */
int (*set_cfm)(struct ofport *ofport, const struct cfm_settings *s);
+/* Checks the status of CFM configured on 'ofport'. Returns true if the
+ * port's CFM status was successfully stored into '*status'. Returns false
+ * if the port did not have CFM configured, in which case '*status' is
+ * indeterminate.
+ *
+ * The caller must provide and owns '*status', but it does not own and must
+ * not modify or free the array returned in 'status->rmps'. */
bool (*get_cfm_status)(const struct ofport *ofport,
struct ofproto_cfm_status *status);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index bf27179..03ca59b 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2843,6 +2843,13 @@ ofproto_get_netflow_ids(const struct ofproto *ofproto,
ofproto->ofproto_class->get_netflow_ids(ofproto, engine_type, engine_id);
}
+/* Checks the status of CFM configured on 'ofp_port' within 'ofproto'. Returns
+ * true if the port's CFM status was successfully stored into '*status'.
+ * Returns false if the port did not have CFM configured, in which case
+ * '*status' is indeterminate.
+ *
+ * The caller must provide and owns '*status', but it does not own and must not
+ * modify or free the array returned in 'status->rmps'. */
bool
ofproto_port_get_cfm_status(const struct ofproto *ofproto, uint16_t ofp_port,
struct ofproto_cfm_status *status)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OVS-VSWITCHD crashes when packet received from datapath
Anyway, I have tested OVS version 1.4.0 and 1.4.1 and they work like a charm :) Thanks, Andi On 6 March 2013 16:45, Ben Pfaff wrote: > On Wed, Mar 06, 2013 at 04:05:01PM +0100, Palo Andi wrote: > > I managed to cross-compile OVS 1.10 and install it on the mipsel router > > running dd-wrt kernel 2.6.23. > > After the patches OVS side and kernel side([1] [2]) it runs. > > But the error from [3] remains. This time it manifest as a Bus error. Gdb > > connected to gdbserver in target machine segfaults also in host > > machine(with no reason), so no backtrace. > > A bus error on a RISC system usually means a misaligned memory access. > We don't get a lot of testing on RISC systems, so it's possible that > such an access (or more than one) has crept in. > > If you do get a backtrace, let us know so that we can fix the problem. > > Thanks, > > Ben. > -- - *University of Rome "La Sapienza"* *Ing Andi Palo* *PhD student* *Department of Computer, Control and Management Engineering (DIAG)* Via Ariosto, 25 - 00185 Rome - Italy Tel: +39 06 77274034 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] nicira-ext: Add Nicira actions NXAST_STACK_PUSH and NXAST_STACK_POP.
The Push action takes a single parameter. Any source allowed by NXAST_REG_MOVE
is allowed to be pushed onto the stack. When the source is a bit field,
its value will be right shifted to bit zero before being pushed onto the
stack. The remaining bits will be set to zero.
The Pop action also takes a single parameter. Any destination allowed by
NXAST_REG_MOVE can be used as the destination of the action. The value, in
case of a bit field, will be taken from top of the stack, starting from
bit zero.
The stack size is not limited. The initial 8KB is statically allocated to
efficiently handle most common use cases. When more stack space is
required, the stack can grow using malloc().
Signed-off-by: Andy Zhou
---
NEWS |2 +
include/openflow/nicira-ext.h | 19 ++
lib/nx-match.c| 150 +
lib/nx-match.h| 26 +++
lib/ofp-actions.c | 44 +++-
lib/ofp-actions.h | 10 +++
lib/ofp-parse.c |6 ++
lib/ofp-util.def |2 +
ofproto/ofproto-dpif.c| 29
tests/ofproto-dpif.at | 20 ++
tests/ovs-ofctl.at|2 +
utilities/ovs-ofctl.8.in | 17 +
12 files changed, 324 insertions(+), 3 deletions(-)
diff --git a/NEWS b/NEWS
index 35b9212..222f240 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,8 @@ post-v1.10.0
- New support for the data encapsulation format of the LISP tunnel
protocol (RFC 6830). An external control plane or manual flow
setup is required for EID-to-RLOC mapping.
+- New "stack" extension for use in actions, to push and pop
+ from NXM fields.
v1.10.0 - xx xxx
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index c4ff904..c6b5035 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -306,6 +306,8 @@ enum nx_action_subtype {
NXAST_WRITE_METADATA, /* struct nx_action_write_metadata */
NXAST_PUSH_MPLS,/* struct nx_action_push_mpls */
NXAST_POP_MPLS, /* struct nx_action_pop_mpls */
+NXAST_STACK_PUSH, /* struct nx_action_stack */
+NXAST_STACK_POP,/* struct nx_action_stack */
};
/* Header for Nicira-defined actions. */
@@ -560,6 +562,23 @@ struct nx_action_reg_load {
};
OFP_ASSERT(sizeof(struct nx_action_reg_load) == 24);
+/* Action structure for NXAST_STACK_PUSH and NXAST_STACK_POP.
+ *
+ * Pushes (or pop) field[offset: offset + n_bits] to (or from)
+ * top of the stack.
+ */
+struct nx_action_stack {
+ovs_be16 type; /* OFPAT_VENDOR. */
+ovs_be16 len; /* Length is 16. */
+ovs_be32 vendor;/* NX_VENDOR_ID. */
+ovs_be16 subtype; /* NXAST_REG_PUSH or NXAST_REG_POP. */
+ovs_be16 offset;/* Bit offset into the field. */
+ovs_be32 field; /* The field used for push or pop. */
+ovs_be16 n_bits;/* (n_bits + 1) bits of the field. */
+uint8_t zero[6];/* Reserved for future use, must be zero.*/
+};
+OFP_ASSERT(sizeof(struct nx_action_stack) == 24);
+
/* Action structure for NXAST_NOTE.
*
* This action has no effect. It is variable length. The switch does not
diff --git a/lib/nx-match.c b/lib/nx-match.c
index e5545de..f4a93a0 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1312,3 +1312,153 @@ nxm_reg_load(const struct mf_subfield *dst, uint64_t
src_data,
sizeof src_data_be * 8);
mf_write_subfield_flow(dst, &src_subvalue, flow);
}
+
+/* nxm_parse_stack_action, works for both push() and pop(). */
+void
+nxm_parse_stack_action(struct ofpact_stack *stack_action, const char *s)
+{
+s = mf_parse_subfield(&stack_action->subfield, s);
+if (*s != '\0') {
+ovs_fatal(0, "%s: trailing garbage following push or pop", s);
+}
+}
+
+void
+nxm_format_stack_push(const struct ofpact_stack *push, struct ds *s)
+{
+ds_put_cstr(s, "push:");
+mf_format_subfield(&push->subfield, s);
+}
+
+void
+nxm_format_stack_pop(const struct ofpact_stack *pop, struct ds *s)
+{
+ds_put_cstr(s, "pop:");
+mf_format_subfield(&pop->subfield, s);
+}
+
+/* Common set for both push and pop actions. */
+static void
+stack_action_from_openflow__(const struct nx_action_stack *nasp,
+struct ofpact_stack *stack_action)
+{
+stack_action->subfield.field = mf_from_nxm_header(ntohl(nasp->field));
+stack_action->subfield.ofs = ntohs(nasp->offset);
+stack_action->subfield.n_bits = ntohs(nasp->n_bits);
+}
+
+static void
+nxm_stack_to_nxast__(const struct ofpact_stack *stack_action,
+struct nx_action_stack *nasp)
+{
+nasp->offset = htons(stack_action->subfield.ofs);
+nasp->n_bits = htons(stack_action->subfield.n_bits);
+nasp->field = htonl(stack_action->subfield.field->nxm
Re: [ovs-dev] [PATCH v7] Tunnel: Cleanup old tunnel infrastructure.
On Mon, Mar 4, 2013 at 1:35 PM, Pravin Shelar wrote: > On Thu, Feb 28, 2013 at 3:58 PM, Jesse Gross wrote: >> On Thu, Feb 28, 2013 at 10:27 AM, Pravin B Shelar wrote: >>> Since userspace flow based tunneling code is checked in, the kernel >>> port based tunneling code can be removed. >>> >>> Patch removes following components: >>> - tunnel ports hash table and moved tunnel ports list to individual >>>vports. >>> - Cleaned per tnl-port config. >>> - OVS_KEY_ATTR_TUN_ID action is removed. >>> >>> Signed-off-by: Pravin B Shelar >>> >>> Bug #15078 >> >> This is great, another 1000 lines of code gone. >> >> Acked-by: Jesse Gross > > Thanks, > I pushed patch to master. > branch 1.10 need few more changes, e.g capwap. I will send separate > patch for that. > >> >> Can you also push this to branch-1.10 so we get the dst_port changes >> in the first version where VXLAN exists? I pushed CAPWAP removal and tunnel cleanup changes to 1.10. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OpenVSwitch and libvirt integration problem at shutdown/reboot
On Wed, Mar 6, 2013 at 7:41 AM, Ernesto Domato wrote: > Sorry for the late response. > > On Mon, Mar 4, 2013 at 7:06 PM, Ansis Atteka wrote: >> On Mon, Mar 4, 2013 at 12:08 PM, Ernesto Domato wrote: >> >> If you do not block on interface creation and libvirt/Open vSwitch >> init.d dependencies are not right, then I think you might end up with >> another race condition, where VM automatic start-up would fail. >> Imagine that: >> 1. Neither Open vSwitch or libvirt are running >> 2. libvirt starts up >> 3. libvirt tries to spin up VM and executes "ovs-vsctl --no-wait >> --timeout=5 -- del-port ... -- add-port ..." command. After 5 seconds >> this command times out, because Open vSwitch wasn't running >> 4. After 6 seconds Open vSwitch starts up, but VM still remains down. >> >> This means that you will have to manually start the VM one more time. >> > > Ok, so I guess that to solve this problem, the right solution would be > that libvirt wait till OVS is up by not timing out, right? I actually see two solutions: 1. get daemon dependencies right (I see that Eric from libvirt project also recommended this); or 2. don't timeout when executing "ovs-vsctl add-port" command from libvirt I would prefer solution #1. If we would go with solution #2, then I am worried that someone else later on will complain that libvirt is stuck again (when OVS was not running). > > I think that could be a good idea, I'll write a new patch for libvirt > then and send it to them for comments. > >>> >>> I also added "--no-wait --timeout 5" when libvirt goes down so it can >>> timeout if ovs-switch is down. >> Just curious, but wasn't this part already solved with libvirt commit >> 98e732fc34a47ad9dfdb64aa4207623ee4c1ebcd (network: prevent infinite >> hang if ovs-vswitchd isn't running)? Are you using libvirt that has >> this patch? >> > > Ok, I'm using the stable version of libvirt and that fix is in > experimental package. Anyway, this fix only adds the timeout flag when > deleting the interface from OVS which does that libvirt don't hang > trying to delete the interface if OVS is down. But it doesn't fix the > issue that the OVS-DB still have the reference to the virtual > interface and so, when you bring the virtual machine up again, it > doesn't response because of this. As you (Ansis Atteka I guess) What do you mean by "doesn't response"? Even, if you pass "--timeout=5" and "--may-exist" to "add-port" command, then sometimes it still indefinitely blocks? > recommended before, my patch adds that libvirt try to delete (with the > --if-exists flag) the interface before adding it again so that problem > is resolved. It was long time ago, but I think that adding "--may-exist" flag to "add-port" command was sufficient. Can you debug this a little bit more and tell me what exactly is failing here (e.g. provide output of "ps -Af | grep ovs-vsctl" should be fine when this blocking happens)? If this indeed turns out to be a problem, then I think a long term solution would be to: 1. mark ovs ports created by libvirt with something like other_config:created-by-libvirt=true 2. If libvirt did not have a chance to delete old ports on shutdown, then at the startup it should iterate over all ports and delete unused ports where other_config:created-by-libvirt=true > >> By the way I tried to execute the same commands as libvirt would have >> executed. Except I ran them directly in the shell. What I observed is >> that ovs-vsctl did not block indefinitely for this corner case: >> >> root@ubuntu:~# service openvswitch-switch stop >> * ovs-brcompatd is not running >> * Killing ovs-vswitchd (13202) >> * Killing ovsdb-server (13193) >> root@ubuntu:~# ovs-vsctl --timeout=5 -- --if-exists del-port p0 >> Mar 04 13:39:14|2|stream_unix|ERR|/tmp/stream-unix.13222.0: >> connection to /var/run/openvswitch/db.sock failed: No such file or >> directory >> Mar 04 13:39:14|3|reconnect|WARN|unix:/var/run/openvswitch/db.sock: >> connection attempt failed (No such file or directory) >> Mar 04 13:39:15|4|stream_unix|ERR|/tmp/stream-unix.13222.1: >> connection to /var/run/openvswitch/db.sock failed: No such file or >> directory >> Mar 04 13:39:15|5|reconnect|WARN|unix:/var/run/openvswitch/db.sock: >> connection attempt failed (No such file or directory) >> Mar 04 13:39:17|6|stream_unix|ERR|/tmp/stream-unix.13222.2: >> connection to /var/run/openvswitch/db.sock failed: No such file or >> directory >> Mar 04 13:39:17|7|reconnect|WARN|unix:/var/run/openvswitch/db.sock: >> connection attempt failed (No such file or directory) >> Alarm clock >> root@ubuntu:~# >> >> I tried this with Open vSwitch: 1.4.3-0ubuntu2.1. Are you seeing the >> same effect with your Open vSwitch? > > Yes, I see the same behavior but after applying the patch that adds > the --timeout flag to the older version of libvirt that is currently > in Debian as stable but is incorporated in experimental package. > > So, summarizing, what I'll do is to download the GIT versi
Re: [ovs-dev] [PATCH] nicira-ext: Add Nicira actions NXAST_STACK_PUSH and NXAST_STACK_POP.
On Tue, Mar 05, 2013 at 04:27:55PM -0800, Andy Zhou wrote:
> The Push action takes a single parameter. Any source allowed by NXAST_REG_MOVE
> is allowed to be pushed onto the stack. When the source is a bit field,
> its value will be right shifted to bit zero before being pushed onto the
> stack. The remaining bits will be set to zero.
>
> The Pop action also takes a single parameter. Any destination allowed by
> NXAST_REG_MOVE can be used as the destination of the action. The value, in
> case of a bit field, will be taken from top of the stack, starting from
> bit zero.
>
> The stack size is not limited. The initial 8KB is statically allocated to
> efficiently handle most common use cases. When more stack space is
> required, the stack can grow using malloc().
>
> Signed-off-by: Andy Zhou
"git am" pointed out whitespace errors:
Applying: nicira-ext: Add Nicira actions NXAST_STACK_PUSH and
NXAST_STACK_POP.
/home/blp/ovs/.git/rebase-apply/patch:282: trailing whitespace.
error = nxm_reg_move_from_openflow(
/home/blp/ovs/.git/rebase-apply/patch:289: space before tab in indent.
(const struct nx_action_reg_load *) a, out);
/home/blp/ovs/.git/rebase-apply/patch:449: trailing whitespace.
union mf_subvalue init_stack[1024/sizeof(union mf_subvalue)];
warning: 3 lines add whitespace errors.
I fixed up a few minor stylistic things by folding in the following.
I shortened or deleted a few of the comments because I felt like they
did not add much valuable information. (For example, it should
normally be clear that an ofpbuf can expand as needed.)
I applied this to master. Thank you!
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index c6b5035..c46185a 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -564,7 +564,7 @@ OFP_ASSERT(sizeof(struct nx_action_reg_load) == 24);
/* Action structure for NXAST_STACK_PUSH and NXAST_STACK_POP.
*
- * Pushes (or pop) field[offset: offset + n_bits] to (or from)
+ * Pushes (or pops) field[offset: offset + n_bits] to (or from)
* top of the stack.
*/
struct nx_action_stack {
@@ -575,7 +575,7 @@ struct nx_action_stack {
ovs_be16 offset;/* Bit offset into the field. */
ovs_be32 field; /* The field used for push or pop. */
ovs_be16 n_bits;/* (n_bits + 1) bits of the field. */
-uint8_t zero[6];/* Reserved for future use, must be zero.*/
+uint8_t zero[6];/* Reserved, must be zero. */
};
OFP_ASSERT(sizeof(struct nx_action_stack) == 24);
diff --git a/lib/nx-match.c b/lib/nx-match.c
index f4a93a0..093f885 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1412,21 +1412,17 @@ nxm_stack_pop_to_nxast(const struct ofpact_stack *stack,
static void
nx_stack_push(struct ofpbuf *stack, union mf_subvalue *v)
{
-/*
- * Assume we have infinitely deep stack. Let ofpbuf grow
- * the stack if necessary.
- */
ofpbuf_put(stack, v, sizeof *v);
}
static union mf_subvalue *
nx_stack_pop(struct ofpbuf *stack)
{
-union mf_subvalue* v = NULL;
+union mf_subvalue *v = NULL;
if (stack->size) {
stack->size -= sizeof *v;
-v = (union mf_subvalue *)(ofpbuf_tail(stack));
+v = (union mf_subvalue *) ofpbuf_tail(stack);
}
return v;
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 1b92225..f1ff392 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -330,13 +330,13 @@ ofpact_from_nxast(const union ofp_action *a, enum
ofputil_action_code code,
break;
case OFPUTIL_NXAST_REG_MOVE:
-error = nxm_reg_move_from_openflow(
- (const struct nx_action_reg_move *) a, out);
+error = nxm_reg_move_from_openflow(
+(const struct nx_action_reg_move *) a, out);
break;
case OFPUTIL_NXAST_REG_LOAD:
error = nxm_reg_load_from_openflow(
- (const struct nx_action_reg_load *) a, out);
+(const struct nx_action_reg_load *) a, out);
break;
case OFPUTIL_NXAST_STACK_PUSH:
@@ -1146,10 +1146,10 @@ ofpact_check__(const struct ofpact *a, const struct
flow *flow, int max_ports,
}
case OFPACT_STACK_PUSH:
-return nxm_stack_push_check(ofpact_get_STACK_PUSH(a), flow);
+return nxm_stack_push_check(ofpact_get_STACK_PUSH(a), flow);
case OFPACT_STACK_POP:
-return nxm_stack_pop_check(ofpact_get_STACK_POP(a), flow);
+return nxm_stack_pop_check(ofpact_get_STACK_POP(a), flow);
case OFPACT_DEC_TTL:
case OFPACT_SET_TUNNEL:
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 0440bc4..4006624 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -306,7 +306,7 @@ struct ofpact_reg_move {
/* OFPACT_STACK_PUSH.
*
- * Used for NXAST_STACK_PUSH and NXAST_STACK_POP */
+ * Used for NXAST_STACK_PUSH and NXAST_STACK_POP. */
struct
Re: [ovs-dev] [ECNv2 5/5] tunnel: Mark ECN status on decapsulated tunnel packets.
On Mar 5, 2013, at 3:49 PM, Ben Pfaff wrote: >> Thanks for the reviews! I'll just wait for your feedback on the >> incremental I sent for part 2 before pushing the series. > > Let me know if you need anything else. Thanks again for the reviews. I pushed this to master and branch-1.10. --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/2] bridge: Fix remote_opstate bug recently introduced.
Commit 9a9e3786b3a8 (ofproto: Merge all the CFM query functions into one.)
mistakenly transformed a tristate variable into a Boolean one. This commit
fixes the problem.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto.h |7 ++-
vswitchd/bridge.c |9 +++--
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index a3c52b5..3ea56df 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -363,9 +363,14 @@ void ofproto_free_ofproto_controller_info(struct shash *);
/* CFM status query. */
struct ofproto_cfm_status {
enum cfm_fault_reason faults; /* 0 if not faulted. */
-bool remote_opstate; /* True if remote CFM endpoint is up. */
int health; /* Health status in [0,100] range. */
+/* 0 if the remote CFM endpoint is operationally down,
+ * 1 if the remote CFM endpoint is operationally up,
+ * -1 if we don't know because the remote CFM endpoint is not in extended
+ * mode. */
+int remote_opstate;
+
/* MPIDs of remote maintenance points whose CCMs have been received. */
const uint64_t *rmps;
size_t n_rmps;
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index b28ef26..311753d 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1769,8 +1769,13 @@ iface_refresh_cfm_stats(struct iface *iface)
}
ovsrec_interface_set_cfm_fault_status(cfg, (char **) reasons, j);
-ovsrec_interface_set_cfm_remote_opstate(cfg, (status.remote_opstate
- ? "up" : "down"));
+if (status.remote_opstate >= 0) {
+const char *remote_opstate = status.remote_opstate ? "up" : "down";
+ovsrec_interface_set_cfm_remote_opstate(cfg, remote_opstate);
+} else {
+ovsrec_interface_set_cfm_remote_opstate(cfg, NULL);
+}
+
ovsrec_interface_set_cfm_remote_mpids(cfg,
(const int64_t *)status.rmps,
status.n_rmps);
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/2] bridge: Fix interpretation of 'health' member of struct ofproto_cfm_status.
Commit 9a9e3786b3a8 (ofproto: Merge all the CFM query functions into one.)
mistakenly interpreted struct ofproto_cfm_status as always being in the
range [0,100]. It can in fact take the value -1 if the health status is
not currently well-defined.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto.h |9 +++--
vswitchd/bridge.c |6 +-
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 3ea56df..5960d7b 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -362,8 +362,8 @@ void ofproto_free_ofproto_controller_info(struct shash *);
/* CFM status query. */
struct ofproto_cfm_status {
-enum cfm_fault_reason faults; /* 0 if not faulted. */
-int health; /* Health status in [0,100] range. */
+/* 0 if not faulted, otherwise a combination of one or more reasons. */
+enum cfm_fault_reason faults;
/* 0 if the remote CFM endpoint is operationally down,
* 1 if the remote CFM endpoint is operationally up,
@@ -371,6 +371,11 @@ struct ofproto_cfm_status {
* mode. */
int remote_opstate;
+/* Ordinarily a "health status" in the range 0...100 inclusive, with 0
+ * being worst and 100 being best, or -1 if the health status is not
+ * well-defined. */
+int health;
+
/* MPIDs of remote maintenance points whose CCMs have been received. */
const uint64_t *rmps;
size_t n_rmps;
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 311753d..f7932c7 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1779,7 +1779,11 @@ iface_refresh_cfm_stats(struct iface *iface)
ovsrec_interface_set_cfm_remote_mpids(cfg,
(const int64_t *)status.rmps,
status.n_rmps);
-ovsrec_interface_set_cfm_health(cfg, &cfm_health, 1);
+if (cfm_health >= 0) {
+ovsrec_interface_set_cfm_health(cfg, &cfm_health, 1);
+} else {
+ovsrec_interface_set_cfm_health(cfg, NULL, 0);
+}
}
}
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: Treat zero MPLS post-decrement TTL as invalid.
RFC 3443 says that an outgoing MPLS TTL of zero is invalid, so we must send
packets that have such a TTL post-decrement to the controller rather than
allowing them to continue through the OpenFlow tables.
Here is the text from RFC 3443 that implies this behavior:
oTTL: This is the TTL value used as the outgoing TTL value (see
section 3.5 for exception). It is always (iTTL - 1) unless otherwise
stated.
oTTL Check: Check if oTTL is greater than 0. If the oTTL Check is
false, then the packet is not forwarded. Note that the oTTL check is
performed only if any outgoing TTL (either IP or MPLS) is set to oTTL
(see section 3.5 for exception).
That is, the output TTL is one less than the input TTL, and a packet with
an output TTL of 0 must not be forwarded.
CC: Bruce Davie
Signed-off-by: Ben Pfaff
---
I swear I sent this out earlier, but I don't see it in the archive
or in my inbox.
ofproto/ofproto-dpif.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 10e4e23..38b8e55 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6062,7 +6062,7 @@ execute_dec_mpls_ttl_action(struct action_xlate_ctx *ctx)
return false;
}
-if (ttl > 0) {
+if (ttl > 1) {
ttl--;
set_mpls_lse_ttl(&ctx->flow.mpls_lse, ttl);
return false;
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] Add support for dec_mpls_ttl action
On Wed, Mar 06, 2013 at 09:26:19AM -0800, Bruce Davie wrote: > The processing for MPLS TTL is somewhat complex compared to IP, > because there are a few different options for what is acceptable > when pushing and popping labels. However, one thing that DOES carry > forward straightforwardly from IP is that if the TTL reaches zero, > the packet MUST be discarded. This is documented in RFC 1812, > Section 5.3. > > If you want to see all the gorey details about MPLS TTL processing, > you need to read RFC 3443. Thanks a lot, I sent out a fix: http://openvswitch.org/pipermail/dev/2013-March/025853.html ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] bridge: Fix remote_opstate bug recently introduced.
Acked-by: Ethan Jackson
On Wed, Mar 6, 2013 at 2:53 PM, Ben Pfaff wrote:
> Commit 9a9e3786b3a8 (ofproto: Merge all the CFM query functions into one.)
> mistakenly transformed a tristate variable into a Boolean one. This commit
> fixes the problem.
>
> Signed-off-by: Ben Pfaff
> ---
> ofproto/ofproto.h |7 ++-
> vswitchd/bridge.c |9 +++--
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index a3c52b5..3ea56df 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -363,9 +363,14 @@ void ofproto_free_ofproto_controller_info(struct
> shash *);
> /* CFM status query. */
> struct ofproto_cfm_status {
> enum cfm_fault_reason faults; /* 0 if not faulted. */
> -bool remote_opstate; /* True if remote CFM endpoint is up.
> */
> int health; /* Health status in [0,100] range. */
>
> +/* 0 if the remote CFM endpoint is operationally down,
> + * 1 if the remote CFM endpoint is operationally up,
> + * -1 if we don't know because the remote CFM endpoint is not in
> extended
> + * mode. */
> +int remote_opstate;
> +
> /* MPIDs of remote maintenance points whose CCMs have been received.
> */
> const uint64_t *rmps;
> size_t n_rmps;
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index b28ef26..311753d 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1769,8 +1769,13 @@ iface_refresh_cfm_stats(struct iface *iface)
> }
> ovsrec_interface_set_cfm_fault_status(cfg, (char **) reasons, j);
>
> -ovsrec_interface_set_cfm_remote_opstate(cfg,
> (status.remote_opstate
> - ? "up" : "down"));
> +if (status.remote_opstate >= 0) {
> +const char *remote_opstate = status.remote_opstate ? "up" :
> "down";
> +ovsrec_interface_set_cfm_remote_opstate(cfg, remote_opstate);
> +} else {
> +ovsrec_interface_set_cfm_remote_opstate(cfg, NULL);
> +}
> +
> ovsrec_interface_set_cfm_remote_mpids(cfg,
>(const int64_t
> *)status.rmps,
>status.n_rmps);
> --
> 1.7.2.5
>
> ___
> 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 2/2] bridge: Fix interpretation of 'health' member of struct ofproto_cfm_status.
Acked-by: Ethan Jackson
On Wed, Mar 6, 2013 at 2:53 PM, Ben Pfaff wrote:
> Commit 9a9e3786b3a8 (ofproto: Merge all the CFM query functions into one.)
> mistakenly interpreted struct ofproto_cfm_status as always being in the
> range [0,100]. It can in fact take the value -1 if the health status is
> not currently well-defined.
>
> Signed-off-by: Ben Pfaff
> ---
> ofproto/ofproto.h |9 +++--
> vswitchd/bridge.c |6 +-
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 3ea56df..5960d7b 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -362,8 +362,8 @@ void ofproto_free_ofproto_controller_info(struct shash
> *);
>
> /* CFM status query. */
> struct ofproto_cfm_status {
> -enum cfm_fault_reason faults; /* 0 if not faulted. */
> -int health; /* Health status in [0,100] range. */
> +/* 0 if not faulted, otherwise a combination of one or more reasons.
> */
> +enum cfm_fault_reason faults;
>
> /* 0 if the remote CFM endpoint is operationally down,
> * 1 if the remote CFM endpoint is operationally up,
> @@ -371,6 +371,11 @@ struct ofproto_cfm_status {
> * mode. */
> int remote_opstate;
>
> +/* Ordinarily a "health status" in the range 0...100 inclusive, with 0
> + * being worst and 100 being best, or -1 if the health status is not
> + * well-defined. */
> +int health;
> +
> /* MPIDs of remote maintenance points whose CCMs have been received.
> */
> const uint64_t *rmps;
> size_t n_rmps;
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 311753d..f7932c7 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1779,7 +1779,11 @@ iface_refresh_cfm_stats(struct iface *iface)
> ovsrec_interface_set_cfm_remote_mpids(cfg,
>(const int64_t
> *)status.rmps,
>status.n_rmps);
> -ovsrec_interface_set_cfm_health(cfg, &cfm_health, 1);
> +if (cfm_health >= 0) {
> +ovsrec_interface_set_cfm_health(cfg, &cfm_health, 1);
> +} else {
> +ovsrec_interface_set_cfm_health(cfg, NULL, 0);
> +}
> }
> }
>
> --
> 1.7.2.5
>
> ___
> 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 2/2] bridge: Fix interpretation of 'health' member of struct ofproto_cfm_status.
Thanks, I applied these to master.
On Wed, Mar 06, 2013 at 04:16:30PM -0800, Ethan Jackson wrote:
> Acked-by: Ethan Jackson
>
>
>
> On Wed, Mar 6, 2013 at 2:53 PM, Ben Pfaff wrote:
>
> > Commit 9a9e3786b3a8 (ofproto: Merge all the CFM query functions into one.)
> > mistakenly interpreted struct ofproto_cfm_status as always being in the
> > range [0,100]. It can in fact take the value -1 if the health status is
> > not currently well-defined.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > ofproto/ofproto.h |9 +++--
> > vswitchd/bridge.c |6 +-
> > 2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index 3ea56df..5960d7b 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -362,8 +362,8 @@ void ofproto_free_ofproto_controller_info(struct shash
> > *);
> >
> > /* CFM status query. */
> > struct ofproto_cfm_status {
> > -enum cfm_fault_reason faults; /* 0 if not faulted. */
> > -int health; /* Health status in [0,100] range. */
> > +/* 0 if not faulted, otherwise a combination of one or more reasons.
> > */
> > +enum cfm_fault_reason faults;
> >
> > /* 0 if the remote CFM endpoint is operationally down,
> > * 1 if the remote CFM endpoint is operationally up,
> > @@ -371,6 +371,11 @@ struct ofproto_cfm_status {
> > * mode. */
> > int remote_opstate;
> >
> > +/* Ordinarily a "health status" in the range 0...100 inclusive, with 0
> > + * being worst and 100 being best, or -1 if the health status is not
> > + * well-defined. */
> > +int health;
> > +
> > /* MPIDs of remote maintenance points whose CCMs have been received.
> > */
> > const uint64_t *rmps;
> > size_t n_rmps;
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 311753d..f7932c7 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -1779,7 +1779,11 @@ iface_refresh_cfm_stats(struct iface *iface)
> > ovsrec_interface_set_cfm_remote_mpids(cfg,
> >(const int64_t
> > *)status.rmps,
> >status.n_rmps);
> > -ovsrec_interface_set_cfm_health(cfg, &cfm_health, 1);
> > +if (cfm_health >= 0) {
> > +ovsrec_interface_set_cfm_health(cfg, &cfm_health, 1);
> > +} else {
> > +ovsrec_interface_set_cfm_health(cfg, NULL, 0);
> > +}
> > }
> > }
> >
> > --
> > 1.7.2.5
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: Print slow-path actions instead of "drop" in dump-flows.
The command "ovs-appctl dpif/dump-flows" would print slow-path actions
as "drop", which could be confusing to users. This is different from
"ovs-dpctl dump-flows", which prints a descriptive reason. This commit
replaces "drop" with the reason.
Bug #14840
Signed-off-by: Justin Pettit
---
ofproto/ofproto-dpif.c | 13 -
tests/ofproto-dpif.at | 14 +++---
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 89f5bf4..9721665 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -8135,7 +8135,18 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn
*conn,
}
ds_put_cstr(&ds, ", actions:");
-format_odp_actions(&ds, subfacet->actions, subfacet->actions_len);
+if (subfacet->slow) {
+uint64_t slow_path_stub[128 / 8];
+const struct nlattr *actions;
+size_t actions_len;
+
+compose_slow_path(ofproto, &subfacet->facet->flow, subfacet->slow,
+slow_path_stub, sizeof slow_path_stub,
+&actions, &actions_len);
+format_odp_actions(&ds, actions, actions_len);
+} else {
+format_odp_actions(&ds, subfacet->actions, subfacet->actions_len);
+}
ds_put_char(&ds, '\n');
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 7915792..b7428ae 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1495,12 +1495,12 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p3
'in_port(3),eth(src=50:54:00:00:00:
])
AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl
-in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:0, bytes:0, used:0.0s, actions:drop
-in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0),
packets:0, bytes:0, used:0.0s, actions:drop
+in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:0, bytes:0, used:0.0s, actions:userspace(pid=0,slow_path(controller))
+in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0),
packets:0, bytes:0, used:0.0s, actions:userspace(pid=0,slow_path(controller))
])
AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl
-in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:0, bytes:0, used:0.0s, actions:drop
+in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:0, bytes:0, used:0.0s, actions:userspace(pid=0,slow_path(controller))
])
OVS_VSWITCHD_STOP
@@ -1520,12 +1520,12 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p3
'in_port(3),eth(src=50:54:00:00:00:
])
AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl
-in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:0, bytes:0, used:0.0s, actions:drop
-in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0),
packets:0, bytes:0, used:0.0s, actions:drop
+in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:0, bytes:0, used:0.0s, actions:userspace(pid=0,slow_path(controller))
+in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0),
packets:0, bytes:0, used:0.0s, actions:userspace(pid=0,slow_path(controller))
])
AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl
-in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:0, bytes:0, used:0.0s, actions:drop
+in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:0, bytes:0, used:0.0s, actions:userspace(pid=0,slow_path(controller))
])
AT_CHECK([ovs-appctl dpif/del-flows br0])
@@ -1533,7 +1533,7 @@ AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort |
STRIP_USED], [0], [dnl
])
AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl
-in_port(
[ovs-dev] [PATCH 1/2] ofproto-dpif: Clarify that 'one_subfacet' may not always be valid.
Signed-off-by: Justin Pettit
---
ofproto/ofproto-dpif.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 89f5bf4..c95daa2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -487,7 +487,9 @@ struct facet {
/* Storage for a single subfacet, to reduce malloc() time and space
* overhead. (A facet always has at least one subfacet and in the common
- * case has exactly one subfacet.) */
+ * case has exactly one subfacet. However, 'one_subfacet' may not
+ * always be valid, since it could have been removed after newer
+ * subfacets were pushed onto the 'subfacets' list.) */
struct subfacet one_subfacet;
};
--
1.7.5.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/2] ofproto-dpif: Add helper function to retrieve a subfacet from a facet.
Signed-off-by: Justin Pettit
---
ofproto/ofproto-dpif.c | 17 -
1 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c95daa2..f230e90 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -513,6 +513,8 @@ static void facet_push_stats(struct facet *);
static void facet_learn(struct facet *);
static void facet_account(struct facet *);
+static struct subfacet *facet_get_subfacet(struct facet *);
+
static bool facet_is_controller_flow(struct facet *);
struct ofport_dpif {
@@ -4441,7 +4443,7 @@ static void
facet_account(struct facet *facet)
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
-struct subfacet *subfacet;
+struct subfacet *subfacet = facet_get_subfacet(facet);
const struct nlattr *a;
unsigned int left;
ovs_be16 vlan_tci;
@@ -4460,8 +4462,6 @@ facet_account(struct facet *facet)
*
* We use the actions from an arbitrary subfacet because they should all
* be equally valid for our purpose. */
-subfacet = CONTAINER_OF(list_front(&facet->subfacets),
-struct subfacet, list_node);
vlan_tci = facet->flow.vlan_tci;
NL_ATTR_FOR_EACH_UNSAFE (a, left,
subfacet->actions, subfacet->actions_len) {
@@ -4598,6 +4598,14 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const
struct flow *flow,
return facet;
}
+/* Return a subfacet from 'facet'. A facet consists of one or more
+ * subfacets, and this function returns one of them. */
+static struct subfacet *facet_get_subfacet(struct facet *facet)
+{
+return CONTAINER_OF(list_front(&facet->subfacets), struct subfacet,
+list_node);
+}
+
static const char *
subfacet_path_to_string(enum subfacet_path path)
{
@@ -4937,8 +4945,7 @@ flow_push_stats(struct facet *facet, const struct
dpif_flow_stats *stats)
{
struct rule_dpif *rule = facet->rule;
struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
-struct subfacet *subfacet = CONTAINER_OF(list_front(&facet->subfacets),
- struct subfacet, list_node);
+struct subfacet *subfacet = facet_get_subfacet(facet);
struct action_xlate_ctx ctx;
ofproto_rule_update_used(&rule->up, stats->used);
--
1.7.5.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
