[PATCH v2 net] net/sched: act_mpls: Add softdep on mpls_gso.ko

[PATCH v2 net] net/sched: act_mpls: Add softdep on mpls_gso.ko

[Guillaume Nault]

TCA_MPLS_ACT_PUSH and TCA_MPLS_ACT_MAC_PUSH might be used on gso
packets. Such packets will thus require mpls_gso.ko for segmentation.

v2: Drop dependency on CONFIG_NET_MPLS_GSO in Kconfig (from Jakub and
    David).

Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 net/sched/act_mpls.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index f40bf9771cb9..5c7456e5b5cf 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -426,6 +426,7 @@ static void __exit mpls_cleanup_module(void)
 module_init(mpls_init_module);
 module_exit(mpls_cleanup_module);
 
+MODULE_SOFTDEP("post: mpls_gso");
 MODULE_AUTHOR("Netronome Systems <oss-drivers@netronome.com>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("MPLS manipulation actions");
-- 
2.21.3

Re: [PATCH v2 net] net/sched: act_mpls: Add softdep on mpls_gso.ko

[Cong Wang]

On Mon, Oct 26, 2020 at 4:23 AM Guillaume Nault <gnault@redhat.com> wrote:
>
> TCA_MPLS_ACT_PUSH and TCA_MPLS_ACT_MAC_PUSH might be used on gso
> packets. Such packets will thus require mpls_gso.ko for segmentation.

Any reason not to call request_module() at run time?

Re: [PATCH v2 net] net/sched: act_mpls: Add softdep on mpls_gso.ko

[Guillaume Nault]

On Tue, Oct 27, 2020 at 10:28:29AM -0700, Cong Wang wrote:
> On Mon, Oct 26, 2020 at 4:23 AM Guillaume Nault <gnault@redhat.com> wrote:
> >
> > TCA_MPLS_ACT_PUSH and TCA_MPLS_ACT_MAC_PUSH might be used on gso
> > packets. Such packets will thus require mpls_gso.ko for segmentation.
> 
> Any reason not to call request_module() at run time?

So that mpls_gso would be loaded only when initialising the
TCA_MPLS_ACT_PUSH or TCA_MPLS_ACT_MAC_PUSH modes?

That could be done, but the dependency on mpls_gso wouldn't be visible
anymore with modinfo. I don't really mind, I just felt that such
information could be important for the end user.

Re: [PATCH v2 net] net/sched: act_mpls: Add softdep on mpls_gso.ko

[David Ahern]

On 10/27/20 3:39 PM, Guillaume Nault wrote:
> On Tue, Oct 27, 2020 at 10:28:29AM -0700, Cong Wang wrote:
>> On Mon, Oct 26, 2020 at 4:23 AM Guillaume Nault <gnault@redhat.com> wrote:
>>>
>>> TCA_MPLS_ACT_PUSH and TCA_MPLS_ACT_MAC_PUSH might be used on gso
>>> packets. Such packets will thus require mpls_gso.ko for segmentation.
>>
>> Any reason not to call request_module() at run time?
> 
> So that mpls_gso would be loaded only when initialising the
> TCA_MPLS_ACT_PUSH or TCA_MPLS_ACT_MAC_PUSH modes?
> 
> That could be done, but the dependency on mpls_gso wouldn't be visible
> anymore with modinfo. I don't really mind, I just felt that such
> information could be important for the end user.
> 

I think the explicit dependency via modinfo is better.

Re: [PATCH v2 net] net/sched: act_mpls: Add softdep on mpls_gso.ko

[Cong Wang]

On Tue, Oct 27, 2020 at 2:39 PM Guillaume Nault <gnault@redhat.com> wrote:
>
> On Tue, Oct 27, 2020 at 10:28:29AM -0700, Cong Wang wrote:
> > On Mon, Oct 26, 2020 at 4:23 AM Guillaume Nault <gnault@redhat.com> wrote:
> > >
> > > TCA_MPLS_ACT_PUSH and TCA_MPLS_ACT_MAC_PUSH might be used on gso
> > > packets. Such packets will thus require mpls_gso.ko for segmentation.
> >
> > Any reason not to call request_module() at run time?
>
> So that mpls_gso would be loaded only when initialising the
> TCA_MPLS_ACT_PUSH or TCA_MPLS_ACT_MAC_PUSH modes?

Yes, exactly.

>
> That could be done, but the dependency on mpls_gso wouldn't be visible
> anymore with modinfo. I don't really mind, I just felt that such
> information could be important for the end user.

I think the dependency is determined at run time based on
TCA_MPLS_ACT_*, so it should be reflected at run time, rather than at
compile time.

If loading mpls_gso even when not needed is not a big deal, I am fine
with your patch too.

Thanks.

Re: [PATCH v2 net] net/sched: act_mpls: Add softdep on mpls_gso.ko

[Guillaume Nault]

On Wed, Oct 28, 2020 at 12:35:17PM -0700, Cong Wang wrote:
> On Tue, Oct 27, 2020 at 2:39 PM Guillaume Nault <gnault@redhat.com> wrote:
> >
> > On Tue, Oct 27, 2020 at 10:28:29AM -0700, Cong Wang wrote:
> > > On Mon, Oct 26, 2020 at 4:23 AM Guillaume Nault <gnault@redhat.com> wrote:
> > > >
> > > > TCA_MPLS_ACT_PUSH and TCA_MPLS_ACT_MAC_PUSH might be used on gso
> > > > packets. Such packets will thus require mpls_gso.ko for segmentation.
> > >
> > > Any reason not to call request_module() at run time?
> >
> > So that mpls_gso would be loaded only when initialising the
> > TCA_MPLS_ACT_PUSH or TCA_MPLS_ACT_MAC_PUSH modes?
> 
> Yes, exactly.
> 
> >
> > That could be done, but the dependency on mpls_gso wouldn't be visible
> > anymore with modinfo. I don't really mind, I just felt that such
> > information could be important for the end user.
> 
> I think the dependency is determined at run time based on
> TCA_MPLS_ACT_*, so it should be reflected at run time, rather than at
> compile time.
> 
> If loading mpls_gso even when not needed is not a big deal, I am fine
> with your patch too.

Loading mpls_gso looks harmless. It just registers GSO handlers for
ETH_P_MPLS_UC and for ETH_P_MPLS_MC with a low priority.

Since we're not adding a build dependency on mpls_gso for act_mpls, I
have a slight preference for having the soft dependency being reported
by modinfo. This gives a chance to the user to figure out that mpls_gso
can be necessary.

> Thanks.
> 

Re: [PATCH v2 net] net/sched: act_mpls: Add softdep on mpls_gso.ko

[Jakub Kicinski]

On Mon, 26 Oct 2020 11:29:45 +0100 Guillaume Nault wrote:
> TCA_MPLS_ACT_PUSH and TCA_MPLS_ACT_MAC_PUSH might be used on gso
> packets. Such packets will thus require mpls_gso.ko for segmentation.
> 
> v2: Drop dependency on CONFIG_NET_MPLS_GSO in Kconfig (from Jakub and
>     David).
> 
> Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>

Applied, thanks!