On Thu, Sep 2, 2021 at 11:22 PM Mike Galbraith <efault@xxxxxx> wrote:
>
> To my eyeballs, below duplication of a couple lines of initialization
> needed by the lockless function is less icky than the double return.
Ack. Something like this seems to avoid the whole issue.
Vlastimil - the alternative I was actually going to suggest for that
"double return value" thing is something we use elsewhere in the VM
code - just use a structure for "state".
It's not hugely common, and I think Mike's solution in this case is
the much simpler one, but I'll mention it anyway because it's actually
been quite successful in the few cases we use it, and it's both
readable to humans and generates fairly good code.
The "generates fairly good code" is generally more true when inlining,
but it's not horrible even outside of that.
So the idea with the "state structure" is that you just pass in both
the arguments and the return values in a struct.
Some examples of this are
- 'struct follow_page_context' in mm/gup.c is an example of a small local one.
- 'struct vm_fault' is an example of a *big* one, that has a lot of
state, and that is actually exported as an interface.
That 'struct vm_fault' one is interesting as an example because it
does that "both inputs and outputs", and has that unnamed "const
struct" member to actually catch mis-uses where some callee would
change those fields (which is a no-no).
It's also an example of something that generates good code despite not
inlining, because it actively makes argument passing simpler and
avoids copying arguments as you call other functions.
Those issues are non-issues in this case, which is why I point to that
'follow_page_context' in the gup code as a very different example of
this, which is much more along the lines of the slub case. It's purely
local to that call chain, and it's basically used to return more
state.
In both cases, the top-level caller just initializes things on the
stack. For that 'struct vm_fault' case you actually *have* to use an
initializer, since the constant fields cannot be changed later. It
makes for pretty legible code, and when things are inlined, all those
fields actually act exactly like just regular local variables shared
across functions.
When things aren't inlined, it can generate extra memory accesses, but
that 'struct vm_fault' is an example of when that isn't even
necessarily worse.
Anyway, I wanted to just point that out as a pattern that has been
quite successful.
Side note: one very special case of that pattern goes back decades:
the VM use of structures that get returned and passed *as* structures.
It's in fact so common that people don't even think about it: 'pte_t'
and friends. They are designed to be opaque data types that are often
multi-word structures.
Quite often those structures have only one or two words in them, which
helps code generation (returning two words can be done in registers),
but they _can_ have more.
Eg x86:
typedef union {
struct {
unsigned long pte_low, pte_high;
};
pteval_t pte;
} pte_t;
or some powerpc cases:
typedef struct { pte_basic_t pte, pte1, pte2, pte3; } pte_t;
just to show that you can actually pass structures not by reference,
but by value, and it's not even unusual in the kernel. That can also
be used to return multiple values if you want to.
Note: code generation really matters, and when doing those multi-value
structures, for code generation it's generally important to limit it
to at most two words.
Anything more, and the compiler will generally be forced to pass it by
copying it to the stack and using a pointer to it, and then you get
the worst of both worlds. You're better off just passing the structure
by reference, and avoiding extra copies on the stack.
ANYWAY.
I'll drop this part from Andrew's patch-bomb, and I can take it later
in its cleaned-up form. You mentioned a git tree elsewhere, maybe even
that way.
Linus
