[SLOF] [PATCH] virtio: Remove global variables in block and 9p driver

Thomas Huth thuth at redhat.com
Thu Feb 2 18:29:39 AEDT 2017


On 02.02.2017 01:00, Alexey Kardashevskiy wrote:
> On 01/02/17 19:08, Thomas Huth wrote:
>> On 01.02.2017 03:14, Alexey Kardashevskiy wrote:
>>> On 18/01/17 00:16, Thomas Huth wrote:
>>>> No need for a global variable to store the virtqueue information here,
>>>> thus let's remove the global vq variable and make it local to the
>>>> init function instead.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth at redhat.com>
>>>> ---
>>>>  lib/libvirtio/virtio-9p.c  | 9 +++------
>>>>  lib/libvirtio/virtio-blk.c | 9 +++------
>>>>  2 files changed, 6 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/lib/libvirtio/virtio-9p.c b/lib/libvirtio/virtio-9p.c
>>>> index 6e9c379..fb329b3 100644
>>>> --- a/lib/libvirtio/virtio-9p.c
>>>> +++ b/lib/libvirtio/virtio-9p.c
>>>> @@ -20,8 +20,6 @@
>>>>  #include "p9.h"
>>>>  #include "virtio-internal.h"
>>>>  
>>>> -static struct vqs vq;
>>>> -
>>>>  /**
>>>>   * Notes for 9P Server config:
>>>>   *
>>>> @@ -163,7 +161,7 @@ static int virtio_9p_transact(void *opaque, uint8_t *tx, int tx_size, uint8_t *r
>>>>  int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf,
>>>>  		   int buf_size)
>>>>  {
>>>> -	struct vring_avail *vq_avail;
>>>> +	struct vqs vq;
>>>>  	int status = VIRTIO_STAT_ACKNOWLEDGE;
>>>>  
>>>>  	/* Check for double open */
>>>> @@ -195,9 +193,8 @@ int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf,
>>>>  	if (virtio_queue_init_vq(dev, &vq, 0))
>>>>  		goto dev_error;
>>>>  
>>>> -	vq_avail = virtio_get_vring_avail(dev, 0);
>>>> -	vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT);
>>>> -	vq_avail->idx = 0;
>>>> +	vq.avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT);
>>>> +	vq.avail->idx = 0;
>>>
>>>
>>> Here is some black magic happening which I cannot understand. @vq is a
>>> local struct, it is not used anywhere except virtio_queue_init_vq() but
>>> even there the only used bit is vq->desc which is allocated in
>>> virtio_queue_init_vq() and then written to @dev via virtio_set_qaddr().
>>>
>>> I'd think that we do not need @vq parameter in virtio_queue_init_vq() and
>>> we can drop these local @vq from "virtio_.*_init", what do I miss?
>>
>> You missed that the "virtqueue available ring" is used again in
>> virtio_9p_transact(). The code gets a pointer to the ring there with
>> virtio_get_vring_avail(). So no, this "black magic" can not be dropped,
>> it is a required setup step.
> 
> I was not talking about the ring itself, I was talking about the vq thingy
> which is a descriptor of the ring.
[...]
> The code gets individual members of vq but not a pointer to vq itself (the
> thing which the patch makes local instead of removing it).
[...]
> I am still missing the point, right? :)

Right. It's a device, and the VRING_AVAIL_F_NO_INTERRUPT flag is read
from the *device* side (thus in this case QEMU). You can not simply
remove this, as far as I can tell.

 Thomas



More information about the SLOF mailing list