Re: [PATCH 1/9] virtio_pci: remove struct virtio_pci_vq_info

2017-02-07 Thread Jason Wang 



On 2017年02月07日 17:38, Christoph Hellwig wrote:

On Tue, Feb 07, 2017 at 03:17:02PM +0800, Jason Wang wrote:

The check is still there.

Meh, I could swear I fixed it up.  Here is an updated version:

---
 From bf5e3b7fd272aea32388570503f00d0ab592fc2a Mon Sep 17 00:00:00 2001
From: Christoph Hellwig 
Date: Wed, 25 Jan 2017 13:40:21 +0100
Subject: virtio_pci: remove struct virtio_pci_vq_info

We don't really need struct virtio_pci_vq_info, as most field in there
are redundant:

  - the vq backpointer is not strictly neede to start with
  - the entry in the vqs list is not needed - the generic virtqueue already
has list, we only need to check if it has a callback to get the same
semantics
  - we can use a simple array to look up the MSI-X vec if needed.
  - That simple array now also duoble serves to replace the per_vq_vectors
flag

Signed-off-by: Christoph Hellwig 


Reviewed-by: Jason Wang 


---
  drivers/virtio/virtio_pci_common.c | 117 +++--
  drivers/virtio/virtio_pci_common.h |  25 +---
  drivers/virtio/virtio_pci_legacy.c |   6 +-
  drivers/virtio/virtio_pci_modern.c |   6 +-
  4 files changed, 39 insertions(+), 115 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c 
b/drivers/virtio/virtio_pci_common.c
index 186cbab327b8..1f9fac7dad61 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -62,16 +62,13 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
  static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
  {
struct virtio_pci_device *vp_dev = opaque;
-   struct virtio_pci_vq_info *info;
irqreturn_t ret = IRQ_NONE;
-   unsigned long flags;
+   struct virtqueue *vq;
  
-	spin_lock_irqsave(_dev->lock, flags);

-   list_for_each_entry(info, _dev->virtqueues, node) {
-   if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+   list_for_each_entry(vq, _dev->vdev.vqs, list) {
+   if (vring_interrupt(irq, vq) == IRQ_HANDLED)
ret = IRQ_HANDLED;
}
-   spin_unlock_irqrestore(_dev->lock, flags);
  
  	return ret;

  }
@@ -167,55 +164,6 @@ static int vp_request_msix_vectors(struct virtio_device 
*vdev, int nvectors,
return err;
  }
  
-static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,

-void (*callback)(struct virtqueue *vq),
-const char *name,
-u16 msix_vec)
-{
-   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-   struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
-   struct virtqueue *vq;
-   unsigned long flags;
-
-   /* fill out our structure that represents an active queue */
-   if (!info)
-   return ERR_PTR(-ENOMEM);
-
-   vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, msix_vec);
-   if (IS_ERR(vq))
-   goto out_info;
-
-   info->vq = vq;
-   if (callback) {
-   spin_lock_irqsave(_dev->lock, flags);
-   list_add(>node, _dev->virtqueues);
-   spin_unlock_irqrestore(_dev->lock, flags);
-   } else {
-   INIT_LIST_HEAD(>node);
-   }
-
-   vp_dev->vqs[index] = info;
-   return vq;
-
-out_info:
-   kfree(info);
-   return vq;
-}
-
-static void vp_del_vq(struct virtqueue *vq)
-{
-   struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-   struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
-   unsigned long flags;
-
-   spin_lock_irqsave(_dev->lock, flags);
-   list_del(>node);
-   spin_unlock_irqrestore(_dev->lock, flags);
-
-   vp_dev->del_vq(info);
-   kfree(info);
-}
-
  /* the config->del_vqs() implementation */
  void vp_del_vqs(struct virtio_device *vdev)
  {
@@ -224,16 +172,15 @@ void vp_del_vqs(struct virtio_device *vdev)
int i;
  
  	list_for_each_entry_safe(vq, n, >vqs, list) {

-   if (vp_dev->per_vq_vectors) {
-   int v = vp_dev->vqs[vq->index]->msix_vector;
+   if (vp_dev->msix_vector_map) {
+   int v = vp_dev->msix_vector_map[vq->index];
  
  			if (v != VIRTIO_MSI_NO_VECTOR)

free_irq(pci_irq_vector(vp_dev->pci_dev, v),
vq);
}
-   vp_del_vq(vq);
+   vp_dev->del_vq(vq);
}
-   vp_dev->per_vq_vectors = false;
  
  	if (vp_dev->intx_enabled) {

free_irq(vp_dev->pci_dev->irq, vp_dev);
@@ -261,8 +208,8 @@ void vp_del_vqs(struct virtio_device *vdev)
vp_dev->msix_names = NULL;
kfree(vp_dev->msix_affinity_masks);
vp_dev->msix_affinity_masks = NULL;
-   kfree(vp_dev->vqs);
-   vp_dev->vqs = NULL;
+   kfree(vp_dev->msix_vector_map);
+

Re: [PATCH 1/9] virtio_pci: remove struct virtio_pci_vq_info

2017-02-06 Thread Jason Wang 



On 2017年02月06日 01:15, Christoph Hellwig wrote:

We don't really need struct virtio_pci_vq_info, as most field in there
are redundant:

  - the vq backpointer is not strictly neede to start with
  - the entry in the vqs list is not needed - the generic virtqueue already
has list, we only need to check if it has a callback to get the same
semantics
  - we can use a simple array to look up the MSI-X vec if needed.
  - That simple array now also duoble serves to replace the per_vq_vectors
flag

Signed-off-by: Christoph Hellwig
---
  drivers/virtio/virtio_pci_common.c | 117 +++--
  drivers/virtio/virtio_pci_common.h |  25 +---
  drivers/virtio/virtio_pci_legacy.c |   6 +-
  drivers/virtio/virtio_pci_modern.c |   6 +-
  4 files changed, 39 insertions(+), 115 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c 
b/drivers/virtio/virtio_pci_common.c
index 186cbab327b8..a33767318cbf 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -62,16 +62,13 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
  static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
  {
struct virtio_pci_device *vp_dev = opaque;
-   struct virtio_pci_vq_info *info;
irqreturn_t ret = IRQ_NONE;
-   unsigned long flags;
+   struct virtqueue *vq;
  
-	spin_lock_irqsave(_dev->lock, flags);

-   list_for_each_entry(info, _dev->virtqueues, node) {
-   if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+   list_for_each_entry(vq, _dev->vdev.vqs, list) {
+   if (vq->callback && vring_interrupt(irq, vq) == IRQ_HANDLED)
ret = IRQ_HANDLED;
}


The check is still there.

Thanks

[PATCH 1/9] virtio_pci: remove struct virtio_pci_vq_info

2017-02-05 Thread Christoph Hellwig 
We don't really need struct virtio_pci_vq_info, as most field in there
are redundant:

 - the vq backpointer is not strictly neede to start with
 - the entry in the vqs list is not needed - the generic virtqueue already
   has list, we only need to check if it has a callback to get the same
   semantics
 - we can use a simple array to look up the MSI-X vec if needed.
 - That simple array now also duoble serves to replace the per_vq_vectors
   flag

Signed-off-by: Christoph Hellwig 
---
 drivers/virtio/virtio_pci_common.c | 117 +++--
 drivers/virtio/virtio_pci_common.h |  25 +---
 drivers/virtio/virtio_pci_legacy.c |   6 +-
 drivers/virtio/virtio_pci_modern.c |   6 +-
 4 files changed, 39 insertions(+), 115 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c 
b/drivers/virtio/virtio_pci_common.c
index 186cbab327b8..a33767318cbf 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -62,16 +62,13 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
 static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
 {
struct virtio_pci_device *vp_dev = opaque;
-   struct virtio_pci_vq_info *info;
irqreturn_t ret = IRQ_NONE;
-   unsigned long flags;
+   struct virtqueue *vq;
 
-   spin_lock_irqsave(_dev->lock, flags);
-   list_for_each_entry(info, _dev->virtqueues, node) {
-   if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+   list_for_each_entry(vq, _dev->vdev.vqs, list) {
+   if (vq->callback && vring_interrupt(irq, vq) == IRQ_HANDLED)
ret = IRQ_HANDLED;
}
-   spin_unlock_irqrestore(_dev->lock, flags);
 
return ret;
 }
@@ -167,55 +164,6 @@ static int vp_request_msix_vectors(struct virtio_device 
*vdev, int nvectors,
return err;
 }
 
-static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned 
index,
-void (*callback)(struct virtqueue *vq),
-const char *name,
-u16 msix_vec)
-{
-   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-   struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
-   struct virtqueue *vq;
-   unsigned long flags;
-
-   /* fill out our structure that represents an active queue */
-   if (!info)
-   return ERR_PTR(-ENOMEM);
-
-   vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, msix_vec);
-   if (IS_ERR(vq))
-   goto out_info;
-
-   info->vq = vq;
-   if (callback) {
-   spin_lock_irqsave(_dev->lock, flags);
-   list_add(>node, _dev->virtqueues);
-   spin_unlock_irqrestore(_dev->lock, flags);
-   } else {
-   INIT_LIST_HEAD(>node);
-   }
-
-   vp_dev->vqs[index] = info;
-   return vq;
-
-out_info:
-   kfree(info);
-   return vq;
-}
-
-static void vp_del_vq(struct virtqueue *vq)
-{
-   struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-   struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
-   unsigned long flags;
-
-   spin_lock_irqsave(_dev->lock, flags);
-   list_del(>node);
-   spin_unlock_irqrestore(_dev->lock, flags);
-
-   vp_dev->del_vq(info);
-   kfree(info);
-}
-
 /* the config->del_vqs() implementation */
 void vp_del_vqs(struct virtio_device *vdev)
 {
@@ -224,16 +172,15 @@ void vp_del_vqs(struct virtio_device *vdev)
int i;
 
list_for_each_entry_safe(vq, n, >vqs, list) {
-   if (vp_dev->per_vq_vectors) {
-   int v = vp_dev->vqs[vq->index]->msix_vector;
+   if (vp_dev->msix_vector_map) {
+   int v = vp_dev->msix_vector_map[vq->index];
 
if (v != VIRTIO_MSI_NO_VECTOR)
free_irq(pci_irq_vector(vp_dev->pci_dev, v),
vq);
}
-   vp_del_vq(vq);
+   vp_dev->del_vq(vq);
}
-   vp_dev->per_vq_vectors = false;
 
if (vp_dev->intx_enabled) {
free_irq(vp_dev->pci_dev->irq, vp_dev);
@@ -261,8 +208,8 @@ void vp_del_vqs(struct virtio_device *vdev)
vp_dev->msix_names = NULL;
kfree(vp_dev->msix_affinity_masks);
vp_dev->msix_affinity_masks = NULL;
-   kfree(vp_dev->vqs);
-   vp_dev->vqs = NULL;
+   kfree(vp_dev->msix_vector_map);
+   vp_dev->msix_vector_map = NULL;
 }
 
 static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
@@ -275,10 +222,6 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, 
unsigned nvqs,
u16 msix_vec;
int i, err, nvectors, allocated_vectors;
 
-   vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
-   if (!vp_dev->vqs)
-   return -ENOMEM;
-
if (per_vq_vectors) {

Re: [PATCH 1/9] virtio_pci: remove struct virtio_pci_vq_info

2017-02-03 Thread Christoph Hellwig 
On Fri, Feb 03, 2017 at 03:54:36PM +0800, Jason Wang wrote:
>> +list_for_each_entry(vq, _dev->vdev.vqs, list) {
>> +if (vq->callback && vring_interrupt(irq, vq) == IRQ_HANDLED)
>
> The check of vq->callback seems redundant, we will check it soon in 
> vring_interrupt().

Good point - I wanted to keep things exactly as-is and dіdn't notice
we were already protected.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9] virtio_pci: remove struct virtio_pci_vq_info

2017-02-02 Thread Jason Wang 



On 2017年01月27日 16:16, Christoph Hellwig wrote:

We don't really need struct virtio_pci_vq_info, as most field in there
are redundant:

  - the vq backpointer is not strictly neede to start with
  - the entry in the vqs list is not needed - the generic virtqueue already
has list, we only need to check if it has a callback to get the same
semantics
  - we can use a simple array to look up the MSI-X vec if needed.
  - That simple array now also duoble serves to replace the per_vq_vectors
flag

Signed-off-by: Christoph Hellwig 
---
  drivers/virtio/virtio_pci_common.c | 117 +++--
  drivers/virtio/virtio_pci_common.h |  25 +---
  drivers/virtio/virtio_pci_legacy.c |   6 +-
  drivers/virtio/virtio_pci_modern.c |   6 +-
  4 files changed, 39 insertions(+), 115 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c 
b/drivers/virtio/virtio_pci_common.c
index 186cbab..a3376731 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -62,16 +62,13 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
  static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
  {
struct virtio_pci_device *vp_dev = opaque;
-   struct virtio_pci_vq_info *info;
irqreturn_t ret = IRQ_NONE;
-   unsigned long flags;
+   struct virtqueue *vq;
  
-	spin_lock_irqsave(_dev->lock, flags);

-   list_for_each_entry(info, _dev->virtqueues, node) {
-   if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+   list_for_each_entry(vq, _dev->vdev.vqs, list) {
+   if (vq->callback && vring_interrupt(irq, vq) == IRQ_HANDLED)


The check of vq->callback seems redundant, we will check it soon in 
vring_interrupt().


Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/9] virtio_pci: remove struct virtio_pci_vq_info

2017-01-27 Thread Christoph Hellwig 
We don't really need struct virtio_pci_vq_info, as most field in there
are redundant:

 - the vq backpointer is not strictly neede to start with
 - the entry in the vqs list is not needed - the generic virtqueue already
   has list, we only need to check if it has a callback to get the same
   semantics
 - we can use a simple array to look up the MSI-X vec if needed.
 - That simple array now also duoble serves to replace the per_vq_vectors
   flag

Signed-off-by: Christoph Hellwig 
---
 drivers/virtio/virtio_pci_common.c | 117 +++--
 drivers/virtio/virtio_pci_common.h |  25 +---
 drivers/virtio/virtio_pci_legacy.c |   6 +-
 drivers/virtio/virtio_pci_modern.c |   6 +-
 4 files changed, 39 insertions(+), 115 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c 
b/drivers/virtio/virtio_pci_common.c
index 186cbab..a3376731 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -62,16 +62,13 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
 static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
 {
struct virtio_pci_device *vp_dev = opaque;
-   struct virtio_pci_vq_info *info;
irqreturn_t ret = IRQ_NONE;
-   unsigned long flags;
+   struct virtqueue *vq;
 
-   spin_lock_irqsave(_dev->lock, flags);
-   list_for_each_entry(info, _dev->virtqueues, node) {
-   if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+   list_for_each_entry(vq, _dev->vdev.vqs, list) {
+   if (vq->callback && vring_interrupt(irq, vq) == IRQ_HANDLED)
ret = IRQ_HANDLED;
}
-   spin_unlock_irqrestore(_dev->lock, flags);
 
return ret;
 }
@@ -167,55 +164,6 @@ static int vp_request_msix_vectors(struct virtio_device 
*vdev, int nvectors,
return err;
 }
 
-static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned 
index,
-void (*callback)(struct virtqueue *vq),
-const char *name,
-u16 msix_vec)
-{
-   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-   struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
-   struct virtqueue *vq;
-   unsigned long flags;
-
-   /* fill out our structure that represents an active queue */
-   if (!info)
-   return ERR_PTR(-ENOMEM);
-
-   vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, msix_vec);
-   if (IS_ERR(vq))
-   goto out_info;
-
-   info->vq = vq;
-   if (callback) {
-   spin_lock_irqsave(_dev->lock, flags);
-   list_add(>node, _dev->virtqueues);
-   spin_unlock_irqrestore(_dev->lock, flags);
-   } else {
-   INIT_LIST_HEAD(>node);
-   }
-
-   vp_dev->vqs[index] = info;
-   return vq;
-
-out_info:
-   kfree(info);
-   return vq;
-}
-
-static void vp_del_vq(struct virtqueue *vq)
-{
-   struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-   struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
-   unsigned long flags;
-
-   spin_lock_irqsave(_dev->lock, flags);
-   list_del(>node);
-   spin_unlock_irqrestore(_dev->lock, flags);
-
-   vp_dev->del_vq(info);
-   kfree(info);
-}
-
 /* the config->del_vqs() implementation */
 void vp_del_vqs(struct virtio_device *vdev)
 {
@@ -224,16 +172,15 @@ void vp_del_vqs(struct virtio_device *vdev)
int i;
 
list_for_each_entry_safe(vq, n, >vqs, list) {
-   if (vp_dev->per_vq_vectors) {
-   int v = vp_dev->vqs[vq->index]->msix_vector;
+   if (vp_dev->msix_vector_map) {
+   int v = vp_dev->msix_vector_map[vq->index];
 
if (v != VIRTIO_MSI_NO_VECTOR)
free_irq(pci_irq_vector(vp_dev->pci_dev, v),
vq);
}
-   vp_del_vq(vq);
+   vp_dev->del_vq(vq);
}
-   vp_dev->per_vq_vectors = false;
 
if (vp_dev->intx_enabled) {
free_irq(vp_dev->pci_dev->irq, vp_dev);
@@ -261,8 +208,8 @@ void vp_del_vqs(struct virtio_device *vdev)
vp_dev->msix_names = NULL;
kfree(vp_dev->msix_affinity_masks);
vp_dev->msix_affinity_masks = NULL;
-   kfree(vp_dev->vqs);
-   vp_dev->vqs = NULL;
+   kfree(vp_dev->msix_vector_map);
+   vp_dev->msix_vector_map = NULL;
 }
 
 static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
@@ -275,10 +222,6 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, 
unsigned nvqs,
u16 msix_vec;
int i, err, nvectors, allocated_vectors;
 
-   vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
-   if (!vp_dev->vqs)
-   return -ENOMEM;
-
if (per_vq_vectors) {