| Message ID | 20221215020155.1619839-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: code split and structured instance walk | expand |
Thu, Dec 15, 2022 at 03:01:41AM CET, kuba@kernel.org wrote: >The devlink code is hard to navigate with 13kLoC in one file. >I really like the way Michal split the ethtool into per-command >files and core. It'd probably be too much to split it all up, Why not? While you are at it, I think that we should split it right away. >but we can at least separate the core parts out of the per-cmd >implementations and put it in a directory so that new commands >can be separate files. > >Move the code, subsequent commit will do a partial split. > >Signed-off-by: Jakub Kicinski <kuba@kernel.org> >--- > net/Makefile | 1 + > net/core/Makefile | 1 - > net/devlink/Makefile | 3 +++ > net/{core/devlink.c => devlink/basic.c} | 0 What's "basic" about it? It sounds a bit misleading. > 4 files changed, 4 insertions(+), 1 deletion(-) > create mode 100644 net/devlink/Makefile > rename net/{core/devlink.c => devlink/basic.c} (100%) > >diff --git a/net/Makefile b/net/Makefile >index 6a62e5b27378..0914bea9c335 100644 >--- a/net/Makefile >+++ b/net/Makefile >@@ -23,6 +23,7 @@ obj-$(CONFIG_BPFILTER) += bpfilter/ > obj-$(CONFIG_PACKET) += packet/ > obj-$(CONFIG_NET_KEY) += key/ > obj-$(CONFIG_BRIDGE) += bridge/ >+obj-$(CONFIG_NET_DEVLINK) += devlink/ Hmm, as devlink is not really designed to be only networking thing, perhaps this is good opportunity to move out of net/ and change the config name to "CONFIG_DEVLINK" ? > obj-$(CONFIG_NET_DSA) += dsa/ > obj-$(CONFIG_ATALK) += appletalk/ > obj-$(CONFIG_X25) += x25/ >diff --git a/net/core/Makefile b/net/core/Makefile >index 5857cec87b83..10edd66a8a37 100644 >--- a/net/core/Makefile >+++ b/net/core/Makefile >@@ -33,7 +33,6 @@ obj-$(CONFIG_LWTUNNEL) += lwtunnel.o > obj-$(CONFIG_LWTUNNEL_BPF) += lwt_bpf.o > obj-$(CONFIG_DST_CACHE) += dst_cache.o > obj-$(CONFIG_HWBM) += hwbm.o >-obj-$(CONFIG_NET_DEVLINK) += devlink.o > obj-$(CONFIG_GRO_CELLS) += gro_cells.o > obj-$(CONFIG_FAILOVER) += failover.o > obj-$(CONFIG_NET_SOCK_MSG) += skmsg.o >diff --git a/net/devlink/Makefile b/net/devlink/Makefile >new file mode 100644 >index 000000000000..ba54922128ab >--- /dev/null >+++ b/net/devlink/Makefile >@@ -0,0 +1,3 @@ >+# SPDX-License-Identifier: GPL-2.0 >+ >+obj-y := basic.o >diff --git a/net/core/devlink.c b/net/devlink/basic.c >similarity index 100% >rename from net/core/devlink.c >rename to net/devlink/basic.c >-- >2.38.1 >
On 12/15/2022 1:51 AM, Jiri Pirko wrote: > Thu, Dec 15, 2022 at 03:01:41AM CET, kuba@kernel.org wrote: >> The devlink code is hard to navigate with 13kLoC in one file. >> I really like the way Michal split the ethtool into per-command >> files and core. It'd probably be too much to split it all up, > > Why not? While you are at it, I think that we should split it right > away. > From the cover letter it sounds like "not enough time". I like splitting things up but would be fine with just one file per sub object type like one for ports, one for regions, etc. Thanks, Jake
On Thu, 15 Dec 2022 10:51:43 +0100 Jiri Pirko wrote: > > net/Makefile | 1 + > > net/core/Makefile | 1 - > > net/devlink/Makefile | 3 +++ > > net/{core/devlink.c => devlink/basic.c} | 0 > > What's "basic" about it? It sounds a bit misleading. Agreed, but try to suggest a better name ;) the_rest_of_it.c ? :) > > 4 files changed, 4 insertions(+), 1 deletion(-) > > create mode 100644 net/devlink/Makefile > > rename net/{core/devlink.c => devlink/basic.c} (100%) > > > >diff --git a/net/Makefile b/net/Makefile > >index 6a62e5b27378..0914bea9c335 100644 > >--- a/net/Makefile > >+++ b/net/Makefile > >@@ -23,6 +23,7 @@ obj-$(CONFIG_BPFILTER) += bpfilter/ > > obj-$(CONFIG_PACKET) += packet/ > > obj-$(CONFIG_NET_KEY) += key/ > > obj-$(CONFIG_BRIDGE) += bridge/ > >+obj-$(CONFIG_NET_DEVLINK) += devlink/ > > Hmm, as devlink is not really designed to be only networking thing, > perhaps this is good opportunity to move out of net/ and change the > config name to "CONFIG_DEVLINK" ? Nothing against it, but don't think it belongs in this patch. So I call scope creep.
On Thu, 15 Dec 2022 10:44:14 -0800 Jacob Keller wrote: > On 12/15/2022 1:51 AM, Jiri Pirko wrote: > > Thu, Dec 15, 2022 at 03:01:41AM CET, kuba@kernel.org wrote: > >> The devlink code is hard to navigate with 13kLoC in one file. > >> I really like the way Michal split the ethtool into per-command > >> files and core. It'd probably be too much to split it all up, > > > > Why not? While you are at it, I think that we should split it right > > away. > > From the cover letter it sounds like "not enough time". I like > splitting things up but would be fine with just one file per sub object > type like one for ports, one for regions, etc. Indeed, I had to draw the line somewhere. Always plenty of opportunities to reshuffle and improve code which has grown organically over the years. But the sun goes down obsitnately...
On 12/15/2022 11:09 AM, Jakub Kicinski wrote: > On Thu, 15 Dec 2022 10:51:43 +0100 Jiri Pirko wrote: >>> net/Makefile | 1 + >>> net/core/Makefile | 1 - >>> net/devlink/Makefile | 3 +++ >>> net/{core/devlink.c => devlink/basic.c} | 0 >> >> What's "basic" about it? It sounds a bit misleading. > > Agreed, but try to suggest a better name ;) the_rest_of_it.c ? :) > I tried to think of something, but you already use core elsewhere in the series. If our long term goal really is to split everything out then maybe "leftover.c"? Or just "devlink/devlink.c" >>> 4 files changed, 4 insertions(+), 1 deletion(-) >>> create mode 100644 net/devlink/Makefile >>> rename net/{core/devlink.c => devlink/basic.c} (100%) >>> >>> diff --git a/net/Makefile b/net/Makefile >>> index 6a62e5b27378..0914bea9c335 100644 >>> --- a/net/Makefile >>> +++ b/net/Makefile >>> @@ -23,6 +23,7 @@ obj-$(CONFIG_BPFILTER) += bpfilter/ >>> obj-$(CONFIG_PACKET) += packet/ >>> obj-$(CONFIG_NET_KEY) += key/ >>> obj-$(CONFIG_BRIDGE) += bridge/ >>> +obj-$(CONFIG_NET_DEVLINK) += devlink/ >> >> Hmm, as devlink is not really designed to be only networking thing, >> perhaps this is good opportunity to move out of net/ and change the >> config name to "CONFIG_DEVLINK" ? > > Nothing against it, but don't think it belongs in this patch. > So I call scope creep. Right. I think this patch set focusing on just the iteration for dumping is good.
On Thu, 15 Dec 2022 11:29:02 -0800 Jacob Keller wrote: > >> What's "basic" about it? It sounds a bit misleading. > > > > Agreed, but try to suggest a better name ;) the_rest_of_it.c ? :) > > I tried to think of something, but you already use core elsewhere in the > series. If our long term goal really is to split everything out then > maybe "leftover.c"? Or just "devlink/devlink.c" leftover.c is fine by me. Jiri?
Thu, Dec 15, 2022 at 08:48:29PM CET, kuba@kernel.org wrote: >On Thu, 15 Dec 2022 11:29:02 -0800 Jacob Keller wrote: >> >> What's "basic" about it? It sounds a bit misleading. >> > >> > Agreed, but try to suggest a better name ;) the_rest_of_it.c ? :) >> >> I tried to think of something, but you already use core elsewhere in the >> series. If our long term goal really is to split everything out then >> maybe "leftover.c"? Or just "devlink/devlink.c" > >leftover.c is fine by me. Jiri? If the goal is to remove the file entirely eventually, I'm okay with that.
Thu, Dec 15, 2022 at 08:09:25PM CET, kuba@kernel.org wrote: >On Thu, 15 Dec 2022 10:51:43 +0100 Jiri Pirko wrote: >> > net/Makefile | 1 + >> > net/core/Makefile | 1 - >> > net/devlink/Makefile | 3 +++ >> > net/{core/devlink.c => devlink/basic.c} | 0 >> >> What's "basic" about it? It sounds a bit misleading. > >Agreed, but try to suggest a better name ;) the_rest_of_it.c ? :) > >> > 4 files changed, 4 insertions(+), 1 deletion(-) >> > create mode 100644 net/devlink/Makefile >> > rename net/{core/devlink.c => devlink/basic.c} (100%) >> > >> >diff --git a/net/Makefile b/net/Makefile >> >index 6a62e5b27378..0914bea9c335 100644 >> >--- a/net/Makefile >> >+++ b/net/Makefile >> >@@ -23,6 +23,7 @@ obj-$(CONFIG_BPFILTER) += bpfilter/ >> > obj-$(CONFIG_PACKET) += packet/ >> > obj-$(CONFIG_NET_KEY) += key/ >> > obj-$(CONFIG_BRIDGE) += bridge/ >> >+obj-$(CONFIG_NET_DEVLINK) += devlink/ >> >> Hmm, as devlink is not really designed to be only networking thing, >> perhaps this is good opportunity to move out of net/ and change the >> config name to "CONFIG_DEVLINK" ? > >Nothing against it, but don't think it belongs in this patch. >So I call scope creep. Yeah, but I mean, since you move it from /net/core to /net/, why not just move it to / ?
On Fri, 16 Dec 2022 10:10:41 +0100 Jiri Pirko wrote: > >> Hmm, as devlink is not really designed to be only networking thing, > >> perhaps this is good opportunity to move out of net/ and change the > >> config name to "CONFIG_DEVLINK" ? > > > >Nothing against it, but don't think it belongs in this patch. > >So I call scope creep. > > Yeah, but I mean, since you move it from /net/core to /net/, why not > just move it to / ? It still needs to depend on NET for sockets, right?
On 12/16/2022 10:32 AM, Jakub Kicinski wrote: > On Fri, 16 Dec 2022 10:10:41 +0100 Jiri Pirko wrote: >>>> Hmm, as devlink is not really designed to be only networking thing, >>>> perhaps this is good opportunity to move out of net/ and change the >>>> config name to "CONFIG_DEVLINK" ? >>> >>> Nothing against it, but don't think it belongs in this patch. >>> So I call scope creep. >> >> Yeah, but I mean, since you move it from /net/core to /net/, why not >> just move it to / ? > > It still needs to depend on NET for sockets, right? Its going to depend on whatever Generic Netlink depends on yea.
diff --git a/net/Makefile b/net/Makefile index 6a62e5b27378..0914bea9c335 100644 --- a/net/Makefile +++ b/net/Makefile @@ -23,6 +23,7 @@ obj-$(CONFIG_BPFILTER) += bpfilter/ obj-$(CONFIG_PACKET) += packet/ obj-$(CONFIG_NET_KEY) += key/ obj-$(CONFIG_BRIDGE) += bridge/ +obj-$(CONFIG_NET_DEVLINK) += devlink/ obj-$(CONFIG_NET_DSA) += dsa/ obj-$(CONFIG_ATALK) += appletalk/ obj-$(CONFIG_X25) += x25/ diff --git a/net/core/Makefile b/net/core/Makefile index 5857cec87b83..10edd66a8a37 100644 --- a/net/core/Makefile +++ b/net/core/Makefile @@ -33,7 +33,6 @@ obj-$(CONFIG_LWTUNNEL) += lwtunnel.o obj-$(CONFIG_LWTUNNEL_BPF) += lwt_bpf.o obj-$(CONFIG_DST_CACHE) += dst_cache.o obj-$(CONFIG_HWBM) += hwbm.o -obj-$(CONFIG_NET_DEVLINK) += devlink.o obj-$(CONFIG_GRO_CELLS) += gro_cells.o obj-$(CONFIG_FAILOVER) += failover.o obj-$(CONFIG_NET_SOCK_MSG) += skmsg.o diff --git a/net/devlink/Makefile b/net/devlink/Makefile new file mode 100644 index 000000000000..ba54922128ab --- /dev/null +++ b/net/devlink/Makefile @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0 + +obj-y := basic.o diff --git a/net/core/devlink.c b/net/devlink/basic.c similarity index 100% rename from net/core/devlink.c rename to net/devlink/basic.c
The devlink code is hard to navigate with 13kLoC in one file. I really like the way Michal split the ethtool into per-command files and core. It'd probably be too much to split it all up, but we can at least separate the core parts out of the per-cmd implementations and put it in a directory so that new commands can be separate files. Move the code, subsequent commit will do a partial split. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- net/Makefile | 1 + net/core/Makefile | 1 - net/devlink/Makefile | 3 +++ net/{core/devlink.c => devlink/basic.c} | 0 4 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 net/devlink/Makefile rename net/{core/devlink.c => devlink/basic.c} (100%)