On Fri, 26 Jun 2015, Jerome Glisse wrote:
> 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_populate sounds good. See below for _walk.
>
>
> > - 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.
But all of the directory references are already hidden entirely in the
iterator _update function. The caller only has to worry about taking
references on the bottom level, so I don't understand why the iterator
needs to return to the caller when it hits the end of a directory. Or for
that matter, why it returns every possible index within a directory to the
caller whether that index is valid or not.
If _next only returned to the caller when it hit a valid hmm_pte (or end),
then only one function would be needed (_next) instead of two
(_update/_walk and _next).
>
>
> > 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.
I agree that nesting walks feels bad. If we can get the hmm_pt_iter API
simple enough, I don't think an hmm_pt_walk callback approach is
necessary.
>
>
> > 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
>