Suggestion - Configurable Source Clock Type for Line Event Timestamping

Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Jack Winch]

Hi Folks,

I recently noticed that in Linux 5.7, gpiolib was changed such that
line events are now timestamped using the system 'monotonic' clock
rather than the system realtime clock.  The rationale for this change
appears to be due to the major use-case of the line event timestamp
data in relation to the nature of the system realtime clock (which can
jump backwards or forwards in time due to adjustments by third-parties
- e.g., NTP or PTP clients, etc).

For most users of the line event timestamp value, the use of the
realtime clock could be problematic due to the potential for
chronological line events to receive timestamp values with a
non-chronological progression (resulting from adjustments being made
to the clock).  This could be the source of a number of bugs,
functional limitations and frustrations which was solved easily enough
by transitioning to the use of the system monotonic clock.  That being
said, I know there are users of the line event timestamp who actively
rely on that value being obtained from the system realtime clock.

My suggestion (which I would be happy to implement myself) is to allow
users to select the clock to be used for line event timestamping on a
per line handle basis.  The merit of this approach is that the
appropriate clock type may be selected on a per line handle basis
according to the needs of the user.  This of course has some
implications which are not desirable without merit, but may be deemed
acceptable in balance with the resultant functionality.  In summary,
these are:

1. Increase in processing overhead and latency of timestamp
acquisition on line event interrupts.  Implementing the proposed
change requires a function call to be made to the appropriate ktime
accessor function, based on what the user has configured as the
timestamp clock source.  In kernel versions from 5.7 to current, a
call is made to the ktime_get_ns() function which is most likely
inlined by the compiler.  This change will result in an actual jump
having to be made, which will have processor and memory access
overhead (potential I$ and D$ misses).  Then there is of course the
overhead of resolving which function to call - either a switch
statement or call by function pointer (probably the latter option).

2. Additions required to the userspace ABI.  Additional IOCTLs will be
required for line handles, allowing the source clock type for line
event timestamping to be get or set.

3. Additions required to libgpiod.  The existing API will have to be
added to in order to provide an abstraction for this new
functionality.  This requires changes to the core C library, as well
as the provided C++ and Python bindings (and their test cases).
Changes will also be required to the WiP libgpiod service and its
d-bus interface.  This change will also affect the proposed future
lightweight libgpiod service.

4. Documentation for both the GPIO subsystem and libgpiod will require
updating.  This should be done as part of the effort to implement this
functionality (if agreed upon) for the target version of the kernel
and libgpiod.

Such that applications now relying on the use of the 'monotonic'
system clock for timestamping line events do not require modification
after the implementation of this functionality (most applications), I
propose the 'monotonic' system clock be the default source clock.  If
the user wants to change this to another clock type, then they may do
so via the proposed additional IOCTLs and / or the proposed changes to
libgpiod.

I would be interested in hearing your thoughts on this suggestion / proposal.

~ Jack

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Kent Gibson]

