* RE: [RFC PATCH v1] devres: align devres.data strictly only for devm_kmalloc() [not found] <74ae22cd-08c1-d846-3e1d-cbc38db87442@free.fr> @ 2019-12-18 14:20 ` Alexey Brodkin 2019-12-18 15:40 ` Marc Gonzalez [not found] ` <bf020a68-00fd-2bb7-c3b6-00f5befa293a@free.fr> 1 sibling, 1 reply; 6+ messages in thread From: Alexey Brodkin @ 2019-12-18 14:20 UTC (permalink / raw) To: Marc Gonzalez Cc: Eugeniy Paltsev, Peter Zijlstra, Robin Murphy, Vineet Gupta, Dmitry Torokhov, Rafael Wysocki, LKML, Bjorn Andersson, Russell King, Mark Brown, Greg Kroah-Hartman, Tejun Heo, arcml, Will Deacon, Linux ARM Hi Marc, We sort of expected something like that to happen at some point. Funny enough it's been a year since my change was accepted in v4.20 and only now somebody noticed :) Though quite a few questions below. > Commit a66d972465d15 ("devres: Align data[] to ARCH_KMALLOC_MINALIGN") > increased the alignment of devres.data unconditionally. > > Some platforms have very strict alignment requirements for DMA-safe > addresses, e.g. 128 bytes on arm64. There, struct devres amounts to: > 3 pointers + pad_to_128 + data + pad_to_256 > i.e. ~220 bytes of padding. Could you please elaborate a bit on mentioned paddings? I may understand the first one for 128 bytes but where does the second one for 256 bytes come from? > Let's enforce the alignment only for devm_kmalloc(). Ok so for devm_kmalloc() we don't change anything, right? We still add the same padding before real data array. > --- > I had not been aware that dynamic allocation granularity on arm64 was > 128 bytes. This means there's a lot of waste on small allocations. Now probably I'm missing something but when do you expect to save something? If those smaller allocations are done with devm_kmalloc() you aren't saving anything. > I suppose there's no easy solution, though. Right! It took a while till I was able to propose something people [almost silently] agreed with. > --- > drivers/base/devres.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index 0bbb328bd17f..bf39188613d9 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -26,14 +26,7 @@ struct devres_node { > > struct devres { > struct devres_node node; > - /* > - * Some archs want to perform DMA into kmalloc caches > - * and need a guaranteed alignment larger than > - * the alignment of a 64-bit integer. > - * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same > - * buffer alignment as if it was allocated by plain kmalloc(). > - */ > - u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; > + u8 data[]; > }; > > struct devres_group { > @@ -789,9 +782,16 @@ static void devm_kmalloc_release(struct device *dev, void *res) > /* noop */ > } > > +#define DEVM_KMALLOC_PADDING_SIZE \ > + (ARCH_KMALLOC_MINALIGN - sizeof(struct devres) % ARCH_KMALLOC_MINALIGN) Even given your update with: ------------------------------->8-------------------------------- #define DEVM_KMALLOC_PADDING_SIZE \ ((ARCH_KMALLOC_MINALIGN - sizeof(struct devres)) % ARCH_KMALLOC_MINALIGN) ------------------------------->8-------------------------------- I don't think I understand why do you need that "% ARCH_KMALLOC_MINALIGN" part? > static int devm_kmalloc_match(struct device *dev, void *res, void *data) > { > - return res == data; > + /* > + * 'res' is dr->data (not DMA-safe) > + * 'data' is the hand-aligned address from devm_kmalloc > + */ > + return res + DEVM_KMALLOC_PADDING_SIZE == data; > } > > /** > @@ -811,6 +811,9 @@ void * devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) > { > struct devres *dr; > > + /* Add enough padding to provide a DMA-safe address */ > + size += DEVM_KMALLOC_PADDING_SIZE; This implementation gets ugly and potentially will lead to problems later when people will start changing code here. Compared to that initially aligned by the compiler dr->data looks much more foolproof. > /* use raw alloc_dr for kmalloc caller tracing */ > dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev)); > if (unlikely(!dr)) > @@ -822,7 +825,7 @@ void * devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) > */ > set_node_dbginfo(&dr->node, "devm_kzalloc_release", size); > devres_add(dev, dr->data); > - return dr->data; > + return dr->data + DEVM_KMALLOC_PADDING_SIZE; Ditto. But first I'd like to understand what are you trying to really do with your change and then we'll see if there could be any better implementation. -Alexey _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v1] devres: align devres.data strictly only for devm_kmalloc() 2019-12-18 14:20 ` [RFC PATCH v1] devres: align devres.data strictly only for devm_kmalloc() Alexey Brodkin @ 2019-12-18 15:40 ` Marc Gonzalez 0 siblings, 0 replies; 6+ messages in thread From: Marc Gonzalez @ 2019-12-18 15:40 UTC (permalink / raw) To: Alexey Brodkin Cc: Eugeniy Paltsev, Peter Zijlstra, Robin Murphy, Vineet Gupta, Dmitry Torokhov, Rafael Wysocki, LKML, Bjorn Andersson, Russell King, Mark Brown, Greg Kroah-Hartman, Tejun Heo, arcml, Will Deacon, Linux ARM On 18/12/2019 15:20, Alexey Brodkin wrote: > On 17/12/2019 16:30, Marc Gonzalez wrote: > >> Commit a66d972465d15 ("devres: Align data[] to ARCH_KMALLOC_MINALIGN") >> increased the alignment of devres.data unconditionally. >> >> Some platforms have very strict alignment requirements for DMA-safe >> addresses, e.g. 128 bytes on arm64. There, struct devres amounts to: >> 3 pointers + pad_to_128 + data + pad_to_256 >> i.e. ~220 bytes of padding. > > Could you please elaborate a bit on mentioned paddings? > I may understand the first one for 128 bytes but where does the > second one for 256 bytes come from? Sure thing. struct devres { struct devres_node node; u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; }; struct devres_node = 3 pointers kmalloc dishes out memory in multiples of ARCH_KMALLOC_MINALIGN bytes. On arm64, ARCH_KMALLOC_MINALIGN = 128 (Everything written below assumes ARCH_KMALLOC_MINALIGN = 128) In alloc_dr() we request sizeof(struct devres) + sizeof(data) from kmalloc. sizeof(struct devres) = 128 because of the alignment directive. I.e. the 'data' field is automatically padded to 128 by the compiler. For most devm allocs (non-devm_kmalloc allocs), data is just 1 or 2 pointers. So kmalloc(128 + 16) allocates 256 bytes. >> Let's enforce the alignment only for devm_kmalloc(). > > Ok so for devm_kmalloc() we don't change anything, right? > We still add the same padding before real data array. (My commit message probably requires improvement & refining.) Yes, the objective of my patch is to keep the same behavior for devm_kmalloc() while reverting to the old behavior for all other uses of struct devres. >> I had not been aware that dynamic allocation granularity on arm64 was >> 128 bytes. This means there's a lot of waste on small allocations. > > Now probably I'm missing something but when do you expect to save something? > If those smaller allocations are done with devm_kmalloc() you aren't > saving anything. With my patch, a "non-kmalloc" struct devres would take 128 bytes, instead of 256. >> I suppose there's no easy solution, though. > > Right! It took a while till I was able to propose something > people [almost silently] agreed with. I meant the wider subject of dynamic allocation granularity. The 128-byte requirement is only for DMA. Some (most?) uses of kmalloc are not for DMA. If the user could provide a flag ("this is to be used for DMA") we could save lots of memory for small non-DMA allocs. >> +#define DEVM_KMALLOC_PADDING_SIZE \ >> + (ARCH_KMALLOC_MINALIGN - sizeof(struct devres) % ARCH_KMALLOC_MINALIGN) > > Even given your update with: > ------------------------------->8-------------------------------- > #define DEVM_KMALLOC_PADDING_SIZE \ > ((ARCH_KMALLOC_MINALIGN - sizeof(struct devres)) % ARCH_KMALLOC_MINALIGN) > ------------------------------->8-------------------------------- > I don't think I understand why do you need that "% ARCH_KMALLOC_MINALIGN" part? To handle the case where sizeof(struct devres) > ARCH_KMALLOC_MINALIGN e.g ARCH_KMALLOC_MINALIGN = 8 and sizeof(struct devres) = 12 >> + /* Add enough padding to provide a DMA-safe address */ >> + size += DEVM_KMALLOC_PADDING_SIZE; > > This implementation gets ugly and potentially will lead to problems later > when people will start changing code here. Compared to that initially aligned by > the compiler dr->data looks much more foolproof. Yes, it's better to let the compiler handle the padding... But, we don't want any padding in the non-devm_kmalloc use-case. We could add a pointer to the data field, but arches with small ARCH_KMALLOC_MINALIGN will have to pay the size increase, which doesn't seem fair to them (x86, amd64). >> @@ -822,7 +825,7 @@ void * devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) >> */ >> set_node_dbginfo(&dr->node, "devm_kzalloc_release", size); >> devres_add(dev, dr->data); >> - return dr->data; >> + return dr->data + DEVM_KMALLOC_PADDING_SIZE; > > Ditto. But first I'd like to understand what are you trying to really do > with your change and then we'll see if there could be any better implementation. Basically, every call to devres_alloc() or devm_add_action() allocates 256 bytes instead of 128. A typical arm64 system will call these thousands of times during driver probe. Regards. _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <bf020a68-00fd-2bb7-c3b6-00f5befa293a@free.fr>]
[parent not found: <20191220140655.GN2827@hirez.programming.kicks-ass.net>]
[parent not found: <9be1d523-e92c-836b-b79d-37e880d092a0@arm.com>]
* RE: [RFC PATCH v1] devres: align devres.data strictly only for devm_kmalloc() [not found] ` <9be1d523-e92c-836b-b79d-37e880d092a0@arm.com> @ 2019-12-20 19:32 ` Alexey Brodkin 2019-12-20 20:23 ` Peter Zijlstra 0 siblings, 1 reply; 6+ messages in thread From: Alexey Brodkin @ 2019-12-20 19:32 UTC (permalink / raw) To: Robin Murphy, Peter Zijlstra, Marc Gonzalez Cc: Eugeniy Paltsev, Greg Kroah-Hartman, Vineet Gupta, Dmitry Torokhov, Rafael Wysocki, LKML, Bjorn Andersson, Russell King, Mark Brown, Tejun Heo, arcml, Will Deacon, Linux ARM Hi Robin, Peter, all, [snip] > On 2019-12-20 2:06 pm, Peter Zijlstra wrote: > > On Fri, Dec 20, 2019 at 11:19:27AM +0100, Marc Gonzalez wrote: > >> Would anyone else have any suggestions, comments, insights, recommendations, > >> improvements, guidance, or wisdom? :-) > > > > Flip devres upside down! > > Which doesn't really help :( > > > **WARNING, wear protective glasses when reading the below** > > > > > > struct devres { > > struct devres_node node; > > void *data; > > }; > > > > /* > > * We place struct devres at the tail of the memory allocation > > * such that data retains the ARCH_KMALLOC_MINALIGN alignment. > > * struct devres itself is just 4 pointers and should therefore > > * only require trivial alignment. > > */ > > static inline struct devres *data2devres(void *data) > > { > > return (struct devres *)(data + ksize(data) - sizeof(struct devres)); > > } > > > > void *alloc_dr(...) > > { > > struct devres *dr; > > void *data; > > > > data = kmalloc(size + sizeof(struct devres), GFP_KERNEL); > > At this point, you'd still need to special-case devm_kmalloc() to ensure > size is rounded up to the next ARCH_KMALLOC_MINALIGN granule, or you'd > go back to the original problem of the struct devres fields potentially > sharing a cache line with the data buffer. That needs to be avoided, > because if the devres list is modified while the buffer is mapped for > noncoherent DMA (which could legitimately happen as they are nominally > distinct allocations with different owners) there's liable to be data > corruption one way or the other. Well it somehow used to work for quite some time now with the data-buffer being allocated with 4 words offset (which is 16 bytes for 32-bit platform and 32 for 64-bit which is still much less than mentioned 128 bytes). Or we just never managed to identify those rare cases when data corruption really happened? > No matter which way round you allocate devres and data, by necessity > they're always going to consume the same total amount of memory. So then the next option I guess is to separate meta-data from data buffers completely. Are there any reasons to not do that other than the hack we're discussing here (meta-data in the beginning of the buffer) used to work OK-ish? -Alexey _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v1] devres: align devres.data strictly only for devm_kmalloc() 2019-12-20 19:32 ` Alexey Brodkin @ 2019-12-20 20:23 ` Peter Zijlstra 2019-12-20 21:02 ` Alexey Brodkin 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2019-12-20 20:23 UTC (permalink / raw) To: Alexey Brodkin Cc: Will Deacon, Marc Gonzalez, Eugeniy Paltsev, Greg Kroah-Hartman, Vineet Gupta, Dmitry Torokhov, Rafael Wysocki, LKML, Bjorn Andersson, Russell King, Mark Brown, Tejun Heo, arcml, Robin Murphy, Linux ARM On Fri, Dec 20, 2019 at 07:32:16PM +0000, Alexey Brodkin wrote: > Well it somehow used to work for quite some time now with the data-buffer > being allocated with 4 words offset (which is 16 bytes for 32-bit platform 3 words, devres_node is 3 words. Which is exactly why we had to change it, the odd alignment caused ARC to explode. > and 32 for 64-bit which is still much less than mentioned 128 bytes). > Or we just never managed to identify those rare cases when data corruption > really happened? The races are rather rare methinks, you'd have to get a list-op concurrently with a DMA. If you get the list corrupted, I'm thinking the crash is fairly likely, albeit really difficuly to debug. > > No matter which way round you allocate devres and data, by necessity > > they're always going to consume the same total amount of memory. > > So then the next option I guess is to separate meta-data from data buffers > completely. Are there any reasons to not do that Dunno, should work just fine I think. > other than the hack we're > discussing here (meta-data in the beginning of the buffer) used to work OK-ish? If meta-data at the beginngin used to work, I don't see why meta-data at the end wouldn't work equally well. They'd be equally broken. But I'm still flabbergasted at the fact that they're doing non-coherent DMA to kmalloc memory, I thought we had a DMA api for that, with a special allocator and everything (but what do I know, I've never used that). _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [RFC PATCH v1] devres: align devres.data strictly only for devm_kmalloc() 2019-12-20 20:23 ` Peter Zijlstra @ 2019-12-20 21:02 ` Alexey Brodkin 2019-12-20 21:47 ` Dmitry Torokhov 0 siblings, 1 reply; 6+ messages in thread From: Alexey Brodkin @ 2019-12-20 21:02 UTC (permalink / raw) To: Peter Zijlstra Cc: Marc Gonzalez, Will Deacon, Vineet Gupta, Dmitry Torokhov, Rafael Wysocki, LKML, Bjorn Andersson, Russell King, Mark Brown, Greg Kroah-Hartman, Tejun Heo, arcml, Eugeniy Paltsev, Linux ARM, Robin Murphy Hi Peter, > > Well it somehow used to work for quite some time now with the data-buffer > > being allocated with 4 words offset (which is 16 bytes for 32-bit platform > > 3 words, devres_node is 3 words. Correct, but 4th word was implicitly there due to the fact on most of 32-bit arches "long long" is aligned by 2 words. > Which is exactly why we had to change it, the odd alignment caused ARC > to explode. I know that better than anybody else as it was my pain & grief :) > > and 32 for 64-bit which is still much less than mentioned 128 bytes). > > Or we just never managed to identify those rare cases when data corruption > > really happened? > > The races are rather rare methinks, you'd have to get a list-op > concurrently with a DMA. > > If you get the list corrupted, I'm thinking the crash is fairly likely, > albeit really difficuly to debug. So that alone IMHO is a good reason to not allow that thing to happen even in theory. > > > No matter which way round you allocate devres and data, by necessity > > > they're always going to consume the same total amount of memory. > > > > So then the next option I guess is to separate meta-data from data buffers > > completely. Are there any reasons to not do that > > Dunno, should work just fine I think. > > > other than the hack we're > > discussing here (meta-data in the beginning of the buffer) used to work OK-ish? > > If meta-data at the beginngin used to work, I don't see why meta-data at > the end wouldn't work equally well. They'd be equally broken. Agree. But if we imagine devm allocations are not used for DMA (which is yet another case of interface usage which was never designed for but alas this happens left and right) then move of the meta-data to the end of the buffers solves [mostly my] problem... but given that DMA case we discuss exists I'm not sure if this move actually worth spending time on. -Alexey _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v1] devres: align devres.data strictly only for devm_kmalloc() 2019-12-20 21:02 ` Alexey Brodkin @ 2019-12-20 21:47 ` Dmitry Torokhov 0 siblings, 0 replies; 6+ messages in thread From: Dmitry Torokhov @ 2019-12-20 21:47 UTC (permalink / raw) To: Alexey Brodkin Cc: Marc Gonzalez, Peter Zijlstra, Will Deacon, Vineet Gupta, Rafael Wysocki, LKML, Bjorn Andersson, Russell King, Mark Brown, Greg Kroah-Hartman, Tejun Heo, arcml, Eugeniy Paltsev, Linux ARM, Robin Murphy On Fri, Dec 20, 2019 at 09:02:24PM +0000, Alexey Brodkin wrote: > Hi Peter, > > > > Well it somehow used to work for quite some time now with the data-buffer > > > being allocated with 4 words offset (which is 16 bytes for 32-bit platform > > > > 3 words, devres_node is 3 words. > > Correct, but 4th word was implicitly there due to the fact > on most of 32-bit arches "long long" is aligned by 2 words. > > > Which is exactly why we had to change it, the odd alignment caused ARC > > to explode. > > I know that better than anybody else as it was my pain & grief :) > > > > and 32 for 64-bit which is still much less than mentioned 128 bytes). > > > Or we just never managed to identify those rare cases when data corruption > > > really happened? > > > > The races are rather rare methinks, you'd have to get a list-op > > concurrently with a DMA. > > > > If you get the list corrupted, I'm thinking the crash is fairly likely, > > albeit really difficuly to debug. > > So that alone IMHO is a good reason to not allow that thing to happen even > in theory. > > > > > No matter which way round you allocate devres and data, by necessity > > > > they're always going to consume the same total amount of memory. > > > > > > So then the next option I guess is to separate meta-data from data buffers > > > completely. Are there any reasons to not do that > > > > Dunno, should work just fine I think. > > > > > other than the hack we're > > > discussing here (meta-data in the beginning of the buffer) used to work OK-ish? > > > > If meta-data at the beginngin used to work, I don't see why meta-data at > > the end wouldn't work equally well. They'd be equally broken. No, not really. With data being ARCH_KMALLOC_MINALIGN and coming after the devres private stuff, given that the another allocation will also be aligned to ARCH_KMALLOC_MINALIGN (because that's what k*alloc will give us) we are guaranteed that DMA will not stomp onto any unrelated data. With devres private coming after data and not having any alignment constraints we may very well clobber it when doing DMA. BTW, I am not sure where the page size restriction you mentioned earlier is coming from. We have been using kmalloc()ed memory as buffers suitable for DMA since forever, and we only need to make sure such data is isolated from other data CPU might be accessing by ARCH_DMA_MINALIGN which is usually L1 cache size. From Documentation/DMA-API-HOWTO.txt: 2) ARCH_DMA_MINALIGN Architectures must ensure that kmalloc'ed buffer is DMA-safe. Drivers and subsystems depend on it. If an architecture isn't fully DMA-coherent (i.e. hardware doesn't ensure that data in the CPU cache is identical to data in main memory), ARCH_DMA_MINALIGN must be set so that the memory allocator makes sure that kmalloc'ed buffer doesn't share a cache line with the others. See arch/arm/include/asm/cache.h as an example. Note that ARCH_DMA_MINALIGN is about DMA memory alignment constraints. You don't need to worry about the architecture data alignment constraints (e.g. the alignment constraints about 64-bit objects). > > Agree. But if we imagine devm allocations are not used for DMA > (which is yet another case of interface usage which was never designed for > but alas this happens left and right) then move of the meta-data to the end of > the buffers solves [mostly my] problem... but given that DMA case we discuss > exists I'm not sure if this move actually worth spending time on. Well, there is a metric ton of devm users that do not allocate memory buffers, but other objects, and for which we do not need to worry about alignment. Thanks. -- Dmitry _______________________________________________ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-20 21:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <74ae22cd-08c1-d846-3e1d-cbc38db87442@free.fr> 2019-12-18 14:20 ` [RFC PATCH v1] devres: align devres.data strictly only for devm_kmalloc() Alexey Brodkin 2019-12-18 15:40 ` Marc Gonzalez [not found] ` <bf020a68-00fd-2bb7-c3b6-00f5befa293a@free.fr> [not found] ` <20191220140655.GN2827@hirez.programming.kicks-ass.net> [not found] ` <9be1d523-e92c-836b-b79d-37e880d092a0@arm.com> 2019-12-20 19:32 ` Alexey Brodkin 2019-12-20 20:23 ` Peter Zijlstra 2019-12-20 21:02 ` Alexey Brodkin 2019-12-20 21:47 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).