Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending

[Rusty Russell <rusty@rustcorp.com.au>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote:
>> Jason Wang <jasowang@redhat.com> writes:
>> > We currently does:
>> >
>> > bufs = (avail->idx - last_used_idx) * 3 / 4;
>> >
>> > This is ok now since we only try to enable the delayed callbacks when
>> > the queue is about to be full. This may not work well when there is
>> > only one pending buffer in the virtqueue (this may be the case after
>> > tx interrupt was enabled). Since virtqueue_enable_cb() will return
>> > false which may cause unnecessary triggering of napis. This patch
>> > correct this by only calculate the four thirds when bufs is not one.
>> 
>> I mildly prefer to avoid the branch, by changing the calculation like
>> so:
>> 
>>         /* Set bufs >= 1, even if there's only one pending buffer */
>>         bufs = (bufs + 1) * 3 / 4;
>
> Or bus * 3/4 + 1
>
>> But it's not clear to me how much this happens.  I'm happy with the
>> patch though, as currently virtqueue_enable_cb_delayed() is the same
>> as virtqueue_enable_cb() if there's only been one buffer added.
>> 
>> Cheers,
>> Rusty.
>
> But isn't this by design?
> The documentation says:
>
>  * This re-enables callbacks but hints to the other side to delay
>  * interrupts until most of the available buffers have been processed;
>
> Clearly, this implies that when there's one buffer it must behave
> exactly the same.

Yes, my mistake.  We could hit the "never gets notified" case with this
change, and that's a much bigger problem.

So I don't think we can accept this patch...
Rusty.