On Sun, Oct 11, 2020 at 03:15:01PM +0100, Jack Winch wrote:
> Hi Folks,
> 
> I recently noticed that in Linux 5.7, gpiolib was changed such that
> line events are now timestamped using the system 'monotonic' clock
> rather than the system realtime clock.  The rationale for this change
> appears to be due to the major use-case of the line event timestamp
> data in relation to the nature of the system realtime clock (which can
> jump backwards or forwards in time due to adjustments by third-parties
> - e.g., NTP or PTP clients, etc).
> 
> For most users of the line event timestamp value, the use of the
> realtime clock could be problematic due to the potential for
> chronological line events to receive timestamp values with a
> non-chronological progression (resulting from adjustments being made
> to the clock).  This could be the source of a number of bugs,
> functional limitations and frustrations which was solved easily enough
> by transitioning to the use of the system monotonic clock.  That being
> said, I know there are users of the line event timestamp who actively
> rely on that value being obtained from the system realtime clock.
> 
> My suggestion (which I would be happy to implement myself) is to allow
> users to select the clock to be used for line event timestamping on a
> per line handle basis.  The merit of this approach is that the
> appropriate clock type may be selected on a per line handle basis
> according to the needs of the user.  This of course has some
> implications which are not desirable without merit, but may be deemed
> acceptable in balance with the resultant functionality.  In summary,
> these are:
> 
> 1. Increase in processing overhead and latency of timestamp
> acquisition on line event interrupts.  Implementing the proposed
> change requires a function call to be made to the appropriate ktime
> accessor function, based on what the user has configured as the
> timestamp clock source.  In kernel versions from 5.7 to current, a
> call is made to the ktime_get_ns() function which is most likely
> inlined by the compiler.  This change will result in an actual jump
> having to be made, which will have processor and memory access
> overhead (potential I$ and D$ misses).  Then there is of course the
> overhead of resolving which function to call - either a switch
> statement or call by function pointer (probably the latter option).
> 
> 2. Additions required to the userspace ABI.  Additional IOCTLs will be
> required for line handles, allowing the source clock type for line
> event timestamping to be get or set.
> 
> 3. Additions required to libgpiod.  The existing API will have to be
> added to in order to provide an abstraction for this new
> functionality.  This requires changes to the core C library, as well
> as the provided C++ and Python bindings (and their test cases).
> Changes will also be required to the WiP libgpiod service and its
> d-bus interface.  This change will also affect the proposed future
> lightweight libgpiod service.
> 
> 4. Documentation for both the GPIO subsystem and libgpiod will require
> updating.  This should be done as part of the effort to implement this
> functionality (if agreed upon) for the target version of the kernel
> and libgpiod.
> 
> Such that applications now relying on the use of the 'monotonic'
> system clock for timestamping line events do not require modification
> after the implementation of this functionality (most applications), I
> propose the 'monotonic' system clock be the default source clock.  If
> the user wants to change this to another clock type, then they may do
> so via the proposed additional IOCTLs and / or the proposed changes to
> libgpiod.
> 
> I would be interested in hearing your thoughts on this suggestion / proposal.
> 

Hi Jack,

Adding Bart in - in case he hasn't noticed this one, as he is the
maintainer for the chardev and libgpiod.

Firstly, mapping from MONOTONIC to REALTIME is quite doable in
userspace, so I'd be more inclined to add a helper to libgpiod to do
that for you.

Secondly, an updated version of the uAPI, v2, is in line to be included
in Linux 5.10, so you should look at that.  Most of your points are
still valid as the MONOTONIC event timestamp is inherited from v1, but
there are more options available should we want to address this in kernel.
e.g. we could return BOTH timestamps in the event.

Also, assuming you were to implement this in kernel, it doesn't need a new
ioctl. It could be implemented by extending the flags field.

But I would want to rule out a userspace solution before entertaining a
kernel change.

Cheers,
Kent.

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Jack Winch]

Hi Kent,

Thanks for the quick response. It was originally my intention to CC
the pair of you on my original email, but I wasn't sure if it was 'the
done thing'.

For most of the users I previously referred to, minimum timestamping
latency (using the realtime clock as the source) is crucial. A
userspace solution might be suitable for the others, but for these
wall clock time sensitive applications the acquisition of a timestamp
value from the system realtime clock is required within the interrupt
handling code.

For context, these wall clock time sensitive users are running on
systems which are PTPv2 clients, with their system realtime clock
synchronised to that of a local PTP Grand Master clock.  In the past,
I have used the TTL Pulse Per Second (PPS) output of the Grand Master
to evaluate methods of timestamping line events with wall clock time
and it was the kernel timestamping which was most suitable for our
application.

Another way to skin the cat could be to create separate kernel modules
for these applications, with them acting as a consumer to the GPIO
subsystem.  That way, interrupts could be setup and handled for line
events, with these application specific kernel modules undertaking the
timestamping using the realtime system clock within the module ISRs.
But that would have to be assessed.

I still believe adding this functionality to the chardev would be
beneficial for users, although I understand your preference for other
solutions first.

Regarding the extending of the flags field, you're absolutely right.
One thing I'd have to go over is how changes to the use of that flag
field could effect other parts of the subsystem.  I would expect that
this change will only be utilised by the chardev in the first instance
however.

I also have a couple of other queries regarding the current and future
state of libgpiod, but I will submit those via a separate thread of
discussion in order to keep each discussion appropriately partitioned.

Thanks,
Jack

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Kent Gibson]

