RE: [RFC PATCH v3 3/4] virtio-rtc: Add alarm feature

[Parav Pandit <parav@nvidia.com>]

> From: Peter Hilber <peter.hilber@opensynergy.com>
> Sent: Wednesday, January 24, 2024 9:11 PM
> 
> On 20.01.24 11:16, Parav Pandit wrote:
> >
> >
> >> From: Peter Hilber <peter.hilber@opensynergy.com>
> >> Sent: Monday, December 18, 2023 12:13 PM
> >>
> >> Add the VIRTIO_RTC_F_ALARM feature (without normative statements).
> >>
> >> The intended use case is: A driver needs to react when an alarm time
> >> has been reached, but the driver may be in a sleep state or powered
> >> off at alarm time. The alarm feature can resume and notify the driver
> >> in this case. Alarms may be retained across device resets (including reset
> on boot).
> >>
> >> Peculiarities
> >> -------------
> >>
> >> Unlike usual alarm clocks, a virtio-rtc alarm-capable clock may step
> >> autonomously at any time: An alarm may change back from "expired" to
> >> "not expired" before the driver has started processing an alarm
> notification.
> >>
> >> To address the above, and the device resets, define "alarm expiration"
> >> in such a way that the driver always has a chance to react to an
> >> alarm, and make the device always responsible for notifying the
> >> driver about an alarm expiration.
> >>
> >> The VIRTIO_RTC_REQ_SET_ALARM_ENABLED request is there so that the
> >> Linux ioctls RTC_AIE_ON and RTC_AIE_OFF only need to emit one request.
> >>
> >> Open Questions
> >> --------------
> >>
> >> - A Linux driver could use the virtio-rtc real-time alarm, through the
> >>   Linux RTC device class, for both CLOCK_REALTIME_ALARM and
> (monotonic)
> >>   CLOCK_BOOTTIME_ALARM alarms. But then, after a clock step, a
> >>   CLOCK_BOOTTIME_ALARM alarm may be late (or early). Perhaps this can
> be
> >>   handled by the driver also setting a virtio-rtc monotonic alarm, so
> >>   may not need to be addressed by the spec.
> >>
> >> - A request returning the minimum and maximum allowed alarm time
> might
> >>   still be added.
> >>
> >> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> >> ---
> >>  device-types/rtc/description.tex | 256
> >> ++++++++++++++++++++++++++++++-
> >>  1 file changed, 254 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/device-types/rtc/description.tex
> >> b/device-types/rtc/description.tex
> >> index 7707472f1663..fcd2f14186e6 100644
> >> --- a/device-types/rtc/description.tex
> >> +++ b/device-types/rtc/description.tex
> >> @@ -4,6 +4,7 @@ \section{RTC Device}\label{sec:Device Types / RTC
> >> Device} time. The device can provide different clocks, e.g.\ for the
> >> UTC or TAI  time standards, or for physical time elapsed since some
> >> past epoch. The  driver can read the clocks with simple or more accurate
> methods.
> >> +Optionally, the driver can set an alarm.
> >>
> >>  \subsection{Device ID}\label{sec:Device Types / RTC Device / Device
> >> ID}
> >>
> >> @@ -13,13 +14,23 @@ \subsection{Virtqueues}\label{sec:Device Types /
> >> RTC Device / Virtqueues}
> >>
> >>  \begin{description}
> >>  \item[0] requestq
> >> +\item[1] alarmq
> >>  \end{description}
> >>
> >>  The driver enqueues requests to the requestq.
> >>
> >> +Through the alarmq, the device notifies the driver about alarm
> >> +expirations. The alarmq exists only if VIRTIO_RTC_F_ALARM was
> >> +negotiated.
> >> +
> > I think is a good example of a need of notification queue from device to
> driver that has multiple use as generic object.
> > On NIC side also we have been thinking to have VQ that holds only the
> completions as currently packed and splitvq unaligned and shorter
> completions are performance reducing areas.
> >
> > A queue which has 1 to 8 physical addresses, and which can hold 8 to 64B
> of notification will be useful.
> > This way per descriptor posting and DMA is not needed.
> > These notifications also work very fast with least amount of descriptor
> caching on the device.
> >
> > For alarms this may not be concern at all in current form but overall, such a
> generic infra has multiple uses.
> > So, I suggest we bring the notification queue.
> 
> IIUC this notification queue still needs to be described.
>
Yes. 
What we need is something like,

A notification queue is a queue that holds device to driver notifications.
The device writes the notification entry, the driver consumes it.
Each notification entry consists of N bytes written by the device. Typically N bytes range from 8 to 64B.
Posting every descriptor of 16B for such small size entry is very inefficient; hence a notification queue is simpler enough that does not have one to one mapping of notification entry and descriptor.
A Notification descriptor points to a page memory. Each page holds one or more notification entries.
Number of notification entries in a descriptor = page_size / notification entry size;
For example, a notification queue consists of 2 4K pages, with 16B notification entry, can receive 512 notifications, with only two descriptors posting, offering 256X reduction in descriptors management at just 0.39% descriptor table size than current packed vq format.

> The alarmq is a dedicated queue on purpose, to make it less intrusive to wake
> the driver up while the driver may be in sleep mode (by making only the
> corresponding interrupt a wake-up interrupt). Sharing the queue with non-
> alarm notifications might create problems in the future.
> 
I wasnât suggesting to share alarm notification with other notifications.

> In my reply for patch 1 I proposed to add this as an alternative interface
> protected by a feature bit. This interface could cover the notification queue
> as well. 
Yes. It can.
> But I am not sure that the alarm feature will be useful for net
> devices.
> 
Unlikely net will use in any near future.

However, a generic notification queue infrastructure can be of good use across device types.
One of the devices should introduce so other can benefit otherwise, spec stays on the inferior path to attempt to utilize current scheme.

For example, Stephen had similar suggest on net patches to offer things to generic devices.

> Thanks for the comment,
> 
> Peter