Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

[Doug Anderson]

Hi,

On Thu, Feb 1, 2024 at 8:05 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, Feb 02, 2024 at 03:49:25AM +0000, Al Viro wrote:
> > On Thu, Feb 01, 2024 at 07:15:48PM -0800, Doug Anderson wrote:
> > > >
> > > > Well, the next step would be to see which regset it is - if you
> > > > see that kind of allocation, print regset->n, regset->size and
> > > > regset->core_note_type.
> > >
> > > Of course! Here are the big ones:
> > >
> > > [   45.875574] DOUG: Allocating 279584 bytes, n=17474, size=16,
> > > core_note_type=1029
> >
> > 0x405, NT_ARM_SVE
> >         [REGSET_SVE] = { /* Scalable Vector Extension */
> >                 .core_note_type = NT_ARM_SVE,
> >                 .n = DIV_ROUND_UP(SVE_PT_SIZE(SVE_VQ_MAX, SVE_PT_REGS_SVE),
> >                                   SVE_VQ_BYTES),
> >                 .size = SVE_VQ_BYTES,
> >
> > IDGI.  Wasn't SVE up to 32 * 2Kbit, i.e. 8Kbyte max?  Any ARM folks around?
> > Sure, I understand that it's variable-sized and we want to allocate enough
> > for the worst case, but can we really get about 280Kb there?  Context switches
> > would be really unpleasant on such boxen...
>
> FWIW, this apparently intends to be "variable, up to SVE_PT_SIZE(...) bytes";
> no idea if SVE_PT_SIZE is the right thing to use here.

+folks from `./scripts/get_maintainer.pl -f arch/arm64/kernel/ptrace.c`

Trying to follow the macros to see where "n" comes from is a maze of
twisty little passages, all alike. Hopefully someone from the ARM
world can help tell if the value of 17474 for n here is correct or if
something is wonky.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

[Al Viro]

On Fri, Feb 02, 2024 at 08:24:17AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Thu, Feb 1, 2024 at 8:05 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Fri, Feb 02, 2024 at 03:49:25AM +0000, Al Viro wrote:
> > > On Thu, Feb 01, 2024 at 07:15:48PM -0800, Doug Anderson wrote:
> > > > >
> > > > > Well, the next step would be to see which regset it is - if you
> > > > > see that kind of allocation, print regset->n, regset->size and
> > > > > regset->core_note_type.
> > > >
> > > > Of course! Here are the big ones:
> > > >
> > > > [   45.875574] DOUG: Allocating 279584 bytes, n=17474, size=16,
> > > > core_note_type=1029
> > >
> > > 0x405, NT_ARM_SVE
> > >         [REGSET_SVE] = { /* Scalable Vector Extension */
> > >                 .core_note_type = NT_ARM_SVE,
> > >                 .n = DIV_ROUND_UP(SVE_PT_SIZE(SVE_VQ_MAX, SVE_PT_REGS_SVE),
> > >                                   SVE_VQ_BYTES),
> > >                 .size = SVE_VQ_BYTES,
> > >
> > > IDGI.  Wasn't SVE up to 32 * 2Kbit, i.e. 8Kbyte max?  Any ARM folks around?
> > > Sure, I understand that it's variable-sized and we want to allocate enough
> > > for the worst case, but can we really get about 280Kb there?  Context switches
> > > would be really unpleasant on such boxen...
> >
> > FWIW, this apparently intends to be "variable, up to SVE_PT_SIZE(...) bytes";
> > no idea if SVE_PT_SIZE is the right thing to use here.
> 
> +folks from `./scripts/get_maintainer.pl -f arch/arm64/kernel/ptrace.c`
> 
> Trying to follow the macros to see where "n" comes from is a maze of
> twisty little passages, all alike. Hopefully someone from the ARM
> world can help tell if the value of 17474 for n here is correct or if
> something is wonky.

It might be interesting to have it print the return value of __regset_get()
in those cases; if *that* is huge, we really have a problem.  If it ends up
small enough to fit into few pages, OTOH...

SVE_VQ_MAX is defined as 255; is that really in units of 128 bits?  IOW,
do we really expect to support 32Kbit registers?  That would drive the
size into that range, all right, but it would really suck on context
switches.

I could be misreading it, though - the macros in there are not easy to
follow and I've never dealt with SVE before, so take the above with
a cartload of salt.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

[Al Viro]

On Fri, Feb 02, 2024 at 04:49:47PM +0000, Al Viro wrote:
> > +folks from `./scripts/get_maintainer.pl -f arch/arm64/kernel/ptrace.c`
> > 
> > Trying to follow the macros to see where "n" comes from is a maze of
> > twisty little passages, all alike. Hopefully someone from the ARM
> > world can help tell if the value of 17474 for n here is correct or if
> > something is wonky.
> 
> It might be interesting to have it print the return value of __regset_get()
> in those cases; if *that* is huge, we really have a problem.  If it ends up
> small enough to fit into few pages, OTOH...
> 
> SVE_VQ_MAX is defined as 255; is that really in units of 128 bits?  IOW,
> do we really expect to support 32Kbit registers?  That would drive the
> size into that range, all right, but it would really suck on context
> switches.
> 
> I could be misreading it, though - the macros in there are not easy to
> follow and I've never dealt with SVE before, so take the above with
> a cartload of salt.

Worse - it's SVE_VQ_MAX is 512; sorry about the confusion.  OK, that would
certainly explain the size (header + 32 registers, each up to 512 * 16 bytes),
but... ouch.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

[Dave Martin]

On Fri, Feb 02, 2024 at 04:55:24PM +0000, Al Viro wrote:
> On Fri, Feb 02, 2024 at 04:49:47PM +0000, Al Viro wrote:
> > > +folks from `./scripts/get_maintainer.pl -f arch/arm64/kernel/ptrace.c`
> > > 
> > > Trying to follow the macros to see where "n" comes from is a maze of
> > > twisty little passages, all alike. Hopefully someone from the ARM
> > > world can help tell if the value of 17474 for n here is correct or if
> > > something is wonky.

Nope, that's the "correct" answer...

> > 
> > It might be interesting to have it print the return value of __regset_get()
> > in those cases; if *that* is huge, we really have a problem.  If it ends up
> > small enough to fit into few pages, OTOH...
> > 
> > SVE_VQ_MAX is defined as 255; is that really in units of 128 bits?  IOW,
> > do we really expect to support 32Kbit registers?  That would drive the
> > size into that range, all right, but it would really suck on context
> > switches.
> > 
> > I could be misreading it, though - the macros in there are not easy to
> > follow and I've never dealt with SVE before, so take the above with
> > a cartload of salt.
> 
> Worse - it's SVE_VQ_MAX is 512; sorry about the confusion.  OK, that would
> certainly explain the size (header + 32 registers, each up to 512 * 16 bytes),
> but... ouch.

Mark Brown [+ Cc] has been taking care of SVE in my absence, but
from memory:

The SVE architecture has a really big maximum vector size (16 * 128 =
2048 bits), and there is a theoretical possibility of it getting bigger
in the future, though unlikely.

Real platforms to date have a much smaller limit, though Qemu can go up
to 2048 bits IIUC.

My aim when working on the ABI was to future-proof it against
foreseeable expansion on the architecture side, but this does mean that
we cannot statically determine a sane limit for the vector size.


I suppose we could have had a more sane limit built into the kernel or a
Kconfig option for it, but it seemed simpler just to determine the size
dynamically depending on the task's current state.  This is not so
important for coredumps, but for the the gdbstub wire protocol etc. it
seemed undesirable to have the regset larger than needed.

Hence the reason for adding ->get_size() in
27e64b4be4b8 ("regset: Add support for dynamically sized regsets").

What I guess was not so obvious from the commit message is the
expected relationship between the actual and maximum possible size
of the regset: for SVE the actual size is in practice going to be *much*
smaller than the max, while the max is crazy large because of being an
ABI design limit chosen for futureproofing purposes.



So, if the only reason for trying to migrate to vmalloc() is to cope
with an insanely sized regset on arm64, I think somehow or other we can
avoid that.

Options:

 a) bring back ->get_size() so that we can allocate the correct size
before generating the regset data;

 b) make aarch64_regsets[] __ro_after_init and set
