[ovs-dev] [PATCH 1/3] Add support for dec_mpls_ttl action

[Jesse Gross]

On Tue, Mar 5, 2013 at 11:08 PM, Simon Horman <horms at verge.net.au> 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 <yamahata at valinux.co.jp>
>> > > > Signed-off-by: Simon Horman <horms at verge.net.au>
>> > >
>> > > 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 <pshelar at nicira.com>
>     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 <pshelar at nicira.com>
>
> 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.