* [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params @ 2014-07-04 10:22 xinhui.pan 2014-07-08 22:39 ` [dm-devel] " Mikulas Patocka 0 siblings, 1 reply; 15+ messages in thread From: xinhui.pan @ 2014-07-04 10:22 UTC (permalink / raw) To: linux-kernel, agk, snitzer, dm-devel; +Cc: yanmin_zhang, Liu, ShuoX KMALLOC_MAX_SIZE is too big, it easily cause memory fragmented and low memory when param_kernel->data_size is also big. That's not nice. So use vmalloc instead of kmalloc when size is larger than (PAGE_SIZE << 2). There are other drivers using kmalloc to gain a big size memory. And that cause our devices to hit hang and many worse issues. We don't benefit much when using kmalloc in such scenario. Signed-off-by: xinhui.pan <xinhuiX.pan@intel.com> Signed-off-by: yanmin.zhang <yanmin_zhang@linux.intel.com> --- drivers/md/dm-ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 5152142..31c3af9 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1709,7 +1709,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern * Use kmalloc() rather than vmalloc() when we can. */ dmi = NULL; - if (param_kernel->data_size <= KMALLOC_MAX_SIZE) { + if (param_kernel->data_size <= (PAGE_SIZE << 2)) { dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN); if (dmi) *param_flags |= DM_PARAMS_KMALLOC; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-04 10:22 [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params xinhui.pan @ 2014-07-08 22:39 ` Mikulas Patocka 2014-07-09 2:01 ` Zhang, Yanmin 0 siblings, 1 reply; 15+ messages in thread From: Mikulas Patocka @ 2014-07-08 22:39 UTC (permalink / raw) To: xinhui.pan; +Cc: linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX, yanmin_zhang Hi I don't really know what is the purpose of this patch. In existing device mapper code, if kmalloc fails, the allocation is retried with __vmalloc. So there is no need to avoid kmalloc aritifically. kmalloc doesn't cause memory fragmentation. If the memory is too fragmented, kmalloc fails. If it isn't, it succeeds. But it doesn't cause memory being fragmented. If you have some other driver that fails because of large kmalloc failure, you should fix it to use scatter-gather DMA or (if the hardware can't do it) preallocate the memory when the driver is loaded. Mikulas On Fri, 4 Jul 2014, xinhui.pan wrote: > KMALLOC_MAX_SIZE is too big, it easily cause memory fragmented and low memory when param_kernel->data_size > is also big. That's not nice. So use vmalloc instead of kmalloc when size is larger than (PAGE_SIZE << 2). > There are other drivers using kmalloc to gain a big size memory. And that cause our devices to hit hang and > many worse issues. We don't benefit much when using kmalloc in such scenario. > > Signed-off-by: xinhui.pan <xinhuiX.pan@intel.com> > Signed-off-by: yanmin.zhang <yanmin_zhang@linux.intel.com> > --- > drivers/md/dm-ioctl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > index 5152142..31c3af9 100644 > --- a/drivers/md/dm-ioctl.c > +++ b/drivers/md/dm-ioctl.c > @@ -1709,7 +1709,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern > * Use kmalloc() rather than vmalloc() when we can. > */ > dmi = NULL; > - if (param_kernel->data_size <= KMALLOC_MAX_SIZE) { > + if (param_kernel->data_size <= (PAGE_SIZE << 2)) { > dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN); > if (dmi) > *param_flags |= DM_PARAMS_KMALLOC; > -- > 1.7.9.5 > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-08 22:39 ` [dm-devel] " Mikulas Patocka @ 2014-07-09 2:01 ` Zhang, Yanmin 2014-07-09 3:37 ` xinhui.pan 2014-07-09 14:53 ` Mikulas Patocka 0 siblings, 2 replies; 15+ messages in thread From: Zhang, Yanmin @ 2014-07-09 2:01 UTC (permalink / raw) To: Mikulas Patocka, xinhui.pan Cc: linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On 2014/7/9 6:39, Mikulas Patocka wrote: > Hi Mikulas, Thanks for your kind comments. > I don't really know what is the purpose of this patch. In existing device > mapper code, if kmalloc fails, the allocation is retried with __vmalloc. > So there is no need to avoid kmalloc aritifically. > > kmalloc doesn't cause memory fragmentation. If the memory is too > fragmented, kmalloc fails. If it isn't, it succeeds. But it doesn't cause > memory being fragmented. I agree with you. The patch's original description is not appropriate. Basically, memory fragmentation is not caused by this kmalloc. The right description is: When memory is fragmented and most memory is used up, kmalloc a big memory might cause lots of OutOFMemory and system might kill lots of processes. Then, system might hang. We are enabling Android on mobiles and tablets. We hit OOM often at Monkey and other stress testing. Sometimes, kernel prints big memory allocation warning. Theoretically, it's a good idea to call kmalloc firstly. If it fails, use vmalloc. However, kernel assumes drivers do the right thing. When drivers call kmalloc to allocate a big memory, memory management would do the best to fulfill it. When memory is fragmented and most memory is used up, such allocation would cause big memory recollection. Some processes might be killed. The worse scenario is after a process is killed, it is restarted and allocates memory again. That causes system hang, or mobile users feel a big stall. We did fix a similar issue in one of our drivers. Usually, kernel and drivers can allocate big continuous memory at booting or initialization stage. After that, they need allocate small order memory. The best is to just allocate order-0 memory. > > If you have some other driver that fails because of large kmalloc failure, > you should fix it to use scatter-gather DMA or (if the hardware can't do > it) preallocate the memory when the driver is loaded. I agree with you. Here the patch fixes the issue, where dm might allocate big continuous memory after booting. Monkey testing triggers it by operating menu Setting=>Security=>InstallfromSDcard. We are catching all places in kernel where big memory allocation happens. This patch is just to fix one case. Thanks, Yanmin > > Mikulas > > > > On Fri, 4 Jul 2014, xinhui.pan wrote: > >> KMALLOC_MAX_SIZE is too big, it easily cause memory fragmented and low memory when param_kernel->data_size >> is also big. That's not nice. So use vmalloc instead of kmalloc when size is larger than (PAGE_SIZE << 2). >> There are other drivers using kmalloc to gain a big size memory. And that cause our devices to hit hang and >> many worse issues. We don't benefit much when using kmalloc in such scenario. >> >> Signed-off-by: xinhui.pan <xinhuiX.pan@intel.com> >> Signed-off-by: yanmin.zhang <yanmin_zhang@linux.intel.com> >> --- >> drivers/md/dm-ioctl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c >> index 5152142..31c3af9 100644 >> --- a/drivers/md/dm-ioctl.c >> +++ b/drivers/md/dm-ioctl.c >> @@ -1709,7 +1709,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern >> * Use kmalloc() rather than vmalloc() when we can. >> */ >> dmi = NULL; >> - if (param_kernel->data_size <= KMALLOC_MAX_SIZE) { >> + if (param_kernel->data_size <= (PAGE_SIZE << 2)) { >> dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN); >> if (dmi) >> *param_flags |= DM_PARAMS_KMALLOC; >> -- >> 1.7.9.5 >> >> -- >> dm-devel mailing list >> dm-devel@redhat.com >> https://www.redhat.com/mailman/listinfo/dm-devel >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-09 2:01 ` Zhang, Yanmin @ 2014-07-09 3:37 ` xinhui.pan 2014-07-09 14:53 ` Mikulas Patocka 1 sibling, 0 replies; 15+ messages in thread From: xinhui.pan @ 2014-07-09 3:37 UTC (permalink / raw) To: Zhang, Yanmin, Mikulas Patocka Cc: linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX 于 2014年07月09日 10:01, Zhang, Yanmin 写道: > On 2014/7/9 6:39, Mikulas Patocka wrote: > >> Hi > > Mikulas, > > Thanks for your kind comments. > >> I don't really know what is the purpose of this patch. In existing device >> mapper code, if kmalloc fails, the allocation is retried with __vmalloc. >> So there is no need to avoid kmalloc aritifically. >> >> kmalloc doesn't cause memory fragmentation. If the memory is too >> fragmented, kmalloc fails. If it isn't, it succeeds. But it doesn't cause >> memory being fragmented. > > I agree with you. The patch's original description is not appropriate. > Basically, memory fragmentation is not caused by this kmalloc. > > The right description is: When memory is fragmented and most memory is used up, > kmalloc a big memory might cause lots of OutOFMemory and system might kill > lots of processes. Then, system might hang. > > We are enabling Android on mobiles and tablets. We hit OOM often at > Monkey and other stress testing. Sometimes, kernel prints big memory allocation > warning. > > Theoretically, it's a good idea to call kmalloc firstly. If it fails, use vmalloc. > However, kernel assumes drivers do the right thing. When drivers call kmalloc to > allocate a big memory, memory management would do the best to fulfill it. When memory > is fragmented and most memory is used up, such allocation would cause big memory > recollection. Some processes might be killed. The worse scenario is after a process > is killed, it is restarted and allocates memory again. That causes system hang, > or mobile users feel a big stall. We did fix a similar issue in one of our drivers. > > Usually, kernel and drivers can allocate big continuous memory at booting or > initialization stage. After that, they need allocate small order memory. The best > is to just allocate order-0 memory. > > >> >> If you have some other driver that fails because of large kmalloc failure, >> you should fix it to use scatter-gather DMA or (if the hardware can't do >> it) preallocate the memory when the driver is loaded. > > I agree with you. Here the patch fixes the issue, where dm might allocate > big continuous memory after booting. Monkey testing triggers it by operating > menu Setting=>Security=>InstallfromSDcard. > hi, Yanmin && Mikulas Thanks for your nice comments. And sorry for confusing you as my comment is not clear enough. In android, there is a command "dumpstate", it run many other commands to collect information. In our scenario, it run command "vdc dump", and vdc uses socket to pass some parameters to "vold", then vold generates ioctl. thanks. > We are catching all places in kernel where big memory allocation happens. > This patch is just to fix one case. > > Thanks, > Yanmin > >> >> Mikulas >> >> >> >> On Fri, 4 Jul 2014, xinhui.pan wrote: >> >>> KMALLOC_MAX_SIZE is too big, it easily cause memory fragmented and low memory when param_kernel->data_size >>> is also big. That's not nice. So use vmalloc instead of kmalloc when size is larger than (PAGE_SIZE << 2). >>> There are other drivers using kmalloc to gain a big size memory. And that cause our devices to hit hang and >>> many worse issues. We don't benefit much when using kmalloc in such scenario. >>> >>> Signed-off-by: xinhui.pan <xinhuiX.pan@intel.com> >>> Signed-off-by: yanmin.zhang <yanmin_zhang@linux.intel.com> >>> --- >>> drivers/md/dm-ioctl.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c >>> index 5152142..31c3af9 100644 >>> --- a/drivers/md/dm-ioctl.c >>> +++ b/drivers/md/dm-ioctl.c >>> @@ -1709,7 +1709,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern >>> * Use kmalloc() rather than vmalloc() when we can. >>> */ >>> dmi = NULL; >>> - if (param_kernel->data_size <= KMALLOC_MAX_SIZE) { >>> + if (param_kernel->data_size <= (PAGE_SIZE << 2)) { >>> dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN); >>> if (dmi) >>> *param_flags |= DM_PARAMS_KMALLOC; >>> -- >>> 1.7.9.5 >>> >>> -- >>> dm-devel mailing list >>> dm-devel@redhat.com >>> https://www.redhat.com/mailman/listinfo/dm-devel >>> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-09 2:01 ` Zhang, Yanmin 2014-07-09 3:37 ` xinhui.pan @ 2014-07-09 14:53 ` Mikulas Patocka 2014-07-22 1:02 ` Zhang, Yanmin 1 sibling, 1 reply; 15+ messages in thread From: Mikulas Patocka @ 2014-07-09 14:53 UTC (permalink / raw) To: Zhang, Yanmin Cc: xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On Wed, 9 Jul 2014, Zhang, Yanmin wrote: > On 2014/7/9 6:39, Mikulas Patocka wrote: > > > Hi > > Mikulas, > > Thanks for your kind comments. > > > I don't really know what is the purpose of this patch. In existing device > > mapper code, if kmalloc fails, the allocation is retried with __vmalloc. > > So there is no need to avoid kmalloc aritifically. > > > > kmalloc doesn't cause memory fragmentation. If the memory is too > > fragmented, kmalloc fails. If it isn't, it succeeds. But it doesn't cause > > memory being fragmented. > > I agree with you. The patch's original description is not appropriate. > Basically, memory fragmentation is not caused by this kmalloc. > > The right description is: When memory is fragmented and most memory is used > up, > kmalloc a big memory might cause lots of OutOFMemory and system might kill > lots of processes. Then, system might hang. The question is - does this particular kmalloc in device mapper cause out of memory or killing of other tasks? It has flags __GFP_NORETRY, __GFP_NOMEMALLOC, __GFP_NOWARN so it shouldn't cause any trouble. It should just fail silently if memory is fragmented. Do you have some stacktrace that identifies this kmalloc as a problem? Do this test - prepare two kernels that are identical, except that one kernel has that one-line change in dm-ioctl. Boot each kernel 10 times, do exactly the same operation after boot. Does the kernel with the patch always behave correctly and does the kernel without the patch always fail? Report the result - how many failures did you get with or without that one-line patch. Without such a test - I just don't believe that your patch makes any difference. Another question - your patch only makes change if some device mapper ioctl has more than 16kB arugments. Which ioctl with more than 16kB arguments do you use? Do you load such a big table to device mapper? How often do you call that ioctl with such big arguments? Mikulas > We are enabling Android on mobiles and tablets. We hit OOM often at > Monkey and other stress testing. Sometimes, kernel prints big memory > allocation warning. > > Theoretically, it's a good idea to call kmalloc firstly. If it fails, > use vmalloc. However, kernel assumes drivers do the right thing. When > drivers call kmalloc to allocate a big memory, memory management would > do the best to fulfill it. When memory is fragmented and most memory is > used up, such allocation would cause big memory recollection. Some > processes might be killed. The worse scenario is after a process is > killed, it is restarted and allocates memory again. That causes system > hang, or mobile users feel a big stall. We did fix a similar issue in > one of our drivers. > > Usually, kernel and drivers can allocate big continuous memory at > booting or initialization stage. After that, they need allocate small > order memory. The best is to just allocate order-0 memory. > > > > > > If you have some other driver that fails because of large kmalloc failure, > > you should fix it to use scatter-gather DMA or (if the hardware can't do > > it) preallocate the memory when the driver is loaded. > > I agree with you. Here the patch fixes the issue, where dm might allocate > big continuous memory after booting. Monkey testing triggers it by operating > menu Setting=>Security=>InstallfromSDcard. > > We are catching all places in kernel where big memory allocation happens. > This patch is just to fix one case. > > Thanks, > Yanmin ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-09 14:53 ` Mikulas Patocka @ 2014-07-22 1:02 ` Zhang, Yanmin 2014-07-22 1:23 ` Alasdair G Kergon 2014-07-23 12:39 ` Mikulas Patocka 0 siblings, 2 replies; 15+ messages in thread From: Zhang, Yanmin @ 2014-07-22 1:02 UTC (permalink / raw) To: Mikulas Patocka Cc: xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On 2014/7/9 22:53, Mikulas Patocka wrote: > > On Wed, 9 Jul 2014, Zhang, Yanmin wrote: > >> On 2014/7/9 6:39, Mikulas Patocka wrote: >> >>> Hi >> Mikulas, >> >> Thanks for your kind comments. >> >>> I don't really know what is the purpose of this patch. In existing device >>> mapper code, if kmalloc fails, the allocation is retried with __vmalloc. >>> So there is no need to avoid kmalloc aritifically. >>> >>> kmalloc doesn't cause memory fragmentation. If the memory is too >>> fragmented, kmalloc fails. If it isn't, it succeeds. But it doesn't cause >>> memory being fragmented. >> I agree with you. The patch's original description is not appropriate. >> Basically, memory fragmentation is not caused by this kmalloc. >> >> The right description is: When memory is fragmented and most memory is used >> up, >> kmalloc a big memory might cause lots of OutOFMemory and system might kill >> lots of processes. Then, system might hang. Sorry for replying you too late. I am very busy in some other critical issues. > The question is - does this particular kmalloc in device mapper cause out > of memory or killing of other tasks? It has flags __GFP_NORETRY, When memory is fragmented, drivers need allocate small pages instead of big memory. Even with __GFP_NORETRY, driver might get a big memory by luck. That means other drivers would get fewer chances to fulfill their memory requests, such like allocating 2 pages for task_struct. Later on, OOM might happen. > __GFP_NOMEMALLOC, __GFP_NOWARN so it shouldn't cause any trouble. It > should just fail silently if memory is fragmented. It's hard to say this call causes out of memory. There are many such places in kernel to allocate big continuous memory. One is in seq_read, where we created a patch to use vmalloc instead of kmalloc to fix it, but got far worse comments as it's very old code. Another is in our own gfx driver. We want to fix all. We can't blame the OOM to just one place. Monkey testing is popular for Android development. We run the testing frequently. It might start lots of applications. Eventually, it is a comprehensive testing. > > Do you have some stacktrace that identifies this kmalloc as a problem? Sometimes, when OOM happens, kernel log shows some backtrace of big continuous memory allocation failure. Sometimes, when board can't respond and watchdog might reset the board after saving thread callchain into disk. > > Do this test - prepare two kernels that are identical, except that one > kernel has that one-line change in dm-ioctl. Boot each kernel 10 times, do > exactly the same operation after boot. Does the kernel with the patch > always behave correctly and does the kernel without the patch always fail? No. Instead of just one, many places have impact on the OOM issue. > Report the result - how many failures did you get with or without that > one-line patch. Without such a test - I just don't believe that your patch > makes any difference. > > Another question - your patch only makes change if some device mapper > ioctl has more than 16kB arugments. Which ioctl with more than 16kB > arguments do you use? Do you load such a big table to device mapper? How > often do you call that ioctl with such big arguments? Xinhui's email mentions the ioctl details. In android, there is a command "dumpstate", it run many other commands to collect information. In our scenario, it run command "vdc dump", and vdc uses socket to pass some parameters to "vold", then vold generates ioctl. Thanks for your patience. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-22 1:02 ` Zhang, Yanmin @ 2014-07-22 1:23 ` Alasdair G Kergon 2014-07-22 2:04 ` Alasdair G Kergon 2014-07-23 3:06 ` Zhang, Yanmin 2014-07-23 12:39 ` Mikulas Patocka 1 sibling, 2 replies; 15+ messages in thread From: Alasdair G Kergon @ 2014-07-22 1:23 UTC (permalink / raw) To: Zhang, Yanmin Cc: Mikulas Patocka, xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX >>> On 2014/7/9 6:39, Mikulas Patocka wrote: >> Which ioctl with more than 16kB >> arguments do you use? Unanswered. Let's ask the same question in a different way: Please supply the output of these three commands on the real-world system on which you believe that that particular allocation is causing you a problem: dmsetup info -c dmsetup table dmsetup status Alasdair ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-22 1:23 ` Alasdair G Kergon @ 2014-07-22 2:04 ` Alasdair G Kergon 2014-07-23 3:15 ` Zhang, Yanmin 2014-07-23 3:06 ` Zhang, Yanmin 1 sibling, 1 reply; 15+ messages in thread From: Alasdair G Kergon @ 2014-07-22 2:04 UTC (permalink / raw) To: Zhang, Yanmin, Mikulas Patocka, xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On Tue, Jul 22, 2014 at 02:23:52AM +0100, Alasdair G Kergon wrote: > Unanswered. Let's ask the same question in a different way: A quick search for 'vold' returns: https://android.googlesource.com/platform/system/vold/ and the code there requests a fixed 64K allocation to hold the names of active volumes. So unlike libdevmapper-based applications where a smaller allocation is used at first and only extended if needed, Android just assumes that 64KB is enough for everyone. So is your claim that: 1. This 64KB allocation for the brief duration of the ioctl to store the names of active device-mapper volumes leads to memory problems? [Mustn't the system *already* be in a bad state if this pushes it over the limit?] and 2. The systems on which this memory shortage occurs have so many volumes (with long names?) that a smaller allocation would not suffice? Alasdair ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-22 2:04 ` Alasdair G Kergon @ 2014-07-23 3:15 ` Zhang, Yanmin 0 siblings, 0 replies; 15+ messages in thread From: Zhang, Yanmin @ 2014-07-23 3:15 UTC (permalink / raw) To: Mikulas Patocka, xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On 2014/7/22 10:04, Alasdair G Kergon wrote: > On Tue, Jul 22, 2014 at 02:23:52AM +0100, Alasdair G Kergon wrote: >> Unanswered. Let's ask the same question in a different way: > > A quick search for 'vold' returns: > https://android.googlesource.com/platform/system/vold/ > and the code there requests a fixed 64K allocation to hold the names of > active volumes. > > So unlike libdevmapper-based applications where a smaller allocation is > used at first and only extended if needed, Android just assumes that > 64KB is enough for everyone. > > So is your claim that: > > 1. This 64KB allocation for the brief duration of the ioctl to store the > names of active device-mapper volumes leads to memory problems? > [Mustn't the system *already* be in a bad state if this pushes it over > the limit?] It's a good question. 1) Usually, Android mobile runs for a long time. It's very command that users don't turn off the phones for months. Users might start lots of applications. memory is used up in the end. Kernel might recollect memory over and over again. 2) We never blames this Out of memory issue fully to DM. 3) We want to improve the OOM issue. > > and > > 2. The systems on which this memory shortage occurs have so many volumes > (with long names?) that a smaller allocation would not suffice? 64K is small, comparing with 2GB, even 4GB total memory. However, this 64K by kmalloc has a strict criteria. It might be continuous physical memory and align with 64K. If memory is used up, freed memory is very fragmented. Such 64K criteria is a hard request. Usually, driver can allocate such memory at booting initialization. After that, it should allocate many sparse pages instead of continuous memory. Here 64K allocation is after booting. Thanks for the kind comments. Yanmin ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-22 1:23 ` Alasdair G Kergon 2014-07-22 2:04 ` Alasdair G Kergon @ 2014-07-23 3:06 ` Zhang, Yanmin 2014-07-23 12:16 ` Mikulas Patocka 1 sibling, 1 reply; 15+ messages in thread From: Zhang, Yanmin @ 2014-07-23 3:06 UTC (permalink / raw) To: Mikulas Patocka, xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On 2014/7/22 9:23, Alasdair G Kergon wrote: >>>> On 2014/7/9 6:39, Mikulas Patocka wrote: >>> Which ioctl with more than 16kB >>> arguments do you use? > Unanswered. Let's ask the same question in a different way: > > Please supply the output of these three commands on the real-world system on > which you believe that that particular allocation is causing you a problem: > dmsetup info -c > dmsetup table > dmsetup status Android doesn't include the command. We compiled lvm2-2_02_107.tar.gz and copy dmsetup to the board. But command reports No devices. # /data/bin/dmsetup info No devices found # cat /proc/devices Character devices: 1 mem 4 /dev/vc/0 4 tty 4 ttyS 4 ttyMFD 5 /dev/tty 5 /dev/console 5 /dev/ptmx 7 vcs 10 misc 13 input 21 sg 29 fb 81 video4linux 89 i2c 108 ppp 116 alsa 128 ptm 136 pts 153 spi 166 ttyACM 180 usb 188 ttyUSB 189 usb_device 202 cpu/msr 203 cpu/cpuid 226 drm 241 mdm_ctrl 242 sep54 243 roccat 244 hidraw 245 ttyGS 246 usbmon 247 ttyPTI 248 gsmtty 249 bsg 250 iio 251 ptp 252 pps 253 media 254 rtc Block devices: 1 ramdisk 259 blkext 7 loop 8 sd 9 md 11 sr 65 sd 66 sd 67 sd 68 sd 69 sd 70 sd 71 sd 128 sd 129 sd 130 sd 131 sd 132 sd 133 sd 134 sd 135 sd 179 mmc 253 device-mapper 254 mdp ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-23 3:06 ` Zhang, Yanmin @ 2014-07-23 12:16 ` Mikulas Patocka 2014-07-23 12:54 ` Alasdair G Kergon 0 siblings, 1 reply; 15+ messages in thread From: Mikulas Patocka @ 2014-07-23 12:16 UTC (permalink / raw) To: Zhang, Yanmin Cc: xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On Wed, 23 Jul 2014, Zhang, Yanmin wrote: > On 2014/7/22 9:23, Alasdair G Kergon wrote: > > > > > On 2014/7/9 6:39, Mikulas Patocka wrote: > > > > Which ioctl with more than 16kB > > > > arguments do you use? > > Unanswered. Let's ask the same question in a different way: > > > > Please supply the output of these three commands on the real-world system on > > which you believe that that particular allocation is causing you a problem: > > dmsetup info -c > > dmsetup table > > dmsetup status > > Android doesn't include the command. We compiled lvm2-2_02_107.tar.gz and copy > dmsetup to the board. But command reports No devices. > > # /data/bin/dmsetup info > No devices found So, it means that you do not use device mapper at all. So, why are you trying to change memory allocation in device mapper? Mikulas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-23 12:16 ` Mikulas Patocka @ 2014-07-23 12:54 ` Alasdair G Kergon 2014-07-23 17:14 ` Mikulas Patocka 0 siblings, 1 reply; 15+ messages in thread From: Alasdair G Kergon @ 2014-07-23 12:54 UTC (permalink / raw) To: Mikulas Patocka, Zhang, Yanmin, xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On Wed, Jul 23, 2014 at 08:16:58AM -0400, Mikulas Patocka wrote: > So, it means that you do not use device mapper at all. So, why are you > trying to change memory allocation in device mapper? So the *test* they run is asking device-mapper to briefly reserve a 64KB buffer when there is no data to report: The answer is not to run that pointless test:) And if a single 64KB allocation really is a big deal, then patch 'vold' in userspace so it doesn't ask for 64KB when it clearly doesn't need it! + int Devmapper::dumpState(SocketClient *c) { + char *buffer = (char *) malloc(1024 * 64); The code has just #define DEVMAPPER_BUFFER_SIZE 4096 for all the other dm ioctls it issues. Only use a larger value when it is needed i.e. if DM_BUFFER_FULL_FLAG gets set. Alasdair ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-23 12:54 ` Alasdair G Kergon @ 2014-07-23 17:14 ` Mikulas Patocka 2014-07-24 5:53 ` Zhang, Yanmin 0 siblings, 1 reply; 15+ messages in thread From: Mikulas Patocka @ 2014-07-23 17:14 UTC (permalink / raw) To: Alasdair G Kergon Cc: Zhang, Yanmin, xinhui.pan, linux-kernel, snitzer, dm-devel, Liu, ShuoX On Wed, 23 Jul 2014, Alasdair G Kergon wrote: > On Wed, Jul 23, 2014 at 08:16:58AM -0400, Mikulas Patocka wrote: > > So, it means that you do not use device mapper at all. So, why are you > > trying to change memory allocation in device mapper? > > So the *test* they run is asking device-mapper to briefly reserve a 64KB > buffer when there is no data to report: The answer is not to run that > pointless test:) > > And if a single 64KB allocation really is a big deal, then patch 'vold' > in userspace so it doesn't ask for 64KB when it clearly doesn't need it! > > + int Devmapper::dumpState(SocketClient *c) { > + char *buffer = (char *) malloc(1024 * 64); > > The code has just > #define DEVMAPPER_BUFFER_SIZE 4096 > for all the other dm ioctls it issues. > > Only use a larger value when it is needed i.e. if DM_BUFFER_FULL_FLAG gets set. > > Alasdair Device mapper shouldn't depend on allocation on any contiguous memory - it will fall back to vmalloc. I still can't believe that their suggested patch makes any difference. This pattern is being repeated over and over in the kernel - for example: if (PIDLIST_TOO_LARGE(count)) return vmalloc(count * sizeof(pid_t)); else return kmalloc(count * sizeof(pid_t), GFP_KERNEL); if (is_vmalloc_addr(p)) vfree(p); else kfree(p); - I think we should make two functions that do this operation (for example kvalloc and kvfree) and convert device mapper and other users to these functions. Then, other kernel subsystems can start to use them to fix memory fragmentation issues. Mikulas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-23 17:14 ` Mikulas Patocka @ 2014-07-24 5:53 ` Zhang, Yanmin 0 siblings, 0 replies; 15+ messages in thread From: Zhang, Yanmin @ 2014-07-24 5:53 UTC (permalink / raw) To: Mikulas Patocka, Alasdair G Kergon Cc: xinhui.pan, linux-kernel, snitzer, dm-devel, Liu, ShuoX On 2014/7/24 1:14, Mikulas Patocka wrote: > > On Wed, 23 Jul 2014, Alasdair G Kergon wrote: > >> On Wed, Jul 23, 2014 at 08:16:58AM -0400, Mikulas Patocka wrote: >>> So, it means that you do not use device mapper at all. So, why are you >>> trying to change memory allocation in device mapper? >> >> So the *test* they run is asking device-mapper to briefly reserve a 64KB >> buffer when there is no data to report: The answer is not to run that >> pointless test:) >> >> And if a single 64KB allocation really is a big deal, then patch 'vold' >> in userspace so it doesn't ask for 64KB when it clearly doesn't need it! >> >> + int Devmapper::dumpState(SocketClient *c) { >> + char *buffer = (char *) malloc(1024 * 64); >> >> The code has just >> #define DEVMAPPER_BUFFER_SIZE 4096 >> for all the other dm ioctls it issues. >> >> Only use a larger value when it is needed i.e. if DM_BUFFER_FULL_FLAG gets set. >> >> Alasdair > Device mapper shouldn't depend on allocation on any contiguous memory - it > will fall back to vmalloc. I still can't believe that their suggested > patch makes any difference. > > This pattern is being repeated over and over in the kernel - for example: > > if (PIDLIST_TOO_LARGE(count)) > return vmalloc(count * sizeof(pid_t)); > else > return kmalloc(count * sizeof(pid_t), GFP_KERNEL); > > > if (is_vmalloc_addr(p)) > vfree(p); > else > kfree(p); > > - I think we should make two functions that do this operation (for example > kvalloc and kvfree) and convert device mapper and other users to these > functions. Then, other kernel subsystems can start to use them to fix > memory fragmentation issues. Thank Mikulas and Alasdair. Before sending out the patch, we know the result. :) It's hard to balance between performance and stability. Anyway, we would try to change seq_read. Yanmin ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dm-devel] [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params 2014-07-22 1:02 ` Zhang, Yanmin 2014-07-22 1:23 ` Alasdair G Kergon @ 2014-07-23 12:39 ` Mikulas Patocka 1 sibling, 0 replies; 15+ messages in thread From: Mikulas Patocka @ 2014-07-23 12:39 UTC (permalink / raw) To: Zhang, Yanmin Cc: xinhui.pan, linux-kernel, agk, snitzer, dm-devel, Liu, ShuoX On Tue, 22 Jul 2014, Zhang, Yanmin wrote: > Sorry for replying you too late. I am very busy in some other critical issues. > > > The question is - does this particular kmalloc in device mapper cause out > > of memory or killing of other tasks? It has flags __GFP_NORETRY, > > When memory is fragmented, drivers need allocate small pages instead of > big memory. Even with __GFP_NORETRY, driver might get a big memory by > luck. That means other drivers would get fewer chances to fulfill their > memory requests, such like allocating 2 pages for task_struct. Later on, > OOM might happen. You claim that that big kmalloc causes memory fragmentation. But memory can get fragmented even if no big kmalloc is used. For example, this program will cause memory fragmentation despite the fact that it never does any multi-page allocation: int main(void) { int i; char *array[65536]; for (i = 0; i < 65536; i++) { array[i] = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); if (array[i] == MAP_FAILED) perror("mmap"), exit(1); array[i][0] = 3; } for (i = 0; i < 65536; i += 2) { if (munmap(array[i], 4096)) perror("munmap"), exit(1); } pause(); return 0; } If you have problems with memory fragmentation - find the piece of code that is failing because of fragmented memory and fix it. To fix it: * if it is DMA memory for device * use continuous memory allocator, or * preallocate the chunk of memory when the driver is loaded and never free it * if it is general memory allocation with kmalloc * change the algorithm, so that it doesn't require big allocation * use the same trick as device mapper - try kmalloc and if it fails, try vmalloc. > > __GFP_NOMEMALLOC, __GFP_NOWARN so it shouldn't cause any trouble. It > > should just fail silently if memory is fragmented. > > It's hard to say this call causes out of memory. There are many such places > in kernel to allocate big continuous memory. One is in seq_read, where we > created a patch to use vmalloc instead of kmalloc to fix it, but got far > worse comments as it's very old code. Another is in our own gfx driver. > We want to fix all. We can't blame the OOM to just one place. vmalloc is much slower than kmalloc, so you can't just go over the Linux source and change every large kmalloc to vmalloc. You can change seq_read to use the same trick as device mapper (use kmalloc and if it fails, fall back to vmalloc). > Monkey testing is popular for Android development. We run the testing > frequently. It might start lots of applications. Eventually, it is a > comprehensive testing. I ask again - do you have some statistically significant test results that show that your patch makes any difference to stability? I suppose, no... > > Do you have some stacktrace that identifies this kmalloc as a problem? > > Sometimes, when OOM happens, kernel log shows some backtrace of big > continuous memory allocation failure. Sometimes, when board can't > respond and watchdog might reset the board after saving thread callchain > into disk. Find places, where the OOM happens (those, that you see on your stacktrace) and fix them. > > Do this test - prepare two kernels that are identical, except that one > > kernel has that one-line change in dm-ioctl. Boot each kernel 10 times, do > > exactly the same operation after boot. Does the kernel with the patch > > always behave correctly and does the kernel without the patch always fail? > > No. Instead of just one, many places have impact on the OOM issue. I repeat again - find the piece of code that is failing because of fragmented memory and fix it. Device mapper isn't failing (because it falls back to vmalloc), so leave it alone. > > Report the result - how many failures did you get with or without that > > one-line patch. Without such a test - I just don't believe that your patch > > makes any difference. > > > > Another question - your patch only makes change if some device mapper > > ioctl has more than 16kB arugments. Which ioctl with more than 16kB > > arguments do you use? Do you load such a big table to device mapper? How > > often do you call that ioctl with such big arguments? > > Xinhui's email mentions the ioctl details. In android, there is a command > "dumpstate", it run many other commands to collect information. In our > scenario, it run command "vdc dump", and vdc uses socket to pass some > parameters to "vold", then vold generates ioctl. > > Thanks for your patience. Mikulas ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-07-24 5:54 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-07-04 10:22 [PATCH] md/dm-ioctl.c: optimize memory allocation in copy_params xinhui.pan 2014-07-08 22:39 ` [dm-devel] " Mikulas Patocka 2014-07-09 2:01 ` Zhang, Yanmin 2014-07-09 3:37 ` xinhui.pan 2014-07-09 14:53 ` Mikulas Patocka 2014-07-22 1:02 ` Zhang, Yanmin 2014-07-22 1:23 ` Alasdair G Kergon 2014-07-22 2:04 ` Alasdair G Kergon 2014-07-23 3:15 ` Zhang, Yanmin 2014-07-23 3:06 ` Zhang, Yanmin 2014-07-23 12:16 ` Mikulas Patocka 2014-07-23 12:54 ` Alasdair G Kergon 2014-07-23 17:14 ` Mikulas Patocka 2014-07-24 5:53 ` Zhang, Yanmin 2014-07-23 12:39 ` Mikulas Patocka
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).