On Mon, Oct 12, 2020 at 08:23:09AM +0100, Jack Winch wrote:
> Hi Kent,
> 
> Thanks for the quick response. It was originally my intention to CC
> the pair of you on my original email, but I wasn't sure if it was 'the
> done thing'.
> 

It probably couldn't hurt, as would prefixing your subject with
[libgpiod] if it is only relevant to the chardev and libgpiod.

And including the relevant sections of the mail you are replying to helps
as well.

> For most of the users I previously referred to, minimum timestamping
> latency (using the realtime clock as the source) is crucial. A
> userspace solution might be suitable for the others, but for these
> wall clock time sensitive applications the acquisition of a timestamp
> value from the system realtime clock is required within the interrupt
> handling code.
> 
> For context, these wall clock time sensitive users are running on
> systems which are PTPv2 clients, with their system realtime clock
> synchronised to that of a local PTP Grand Master clock.  In the past,
> I have used the TTL Pulse Per Second (PPS) output of the Grand Master
> to evaluate methods of timestamping line events with wall clock time
> and it was the kernel timestamping which was most suitable for our
> application.
> 

So a mapping from the MONOTONIC timestamp, taken in the ISR and returned
in the event, to the equivalent REALTIME timestamp is not reliable as
there is jitter between the two clocks?

Cheers,
Kent.

> Another way to skin the cat could be to create separate kernel modules
> for these applications, with them acting as a consumer to the GPIO
> subsystem.  That way, interrupts could be setup and handled for line
> events, with these application specific kernel modules undertaking the
> timestamping using the realtime system clock within the module ISRs.
> But that would have to be assessed.
> 
> I still believe adding this functionality to the chardev would be
> beneficial for users, although I understand your preference for other
> solutions first.
> 
> Regarding the extending of the flags field, you're absolutely right.
> One thing I'd have to go over is how changes to the use of that flag
> field could effect other parts of the subsystem.  I would expect that
> this change will only be utilised by the chardev in the first instance
> however.
> 
> I also have a couple of other queries regarding the current and future
> state of libgpiod, but I will submit those via a separate thread of
> discussion in order to keep each discussion appropriately partitioned.
> 
> Thanks,
> Jack

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Bartosz Golaszewski]

On Sun, Oct 11, 2020 at 5:11 PM Jack Winch <sunt.un.morcov@gmail.com> wrote:
>
> Hi Folks,
>

Hi!

Thanks Kent for bringing this to my attention. I've Cc'd even more
people who may be interested in this discussion.

> I recently noticed that in Linux 5.7, gpiolib was changed such that
> line events are now timestamped using the system 'monotonic' clock
> rather than the system realtime clock.  The rationale for this change
> appears to be due to the major use-case of the line event timestamp
> data in relation to the nature of the system realtime clock (which can
> jump backwards or forwards in time due to adjustments by third-parties
> - e.g., NTP or PTP clients, etc).
>
> For most users of the line event timestamp value, the use of the
> realtime clock could be problematic due to the potential for
> chronological line events to receive timestamp values with a
> non-chronological progression (resulting from adjustments being made
> to the clock).  This could be the source of a number of bugs,
> functional limitations and frustrations which was solved easily enough
> by transitioning to the use of the system monotonic clock.  That being
> said, I know there are users of the line event timestamp who actively
> rely on that value being obtained from the system realtime clock.
>

Technically we're not allowed to break user-space so had anyone
actually complained we would be forced to revert this.

> My suggestion (which I would be happy to implement myself) is to allow
> users to select the clock to be used for line event timestamping on a
> per line handle basis.  The merit of this approach is that the
> appropriate clock type may be selected on a per line handle basis
> according to the needs of the user.  This of course has some
> implications which are not desirable without merit, but may be deemed
> acceptable in balance with the resultant functionality.  In summary,
> these are:
>

We still haven't released uAPI v2 so I'm open to some last-minute
changes if they make sense (as you explained in the other email about
in-kernel timestamping) and are posted soon (before rc3-rc4). I'd like
to hear Arnd's opinion on this first though.

As for v1 - I think we all agree that it won't be getting any new
features anymore.

> 1. Increase in processing overhead and latency of timestamp
> acquisition on line event interrupts.  Implementing the proposed
> change requires a function call to be made to the appropriate ktime
> accessor function, based on what the user has configured as the
> timestamp clock source.  In kernel versions from 5.7 to current, a
> call is made to the ktime_get_ns() function which is most likely
> inlined by the compiler.  This change will result in an actual jump
> having to be made, which will have processor and memory access
> overhead (potential I$ and D$ misses).  Then there is of course the
> overhead of resolving which function to call - either a switch
> statement or call by function pointer (probably the latter option).
>
> 2. Additions required to the userspace ABI.  Additional IOCTLs will be
> required for line handles, allowing the source clock type for line
> event timestamping to be get or set.
>
> 3. Additions required to libgpiod.  The existing API will have to be
> added to in order to provide an abstraction for this new
> functionality.  This requires changes to the core C library, as well
> as the provided C++ and Python bindings (and their test cases).
> Changes will also be required to the WiP libgpiod service and its
> d-bus interface.  This change will also affect the proposed future
> lightweight libgpiod service.
>

I'm not sure if you're referring to my incomplete d-bus interface I
worked on some time ago and never finished (I plan to get back to it
once libgpiod v2.0 is out) or something else? And what is libgpiod
service and lightweight libgpiod service?

Backward incompatible changes in libgpiod are currently fine - we're
working on a new major release to provide support for new features of
uAPI v2 so it's the best moment to propose them.

> 4. Documentation for both the GPIO subsystem and libgpiod will require
> updating.  This should be done as part of the effort to implement this
> functionality (if agreed upon) for the target version of the kernel
> and libgpiod.
>
> Such that applications now relying on the use of the 'monotonic'
> system clock for timestamping line events do not require modification
> after the implementation of this functionality (most applications), I
> propose the 'monotonic' system clock be the default source clock.  If
> the user wants to change this to another clock type, then they may do
> so via the proposed additional IOCTLs and / or the proposed changes to
> libgpiod.
>
> I would be interested in hearing your thoughts on this suggestion / proposal.
>
> ~ Jack

Bartosz

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Jack Winch]

> It probably couldn't hurt, as would prefixing your subject with
> [libgpiod] if it is only relevant to the chardev and libgpiod.
> And including the relevant sections of the mail you are replying to helps
> as well.


Thanks for the pointers.

> So a mapping from the MONOTONIC timestamp, taken in the ISR and returned
> in the event, to the equivalent REALTIME timestamp is not reliable as
> there is jitter between the two clocks?


That is correct.  There are also other implications due to how
adjustments are made to the realtime clock by a system's PTP client
service.  The PTPv2 standard (IEEE 1588:2008) does not specify how the
calculated offset between a master and a slave clock is used to adjust
the slave clock, such that the slave clock is brought into
coordination with the master clock.  This procedure is called 'clock
discipline' and there are numerous ways of achieving it.

So the short answer to that question is it is better 'not to d*ck with
the data' and to obtain timestamp values directly from the realtime
clock when handling an event, rather than using the 'monotonic' clock
to try to resolve a realtime value.

> Technically we're not allowed to break user-space so had anyone
> actually complained we would be forced to revert this.


That was my initial thought, but for most users, they do want a
'monotonic' time value for the event timestamp.  So this functionality
really ought to be available to them (for they are in the majority).
I have only noticed the change recently due to the use of downstream
versions of the kernel.

> We still haven't released uAPI v2 so I'm open to some last-minute
> changes if they make sense (as you explained in the other email about
> in-kernel timestamping) and are posted soon (before rc3-rc4). I'd like
> to hear Arnd's opinion on this first though.


What's the timescales for this?  As I would be doing this in a
personal capacity, I will likely have to look at this over a couple of
weeks.  I would also be limited to testing on an ARMv8 platform, as I
currently have limited access to hardware.

> I'm not sure if you're referring to my incomplete d-bus interface I
> worked on some time ago and never finished (I plan to get back to it
> once libgpiod v2.0 is out) or something else? And what is libgpiod
> service and lightweight libgpiod service?


That's the one.  I asked you about its progress back in Feb, but it
seemed to be something on the backburner.  That's what I meant by
'libgpiod service' for want of a better term.

And by 'lightweight libgpiod service', I mean the planned minimal
server application which makes use of minimal dependencies and a
socket-based interface (as opposed to d-bus) to provide similar
capability on resource constrained devices.

Jack


On Mon, Oct 12, 2020 at 11:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Sun, Oct 11, 2020 at 5:11 PM Jack Winch <sunt.un.morcov@gmail.com> wrote:
> >
> > Hi Folks,
> >
>
> Hi!
>
> Thanks Kent for bringing this to my attention. I've Cc'd even more
> people who may be interested in this discussion.
>
> > I recently noticed that in Linux 5.7, gpiolib was changed such that
> > line events are now timestamped using the system 'monotonic' clock
> > rather than the system realtime clock.  The rationale for this change
> > appears to be due to the major use-case of the line event timestamp
> > data in relation to the nature of the system realtime clock (which can
> > jump backwards or forwards in time due to adjustments by third-parties
> > - e.g., NTP or PTP clients, etc).
> >
> > For most users of the line event timestamp value, the use of the
> > realtime clock could be problematic due to the potential for
> > chronological line events to receive timestamp values with a
> > non-chronological progression (resulting from adjustments being made
> > to the clock).  This could be the source of a number of bugs,
> > functional limitations and frustrations which was solved easily enough
> > by transitioning to the use of the system monotonic clock.  That being
> > said, I know there are users of the line event timestamp who actively
> > rely on that value being obtained from the system realtime clock.
> >
>
> Technically we're not allowed to break user-space so had anyone
> actually complained we would be forced to revert this.
>
> > My suggestion (which I would be happy to implement myself) is to allow
> > users to select the clock to be used for line event timestamping on a
> > per line handle basis.  The merit of this approach is that the
> > appropriate clock type may be selected on a per line handle basis
> > according to the needs of the user.  This of course has some
> > implications which are not desirable without merit, but may be deemed
> > acceptable in balance with the resultant functionality.  In summary,
> > these are:
> >
>
> We still haven't released uAPI v2 so I'm open to some last-minute
> changes if they make sense (as you explained in the other email about
> in-kernel timestamping) and are posted soon (before rc3-rc4). I'd like
> to hear Arnd's opinion on this first though.
>
> As for v1 - I think we all agree that it won't be getting any new
> features anymore.
>
> > 1. Increase in processing overhead and latency of timestamp
> > acquisition on line event interrupts.  Implementing the proposed
> > change requires a function call to be made to the appropriate ktime
> > accessor function, based on what the user has configured as the
> > timestamp clock source.  In kernel versions from 5.7 to current, a
> > call is made to the ktime_get_ns() function which is most likely
> > inlined by the compiler.  This change will result in an actual jump
> > having to be made, which will have processor and memory access
> > overhead (potential I$ and D$ misses).  Then there is of course the
> > overhead of resolving which function to call - either a switch
> > statement or call by function pointer (probably the latter option).
> >
> > 2. Additions required to the userspace ABI.  Additional IOCTLs will be
> > required for line handles, allowing the source clock type for line
> > event timestamping to be get or set.
> >
> > 3. Additions required to libgpiod.  The existing API will have to be
> > added to in order to provide an abstraction for this new
> > functionality.  This requires changes to the core C library, as well
> > as the provided C++ and Python bindings (and their test cases).
> > Changes will also be required to the WiP libgpiod service and its
> > d-bus interface.  This change will also affect the proposed future
> > lightweight libgpiod service.
> >
>
> I'm not sure if you're referring to my incomplete d-bus interface I
> worked on some time ago and never finished (I plan to get back to it
> once libgpiod v2.0 is out) or something else? And what is libgpiod
> service and lightweight libgpiod service?
>
> Backward incompatible changes in libgpiod are currently fine - we're
> working on a new major release to provide support for new features of
> uAPI v2 so it's the best moment to propose them.
>
> > 4. Documentation for both the GPIO subsystem and libgpiod will require
> > updating.  This should be done as part of the effort to implement this
> > functionality (if agreed upon) for the target version of the kernel
> > and libgpiod.
> >
> > Such that applications now relying on the use of the 'monotonic'
> > system clock for timestamping line events do not require modification
> > after the implementation of this functionality (most applications), I
> > propose the 'monotonic' system clock be the default source clock.  If
> > the user wants to change this to another clock type, then they may do
> > so via the proposed additional IOCTLs and / or the proposed changes to
> > libgpiod.
> >
> > I would be interested in hearing your thoughts on this suggestion / proposal.
> >
> > ~ Jack
>
> Bartosz

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Bartosz Golaszewski]

