Re: [PATCH v2 0/2] make POSIX timers optional

[David Miller]

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

[Jiri Benc]

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

[Jiri Benc]

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

[Thomas Gleixner]

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

[Richard Cochran]

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

[Nicolas Pitre]

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

[Thomas Gleixner]

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

[Nicolas Pitre]

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

[Nicolas Pitre]

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

[Richard Cochran]

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

[Richard Cochran]

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