[dpdk-dev] Overriding rte_config.h

[dpdk-dev] Overriding rte_config.h

[Ben Magistro]

With the transition to meson, what is the best way to provide custom values
to parameters in rte_config.h?  When using makefiles, (from memory, I
think) we used common_base as a template that was copied in as a
replacement for defconfig_x86....  Our current thinking is to apply a
locally maintained patch so that we can track custom values easier to the
rte_config.h file unless there is another way to pass in an overridden
value.  As an example, one of the values we are customizing is
IP_FRAG_MAX_FRAG.

Cheers,

Ben Magistro

Re: [dpdk-dev] Overriding rte_config.h

[Bruce Richardson]

On Fri, Oct 29, 2021 at 09:48:30AM -0400, Ben Magistro wrote:
> With the transition to meson, what is the best way to provide custom values
> to parameters in rte_config.h?  When using makefiles, (from memory, I
> think) we used common_base as a template that was copied in as a
> replacement for defconfig_x86....  Our current thinking is to apply a
> locally maintained patch so that we can track custom values easier to the
> rte_config.h file unless there is another way to pass in an overridden
> value.  As an example, one of the values we are customizing is
> IP_FRAG_MAX_FRAG.
> 
> Cheers,
> 
There is no one defined way for overriding values in rte_config with the
meson build system, as values there are ones that should rarely need to be
overridden. If it's the case that one does need tuning, we generally want
to look to either change the default so it works for everyone, or
alternatively look to replace it with a runtime option.

In the absense of that, a locally maintained patch may be reasonable. To
what value do you want to change MAX_FRAG? Would it be worth considering as
a newer default value in DPDK itself, since the current default is fairly
low?

Re: [dpdk-dev] Overriding rte_config.h

[Ananyev, Konstantin]

> On Fri, Oct 29, 2021 at 09:48:30AM -0400, Ben Magistro wrote:
> > With the transition to meson, what is the best way to provide custom values
> > to parameters in rte_config.h?  When using makefiles, (from memory, I
> > think) we used common_base as a template that was copied in as a
> > replacement for defconfig_x86....  Our current thinking is to apply a
> > locally maintained patch so that we can track custom values easier to the
> > rte_config.h file unless there is another way to pass in an overridden
> > value.  As an example, one of the values we are customizing is
> > IP_FRAG_MAX_FRAG.
> >
> > Cheers,
> >
> There is no one defined way for overriding values in rte_config with the
> meson build system, as values there are ones that should rarely need to be
> overridden. If it's the case that one does need tuning, we generally want
> to look to either change the default so it works for everyone, or
> alternatively look to replace it with a runtime option.
> 
> In the absense of that, a locally maintained patch may be reasonable. To
> what value do you want to change MAX_FRAG? Would it be worth considering as
> a newer default value in DPDK itself, since the current default is fairly
> low?

That might be an option, with IP_FRAG_MAX_FRAG==8 it should be able
to cover common jumbo frame size (9K) pretty easily.
As a drawback default reassembly table size will double. 
Even better would be to go a step further and rework lib/ip_frag
to make it configurable runtime parameter.

Re: [dpdk-dev] Overriding rte_config.h

[Bruce Richardson]

On Tue, Nov 02, 2021 at 11:20:04AM +0000, Ananyev, Konstantin wrote:
> 
> > On Fri, Oct 29, 2021 at 09:48:30AM -0400, Ben Magistro wrote:
> > > With the transition to meson, what is the best way to provide custom values
> > > to parameters in rte_config.h?  When using makefiles, (from memory, I
> > > think) we used common_base as a template that was copied in as a
> > > replacement for defconfig_x86....  Our current thinking is to apply a
> > > locally maintained patch so that we can track custom values easier to the
> > > rte_config.h file unless there is another way to pass in an overridden
> > > value.  As an example, one of the values we are customizing is
> > > IP_FRAG_MAX_FRAG.
> > >
> > > Cheers,
> > >
> > There is no one defined way for overriding values in rte_config with the
> > meson build system, as values there are ones that should rarely need to be
> > overridden. If it's the case that one does need tuning, we generally want
> > to look to either change the default so it works for everyone, or
> > alternatively look to replace it with a runtime option.
> >
> > In the absense of that, a locally maintained patch may be reasonable. To
> > what value do you want to change MAX_FRAG? Would it be worth considering as
> > a newer default value in DPDK itself, since the current default is fairly
> > low?
> 
> That might be an option, with IP_FRAG_MAX_FRAG==8 it should be able
> to cover common jumbo frame size (9K) pretty easily.
> As a drawback default reassembly table size will double.

