Hi Rick,

Sorry for the delay, I was a bit preoccupied with $stuff.

On Thu, Oct 05, 2023 at 06:09:07PM +0000, Edgecombe, Rick P wrote:

On Thu, 2023-10-05 at 08:26 +0300, Mike Rapoport wrote:

quoted

On Wed, Oct 04, 2023 at 03:39:26PM +0000, Edgecombe, Rick P wrote:

quoted

On Tue, 2023-10-03 at 17:29 -0700, Rick Edgecombe wrote:

quoted

It seems a bit weird to copy all of this. Is it trying to be
faster
or
something?

Couldn't it just check r->start in execmem_text/data_alloc() path
and
switch to EXECMEM_DEFAULT if needed then? The
execmem_range_is_data()
part that comes later could be added to the logic there too. So
this
seems like unnecessary complexity to me or I don't see the
reason.

I guess this is a bad idea because if you have the full size array
sitting around anyway you might as well use it and reduce the
exec_mem_alloc() logic.

That's was the idea, indeed. :)

quoted

Just looking at it from the x86 side (and
similar) though, where there is actually only one execmem_range and
it
building this whole array with identical data and it seems weird.

Right, most architectures have only one range, but to support all
variants
that we have, execmem has to maintain the whole array.

What about just having an index into a smaller set of ranges. The
module area and the extra JIT area. So ->ranges can be size 3
(statically allocated in the arch code) for three areas and then the
index array can be size EXECMEM_TYPE_MAX. The default 0 value of the
indexing array will point to the default area and any special areas can
be set in the index point to the desired range.

Looking at how it would do for x86 and arm64, it looks maybe a bit
better to me. A little bit less code and memory usage, and a bit easier
to trace the configuration through to the final state (IMO). What do
you think? Very rough, on top of this series, below.

I like your suggestion to only have definitions of actual ranges in arch
code and index array to redirect allocation requests to the right range.
I'll make the next version along the lines of your patch.

As I was playing around with this, I was also wondering why it needs
two copies of struct execmem_params: one returned from the arch code
and one in exec mem. 

No actual reason, one copy is enough, thanks for catching this.

And why the temporary arch copy is ro_after_init,
but the final execmem.c copy is not ro_after_init?

I just missed it, thanks for pointing out.
 
-- 
Sincerely yours,
Mike.