* Re: [PATCH] regset: use vmalloc() for regset_get_alloc() [not found] ` <20240202040503.GX2087318@ZenIV> @ 2024-02-02 16:24 ` Doug Anderson 2024-02-02 16:49 ` Al Viro 0 siblings, 1 reply; 7+ messages in thread From: Doug Anderson @ 2024-02-02 16:24 UTC (permalink / raw) To: Al Viro Cc: Christian Brauner, Eric Biederman, Jan Kara, Kees Cook, linux-fsdevel, linux-kernel, linux-mm, Oleg Nesterov, Catalin Marinas, Will Deacon, Linux ARM 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] regset: use vmalloc() for regset_get_alloc() 2024-02-02 16:24 ` [PATCH] regset: use vmalloc() for regset_get_alloc() Doug Anderson @ 2024-02-02 16:49 ` Al Viro 2024-02-02 16:55 ` Al Viro 0 siblings, 1 reply; 7+ messages in thread From: Al Viro @ 2024-02-02 16:49 UTC (permalink / raw) To: Doug Anderson Cc: Christian Brauner, Eric Biederman, Jan Kara, Kees Cook, linux-fsdevel, linux-kernel, linux-mm, Oleg Nesterov, Catalin Marinas, Will Deacon, Linux ARM 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] regset: use vmalloc() for regset_get_alloc() 2024-02-02 16:49 ` Al Viro @ 2024-02-02 16:55 ` Al Viro 2024-02-02 18:07 ` Dave Martin 0 siblings, 1 reply; 7+ messages in thread From: Al Viro @ 2024-02-02 16:55 UTC (permalink / raw) To: Doug Anderson Cc: Christian Brauner, Eric Biederman, Jan Kara, Kees Cook, linux-fsdevel, linux-kernel, linux-mm, Oleg Nesterov, Catalin Marinas, Will Deacon, Linux ARM 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] regset: use vmalloc() for regset_get_alloc() 2024-02-02 16:55 ` Al Viro @ 2024-02-02 18:07 ` Dave Martin 2024-02-02 19:13 ` Doug Anderson 2024-02-02 19:42 ` Mark Brown 0 siblings, 2 replies; 7+ messages in thread From: Dave Martin @ 2024-02-02 18:07 UTC (permalink / raw) To: Al Viro Cc: Doug Anderson, Christian Brauner, Eric Biederman, Jan Kara, Kees Cook, linux-fsdevel, linux-kernel, linux-mm, Oleg Nesterov, Catalin Marinas, Will Deacon, Mark Brown, Linux ARM 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] regset: use vmalloc() for regset_get_alloc() 2024-02-02 18:07 ` Dave Martin @ 2024-02-02 19:13 ` Doug Anderson 2024-02-02 19:42 ` Mark Brown 1 sibling, 0 replies; 7+ messages in thread From: Doug Anderson @ 2024-02-02 19:13 UTC (permalink / raw) To: Dave Martin Cc: Al Viro, Christian Brauner, Eric Biederman, Jan Kara, Kees Cook, linux-fsdevel, linux-kernel, linux-mm, Oleg Nesterov, Catalin Marinas, Will Deacon, Mark Brown, Linux ARM 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] regset: use vmalloc() for regset_get_alloc() 2024-02-02 18:07 ` Dave Martin 2024-02-02 19:13 ` Doug Anderson @ 2024-02-02 19:42 ` Mark Brown 2024-02-02 20:38 ` Doug Anderson 1 sibling, 1 reply; 7+ messages in thread From: Mark Brown @ 2024-02-02 19:42 UTC (permalink / raw) To: Dave Martin Cc: Al Viro, Doug Anderson, Christian Brauner, Eric Biederman, Jan Kara, Kees Cook, linux-fsdevel, linux-kernel, linux-mm, Oleg Nesterov, Catalin Marinas, Will Deacon, Linux ARM [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] regset: use vmalloc() for regset_get_alloc() 2024-02-02 19:42 ` Mark Brown @ 2024-02-02 20:38 ` Doug Anderson 0 siblings, 0 replies; 7+ messages in thread From: Doug Anderson @ 2024-02-02 20:38 UTC (permalink / raw) To: Mark Brown Cc: Dave Martin, Al Viro, Christian Brauner, Eric Biederman, Jan Kara, Kees Cook, linux-fsdevel, linux-kernel, linux-mm, Oleg Nesterov, Catalin Marinas, Will Deacon, Linux ARM 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-02-02 20:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240201171159.1.Id9ad163b60d21c9e56c2d686b0cc9083a8ba7924@changeid>
[not found] ` <20240202012249.GU2087318@ZenIV>
[not found] ` <CAD=FV=X5dpMyCGg4Xn+ApRwmiLB5zB0LTMCoSfW_X6eAsfQy8w@mail.gmail.com>
[not found] ` <20240202030438.GV2087318@ZenIV>
[not found] ` <CAD=FV=Wbq7R9AirvxnW1aWoEnp2fWQrwBsxsDB46xbfTLHCZ4w@mail.gmail.com>
[not found] ` <20240202034925.GW2087318@ZenIV>
[not found] ` <20240202040503.GX2087318@ZenIV>
2024-02-02 16:24 ` [PATCH] regset: use vmalloc() for regset_get_alloc() Doug Anderson
2024-02-02 16:49 ` Al Viro
2024-02-02 16:55 ` Al Viro
2024-02-02 18:07 ` Dave Martin
2024-02-02 19:13 ` Doug Anderson
2024-02-02 19:42 ` Mark Brown
2024-02-02 20:38 ` Doug Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox