| Message ID | 20211207181045.1246688-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [RFC] counter: Expand API with a function for an exact timestamp | expand |
On 07.12.2021 19:10:45, Uwe Kleine-König wrote: > Some hardware units capture a timestamp for the counted event. To > increase precision add a variant of counter_push_event() that allows > passing this timestamp. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > the difficulty is that the captured timer value is in a unit different > from the one provided by ktime_get_ns(). So maybe some helper functions > will be needed to convert the local timer value to a ktime timestamp? > > So usage would be something like: > > ktime_now = ktime_get_ns(); > local_now = readl(CNT); > local_event = readl(...); > > ktime_event = ktime_now - (local_now - local_event) * somefactor >> someshift; You can use cyclecounter/timecounter for this. Marc
On Tue, Dec 07, 2021 at 07:10:45PM +0100, Uwe Kleine-König wrote: > Some hardware units capture a timestamp for the counted event. To > increase precision add a variant of counter_push_event() that allows > passing this timestamp. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > the difficulty is that the captured timer value is in a unit different > from the one provided by ktime_get_ns(). So maybe some helper functions > will be needed to convert the local timer value to a ktime timestamp? > > So usage would be something like: > > ktime_now = ktime_get_ns(); > local_now = readl(CNT); > local_event = readl(...); > > ktime_event = ktime_now - (local_now - local_event) * somefactor >> someshift; > > counter_push_event_ts(count, event, channel, ktime_event); > > This improves the precision because irq latency doesn't influence > the resulting timestamp. The precision then only depends on the timer > resolution and the delay between ktime_get_ns() and readl(CNT); > > I don't have a driver (yet) that makes use of this, the hardware where > this will matter will be stm32mp1. > > Best regards > Uwe Hello Uwe, Precision logging of counter events was one of the main motivations for the creation of the Counter character device interface, so if we can reduce the jitter of the timestamp by utilizing hardware-provided values, I'm all for it. That being said, we should take care in deciding how to expose this data in the Counter interfaces because not all devices support such functionality and yet users should expect a standard data format to code against. Considering this, I think it better to keep the struct counter_event timestamp the way it is right now as ktime_get_ns() so that users have a consistent way to retrieve the timestamp of when the Counter event was pushed to the queue. In order to support the hardware-provided timestamp, how about exposing local_event and local_now as Counter extensions? You can set a watches for the local timestamps to log them into the queue with each event, then perform the ktime_event calculation in userspace using the struct counter_event timestamp value. William Breathitt Gray > > drivers/counter/counter-chrdev.c | 25 +++++++++++++++++++++---- > include/linux/counter.h | 2 ++ > 2 files changed, 23 insertions(+), 4 deletions(-) > > diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c > index b7c62f957a6a..6381f7246d59 100644 > --- a/drivers/counter/counter-chrdev.c > +++ b/drivers/counter/counter-chrdev.c > @@ -521,16 +521,17 @@ static int counter_get_data(struct counter_device *const counter, > } > > /** > - * counter_push_event - queue event for userspace reading > + * counter_push_event_ts - queue event with a timestamp for userspace reading > * @counter: pointer to Counter structure > * @event: triggered event > * @channel: event channel > + * @timestamp: the ktime when the event occurred > * > * Note: If no one is watching for the respective event, it is silently > * discarded. > */ > -void counter_push_event(struct counter_device *const counter, const u8 event, > - const u8 channel) > +void counter_push_event_ts(struct counter_device *const counter, const u8 event, > + const u8 channel, u64 timestamp) > { > struct counter_event ev; > unsigned int copied = 0; > @@ -538,7 +539,7 @@ void counter_push_event(struct counter_device *const counter, const u8 event, > struct counter_event_node *event_node; > struct counter_comp_node *comp_node; > > - ev.timestamp = ktime_get_ns(); > + ev.timestamp = timestamp; > ev.watch.event = event; > ev.watch.channel = channel; > > @@ -570,4 +571,20 @@ void counter_push_event(struct counter_device *const counter, const u8 event, > if (copied) > wake_up_poll(&counter->events_wait, EPOLLIN); > } > +EXPORT_SYMBOL_GPL(counter_push_event_ts); > + > +/** > + * counter_push_event - queue event for userspace reading > + * @counter: pointer to Counter structure > + * @event: triggered event > + * @channel: event channel > + * > + * Note: If no one is watching for the respective event, it is silently > + * discarded. > + */ > +void counter_push_event(struct counter_device *const counter, const u8 event, > + const u8 channel) > +{ > + counter_push_event_ts(counter, event, channel, ktime_get_ns()); > +} > EXPORT_SYMBOL_GPL(counter_push_event); > diff --git a/include/linux/counter.h b/include/linux/counter.h > index b7d0a00a61cf..596e7e58e463 100644 > --- a/include/linux/counter.h > +++ b/include/linux/counter.h > @@ -333,6 +333,8 @@ int counter_register(struct counter_device *const counter); > void counter_unregister(struct counter_device *const counter); > int devm_counter_register(struct device *dev, > struct counter_device *const counter); > +void counter_push_event_ts(struct counter_device *const counter, const u8 event, > + const u8 channel, u64 timestamp) > void counter_push_event(struct counter_device *const counter, const u8 event, > const u8 channel); > > -- > 2.30.2 >
Hello William, On Mon, Dec 13, 2021 at 04:40:55PM +0900, William Breathitt Gray wrote: > On Tue, Dec 07, 2021 at 07:10:45PM +0100, Uwe Kleine-König wrote: > > Some hardware units capture a timestamp for the counted event. To > > increase precision add a variant of counter_push_event() that allows > > passing this timestamp. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > Hello, > > > > the difficulty is that the captured timer value is in a unit different > > from the one provided by ktime_get_ns(). So maybe some helper functions > > will be needed to convert the local timer value to a ktime timestamp? > > > > So usage would be something like: > > > > ktime_now = ktime_get_ns(); > > local_now = readl(CNT); > > local_event = readl(...); > > > > ktime_event = ktime_now - (local_now - local_event) * somefactor >> someshift; > > > > counter_push_event_ts(count, event, channel, ktime_event); > > > > This improves the precision because irq latency doesn't influence > > the resulting timestamp. The precision then only depends on the timer > > resolution and the delay between ktime_get_ns() and readl(CNT); > > > > I don't have a driver (yet) that makes use of this, the hardware where > > this will matter will be stm32mp1. > > Precision logging of counter events was one of the main motivations for > the creation of the Counter character device interface, so if we can > reduce the jitter of the timestamp by utilizing hardware-provided > values, I'm all for it. That being said, we should take care in deciding > how to expose this data in the Counter interfaces because not all > devices support such functionality and yet users should expect a > standard data format to code against. > > Considering this, I think it better to keep the struct counter_event > timestamp the way it is right now as ktime_get_ns() so that users have a > consistent way to retrieve the timestamp of when the Counter event was > pushed to the queue. Isn't it totally irrelevant to userspace when the event was pushed to the queue? I claim userspace is only ever interested when the event happend and the queue is only an implementation detail of the counter framework. The documentation for the timestamp member of struct counter_event is best estimate of time of event occurrence, in nanoseconds . That looks sane, and following that, the best estimate is the hw timestamp?! (Well, if you experience high latencies, the timestamp of the queuing might be interesting for debugging, but in this case I'd prefer tracing support over exposing implementation details in the API.) > In order to support the hardware-provided > timestamp, how about exposing local_event and local_now as Counter > extensions? You can set a watches for the local timestamps to log them > into the queue with each event, then perform the ktime_event calculation > in userspace using the struct counter_event timestamp value. An upside of your suggestion is that calculating the ktime for the event isn't done unless userspace needs it. Still I'd prefer to use .timestamp for an as-accurate-as-possible information for the event. Best regards Uwe
On Mon, Dec 13, 2021 at 10:04:48AM +0100, Uwe Kleine-König wrote: > Hello William, > > On Mon, Dec 13, 2021 at 04:40:55PM +0900, William Breathitt Gray wrote: > > On Tue, Dec 07, 2021 at 07:10:45PM +0100, Uwe Kleine-König wrote: > > > Some hardware units capture a timestamp for the counted event. To > > > increase precision add a variant of counter_push_event() that allows > > > passing this timestamp. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > Hello, > > > > > > the difficulty is that the captured timer value is in a unit different > > > from the one provided by ktime_get_ns(). So maybe some helper functions > > > will be needed to convert the local timer value to a ktime timestamp? > > > > > > So usage would be something like: > > > > > > ktime_now = ktime_get_ns(); > > > local_now = readl(CNT); > > > local_event = readl(...); > > > > > > ktime_event = ktime_now - (local_now - local_event) * somefactor >> someshift; > > > > > > counter_push_event_ts(count, event, channel, ktime_event); > > > > > > This improves the precision because irq latency doesn't influence > > > the resulting timestamp. The precision then only depends on the timer > > > resolution and the delay between ktime_get_ns() and readl(CNT); > > > > > > I don't have a driver (yet) that makes use of this, the hardware where > > > this will matter will be stm32mp1. > > > > Precision logging of counter events was one of the main motivations for > > the creation of the Counter character device interface, so if we can > > reduce the jitter of the timestamp by utilizing hardware-provided > > values, I'm all for it. That being said, we should take care in deciding > > how to expose this data in the Counter interfaces because not all > > devices support such functionality and yet users should expect a > > standard data format to code against. > > > > Considering this, I think it better to keep the struct counter_event > > timestamp the way it is right now as ktime_get_ns() so that users have a > > consistent way to retrieve the timestamp of when the Counter event was > > pushed to the queue. > > Isn't it totally irrelevant to userspace when the event was pushed to > the queue? I claim userspace is only ever interested when the event > happend and the queue is only an implementation detail of the counter > framework. The documentation for the timestamp member of struct > counter_event is > > best estimate of time of event occurrence, in nanoseconds > > . That looks sane, and following that, the best estimate is the hw > timestamp?! > > (Well, if you experience high latencies, the timestamp of the queuing > might be interesting for debugging, but in this case I'd prefer tracing > support over exposing implementation details in the API.) All right your argument makes sense, the primary purpose of this timestamp is to mark the time the event actually occurred so queue latency debugging would certainly be secondary to that end. In that case, providing a more accurate timestamp via a counter_push_event_ts() call would be preferable if the hardware is capable of doing so. > > In order to support the hardware-provided > > timestamp, how about exposing local_event and local_now as Counter > > extensions? You can set a watches for the local timestamps to log them > > into the queue with each event, then perform the ktime_event calculation > > in userspace using the struct counter_event timestamp value. > > An upside of your suggestion is that calculating the ktime for the event > isn't done unless userspace needs it. Still I'd prefer to use .timestamp > for an as-accurate-as-possible information for the event. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | We can additionally expose the local_event and local_now values as Counter extensions if userspace wants to look into that data more directly. If the objective is to reduce latency by moving the timestamp calculation to userspace, we might need to introduce some way to adjust the "timestamp mode" or similar to disable the automatic calculation when the user wants to do calculate it themselves in userspace instead. However, I suspect the effort is not worth it if you're just saving one or two read operations on the stm32mp1; you might as well just calculate the ktime automatically for each event and not worry about exposing the local hardware data to userspace for now until the need arises. William Breathitt Gray
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c index b7c62f957a6a..6381f7246d59 100644 --- a/drivers/counter/counter-chrdev.c +++ b/drivers/counter/counter-chrdev.c @@ -521,16 +521,17 @@ static int counter_get_data(struct counter_device *const counter, } /** - * counter_push_event - queue event for userspace reading + * counter_push_event_ts - queue event with a timestamp for userspace reading * @counter: pointer to Counter structure * @event: triggered event * @channel: event channel + * @timestamp: the ktime when the event occurred * * Note: If no one is watching for the respective event, it is silently * discarded. */ -void counter_push_event(struct counter_device *const counter, const u8 event, - const u8 channel) +void counter_push_event_ts(struct counter_device *const counter, const u8 event, + const u8 channel, u64 timestamp) { struct counter_event ev; unsigned int copied = 0; @@ -538,7 +539,7 @@ void counter_push_event(struct counter_device *const counter, const u8 event, struct counter_event_node *event_node; struct counter_comp_node *comp_node; - ev.timestamp = ktime_get_ns(); + ev.timestamp = timestamp; ev.watch.event = event; ev.watch.channel = channel; @@ -570,4 +571,20 @@ void counter_push_event(struct counter_device *const counter, const u8 event, if (copied) wake_up_poll(&counter->events_wait, EPOLLIN); } +EXPORT_SYMBOL_GPL(counter_push_event_ts); + +/** + * counter_push_event - queue event for userspace reading + * @counter: pointer to Counter structure + * @event: triggered event + * @channel: event channel + * + * Note: If no one is watching for the respective event, it is silently + * discarded. + */ +void counter_push_event(struct counter_device *const counter, const u8 event, + const u8 channel) +{ + counter_push_event_ts(counter, event, channel, ktime_get_ns()); +} EXPORT_SYMBOL_GPL(counter_push_event); diff --git a/include/linux/counter.h b/include/linux/counter.h index b7d0a00a61cf..596e7e58e463 100644 --- a/include/linux/counter.h +++ b/include/linux/counter.h @@ -333,6 +333,8 @@ int counter_register(struct counter_device *const counter); void counter_unregister(struct counter_device *const counter); int devm_counter_register(struct device *dev, struct counter_device *const counter); +void counter_push_event_ts(struct counter_device *const counter, const u8 event, + const u8 channel, u64 timestamp) void counter_push_event(struct counter_device *const counter, const u8 event, const u8 channel);
Some hardware units capture a timestamp for the counted event. To increase precision add a variant of counter_push_event() that allows passing this timestamp. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, the difficulty is that the captured timer value is in a unit different from the one provided by ktime_get_ns(). So maybe some helper functions will be needed to convert the local timer value to a ktime timestamp? So usage would be something like: ktime_now = ktime_get_ns(); local_now = readl(CNT); local_event = readl(...); ktime_event = ktime_now - (local_now - local_event) * somefactor >> someshift; counter_push_event_ts(count, event, channel, ktime_event); This improves the precision because irq latency doesn't influence the resulting timestamp. The precision then only depends on the timer resolution and the delay between ktime_get_ns() and readl(CNT); I don't have a driver (yet) that makes use of this, the hardware where this will matter will be stm32mp1. Best regards Uwe drivers/counter/counter-chrdev.c | 25 +++++++++++++++++++++---- include/linux/counter.h | 2 ++ 2 files changed, 23 insertions(+), 4 deletions(-)