diff mbox series

kni: reduce interface name size

Message ID 20191115114107.30737-1-michael.pfeiffer@tu-ilmenau.de (mailing list archive)
State Superseded, archived
Headers
Series kni: reduce interface name size |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed

Commit Message

Michael Pfeiffer Nov. 15, 2019, 11:41 a.m. UTC 
  The name in rte_kni_device_info is passed to the kernel, which allows
interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc with a
longer name currently trigger a kernel BUG in alloc_netdev_mqs in
net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this situation.

Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
---
 lib/librte_eal/linux/eal/include/rte_kni_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Igor Ryzhov Nov. 15, 2019, 12:30 p.m. UTC | #1 
Hi Michael,

Isn't it better to set it to IFNAMSIZ instead of 16?

Best regards,
Igot

On Fri, Nov 15, 2019 at 2:41 PM Michael Pfeiffer <
michael.pfeiffer@tu-ilmenau.de> wrote:

> The name in rte_kni_device_info is passed to the kernel, which allows
> interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc with a
> longer name currently trigger a kernel BUG in alloc_netdev_mqs in
> net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this situation.
>
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> ---
>  lib/librte_eal/linux/eal/include/rte_kni_common.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/linux/eal/include/rte_kni_common.h
> b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> index 46f75a710..59339271b 100644
> --- a/lib/librte_eal/linux/eal/include/rte_kni_common.h
> +++ b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> @@ -18,7 +18,7 @@
>  /**
>   * KNI name is part of memzone name.
>   */
> -#define RTE_KNI_NAMESIZE 32
> +#define RTE_KNI_NAMESIZE 16
>
>  #define RTE_CACHE_LINE_MIN_SIZE 64
>
> --
> 2.20.1
>
>
Ferruh Yigit Nov. 15, 2019, 12:43 p.m. UTC | #2 
On 11/15/2019 12:30 PM, Igor Ryzhov wrote:
> Hi Michael,
> 
> Isn't it better to set it to IFNAMSIZ instead of 16?

I was thinking same, but "linux/if.h" included only for kernel, so instead of
introducing new dependency in user side, perhaps better to keep it as hardcoded
value.

> 
> Best regards,
> Igot
> 
> On Fri, Nov 15, 2019 at 2:41 PM Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de
> <mailto:michael.pfeiffer@tu-ilmenau.de>> wrote:
> 
>     The name in rte_kni_device_info is passed to the kernel, which allows
>     interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc with a
>     longer name currently trigger a kernel BUG in alloc_netdev_mqs in
>     net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this situation.
> 
>     Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de
>     <mailto:michael.pfeiffer@tu-ilmenau.de>>
>     ---
>      lib/librte_eal/linux/eal/include/rte_kni_common.h | 2 +-
>      1 file changed, 1 insertion(+), 1 deletion(-)
> 
>     diff --git a/lib/librte_eal/linux/eal/include/rte_kni_common.h
>     b/lib/librte_eal/linux/eal/include/rte_kni_common.h
>     index 46f75a710..59339271b 100644
>     --- a/lib/librte_eal/linux/eal/include/rte_kni_common.h
>     +++ b/lib/librte_eal/linux/eal/include/rte_kni_common.h
>     @@ -18,7 +18,7 @@
>      /**
>       * KNI name is part of memzone name.
>       */
>     -#define RTE_KNI_NAMESIZE 32
>     +#define RTE_KNI_NAMESIZE 16
> 
>      #define RTE_CACHE_LINE_MIN_SIZE 64
> 
>     -- 
>     2.20.1
>
Michael Pfeiffer Nov. 15, 2019, 12:43 p.m. UTC | #3 
Hi Igor,
you're right, RTE_KNI_NAMESIZE == IFNAMSIZ is the intention. However,
to my understanding linux/if.h (where IFNAMSIZ is defined) is only
included when building kernel code. I thought maybe this was
intentional to keep rte_kni_common.h free of Linux kernel dependencies
(when building userland code).

In practice, it probably won't matter, as the KNI kernel module is
available for Linux only at the moment. I will therefore gladly change
this to IFNAMSIZ if you and Ferruh think this is the way to go.

Regards
Michael

On Fri, 2019-11-15 at 15:30 +0300, Igor Ryzhov wrote:
> Hi Michael,
> 
> Isn't it better to set it to IFNAMSIZ instead of 16?
> 
> Best regards,
> Igot
> 
> On Fri, Nov 15, 2019 at 2:41 PM Michael Pfeiffer <
> michael.pfeiffer@tu-ilmenau.de> wrote:
> 
> > The name in rte_kni_device_info is passed to the kernel, which
> > allows
> > interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc
> > with a
> > longer name currently trigger a kernel BUG in alloc_netdev_mqs in
> > net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this situation.
> > 
> > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> > ---
> >  lib/librte_eal/linux/eal/include/rte_kni_common.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_eal/linux/eal/include/rte_kni_common.h
> > b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> > index 46f75a710..59339271b 100644
> > --- a/lib/librte_eal/linux/eal/include/rte_kni_common.h
> > +++ b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> > @@ -18,7 +18,7 @@
> >  /**
> >   * KNI name is part of memzone name.
> >   */
> > -#define RTE_KNI_NAMESIZE 32
> > +#define RTE_KNI_NAMESIZE 16
> > 
> >  #define RTE_CACHE_LINE_MIN_SIZE 64
> > 
> > --
> > 2.20.1
> > 
> >
Ferruh Yigit Nov. 15, 2019, 12:49 p.m. UTC | #4 
On 11/15/2019 12:43 PM, Michael Pfeiffer wrote:
> Hi Igor,
> you're right, RTE_KNI_NAMESIZE == IFNAMSIZ is the intention. However,
> to my understanding linux/if.h (where IFNAMSIZ is defined) is only
> included when building kernel code. I thought maybe this was
> intentional to keep rte_kni_common.h free of Linux kernel dependencies
> (when building userland code).
> 
> In practice, it probably won't matter, as the KNI kernel module is
> available for Linux only at the moment. I will therefore gladly change
> this to IFNAMSIZ if you and Ferruh think this is the way to go.

I think better to not add dependency to userland, and I don't expect IFNAMSIZ
changing in kernel side, so safe to keep hardcoded value,
only perhaps a simple /* IFNAMSIZ  */ comment can be added to highlight the
relation?

> 
> Regards
> Michael
> 
> On Fri, 2019-11-15 at 15:30 +0300, Igor Ryzhov wrote:
>> Hi Michael,
>>
>> Isn't it better to set it to IFNAMSIZ instead of 16?
>>
>> Best regards,
>> Igot
>>
>> On Fri, Nov 15, 2019 at 2:41 PM Michael Pfeiffer <
>> michael.pfeiffer@tu-ilmenau.de> wrote:
>>
>>> The name in rte_kni_device_info is passed to the kernel, which
>>> allows
>>> interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc
>>> with a
>>> longer name currently trigger a kernel BUG in alloc_netdev_mqs in
>>> net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this situation.
>>>
>>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>>> ---
>>>  lib/librte_eal/linux/eal/include/rte_kni_common.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_eal/linux/eal/include/rte_kni_common.h
>>> b/lib/librte_eal/linux/eal/include/rte_kni_common.h
>>> index 46f75a710..59339271b 100644
>>> --- a/lib/librte_eal/linux/eal/include/rte_kni_common.h
>>> +++ b/lib/librte_eal/linux/eal/include/rte_kni_common.h
>>> @@ -18,7 +18,7 @@
>>>  /**
>>>   * KNI name is part of memzone name.
>>>   */
>>> -#define RTE_KNI_NAMESIZE 32
>>> +#define RTE_KNI_NAMESIZE 16
>>>
>>>  #define RTE_CACHE_LINE_MIN_SIZE 64
>>>
>>> --
>>> 2.20.1
>>>
>>>
Michael Pfeiffer Nov. 15, 2019, 12:52 p.m. UTC | #5 
Hi Ferruh,

On Fri, 2019-11-15 at 12:49 +0000, Ferruh Yigit wrote:
> On 11/15/2019 12:43 PM, Michael Pfeiffer wrote:
> > Hi Igor,
> > you're right, RTE_KNI_NAMESIZE == IFNAMSIZ is the intention.
> > However,
> > to my understanding linux/if.h (where IFNAMSIZ is defined) is only
> > included when building kernel code. I thought maybe this was
> > intentional to keep rte_kni_common.h free of Linux kernel
> > dependencies
> > (when building userland code).
> > 
> > In practice, it probably won't matter, as the KNI kernel module is
> > available for Linux only at the moment. I will therefore gladly
> > change
> > this to IFNAMSIZ if you and Ferruh think this is the way to go.
> 
> I think better to not add dependency to userland, and I don't expect
> IFNAMSIZ
> changing in kernel side, so safe to keep hardcoded value,
> only perhaps a simple /* IFNAMSIZ  */ comment can be added to
> highlight the
> relation?

perfect, I will update the patch accordingly.

Regards
Michael

