Re: [PATCH v12 5/8] virtio-balloon: VIRTIO_BALLOON_F_SG

[Wei Wang]

On 07/14/2017 04:19 AM, Michael S. Tsirkin wrote:
> On Thu, Jul 13, 2017 at 03:42:35PM +0800, Wei Wang wrote:
> > On 07/12/2017 09:56 PM, Michael S. Tsirkin wrote:
> > > So the way I see it, there are several issues:
> > >
> > > - internal wait - forces multiple APIs like kick/kick_sync
> > >     note how kick_sync can fail but your code never checks return code
> > > - need to re-write the last descriptor - might not work
> > >     for alternative layouts which always expose descriptors
> > >     immediately
> > Probably it wasn't clear. Please let me explain the two functions here:
> >
> > 1) virtqueue_add_chain_desc(vq, head_id, prev_id,..):
> > grabs a desc from the vq and inserts it to the chain tail (which is indexed
> > by
> > prev_id, probably better to call it tail_id). Then, the new added desc
> > becomes
> > the tail (i.e. the last desc). The _F_NEXT flag is cleared for each desc
> > when it's
> > added to the chain, and set when another desc comes to follow later.
> And this only works if there are multiple rings like
> avail + descriptor ring.
> It won't work e.g. with the proposed new layout where
> writing out a descriptor exposes it immediately.

I think it can support the 1.1 proposal, too. But before getting
into that, I think we first need to deep dive into the implementation
and usage of _first/next/last. The usage would need to lock the vq
from the first to the end (otherwise, the returned info about the number
of available desc in the vq, i.e. num_free, would be invalid):

lock(vq);
add_first();
add_next();
add_last();
unlock(vq);

However, I think the case isn't this simple, since we need to check more 
things
after each add_xx() step. For example, if only one entry is available at 
the time
we start to use the vq, that is, num_free is 0 after add_first(), we 
wouldn't be
able to add_next and add_last. So, it would work like this:

start:
    ...get free page block..
    lock(vq)
retry:
    ret = add_first(..,&num_free,);
    if(ret == -ENOSPC) {
        goto retry;
    } else if (!num_free) {
        add_chain_head();
        unlock(vq);
        kick & wait;
        goto start;
    }
next_one:
    ...get free page block..
    add_next(..,&num_free,);
    if (!num_free) {
        add_chain_head();
        unlock(vq);
        kick & wait;
        goto start;
    } if (num_free == 1) {
        ...get free page block..
        add_last(..);
        unlock(vq);
        kick & wait;
        goto start;
    } else {
        goto next_one;
    }

The above seems unnecessary to me to have three different APIs.
That's the reason to combine them into one virtqueue_add_chain_desc().

-- or, do you have a different thought about using the three APIs?


Implementation Reference:

struct desc_iterator {
    unsigned int head;
    unsigned int tail;
};

add_first(*vq, *desc_iterator, *num_free, ..)
{
    if (vq->vq.num_free < 1)
        return -ENOSPC;
    get_desc(&desc_id);
    desc[desc_id].flag &= ~_F_NEXT;
    desc_iterator->head = desc_id
    desc_iterator->tail = desc_iterator->head;
    *num_free = vq->vq.num_free;
}

add_next(vq, desc_iterator, *num_free,..)
{
    get_desc(&desc_id);
    desc[desc_id].flag &= ~_F_NEXT;
    desc[desc_iterator.tail].next = desc_id;
    desc[desc_iterator->tail].flag |= _F_NEXT;
    desc_iterator->tail = desc_id;
    *num_free = vq->vq.num_free;
}

add_last(vq, desc_iterator,..)
{
    get_desc(&desc_id);
    desc[desc_id].flag &= ~_F_NEXT;
    desc[desc_iterator.tail].next = desc_id;
    desc_iterator->tail = desc_id;

    add_chain_head(); // put the desc_iterator.head to the ring
}


Best,
Wei