2023-02-17 03:40:10

by Gavin Li

[permalink] [raw]
Subject: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

vxlan_build_gbp_hdr will be used by other modules to build gbp option in
vxlan header according to gbp flags.

Signed-off-by: Gavin Li <[email protected]>
Reviewed-by: Gavi Teitz <[email protected]>
Reviewed-by: Roi Dayan <[email protected]>
Reviewed-by: Maor Dickman <[email protected]>
Acked-by: Saeed Mahameed <[email protected]>
---
drivers/net/vxlan/vxlan_core.c | 19 -------------------
include/net/vxlan.h | 19 +++++++++++++++++++
2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 86967277ab97..13faab36b3e1 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2140,25 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
return false;
}

-static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_metadata *md)
-{
- struct vxlanhdr_gbp *gbp;
-
- if (!md->gbp)
- return;
-
- gbp = (struct vxlanhdr_gbp *)vxh;
- vxh->vx_flags |= VXLAN_HF_GBP;
-
- if (md->gbp & VXLAN_GBP_DONT_LEARN)
- gbp->dont_learn = 1;
-
- if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
- gbp->policy_applied = 1;
-
- gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
-}
-
static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, __be16 protocol)
{
struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index bca5b01af247..b6d419fa7ab1 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
return true;
}

+static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct vxlan_metadata *md)
+{
+ struct vxlanhdr_gbp *gbp;
+
+ if (!md->gbp)
+ return;
+
+ gbp = (struct vxlanhdr_gbp *)vxh;
+ vxh->vx_flags |= VXLAN_HF_GBP;
+
+ if (md->gbp & VXLAN_GBP_DONT_LEARN)
+ gbp->dont_learn = 1;
+
+ if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
+ gbp->policy_applied = 1;
+
+ gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
+}
+
#endif
--
2.31.1



2023-02-19 20:32:42

by Simon Horman

[permalink] [raw]
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
> vxlan header according to gbp flags.
>
> Signed-off-by: Gavin Li <[email protected]>
> Reviewed-by: Gavi Teitz <[email protected]>
> Reviewed-by: Roi Dayan <[email protected]>
> Reviewed-by: Maor Dickman <[email protected]>
> Acked-by: Saeed Mahameed <[email protected]>

I do wonder if this needs to be a static inline function.
But nonetheless,

Reviewed-by: Simon Horman <[email protected]>

> ---
> drivers/net/vxlan/vxlan_core.c | 19 -------------------
> include/net/vxlan.h | 19 +++++++++++++++++++
> 2 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 86967277ab97..13faab36b3e1 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -2140,25 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
> return false;
> }
>
> -static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_metadata *md)
> -{
> - struct vxlanhdr_gbp *gbp;
> -
> - if (!md->gbp)
> - return;
> -
> - gbp = (struct vxlanhdr_gbp *)vxh;
> - vxh->vx_flags |= VXLAN_HF_GBP;
> -
> - if (md->gbp & VXLAN_GBP_DONT_LEARN)
> - gbp->dont_learn = 1;
> -
> - if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
> - gbp->policy_applied = 1;
> -
> - gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
> -}
> -
> static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, __be16 protocol)
> {
> struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index bca5b01af247..b6d419fa7ab1 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
> return true;
> }
>
> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct vxlan_metadata *md)
> +{
> + struct vxlanhdr_gbp *gbp;
> +
> + if (!md->gbp)
> + return;
> +
> + gbp = (struct vxlanhdr_gbp *)vxh;
> + vxh->vx_flags |= VXLAN_HF_GBP;
> +
> + if (md->gbp & VXLAN_GBP_DONT_LEARN)
> + gbp->dont_learn = 1;
> +
> + if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
> + gbp->policy_applied = 1;
> +
> + gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
> +}
> +
> #endif
> --
> 2.31.1
>

2023-02-20 02:05:27

by Gavin Li

[permalink] [raw]
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr


On 2/20/2023 4:32 AM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
>> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
>> vxlan header according to gbp flags.
>>
>> Signed-off-by: Gavin Li <[email protected]>
>> Reviewed-by: Gavi Teitz <[email protected]>
>> Reviewed-by: Roi Dayan <[email protected]>
>> Reviewed-by: Maor Dickman <[email protected]>
>> Acked-by: Saeed Mahameed <[email protected]>
> I do wonder if this needs to be a static inline function.
> But nonetheless,

Will get "unused-function" from gcc without "inline"

./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but
not used [-Wunused-function]
 static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
vxlan_metadata *md)