Maybe not. I'm not an expert in the library, but it seems the basic struct
used for tracking the packets and fragments is "struct ip_frag_pkt". Due to
the other data in the struct and the linked-list overheads, the actual size
increase when doubling MAX_FRAG from 4 to 8 is only 25%. According to gdb
on my debug build it goes from 192B to 256B.

> Even better would be to go a step further and rework lib/ip_frag
> to make it configurable runtime parameter.
>
Agree. However, that's not as quick a fix as just increasing the default
max segs value which could be done immediately if there is consensus on it.

/Bruce 

Re: [dpdk-dev] Overriding rte_config.h

[Ananyev, Konstantin]

> > > On Fri, Oct 29, 2021 at 09:48:30AM -0400, Ben Magistro wrote:
> > > > With the transition to meson, what is the best way to provide custom values
> > > > to parameters in rte_config.h?  When using makefiles, (from memory, I
> > > > think) we used common_base as a template that was copied in as a
> > > > replacement for defconfig_x86....  Our current thinking is to apply a
> > > > locally maintained patch so that we can track custom values easier to the
> > > > rte_config.h file unless there is another way to pass in an overridden
> > > > value.  As an example, one of the values we are customizing is
> > > > IP_FRAG_MAX_FRAG.
> > > >
> > > > Cheers,
> > > >
> > > There is no one defined way for overriding values in rte_config with the
> > > meson build system, as values there are ones that should rarely need to be
> > > overridden. If it's the case that one does need tuning, we generally want
> > > to look to either change the default so it works for everyone, or
> > > alternatively look to replace it with a runtime option.
> > >
> > > In the absense of that, a locally maintained patch may be reasonable. To
> > > what value do you want to change MAX_FRAG? Would it be worth considering as
> > > a newer default value in DPDK itself, since the current default is fairly
> > > low?
> >
> > That might be an option, with IP_FRAG_MAX_FRAG==8 it should be able
> > to cover common jumbo frame size (9K) pretty easily.
> > As a drawback default reassembly table size will double.
> 
> Maybe not. I'm not an expert in the library, but it seems the basic struct
> used for tracking the packets and fragments is "struct ip_frag_pkt". Due to
> the other data in the struct and the linked-list overheads, the actual size
> increase when doubling MAX_FRAG from 4 to 8 is only 25%. According to gdb
> on my debug build it goes from 192B to 256B.

Ah yes, you right, struct ip_frag should fit into 16B, key seems the biggest one. 

> 
> > Even better would be to go a step further and rework lib/ip_frag
> > to make it configurable runtime parameter.
> >
> Agree. However, that's not as quick a fix as just increasing the default
> max segs value which could be done immediately if there is consensus on it.

You mean for 21.11?
I don't mind in principle, but would like to know other people thoughts here.
Another thing -  we didn't announce it in advance, and it is definitely an ABI change. 

Re: [dpdk-dev] Overriding rte_config.h

[Bruce Richardson]

On Tue, Nov 02, 2021 at 12:24:43PM +0000, Ananyev, Konstantin wrote:
> 
> > > > On Fri, Oct 29, 2021 at 09:48:30AM -0400, Ben Magistro wrote:
> > > > > With the transition to meson, what is the best way to provide custom values
> > > > > to parameters in rte_config.h?  When using makefiles, (from memory, I
> > > > > think) we used common_base as a template that was copied in as a
> > > > > replacement for defconfig_x86....  Our current thinking is to apply a
> > > > > locally maintained patch so that we can track custom values easier to the
> > > > > rte_config.h file unless there is another way to pass in an overridden
> > > > > value.  As an example, one of the values we are customizing is
> > > > > IP_FRAG_MAX_FRAG.
> > > > >
> > > > > Cheers,
> > > > >
> > > > There is no one defined way for overriding values in rte_config with the
> > > > meson build system, as values there are ones that should rarely need to be
> > > > overridden. If it's the case that one does need tuning, we generally want
> > > > to look to either change the default so it works for everyone, or
> > > > alternatively look to replace it with a runtime option.
> > > >
> > > > In the absense of that, a locally maintained patch may be reasonable. To
> > > > what value do you want to change MAX_FRAG? Would it be worth considering as
> > > > a newer default value in DPDK itself, since the current default is fairly
> > > > low?
> > >
> > > That might be an option, with IP_FRAG_MAX_FRAG==8 it should be able
> > > to cover common jumbo frame size (9K) pretty easily.
> > > As a drawback default reassembly table size will double.
> >
> > Maybe not. I'm not an expert in the library, but it seems the basic struct
> > used for tracking the packets and fragments is "struct ip_frag_pkt". Due to
> > the other data in the struct and the linked-list overheads, the actual size
> > increase when doubling MAX_FRAG from 4 to 8 is only 25%. According to gdb
> > on my debug build it goes from 192B to 256B.
> 
> Ah yes, you right, struct ip_frag should fit into 16B, key seems the biggest one.
> 
> >
> > > Even better would be to go a step further and rework lib/ip_frag
> > > to make it configurable runtime parameter.
> > >
> > Agree. However, that's not as quick a fix as just increasing the default
> > max segs value which could be done immediately if there is consensus on it.
> 
> You mean for 21.11?
> I don't mind in principle, but would like to know other people thoughts here.
> Another thing -  we didn't announce it in advance, and it is definitely an ABI change.

I notice from this patch you submitted that the main structure in question
is being hidden[1]. Will it still be an ABI change if that patch is merged
in? Alternatively, should a fragment count increase be considered as part of
that change?

/Bruce

[1] http://patches.dpdk.org/project/dpdk/patch/20211101124915.9640-1-konstantin.ananyev@intel.com/

Re: [dpdk-dev] Overriding rte_config.h

[Ananyev, Konstantin]

> > > > > On Fri, Oct 29, 2021 at 09:48:30AM -0400, Ben Magistro wrote:
> > > > > > With the transition to meson, what is the best way to provide custom values
> > > > > > to parameters in rte_config.h?  When using makefiles, (from memory, I
> > > > > > think) we used common_base as a template that was copied in as a
> > > > > > replacement for defconfig_x86....  Our current thinking is to apply a
> > > > > > locally maintained patch so that we can track custom values easier to the
> > > > > > rte_config.h file unless there is another way to pass in an overridden
> > > > > > value.  As an example, one of the values we are customizing is
> > > > > > IP_FRAG_MAX_FRAG.
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > There is no one defined way for overriding values in rte_config with the
> > > > > meson build system, as values there are ones that should rarely need to be
> > > > > overridden. If it's the case that one does need tuning, we generally want
> > > > > to look to either change the default so it works for everyone, or
> > > > > alternatively look to replace it with a runtime option.
> > > > >
> > > > > In the absense of that, a locally maintained patch may be reasonable. To
> > > > > what value do you want to change MAX_FRAG? Would it be worth considering as
> > > > > a newer default value in DPDK itself, since the current default is fairly
> > > > > low?
> > > >
> > > > That might be an option, with IP_FRAG_MAX_FRAG==8 it should be able
> > > > to cover common jumbo frame size (9K) pretty easily.
> > > > As a drawback default reassembly table size will double.
> > >
> > > Maybe not. I'm not an expert in the library, but it seems the basic struct
> > > used for tracking the packets and fragments is "struct ip_frag_pkt". Due to
> > > the other data in the struct and the linked-list overheads, the actual size
> > > increase when doubling MAX_FRAG from 4 to 8 is only 25%. According to gdb
> > > on my debug build it goes from 192B to 256B.
> >
> > Ah yes, you right, struct ip_frag should fit into 16B, key seems the biggest one.
> >
> > >
> > > > Even better would be to go a step further and rework lib/ip_frag
> > > > to make it configurable runtime parameter.
> > > >
> > > Agree. However, that's not as quick a fix as just increasing the default
> > > max segs value which could be done immediately if there is consensus on it.
> >
> > You mean for 21.11?
> > I don't mind in principle, but would like to know other people thoughts here.
> > Another thing -  we didn't announce it in advance, and it is definitely an ABI change.
> 
> I notice from this patch you submitted that the main structure in question
> is being hidden[1]. Will it still be an ABI change if that patch is merged
> in?

Yes, it would unfortunately:
struct rte_ip_frag_death_row still remains public.

> Alternatively, should a fragment count increase be considered as part of
> that change?

I don't think they are really related.
This patch just hides some structs that are already marked as 'internal'
and not used by public API. It doesn't make any changes in the public structs layout.
But I suppose we can bring that question (increase of RTE_LIBRTE_IP_FRAG_MAX_FRAG) to
tomorrow TB meeting, and ask for approval.
 
