Re: [PATCH v2 0/2] make POSIX timers optional
From: Nicolas Pitre Date: Tue, 20 Sep 2016 15:56:38 -0400 > Many embedded systems don't need the full POSIX timer support. > Configuring them out provides a nice kernel image size reduction. > > When POSIX timers are configured out, the PTP clock subsystem should be > left out as well. However a bunch of ethernet drivers currently *select* > it in their Kconfig entries. Therefore some more tweaks were needed to > break that hard dependency for those drivers to still be configured in > if desired. > > It was agreed that the best path upstream for those patches is via > John Stultz's timer tree. Acked-by: David S. Miller
Re: [PATCH v2 0/2] make POSIX timers optional
On Wed, 21 Sep 2016 10:38:52 +0200, Richard Cochran wrote: > Embedded people like to optimize their systems. One pattern I have > more than once is that a multihomed design designates a special PTP > interface, often with a different HW than the other ports. PTP > support adds extra code into the hot path, and for that reason people > want to turn it off on interfaces that don't need it. We really need to find some middle way. It's impossible to support every little corner case. As is not reasonable to have a single configuration only kernel. I think this scenario falls into the little corner case. If you need PTP, you just enable it. The PTP core will be much larger than the added code in the individual drivers anyway. And if there's run time impact on the fast path with time stamping switched off on an interface, it's a bug that has to be fixed. Jiri
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, 20 Sep 2016 23:09:52 +0200 (CEST), Thomas Gleixner wrote: > Now if you want to distangle PTP from a driver then you split it at the > driver level and not at the PTP level: > > DRIVER_X > tristate "Driver X" > > DRIVER_X_PTP > bool "Enable PTP support" > default y if !MAKE_IT_TINY > depends on DRIVER_X > select PTP Ouch. So, after the hassle to remove the VXLAN and GENEVE configs from tons of drivers, we'll add another one instead? That's just silly. If we did this for every thing we support in NICs, we would end up with something completely unmanageable. PTP should be really configured by a single switch. Btw., your suggestion does not work, select is not recursive. > We have already drivers following that scheme. That way you make the PTP > support in the driver conditional on DRIVER_X_PTP and have no hassle with > modules and dependencies. We've recently got rid of some of those, thankfully. Caused more harm than good. Jiri
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, 20 Sep 2016, Nicolas Pitre wrote: > There are way more drivers than subsystems and if you had to go around > unselecting all NIC drivers for CONFIG_ETHERNET to be turned off, and > with CONFIG_ETHERNET=n you'd finally be able to turn networking off, > then this would be a nightmare. It's also a nightmare to try to enable something which depends on a chain of config dependencies. You search A and see that it depends on B. Now you try to enable B and find out it depends on C. The deepest chain I found so far was 5 levels deep. That's horrible. > IMHO it is much nicer for the poor user configuring the kernel to have a > single configuration prompt for PTP support, and then have whatever > driver that can provide a PTP clock just do it (or omit it) based on > that single prompt. Prompting for PTP support for each individual > ethernet driver is silly. No, it's not. PTP support in a NIC can require a substantial amount of code and adds overhead in the hotpath. So if you have two NICs on your system and only use PTP on one of them then configuring out PTP for the other is desired. And talking about nightmares. With your patch I can now end up with the following configuration: CONFIG_NETWORK_DRIVER=y CONFIG_PTP_1588_CLOCK=m which makes the network driver to use the stub functions due to IS_REACHABLE(). That's going to be an utter nightmare for the poor user to figure out why PTP suddenly does not work anymore. That's simply crap. Thanks, tglx
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, Sep 20, 2016 at 06:47:02PM -0400, Nicolas Pitre wrote: > IMHO it is much nicer for the poor user configuring the kernel to have a > single configuration prompt for PTP support, and then have whatever > driver that can provide a PTP clock just do it (or omit it) based on > that single prompt. Prompting for PTP support for each individual > ethernet driver is silly. Embedded people like to optimize their systems. One pattern I have more than once is that a multihomed design designates a special PTP interface, often with a different HW than the other ports. PTP support adds extra code into the hot path, and for that reason people want to turn it off on interfaces that don't need it. So Thomas' idea is a better solution. It reduces the tinification in this area to a careful kernel configuration. Yes, that is more work to configure than having one "big red button" to push. The burden is on the tinification user to get this right, and that is the right way, IMHO. After all, this can be scripted, and such users have to configure very carefully in any case. Thanks, Richard
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, 20 Sep 2016, Thomas Gleixner wrote: > I think the whole approach is wrong because it makes the PTP split at the > wrong level. > > Currently we have: > > DRIVER_X > tristate "Driver X" > select PTP > > In order to make POSIX_CLOCK configurable we should have > > PTP > tristate "PTP" > select POSIX_CLOCK > > Now if you want to distangle PTP from a driver then you split it at the > driver level and not at the PTP level: > > DRIVER_X > tristate "Driver X" > > DRIVER_X_PTP > bool "Enable PTP support" > default y if !MAKE_IT_TINY > depends on DRIVER_X > select PTP > > We have already drivers following that scheme. That way you make the PTP > support in the driver conditional on DRIVER_X_PTP and have no hassle with > modules and dependencies. I beg to disagree. There are way more drivers than subsystems and if you had to go around unselecting all NIC drivers for CONFIG_ETHERNET to be turned off, and with CONFIG_ETHERNET=n you'd finally be able to turn networking off, then this would be a nightmare. IMHO it is much nicer for the poor user configuring the kernel to have a single configuration prompt for PTP support, and then have whatever driver that can provide a PTP clock just do it (or omit it) based on that single prompt. Prompting for PTP support for each individual ethernet driver is silly. Nicolas
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, 20 Sep 2016, Nicolas Pitre wrote: > On Tue, 20 Sep 2016, Richard Cochran wrote: > > > On Tue, Sep 20, 2016 at 10:25:56PM +0200, Richard Cochran wrote: > > > After this series, if I don't pay enough attention to dmesg, then I > > > have lost functionality that I had in step #1. That sucks, and it has > > > nothing to do with the tinification option at all. It will bite even > > > if I have no knowledge of it. That isn't acceptable to me. > > > > Can't you leave all the "select PTP_1588_CLOCK" alone and simply add > > > > #ifdef CONFIG_POSIX_TIMERS > > // global declarations > > #else > > // static inlines > > #endif Eew. No! That's an even more blantant layering violation. > > to ptp_clock_kernel.h, and then sandwich ptp_clock.c in > > #ifdef CONFIG_POSIX_TIMERS ... #endif ? > > Sure I could... but I'm sure I'll be flamed by others for making things > even more obscure and hackish than they are right now. I think the whole approach is wrong because it makes the PTP split at the wrong level. Currently we have: DRIVER_X tristate "Driver X" select PTP In order to make POSIX_CLOCK configurable we should have PTP tristate "PTP" select POSIX_CLOCK Now if you want to distangle PTP from a driver then you split it at the driver level and not at the PTP level: DRIVER_X tristate "Driver X" DRIVER_X_PTP bool "Enable PTP support" default y if !MAKE_IT_TINY depends on DRIVER_X select PTP We have already drivers following that scheme. That way you make the PTP support in the driver conditional on DRIVER_X_PTP and have no hassle with modules and dependencies. Your tiny config can simply disable all the PTP extra bits and then you can disable PTP and finally POSIX_TIMERS. Thanks, tglx
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, 20 Sep 2016, Richard Cochran wrote: > On Tue, Sep 20, 2016 at 10:25:56PM +0200, Richard Cochran wrote: > > After this series, if I don't pay enough attention to dmesg, then I > > have lost functionality that I had in step #1. That sucks, and it has > > nothing to do with the tinification option at all. It will bite even > > if I have no knowledge of it. That isn't acceptable to me. > > Can't you leave all the "select PTP_1588_CLOCK" alone and simply add > > #ifdef CONFIG_POSIX_TIMERS > // global declarations > #else > // static inlines > #endif > > to ptp_clock_kernel.h, and then sandwich ptp_clock.c in > #ifdef CONFIG_POSIX_TIMERS ... #endif ? Sure I could... but I'm sure I'll be flamed by others for making things even more obscure and hackish than they are right now. Oh well... Let's go fix the Kconfig parser then. Nicolas
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, 20 Sep 2016, Richard Cochran wrote: > On Tue, Sep 20, 2016 at 03:56:38PM -0400, Nicolas Pitre wrote: > > - Add a warning for the case where PTP clock subsystem is modular and a > > driver providing a clock is built-in rather than silently ignoring it. > > Suggested by Jiri Benc. > > So I am really not happy with this. Here is a common embedded > workflow, at least for me: > > 1. take some given Kconfig and get it running on the target. > > 2. for the given HW, change the modules into built-ins, and forget >module loading > > After this series, if I don't pay enough attention to dmesg, then I > have lost functionality that I had in step #1. Would that given config from #1 typically have CONFIG_EXPERT actually set? Ultimately, do you know a way to restrict a tristate to y or n? A tristate can be limited to m or n with "depends on m" but it doesn't appear to be possible to exclude m with a promotion to y. Nicolas
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, Sep 20, 2016 at 10:25:56PM +0200, Richard Cochran wrote: > After this series, if I don't pay enough attention to dmesg, then I > have lost functionality that I had in step #1. That sucks, and it has > nothing to do with the tinification option at all. It will bite even > if I have no knowledge of it. That isn't acceptable to me. Can't you leave all the "select PTP_1588_CLOCK" alone and simply add #ifdef CONFIG_POSIX_TIMERS // global declarations #else // static inlines #endif to ptp_clock_kernel.h, and then sandwich ptp_clock.c in #ifdef CONFIG_POSIX_TIMERS ... #endif ? Thanks, Richard
Re: [PATCH v2 0/2] make POSIX timers optional
On Tue, Sep 20, 2016 at 03:56:38PM -0400, Nicolas Pitre wrote: > - Add a warning for the case where PTP clock subsystem is modular and a > driver providing a clock is built-in rather than silently ignoring it. > Suggested by Jiri Benc. So I am really not happy with this. Here is a common embedded workflow, at least for me: 1. take some given Kconfig and get it running on the target. 2. for the given HW, change the modules into built-ins, and forget module loading After this series, if I don't pay enough attention to dmesg, then I have lost functionality that I had in step #1. That sucks, and it has nothing to do with the tinification option at all. It will bite even if I have no knowledge of it. That isn't acceptable to me. Thanks, Richard