> > Regards
> > Michael
> > 
> > On Fri, 2019-11-15 at 15:30 +0300, Igor Ryzhov wrote:
> > > Hi Michael,
> > > 
> > > Isn't it better to set it to IFNAMSIZ instead of 16?
> > > 
> > > Best regards,
> > > Igot
> > > 
> > > On Fri, Nov 15, 2019 at 2:41 PM Michael Pfeiffer <
> > > michael.pfeiffer@tu-ilmenau.de> wrote:
> > > 
> > > > The name in rte_kni_device_info is passed to the kernel, which
> > > > allows
> > > > interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc
> > > > with a
> > > > longer name currently trigger a kernel BUG in alloc_netdev_mqs
> > > > in
> > > > net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this
> > > > situation.
> > > > 
> > > > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de
> > > > >
> > > > ---
> > > >  lib/librte_eal/linux/eal/include/rte_kni_common.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/librte_eal/linux/eal/include/rte_kni_common.h
> > > > b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> > > > index 46f75a710..59339271b 100644
> > > > --- a/lib/librte_eal/linux/eal/include/rte_kni_common.h
> > > > +++ b/lib/librte_eal/linux/eal/include/rte_kni_common.h
> > > > @@ -18,7 +18,7 @@
> > > >  /**
> > > >   * KNI name is part of memzone name.
> > > >   */
> > > > -#define RTE_KNI_NAMESIZE 32
> > > > +#define RTE_KNI_NAMESIZE 16
> > > > 
> > > >  #define RTE_CACHE_LINE_MIN_SIZE 64
> > > > 
> > > > --
> > > > 2.20.1
> > > > 
> > > >
Stephen Hemminger Nov. 15, 2019, 4:23 p.m. UTC | #6 
On Fri, 15 Nov 2019 12:41:07 +0100
Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de> wrote:

> The name in rte_kni_device_info is passed to the kernel, which allows
> interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc with a
> longer name currently trigger a kernel BUG in alloc_netdev_mqs in
> net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this situation.
> 
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>

Why not kill the NAMESIZE define in KNI and use the kernel one.
Ferruh Yigit Nov. 19, 2019, 12:13 p.m. UTC | #7 
On 11/15/2019 4:23 PM, Stephen Hemminger wrote:
> On Fri, 15 Nov 2019 12:41:07 +0100
> Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de> wrote:
> 
>> The name in rte_kni_device_info is passed to the kernel, which allows
>> interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc with a
>> longer name currently trigger a kernel BUG in alloc_netdev_mqs in
>> net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this situation.
>>
>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> 
> Why not kill the NAMESIZE define in KNI and use the kernel one.
> 


As mentioned in the thread, userspace doesn't know NAMESIZE, only kernel module
does, because "linux/if.h" is only included by kernel module.

I don't know if will there be any side affect of including relevant header and
creating this dependency to the userspace, but I am not for taking that chance
for this benefit, the comment already clarifies the value and it is not
something that will change.
David Marchand Nov. 19, 2019, 8:58 p.m. UTC | #8 
On Tue, Nov 19, 2019 at 1:13 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 11/15/2019 4:23 PM, Stephen Hemminger wrote:
> > On Fri, 15 Nov 2019 12:41:07 +0100
> > Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de> wrote:
> >
> >> The name in rte_kni_device_info is passed to the kernel, which allows
> >> interface names with at most 16 bytes (IFNAMSIZ). rte_kni_alloc with a
> >> longer name currently trigger a kernel BUG in alloc_netdev_mqs in
> >> net/core/dev.c. Reduce RTE_KNI_NAMESIZE to prevent this situation.
> >>
> >> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> >
> > Why not kill the NAMESIZE define in KNI and use the kernel one.
> >
>
>
> As mentioned in the thread, userspace doesn't know NAMESIZE, only kernel module
> does, because "linux/if.h" is only included by kernel module.
>
> I don't know if will there be any side affect of including relevant header and
> creating this dependency to the userspace, but I am not for taking that chance
> for this benefit, the comment already clarifies the value and it is not
> something that will change.

We can still revisit this later.
I will go and apply the acked v2 https://patchwork.dpdk.org/patch/63042/
diff mbox series

Patch

diff --git a/lib/librte_eal/linux/eal/include/rte_kni_common.h b/lib/librte_eal/linux/eal/include/rte_kni_common.h
index 46f75a710..59339271b 100644
--- a/lib/librte_eal/linux/eal/include/rte_kni_common.h
+++ b/lib/librte_eal/linux/eal/include/rte_kni_common.h
@@ -18,7 +18,7 @@ 
 /**
  * KNI name is part of memzone name.
  */
-#define RTE_KNI_NAMESIZE 32
+#define RTE_KNI_NAMESIZE 16
 
 #define RTE_CACHE_LINE_MIN_SIZE 64