> /Bruce
> 
> [1] http://patches.dpdk.org/project/dpdk/patch/20211101124915.9640-1-konstantin.ananyev@intel.com/

Re: [dpdk-dev] Overriding rte_config.h

[Ben Magistro]

Thanks for the clarification.

I agree bumping RTE_LIBRTE_IP_FRAG_MAX_FRAG to 8 probably makes sense to
easily support jumbo frames.

The other use case we have is supporting highly fragmented UDP.  To support
this we were increasing to 64 (next power of 2)  based on a 64K UDP max and
a link MTU of 1200 (VPN/tunneling).  I am not sure this is a value that
makes sense for the majority of use cases.

On Tue, Nov 2, 2021 at 11:09 AM Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:

>
> > > > > > On Fri, Oct 29, 2021 at 09:48:30AM -0400, Ben Magistro wrote:
> > > > > > > With the transition to meson, what is the best way to provide
> custom values
> > > > > > > to parameters in rte_config.h?  When using makefiles, (from
> memory, I
> > > > > > > think) we used common_base as a template that was copied in as
> a
> > > > > > > replacement for defconfig_x86....  Our current thinking is to
> apply a
> > > > > > > locally maintained patch so that we can track custom values
> easier to the
> > > > > > > rte_config.h file unless there is another way to pass in an
> overridden
> > > > > > > value.  As an example, one of the values we are customizing is
> > > > > > > IP_FRAG_MAX_FRAG.
> > > > > > >
> > > > > > > Cheers,
> > > > > > >
> > > > > > There is no one defined way for overriding values in rte_config
> with the
> > > > > > meson build system, as values there are ones that should rarely
> need to be
> > > > > > overridden. If it's the case that one does need tuning, we
> generally want
> > > > > > to look to either change the default so it works for everyone, or
> > > > > > alternatively look to replace it with a runtime option.
> > > > > >
> > > > > > In the absense of that, a locally maintained patch may be
> reasonable. To
> > > > > > what value do you want to change MAX_FRAG? Would it be worth
> considering as
> > > > > > a newer default value in DPDK itself, since the current default
> is fairly
> > > > > > low?
> > > > >
> > > > > That might be an option, with IP_FRAG_MAX_FRAG==8 it should be able
> > > > > to cover common jumbo frame size (9K) pretty easily.
> > > > > As a drawback default reassembly table size will double.
> > > >
> > > > Maybe not. I'm not an expert in the library, but it seems the basic
> struct
> > > > used for tracking the packets and fragments is "struct ip_frag_pkt".
> Due to
> > > > the other data in the struct and the linked-list overheads, the
> actual size
> > > > increase when doubling MAX_FRAG from 4 to 8 is only 25%. According
> to gdb
> > > > on my debug build it goes from 192B to 256B.
> > >
> > > Ah yes, you right, struct ip_frag should fit into 16B, key seems the
> biggest one.
> > >
> > > >
> > > > > Even better would be to go a step further and rework lib/ip_frag
> > > > > to make it configurable runtime parameter.
> > > > >
> > > > Agree. However, that's not as quick a fix as just increasing the
> default
> > > > max segs value which could be done immediately if there is consensus
> on it.
> > >
> > > You mean for 21.11?
> > > I don't mind in principle, but would like to know other people
> thoughts here.
> > > Another thing -  we didn't announce it in advance, and it is
> definitely an ABI change.
> >
> > I notice from this patch you submitted that the main structure in
> question
> > is being hidden[1]. Will it still be an ABI change if that patch is
> merged
> > in?
>
> Yes, it would unfortunately:
> struct rte_ip_frag_death_row still remains public.
>
> > Alternatively, should a fragment count increase be considered as part of
> > that change?
>
> I don't think they are really related.
> This patch just hides some structs that are already marked as 'internal'
> and not used by public API. It doesn't make any changes in the public
> structs layout.
> But I suppose we can bring that question (increase of
> RTE_LIBRTE_IP_FRAG_MAX_FRAG) to
> tomorrow TB meeting, and ask for approval.
>
> > /Bruce
> >
> > [1]
> http://patches.dpdk.org/project/dpdk/patch/20211101124915.9640-1-konstantin.ananyev@intel.com/
>

Re: [dpdk-dev] Overriding rte_config.h

[Ananyev, Konstantin]

Hi Ben,

I also don’t think 64 is a common case here.
For such cases we probably should think up some different approach for the reassembly table.

