Re: [PATCH 06/36] HMM: add HMM page table v2.

[Jerome Glisse]

On Thu, Jun 25, 2015 at 03:57:29PM -0700, Mark Hairgrove wrote:
> On Thu, 21 May 2015, j.glisse@xxxxxxxxx wrote:
> > From: Jérôme Glisse <jglisse@xxxxxxxxxx>
> > [...]
> > +
> > +void hmm_pt_iter_init(struct hmm_pt_iter *iter);
> > +void hmm_pt_iter_fini(struct hmm_pt_iter *iter, struct hmm_pt *pt);
> > +unsigned long hmm_pt_iter_next(struct hmm_pt_iter *iter,
> > +			       struct hmm_pt *pt,
> > +			       unsigned long addr,
> > +			       unsigned long end);
> > +dma_addr_t *hmm_pt_iter_update(struct hmm_pt_iter *iter,
> > +			       struct hmm_pt *pt,
> > +			       unsigned long addr);
> > +dma_addr_t *hmm_pt_iter_fault(struct hmm_pt_iter *iter,
> > +			      struct hmm_pt *pt,
> > +			      unsigned long addr);
> 
> I've got a few more thoughts on hmm_pt_iter after looking at some of the 
> later patches. I think I've convinced myself that this patch functionally 
> works as-is, but I've got some suggestions and questions about the design.
> 
> Right now there are these three major functions:
> 
> 1) hmm_pt_iter_update(addr)
>    - Returns the hmm_pte * for addr, or NULL if none exists.
> 
> 2) hmm_pt_iter_fault(addr)
>    - Returns the hmm_pte * for addr, allocating a new one if none exists.
> 
> 3) hmm_pt_iter_next(addr, end)
>    - Returns the next possibly-valid address. The caller must use
>      hmm_pt_iter_update to check if there really is an hmm_pte there.
> 
> In my view, there are two sources of confusion here:
> - Naming. "update" shares a name with the HMM mirror callback, and it also
>   implies that the page tables are "updated" as a result of the call. 
>   "fault" likewise implies that the function handles a fault in some way.
>   Neither of these implications are true.

Maybe hmm_pt_iter_walk & hmm_pt_iter_populate are better name ?


> - hmm_pt_iter_next and hmm_pt_iter_update have some overlapping
>   functionality when compared to traditional iterators, requiring the 
>   callers to all do this sort of thing:
> 
>         hmm_pte = hmm_pt_iter_update(&iter, &mirror->pt, addr);
>         if (!hmm_pte) {
>             addr = hmm_pt_iter_next(&iter, &mirror->pt,
>                         addr, event->end);
>             continue;
>         }
> 
> Wouldn't it be more efficient and simpler to have _next do all the 
> iteration internally so it always returns the next valid entry? Then you 
> could combine _update and _next into a single function, something along 
> these lines (which also addresses the naming concern):
> 
> void hmm_pt_iter_init(iter, pt, start, end);
> unsigned long hmm_pt_iter_next(iter, hmm_pte *);
> unsigned long hmm_pt_iter_next_alloc(iter, hmm_pte *);
> 
> hmm_pt_iter_next would return the address and ptep of the next valid 
> entry, taking the place of the existing _update and _next functions. 
> hmm_pt_iter_next_alloc takes the place of _fault.
> 
> Also, since the _next functions don't take in an address, the iterator 
> doesn't have to handle the input addr being different from iter->cur.

It would still need to do the same kind of test, this test is really to
know when you switch from one directory to the next and to drop and take
reference accordingly.


> The logical extent of this is a callback approach like mm_walk. That would 
> be nice because the caller wouldn't have to worry about making the _init 
> and _fini calls. I assume you didn't go with this approach because 
> sometimes you need to iterate over hmm_pt while doing an mm_walk itself, 
> and you didn't want the overhead of nesting those?

Correct i do not want to do a hmm_pt_walk inside a mm_walk, that sounded and
looked bad in my mind. That being said i could add a hmm_pt_walk like mm_walk
for device driver and simply have it using the hmm_pt_iter internally.


> Finally, another minor thing I just noticed: shouldn't hmm_pt.h include 
> <linux/bitops.h> since it uses all of the clear/set/test bit APIs?

Good catch, i forgot that.

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/