On Mon, Oct 12, 2020 at 1:01 PM Jack Winch <sunt.un.morcov@gmail.com> wrote:
>

[snip]

>
> > We still haven't released uAPI v2 so I'm open to some last-minute
> > changes if they make sense (as you explained in the other email about
> > in-kernel timestamping) and are posted soon (before rc3-rc4). I'd like
> > to hear Arnd's opinion on this first though.
>
>
> What's the timescales for this?  As I would be doing this in a
> personal capacity, I will likely have to look at this over a couple of
> weeks.  I would also be limited to testing on an ARMv8 platform, as I
> currently have limited access to hardware.
>

It would be 3-4 weeks from now.

In terms of effort: it doesn't look too complicated. It looks like we
need to add a new flag to the uAPI:
GPIO_V2_LINE_FLAG_EDGE_CLOCK_REALTIME which would make the edge
detector use the real-time clock. I wouldn't stress too much about the
performance of obtaining the timestamp - it's probably negligible
compared to passing the event struct over to user-space.

Kent does the above look right?

Bart

[snip]

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Kent Gibson]

On Mon, Oct 12, 2020 at 03:39:21PM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 12, 2020 at 1:01 PM Jack Winch <sunt.un.morcov@gmail.com> wrote:
> >
> 
> [snip]
> 
> >
> > > We still haven't released uAPI v2 so I'm open to some last-minute
> > > changes if they make sense (as you explained in the other email about
> > > in-kernel timestamping) and are posted soon (before rc3-rc4). I'd like
> > > to hear Arnd's opinion on this first though.
> >
> >
> > What's the timescales for this?  As I would be doing this in a
> > personal capacity, I will likely have to look at this over a couple of
> > weeks.  I would also be limited to testing on an ARMv8 platform, as I
> > currently have limited access to hardware.
> >
> 
> It would be 3-4 weeks from now.
> 
> In terms of effort: it doesn't look too complicated. It looks like we
> need to add a new flag to the uAPI:
> GPIO_V2_LINE_FLAG_EDGE_CLOCK_REALTIME which would make the edge
> detector use the real-time clock. I wouldn't stress too much about the
> performance of obtaining the timestamp - it's probably negligible
> compared to passing the event struct over to user-space.
> 
> Kent does the above look right?
> 

Could we make that GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME, as it
controls the source of the timestamp in struct gpio_v2_line_event?

Otherwise I don't see any problem with it, and I can have a patch out in
a day or two.

Cheers,
Kent.

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Bartosz Golaszewski]

On Mon, Oct 12, 2020 at 4:21 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Oct 12, 2020 at 03:39:21PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Oct 12, 2020 at 1:01 PM Jack Winch <sunt.un.morcov@gmail.com> wrote:
> > >
> >
> > [snip]
> >
> > >
> > > > We still haven't released uAPI v2 so I'm open to some last-minute
> > > > changes if they make sense (as you explained in the other email about
> > > > in-kernel timestamping) and are posted soon (before rc3-rc4). I'd like
> > > > to hear Arnd's opinion on this first though.
> > >
> > >
> > > What's the timescales for this?  As I would be doing this in a
> > > personal capacity, I will likely have to look at this over a couple of
> > > weeks.  I would also be limited to testing on an ARMv8 platform, as I
> > > currently have limited access to hardware.
> > >
> >
> > It would be 3-4 weeks from now.
> >
> > In terms of effort: it doesn't look too complicated. It looks like we
> > need to add a new flag to the uAPI:
> > GPIO_V2_LINE_FLAG_EDGE_CLOCK_REALTIME which would make the edge
> > detector use the real-time clock. I wouldn't stress too much about the
> > performance of obtaining the timestamp - it's probably negligible
> > compared to passing the event struct over to user-space.
> >
> > Kent does the above look right?
> >
>
> Could we make that GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME, as it
> controls the source of the timestamp in struct gpio_v2_line_event?
>

Sure, even better!

> Otherwise I don't see any problem with it, and I can have a patch out in
> a day or two.
>
> Cheers,
> Kent.

Bartosz

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Linus Walleij]

