Virtualization - Feb 2012 - [PATCH 2/2] virtio-serial: setup_port_vq when adding port

Hi,

Sorry for the late reply.

On (Thu) 12 Jan 2012 [09:20:07], zanghongyong at huawei.com
wrote:
> From: Hongyong Zang <zanghongyong at huawei.com>
> 
> Add setup_port_vq(). Create the io ports' vqs when add_port.
Can you describe the changes in more detail, please?
> Signed-off-by: Hongyong Zang <zanghongyong at huawei.com>
> ---
>  drivers/char/virtio_console.c |   65
++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 8e3c46d..2e5187e 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1132,6 +1132,55 @@ static void send_sigio_to_port(struct port *port)
>  		kill_fasync(&port->async_queue, SIGIO, POLL_OUT);
>  }
>  
> +static void in_intr(struct virtqueue *vq);
> +static void out_intr(struct virtqueue *vq);
> +
> +static int setup_port_vq(struct ports_device *portdev,  u32 id)
> +{
> +	int err, vq_num;
> +	vq_callback_t **io_callbacks;
> +	char **io_names;
> +	struct virtqueue **vqs;
> +	u32 i,j,nr_ports,nr_queues;
> +
> +	err = 0;
> +	vq_num = (id + 1) * 2;
> +	nr_ports = portdev->config.max_nr_ports;
> +	nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
> +
> +	vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
> +	io_callbacks = kmalloc(nr_queues * sizeof(vq_callback_t *), GFP_KERNEL);
> +	io_names = kmalloc(nr_queues * sizeof(char *), GFP_KERNEL);
> +	if (!vqs || !io_callbacks || !io_names) {
> +		err = -ENOMEM;
> +		goto free;
> +	}
> +
> +	for (i = 0, j = 0; i <= nr_ports; i++) {
> +		io_callbacks[j] = in_intr;
> +		io_callbacks[j + 1] = out_intr;
> +		io_names[j] = NULL;
> +		io_names[j + 1] = NULL;
> +		j += 2;
> +	}
> +	io_names[vq_num] = "serial-input";
> +	io_names[vq_num + 1] = "serial-output";
> +	err = portdev->vdev->config->find_vqs(portdev->vdev,
nr_queues, vqs,
> +				io_callbacks,
> +				(const char **)io_names);
> +	if (err)
> +		goto free;
> +	portdev->in_vqs[id] = vqs[vq_num];
> +	portdev->out_vqs[id] = vqs[vq_num + 1];
I don't think this approach will work fine for port hot-plug /
hot-unplug cases at all.  For example, I first start qemu with one
port, at id 1.  Then I add a port at id 5, then at 2, then at 10.
Will that be fine?
> +
> +free:
> +	kfree(io_names);
> +	kfree(io_callbacks);
> +	kfree(vqs);
> +
> +	return err;
> +}
> +
>  static int add_port(struct ports_device *portdev, u32 id)
>  {
>  	char debugfs_name[16];
> @@ -1163,6 +1212,14 @@ static int add_port(struct ports_device *portdev,
u32 id)
>  
>  	port->outvq_full = false;
>  
> +	if (!portdev->in_vqs[port->id] &&
!portdev->out_vqs[port->id]) {
> +		spin_lock(&portdev->ports_lock);
> +		err = setup_port_vq(portdev, port->id);
> +		spin_unlock(&portdev->ports_lock);
> +		if (err)
> +			goto free_port;
> +	}
> +
>  	port->in_vq = portdev->in_vqs[port->id];
>  	port->out_vq = portdev->out_vqs[port->id];
>  
> @@ -1614,8 +1671,8 @@ static int init_vqs(struct ports_device *portdev)
>  			j += 2;
>  			io_callbacks[j] = in_intr;
>  			io_callbacks[j + 1] = out_intr;
> -			io_names[j] = "input";
> -			io_names[j + 1] = "output";
> +			io_names[j] = NULL;
> +			io_names[j + 1] = NULL;
>  		}
>  	}
>  	/* Find the queues. */
> @@ -1635,8 +1692,8 @@ static int init_vqs(struct ports_device *portdev)
>  
>  		for (i = 1; i < nr_ports; i++) {
>  			j += 2;
> -			portdev->in_vqs[i] = vqs[j];
> -			portdev->out_vqs[i] = vqs[j + 1];
> +			portdev->in_vqs[i] = NULL;
> +			portdev->out_vqs[i] = NULL;
>  		}
>  	}
>  	kfree(io_names);
So a queue once created will not be removed unless the module is
removed or the device is removed.  That seems reasonable, port
hot-unplug will keep queues around, as is the case now.

		Amit