aarch64_regsets[REGSET_SVE].n based on the boot-time probed maximum size
(which will be sane); or

 c) allow membufs to grow if needed (sounds fragile though, and may be
hard to justify just for one arch?).


Thoughts?

If people don't want to bring back get_size(), then (b) doesn't look
too bad.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

[Doug Anderson]

Hi,

On Fri, Feb 2, 2024 at 10:08 AM Dave Martin <Dave.Martin@arm.com> wrote:
>
> So, if the only reason for trying to migrate to vmalloc() is to cope
> with an insanely sized regset on arm64, I think somehow or other we can
> avoid that.

Right. The only reason for the patch to switch to vmalloc() was in
reaction to seeing the order 7 memory allocation. If we can decrease
that to something sensible then I'm happy enough keeping the
allocation as kmalloc().


> Options:
>
>  a) bring back ->get_size() so that we can allocate the correct size
> before generating the regset data;
>
>  b) make aarch64_regsets[] __ro_after_init and set
> aarch64_regsets[REGSET_SVE].n based on the boot-time probed maximum size
> (which will be sane); or
>
>  c) allow membufs to grow if needed (sounds fragile though, and may be
> hard to justify just for one arch?).
>
>
> Thoughts?
>
> If people don't want to bring back get_size(), then (b) doesn't look
> too bad.