On Sun, Oct 11, 2020 at 5:11 PM Jack Winch <sunt.un.morcov@gmail.com> wrote:

> I recently noticed that in Linux 5.7, gpiolib was changed such that
> line events are now timestamped using the system 'monotonic' clock
> rather than the system realtime clock.  The rationale for this change
> appears to be due to the major use-case of the line event timestamp
> data in relation to the nature of the system realtime clock (which can
> jump backwards or forwards in time due to adjustments by third-parties
> - e.g., NTP or PTP clients, etc).

The background is in the commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib.c?id=f8850206e160bfe35de9ca2e726ab6d6b8cb77dd

Also study the solution in IIO that started the discussion about
all this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=bc2b7dab629a51e8beb5fda4222c62a23b729f26

> I know there are users of the line event timestamp who actively
> rely on that value being obtained from the system realtime clock.

(then in follow-up mail)
> For context, these wall clock time sensitive users are running on
> systems which are PTPv2 clients, with their system realtime clock
> synchronised to that of a local PTP Grand Master clock.  In the past,
> I have used the TTL Pulse Per Second (PPS) output of the Grand Master
> to evaluate methods of timestamping line events with wall clock time
> and it was the kernel timestamping which was most suitable for our
> application.

As Arnd stated in the thread from 2018:
"most of these clocks make no sense at all for a random user
space interface, mainly because I wouldn't trust user space
programmers to make an informed decision which of those
seven to use."

So I suspect you actually managed to make a good argument
for using the realtime clock, we didn't see that coming. :)

But why can't userspace (whether your application or libgpiod)
just call the system clock_gettime() function at the arrival of a
lineevent in that case, and this will be quick due to using vDSO
on most arch:es so no user-to-kernelspace switch will be
required?

clock_gettime() can get you any of the things IIO can get
you.

If you really need the timestamp to be as close as possible
to the actual event then I see why you want the kernel to
make the timestamp in the hard IRQ handler already, but
I just want to confirm that you really really need this.

> My suggestion (which I would be happy to implement myself) is to allow
> users to select the clock to be used for line event timestamping on a
> per line handle basis.

I suppose that makes more sense than the "global switch" in sysfs
for the whole device that IIO uses. At least it is a clear sign that the
user wants this specific type of timestamp.

> 1. Increase in processing overhead and latency of timestamp
> acquisition on line event interrupts.  Implementing the proposed
> change requires a function call to be made to the appropriate ktime
> accessor function, based on what the user has configured as the
> timestamp clock source.  In kernel versions from 5.7 to current, a
> call is made to the ktime_get_ns() function which is most likely
> inlined by the compiler.  This change will result in an actual jump
> having to be made, which will have processor and memory access
> overhead (potential I$ and D$ misses).  Then there is of course the
> overhead of resolving which function to call - either a switch
> statement or call by function pointer (probably the latter option).

Given the overall latency of the kernelspace to userspace
switch and the whole kernel executing around us this is not of
any big concern I'd say, though I will stand corrected in the
face of real-world usecase.

Yours,
Linus Walleij

Re: Suggestion - Configurable Source Clock Type for Line Event Timestamping

[Jack Winch]

> The background is in the commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib.c?id=f8850206e160bfe35de9ca2e726ab6d6b8cb77dd
> Also study the solution in IIO that started the discussion about
> all this:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=bc2b7dab629a51e8beb5fda4222c62a23b729f26


Thanks for the additional background information.  Funnily enough, I
had my sights on this part of the IIO functionality next.

> As Arnd stated in the thread from 2018:
> "most of these clocks make no sense at all for a random user
> space interface, mainly because I wouldn't trust user space
> programmers to make an informed decision which of those
> seven to use."
>
> So I suspect you actually managed to make a good argument
> for using the realtime clock, we didn't see that coming. :)


Well that's reassuring.  Considering doing this work is how I spent my
mid-teens, it looks like that time was well spent. :)

> If you really need the timestamp to be as close as possible
> to the actual event then I see why you want the kernel to
> make the timestamp in the hard IRQ handler already, but
> I just want to confirm that you really really need this.


Confirmed.  I see Kent has submitted a patch series for the chardev
and the v2 uAPI.  So I'll follow the patch threads from now on.  Thank
you all for this addition.

Cheers,
Jack