Re: [virtio-dev] Re: [PATCH v8 08/16] packed virtqueues: more efficient virtqueue layout

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 02/28/2018 04:40 PM, Michael S. Tsirkin wrote:
> On Wed, Feb 28, 2018 at 02:59:51PM +0100, Halil Pasic wrote:
>>
>>
>> On 02/28/2018 02:42 PM, Jens Freimann wrote:
>>> On Tue, Feb 27, 2018 at 12:29:11PM +0100, Halil Pasic wrote:
>>>>
>>>>
>>>> On 02/27/2018 11:23 AM, Jens Freimann wrote:
>>>>> On Mon, Feb 26, 2018 at 11:05:14PM +0200, Michael S. Tsirkin wrote:
>>>>>> On Mon, Feb 26, 2018 at 06:19:21PM +0100, Halil Pasic wrote:
>>>>>>>> +?????????????????????????????? vq->driver_event.flags = 0x1;
>>>>>>>> +?????????????????????????????? memory_barrier();
>>>>>>>> +
>>>>>>>> +?????????????????????????????? flags = d->flags;
>>>>>>>> +?????????????????????????????? bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL);
>>>>>>>> +?????????????????????????????? bool used = flags & (1 << VIRTQ_DESC_F_USED);
>>>>>>>> +?????????????????????????????? if (avail != used) {
>>>>>>>> +?????????????????????????????????????????????? break;
>>>>>>>> +?????????????????????????????? }
>>>>>>>> +
>>>>>>>> +?????????????????????????????? vq->driver_event.flags = 0x2;
>>>>>>>> +?????????????? }
>>>>>>>> +
>>>>>>>> +?????? read_memory_barrier();
>>>>>>>
>>>>>>> Now with the condition avail != used a freshly (that is zero initialized)
>>>>>>> ring would appear all used. And we would do process_buffer(d) for the
>>>>>>> whole ring if this code happens to get executed. Do we have to make
>>>>>>> sure that this does not happen?
>>>>>>
>>>>>> I'll have to think about this.
>>>>>
>>>>> With the wrap counter initialized to 1 descriptors would not be seen
>>>>> as used.
>>>>
>>>> Looking at the code... In vhost:
>>>>
>>>> +static bool desc_is_avail(struct vhost_virtqueue *vq,
>>>> +              struct vring_desc_packed *desc)
>>>> +{
>>>> +    if (vq->used_wrap_counter)
>>>> +        if ((desc->flags & DESC_AVAIL) && !(desc->flags & DESC_USED))
>>>> +            return true;
>>>> +    if (vq->used_wrap_counter == false)
>>>> +        if (!(desc->flags & DESC_AVAIL) && (desc->flags & DESC_USED))
>>>> +            return true;
>>>> +
>>>> +    return false;
>>>> +}
>>>>
>>>> Here the bit pattern corresponding to available depends on the
>>>> value of the wrap counter. I kind of anticipated this, but I could not
>>>> find it defined in this spec.
>>>>
>>>> OTOH in guest:
>>>>
>>>> +static inline bool more_used_packed(const struct vring_virtqueue *vq)
>>>> +{
>>>> +    u16 last_used, flags;
>>>> +    bool avail, used;
>>>> +
>>>> +    if (vq->vq.num_free == vq->vring.num)
>>>> +        return false;
>>>> +
>>>> +    last_used = vq->last_used_idx;
>>>> +    flags = virtio16_to_cpu(vq->vq.vdev,
>>>> +                vq->vring_packed.desc[last_used].flags);
>>>> +    avail = flags & VRING_DESC_F_AVAIL(1);
>>>> +    used = flags & VRING_DESC_F_USED(1);
>>>> +
>>>> +    return avail == used;
>>>> +}
>>>>
>>>> if the next to be used descriptor is actually used does not depend on
>>>> any wrap counter, but has vq->vq.num_free == vq->vring.num as an extra
>>>> condition. This extra condition is basically 'there are outstanding
>>>> descriptors' and those are obviously either 'available' or yet to be observed
>>>> 'used' descriptors. Right after initialization is covered by this extra
>>>> condition. And obviously if the descriptor in question is still available
>>>> flags & VRING_DESC_F_AVAIL(1) != flags & VRING_DESC_F_USED(1) holds, so
>>>> with the extra condition we are right there where we want to be.
>>>>
>>>> But I could not find the extra condition in the spec.
>>>>
>>>> With that said, I also want to point out that I don't understand
>>>> your statement Jens. I don't see a way to express the condition corresponding
>>>> to more_used_packed() using the driver wrap counter (avail_wrap_count).
>>>> Of course a wrap counter that wraps when last_used wraps could be used
>>>> to (but no such counter is mentioned here AFAIU).
>>>
>>> Not sure I get this.
>>> I was merely saying that when descriptor flags are initialized to 0
>>> and the wrap counters to 1, then it is not the case that the driver
>>> would see all descriptors as used because it takes the wrap counter
>>> into account.
>>>
>>
>> Please point me to the paragraph where it's written  how is the wrap
>> counter to be taken into account when trying to determine if the
>> desc_ring[last_used] (the descriptor we are polling) is used or not.
>>
>> Nothing like that being specified (or at least I could not find it)
>> was actually what I got hooked on.
>>
>> Regards,
>> Halil
>>
> 
> Maintaining an internal "last used wrap counter" is one way to
> detect ring entry change. Another would be to maintain
> a per-entry "last used flag".
> 
> I should probably do this in pseudo-code, and maybe add an
> implementation note.
> 

Thanks! I will revisit this once you have a proposed solution.

Regards,
Halil