[virtio-dev] Clarification of VIRTIO_F_EVENT_IDX driver conditions.

[virtio-dev] Clarification of VIRTIO_F_EVENT_IDX driver conditions.

[Patrick Mosca]

We ran into an edge case in the VIRTIO_F_EVENT_IDX feature where a notification could be missed if there is a "streak" in skipped notifications. If the difference between the event index and available index wraps before a notification is needed, there exists a case where a notification must be sent but is not sent.


Signed-off-by: Patrick Mosca <pmosca@apple.com>
Signed-off-by: Cameron Esfahani <dirty@apple.com>
---
 split-ring.tex | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/split-ring.tex b/split-ring.tex
index bfef62d..9650e5e 100644
--- a/split-ring.tex
+++ b/split-ring.tex
@@ -530,6 +530,9 @@ \subsection{Available Buffer Notification Suppression}\label{sec:Basic Facilitie
         \item If the \field{idx} field in the available ring (which determined
           where that descriptor index was placed) was equal to
           \field{avail_event}, the driver MUST send a notification.
+        \item If the number of descriptors added to the queue since
+          the last notification is greater than 65535, the driver MUST
+          send a notification.
         \item Otherwise the driver SHOULD NOT send a notification.
   \end{itemize}
 \end{itemize}
-- 
2.32.0 (Apple Git-131)


-Patrick Mosca
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Clarification of VIRTIO_F_EVENT_IDX driver conditions.

[Michael S. Tsirkin]

On Wed, Jun 01, 2022 at 01:03:27PM -0700, Patrick Mosca wrote:
> We ran into an edge case in the VIRTIO_F_EVENT_IDX feature where a notification could be missed if there is a "streak" in skipped notifications. If the difference between the event index and available index wraps before a notification is needed, there exists a case where a notification must be sent but is not sent.
> 
> 
> Signed-off-by: Patrick Mosca <pmosca@apple.com>
> Signed-off-by: Cameron Esfahani <dirty@apple.com>
> ---
>  split-ring.tex | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/split-ring.tex b/split-ring.tex
> index bfef62d..9650e5e 100644
> --- a/split-ring.tex
> +++ b/split-ring.tex
> @@ -530,6 +530,9 @@ \subsection{Available Buffer Notification Suppression}\label{sec:Basic Facilitie
>          \item If the \field{idx} field in the available ring (which determined
>            where that descriptor index was placed) was equal to
>            \field{avail_event}, the driver MUST send a notification.
> +        \item If the number of descriptors added to the queue since
> +          the last notification is greater than 65535, the driver MUST
> +          send a notification.
>          \item Otherwise the driver SHOULD NOT send a notification.
>    \end{itemize}
>  \end{itemize}
> -- 
> 2.32.0 (Apple Git-131)
> 
> 
> -Patrick Mosca

I am not sure I understand. Can you give an example of how the issue
triggers please?

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Clarification of VIRTIO_F_EVENT_IDX driver conditions.

[Patrick Mosca]

The issue may trigger in the unlikely uint16_t overflow case of computing the difference between two index positions. An example would be a network driver where the host and guest are producing and consuming steadily but at different rates. The producer is producing faster than the consumer such that the consumer never needed to be notified of new events, as it is still playing catch up processing the queue.

If the producer finally catches up to the consumer and the catchup occurs just past the overflow point, there exists a case where a notification must be sent but is not sent according to the current ruleset. 

-Patrick Mosca

> On Jun 5, 2022, at 1:29 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Wed, Jun 01, 2022 at 01:03:27PM -0700, Patrick Mosca wrote:
>> We ran into an edge case in the VIRTIO_F_EVENT_IDX feature where a notification could be missed if there is a "streak" in skipped notifications. If the difference between the event index and available index wraps before a notification is needed, there exists a case where a notification must be sent but is not sent.
>> 
>> 
>> Signed-off-by: Patrick Mosca <pmosca@apple.com>
>> Signed-off-by: Cameron Esfahani <dirty@apple.com>
>> ---
>> split-ring.tex | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/split-ring.tex b/split-ring.tex
>> index bfef62d..9650e5e 100644
>> --- a/split-ring.tex
>> +++ b/split-ring.tex
>> @@ -530,6 +530,9 @@ \subsection{Available Buffer Notification Suppression}\label{sec:Basic Facilitie
>>         \item If the \field{idx} field in the available ring (which determined
>>           where that descriptor index was placed) was equal to
>>           \field{avail_event}, the driver MUST send a notification.
>> +        \item If the number of descriptors added to the queue since
>> +          the last notification is greater than 65535, the driver MUST
>> +          send a notification.
>>         \item Otherwise the driver SHOULD NOT send a notification.
>>   \end{itemize}
>> \end{itemize}
>> -- 
>> 2.32.0 (Apple Git-131)
>> 
>> 
>> -Patrick Mosca
> 
> I am not sure I understand. Can you give an example of how the issue
> triggers please?
> 
> -- 
> MST
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Clarification of VIRTIO_F_EVENT_IDX driver conditions.

[Michael S. Tsirkin]

On Thu, Jun 16, 2022 at 01:53:38PM -0700, Patrick Mosca wrote:
> The issue may trigger in the unlikely uint16_t overflow case of
> computing the difference between two index positions.

Are you sure this is not an implementation bug? I don't see
anywhere in the spec where we suggest computing the difference
between two index positions.

> An
> example would be a network driver where the host and guest are
> producing and consuming steadily but at different rates. The
> producer is producing faster than the consumer such that the
> consumer never needed to be notified of new events, as it is
> still playing catch up processing the queue.  If the producer
> finally catches up to the consumer and the catchup occurs just
> past the overflow point, there exists a case where a
> notification must be sent but is not sent according to the
> current ruleset. 
> 
> -Patrick Mosca

I don't see it sorry. Example?

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Clarification of VIRTIO_F_EVENT_IDX driver conditions.

[Patrick Mosca]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

> Are you sure this is not an implementation bug? 

Possibly yes, but the specification could probably be made clearer to avoid such an implementation bug.

My concern is that the helper method in question was originally part of the 0.9.5 specification, even though it is no longer directly provided in the 1.0+ versions of the specification.

static inline int vring_need_event(uint16_t event_idx, uint16_t new_idx, uint16_t old_idx)
{
	return (uint16_t)(new_idx - event_idx - 1) < (uint16_t)(new_idx - old_idx);
}

Neither the 0.9.5 nor the current specification mention any requirement of notifying during a wrap, even though the Linux implementation correctly checks that unmentioned case, which was not provided in that 0.9.5 helper method or explicitly mentioned in the current specification.

https://github.com/torvalds/linux/blob/7a68065eb9cd194cf03f135c9211eeb2d5c4c0a0/drivers/vhost/vringh.c#L522

-Patrick Mosca

> On Jun 17, 2022, at 3:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Thu, Jun 16, 2022 at 01:53:38PM -0700, Patrick Mosca wrote:
>> The issue may trigger in the unlikely uint16_t overflow case of
>> computing the difference between two index positions.
> 
> Are you sure this is not an implementation bug? I don't see
> anywhere in the spec where we suggest computing the difference
> between two index positions.
> 
>> An
>> example would be a network driver where the host and guest are
>> producing and consuming steadily but at different rates. The
>> producer is producing faster than the consumer such that the
>> consumer never needed to be notified of new events, as it is
>> still playing catch up processing the queue.  If the producer
>> finally catches up to the consumer and the catchup occurs just
>> past the overflow point, there exists a case where a
>> notification must be sent but is not sent according to the
>> current ruleset. 
>> 
>> -Patrick Mosca
> 
> I don't see it sorry. Example?
> 
> -- 
> MST
> 


[-- Attachment #2: Type: text/html, Size: 2907 bytes --]

Re: [virtio-dev] Clarification of VIRTIO_F_EVENT_IDX driver conditions.

[Patrick Mosca]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

Checking in here. Is the problem a little more clear now?

-Patrick Mosca

> On Jun 24, 2022, at 3:39 PM, Patrick Mosca <pmosca@apple.com> wrote:
> 
>> Are you sure this is not an implementation bug? 
> 
> Possibly yes, but the specification could probably be made clearer to avoid such an implementation bug.
> 
> My concern is that the helper method in question was originally part of the 0.9.5 specification, even though it is no longer directly provided in the 1.0+ versions of the specification.
> 
> static inline int vring_need_event(uint16_t event_idx, uint16_t new_idx, uint16_t old_idx)
> {
> 	return (uint16_t)(new_idx - event_idx - 1) < (uint16_t)(new_idx - old_idx);
> }
> 
> Neither the 0.9.5 nor the current specification mention any requirement of notifying during a wrap, even though the Linux implementation correctly checks that unmentioned case, which was not provided in that 0.9.5 helper method or explicitly mentioned in the current specification.
> 
> https://github.com/torvalds/linux/blob/7a68065eb9cd194cf03f135c9211eeb2d5c4c0a0/drivers/vhost/vringh.c#L522
> 
> -Patrick Mosca
> 
>> On Jun 17, 2022, at 3:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>> On Thu, Jun 16, 2022 at 01:53:38PM -0700, Patrick Mosca wrote:
>>> The issue may trigger in the unlikely uint16_t overflow case of
>>> computing the difference between two index positions.
>> 
>> Are you sure this is not an implementation bug? I don't see
>> anywhere in the spec where we suggest computing the difference
>> between two index positions.
>> 
>>> An
>>> example would be a network driver where the host and guest are
>>> producing and consuming steadily but at different rates. The
>>> producer is producing faster than the consumer such that the
>>> consumer never needed to be notified of new events, as it is
>>> still playing catch up processing the queue.  If the producer
>>> finally catches up to the consumer and the catchup occurs just
>>> past the overflow point, there exists a case where a
>>> notification must be sent but is not sent according to the
>>> current ruleset. 
>>> 
>>> -Patrick Mosca
>> 
>> I don't see it sorry. Example?
>> 
>> -- 
>> MST
>> 
> 


[-- Attachment #2: Type: text/html, Size: 3371 bytes --]