Messages in this thread | | | Date | Mon, 04 May 2009 23:37:38 +0300 | From | Izik Eidus <> | Subject | Re: [PATCH 3/6] ksm: change the KSM_REMOVE_MEMORY_REGION ioctl. |
| |
Hugh Dickins wrote: > On Sun, 3 May 2009, Izik Eidus wrote: > > >> This patch change the KSM_REMOVE_MEMORY_REGION ioctl to be specific per >> memory region (instead of flushing all the registred memory regions inside >> the file descriptor like it happen now) >> >> The previoes api was: >> user register memory regions using KSM_REGISTER_MEMORY_REGION inside the fd, >> and then when he wanted to remove just one memory region, he had to remove them >> all using KSM_REMOVE_MEMORY_REGION. >> >> This patch change this beahivor by chaning the KSM_REMOVE_MEMORY_REGION >> ioctl to recive another paramter that it is the begining of the virtual >> address that is wanted to be removed. >> >> (user can still remove all the memory regions all at once, by just closing >> the file descriptor) >> >> Signed-off-by: Izik Eidus <ieidus@redhat.com> >> > > I realize that it's ridiculous to break my silence
Hello ! :)
> with a comment > on this particular patch, when I've not yet commented on KSM as a > whole. (In the last few days I have at last managed to set aside > some time to give KSM the attention it deserves, but I'm still > not yet through and ready to comment.) > > However, although this patch is on the right lines (certainly you > should be allowing to remove individual regions rather than just > all at once), I believe the patch is seriously broken and corrupting > as is, so thought I'd better speak up now. > > remove_mm_from_hash_and_tree(slot->mm) is still doing its own > silly loop through the slots: > list_for_each_entry(slot, &slots, link) > if (slot->mm == mm) > break; > So it will be operating on whatever it finds first
I just started to write big answer that go over the code path to show why you are wrong, and then found the problem.
Thanks i will fix it and resend...
> , in general > the wrong slot, and I expect havoc to follow once you kfree(slot). > > Easily fixed: replace remove_mm_from_hash_and_tree(mm) > by remove_slot_from_hash_and_tree(slot). >
Yea, remove_mm_from_hash_and_tree(mm) is surely something that we dont need that was left from the old code base.
Thanks. > Hugh > > >> --- >> mm/ksm.c | 31 +++++++++++++++++++++---------- >> 1 files changed, 21 insertions(+), 10 deletions(-) >> >> diff --git a/mm/ksm.c b/mm/ksm.c >> index 982dfff..c14019f 100644 >> --- a/mm/ksm.c >> +++ b/mm/ksm.c >> @@ -561,17 +561,20 @@ static void remove_mm_from_hash_and_tree(struct mm_struct *mm) >> list_del(&slot->link); >> } >> >> -static int ksm_sma_ioctl_remove_memory_region(struct ksm_sma *ksm_sma) >> +static int ksm_sma_ioctl_remove_memory_region(struct ksm_sma *ksm_sma, >> + unsigned long addr) >> { >> struct ksm_mem_slot *slot, *node; >> >> down_write(&slots_lock); >> list_for_each_entry_safe(slot, node, &ksm_sma->sma_slots, sma_link) { >> - remove_mm_from_hash_and_tree(slot->mm); >> - mmput(slot->mm); >> - list_del(&slot->sma_link); >> - kfree(slot); >> - ksm_sma->nregions--; >> + if (addr == slot->addr) { >> + remove_mm_from_hash_and_tree(slot->mm); >> + mmput(slot->mm); >> + list_del(&slot->sma_link); >> + kfree(slot); >> + ksm_sma->nregions--; >> + } >> } >> up_write(&slots_lock); >> return 0; >> @@ -579,12 +582,20 @@ static int ksm_sma_ioctl_remove_memory_region(struct ksm_sma *ksm_sma) >> >> static int ksm_sma_release(struct inode *inode, struct file *filp) >> { >> + struct ksm_mem_slot *slot, *node; >> struct ksm_sma *ksm_sma = filp->private_data; >> - int r; >> >> - r = ksm_sma_ioctl_remove_memory_region(ksm_sma); >> + down_write(&slots_lock); >> + list_for_each_entry_safe(slot, node, &ksm_sma->sma_slots, sma_link) { >> + remove_mm_from_hash_and_tree(slot->mm); >> + mmput(slot->mm); >> + list_del(&slot->sma_link); >> + kfree(slot); >> + } >> + up_write(&slots_lock); >> + >> kfree(ksm_sma); >> - return r; >> + return 0; >> } >> >> static long ksm_sma_ioctl(struct file *filp, >> @@ -607,7 +618,7 @@ static long ksm_sma_ioctl(struct file *filp, >> break; >> } >> case KSM_REMOVE_MEMORY_REGION: >> - r = ksm_sma_ioctl_remove_memory_region(sma); >> + r = ksm_sma_ioctl_remove_memory_region(sma, arg); >> break; >> } >> >> -- >> 1.5.6.5 >>
| |