* RE: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Shreyas Bhatewara @ 2010-05-06 7:25 UTC (permalink / raw)
To: Scott Feldman, Arnd Bergmann, Dmitry Torokhov
Cc: Pankaj Thakkar, pv-drivers@vmware.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Christoph Hellwig
In-Reply-To: <C807719A.2E910%scofeldm@cisco.com>
> -----Original Message-----
> From: Scott Feldman [mailto:scofeldm@cisco.com]
> Sent: Wednesday, May 05, 2010 7:04 PM
> To: Shreyas Bhatewara; Arnd Bergmann; Dmitry Torokhov
> Cc: Christoph Hellwig; pv-drivers@vmware.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; virtualization@lists.linux-
> foundation.org; Pankaj Thakkar
> Subject: Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for
> vmxnet3
>
> On 5/5/10 10:29 AM, "Dmitry Torokhov" <dtor@vmware.com> wrote:
>
> > It would not be a binary blob but software properly released under
> GPL.
> > The current plan is for the shell to enforce GPL requirement on the
> > plugin code, similar to what module loaded does for regular kernel
> > modules.
>
> On 5/5/10 3:05 PM, "Shreyas Bhatewara" <sbhatewara@vmware.com> wrote:
>
> > The plugin image is not linked against Linux kernel. It is OS
> agnostic infact
> > (Eg. same plugin works for Linux and Windows VMs)
>
> Are there any issues with injecting the GPL-licensed plug-in into the
> Windows vmxnet3 NDIS driver?
>
> -scott
Scott,
Thanks for pointing out. This issue can be resolved by adding exception to the plugin license which allows it to link to a non-free program .(http://www.gnu.org/licenses/gpl-faq.html#GPLPluginsInNF)
->Shreyas
^ permalink raw reply
* Re: virtio: put last_used and last_avail index into ring itself.
From: Michael S. Tsirkin @ 2010-05-06 6:27 UTC (permalink / raw)
To: Rusty Russell
Cc: Eric Dumazet, kvm, netdev, linux-kernel, virtualization, linux-mm,
s.hetze, hpa, Daniel Walker, mingo, akpm
In-Reply-To: <201005061022.13815.rusty@rustcorp.com.au>
On Thu, May 06, 2010 at 10:22:12AM +0930, Rusty Russell wrote:
> On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote:
> > > virtio: put last_used and last_avail index into ring itself.
> > >
> > > Generally, the other end of the virtio ring doesn't need to see where
> > > you're up to in consuming the ring. However, to completely understand
> > > what's going on from the outside, this information must be exposed.
> > > For example, if you want to save and restore a virtio_ring, but you're
> > > not the consumer because the kernel is using it directly.
> > >
> > > Fortunately, we have room to expand: the ring is always a whole number
> > > of pages and there's hundreds of bytes of padding after the avail ring
> > > and the used ring, whatever the number of descriptors (which must be a
> > > power of 2).
> > >
> > > We add a feature bit so the guest can tell the host that it's writing
> > > out the current value there, if it wants to use that.
> > >
> > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >
> > I've been looking at this patch some more (more on why
> > later), and I wonder: would it be better to add some
> > alignment to the last used index address, so that
> > if we later add more stuff at the tail, it all
> > fits in a single cache line?
>
> In theory, but not in practice. We don't have many rings, so the
> difference between 1 and 2 cache lines is not very much.
Fair enough.
> > We use a new feature bit anyway, so layout change should not be
> > a problem.
> >
> > Since I raised the question of caches: for used ring,
> > the ring is not aligned to 64 bit, so on CPUs with 64 bit
> > or larger cache lines, used entries will often cross
> > cache line boundaries. Am I right and might it
> > have been better to align ring entries to cache line boundaries?
> >
> > What do you think?
>
> I think everyone is settled on 128 byte cache lines for the forseeable
> future, so it's not really an issue.
>
> Cheers,
> Rusty.
You mean with 64 bit descriptors we will be bouncing a cache line
between host and guest, anyway?
--
MST
^ permalink raw reply
* Re: [PATCH RFC] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-06 6:19 UTC (permalink / raw)
To: Rusty Russell; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <201005061201.35223.rusty@rustcorp.com.au>
On Thu, May 06, 2010 at 12:01:34PM +0930, Rusty Russell wrote:
> On Thu, 6 May 2010 06:28:14 am Michael S. Tsirkin wrote:
> > Rusty,
> > this is a simplified form of a patch you posted in the past.
> > I have a vhost patch that, using this feature, shows external
> > to host bandwidth grow from 5 to 7 GB/s, by avoiding
> > an interrupt in the window after previous interrupt
> > was sent and before interrupts were disabled for the vq.
> > With vhost under some external to host loads I see
> > this window being hit about 30% sometimes.
>
> Fascinating. So you use this to guess if the guest is still processing?
Exactly.
> I haven't thought about it hard, but is that racy?
I thought about this really hard and I don't think it's
necessarily racy, as long as (pseudo code):
guest:
start:
disable interrupts
read(used)
write(last used)
enable interrupts
mb()
if read(used)
goto start
host:
write used
mb()
if (reached(read(last used), used))
interrupt
IOW, guest does read then write then read, host does write then read.
Now, the only way to miss an interrupt is if we read last used
value before it is written so we think guest is still processing.
But if that happens, this means that host has written used before
guest updated last used, and for this reason peek will see
used value and restart polling.
IOW, to miss an interrupt host must read a stale value.
For this host must read before guest write, then
host write already happened, so second read in
guest will see an updated value host has written.
Now, I also added an mb() in guest between read and write so
that last used index write can not get ahead of used index read.
It does feel good to have it there, but I can not say why
it's helpful. Works fine without it, but then these
subtle races might be hard to trigger. What do you think?
> Obviously happy to apply this when you finalize it.
>
> Thanks!
> Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Re: [PATCH] virtio-spec: document block CMD and FLUSH
From: Rusty Russell @ 2010-05-06 6:05 UTC (permalink / raw)
To: Neil Brown
Cc: tytso, kvm, Michael S. Tsirkin, Jamie Lokier, qemu-devel,
virtualization, Jens Axboe, hch
In-Reply-To: <20100505160343.264fd015@notabene.brown>
On Wed, 5 May 2010 03:33:43 pm Neil Brown wrote:
> On Wed, 5 May 2010 14:28:41 +0930
> Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> > On Wed, 5 May 2010 05:47:05 am Jamie Lokier wrote:
> > > Jens Axboe wrote:
> > > > On Tue, May 04 2010, Rusty Russell wrote:
> > > > > ISTR someone mentioning a desire for such an API years ago, so CC'ing the
> > > > > usual I/O suspects...
> > > >
> > > > It would be nice to have a more fuller API for this, but the reality is
> > > > that only the flush approach is really workable. Even just strict
> > > > ordering of requests could only be supported on SCSI, and even there the
> > > > kernel still lacks proper guarantees on error handling to prevent
> > > > reordering there.
> > >
> > > There's a few I/O scheduling differences that might be useful:
> > >
> > > 1. The I/O scheduler could freely move WRITEs before a FLUSH but not
> > > before a BARRIER. That might be useful for time-critical WRITEs,
> > > and those issued by high I/O priority.
> >
> > This is only because noone actually wants flushes or barriers, though
> > I/O people seem to only offer that. We really want "<these writes> must
> > occur before <this write>". That offers maximum choice to the I/O subsystem
> > and potentially to smart (virtual?) disks.
> >
> > > 2. The I/O scheduler could move WRITEs after a FLUSH if the FLUSH is
> > > only for data belonging to a particular file (e.g. fdatasync with
> > > no file size change, even on btrfs if O_DIRECT was used for the
> > > writes being committed). That would entail tagging FLUSHes and
> > > WRITEs with a fs-specific identifier (such as inode number), opaque
> > > to the scheduler which only checks equality.
> >
> > This is closer. In userspace I'd be happy with a "all prior writes to this
> > struct file before all future writes". Even if the original guarantees were
> > stronger (ie. inode basis). We currently implement transactions using 4 fsync
> > /msync pairs.
> >
> > write_recovery_data(fd);
> > fsync(fd);
> > msync(mmap);
> > write_recovery_header(fd);
> > fsync(fd);
> > msync(mmap);
> > overwrite_with_new_data(fd);
> > fsync(fd);
> > msync(mmap);
> > remove_recovery_header(fd);
> > fsync(fd);
> > msync(mmap);
>
> Seems over-zealous.
> If the recovery_header held a strong checksum of the recovery_data you would
> not need the first fsync, and as long as you have two places to write recovery
> data, you don't need the 3rd and 4th syncs.
> Just:
> write_internally_checksummed_recovery_data_and_header_to_unused_log_space()
> fsync / msync
> overwrite_with_new_data()
>
> To recovery you choose the most recent log_space and replay the content.
> That may be a redundant operation, but that is no loss.
I think you missed a checksum for the new data? Otherwise we can't tell if
the new data is completely written. But yes, I will steal this scheme for
TDB2, thanks!
In practice, it's the first sync which is glacial, the rest are pretty cheap.
> Also cannot see the point of msync if you have already performed an fsync,
> and if there is a point, I would expect you to call msync before
> fsync... Maybe there is some subtlety there that I am not aware of.
I assume it's this from the msync man page:
msync() flushes changes made to the in-core copy of a file that was
mapped into memory using mmap(2) back to disk. Without use of this
call there is no guarantee that changes are written back before mun‐
map(2) is called.
> > It's an implementation detail; barrier has less flexibility because it has
> > less information about what is required. I'm saying I want to give you as
> > much information as I can, even if you don't use it yet.
>
> Only we know that approach doesn't work.
> People will learn that they don't need to give the extra information to still
> achieve the same result - just like they did with ext3 and fsync.
> Then when we improve the implementation to only provide the guarantees that
> you asked for, people will complain that they are getting empty files that
> they didn't expect.
I think that's an oversimplification: IIUC that occurred to people *not*
using fsync(). They weren't using it because it was too slow. Providing
a primitive which is as fast or faster and more specific doesn't have the
same magnitude of social issues.
And we can't write userspace interfaces for idiots only.
> The abstraction I would like to see is a simple 'barrier' that contains no
> data and has a filesystem-wide effect.
I think you lack ambition ;)
Thinking about the single-file use case (eg. kvm guest or tdb), isn't that
suboptimal for md? Since you have to hand your barrier to every device
whereas a file-wide primitive may theoretically only go to some.
Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC] virtio: put last seen used index into ring itself
From: Rusty Russell @ 2010-05-06 2:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <20100505205814.GA7090@redhat.com>
On Thu, 6 May 2010 06:28:14 am Michael S. Tsirkin wrote:
> Rusty,
> this is a simplified form of a patch you posted in the past.
> I have a vhost patch that, using this feature, shows external
> to host bandwidth grow from 5 to 7 GB/s, by avoiding
> an interrupt in the window after previous interrupt
> was sent and before interrupts were disabled for the vq.
> With vhost under some external to host loads I see
> this window being hit about 30% sometimes.
Fascinating. So you use this to guess if the guest is still processing?
I haven't thought about it hard, but is that racy?
Obviously happy to apply this when you finalize it.
Thanks!
Rusty.
^ permalink raw reply
* Re: [PATCH 0/3] virtio: console: Handle multiple console port resizes
From: Rusty Russell @ 2010-05-06 2:27 UTC (permalink / raw)
To: Amit Shah; +Cc: Virtualization List
In-Reply-To: <1273091709-19963-1-git-send-email-amit.shah@redhat.com>
On Thu, 6 May 2010 06:05:06 am Amit Shah wrote:
> Hello,
>
> This series adds resize support for multiple console ports. The size
> for each console is stored in its structure and the host informs the
> guest about size changes via the VIRTIO_CONSOLE_RESIZE control
> message.
Thanks, applied!
Rusty.
^ permalink raw reply
* Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Scott Feldman @ 2010-05-06 2:03 UTC (permalink / raw)
To: Shreyas Bhatewara, Arnd Bergmann, Dmitry Torokhov
Cc: Pankaj Thakkar, pv-drivers@vmware.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Christoph Hellwig
In-Reply-To: <89E2752CFA8EC044846EB8499819134102AF2896F5@EXCH-MBX-4.vmware.com>
On 5/5/10 10:29 AM, "Dmitry Torokhov" <dtor@vmware.com> wrote:
> It would not be a binary blob but software properly released under GPL.
> The current plan is for the shell to enforce GPL requirement on the
> plugin code, similar to what module loaded does for regular kernel
> modules.
On 5/5/10 3:05 PM, "Shreyas Bhatewara" <sbhatewara@vmware.com> wrote:
> The plugin image is not linked against Linux kernel. It is OS agnostic infact
> (Eg. same plugin works for Linux and Windows VMs)
Are there any issues with injecting the GPL-licensed plug-in into the
Windows vmxnet3 NDIS driver?
-scott
^ permalink raw reply
* RE: [PATCH 1/1] staging: hv: Add Time Sync feature to hv_utils module
From: Joe Perches @ 2010-05-06 1:59 UTC (permalink / raw)
To: Haiyang Zhang
Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
virtualization@lists.osdl.org, gregkh@suse.de, Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF72662A6F52@TK5EX14MBXC130.redmond.corp.microsoft.com>
On Thu, 2010-05-06 at 01:42 +0000, Haiyang Zhang wrote:
> > Why a maximum of 50 samples?
> After reboot the flag ICTIMESYNCFLAG_SYNC is included in the
> first time message after the timesync channel is opened. Since the
> hv_utils module is loaded after hv_vmbus, the first message is usually
> missed. The other thing is, systime is automatically set to emulated
> hardware clock which may not be UTC time or the same time
> zone. So, to override these effects, we use the first 50 time samples
> for initial system time setting.
I suggest putting that in a commit message or a code comment.
cheers, Joe
^ permalink raw reply
* RE: [PATCH 1/1] staging: hv: Add Time Sync feature to hv_utils module
From: Haiyang Zhang @ 2010-05-06 1:42 UTC (permalink / raw)
To: Joe Perches
Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
virtualization@lists.osdl.org, gregkh@suse.de, Hank Janssen
In-Reply-To: <1273107494.1735.87.camel@Joe-Laptop.home>
> Why a maximum of 50 samples?
After reboot the flag ICTIMESYNCFLAG_SYNC is included in the
first time message after the timesync channel is opened. Since the
hv_utils module is loaded after hv_vmbus, the first message is usually
missed. The other thing is, systime is automatically set to emulated
hardware clock which may not be UTC time or the same time
zone. So, to override these effects, we use the first 50 time samples
for initial system time setting.
> It might be better to do something like this
> so the ns_to_timespec isn't performe when unnecessary.
Thanks for the optimization, I will put it into the code.
- Haiyang
^ permalink raw reply
* Re: question on virtio
From: Rusty Russell @ 2010-05-06 1:32 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, qemu-devel, Anthony Liguori, virtualization
In-Reply-To: <20100505110947.GA27872@redhat.com>
On Wed, 5 May 2010 08:39:47 pm Michael S. Tsirkin wrote:
> Hi!
> I see this in virtio_ring.c:
>
> /* Put entry in available array (but don't update avail->idx *
> until they do sync). */
>
> Why is it done this way?
> It seems that updating the index straight away would be simpler, while
> this might allow the host to specilatively look up the buffer and handle
> it, without waiting for the kick.
I agree. From my TODO:
what if we actually expose in ->add_buf?
I don't *think* anyone adds buffers without being ready for them to be used,
so changing this should be safe.
Want to give it a try and report back?
Thanks!
Rusty.
^ permalink raw reply
* Re: [PATCH 1/1] staging: hv: Add Time Sync feature to hv_utils module
From: Joe Perches @ 2010-05-06 0:58 UTC (permalink / raw)
To: Haiyang Zhang
Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
virtualization@lists.osdl.org, gregkh@suse.de, Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF72662A6D3B@TK5EX14MBXC130.redmond.corp.microsoft.com>
On Wed, 2010-05-05 at 19:23 +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
>
> Subject: Add Time Sync feature to hv_utils module.
> The Time Sync feature synchronizes guest time to host UTC time after reboot,
> and restore from saved/paused state.
> +static void adj_guesttime(winfiletime_t hosttime, u8 flags)
> +{
> + s64 host_tns;
> + struct timespec host_ts;
> + static s32 scnt = 50;
Why a maximum of 50 samples?
> + host_tns = (hosttime - WLTIMEDELTA) * 100;
> + host_ts = ns_to_timespec(host_tns);
> +
> + if ((flags & ICTIMESYNCFLAG_SYNC) != 0) {
> + do_settimeofday(&host_ts);
> + return;
> + }
> +
> + if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 &&
> + scnt > 0) {
> + scnt--;
> + do_settimeofday(&host_ts);
> + }
It might be better to do something like this
so the ns_to_timespec isn't performe when unnecessary.
static void settimeofday(winfiletime_t hosttime)
{
s64 host_tns = (hosttime - WLTIMEDELTA) * 100;
struct timespec host_ts = ns_to_timespec(host_tns);
do_settimeofday(&host_ts);
}
static void adj_guesttime(winfiletime_t hosttime, u8 flags)
{
static s32 scnt = 50;
if ((flags & ICTIMESYNCFLAG_SYNC) != 0) {
settimeofday(hosttime);
return;
}
if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 && scnt > 0) {
scnt--;
settimeofday(hosttime);
}
}
^ permalink raw reply
* Re: virtio: put last_used and last_avail index into ring itself.
From: Rusty Russell @ 2010-05-06 0:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eric Dumazet, kvm, netdev, linux-kernel, virtualization, linux-mm,
s.hetze, hpa, Daniel Walker, mingo, akpm
In-Reply-To: <20100504182236.GA14141@redhat.com>
On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote:
> > virtio: put last_used and last_avail index into ring itself.
> >
> > Generally, the other end of the virtio ring doesn't need to see where
> > you're up to in consuming the ring. However, to completely understand
> > what's going on from the outside, this information must be exposed.
> > For example, if you want to save and restore a virtio_ring, but you're
> > not the consumer because the kernel is using it directly.
> >
> > Fortunately, we have room to expand: the ring is always a whole number
> > of pages and there's hundreds of bytes of padding after the avail ring
> > and the used ring, whatever the number of descriptors (which must be a
> > power of 2).
> >
> > We add a feature bit so the guest can tell the host that it's writing
> > out the current value there, if it wants to use that.
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> I've been looking at this patch some more (more on why
> later), and I wonder: would it be better to add some
> alignment to the last used index address, so that
> if we later add more stuff at the tail, it all
> fits in a single cache line?
In theory, but not in practice. We don't have many rings, so the
difference between 1 and 2 cache lines is not very much.
> We use a new feature bit anyway, so layout change should not be
> a problem.
>
> Since I raised the question of caches: for used ring,
> the ring is not aligned to 64 bit, so on CPUs with 64 bit
> or larger cache lines, used entries will often cross
> cache line boundaries. Am I right and might it
> have been better to align ring entries to cache line boundaries?
>
> What do you think?
I think everyone is settled on 128 byte cache lines for the forseeable
future, so it's not really an issue.
Cheers,
Rusty.
^ permalink raw reply
* RE: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Shreyas Bhatewara @ 2010-05-05 22:05 UTC (permalink / raw)
To: Arnd Bergmann, Dmitry Torokhov
Cc: Pankaj Thakkar, pv-drivers@vmware.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Christoph Hellwig
In-Reply-To: <201005052353.16544.arnd@arndb.de>
> -----Original Message-----
> From: pv-drivers-bounces@vmware.com [mailto:pv-drivers-
> bounces@vmware.com] On Behalf Of Arnd Bergmann
> Sent: Wednesday, May 05, 2010 2:53 PM
> To: Dmitry Torokhov
> Cc: Christoph Hellwig; pv-drivers@vmware.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; virtualization@lists.linux-
> foundation.org; Pankaj Thakkar
> Subject: Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for
> vmxnet3
>
> On Wednesday 05 May 2010 22:36:31 Dmitry Torokhov wrote:
> >
> > On Wednesday 05 May 2010 01:09:48 pm Arnd Bergmann wrote:
> > > > > If you have any interesting in developing this further, do:
> > > > >
> > > > > (1) move the limited VF drivers directly into the kernel tree,
> > > > > talk to them through a normal ops vector
> > > >
> > > > [PT] This assumes that all the VF drivers would always be
> available.
> > > > Also we have to support windows and our current design supports
> it
> > > > nicely in an OS agnostic manner.
> > >
> > > Your approach assumes that the plugin is always available, which
> has
> > > exactly the same implications.
> >
> > Since plugin[s] are carried by the host they are indeed always
> > available.
>
> But what makes you think that you can build code that can be linked
> into arbitrary future kernel versions? The kernel does not define any
> calling conventions that are stable across multiple versions or
> configurations. For example, you'd have to provide different binaries
> for each combination of
The plugin image is not linked against Linux kernel. It is OS agnostic infact (Eg. same plugin works for Linux and Windows VMs)
Plugin is built against the shell API interface. It is loaded by hypervisor in a set of pages provided by shell. Guest OS specific tasks (like allocation of pages for plugin to load) are handled by shell and this is the one which will be upstreamed in Linux kernel. Maintenance of shell is the same as for any other driver currently existing in Linux kernel.
->Shreyas
>
> - 32/64 bit code
> - gcc -mregparm=?
> - lockdep
> - tracepoints
> - stackcheck
> - NOMMU
> - highmem
> - whatever new gets merged
>
> If you build the plugins only for specific versions of "enterprise"
> Linux
> kernels, the code becomes really hard to debug and maintain.
> If you wrap everything in your own version of the existing interfaces,
> your
> code gets bloated to the point of being unmaintainable.
>
> So I have to correct myself: this is very different from assuming the
> driver is available in the guest, it's actually much worse.
>
> Arnd
> _______________________________________________
> Pv-drivers mailing list
> Pv-drivers@vmware.com
> http://mailman2.vmware.com/mailman/listinfo/pv-drivers
^ permalink raw reply
* Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Arnd Bergmann @ 2010-05-05 21:53 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Christoph Hellwig, pv-drivers@vmware.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Pankaj Thakkar
In-Reply-To: <201005051336.32712.dtor@vmware.com>
On Wednesday 05 May 2010 22:36:31 Dmitry Torokhov wrote:
>
> On Wednesday 05 May 2010 01:09:48 pm Arnd Bergmann wrote:
> > > > If you have any interesting in developing this further, do:
> > > >
> > > > (1) move the limited VF drivers directly into the kernel tree,
> > > > talk to them through a normal ops vector
> > >
> > > [PT] This assumes that all the VF drivers would always be available.
> > > Also we have to support windows and our current design supports it
> > > nicely in an OS agnostic manner.
> >
> > Your approach assumes that the plugin is always available, which has
> > exactly the same implications.
>
> Since plugin[s] are carried by the host they are indeed always
> available.
But what makes you think that you can build code that can be linked
into arbitrary future kernel versions? The kernel does not define any
calling conventions that are stable across multiple versions or
configurations. For example, you'd have to provide different binaries
for each combination of
- 32/64 bit code
- gcc -mregparm=?
- lockdep
- tracepoints
- stackcheck
- NOMMU
- highmem
- whatever new gets merged
If you build the plugins only for specific versions of "enterprise" Linux
kernels, the code becomes really hard to debug and maintain.
If you wrap everything in your own version of the existing interfaces, your
code gets bloated to the point of being unmaintainable.
So I have to correct myself: this is very different from assuming the
driver is available in the guest, it's actually much worse.
Arnd
^ permalink raw reply
* Re: [PATCH RFC] virtio: put last seen used index into ring itself
From: Dor Laor @ 2010-05-05 21:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <20100505205814.GA7090@redhat.com>
On 05/05/2010 11:58 PM, Michael S. Tsirkin wrote:
> Generally, the Host end of the virtio ring doesn't need to see where
> Guest is up to in consuming the ring. However, to completely understand
> what's going on from the outside, this information must be exposed.
> For example, host can reduce the number of interrupts by detecting
> that the guest is currently handling previous buffers.
>
> Fortunately, we have room to expand: the ring is always a whole number
> of pages and there's hundreds of bytes of padding after the avail ring
> and the used ring, whatever the number of descriptors (which must be a
> power of 2).
>
> We add a feature bit so the guest can tell the host that it's writing
> out the current value there, if it wants to use that.
>
> This is based on a patch by Rusty Russell, with the main difference
> being that we dedicate a feature bit to guest to tell the host it is
> writing the used index. This way we don't need to force host to publish
> the last available index until we have a use for it.
>
> Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> ---
>
> Rusty,
> this is a simplified form of a patch you posted in the past.
> I have a vhost patch that, using this feature, shows external
> to host bandwidth grow from 5 to 7 GB/s, by avoiding
You mean external to guest I guess.
We have a similar issue with virtio-blk - when using very fast
multi-spindle storage on the host side, there are too many irq injection
events. This patch should probably reduce them allot.
The principle exactly matches the Xen ring.
> an interrupt in the window after previous interrupt
> was sent and before interrupts were disabled for the vq.
> With vhost under some external to host loads I see
> this window being hit about 30% sometimes.
>
> I'm finalizing the host bits and plan to send
> the final version for inclusion when all's ready,
> but I'd like to hear comments meanwhile.
>
> drivers/virtio/virtio_ring.c | 28 +++++++++++++++++-----------
> include/linux/virtio_ring.h | 14 +++++++++++++-
> 2 files changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 1ca8890..7729aba 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -89,9 +89,6 @@ struct vring_virtqueue
> /* Number we've added since last sync. */
> unsigned int num_added;
>
> - /* Last used index we've seen. */
> - u16 last_used_idx;
> -
> /* How to notify other side. FIXME: commonalize hcalls! */
> void (*notify)(struct virtqueue *vq);
>
> @@ -285,12 +282,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
>
> static inline bool more_used(const struct vring_virtqueue *vq)
> {
> - return vq->last_used_idx != vq->vring.used->idx;
> + return *vq->vring.last_used_idx != vq->vring.used->idx;
> }
>
> void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> + struct vring_used_elem *u;
> void *ret;
> unsigned int i;
>
> @@ -307,12 +305,13 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> return NULL;
> }
>
> - /* Only get used array entries after they have been exposed by host. */
> - virtio_rmb();
> -
> - i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
> - *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
> + /* Only get used array entries after they have been exposed by host.
> + * Need mb(), not just rmb() because we write last_used_idx below. */
> + virtio_mb();
>
> + u =&vq->vring.used->ring[*vq->vring.last_used_idx % vq->vring.num];
> + i = u->id;
> + *len = u->len;
> if (unlikely(i>= vq->vring.num)) {
> BAD_RING(vq, "id %u out of range\n", i);
> return NULL;
> @@ -325,7 +324,8 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> /* detach_buf clears data, so grab it now. */
> ret = vq->data[i];
> detach_buf(vq, i);
> - vq->last_used_idx++;
> + (*vq->vring.last_used_idx)++;
> +
> END_USE(vq);
> return ret;
> }
> @@ -431,7 +431,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> vq->vq.name = name;
> vq->notify = notify;
> vq->broken = false;
> - vq->last_used_idx = 0;
> + *vq->vring.last_used_idx = 0;
> vq->num_added = 0;
> list_add_tail(&vq->vq.list,&vdev->vqs);
> #ifdef DEBUG
> @@ -440,6 +440,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
>
> vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
>
> + /* We publish used index whether Host offers it or not: if not, it's
> + * junk space anyway. But calling this acknowledges the feature. */
> + virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED);
> +
> /* No callback? Tell other side not to bother us. */
> if (!callback)
> vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> @@ -473,6 +477,8 @@ void vring_transport_features(struct virtio_device *vdev)
> switch (i) {
> case VIRTIO_RING_F_INDIRECT_DESC:
> break;
> + case VIRTIO_RING_F_PUBLISH_INDICES:
> + break;
> default:
> /* We don't understand this bit. */
> clear_bit(i, vdev->features);
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index e4d144b..9d01de9 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -29,6 +29,9 @@
> /* We support indirect buffer descriptors */
> #define VIRTIO_RING_F_INDIRECT_DESC 28
>
> +/* The Guest publishes last-seen used index at the end of the avail ring. */
> +#define VIRTIO_RING_F_PUBLISH_USED 29
> +
> /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
> struct vring_desc {
> /* Address (guest-physical). */
> @@ -69,6 +72,8 @@ struct vring {
> struct vring_avail *avail;
>
> struct vring_used *used;
> + /* Last used index seen by the Guest. */
> + __u16 *last_used_idx;
> };
>
> /* The standard layout for the ring is a continuous chunk of memory which looks
> @@ -83,6 +88,7 @@ struct vring {
> * __u16 avail_flags;
> * __u16 avail_idx;
> * __u16 available[num];
> + * __u16 last_used_idx;
> *
> * // Padding to the next align boundary.
> * char pad[];
> @@ -101,11 +107,17 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p,
> vr->avail = p + num*sizeof(struct vring_desc);
> vr->used = (void *)(((unsigned long)&vr->avail->ring[num] + align-1)
> & ~(align - 1));
> + /* We publish the last-seen used index at the end of the available ring.
> + * It is at the end for backwards compatibility. */
> + vr->last_used_idx =&(vr)->avail->ring[num];
> + /* Verify that last used index does not spill over the used ring. */
> + BUG_ON((void *)vr->last_used_idx +
> + sizeof *vr->last_used_idx> (void *)vr->used);
> }
>
> static inline unsigned vring_size(unsigned int num, unsigned long align)
> {
> - return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
> + return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)
> + align - 1)& ~(align - 1))
> + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
> }
^ permalink raw reply
* Re: question on virtio
From: Michael S. Tsirkin @ 2010-05-05 20:59 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, kvm, virtualization
In-Reply-To: <4BE1C99F.6050205@codemonkey.ws>
On Wed, May 05, 2010 at 02:40:15PM -0500, Anthony Liguori wrote:
> On 05/05/2010 06:09 AM, Michael S. Tsirkin wrote:
>> Hi!
>> I see this in virtio_ring.c:
>>
>> /* Put entry in available array (but don't update avail->idx *
>> until they do sync). */
>>
>> Why is it done this way?
>> It seems that updating the index straight away would be simpler, while
>> this might allow the host to specilatively look up the buffer and handle
>> it, without waiting for the kick.
>>
>
> It should be okay as long as you don't update idx for partial vectors.
>
> Regards,
>
> Anthony Liguori
Sorry, what do you mean by partial vectors here?
^ permalink raw reply
* [PATCH RFC] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-05 20:58 UTC (permalink / raw)
To: Rusty Russell, linux-kernel, virtualization, kvm, qemu-devel
Generally, the Host end of the virtio ring doesn't need to see where
Guest is up to in consuming the ring. However, to completely understand
what's going on from the outside, this information must be exposed.
For example, host can reduce the number of interrupts by detecting
that the guest is currently handling previous buffers.
Fortunately, we have room to expand: the ring is always a whole number
of pages and there's hundreds of bytes of padding after the avail ring
and the used ring, whatever the number of descriptors (which must be a
power of 2).
We add a feature bit so the guest can tell the host that it's writing
out the current value there, if it wants to use that.
This is based on a patch by Rusty Russell, with the main difference
being that we dedicate a feature bit to guest to tell the host it is
writing the used index. This way we don't need to force host to publish
the last available index until we have a use for it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Rusty,
this is a simplified form of a patch you posted in the past.
I have a vhost patch that, using this feature, shows external
to host bandwidth grow from 5 to 7 GB/s, by avoiding
an interrupt in the window after previous interrupt
was sent and before interrupts were disabled for the vq.
With vhost under some external to host loads I see
this window being hit about 30% sometimes.
I'm finalizing the host bits and plan to send
the final version for inclusion when all's ready,
but I'd like to hear comments meanwhile.
drivers/virtio/virtio_ring.c | 28 +++++++++++++++++-----------
include/linux/virtio_ring.h | 14 +++++++++++++-
2 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 1ca8890..7729aba 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -89,9 +89,6 @@ struct vring_virtqueue
/* Number we've added since last sync. */
unsigned int num_added;
- /* Last used index we've seen. */
- u16 last_used_idx;
-
/* How to notify other side. FIXME: commonalize hcalls! */
void (*notify)(struct virtqueue *vq);
@@ -285,12 +282,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
static inline bool more_used(const struct vring_virtqueue *vq)
{
- return vq->last_used_idx != vq->vring.used->idx;
+ return *vq->vring.last_used_idx != vq->vring.used->idx;
}
void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ struct vring_used_elem *u;
void *ret;
unsigned int i;
@@ -307,12 +305,13 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
return NULL;
}
- /* Only get used array entries after they have been exposed by host. */
- virtio_rmb();
-
- i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
- *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+ /* Only get used array entries after they have been exposed by host.
+ * Need mb(), not just rmb() because we write last_used_idx below. */
+ virtio_mb();
+ u = &vq->vring.used->ring[*vq->vring.last_used_idx % vq->vring.num];
+ i = u->id;
+ *len = u->len;
if (unlikely(i >= vq->vring.num)) {
BAD_RING(vq, "id %u out of range\n", i);
return NULL;
@@ -325,7 +324,8 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
/* detach_buf clears data, so grab it now. */
ret = vq->data[i];
detach_buf(vq, i);
- vq->last_used_idx++;
+ (*vq->vring.last_used_idx)++;
+
END_USE(vq);
return ret;
}
@@ -431,7 +431,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
vq->vq.name = name;
vq->notify = notify;
vq->broken = false;
- vq->last_used_idx = 0;
+ *vq->vring.last_used_idx = 0;
vq->num_added = 0;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
@@ -440,6 +440,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+ /* We publish used index whether Host offers it or not: if not, it's
+ * junk space anyway. But calling this acknowledges the feature. */
+ virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED);
+
/* No callback? Tell other side not to bother us. */
if (!callback)
vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
@@ -473,6 +477,8 @@ void vring_transport_features(struct virtio_device *vdev)
switch (i) {
case VIRTIO_RING_F_INDIRECT_DESC:
break;
+ case VIRTIO_RING_F_PUBLISH_INDICES:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev->features);
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index e4d144b..9d01de9 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -29,6 +29,9 @@
/* We support indirect buffer descriptors */
#define VIRTIO_RING_F_INDIRECT_DESC 28
+/* The Guest publishes last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_USED 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
struct vring_desc {
/* Address (guest-physical). */
@@ -69,6 +72,8 @@ struct vring {
struct vring_avail *avail;
struct vring_used *used;
+ /* Last used index seen by the Guest. */
+ __u16 *last_used_idx;
};
/* The standard layout for the ring is a continuous chunk of memory which looks
@@ -83,6 +88,7 @@ struct vring {
* __u16 avail_flags;
* __u16 avail_idx;
* __u16 available[num];
+ * __u16 last_used_idx;
*
* // Padding to the next align boundary.
* char pad[];
@@ -101,11 +107,17 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p,
vr->avail = p + num*sizeof(struct vring_desc);
vr->used = (void *)(((unsigned long)&vr->avail->ring[num] + align-1)
& ~(align - 1));
+ /* We publish the last-seen used index at the end of the available ring.
+ * It is at the end for backwards compatibility. */
+ vr->last_used_idx = &(vr)->avail->ring[num];
+ /* Verify that last used index does not spill over the used ring. */
+ BUG_ON((void *)vr->last_used_idx +
+ sizeof *vr->last_used_idx > (void *)vr->used);
}
static inline unsigned vring_size(unsigned int num, unsigned long align)
{
- return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
+ return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)
+ align - 1) & ~(align - 1))
+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
}
--
1.7.1.12.g42b7f
^ permalink raw reply related
* Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Dmitry Torokhov @ 2010-05-05 20:36 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christoph Hellwig, pv-drivers@vmware.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Pankaj Thakkar
In-Reply-To: <201005052209.48465.arnd@arndb.de>
On Wednesday 05 May 2010 01:09:48 pm Arnd Bergmann wrote:
> > > If you have any interesting in developing this further, do:
> > >
> > > (1) move the limited VF drivers directly into the kernel tree,
> > > talk to them through a normal ops vector
> >
> > [PT] This assumes that all the VF drivers would always be available.
> > Also we have to support windows and our current design supports it
> > nicely in an OS agnostic manner.
>
> Your approach assumes that the plugin is always available, which has
> exactly the same implications.
Since plugin[s] are carried by the host they are indeed always
available.
--
Dmitry
^ permalink raw reply
* [PATCH 3/3] virtio: console: Accept console size along with resize control message
From: Amit Shah @ 2010-05-05 20:35 UTC (permalink / raw)
To: Rusty Russell
Cc: Amit Shah, Christian Borntraeger, Kusanagi Kouichi, linuxppc-dev,
Virtualization List
In-Reply-To: <1273091709-19963-3-git-send-email-amit.shah@redhat.com>
The VIRTIO_CONSOLE_RESIZE control message sent to us by the host now
contains the new {rows, cols} values for the console. This ensures each
console port gets its own size, and we don't depend on the config-space
rows and cols values at all now.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
CC: linuxppc-dev@ozlabs.org
CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
---
drivers/char/virtio_console.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index ccfe68a..5cab839 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1194,12 +1194,23 @@ static void handle_control_message(struct ports_device *portdev,
* have to notify the host first.
*/
break;
- case VIRTIO_CONSOLE_RESIZE:
+ case VIRTIO_CONSOLE_RESIZE: {
+ struct {
+ __u16 rows;
+ __u16 cols;
+ } size;
+
if (!is_console_port(port))
break;
+
+ memcpy(&size, buf->buf + buf->offset + sizeof(*cpkt),
+ sizeof(size));
+ set_console_size(port, size.rows, size.cols);
+
port->cons.hvc->irq_requested = 1;
resize_console(port);
break;
+ }
case VIRTIO_CONSOLE_PORT_OPEN:
port->host_connected = cpkt->value;
wake_up_interruptible(&port->waitqueue);
--
1.6.2.5
^ permalink raw reply related
* [PATCH 2/3] virtio: console: Store each console's size in the console structure
From: Amit Shah @ 2010-05-05 20:35 UTC (permalink / raw)
To: Rusty Russell
Cc: Amit Shah, Christian Borntraeger, Kusanagi Kouichi, linuxppc-dev,
Virtualization List
In-Reply-To: <1273091709-19963-2-git-send-email-amit.shah@redhat.com>
With support for multiple consoles, just using one {rows,cols} pair in
the config space is not going to suffice.
Store each console's size as part of the console struct.
This changes the behaviour for one case when multiport is not enabled:
when notifier_add_vio() is called, the console size is taken from that
of the last config-space update instead of fetching it afresh from the
config space.
Also add a helper to update the size in the console struct as we'll need
to use the same code to update the size via control messages when
multiport support is enabled.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
CC: linuxppc-dev@ozlabs.org
CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
---
drivers/char/virtio_console.c | 43 +++++++++++++++++++++++++++++-----------
1 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e2d05ea..ccfe68a 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -78,6 +78,9 @@ struct console {
/* The hvc device associated with this console port */
struct hvc_struct *hvc;
+ /* The size of the console */
+ struct winsize ws;
+
/*
* This number identifies the number that we used to register
* with hvc in hvc_instantiate() and hvc_alloc(); this is the
@@ -773,22 +776,14 @@ static int get_chars(u32 vtermno, char *buf, int count)
static void resize_console(struct port *port)
{
struct virtio_device *vdev;
- struct winsize ws;
/* The port could have been hot-unplugged */
- if (!port)
+ if (!port || !is_console_port(port))
return;
vdev = port->portdev->vdev;
- if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) {
- vdev->config->get(vdev,
- offsetof(struct virtio_console_config, cols),
- &ws.ws_col, sizeof(u16));
- vdev->config->get(vdev,
- offsetof(struct virtio_console_config, rows),
- &ws.ws_row, sizeof(u16));
- hvc_resize(port->cons.hvc, ws);
- }
+ if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+ hvc_resize(port->cons.hvc, port->cons.ws);
}
/* We set the configuration at this point, since we now have a tty */
@@ -952,6 +947,15 @@ static const struct file_operations port_debugfs_ops = {
.read = debugfs_read,
};
+static void set_console_size(struct port *port, u16 rows, u16 cols)
+{
+ if (!port || !is_console_port(port))
+ return;
+
+ port->cons.ws.ws_row = rows;
+ port->cons.ws.ws_col = cols;
+}
+
static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
{
struct port_buffer *buf;
@@ -1000,6 +1004,8 @@ static int add_port(struct ports_device *portdev, u32 id)
port->inbuf = NULL;
port->cons.hvc = NULL;
+ port->cons.ws.ws_row = port->cons.ws.ws_col = 0;
+
port->host_connected = port->guest_connected = false;
port->outvq_full = false;
@@ -1320,6 +1326,19 @@ static void config_intr(struct virtio_device *vdev)
portdev = vdev->priv;
if (!use_multiport(portdev)) {
+ struct port *port;
+ u16 rows, cols;
+
+ vdev->config->get(vdev,
+ offsetof(struct virtio_console_config, cols),
+ &cols, sizeof(u16));
+ vdev->config->get(vdev,
+ offsetof(struct virtio_console_config, rows),
+ &rows, sizeof(u16));
+
+ port = find_port_by_id(portdev, 0);
+ set_console_size(port, rows, cols);
+
/*
* We'll use this way of resizing only for legacy
* support. For newer userspace
@@ -1327,7 +1346,7 @@ static void config_intr(struct virtio_device *vdev)
* to indicate console size changes so that it can be
* done per-port.
*/
- resize_console(find_port_by_id(portdev, 0));
+ resize_console(port);
}
}
--
1.6.2.5
^ permalink raw reply related
* [PATCH 1/3] virtio: console: Resize console port 0 on config intr only if multiport is off
From: Amit Shah @ 2010-05-05 20:35 UTC (permalink / raw)
To: Rusty Russell
Cc: Amit Shah, Christian Borntraeger, Kusanagi Kouichi, linuxppc-dev,
Virtualization List
In-Reply-To: <1273091709-19963-1-git-send-email-amit.shah@redhat.com>
When using multiport, we'll use control messages. Ensure we don't
accidentally update port 0 size on config interrupts.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
CC: linuxppc-dev@ozlabs.org
CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
---
drivers/char/virtio_console.c | 17 ++++++++++-------
1 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index a64558f..e2d05ea 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1319,13 +1319,16 @@ static void config_intr(struct virtio_device *vdev)
portdev = vdev->priv;
- /*
- * We'll use this way of resizing only for legacy support.
- * For newer userspace (VIRTIO_CONSOLE_F_MULTPORT+), use
- * control messages to indicate console size changes so that
- * it can be done per-port
- */
- resize_console(find_port_by_id(portdev, 0));
+ if (!use_multiport(portdev)) {
+ /*
+ * We'll use this way of resizing only for legacy
+ * support. For newer userspace
+ * (VIRTIO_CONSOLE_F_MULTPORT+), use control messages
+ * to indicate console size changes so that it can be
+ * done per-port.
+ */
+ resize_console(find_port_by_id(portdev, 0));
+ }
}
static int init_vqs(struct ports_device *portdev)
--
1.6.2.5
^ permalink raw reply related
* [PATCH 0/3] virtio: console: Handle multiple console port resizes
From: Amit Shah @ 2010-05-05 20:35 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, Virtualization List
Hello,
This series adds resize support for multiple console ports. The size
for each console is stored in its structure and the host informs the
guest about size changes via the VIRTIO_CONSOLE_RESIZE control
message.
This is easily tested by the qemu patches provided by Kusanagi Kouichi
with some updates, which I will shortly send to the qemu-devel list.
Please apply,
Amit.
Amit Shah (3):
virtio: console: Resize console port 0 on config intr only if
multiport is off
virtio: console: Store each console's size in the console structure
virtio: console: Accept console size along with resize control
message
drivers/char/virtio_console.c | 71 ++++++++++++++++++++++++++++++-----------
1 files changed, 52 insertions(+), 19 deletions(-)
^ permalink raw reply
* Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Arnd Bergmann @ 2010-05-05 20:09 UTC (permalink / raw)
To: virtualization
Cc: Christoph Hellwig, Dmitry Torokhov, pv-drivers@vmware.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Pankaj Thakkar
In-Reply-To: <F1354E79A137A24CBA60059AA65CB1B802A235C535@EXCH-MBX-2.vmware.com>
On Wednesday 05 May 2010 19:47:10 Pankaj Thakkar wrote:
> >
> > Forget about the licensing. Loading binary blobs written to a shim
> > layer is a complete pain in the ass and totally unsupportable, and
> > also uninteresting because of the overhead.
>
> [PT] Why do you think it is unsupportable? How different is it from any module
> written against a well maintained interface? What overhead are you talking about?
We have the right number of module loaders in the kernel: one. If you
add another one, you're doubling the amount of code that anyone
working on that code needs to know about.
> > If you have any interesting in developing this further, do:
> >
> > (1) move the limited VF drivers directly into the kernel tree,
> > talk to them through a normal ops vector
> [PT] This assumes that all the VF drivers would always be available.
> Also we have to support windows and our current design supports it
> nicely in an OS agnostic manner.
Your approach assumes that the plugin is always available, which has
exactly the same implications.
> > (2) get rid of the whole shim crap and instead integrate the limited
> > VF driver with the full VF driver we already have, instead of
> > duplicating the code
> [PT] Having a full VF driver adds a lot of dependency on the guest VM
> and this is what NPA tries to avoid.
If you have the limited driver for some hardware that does not have
the real thing, we could still ship just that. I would however guess
that most vendors are interested in not just running in vmware but
also other hypervisors that still require the full driver, so that
case would be rare, especially in the long run.
Arnd
^ permalink raw reply
* Re: RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Pankaj Thakkar @ 2010-05-05 19:44 UTC (permalink / raw)
To: Avi Kivity
Cc: pv-drivers@vmware.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <4BE1B217.80600@redhat.com>
On Wed, May 05, 2010 at 10:59:51AM -0700, Avi Kivity wrote:
> Date: Wed, 5 May 2010 10:59:51 -0700
> From: Avi Kivity <avi@redhat.com>
> To: Pankaj Thakkar <pthakkar@vmware.com>
> CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
> "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
> "virtualization@lists.linux-foundation.org"
> <virtualization@lists.linux-foundation.org>,
> "pv-drivers@vmware.com" <pv-drivers@vmware.com>,
> Shreyas Bhatewara <sbhatewara@vmware.com>
> Subject: Re: RFC: Network Plugin Architecture (NPA) for vmxnet3
>
> On 05/05/2010 02:02 AM, Pankaj Thakkar wrote:
> > 2. Hypervisor control: All control operations from the guest such as programming
> > MAC address go through the hypervisor layer and hence can be subjected to
> > hypervisor policies. The PF driver can be further used to put policy decisions
> > like which VLAN the guest should be on.
> >
>
> Is this enforced? Since you pass the hardware through, you can't rely
> on the guest actually doing this, yes?
We don't pass the whole VF to the guest. Only the BAR which is responsible for
TX/RX/intr is mapped into guest space. The interface between the shell and
plugin only allows to do operations related to TX and RX such as send a packet
to the VF, allocate RX buffers, indicate a packet upto the shell. All control
operations are handled by the shell and the shell does what the existing
vmxnet3 drivers does (touch a specific register and let the device emulation do
the work). When a VF is mapped to the guest the hypervisor knows this and
programs the h/w accordingly on behalf of the shell. So for example if the VM
does a MAC address change inside the guest, the shell would write to
VMXNET3_REG_MAC{L|H} registers which would trigger the device emulation to read
the new mac address and update its internal virtual port information for the
virtual switch and if the VF is mapped it would also program the embedded
switch RX filters to reflect the new mac address.
>
> > The plugin image is provided by the IHVs along with the PF driver and is
> > packaged in the hypervisor. The plugin image is OS agnostic and can be loaded
> > either into a Linux VM or a Windows VM. The plugin is written against the Shell
> > API interface which the shell is responsible for implementing. The API
> > interface allows the plugin to do TX and RX only by programming the hardware
> > rings (along with things like buffer allocation and basic initialization). The
> > virtual machine comes up in paravirtualized/emulated mode when it is booted.
> > The hypervisor allocates the VF and other resources and notifies the shell of
> > the availability of the VF. The hypervisor injects the plugin into memory
> > location specified by the shell. The shell initializes the plugin by calling
> > into a known entry point and the plugin initializes the data path. The control
> > path is already initialized by the PF driver when the VF is allocated. At this
> > point the shell switches to using the loaded plugin to do all further TX and RX
> > operations. The guest networking stack does not participate in these operations
> > and continues to function normally. All the control operations continue being
> > trapped by the hypervisor and are directed to the PF driver as needed. For
> > example, if the MAC address changes the hypervisor updates its internal state
> > and changes the state of the embedded switch as well through the PF control
> > API.
> >
>
> This is essentially a miniature network stack with a its own mini
> bonding layer, mini hotplug, and mini API, except s/API/ABI/. Is this a
> correct view?
To some extent yes but there is no complicated bonding nor there is any thing
like a PCI hotplug. The shell interface is small and the OS always interacts
with the shell as the main driver. Based on the underlying VF the plugin
changes and this plugin as well is really small. Our vmxnet3 s/w plugin is
about 1300 lines with whitespaces and comments and the Intel Kawela plugin is
about 1100 lines with whitspaces and comments. The design principle is to put
more of the complexity related to initialization/control into the PF driver
rather than in plugin.
>
> If so, the Linuxy approach would be to use the ordinary drivers and the
> Linux networking API, and hide the bond setup using namespaces. The
> bond driver, or perhaps a new, similar, driver can be enhanced to
> propagate ethtool commands to its (hidden) components, and to have a
> control channel with the hypervisor.
>
> This would make the approach hypervisor agnostic, you're just pairing
> two devices and presenting them to the rest of the stack as a single device.
>
> > We have reworked our existing Linux vmxnet3 driver to accomodate NPA by
> > splitting the driver into two parts: Shell and Plugin. The new split driver is
> >
>
> So the Shell would be the reworked or new bond driver, and Plugins would
> be ordinary Linux network drivers.
In NPA we do not rely on the guest OS to provide any of these services like
bonding or PCI hotplug. We don't rely on the guest OS to unmap a VF and switch
a VM out of passthrough. In a bonding approach that becomes an issue you can't
just yank a device from underneath, you have to wait for the OS to process the
request and switch from using VF to the emulated device and this makes the
hypervisor dependent on the guest OS. Also we don't rely on the presence of all
the drivers inside the guest OS (be it Linux or Windows), the ESX hypervisor
carries all the plugins and the PF drivers and injects the right one as needed.
These plugins are guest agnostic and the IHVs do not have to write plugins for
different OS.
Thanks,
-pankaj
^ permalink raw reply
* Re: question on virtio
From: Anthony Liguori @ 2010-05-05 19:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, kvm, virtualization
In-Reply-To: <20100505110947.GA27872@redhat.com>
On 05/05/2010 06:09 AM, Michael S. Tsirkin wrote:
> Hi!
> I see this in virtio_ring.c:
>
> /* Put entry in available array (but don't update avail->idx *
> until they do sync). */
>
> Why is it done this way?
> It seems that updating the index straight away would be simpler, while
> this might allow the host to specilatively look up the buffer and handle
> it, without waiting for the kick.
>
It should be okay as long as you don't update idx for partial vectors.
Regards,
Anthony Liguori
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox