On Mon, Jun 14, 2021 at 11:42 AM Joao Martins <joao.m.martins@xxxxxxxxxx> wrote: > > > > On 6/7/21 8:32 PM, Dan Williams wrote: > > On Mon, Jun 7, 2021 at 6:49 AM Joao Martins <joao.m.martins@xxxxxxxxxx> wrote: > > [..] > >>> Given all of the above I'm wondering if there should be a new > >>> "compound specific" flavor of this routine rather than trying to > >>> cleverly inter mingle the old path with the new. This is easier > >>> because you have already done the work in previous patches to break > >>> this into helpers. So just have memmap_init_zone_device() do it the > >>> "old" way and memmap_init_compound() handle all the tail page init + > >>> optimizations. > >>> > >> I can separate it out, should be easier to follow. > >> > >> Albeit just a note, I think memmap_init_compound() should be the new normal as metadata > >> more accurately represents what goes on the page tables. That's regardless of > >> vmemmap-based gains, and hence why my train of thought was to not separate it. > >> > >> After this series, all devmap pages where @geometry matches @align will have compound > >> pages be used instead. And we enable that in device-dax as first user (in the next patch). > >> altmap or not so far just differentiates on the uniqueness of struct pages as the former > >> doesn't reuse base pages that only contain tail pages and consequently makes us initialize > >> all tail struct pages. > > > > I think combining the cases into a common central routine makes the > > code that much harder to maintain. A small duplication cost is worth > > it in my opinion to help readability / maintainability. > > > I am addressing this comment and taking a step back. By just moving the tail page init to > memmap_init_compound() this gets a lot more readable. Albeit now I think having separate > top-level loops over pfns, doesn't bring much improvement there. > > Here's what I have by moving just tails init to a separate routine. See your original > suggestion after the scissors mark. I have a slight inclination towards the first one, but > no really strong preference. Thoughts? I like your "first one" too.
