lkml.org 
[lkml]   [2015]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 06/36] HMM: add HMM page table v2.


On Thu, 21 May 2015, j.glisse@gmail.com wrote:

> From: Jérôme Glisse <jglisse@redhat.com>
>
> [...]
> +
> +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.

- 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.

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?

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?
\
 
 \ /
  Last update: 2015-06-26 01:21    [W:0.319 / U:1.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site