Re: Re: [PATCH 05/15] HMM: introduce heterogeneous memory management v4.

[Jerome Glisse]

On Mon, Aug 03, 2015 at 12:21:14PM +0000, GIRISH K S wrote:
> On Mon, Aug 03, 2015 at 01:20:13PM +0530, Girish KS wrote:
> > On 18-Jul-2015 12:47 am, "JÃÆÃÂÃâÃÂÃâÃÂÃÆÃÂÃâÃÂÃâÃÂe Glisse" wrote:
> > >
> 
> [...]
> 
> > > +int hmm_mirror_register(struct hmm_mirror *mirror)
> > > +{
> > > +       struct mm_struct *mm = current->mm;
> > > +       struct hmm *hmm = NULL;
> > > +       int ret = 0;
> > > +
> > > +       /* Sanity checks. */
> > > +       BUG_ON(!mirror);
> > > +       BUG_ON(!mirror->device);
> > > +       BUG_ON(!mm);
> > > +
> > > +       /*
> > > +        * Initialize the mirror struct fields, the mlist init and del
> > dance is
> > > +        * necessary to make the error path easier for driver and for hmm.
> > > +        */
> > > +       kref_init(&mirror->kref);
> > > +       INIT_HLIST_NODE(&mirror->mlist);
> > > +       INIT_LIST_HEAD(&mirror->dlist);
> > > +       spin_lock(&mirror->device->lock);
> > > +       list_add(&mirror->dlist, &mirror->device->mirrors);
> > > +       spin_unlock(&mirror->device->lock);
> > > +
> > > +       down_write(&mm->mmap_sem);
> > > +
> > > +       hmm = mm->hmm ? hmm_ref(hmm) : NULL;
> > 
> > Instead of hmm mm->hmm would be the right param to be passed.  Here even
> > though mm->hmm is true hmm_ref returns NULL. Because hmm is not updated
> > after initialization in the beginning.
> 
> ENOPARSE ? While this can be simplified to hmm = hmm_ref(mm->hmm); I do not
> see what you mean. The mm struct might already have a valid hmm field set,
> and that valid hmm struct might also already be in the process of being
> destroy. So hmm_ref() might either return the same hmm pointer if the hmm
> object is not about to be release or NULL. But at this point there is no
> certainty on the return value of hmm_ref().
> 
> I didn't mean hmm = hmm_ref(mm->hmm);. I ll try to put it in a better way.
> The hmm local variable is initialized to NULL in the start of the function
> (struct hmm *hmm = NULL;), and this is not modified till it is passed to
> hmm_ref.  So hmm_ref would always return a NULL irrespective of mm->hmm is
> NULL or valid address.  
> So  the statement hmm = mm->hmm ? hmm_ref(hmm) : NULL; should be replaced
> as hmm = mm->hmm ? hmm_ref(mm->hmm) : NULL;. 

Oh yeah typo probably outcome of many patch reorg i did.

Cheers,
JÃrÃme
--
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/