| Message ID | 20230214134915.199004-1-jhs@mojatatu.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net/sched: Retire some tc qdiscs and classifiers | expand |
Refer to: https://lore.kernel.org/netdev/20230214134915.199004-1-jhs@mojatatu.com/T/#t I dont believe this has been done before so some introspection is needed. The option seem to be: 1) Delete it from the iproute2 version that maps to a kernel version 2) Keep it around and dont bother syncing specific kernel headers in the next version. This would allow newer iproute2 to continue supporting older kernels. Additionally, add an ominous message saying that the feature is no longer supported in newer kernels. cheers, jamal
Tue, Feb 14, 2023 at 02:49:10PM CET, jhs@mojatatu.com wrote: >The CBQ + dsmark qdiscs and the tcindex + rsvp classifiers have served us for >over 2 decades. Unfortunately, they have not been getting much attention due >to reduced usage. While we dont have a good metric for tabulating how much use >a specific kernel feature gets, for these specific features we observed that >some of the functionality has been broken for some time and no users complained. >In addition, syzkaller has been going to town on most of these and finding >issues; and while we have been fixing those issues, at times it becomes obvious >that we would need to perform bigger surgeries to resolve things found while >getting a syzkaller fix in place. After some discussion we feel that in order >to reduce the maintenance burden it is best to retire them. > >This patchset leaves the UAPI alone. I could send another version which deletes >the UAPI as well. AFAIK, this has not been done before - so it wasnt clear what >how to handle UAPI. It seems legit to just delete it but we would need to >coordinate with iproute2 (given they sync up with kernel uapi headers). There I think we have to let the UAPI there to rot in order not to break compilation of apps that use those (no relation to iproute2). >are probably other users we don't know of that copy kernel headers. >If folks feel differently I will resend the patches deleting UAPI for these >qdiscs and classifiers. > >I will start another thread on iproute2 before sending any patches to iproute2. > >Jamal Hadi Salim (5): > net/sched: Retire CBQ qdisc > net/sched: Retire ATM qdisc > net/sched: Retire dsmark qdisc > net/sched: Retire tcindex classifier > net/sched: Retire rsvp classifier set- Acked-by: Jiri Pirko <jiri@nvidia.com>
On Tue, 14 Feb 2023 15:25:40 +0100 Jiri Pirko wrote: > Tue, Feb 14, 2023 at 02:49:10PM CET, jhs@mojatatu.com wrote: > >The CBQ + dsmark qdiscs and the tcindex + rsvp classifiers have served us for > >over 2 decades. Unfortunately, they have not been getting much attention due > >to reduced usage. While we dont have a good metric for tabulating how much use > >a specific kernel feature gets, for these specific features we observed that > >some of the functionality has been broken for some time and no users complained. > >In addition, syzkaller has been going to town on most of these and finding > >issues; and while we have been fixing those issues, at times it becomes obvious > >that we would need to perform bigger surgeries to resolve things found while > >getting a syzkaller fix in place. After some discussion we feel that in order > >to reduce the maintenance burden it is best to retire them. > > > >This patchset leaves the UAPI alone. I could send another version which deletes > >the UAPI as well. AFAIK, this has not been done before - so it wasnt clear what > >how to handle UAPI. It seems legit to just delete it but we would need to > >coordinate with iproute2 (given they sync up with kernel uapi headers). There > > I think we have to let the UAPI there to rot in order not to break > compilation of apps that use those (no relation to iproute2). Yeah, I was hoping there's no other users but this is the first match on GitHub: https://github.com/t2mune/nield/blob/0c0848d1d1e6c006185465ee96aeb5a13a1589e6/src/tcmsg_qdisc_dsmark.c :(
On Tue, 14 Feb 2023 13:40:13 -0800 Jakub Kicinski wrote: > > I think we have to let the UAPI there to rot in order not to break > > compilation of apps that use those (no relation to iproute2). > > Yeah, I was hoping there's no other users but this is the first match > on GitHub: > > https://github.com/t2mune/nield/blob/0c0848d1d1e6c006185465ee96aeb5a13a1589e6/src/tcmsg_qdisc_dsmark.c > > :( I mean that in the context of deleting the uAPI, not the support, to be clear.
On Tue, Feb 14, 2023 at 4:41 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 14 Feb 2023 13:40:13 -0800 Jakub Kicinski wrote: > > > I think we have to let the UAPI there to rot in order not to break > > > compilation of apps that use those (no relation to iproute2). > > > > Yeah, I was hoping there's no other users but this is the first match > > on GitHub: > > > > https://github.com/t2mune/nield/blob/0c0848d1d1e6c006185465ee96aeb5a13a1589e6/src/tcmsg_qdisc_dsmark.c > > > > :( > > I mean that in the context of deleting the uAPI, not the support, > to be clear. Looking at that code - the user is keeping their own copy of the uapi and listening to generated events from the kernel. With new kernels those events will never come, so they wont be able to print them. IOW, there's no dependency on the uapi being in the kernel - but maybe worth keeping. Note, even if they were to send queries - they will just get a failure back. cheers, jamal
On Tue, 14 Feb 2023 18:05:23 -0500 Jamal Hadi Salim wrote: > Looking at that code - the user is keeping their own copy of the uapi > and listening to generated events from the kernel. I searched the repo for TCA_TCINDEX_CLASSID - I don't see the local copy of the header. Can you point me?
On Tue, Feb 14, 2023 at 6:22 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 14 Feb 2023 18:05:23 -0500 Jamal Hadi Salim wrote: > > Looking at that code - the user is keeping their own copy of the uapi > > and listening to generated events from the kernel. > > I searched the repo for TCA_TCINDEX_CLASSID - I don't see the local > copy of the header. Can you point me? Sorry, I was wrong, this tool's configure script pulls those headers on the host from whatever kernel uapi path (eg on my debian machine : /usr/src/linux/include ). If it doesnt find the headers it wont generate the build in the Makefile. So if at some point the uapi was deleted it just wont compile code to listen to cbq, dsmark etc. So it will work in either case (of deleting or keeping the uapi around). It may be a bad sample space - but if we do keep the uapi, how long would that be for? cheers, jamal
On Tue, 14 Feb 2023 18:52:18 -0500 Jamal Hadi Salim wrote: > So it will work in either case (of deleting or keeping the uapi > around). It may be a bad sample space - but if we do keep the uapi, > how long would that be for? Last time we deleted some ATM driver and the uAPI removal broke a random ATN package in debian, IIRC. The due diligence would probably mean trying to find out if there's anything in debian which needs those defines.. Is it possible to "download all the debian sources"? Or those which depend on the kernel headers?
Wed, Feb 15, 2023 at 12:05:23AM CET, jhs@mojatatu.com wrote: >On Tue, Feb 14, 2023 at 4:41 PM Jakub Kicinski <kuba@kernel.org> wrote: >> >> On Tue, 14 Feb 2023 13:40:13 -0800 Jakub Kicinski wrote: >> > > I think we have to let the UAPI there to rot in order not to break >> > > compilation of apps that use those (no relation to iproute2). >> > >> > Yeah, I was hoping there's no other users but this is the first match >> > on GitHub: >> > >> > https://github.com/t2mune/nield/blob/0c0848d1d1e6c006185465ee96aeb5a13a1589e6/src/tcmsg_qdisc_dsmark.c >> > >> > :( >> >> I mean that in the context of deleting the uAPI, not the support, >> to be clear. > >Looking at that code - the user is keeping their own copy of the uapi >and listening to generated events from the kernel. >With new kernels those events will never come, so they wont be able to >print them. IOW, there's no dependency on the uapi being in the kernel >- but maybe worth keeping. >Note, even if they were to send queries - they will just get a failure back. Guys. I don't think that matters. There might be some non-public code using this headers and we don't want to break them either. I believe we have to stick with it. But perhaps we can include some note there. > >cheers, >jamal
On 15/02/2023 04:50, Jiri Pirko wrote: > Wed, Feb 15, 2023 at 12:05:23AM CET, jhs@mojatatu.com wrote: >> On Tue, Feb 14, 2023 at 4:41 PM Jakub Kicinski <kuba@kernel.org> wrote: >>> >>> On Tue, 14 Feb 2023 13:40:13 -0800 Jakub Kicinski wrote: >>>>> I think we have to let the UAPI there to rot in order not to break >>>>> compilation of apps that use those (no relation to iproute2). >>>> >>>> Yeah, I was hoping there's no other users but this is the first match >>>> on GitHub: >>>> >>>> https://github.com/t2mune/nield/blob/0c0848d1d1e6c006185465ee96aeb5a13a1589e6/src/tcmsg_qdisc_dsmark.c >>>> >>>> :( >>> >>> I mean that in the context of deleting the uAPI, not the support, >>> to be clear. >> >> Looking at that code - the user is keeping their own copy of the uapi >> and listening to generated events from the kernel. >> With new kernels those events will never come, so they wont be able to >> print them. IOW, there's no dependency on the uapi being in the kernel >> - but maybe worth keeping. >> Note, even if they were to send queries - they will just get a failure back. > > Guys. I don't think that matters. There might be some non-public code > using this headers and we don't want to break them either. I believe we > have to stick with it. But perhaps we can include some note there. > I don't know the compilation requirements for uapi, but it could leverage the deprecated attribute, when available as a compiler extension (it's queued for C23 as well[0]). E.g. https://godbolt.org/z/d535jh9j8 Essentially anytime someone uses one of the Netlink attributes they will get a harmless warning like: <source>:22:18: warning: 'TCA_TCINDEX_CLASSID' is deprecated [-Wdeprecated-declarations] [0] https://en.cppreference.com/w/c/language/attributes/deprecated
On Wed, Feb 15, 2023 at 8:42 AM Pedro Tammela <pctammela@mojatatu.com> wrote: > > On 15/02/2023 04:50, Jiri Pirko wrote: > > Wed, Feb 15, 2023 at 12:05:23AM CET, jhs@mojatatu.com wrote: > >> On Tue, Feb 14, 2023 at 4:41 PM Jakub Kicinski <kuba@kernel.org> wrote: > >>> > >>> On Tue, 14 Feb 2023 13:40:13 -0800 Jakub Kicinski wrote: > >>>>> I think we have to let the UAPI there to rot in order not to break > >>>>> compilation of apps that use those (no relation to iproute2). > >>>> > >>>> Yeah, I was hoping there's no other users but this is the first match > >>>> on GitHub: > >>>> > >>>> https://github.com/t2mune/nield/blob/0c0848d1d1e6c006185465ee96aeb5a13a1589e6/src/tcmsg_qdisc_dsmark.c > >>>> > >>>> :( > >>> > >>> I mean that in the context of deleting the uAPI, not the support, > >>> to be clear. > >> > >> Looking at that code - the user is keeping their own copy of the uapi > >> and listening to generated events from the kernel. > >> With new kernels those events will never come, so they wont be able to > >> print them. IOW, there's no dependency on the uapi being in the kernel > >> - but maybe worth keeping. > >> Note, even if they were to send queries - they will just get a failure back. > > > > Guys. I don't think that matters. There might be some non-public code > > using this headers and we don't want to break them either. I believe we > > have to stick with it. But perhaps we can include some note there. > > > > I don't know the compilation requirements for uapi, but it could > leverage the deprecated attribute, when available as a compiler > extension (it's queued for C23 as well[0]). > > E.g. https://godbolt.org/z/d535jh9j8 > > Essentially anytime someone uses one of the Netlink attributes they will > get a harmless warning like: > <source>:22:18: warning: 'TCA_TCINDEX_CLASSID' is deprecated > [-Wdeprecated-declarations] > > [0] https://en.cppreference.com/w/c/language/attributes/deprecated That would work well actually. So iproute2 pulling these headers from the kernel will get warned. I think the outstanding question is still: How long do we keep the headers around? cheers, jamal
On Tue, 2023-02-14 at 08:49 -0500, Jamal Hadi Salim wrote: > The CBQ + dsmark qdiscs and the tcindex + rsvp classifiers have served us for > over 2 decades. Unfortunately, they have not been getting much attention due > to reduced usage. While we dont have a good metric for tabulating how much use > a specific kernel feature gets, for these specific features we observed that > some of the functionality has been broken for some time and no users complained. > In addition, syzkaller has been going to town on most of these and finding > issues; and while we have been fixing those issues, at times it becomes obvious > that we would need to perform bigger surgeries to resolve things found while > getting a syzkaller fix in place. After some discussion we feel that in order > to reduce the maintenance burden it is best to retire them. > > This patchset leaves the UAPI alone. I could send another version which deletes > the UAPI as well. AFAIK, this has not been done before - so it wasnt clear what > how to handle UAPI. It seems legit to just delete it but we would need to > coordinate with iproute2 (given they sync up with kernel uapi headers). There > are probably other users we don't know of that copy kernel headers. > If folks feel differently I will resend the patches deleting UAPI for these > qdiscs and classifiers. I guess we could additionally remove all the references to the retired qdiscs and classifiers from the default configs and from the self-tests dependencies. AFAICS such references do not cause any problem, as the now unexisting kconfig knobs are automatically stripped at config generation time by kbuild, but possibly worth a follow-up patch/series. Thanks! Paolo
Hello: This series was applied to netdev/net-next.git (master) by Paolo Abeni <pabeni@redhat.com>: On Tue, 14 Feb 2023 08:49:10 -0500 you wrote: > The CBQ + dsmark qdiscs and the tcindex + rsvp classifiers have served us for > over 2 decades. Unfortunately, they have not been getting much attention due > to reduced usage. While we dont have a good metric for tabulating how much use > a specific kernel feature gets, for these specific features we observed that > some of the functionality has been broken for some time and no users complained. > In addition, syzkaller has been going to town on most of these and finding > issues; and while we have been fixing those issues, at times it becomes obvious > that we would need to perform bigger surgeries to resolve things found while > getting a syzkaller fix in place. After some discussion we feel that in order > to reduce the maintenance burden it is best to retire them. > > [...] Here is the summary with links: - [net-next,1/5] net/sched: Retire CBQ qdisc https://git.kernel.org/netdev/net-next/c/051d44209842 - [net-next,2/5] net/sched: Retire ATM qdisc https://git.kernel.org/netdev/net-next/c/fb38306ceb9e - [net-next,3/5] net/sched: Retire dsmark qdisc https://git.kernel.org/netdev/net-next/c/bbe77c14ee61 - [net-next,4/5] net/sched: Retire tcindex classifier https://git.kernel.org/netdev/net-next/c/8c710f75256b - [net-next,5/5] net/sched: Retire rsvp classifier https://git.kernel.org/netdev/net-next/c/265b4da82dbf You are awesome, thank you!