On 2012/2/1,??? 16:12, Amit Shah wrote:
> Hi,
>
> Sorry for the late reply.
>
> On (Thu) 12 Jan 2012 [09:20:07], zanghongyong at huawei.com wrote:
>> From: Hongyong Zang<zanghongyong at huawei.com>
>>
>> Add setup_port_vq(). Create the io ports' vqs when add_port.
> Can you describe the changes in more detail, please?
The motivation of this patch is as follows. When we use virtio-serial
for communication between guest and host, we usually use only a few
ports (for example 1 or 2 ports), so there's no need to create max ports
when build a virtio-serial. The patch does the port hot-plug thing without
port hot-unplug.
>
>> Signed-off-by: Hongyong Zang<zanghongyong at huawei.com>
>> ---
>>   drivers/char/virtio_console.c |   65
++++++++++++++++++++++++++++++++++++++--
>>   1 files changed, 61 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/virtio_console.c
b/drivers/char/virtio_console.c
>> index 8e3c46d..2e5187e 100644
>> --- a/drivers/char/virtio_console.c
>> +++ b/drivers/char/virtio_console.c
>> @@ -1132,6 +1132,55 @@ static void send_sigio_to_port(struct port
*port)
>>   		kill_fasync(&port->async_queue, SIGIO, POLL_OUT);
>>   }
>>
>> +static void in_intr(struct virtqueue *vq);
>> +static void out_intr(struct virtqueue *vq);
>> +
>> +static int setup_port_vq(struct ports_device *portdev,  u32 id)
>> +{
>> +	int err, vq_num;
>> +	vq_callback_t **io_callbacks;
>> +	char **io_names;
>> +	struct virtqueue **vqs;
>> +	u32 i,j,nr_ports,nr_queues;
>> +
>> +	err = 0;
>> +	vq_num = (id + 1) * 2;
>> +	nr_ports = portdev->config.max_nr_ports;
>> +	nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
>> +
>> +	vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
>> +	io_callbacks = kmalloc(nr_queues * sizeof(vq_callback_t *),
GFP_KERNEL);
>> +	io_names = kmalloc(nr_queues * sizeof(char *), GFP_KERNEL);
>> +	if (!vqs || !io_callbacks || !io_names) {
>> +		err = -ENOMEM;
>> +		goto free;
>> +	}
>> +
>> +	for (i = 0, j = 0; i<= nr_ports; i++) {
>> +		io_callbacks[j] = in_intr;
>> +		io_callbacks[j + 1] = out_intr;
>> +		io_names[j] = NULL;
>> +		io_names[j + 1] = NULL;
>> +		j += 2;
>> +	}
>> +	io_names[vq_num] = "serial-input";
>> +	io_names[vq_num + 1] = "serial-output";
>> +	err = portdev->vdev->config->find_vqs(portdev->vdev,
nr_queues, vqs,
>> +				io_callbacks,
>> +				(const char **)io_names);
>> +	if (err)
>> +		goto free;
>> +	portdev->in_vqs[id] = vqs[vq_num];
>> +	portdev->out_vqs[id] = vqs[vq_num + 1];
> I don't think this approach will work fine for port hot-plug /
> hot-unplug cases at all.  For example, I first start qemu with one
> port, at id 1.  Then I add a port at id 5, then at 2, then at 10.
> Will that be fine?
yes.
In this case, the virtio-serial will create id 1's queue when probe the
device. Then, add port id 5, it will create the id 5's queue and the queues
of id 2-4 still be null.
we find the right ioport by vq_num, and only the io_name[vq_num] and
io_name[vq_num+1] are not null. So after
portdev->vdev->config->find_vqs(),
the id 5's queues are created(see the changes in virtio-pci), the others 
are
still null.
>
>> +
>> +free:
>> +	kfree(io_names);
>> +	kfree(io_callbacks);
>> +	kfree(vqs);
>> +
>> +	return err;
>> +}
>> +
>>   static int add_port(struct ports_device *portdev, u32 id)
>>   {
>>   	char debugfs_name[16];
>> @@ -1163,6 +1212,14 @@ static int add_port(struct ports_device
*portdev, u32 id)
>>
>>   	port->outvq_full = false;
>>
>> +	if (!portdev->in_vqs[port->id]&& 
!portdev->out_vqs[port->id]) {
>> +		spin_lock(&portdev->ports_lock);
>> +		err = setup_port_vq(portdev, port->id);
>> +		spin_unlock(&portdev->ports_lock);
>> +		if (err)
>> +			goto free_port;
>> +	}
>> +
>>   	port->in_vq = portdev->in_vqs[port->id];
>>   	port->out_vq = portdev->out_vqs[port->id];
>>
>> @@ -1614,8 +1671,8 @@ static int init_vqs(struct ports_device *portdev)
>>   			j += 2;
>>   			io_callbacks[j] = in_intr;
>>   			io_callbacks[j + 1] = out_intr;
>> -			io_names[j] = "input";
>> -			io_names[j + 1] = "output";
>> +			io_names[j] = NULL;
>> +			io_names[j + 1] = NULL;
>>   		}
>>   	}
>>   	/* Find the queues. */
>> @@ -1635,8 +1692,8 @@ static int init_vqs(struct ports_device *portdev)
>>
>>   		for (i = 1; i<  nr_ports; i++) {
>>   			j += 2;
>> -			portdev->in_vqs[i] = vqs[j];
>> -			portdev->out_vqs[i] = vqs[j + 1];
>> +			portdev->in_vqs[i] = NULL;
>> +			portdev->out_vqs[i] = NULL;
>>   		}
>>   	}
>>   	kfree(io_names);
> So a queue once created will not be removed unless the module is
> removed or the device is removed.  That seems reasonable, port
> hot-unplug will keep queues around, as is the case now.
As we use the virtio-serial with only a few io-ports, the maxport queues 
may
waste more memory. So we try to create queues when port hot-plug, as for
port hot-unplug there's no change.
>
> 		Amit
>
> .
>