On Thu, Sep 10, 2020 at 12:11 PM Gerald Schaefer <gerald.schaefer@xxxxxxxxxxxxx> wrote: > > That sounds a lot like the pXd_offset_orig() from Martins first approach > in this thread: > https://lore.kernel.org/linuxppc-dev/20190418100218.0a4afd51@mschwideX1/ I have to admit to finding that name horrible, but aside from that, yes. I don't think "pXd_offset_orig()" makes any sense as a name. Yes, "orig" may make sense as the variable name (as in "this was the original value we read"), but a function name should describe what it *does*, not what the arguments are. Plus "original" doesn't make sense to me anyway, since we're not modifying it. To me, "original" means that there's a final version too, which this interface in no way implies. It's just "this is the value we already read". ("orig" does make some sense in that fault path - because by definition we *are* going to modify the page table entry, that's the whole point of the fault - we need to do something to not keep faulting. But here, we're not at all necessarily modifying the page table contents, we're just following them and readign the values once) Of course, I don't know what a better name would be to describe what is actually going on, I'm just explaining why I hate that naming. *Maybe* something like just "pXd_offset_value()" together with a comment explaining that it's given the upper pXd pointer _and_ the value behind it, and it needs to return the next level offset? I dunno. "value" doesn't really seem horribly descriptive either, but at least it doesn't feel actively misleading to me. Yeah, I get hung up on naming sometimes. I don't tend to care much about private local variables ("i" is a perfectly fine variable name), but these kinds of somewhat subtle cross-architecture definitions I feel matter. Linus