>
> Reviewed-by: Simon Horman <[email protected]>
>
>> ---
>> drivers/net/vxlan/vxlan_core.c | 19 -------------------
>> include/net/vxlan.h | 19 +++++++++++++++++++
>> 2 files changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
>> index 86967277ab97..13faab36b3e1 100644
>> --- a/drivers/net/vxlan/vxlan_core.c
>> +++ b/drivers/net/vxlan/vxlan_core.c
>> @@ -2140,25 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
>> return false;
>> }
>>
>> -static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_metadata *md)
>> -{
>> - struct vxlanhdr_gbp *gbp;
>> -
>> - if (!md->gbp)
>> - return;
>> -
>> - gbp = (struct vxlanhdr_gbp *)vxh;
>> - vxh->vx_flags |= VXLAN_HF_GBP;
>> -
>> - if (md->gbp & VXLAN_GBP_DONT_LEARN)
>> - gbp->dont_learn = 1;
>> -
>> - if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
>> - gbp->policy_applied = 1;
>> -
>> - gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
>> -}
>> -
>> static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, __be16 protocol)
>> {
>> struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
>> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
>> index bca5b01af247..b6d419fa7ab1 100644
>> --- a/include/net/vxlan.h
>> +++ b/include/net/vxlan.h
>> @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
>> return true;
>> }
>>
>> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct vxlan_metadata *md)
>> +{
>> + struct vxlanhdr_gbp *gbp;
>> +
>> + if (!md->gbp)
>> + return;
>> +
>> + gbp = (struct vxlanhdr_gbp *)vxh;
>> + vxh->vx_flags |= VXLAN_HF_GBP;
>> +
>> + if (md->gbp & VXLAN_GBP_DONT_LEARN)
>> + gbp->dont_learn = 1;
>> +
>> + if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
>> + gbp->policy_applied = 1;
>> +
>> + gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
>> +}
>> +
>> #endif
>> --
>> 2.31.1
>>

2023-02-20 06:40:41

by Simon Horman

[permalink] [raw]
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

On Mon, Feb 20, 2023 at 10:05:00AM +0800, Gavin Li wrote:
>
> On 2/20/2023 4:32 AM, Simon Horman wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
> > > vxlan_build_gbp_hdr will be used by other modules to build gbp option in
> > > vxlan header according to gbp flags.
> > >
> > > Signed-off-by: Gavin Li <[email protected]>
> > > Reviewed-by: Gavi Teitz <[email protected]>
> > > Reviewed-by: Roi Dayan <[email protected]>
> > > Reviewed-by: Maor Dickman <[email protected]>
> > > Acked-by: Saeed Mahameed <[email protected]>
> > I do wonder if this needs to be a static inline function.
> > But nonetheless,
>
> Will get "unused-function" from gcc without "inline"
>
> ./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but not
> used [-Wunused-function]
>  static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
> vxlan_metadata *md)

Right. But what I was really wondering is if the definition
of the function could stay in drivers/net/vxlan/vxlan_core.c,
without being static. And have a declaration in include/net/vxlan.h

> > Reviewed-by: Simon Horman <[email protected]>
> >
> > > ---
> > > drivers/net/vxlan/vxlan_core.c | 19 -------------------
> > > include/net/vxlan.h | 19 +++++++++++++++++++
> > > 2 files changed, 19 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > > index 86967277ab97..13faab36b3e1 100644
> > > --- a/drivers/net/vxlan/vxlan_core.c
> > > +++ b/drivers/net/vxlan/vxlan_core.c
> > > @@ -2140,25 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
> > > return false;
> > > }
> > >
> > > -static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_metadata *md)
> > > -{
> > > - struct vxlanhdr_gbp *gbp;
> > > -
> > > - if (!md->gbp)
> > > - return;
> > > -
> > > - gbp = (struct vxlanhdr_gbp *)vxh;
> > > - vxh->vx_flags |= VXLAN_HF_GBP;
> > > -
> > > - if (md->gbp & VXLAN_GBP_DONT_LEARN)
> > > - gbp->dont_learn = 1;
> > > -
> > > - if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
> > > - gbp->policy_applied = 1;
> > > -
> > > - gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
> > > -}
> > > -
> > > static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, __be16 protocol)
> > > {
> > > struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
> > > diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> > > index bca5b01af247..b6d419fa7ab1 100644
> > > --- a/include/net/vxlan.h
> > > +++ b/include/net/vxlan.h
> > > @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
> > > return true;
> > > }
> > >
> > > +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct vxlan_metadata *md)
> > > +{
> > > + struct vxlanhdr_gbp *gbp;
> > > +
> > > + if (!md->gbp)
> > > + return;
> > > +
> > > + gbp = (struct vxlanhdr_gbp *)vxh;
> > > + vxh->vx_flags |= VXLAN_HF_GBP;
> > > +
> > > + if (md->gbp & VXLAN_GBP_DONT_LEARN)
> > > + gbp->dont_learn = 1;
> > > +
> > > + if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
> > > + gbp->policy_applied = 1;
> > > +
> > > + gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
> > > +}
> > > +
> > > #endif
> > > --
> > > 2.31.1
> > >
>

