Re: [PATCH V4 2/2] tcm_vhost: Add hotplug/hotunplug support
On Tue, Mar 26, 2013 at 10:46:28PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 26, 2013 at 10:56:33AM +0800, Asias He wrote:
> > On Mon, Mar 25, 2013 at 01:10:33PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Mar 25, 2013 at 03:48:06PM +0800, Asias He wrote:
> > > > On Sun, Mar 24, 2013 at 05:20:09PM +0200, Michael S. Tsirkin wrote:
> > > > > On Fri, Mar 22, 2013 at 01:39:05PM +0800, Asias He wrote:
> > > > > > In commit 365a7150094 ([SCSI] virtio-scsi: hotplug support for
> > > > > > virtio-scsi), hotplug support is added to virtio-scsi.
> > > > > >
> > > > > > This patch adds hotplug and hotunplug support to tcm_vhost.
> > > > > >
> > > > > > You can create or delete a LUN in targetcli to hotplug or hotunplug
> > > > > > a
> > > > > > LUN in guest.
> > > > > >
> > > > > > Changes in v4:
> > > > > > - Drop tcm_vhost_check_endpoint in tcm_vhost_send_evt
> > > > > > - Add tcm_vhost_check_endpoint in vhost_scsi_evt_handle_kick
> > > > > >
> > > > > > Changes in v3:
> > > > > > - Separate the bug fix to another thread
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Remove code duplication in tcm_vhost_{hotplug,hotunplug}
> > > > > > - Fix racing of vs_events_nr
> > > > > > - Add flush fix patch to this series
> > > > > >
> > > > > > Signed-off-by: Asias He
> > > > > > Reviewed-by: Stefan Hajnoczi
> > > > > > ---
> > > > > > drivers/vhost/tcm_vhost.c | 212
> > > > > > --
> > > > > > drivers/vhost/tcm_vhost.h | 10 +++
> > > > > > 2 files changed, 217 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > > > > > index d81e3a9..e734ead 100644
> > > > > > --- a/drivers/vhost/tcm_vhost.c
> > > > > > +++ b/drivers/vhost/tcm_vhost.c
> > > > > > @@ -62,6 +62,9 @@ enum {
> > > > > >
> > > > > > #define VHOST_SCSI_MAX_TARGET 256
> > > > > > #define VHOST_SCSI_MAX_VQ 128
> > > > > > +#define VHOST_SCSI_MAX_EVENT 128
> > > > > > +
> > > > > > +#define VHOST_SCSI_FEATURES (VHOST_FEATURES | (1ULL <<
> > > > > > VIRTIO_SCSI_F_HOTPLUG))
> > > > > >
> > > > > > struct vhost_scsi {
> > > > > > /* Protected by vhost_scsi->dev.mutex */
> > > > > > @@ -73,6 +76,12 @@ struct vhost_scsi {
> > > > > >
> > > > > > struct vhost_work vs_completion_work; /* cmd completion work
> > > > > > item */
> > > > > > struct llist_head vs_completion_list; /* cmd completion queue */
> > > > > > +
> > > > > > + struct vhost_work vs_event_work; /* evt injection work item */
> > > > > > + struct llist_head vs_event_list; /* evt injection queue */
> > > > > > +
> > > > > > + bool vs_events_dropped; /* any missed events, protected by
> > > > > > dev.mutex */
> > > > > > + u64 vs_events_nr; /* num of pending events, protected by
> > > > > > dev.mutex */
> > > > >
> > > > > Woa u64. We allow up to 128 of these. int will do.
> > > >
> > > > u64 is used before we limit the total number of events, changed to int.
> > > >
> > > > > > };
> > > > > >
> > > > > > /* Local pointer to allocated TCM configfs fabric module */
> > > > > > @@ -120,6 +129,16 @@ static bool tcm_vhost_check_endpoint(struct
> > > > > > vhost_virtqueue *vq)
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > +static bool tcm_vhost_check_events_dropped(struct vhost_scsi *vs)
> > > > > > +{
> > > > > > + bool ret;
> > > > > > +
> > > > > > + mutex_lock(&vs->dev.mutex);
> > > > > > + ret = vs->vs_events_dropped;
> > > > > > + mutex_unlock(&vs->dev.mutex);
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
> > > > > > {
> > > > > > return 1;
> > > > > > @@ -370,6 +389,36 @@ static int tcm_vhost_queue_tm_rsp(struct
> > > > > > se_cmd *se_cmd)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +static void tcm_vhost_free_evt(struct vhost_scsi *vs, struct
> > > > > > tcm_vhost_evt *evt)
> > > > > > +{
> > > > > > + mutex_lock(&vs->dev.mutex);
> > > > > > + vs->vs_events_nr--;
> > > > > > + kfree(evt);
> > > > > > + mutex_unlock(&vs->dev.mutex);
> > > > > > +}
> > > > > > +
> > > > > > +static struct tcm_vhost_evt *tcm_vhost_allocate_evt(struct
> > > > > > vhost_scsi *vs,
> > > > > > + u32 event, u32 reason)
> > > > > > +{
> > > > > > + struct tcm_vhost_evt *evt;
> > > > > > +
> > > > > > + mutex_lock(&vs->dev.mutex);
> > > > > > + if (vs->vs_events_nr > VHOST_SCSI_MAX_EVENT) {
> > > > >
> > > > >
> > > > > Se eventfd dropped flag here and stop worrying about it in callers?
> > > >
> > > > Set the flag here now and do not bother the callers.
> > > >
> > > > > > + mutex_unlock(&vs->dev.mutex);
> > > > > > + return NULL;
> > > > > > + }
> > > > > > +
> > > > > > + evt = kzalloc(sizeof(*evt), GFP_KERNEL);
> > > > > > + if (evt) {
> > > > > > + evt->event.event = event;
> > > > > > + evt->event.reason = reason;
> > >
Re: [PATCH V2 1/3] tcm_vhost: Wait for pending requests in vhost_scsi_flush()
On Tue, Mar 26, 2013 at 10:51:54PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 26, 2013 at 10:38:27AM +0800, Asias He wrote:
> > On Mon, Mar 25, 2013 at 01:13:39PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Mar 25, 2013 at 03:39:42PM +0800, Asias He wrote:
> > > > On Sun, Mar 24, 2013 at 05:11:37PM +0200, Michael S. Tsirkin wrote:
> > > > > On Fri, Mar 22, 2013 at 02:55:27PM +0800, Asias He wrote:
> > > > > > This patch makes vhost_scsi_flush() wait for all the pending
> > > > > > requests
> > > > > > issued before the flush operation to be finished.
> > > > > >
> > > > > > Signed-off-by: Asias He
> > > > > > ---
> > > > > > drivers/vhost/tcm_vhost.c | 117
> > > > > > ++
> > > > > > drivers/vhost/tcm_vhost.h | 4 ++
> > > > > > 2 files changed, 101 insertions(+), 20 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > > > > > index e734ead..dc0af52 100644
> > > > > > --- a/drivers/vhost/tcm_vhost.c
> > > > > > +++ b/drivers/vhost/tcm_vhost.c
> > > > > > @@ -82,6 +82,15 @@ struct vhost_scsi {
> > > > > >
> > > > > > bool vs_events_dropped; /* any missed events, protected by
> > > > > > dev.mutex */
> > > > > > u64 vs_events_nr; /* num of pending events, protected by
> > > > > > dev.mutex */
> > > > > > +
> > > > > > + /*
> > > > > > +* vs_inflight[0]/[1] are used to track requests issued
> > > > > > +* before/during the flush operation
> > > > > > +*/
> > > > > > + u64 vs_inflight[2];
> > > > > > + wait_queue_head_t vs_flush_wait; /* wait queue for flush
> > > > > > operation */
> > > > > > + spinlock_t vs_flush_lock; /* lock to protect vs_during_flush */
> > > > > > + int vs_during_flush; /* flag to indicate if we are in flush
> > > > > > operation */
> > > > >
> > > > > So this adds a spinlock on data path and I'm not sure
> > > > > I understand why this is correct (see also comment below).
> > > >
> > > > vs_flush_lock is accessed in:
> > > >
> > > > 1) tcm_vhost_allocate_evt() and tcm_vhost_free_evt()
> > > > 2) vhost_scsi_allocate_cmd() and vhost_scsi_free_cmd().
> > > >
> > > > The former is not on data path. The later is always executed in the
> > > > vhost thread. So we can almost always take the lock with no contention.
> > > >
> > > > And I am not seeing any real perf differences.
> > > >
> > > > > And generally we should try to avoid reimplementing refcounting. How
> > > > > about we use a kref pointer instead? Access can use RCU (or maybe put
> > > > > it in vq->private and use the tricky vhost version of RCU).
> > > > > Initialize
> > > > > it to 1. To flush you replace the pointer, decrement then wait for
> > > > > refcount to reach 0.
> > > >
> > > > This makes the code even more tricky and hard to understand. I am not
> > > > sure this is a place where kref is the right choice.
> > >
> > > Point is, this homegrown reference-counting implementation seems to be
> > > buggy, see the comment below. And it is doing flushes which is similar
> > > to RCU anyway. Direct use of RCU will at least be well documented.
> >
> > If we do full flush. We do no bothering the complex at all.
>
> Hmm parse error. What do you mean?
I mean if we flush all the requests. It would be much simpler.
> > > > > This still adds atomics on data path so maybe worth benchmarking to
> > > > > verify performance overhead is not measureable, but at least it's
> > > > > one atomic and not a full lock.
> > > > > Hmm?
> > > > >
> > > > >
> > > > > > };
> > > > > >
> > > > > > /* Local pointer to allocated TCM configfs fabric module */
> > > > > > @@ -99,6 +108,46 @@ static int iov_num_pages(struct iovec *iov)
> > > > > >((unsigned long)iov->iov_base & PAGE_MASK)) >>
> > > > > > PAGE_SHIFT;
> > > > > > }
> > > > > >
> > > > > > +static int tcm_vhost_inc_inflight(struct vhost_scsi *vs)
> > > > > > +{
> > > > > > + int during_flush;
> > > > > > +
> > > > > > + spin_lock(&vs->vs_flush_lock);
> > > > > > + during_flush = vs->vs_during_flush;
> > > > > > + vs->vs_inflight[during_flush]++;
> > > > > > + spin_unlock(&vs->vs_flush_lock);
> > > > > > +
> > > > > > + return during_flush;
> > > > > > +}
> > > > > > +
> > > > > > +static void tcm_vhost_dec_inflight(struct vhost_scsi *vs, int
> > > > > > during_flush)
> > > > > > +{
> > > > > > + u64 inflight;
> > > > > > +
> > > > > > + spin_lock(&vs->vs_flush_lock);
> > > > > > + inflight = vs->vs_inflight[during_flush]--;
> > > > > > + /*
> > > > > > +* Wakeup the waiter when all the requests issued before the
> > > > > > flush
> > > > > > +* operation are finished and we are during the flush operation.
> > > > > > +*/
> > > > > > + if (!inflight && !during_flush && vs->vs_during_flush)
> > > > > > + wake_up(&vs->vs_flush_wait);
> > > > > > + spin_unlock(&vs->vs_flush_lock);
> > > > > > +}
> > > > > > +
> > > > > > +static bool tcm_vhost_done_inflight(stru
Re: [PATCH 16/22] virtio_pci: use separate notification offsets for each vq.
On 03/26/2013 05:22 PM, H. Peter Anvin wrote: > I would say let it be undefined... in most cases the host will know what > device(s) will matter; e.g. if the guest is ppc no point in providing an I/O > BAR. For pluggable physical devices, though, both should be provided. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 16/22] virtio_pci: use separate notification offsets for each vq.
I would say let it be undefined... in most cases the host will know what device(s) will matter; e.g. if the guest is ppc no point in providing an I/O BAR. Rusty Russell wrote: >"Michael S. Tsirkin" writes: >> On Mon, Mar 25, 2013 at 08:30:28PM +1030, Rusty Russell wrote: >>> Let's go back a level. Do we still need I/O bars at all now? Or >can we >>> say "if you want hundreds of vqs, use mem bars"? >>> >>> hpa wanted the option to have either, but do we still want that? >> >> hpa says having both is required for BIOS, not just for speed with >KVM. > >OK so the offset must not be applied to the I/O bar as you suggested. > >Since AFAICT I/O bars are deprecated, should we insist that there be a >memory bar, and the I/O bar is optional? Or just leave it entirely >undefined, and say there can be either or both? > >I dislike the idea of BIOS code which assumed an I/O bar and thus won't >work with a compliant device which doesn't provide one. I'd prefer all >compliant drivers to work with all compliant devices. > >Cheers, >Rusty. -- Sent from my mobile phone. Please excuse brevity and lack of formatting. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 16/22] virtio_pci: use separate notification offsets for each vq.
"Michael S. Tsirkin" writes: > On Mon, Mar 25, 2013 at 08:30:28PM +1030, Rusty Russell wrote: >> Let's go back a level. Do we still need I/O bars at all now? Or can we >> say "if you want hundreds of vqs, use mem bars"? >> >> hpa wanted the option to have either, but do we still want that? > > hpa says having both is required for BIOS, not just for speed with KVM. OK so the offset must not be applied to the I/O bar as you suggested. Since AFAICT I/O bars are deprecated, should we insist that there be a memory bar, and the I/O bar is optional? Or just leave it entirely undefined, and say there can be either or both? I dislike the idea of BIOS code which assumed an I/O bar and thus won't work with a compliant device which doesn't provide one. I'd prefer all compliant drivers to work with all compliant devices. Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drivers: virtio: Use PTR_RET function
Andru Gheorghiu writes: > PTR_RET does return. It's perfectly equivalent to using IS_ERR and the > returning PTR_ERR. The implementation is here [1]. Um, I read the implementation, thanks. > The reason for using it is this: if you have a function that does > something why not call it instead of reproducing it's behavior by > explicitly writing what it does. Because clarity matters, and this function makes callers less clear. It's the most breathtakingly bad name since BUILD_BUG_ON_ZERO(). Why not change PTR_ERR to return 0 if !IS_ERR()? Noone breaks, gcc probably produces the same code, and noone needs to learn your weird new kernel meme. In fact, as gcc will produce the same code for "if (PTR_ERR(p))" as it does for "if (IS_ERR(p))", you get to be one of the very, very few people who have ever *reduced* the complexity of a kernel interface. Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] xen: fix fbdev frontend kconfig warning
From: Randy Dunlap Fix kconfig dependency warning for XEN_FBDEV_FRONTEND: warning: (XEN_FBDEV_FRONTEND) selects INPUT_XEN_KBDDEV_FRONTEND which has unmet direct dependencies (!UML && INPUT && INPUT_MISC && XEN) Signed-off-by: Randy Dunlap Cc: Konrad Rzeszutek Wilk Cc: Jeremy Fitzhardinge Cc: xen-de...@lists.xensource.com Cc: virtualization@lists.linux-foundation.org --- drivers/video/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- lnx-39-rc4.orig/drivers/video/Kconfig +++ lnx-39-rc4/drivers/video/Kconfig @@ -2277,7 +2277,7 @@ config XEN_FBDEV_FRONTEND select FB_SYS_IMAGEBLIT select FB_SYS_FOPS select FB_DEFERRED_IO - select INPUT_XEN_KBDDEV_FRONTEND + select INPUT_XEN_KBDDEV_FRONTEND if INPUT select XEN_XENBUS_FRONTEND default y help ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 1/3] tcm_vhost: Wait for pending requests in vhost_scsi_flush()
On Tue, Mar 26, 2013 at 10:38:27AM +0800, Asias He wrote:
> On Mon, Mar 25, 2013 at 01:13:39PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Mar 25, 2013 at 03:39:42PM +0800, Asias He wrote:
> > > On Sun, Mar 24, 2013 at 05:11:37PM +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Mar 22, 2013 at 02:55:27PM +0800, Asias He wrote:
> > > > > This patch makes vhost_scsi_flush() wait for all the pending requests
> > > > > issued before the flush operation to be finished.
> > > > >
> > > > > Signed-off-by: Asias He
> > > > > ---
> > > > > drivers/vhost/tcm_vhost.c | 117
> > > > > ++
> > > > > drivers/vhost/tcm_vhost.h | 4 ++
> > > > > 2 files changed, 101 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > > > > index e734ead..dc0af52 100644
> > > > > --- a/drivers/vhost/tcm_vhost.c
> > > > > +++ b/drivers/vhost/tcm_vhost.c
> > > > > @@ -82,6 +82,15 @@ struct vhost_scsi {
> > > > >
> > > > > bool vs_events_dropped; /* any missed events, protected by
> > > > > dev.mutex */
> > > > > u64 vs_events_nr; /* num of pending events, protected by
> > > > > dev.mutex */
> > > > > +
> > > > > + /*
> > > > > + * vs_inflight[0]/[1] are used to track requests issued
> > > > > + * before/during the flush operation
> > > > > + */
> > > > > + u64 vs_inflight[2];
> > > > > + wait_queue_head_t vs_flush_wait; /* wait queue for flush
> > > > > operation */
> > > > > + spinlock_t vs_flush_lock; /* lock to protect vs_during_flush */
> > > > > + int vs_during_flush; /* flag to indicate if we are in flush
> > > > > operation */
> > > >
> > > > So this adds a spinlock on data path and I'm not sure
> > > > I understand why this is correct (see also comment below).
> > >
> > > vs_flush_lock is accessed in:
> > >
> > > 1) tcm_vhost_allocate_evt() and tcm_vhost_free_evt()
> > > 2) vhost_scsi_allocate_cmd() and vhost_scsi_free_cmd().
> > >
> > > The former is not on data path. The later is always executed in the
> > > vhost thread. So we can almost always take the lock with no contention.
> > >
> > > And I am not seeing any real perf differences.
> > >
> > > > And generally we should try to avoid reimplementing refcounting. How
> > > > about we use a kref pointer instead? Access can use RCU (or maybe put
> > > > it in vq->private and use the tricky vhost version of RCU). Initialize
> > > > it to 1. To flush you replace the pointer, decrement then wait for
> > > > refcount to reach 0.
> > >
> > > This makes the code even more tricky and hard to understand. I am not
> > > sure this is a place where kref is the right choice.
> >
> > Point is, this homegrown reference-counting implementation seems to be
> > buggy, see the comment below. And it is doing flushes which is similar
> > to RCU anyway. Direct use of RCU will at least be well documented.
>
> If we do full flush. We do no bothering the complex at all.
Hmm parse error. What do you mean?
> > > > This still adds atomics on data path so maybe worth benchmarking to
> > > > verify performance overhead is not measureable, but at least it's
> > > > one atomic and not a full lock.
> > > > Hmm?
> > > >
> > > >
> > > > > };
> > > > >
> > > > > /* Local pointer to allocated TCM configfs fabric module */
> > > > > @@ -99,6 +108,46 @@ static int iov_num_pages(struct iovec *iov)
> > > > > ((unsigned long)iov->iov_base & PAGE_MASK)) >>
> > > > > PAGE_SHIFT;
> > > > > }
> > > > >
> > > > > +static int tcm_vhost_inc_inflight(struct vhost_scsi *vs)
> > > > > +{
> > > > > + int during_flush;
> > > > > +
> > > > > + spin_lock(&vs->vs_flush_lock);
> > > > > + during_flush = vs->vs_during_flush;
> > > > > + vs->vs_inflight[during_flush]++;
> > > > > + spin_unlock(&vs->vs_flush_lock);
> > > > > +
> > > > > + return during_flush;
> > > > > +}
> > > > > +
> > > > > +static void tcm_vhost_dec_inflight(struct vhost_scsi *vs, int
> > > > > during_flush)
> > > > > +{
> > > > > + u64 inflight;
> > > > > +
> > > > > + spin_lock(&vs->vs_flush_lock);
> > > > > + inflight = vs->vs_inflight[during_flush]--;
> > > > > + /*
> > > > > + * Wakeup the waiter when all the requests issued before the
> > > > > flush
> > > > > + * operation are finished and we are during the flush operation.
> > > > > + */
> > > > > + if (!inflight && !during_flush && vs->vs_during_flush)
> > > > > + wake_up(&vs->vs_flush_wait);
> > > > > + spin_unlock(&vs->vs_flush_lock);
> > > > > +}
> > > > > +
> > > > > +static bool tcm_vhost_done_inflight(struct vhost_scsi *vs)
> > > > > +{
> > > > > + bool ret = false;
> > > > > +
> > > > > + /* The requests issued before the flush operation are finished
> > > > > ? */
> > > > > + spin_lock(&vs->vs_flush_lock);
> > > > > + if (!vs->vs_inflight[0])
> > > > > + ret = true;
> > > > > + spin_unlo
Re: [PATCH V4 2/2] tcm_vhost: Add hotplug/hotunplug support
On Tue, Mar 26, 2013 at 10:56:33AM +0800, Asias He wrote:
> On Mon, Mar 25, 2013 at 01:10:33PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Mar 25, 2013 at 03:48:06PM +0800, Asias He wrote:
> > > On Sun, Mar 24, 2013 at 05:20:09PM +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Mar 22, 2013 at 01:39:05PM +0800, Asias He wrote:
> > > > > In commit 365a7150094 ([SCSI] virtio-scsi: hotplug support for
> > > > > virtio-scsi), hotplug support is added to virtio-scsi.
> > > > >
> > > > > This patch adds hotplug and hotunplug support to tcm_vhost.
> > > > >
> > > > > You can create or delete a LUN in targetcli to hotplug or hotunplug a
> > > > > LUN in guest.
> > > > >
> > > > > Changes in v4:
> > > > > - Drop tcm_vhost_check_endpoint in tcm_vhost_send_evt
> > > > > - Add tcm_vhost_check_endpoint in vhost_scsi_evt_handle_kick
> > > > >
> > > > > Changes in v3:
> > > > > - Separate the bug fix to another thread
> > > > >
> > > > > Changes in v2:
> > > > > - Remove code duplication in tcm_vhost_{hotplug,hotunplug}
> > > > > - Fix racing of vs_events_nr
> > > > > - Add flush fix patch to this series
> > > > >
> > > > > Signed-off-by: Asias He
> > > > > Reviewed-by: Stefan Hajnoczi
> > > > > ---
> > > > > drivers/vhost/tcm_vhost.c | 212
> > > > > --
> > > > > drivers/vhost/tcm_vhost.h | 10 +++
> > > > > 2 files changed, 217 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > > > > index d81e3a9..e734ead 100644
> > > > > --- a/drivers/vhost/tcm_vhost.c
> > > > > +++ b/drivers/vhost/tcm_vhost.c
> > > > > @@ -62,6 +62,9 @@ enum {
> > > > >
> > > > > #define VHOST_SCSI_MAX_TARGET256
> > > > > #define VHOST_SCSI_MAX_VQ128
> > > > > +#define VHOST_SCSI_MAX_EVENT 128
> > > > > +
> > > > > +#define VHOST_SCSI_FEATURES (VHOST_FEATURES | (1ULL <<
> > > > > VIRTIO_SCSI_F_HOTPLUG))
> > > > >
> > > > > struct vhost_scsi {
> > > > > /* Protected by vhost_scsi->dev.mutex */
> > > > > @@ -73,6 +76,12 @@ struct vhost_scsi {
> > > > >
> > > > > struct vhost_work vs_completion_work; /* cmd completion work
> > > > > item */
> > > > > struct llist_head vs_completion_list; /* cmd completion queue */
> > > > > +
> > > > > + struct vhost_work vs_event_work; /* evt injection work item */
> > > > > + struct llist_head vs_event_list; /* evt injection queue */
> > > > > +
> > > > > + bool vs_events_dropped; /* any missed events, protected by
> > > > > dev.mutex */
> > > > > + u64 vs_events_nr; /* num of pending events, protected by
> > > > > dev.mutex */
> > > >
> > > > Woa u64. We allow up to 128 of these. int will do.
> > >
> > > u64 is used before we limit the total number of events, changed to int.
> > >
> > > > > };
> > > > >
> > > > > /* Local pointer to allocated TCM configfs fabric module */
> > > > > @@ -120,6 +129,16 @@ static bool tcm_vhost_check_endpoint(struct
> > > > > vhost_virtqueue *vq)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > +static bool tcm_vhost_check_events_dropped(struct vhost_scsi *vs)
> > > > > +{
> > > > > + bool ret;
> > > > > +
> > > > > + mutex_lock(&vs->dev.mutex);
> > > > > + ret = vs->vs_events_dropped;
> > > > > + mutex_unlock(&vs->dev.mutex);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
> > > > > {
> > > > > return 1;
> > > > > @@ -370,6 +389,36 @@ static int tcm_vhost_queue_tm_rsp(struct se_cmd
> > > > > *se_cmd)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static void tcm_vhost_free_evt(struct vhost_scsi *vs, struct
> > > > > tcm_vhost_evt *evt)
> > > > > +{
> > > > > + mutex_lock(&vs->dev.mutex);
> > > > > + vs->vs_events_nr--;
> > > > > + kfree(evt);
> > > > > + mutex_unlock(&vs->dev.mutex);
> > > > > +}
> > > > > +
> > > > > +static struct tcm_vhost_evt *tcm_vhost_allocate_evt(struct
> > > > > vhost_scsi *vs,
> > > > > + u32 event, u32 reason)
> > > > > +{
> > > > > + struct tcm_vhost_evt *evt;
> > > > > +
> > > > > + mutex_lock(&vs->dev.mutex);
> > > > > + if (vs->vs_events_nr > VHOST_SCSI_MAX_EVENT) {
> > > >
> > > >
> > > > Se eventfd dropped flag here and stop worrying about it in callers?
> > >
> > > Set the flag here now and do not bother the callers.
> > >
> > > > > + mutex_unlock(&vs->dev.mutex);
> > > > > + return NULL;
> > > > > + }
> > > > > +
> > > > > + evt = kzalloc(sizeof(*evt), GFP_KERNEL);
> > > > > + if (evt) {
> > > > > + evt->event.event = event;
> > > > > + evt->event.reason = reason;
> > > > > + vs->vs_events_nr++;
> > > > > + }
> > > > > + mutex_unlock(&vs->dev.mutex);
> > > > > +
> > > > > + return evt;
> > > > > +}
> > > > > +
> > > > > static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
> > > > > {
> > > > >
Re: [PATCH 16/22] virtio_pci: use separate notification offsets for each vq.
On Mon, Mar 25, 2013 at 08:30:28PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" writes:
> > On Fri, Mar 22, 2013 at 01:22:57PM +1030, Rusty Russell wrote:
> >> "Michael S. Tsirkin" writes:
> >> > I would like an option for hypervisor to simply say "Do IO
> >> > to this fixed address for this VQ". Then virtio can avoid using IO BARs
> >> > completely.
> >>
> >> It could be done. AFAICT, this would be an x86-ism, though, which is a
> >> little nasty.
> >
> > Okay, talked to HPA and he suggests a useful extension of my
> > or rather Gleb's earlier idea
> > (which was accessing mmio from special asm code which puts the value in
> > a known predefined register):
> > if we make each queue use a different address, then we avoid
> > the need to emulate the instruction (because we get GPA in the VMCS),
> > and the value can just be ignored.
>
> I had the same thought, but obviously lost it when I re-parsed your
> message.
I will try to implement this in KVM, and benchmark. Then we'll see.
> > There's still some overhead (CPU simply seems to take a bit more
> > time to handle an EPT violation than an IO access)
> > and we need to actually add such code in kvm in host kernel,
> > but it sure looks nice since unlike my idea it does not
> > need anything special in the guest, and it will just work
> > for a physical virtio device if such ever surfaces.
>
> I think a physical virtio device would be a bit weird, but it's a nice
> sanity check.
>
> But if we do this, let's drop back to the simpler layout suggested in
> the original patch (a u16 offset, and you write the vq index there).
> >> @@ -150,7 +153,9 @@ struct virtio_pci_common_cfg {
> >>__le16 queue_size; /* read-write, power of 2. */
> >>__le16 queue_msix_vector;/* read-write */
> >>__le16 queue_enable;/* read-write */
> >> - __le16 queue_notify;/* read-only */
> >> + __le16 unused2;
> >> + __le32 queue_notify_val;/* read-only */
> >> + __le32 queue_notify_off;/* read-only */
> >>__le64 queue_desc; /* read-write */
> >>__le64 queue_avail; /* read-write */
> >>__le64 queue_used; /* read-write */
> >
> > So how exactly do the offsets mesh with the dual capability? For IO we
> > want to use the same address and get queue from the data, for memory we
> > want a per queue address ...
>
> Let's go back a level. Do we still need I/O bars at all now? Or can we
> say "if you want hundreds of vqs, use mem bars"?
>
> hpa wanted the option to have either, but do we still want that?
>
> Cheers,
> Rusty.
hpa says having both is required for BIOS, not just for speed with KVM.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drivers: virtio: Use PTR_RET function
PTR_RET does return. It's perfectly equivalent to using IS_ERR and the returning PTR_ERR. The implementation is here [1]. The reason for using it is this: if you have a function that does something why not call it instead of reproducing it's behavior by explicitly writing what it does. [1] http://lxr.free-electrons.com/source/include/linux/err.h#L55 Alexandru Gheorghiu On Tue, Mar 26, 2013 at 7:01 AM, Andrew Morton wrote: > On Tue, 26 Mar 2013 13:57:09 +1030 Rusty Russell > wrote: > > > Alexandru Gheorghiu writes: > > > > > Used PTR_RET function instead of IS_ERR and PTR_ERR. > > > Patch found using coccinelle. > > > > WTF is PTR_RET? PTR_RET doesn't return anything. Why is it called > > that? It doesn't even make sense. > > > > ZERO_OR_PTR_ERR() maybe. > > > > But what problem are we solving? Insufficient churn in the tree? Code > > being too readable? This isn't some hard-to-get right corner case, or a > > missed optimization. > > > > Andrew, what am I missing here? > > It seemed like a good idea at the time. Merged it two years ago and > have since been keenly awaiting an opportunity to use it. > > It seems that people _have_ been using it, but mainly netfilter people > and we know they're all crazy ;) > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
