Re: [PATCH 3/6] ksm: change the KSM_REMOVE_MEMORY_REGION ioctl.

[Izik Eidus]

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@xxxxxxxxxx>
> >     
>
> 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
> >     

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/