Messages in this thread | | | Date | Thu, 15 Mar 2018 14:55:02 +0200 | From | "Michael S. Tsirkin" <> | Subject | Re: [PATCH v5] drivers/misc: vm_gen_counter: initial driver implementation |
| |
On Thu, Mar 15, 2018 at 08:57:05AM +0200, Gal Hammer wrote: > On Wed, Mar 14, 2018 at 9:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Mar 14, 2018 at 07:25:36PM +0100, Greg KH wrote: > >> On Tue, Mar 13, 2018 at 07:40:51PM +0200, Michael S. Tsirkin wrote: > >> > I think it's a good idea to use sysfs for this. However, > >> > there are a couple of missing interfaces here: > >> > > >> > 1. Userspace needs a way to know when this value changes. > >> > I see no change notifications here and that does not seem right. > >> > >> How can these change? > > > > It's a hardware register. It changes when hardware feels like it :) > > In particular, it changes whenever VM is migrated or snapshotted. > > This value doesn't change when a VM is migrated. It is unlikely that > this value will be changed so frequently that a direct access to the > memory is required. Even in QEMU, the current implementation was > merged without an option to change the generation id on-the-fly. One > must run a new instance in order to set a new value, which means that > no application will be running during that time.
The point is still that it changes without an application or the kernel doing anything.
> >> > 2. Userspace needs to be able to read these without > >> > system calls. > >> > >> Ick, what? Why not? > >> > >> > Pls add mmap support to the raw format. > >> > >> For a single integer? Why do you need mmap for this? What is so > >> "performant" that needs to touch a sysfs file? > >> > (Phys address is not guaranteed to be page-aligned so you will > >> > probably want an offset attribute for that as well). > >> > >> Ick ick ick, that's why it's good to just stick with a sysfs file. > > I agree with Greg here. The user is able to read the value, and then > wait for a notification if she cares about changes.
OK. Patch has to implement notifications for this to work though. That's missing.
> >> Have you tested just how long this takes to see if the open/read/close > >> is really the bottleneck, or if the io on reading the value is the > >> bottleneck? > >> > >> thanks, > >> > >> greg k-h > > > > Well an application needs to check this value basically after > > every database transaction. So I'm pretty sure it's a performance > > sensitive path. But yes, I didn't profile any apps since they > > are yet to be written to use this interface. > > I'm fine deferring point 2 for now. > > > > -- > > MST > > Thanks, > > Gal.
| |