md/dm-ioctl.c: optimize memory allocation in copy_params

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 <[email protected]>

Signed-off-by yanmin.zhang <[email protected]>

---
 drivers/md/dm-ioctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/md/dm-ioctl.c +1 -1

index 5152142..31c3af9 100644
@@ -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;

Signature · 6 lines

1.7.9.5

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

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

Quoted 32 lines · xinhui.pan

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 <[email protected]>

Signed-off-by yanmin.zhang <[email protected]>

---
  drivers/md/dm-ioctl.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/md/dm-ioctl.c +1 -1

index 5152142..31c3af9 100644
@@ -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;

Signature · 6 lines

1.7.9.5

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Quoted 1 line · Mikulas Patocka

Hi

Mikulas,

Thanks for your kind comments.

Quoted 7 lines

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.

Quoted 1 line

Quoted 3 lines

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

Quoted 39 lines

Mikulas

Quoted 32 lines · xinhui.pan

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 <[email protected]>

Signed-off-by yanmin.zhang <[email protected]>

---
  drivers/md/dm-ioctl.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/md/dm-ioctl.c +1 -1

index 5152142..31c3af9 100644
@@ -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;

Signature · 6 lines

1.7.9.5

-- 
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Quoted 1 line · Mikulas Patocka

Hi

Mikulas,

Thanks for your kind comments.

Quoted 7 lines

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.

Quoted 4 lines

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

Quoted 14 lines · Yanmin

Quoted 1 line · Mikulas Patocka

Hi

Mikulas,

Thanks for your kind comments.

Quoted 7 lines

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.

Quoted 7 lines

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?

Quoted 3 lines

Quoted 1 line · Mikulas Patocka

Which ioctl with more than 16kB
arguments do you use?

Unanswered. Let's ask the same question in a different way:

Quoted 3 lines

Quoted 3 lines

Quoted 1 line · Mikulas Patocka

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

Quoted 1 line · Alasdair G Kergon

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?

Quoted 10 lines · Alasdair G Kergon

Quoted 3 lines

Quoted 3 lines

Quoted 1 line · Mikulas Patocka

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

Sorry for replying you too late. I am very busy in some other critical issues.

Quoted 2 lines

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.

Quoted 2 lines

__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.

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 8 lines

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.

So, it means that you do not use device mapper at all. So, why are you trying to change memory allocation in device mapper?

Quoted 2 lines · Mikulas Patocka

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

Quoted 14 lines · Alasdair G Kergon

Quoted 2 lines · Mikulas Patocka

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);

Quoted 7 lines

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.