Re: [PATCH] Doc:kvm: Fix typo in Doc/virtual/kvm
On Sun, 4 Oct 2015 00:46:21 +0900 Masanari Iida wrote: > This patch fix spelling typos in Documentation/virtual/kvm. Applied to the docs tree, thanks. jon -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add ksm kernel shared memory driver.
On Tue, 2 Dec 2008 13:24:11 -0800 Chris Wright <[EMAIL PROTECTED]> wrote: > > Using current known techniques. A random collision is just as bad > > news. > > And, just to clarify, your concern would extend to any digest based > comparison? Or are you specifically concerned about sha1? Wouldn't this issue just go away if the code simply compared the full pages, rather than skipping the hashed 128 bytes at the beginning? Given the cost of this whole operation (which, it seems, can involve copying one of the pages before testing for equality), skipping the comparison of 128 bytes seems like a bit of a premature optimization. jon -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add ksm kernel shared memory driver
[Let's see if I can get through the rest without premature sends...] On Wed, 12 Nov 2008 00:17:39 +0200 Izik Eidus <[EMAIL PROTECTED]> wrote: > > Actually, it occurs to me that there's no sanity checks on any of > > the values passed in by ioctl(). What happens if the user tells > > KSM to scan a bogus range of memory? > > > > Well get_user_pages() run in context of the process, therefore it > should fail in "bogus range of memory" But it will fail in a totally silent and mysterious way. Doesn't it seem better to verify the values when you can return a meaningful error code to the caller? The other ioctl() calls have the same issue; you can start the thread with nonsensical values for the number of pages to scan and the sleep time. > > > Any benchmarks on the runtime cost of having KSM running? > > > > This one is problematic, ksm can take anything from 0% to 100% cpu > its all depend on how fast you run it. > it have 3 parameters: > number of pages to scan before it go to sleep > maximum number of pages to merge while we scanning the above pages > (merging is expensive) > time to sleep (when runing from userspace using /dev/ksm, we actually > do it there (userspace) What about things like cache effects from scanning all those pages? My guess is that, if you're trying to run dozens of Windows guests, cache usage is not at the top of your list of concerns, but I could be wrong. Usually am... Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add ksm kernel shared memory driver
On Wed, 12 Nov 2008 00:17:39 +0200
Izik Eidus <[EMAIL PROTECTED]> wrote:
> >> +static int ksm_dev_open(struct inode *inode, struct file *filp)
> >> +{
> >> + try_module_get(THIS_MODULE);
> >> + return 0;
> >> +}
> >> +
> >> +static int ksm_dev_release(struct inode *inode, struct file *filp)
> >> +{
> >> + module_put(THIS_MODULE);
> >> + return 0;
> >> +}
> >> +
> >> +static struct file_operations ksm_chardev_ops = {
> >> + .open = ksm_dev_open,
> >> + .release= ksm_dev_release,
> >> + .unlocked_ioctl = ksm_dev_ioctl,
> >> + .compat_ioctl = ksm_dev_ioctl,
> >> +};
> >>
> >
> > Why do you roll your own module reference counting? Is there a
> > reason you don't just set .owner and let the VFS handle it?
> >
>
> Yes, I am taking get_task_mm() if the module will be removed before i
> free the mms, things will go wrong
But...if you set .owner, the VFS will do the try_module_get() *before*
calling into your module (as an added bonus, it will actually check the
return value too). All you've succeeded in doing here is adding a
microscopic race to the module reference counting; otherwise the end
result is the same.
jon
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add ksm kernel shared memory driver
I don't claim to begin to really understand the deep VM side of this
patch, but I can certainly pick nits as I work through it...sorry for
the lack of anything more substantive.
> +static struct list_head slots;
Some of these file-static variable names seem a little..terse...
> +#define PAGECMP_OFFSET 128
> +#define PAGEHASH_SIZE (PAGECMP_OFFSET ? PAGECMP_OFFSET : PAGE_SIZE)
> +/* hash the page */
> +static void page_hash(struct page *page, unsigned char *digest)
So is this really saying that you only hash the first 128 bytes, relying on
full compares for the rest? I assume there's a perfectly good reason for
doing it that way, but it's not clear to me from reading the code. Do you
gain performance which is not subsequently lost in the (presumably) higher
number of hash collisions?
> +static int ksm_scan_start(struct ksm_scan *ksm_scan, int scan_npages,
> + int max_npages)
> +{
> + struct ksm_mem_slot *slot;
> + struct page *page[1];
> + int val;
> + int ret = 0;
> +
> + down_read(&slots_lock);
> +
> + scan_update_old_index(ksm_scan);
> +
> + while (scan_npages > 0 && max_npages > 0) {
Should this loop maybe check kthread_run too? It seems like you could loop
for a long time after kthread_run has been set to zero.
> +static int ksm_dev_open(struct inode *inode, struct file *filp)
> +{
> + try_module_get(THIS_MODULE);
> + return 0;
> +}
> +
> +static int ksm_dev_release(struct inode *inode, struct file *filp)
> +{
> + module_put(THIS_MODULE);
> + return 0;
> +}
> +
> +static struct file_operations ksm_chardev_ops = {
> + .open = ksm_dev_open,
> + .release= ksm_dev_release,
> + .unlocked_ioctl = ksm_dev_ioctl,
> + .compat_ioctl = ksm_dev_ioctl,
> +};
Why do you roll your own module reference counting? Is there a reason you
don't just set .owner and let the VFS handle it?
Given that the KSM_REGISTER_MEMORY_REGION ioctl() creates unswappable
memory, should there be some sort of capability check done there? A check
for starting/stopping the thread might also make sense. Or is that
expected to be handled via permissions on /dev/ksm?
Actually, it occurs to me that there's no sanity checks on any of the
values passed in by ioctl(). What happens if the user tells KSM to scan a
bogus range of memory?
Any benchmarks on the runtime cost of having KSM running?
jon
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