2023-02-20 07:15:50

by Gavin Li

[permalink] [raw]
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr


On 2/20/2023 2:40 PM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Feb 20, 2023 at 10:05:00AM +0800, Gavin Li wrote:
>> On 2/20/2023 4:32 AM, Simon Horman wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
>>>> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
>>>> vxlan header according to gbp flags.
>>>>
>>>> Signed-off-by: Gavin Li <[email protected]>
>>>> Reviewed-by: Gavi Teitz <[email protected]>
>>>> Reviewed-by: Roi Dayan <[email protected]>
>>>> Reviewed-by: Maor Dickman <[email protected]>
>>>> Acked-by: Saeed Mahameed <[email protected]>
>>> I do wonder if this needs to be a static inline function.
>>> But nonetheless,
>> Will get "unused-function" from gcc without "inline"
>>
>> ./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but not
>> used [-Wunused-function]
>> static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
>> vxlan_metadata *md)
> Right. But what I was really wondering is if the definition
> of the function could stay in drivers/net/vxlan/vxlan_core.c,
> without being static. And have a declaration in include/net/vxlan.h

Tried that the first time the function was called by driver code. It
would introduce dependency in linking between the driver and the kernel
module.

Do you think it's OK to have such dependency?

>
>>> Reviewed-by: Simon Horman <[email protected]>
>>>
>>>> ---
>>>> drivers/net/vxlan/vxlan_core.c | 19 -------------------
>>>> include/net/vxlan.h | 19 +++++++++++++++++++
>>>> 2 files changed, 19 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
>>>> index 86967277ab97..13faab36b3e1 100644
>>>> --- a/drivers/net/vxlan/vxlan_core.c
>>>> +++ b/drivers/net/vxlan/vxlan_core.c
>>>> @@ -2140,25 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
>>>> return false;
>>>> }
>>>>
>>>> -static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_metadata *md)
>>>> -{
>>>> - struct vxlanhdr_gbp *gbp;
>>>> -
>>>> - if (!md->gbp)
>>>> - return;
>>>> -
>>>> - gbp = (struct vxlanhdr_gbp *)vxh;
>>>> - vxh->vx_flags |= VXLAN_HF_GBP;
>>>> -
>>>> - if (md->gbp & VXLAN_GBP_DONT_LEARN)
>>>> - gbp->dont_learn = 1;
>>>> -
>>>> - if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
>>>> - gbp->policy_applied = 1;
>>>> -
>>>> - gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
>>>> -}
>>>> -
>>>> static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, __be16 protocol)
>>>> {
>>>> struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
>>>> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
>>>> index bca5b01af247..b6d419fa7ab1 100644
>>>> --- a/include/net/vxlan.h
>>>> +++ b/include/net/vxlan.h
>>>> @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
>>>> return true;
>>>> }
>>>>
>>>> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct vxlan_metadata *md)
>>>> +{
>>>> + struct vxlanhdr_gbp *gbp;
>>>> +
>>>> + if (!md->gbp)
>>>> + return;
>>>> +
>>>> + gbp = (struct vxlanhdr_gbp *)vxh;
>>>> + vxh->vx_flags |= VXLAN_HF_GBP;
>>>> +
>>>> + if (md->gbp & VXLAN_GBP_DONT_LEARN)
>>>> + gbp->dont_learn = 1;
>>>> +
>>>> + if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
>>>> + gbp->policy_applied = 1;
>>>> +
>>>> + gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
>>>> +}
>>>> +
>>>> #endif
>>>> --
>>>> 2.31.1
>>>>

2023-02-20 10:32:15

by Simon Horman

[permalink] [raw]
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

On Mon, Feb 20, 2023 at 03:15:20PM +0800, Gavin Li wrote:
>
> On 2/20/2023 2:40 PM, Simon Horman wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Mon, Feb 20, 2023 at 10:05:00AM +0800, Gavin Li wrote:
> > > On 2/20/2023 4:32 AM, Simon Horman wrote:
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
> > > > > vxlan_build_gbp_hdr will be used by other modules to build gbp option in
> > > > > vxlan header according to gbp flags.
> > > > >
> > > > > Signed-off-by: Gavin Li <[email protected]>
> > > > > Reviewed-by: Gavi Teitz <[email protected]>
> > > > > Reviewed-by: Roi Dayan <[email protected]>
> > > > > Reviewed-by: Maor Dickman <[email protected]>
> > > > > Acked-by: Saeed Mahameed <[email protected]>
> > > > I do wonder if this needs to be a static inline function.
> > > > But nonetheless,
> > > Will get "unused-function" from gcc without "inline"
> > >
> > > ./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but not
> > > used [-Wunused-function]
> > > static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
> > > vxlan_metadata *md)
> > Right. But what I was really wondering is if the definition
> > of the function could stay in drivers/net/vxlan/vxlan_core.c,
> > without being static. And have a declaration in include/net/vxlan.h
>
> Tried that the first time the function was called by driver code. It would
> introduce dependency in linking between the driver and the kernel module.
>
> Do you think it's OK to have such dependency?

