| Message ID | 1342564640.18004.492.camel@haakon2.linux-iscsi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 17, 2012 at 03:37:20PM -0700, Nicholas A. Bellinger wrote: > On Wed, 2012-07-18 at 01:18 +0300, Michael S. Tsirkin wrote: > > On Tue, Jul 17, 2012 at 03:02:08PM -0700, Nicholas A. Bellinger wrote: > > > On Wed, 2012-07-18 at 00:34 +0300, Michael S. Tsirkin wrote: > > > > On Tue, Jul 17, 2012 at 02:17:22PM -0700, Nicholas A. Bellinger wrote: > > > > > On Tue, 2012-07-17 at 18:05 +0300, Michael S. Tsirkin wrote: > > > > > > On Wed, Jul 11, 2012 at 09:15:00PM +0000, Nicholas A. Bellinger wrote: > > > > > > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > <SNIP> > > > > As mentioned in the response to Anthony, we are using the same generic > > > fabric ABI in drivers/target/target_core_fabric_configfs.c since .38. > > > > > > That part is not going to change, and it has not changed for any of the > > > other 7 target fabric modules we've merged into mainline since then. > > > > > > > > Also, tcm_vhost has been marked as Experimental following virtio-scsi. > > > > > I'd much rather leave it at Experimental until we merge upstream > > > > > userspace support. If userspace support never ends up materializing, > > > > > I'm fine with dropping it all together. > > > > > > > > Once it's in kernel you never know who will use this driver. > > > > Experimental does not mean driver can be dropped, staging does. > > > > > > > > > > Yes, that's the point of being in mainline. People using the code, > > > right..? > > > > Exactly. I am just worried about in the end being no major users and we > > are being stuck with a niche driver that as a result is very hard to test. > > And the reason for the fear is the initial negative reaction from the > > qemu side. And no if it's there we can't just drop it. > > > > That is certainly a reasonable concern.. > > > > > > However at this point given that there is a 3x performance gap between > > > > > virtio-scsi-raw + virtio-scsi+tcm_vhost for random mixed small block > > > > > I/O, and we still need the latter to do proper SCSI CDB passthrough for > > > > > non TYPE_DISK devices I'm hoping that we can agree on userspace bits > > > > > once tcm_vhost is merged. > > > > > > > > > > --nab > > > > > > > > I do think upstream kernel would help you nail userspace issues too > > > > but at this point it looks like either staging meterial or 3.6 is too > > > > early. > > > > > > > > > > I think for-3.6 is just the right time for this kernel code. Seriously, > > > The basic ABI fabric layout for /sys/kernel/config/target/vhost/ is > > > going to be the same now for-3.6, the same for-3.7, and the same for .38 > > > code. > > > > > > I'd be happy to move tcm_vhost back to drivers/target/ for now, and we > > > move it to drivers/vhost/ once the userspace bits are worked out..? > > > > > > Would that be a reasonable compromise to move forward..? > > > > > > --nab > > > > I don't see how it helps. The driver is either a guaranteed ABI or not. > > I'd prefer not to have vhost users outside drivers/vhost/ since it is > > harder for me to keep track of them. > > > > What's the problem with staging proposal? It's just another hoop to jump > > through to enable it? > > > > Yeah, I'm OK with just adding a CONFIG_STAGING tag is a reasonable step > forward for-3.6. > > Adding the following patch into target-pending/for-next-merge now: > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig > index ccbeb6f..2cd7135 100644 > --- a/drivers/vhost/Kconfig > +++ b/drivers/vhost/Kconfig > @@ -11,7 +11,7 @@ config VHOST_NET > > config TCM_VHOST > tristate "TCM_VHOST fabric module (EXPERIMENTAL)" > - depends on TARGET_CORE && EVENTFD && EXPERIMENTAL && m > + depends on TARGET_CORE && EVENTFD && EXPERIMENTAL && STAGING && m > default n > ---help--- > Say M here to enable the TCM_VHOST fabric module for use with virtio-scsi guests > > Hmm that's not explicit enough, someone might enable CONFIG_STAGING for some other reason and won't notice the dependency. We need it to appear with other staging drivers in the menu, so there needs to be a Kconfig that is included from drivers/staging/Kconfig. For example, we can create drivers/vhost/staging/Kconfig or drivers/vhost/tcm/Kconfig and include it from drivers/staging/Kconfig. nouveau did something like this for a while, see f3c93cbde7eab38671ae085cb1027b08f5f36757. No need to move the rest of the code.
On Wed, 2012-07-18 at 02:11 +0300, Michael S. Tsirkin wrote: > On Tue, Jul 17, 2012 at 03:37:20PM -0700, Nicholas A. Bellinger wrote: > > On Wed, 2012-07-18 at 01:18 +0300, Michael S. Tsirkin wrote: > > > On Tue, Jul 17, 2012 at 03:02:08PM -0700, Nicholas A. Bellinger wrote: > > > > On Wed, 2012-07-18 at 00:34 +0300, Michael S. Tsirkin wrote: <SNIP> > > > > > > I don't see how it helps. The driver is either a guaranteed ABI or not. > > > I'd prefer not to have vhost users outside drivers/vhost/ since it is > > > harder for me to keep track of them. > > > > > > What's the problem with staging proposal? It's just another hoop to jump > > > through to enable it? > > > > > > > Yeah, I'm OK with just adding a CONFIG_STAGING tag is a reasonable step > > forward for-3.6. > > > > Adding the following patch into target-pending/for-next-merge now: > > > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig > > index ccbeb6f..2cd7135 100644 > > --- a/drivers/vhost/Kconfig > > +++ b/drivers/vhost/Kconfig > > @@ -11,7 +11,7 @@ config VHOST_NET > > > > config TCM_VHOST > > tristate "TCM_VHOST fabric module (EXPERIMENTAL)" > > - depends on TARGET_CORE && EVENTFD && EXPERIMENTAL && m > > + depends on TARGET_CORE && EVENTFD && EXPERIMENTAL && STAGING && m > > default n > > ---help--- > > Say M here to enable the TCM_VHOST fabric module for use with virtio-scsi guests > > > > > > Hmm that's not explicit enough, someone might enable CONFIG_STAGING for > some other reason and won't notice the dependency. > We need it to appear with other staging drivers in the menu, > so there needs to be a Kconfig that is included from > drivers/staging/Kconfig. > Mmmmm, I am sensing a linux-next merge conflict with staging-next and/or another for-next-merge rebase coming on.. > For example, we can create > drivers/vhost/staging/Kconfig or drivers/vhost/tcm/Kconfig and include > it from drivers/staging/Kconfig. nouveau did something like this for a > while, see f3c93cbde7eab38671ae085cb1027b08f5f36757. > > No need to move the rest of the code. > OK, lets use drivers/vhost/tcm/Kconfig and I'll post a incremental patch to make it appear under staging it shortly. (CC'ing Greg-KH for good measure.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig index ccbeb6f..2cd7135 100644 --- a/drivers/vhost/Kconfig +++ b/drivers/vhost/Kconfig @@ -11,7 +11,7 @@ config VHOST_NET config TCM_VHOST tristate "TCM_VHOST fabric module (EXPERIMENTAL)" - depends on TARGET_CORE && EVENTFD && EXPERIMENTAL && m + depends on TARGET_CORE && EVENTFD && EXPERIMENTAL && STAGING && m default n ---help--- Say M here to enable the TCM_VHOST fabric module for use with virtio-scsi guests