From: Ben Magistro <koncept1@gmail.com>
Sent: Wednesday, November 3, 2021 2:38 PM
To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
Cc: Richardson, Bruce <bruce.richardson@intel.com>; dev@dpdk.org; ben.magistro@trinitycyber.com; Stefan Baranoff <stefan.baranoff@trinitycyber.com>
Subject: Re: [dpdk-dev] Overriding rte_config.h

Thanks for the clarification.

I agree bumping RTE_LIBRTE_IP_FRAG_MAX_FRAG to 8 probably makes sense to easily support jumbo frames.

The other use case we have is supporting highly fragmented UDP.  To support this we were increasing to 64 (next power of 2)  based on a 64K UDP max and a link MTU of 1200 (VPN/tunneling).  I am not sure this is a value that makes sense for the majority of use cases.

On Tue, Nov 2, 2021 at 11:09 AM Ananyev, Konstantin <konstantin.ananyev@intel.com<mailto:konstantin.ananyev@intel.com>> wrote:

> > > > > On Fri, Oct 29, 2021 at 09:48:30AM -0400, Ben Magistro wrote:
> > > > > > With the transition to meson, what is the best way to provide custom values
> > > > > > to parameters in rte_config.h?  When using makefiles, (from memory, I
> > > > > > think) we used common_base as a template that was copied in as a
> > > > > > replacement for defconfig_x86....  Our current thinking is to apply a
> > > > > > locally maintained patch so that we can track custom values easier to the
> > > > > > rte_config.h file unless there is another way to pass in an overridden
> > > > > > value.  As an example, one of the values we are customizing is
> > > > > > IP_FRAG_MAX_FRAG.
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > There is no one defined way for overriding values in rte_config with the
> > > > > meson build system, as values there are ones that should rarely need to be
> > > > > overridden. If it's the case that one does need tuning, we generally want
> > > > > to look to either change the default so it works for everyone, or
> > > > > alternatively look to replace it with a runtime option.
> > > > >
> > > > > In the absense of that, a locally maintained patch may be reasonable. To
> > > > > what value do you want to change MAX_FRAG? Would it be worth considering as
> > > > > a newer default value in DPDK itself, since the current default is fairly
> > > > > low?
> > > >
> > > > That might be an option, with IP_FRAG_MAX_FRAG==8 it should be able
> > > > to cover common jumbo frame size (9K) pretty easily.
> > > > As a drawback default reassembly table size will double.
> > >
> > > Maybe not. I'm not an expert in the library, but it seems the basic struct
> > > used for tracking the packets and fragments is "struct ip_frag_pkt". Due to
> > > the other data in the struct and the linked-list overheads, the actual size
> > > increase when doubling MAX_FRAG from 4 to 8 is only 25%. According to gdb
> > > on my debug build it goes from 192B to 256B.
> >
> > Ah yes, you right, struct ip_frag should fit into 16B, key seems the biggest one.
> >
> > >
> > > > Even better would be to go a step further and rework lib/ip_frag
> > > > to make it configurable runtime parameter.
> > > >
> > > Agree. However, that's not as quick a fix as just increasing the default
> > > max segs value which could be done immediately if there is consensus on it.
> >
> > You mean for 21.11?
> > I don't mind in principle, but would like to know other people thoughts here.
> > Another thing -  we didn't announce it in advance, and it is definitely an ABI change.
>
> I notice from this patch you submitted that the main structure in question
> is being hidden[1]. Will it still be an ABI change if that patch is merged
> in?

Yes, it would unfortunately:
struct rte_ip_frag_death_row still remains public.

> Alternatively, should a fragment count increase be considered as part of
> that change?

I don't think they are really related.
This patch just hides some structs that are already marked as 'internal'
and not used by public API. It doesn't make any changes in the public structs layout.
But I suppose we can bring that question (increase of RTE_LIBRTE_IP_FRAG_MAX_FRAG) to
tomorrow TB meeting, and ask for approval.

> /Bruce
>
> [1] http://patches.dpdk.org/project/dpdk/patch/20211101124915.9640-1-konstantin.ananyev@intel.com/

Re: [dpdk-dev] Overriding rte_config.h

[Stephen Hemminger]

On Thu, 4 Nov 2021 11:03:21 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> Hi Ben,
> 
> I also don’t think 64 is a common case here.
> For such cases we probably should think up some different approach for the reassembly table.

Larger values make it easier to DoS the IP fragment table.
See the Linux fragsmack vulnerability.