[PATCH net-next iproute2] ip: increase number of MPLS labels

[PATCH net-next iproute2] ip: increase number of MPLS labels

[David Ahern]

Kernel now supports more than 2 labels. Increase ip to
handle up to 16 labels.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 include/utils.h | 8 ++++----
 lib/utils.c     | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 8c12e1e2a60c..a69e176c260d 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -54,6 +54,9 @@ void incomplete_command(void) __attribute__((noreturn));
 #define NEXT_ARG_FWD() do { argv++; argc--; } while(0)
 #define PREV_ARG() do { argv--; argc++; } while(0)
 
+/* Maximum number of labels the mpls helpers support */
+#define MPLS_MAX_LABELS 16
+
 typedef struct
 {
 	__u16 flags;
@@ -61,7 +64,7 @@ typedef struct
 	__s16 bitlen;
 	/* These next two fields match rtvia */
 	__u16 family;
-	__u32 data[8];
+	__u32 data[MPLS_MAX_LABELS];
 } inet_prefix;
 
 #define PREFIXLEN_SPECIFIED 1
@@ -88,9 +91,6 @@ struct ipx_addr {
 # define AF_MPLS 28
 #endif
 
-/* Maximum number of labels the mpls helpers support */
-#define MPLS_MAX_LABELS 8
-
 __u32 get_addr32(const char *name);
 int get_addr_1(inet_prefix *dst, const char *arg, int family);
 int get_prefix_1(inet_prefix *dst, char *arg, int family);
diff --git a/lib/utils.c b/lib/utils.c
index 6d5642f4f1f3..c23251067180 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -526,7 +526,7 @@ int get_addr_1(inet_prefix *addr, const char *name, int family)
 		addr->bytelen = 4;
 		addr->bitlen = 20;
 		/* How many bytes do I need? */
-		for (i = 0; i < 8; i++) {
+		for (i = 0; i < MPLS_MAX_LABELS; i++) {
 			if (ntohl(addr->data[i]) & MPLS_LS_S_MASK) {
 				addr->bytelen = (i + 1)*4;
 				break;
-- 
2.1.4

Re: [PATCH net-next iproute2] ip: increase number of MPLS labels

[Stephen Hemminger]

On Sat, 29 Apr 2017 20:48:50 -0700
David Ahern <dsa@cumulusnetworks.com> wrote:

> Kernel now supports more than 2 labels. Increase ip to
> handle up to 16 labels.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
>  include/utils.h | 8 ++++----
>  lib/utils.c     | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/utils.h b/include/utils.h
> index 8c12e1e2a60c..a69e176c260d 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -54,6 +54,9 @@ void incomplete_command(void) __attribute__((noreturn));
>  #define NEXT_ARG_FWD() do { argv++; argc--; } while(0)
>  #define PREV_ARG() do { argv--; argc++; } while(0)
>  
> +/* Maximum number of labels the mpls helpers support */
> +#define MPLS_MAX_LABELS 16
> +

Why is the kernel limit not in include/uapi/ header file?

Re: [PATCH net-next iproute2] ip: increase number of MPLS labels

[David Ahern]

On 4/30/17 12:04 AM, Stephen Hemminger wrote:
> On Sat, 29 Apr 2017 20:48:50 -0700
> David Ahern <dsa@cumulusnetworks.com> wrote:
> 
>> Kernel now supports more than 2 labels. Increase ip to
>> handle up to 16 labels.
>>
>> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
>> ---
>>  include/utils.h | 8 ++++----
>>  lib/utils.c     | 2 +-
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/utils.h b/include/utils.h
>> index 8c12e1e2a60c..a69e176c260d 100644
>> --- a/include/utils.h
>> +++ b/include/utils.h
>> @@ -54,6 +54,9 @@ void incomplete_command(void) __attribute__((noreturn));
>>  #define NEXT_ARG_FWD() do { argv++; argc--; } while(0)
>>  #define PREV_ARG() do { argv--; argc++; } while(0)
>>  
>> +/* Maximum number of labels the mpls helpers support */
>> +#define MPLS_MAX_LABELS 16
>> +
> 
> Why is the kernel limit not in include/uapi/ header file?
> 

I believe Eric had reasons, but not sure why.

Re: [PATCH net-next iproute2] ip: increase number of MPLS labels

[Stephen Hemminger]

On Sun, 30 Apr 2017 17:42:15 -0600
David Ahern <dsa@cumulusnetworks.com> wrote:

> On 4/30/17 12:04 AM, Stephen Hemminger wrote:
> > On Sat, 29 Apr 2017 20:48:50 -0700
> > David Ahern <dsa@cumulusnetworks.com> wrote:
> >   
> >> Kernel now supports more than 2 labels. Increase ip to
> >> handle up to 16 labels.
> >>
> >> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> >> ---
> >>  include/utils.h | 8 ++++----
> >>  lib/utils.c     | 2 +-
> >>  2 files changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/utils.h b/include/utils.h
> >> index 8c12e1e2a60c..a69e176c260d 100644
> >> --- a/include/utils.h
> >> +++ b/include/utils.h
> >> @@ -54,6 +54,9 @@ void incomplete_command(void) __attribute__((noreturn));
> >>  #define NEXT_ARG_FWD() do { argv++; argc--; } while(0)
> >>  #define PREV_ARG() do { argv--; argc++; } while(0)
> >>  
> >> +/* Maximum number of labels the mpls helpers support */
> >> +#define MPLS_MAX_LABELS 16
> >> +  
> > 
> > Why is the kernel limit not in include/uapi/ header file?
> >   
> 
> I believe Eric had reasons, but not sure why.

I just don't want iproute2 utilities chasing kernel values.
Would prefer either make utilities take any size, or inherit maximum from kernel headers.

Re: [PATCH net-next iproute2] ip: increase number of MPLS labels

[Eric W. Biederman]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Sun, 30 Apr 2017 17:42:15 -0600
> David Ahern <dsa@cumulusnetworks.com> wrote:
>
>> On 4/30/17 12:04 AM, Stephen Hemminger wrote:
>> > On Sat, 29 Apr 2017 20:48:50 -0700
>> > David Ahern <dsa@cumulusnetworks.com> wrote:
>> >   
>> >> Kernel now supports more than 2 labels. Increase ip to
>> >> handle up to 16 labels.
>> >>
>> >> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
>> >> ---
>> >>  include/utils.h | 8 ++++----
>> >>  lib/utils.c     | 2 +-
>> >>  2 files changed, 5 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/include/utils.h b/include/utils.h
>> >> index 8c12e1e2a60c..a69e176c260d 100644
>> >> --- a/include/utils.h
>> >> +++ b/include/utils.h
>> >> @@ -54,6 +54,9 @@ void incomplete_command(void) __attribute__((noreturn));
>> >>  #define NEXT_ARG_FWD() do { argv++; argc--; } while(0)
>> >>  #define PREV_ARG() do { argv--; argc++; } while(0)
>> >>  
>> >> +/* Maximum number of labels the mpls helpers support */
>> >> +#define MPLS_MAX_LABELS 16
>> >> +  
>> > 
>> > Why is the kernel limit not in include/uapi/ header file?
>> >   
>> 
>> I believe Eric had reasons, but not sure why.
>
> I just don't want iproute2 utilities chasing kernel values.
> Would prefer either make utilities take any size, or inherit maximum
> from kernel headers.

Basically the kernel maximum is how large we can make struct mpls_route
without while being < 4096 aka PAGE_SIZE.  Which also happens to be
larger than the largest known consumer.

Which basically makes the limit meaningless for userspace.  MPLS does
not actually have a maximum (other than packet size) in the protocol.

So we need a number large enough that people don't care or something
completely arbitrary in iproute.

At 8 labels we are the same size as an ipv6 address which is where
the limitation of 8 came from in the code.  It was easy and it allowed
for future growth.

Taking a quick look at the code iproure uses inet_prefix for an address
in any protocol family and it is used fairly widely in the code.
Further addresses are placed in what appear to be fixed sized
preallocated netlink packets.  Hmmmmm....

So a completely arbitrary number of labels seems like over-enginnerring.
It looks like what is more interesting is using as a limit the remaining
space in the rtnetlink buffer.  Perhaps a function that combines
the efforts of get_addr and addattr_l/rta_addattr_l is the way to go.

That would appear to make it easier to add new address families in the future.

Dave is there a practical reason that is motivating increasing the size
in iproute?  Or is the desire to ensure that iproute can take full
advantage of the kernel?

Eric

Re: [PATCH net-next iproute2] ip: increase number of MPLS labels

[David Ahern]

On 5/1/17 3:03 PM, Eric W. Biederman wrote:
> Basically the kernel maximum is how large we can make struct mpls_route
> without while being < 4096 aka PAGE_SIZE.  Which also happens to be
> larger than the largest known consumer.

2 limits in the kernel:
1. max allocation for mpls_route of 4096
2. max number of labels of 30.

The latter was necessitated by the LWT path. I wanted a cap on it and
for the cap to be consistent for both ingress and LSP. To that end I
went with a maximum of 30 labels in the kernel: 4k max allocation = ~30
labels per nexthop with ~30 nexthops. For both 30 is excessive, overkill
number. (though someone did ask me recently why not 512 labels. seriously)

I can't say it makes sense for iproute2 to support 30 labels.
Argumentative that even 16 is too high, but some folks have argued for it.


> Dave is there a practical reason that is motivating increasing the size
> in iproute?  Or is the desire to ensure that iproute can take full
> advantage of the kernel?

I suspect only routing daemons would inject a high label stack. It would
be nice for iproute2 to 1. be able to print that route properly
(currently does not if the label count exceeds what it can handle), 2.
be able to reinject the route. Even with scripting creating a route
using ip with a lot of labels is a PITA.

Re: [PATCH net-next iproute2] ip: increase number of MPLS labels

[Eric W. Biederman]

David Ahern <dsa@cumulusnetworks.com> writes:

> On 5/1/17 3:03 PM, Eric W. Biederman wrote:
>> Basically the kernel maximum is how large we can make struct mpls_route
>> without while being < 4096 aka PAGE_SIZE.  Which also happens to be
>> larger than the largest known consumer.
>
> 2 limits in the kernel:
> 1. max allocation for mpls_route of 4096
> 2. max number of labels of 30.

Ah yes. I remember now.  That conversation was just long enough ago that
the details had slipped my mind.

> The latter was necessitated by the LWT path. I wanted a cap on it and
> for the cap to be consistent for both ingress and LSP. To that end I
> went with a maximum of 30 labels in the kernel: 4k max allocation = ~30
> labels per nexthop with ~30 nexthops. For both 30 is excessive, overkill
> number. (though someone did ask me recently why not 512 labels. seriously)
>
> I can't say it makes sense for iproute2 to support 30 labels.
> Argumentative that even 16 is too high, but some folks have argued for it.
>
>
>> Dave is there a practical reason that is motivating increasing the size
>> in iproute?  Or is the desire to ensure that iproute can take full
>> advantage of the kernel?
>
> I suspect only routing daemons would inject a high label stack. It would
> be nice for iproute2 to 1. be able to print that route properly
> (currently does not if the label count exceeds what it can handle), 2.
> be able to reinject the route. Even with scripting creating a route
> using ip with a lot of labels is a PITA.

Yes.  At this point I will agree with Stephen Hemminger.  The sensible
change to iproute is to rework the code so that it supports an arbitrary
number of labels.  At least up to iproutes fixed sized rtnetlink packet
buffers.

It is a bit more work but then iproute won't be something that needs
worrying about any more.  It will just work with whatever size addresses
are thrown at it.

Eric