IMHO, yes. But others may feel differently.

I do wonder if any performance overhead of a non-inline function
also needs to be considered.

2023-02-20 20:30:34

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

On Mon, 20 Feb 2023 11:31:59 +0100 Simon Horman wrote:
> On Mon, Feb 20, 2023 at 03:15:20PM +0800, Gavin Li wrote:
> > > Right. But what I was really wondering is if the definition
> > > of the function could stay in drivers/net/vxlan/vxlan_core.c,
> > > without being static. And have a declaration in include/net/vxlan.h
> >
> > Tried that the first time the function was called by driver code. It would
> > introduce dependency in linking between the driver and the kernel module.
> >
> > Do you think it's OK to have such dependency?
>
> IMHO, yes. But others may feel differently.
>
> I do wonder if any performance overhead of a non-inline function
> also needs to be considered.

Do you recall any details of why Hannes broke the dependency in the
first place?
Commit b7aade15485a ("vxlan: break dependency with netdev drivers")
Maybe we should stick to the static inline, it doesn't look too
large/terrible?

2023-02-21 07:39:29

by Paolo Abeni

[permalink] [raw]
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

On Mon, 2023-02-20 at 12:30 -0800, Jakub Kicinski wrote:
> On Mon, 20 Feb 2023 11:31:59 +0100 Simon Horman wrote:
> > On Mon, Feb 20, 2023 at 03:15:20PM +0800, Gavin Li wrote:
> > > > Right. But what I was really wondering is if the definition
> > > > of the function could stay in drivers/net/vxlan/vxlan_core.c,
> > > > without being static. And have a declaration in include/net/vxlan.h
> > >
> > > Tried that the first time the function was called by driver code. It would
> > > introduce dependency in linking between the driver and the kernel module.
> > >
> > > Do you think it's OK to have such dependency?
> >
> > IMHO, yes. But others may feel differently.
> >
> > I do wonder if any performance overhead of a non-inline function
> > also needs to be considered.
>
> Do you recall any details of why Hannes broke the dependency in the
> first place?

IIRC it was that was a cleanup thing, so that setup not using vxlan
does not load the module (and the related deps chain) for no reasons.

Cheers,

Paolo

> Commit b7aade15485a ("vxlan: break dependency with netdev drivers")
> Maybe we should stick to the static inline, it doesn't look too
> large/terrible?

IMHO static inline is good enough here.

Thanks,

Paolo


2023-02-21 09:31:16

by Simon Horman

[permalink] [raw]
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

On Tue, Feb 21, 2023 at 08:38:17AM +0100, Paolo Abeni wrote:
> On Mon, 2023-02-20 at 12:30 -0800, Jakub Kicinski wrote:
> > On Mon, 20 Feb 2023 11:31:59 +0100 Simon Horman wrote:
> > > On Mon, Feb 20, 2023 at 03:15:20PM +0800, Gavin Li wrote:
> > > > > Right. But what I was really wondering is if the definition
> > > > > of the function could stay in drivers/net/vxlan/vxlan_core.c,
> > > > > without being static. And have a declaration in include/net/vxlan.h
> > > >
> > > > Tried that the first time the function was called by driver code. It would
> > > > introduce dependency in linking between the driver and the kernel module.
> > > >
> > > > Do you think it's OK to have such dependency?
> > >
> > > IMHO, yes. But others may feel differently.
> > >
> > > I do wonder if any performance overhead of a non-inline function
> > > also needs to be considered.
> >
> > Do you recall any details of why Hannes broke the dependency in the
> > first place?
>
> IIRC it was that was a cleanup thing, so that setup not using vxlan
> does not load the module (and the related deps chain) for no reasons.
>
> Cheers,
>
> Paolo
>
> > Commit b7aade15485a ("vxlan: break dependency with netdev drivers")
> > Maybe we should stick to the static inline, it doesn't look too
> > large/terrible?
>
> IMHO static inline is good enough here.

Thanks Paolo and Jakub,

I do not recall the background to the change.
But your reasoning sounds good to me.

Let's stick with static inline.