| Message ID | 1493524130-10220-1-git-send-email-dsa@cumulusnetworks.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
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?
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.
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.
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
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.
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
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;
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(-)