Either a) or b) sounds fine to me, but I'm just a visitor to this code
so maybe I'll let the adults in the room chime in with their opinions.
;-) Also: if you think it's fruitful for me to try to write a patch to
do either of those then I can, but I also wouldn't object at all to
someone else writing a patch to fix this and I can just provide a
Tested-by and/or Reviewed-by. Let me know.

-Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1619 bytes --]

On Fri, Feb 02, 2024 at 06:07:54PM +0000, Dave Martin wrote:

> So, if the only reason for trying to migrate to vmalloc() is to cope
> with an insanely sized regset on arm64, I think somehow or other we can
> avoid that.

With SME we do routinely see the full glory of the 64K regset for ZA in
emulated systems so I think we have to treat it as an issue.

> Options:

>  a) bring back ->get_size() so that we can allocate the correct size
> before generating the regset data;

>  b) make aarch64_regsets[] __ro_after_init and set
> aarch64_regsets[REGSET_SVE].n based on the boot-time probed maximum size
> (which will be sane); or

Either of those seems sensible to me, a function would minimise the size
of allocations based on the process configuration which would be nice
and given that we're doing allocations it's probably reasonable
overhead.

>  c) allow membufs to grow if needed (sounds fragile though, and may be
> hard to justify just for one arch?).

I'm having a hard time getting enthusiastic about that one for the
reasons you mention.

We can also just lower the maximum size we tell the ptrace core to the
actual architectural maximum since AFAICT we don't expose that anywhere
external, I've got a patch in CI for that.  We'd still be allocating
more memory than we need for practical systems but less extravagantly
so.  It seems more suitable for an immediate fix for people to pick up
for production.

It did occur to me at some point in the past that we should avoid
telling the core about regsets that aren't physically supported on the
current system, I didn't get round to looking at that yet.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] regset: use vmalloc() for regset_get_alloc()

[Doug Anderson]

Hi,

On Fri, Feb 2, 2024 at 11:43 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Feb 02, 2024 at 06:07:54PM +0000, Dave Martin wrote:
>
> > So, if the only reason for trying to migrate to vmalloc() is to cope
> > with an insanely sized regset on arm64, I think somehow or other we can
> > avoid that.
>
> With SME we do routinely see the full glory of the 64K regset for ZA in
> emulated systems so I think we have to treat it as an issue.

Ah, got it. 64K is much less likely to be as big of a problem (only an
order 4 allocation), but you're right that it's still a size where
kvmalloc() would be an improvement. With that in mind I'll plan to
send out a v2 of my patch where I use kvmalloc() instead of vmalloc()
and update the commit description a bit, including a link to this
thread. Then I will assume that others on this thread will move
forward with actually making the allocations smaller.

Please yell if the above sounds wrong. :-)

-Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel