| Message ID | 1f6cab15bbd15666795061c55563aaf6a386e90e.1603708007.git.gnault@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net/sched: act_mpls: Add softdep on mpls_gso.ko | expand |
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?
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.
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.
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!
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.
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. >
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");
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(+)