| Message ID | 20131215192508.GA10514@psi-dev26.jf.intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Sun 2013-12-15 11:25:08, David Cohen wrote: > On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote: > > On Thu 2013-12-12 21:18:23, David Cohen wrote: > > > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more > > > smart. > > > > > > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid > > > setting the callbacks when such #ifdef's aren't defined, they don't > > > handle compiler to avoid messages like that: > > > > > > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function] > > > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function] > > > > > > As result, those macros get rid of #ifdef's when setting callbacks but > > > not when implementing them. > > > > > > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef > > > the callbacks implementation as well. > > > > Well... Interesting trickery, but it means that resulting kernel > > will be bigge due to the dead functions no? > > Actually, it doesn't get bigger. Before sending the patch I did this > dummy test app: > > -------------------------------------------------------------------------------- > #include <stdio.h> > > #define USE_IT_OR_LOOSE_IT(fn) ((void *)((unsigned long)(fn) - (unsigned long)(fn))) > > #ifdef MAKE_ME_NULL > static int func1(int a) > { > printf("Hey!!\n"); > return 0; > } > #endif I thought that point of this patch series was getting rid of the #ifdefs around the function...? Now I'm confused. > struct global_data { > int (*func)(int); > }; > > static struct global_data gd = { > #ifdef MAKE_ME_NULL > .func = USE_IT_OR_LOOSE_IT(func1), If you have ifdef around the function, why do you need magic here? Why not .func = func1 ? Basically the warning tells you that you want the ifdef around the function, too... (Otherwise you waste space). That seems like good warning. Pavel
On Fri, Dec 20, 2013 at 08:55:27PM +0100, Pavel Machek wrote: > On Sun 2013-12-15 11:25:08, David Cohen wrote: > > On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote: > > > On Thu 2013-12-12 21:18:23, David Cohen wrote: > > > > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more > > > > smart. > > > > > > > > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid > > > > setting the callbacks when such #ifdef's aren't defined, they don't > > > > handle compiler to avoid messages like that: > > > > > > > > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function] > > > > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function] > > > > > > > > As result, those macros get rid of #ifdef's when setting callbacks but > > > > not when implementing them. > > > > > > > > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef > > > > the callbacks implementation as well. > > > > > > Well... Interesting trickery, but it means that resulting kernel > > > will be bigge due to the dead functions no? > > > > Actually, it doesn't get bigger. Before sending the patch I did this > > dummy test app: > > > > -------------------------------------------------------------------------------- > > #include <stdio.h> > > > > #define USE_IT_OR_LOOSE_IT(fn) ((void *)((unsigned long)(fn) - (unsigned long)(fn))) > > > > #ifdef MAKE_ME_NULL > > static int func1(int a) > > { > > printf("Hey!!\n"); > > return 0; > > } > > #endif > > I thought that point of this patch series was getting rid of the > #ifdefs around the function...? Now I'm confused. Maybe you're misinterpreting the test :) This #ifdef is used to make this same test code to replicate both scenarios according to -DMAKE_ME_NULL (just pay attention to actual resulting code after #ifdef's are tested. the #ifdef here is nor related to actual #ifdef on kernel). Here are both scenarios: (1) Not using my trickery (which needs the function to not be present). (2) Using my trickery (which needs to function to stay). With -DMAKE_ME_NULL we replicate (2), then the function *is* there but gcc gets rid of it on resulting binary without warnings if used with -O2. Without -DMAKE_ME_NULL we replicate (1). The #ifdef will fail and then remove the function which is an obvious scenario the function won't be part of resulting binary. If we use -S option to have human readable resulting assembly code (which is kind of 1:1 for resulting binary), we can compare the result of (1) and (2) and check they are pretty similar. This proves gcc behaves as expected with my patch: do not need #ifdef and do not generate dead codes to resulting binary. > > > struct global_data { > > int (*func)(int); > > }; > > > > static struct global_data gd = { > > #ifdef MAKE_ME_NULL > > .func = USE_IT_OR_LOOSE_IT(func1), > > If you have ifdef around the function, why do you need magic here? Why > not This #ifdef is necessary to prevent the function to be used when it doesn't exist due to above #ifdef. But once again: don't misinterpret the #ifdefs in this test app with the ones in kernel. They are not related at all. If it's still confusing you just make 2 test apps without #ifdeds out of this one where one keeps the code inside #ifdefs and the other doesn't. > > .func = func1 > > ? > > Basically the warning tells you that you want the ifdef around the > function, too... (Otherwise you waste space). That seems like good > warning. Just check my first explanation. Br, David Cohen > > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 20, 2013 at 12:23:36PM -0800, David Cohen wrote: > On Fri, Dec 20, 2013 at 08:55:27PM +0100, Pavel Machek wrote: > > On Sun 2013-12-15 11:25:08, David Cohen wrote: > > > On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote: > > > > On Thu 2013-12-12 21:18:23, David Cohen wrote: > > > > > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more > > > > > smart. > > > > > > > > > > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid > > > > > setting the callbacks when such #ifdef's aren't defined, they don't > > > > > handle compiler to avoid messages like that: > > > > > > > > > > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function] > > > > > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function] > > > > > > > > > > As result, those macros get rid of #ifdef's when setting callbacks but > > > > > not when implementing them. > > > > > > > > > > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef > > > > > the callbacks implementation as well. > > > > > > > > Well... Interesting trickery, but it means that resulting kernel > > > > will be bigge due to the dead functions no? > > > > > > Actually, it doesn't get bigger. Before sending the patch I did this > > > dummy test app: > > > > > > -------------------------------------------------------------------------------- > > > #include <stdio.h> > > > > > > #define USE_IT_OR_LOOSE_IT(fn) ((void *)((unsigned long)(fn) - (unsigned long)(fn))) > > > > > > #ifdef MAKE_ME_NULL > > > static int func1(int a) > > > { > > > printf("Hey!!\n"); > > > return 0; > > > } > > > #endif > > > > I thought that point of this patch series was getting rid of the > > #ifdefs around the function...? Now I'm confused. > > Maybe you're misinterpreting the test :) > > This #ifdef is used to make this same test code to replicate both > scenarios according to -DMAKE_ME_NULL (just pay attention to actual > resulting code after #ifdef's are tested. the #ifdef here is nor related > to actual #ifdef on kernel). Here are both scenarios: > > (1) Not using my trickery (which needs the function to not be present). > (2) Using my trickery (which needs to function to stay). > > With -DMAKE_ME_NULL we replicate (2), then the function *is* there but > gcc gets rid of it on resulting binary without warnings if used with -O2. > > Without -DMAKE_ME_NULL we replicate (1). The #ifdef will fail and then > remove the function which is an obvious scenario the function won't be > part of resulting binary. > > If we use -S option to have human readable resulting assembly code > (which is kind of 1:1 for resulting binary), we can compare the result > of (1) and (2) and check they are pretty similar. > This proves gcc behaves as expected with my patch: do not need #ifdef > and do not generate dead codes to resulting binary. > > > > > > struct global_data { > > > int (*func)(int); > > > }; > > > > > > static struct global_data gd = { > > > #ifdef MAKE_ME_NULL > > > .func = USE_IT_OR_LOOSE_IT(func1), > > > > If you have ifdef around the function, why do you need magic here? Why > > not > > This #ifdef is necessary to prevent the function to be used when it > doesn't exist due to above #ifdef. But once again: don't misinterpret > the #ifdefs in this test app with the ones in kernel. They are not > related at all. If it's still confusing you just make 2 test apps > without #ifdeds out of this one where one keeps the code inside #ifdefs > and the other doesn't. > > > > > .func = func1 > > > > ? > > > > Basically the warning tells you that you want the ifdef around the > > function, too... (Otherwise you waste space). That seems like good > > warning. > > Just check my first explanation. Ping :) Comments here? Br, David Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 14, 2014 at 02:42:11PM -0800, David Cohen wrote: > On Fri, Dec 20, 2013 at 12:23:36PM -0800, David Cohen wrote: > > On Fri, Dec 20, 2013 at 08:55:27PM +0100, Pavel Machek wrote: > > > On Sun 2013-12-15 11:25:08, David Cohen wrote: > > > > On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote: > > > > > On Thu 2013-12-12 21:18:23, David Cohen wrote: > > > > > > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more > > > > > > smart. > > > > > > > > > > > > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid > > > > > > setting the callbacks when such #ifdef's aren't defined, they don't > > > > > > handle compiler to avoid messages like that: > > > > > > > > > > > > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function] > > > > > > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function] > > > > > > > > > > > > As result, those macros get rid of #ifdef's when setting callbacks but > > > > > > not when implementing them. > > > > > > > > > > > > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef > > > > > > the callbacks implementation as well. > > > > > > > > > > Well... Interesting trickery, but it means that resulting kernel > > > > > will be bigge due to the dead functions no? > > > > > > > > Actually, it doesn't get bigger. Before sending the patch I did this > > > > dummy test app: > > > > > > > > -------------------------------------------------------------------------------- > > > > #include <stdio.h> > > > > > > > > #define USE_IT_OR_LOOSE_IT(fn) ((void *)((unsigned long)(fn) - (unsigned long)(fn))) > > > > > > > > #ifdef MAKE_ME_NULL > > > > static int func1(int a) > > > > { > > > > printf("Hey!!\n"); > > > > return 0; > > > > } > > > > #endif > > > > > > I thought that point of this patch series was getting rid of the > > > #ifdefs around the function...? Now I'm confused. > > > > Maybe you're misinterpreting the test :) > > > > This #ifdef is used to make this same test code to replicate both > > scenarios according to -DMAKE_ME_NULL (just pay attention to actual > > resulting code after #ifdef's are tested. the #ifdef here is nor related > > to actual #ifdef on kernel). Here are both scenarios: > > > > (1) Not using my trickery (which needs the function to not be present). > > (2) Using my trickery (which needs to function to stay). > > > > With -DMAKE_ME_NULL we replicate (2), then the function *is* there but > > gcc gets rid of it on resulting binary without warnings if used with -O2. > > > > Without -DMAKE_ME_NULL we replicate (1). The #ifdef will fail and then > > remove the function which is an obvious scenario the function won't be > > part of resulting binary. > > > > If we use -S option to have human readable resulting assembly code > > (which is kind of 1:1 for resulting binary), we can compare the result > > of (1) and (2) and check they are pretty similar. > > This proves gcc behaves as expected with my patch: do not need #ifdef > > and do not generate dead codes to resulting binary. > > > > > > > > > struct global_data { > > > > int (*func)(int); > > > > }; > > > > > > > > static struct global_data gd = { > > > > #ifdef MAKE_ME_NULL > > > > .func = USE_IT_OR_LOOSE_IT(func1), > > > > > > If you have ifdef around the function, why do you need magic here? Why > > > not > > > > This #ifdef is necessary to prevent the function to be used when it > > doesn't exist due to above #ifdef. But once again: don't misinterpret > > the #ifdefs in this test app with the ones in kernel. They are not > > related at all. If it's still confusing you just make 2 test apps > > without #ifdeds out of this one where one keeps the code inside #ifdefs > > and the other doesn't. > > > > > > > > .func = func1 > > > > > > ? > > > > > > Basically the warning tells you that you want the ifdef around the > > > function, too... (Otherwise you waste space). That seems like good > > > warning. > > > > Just check my first explanation. > > Ping :) > > Comments here? I found few problems to be fixed prior to be possible this optimization. Many drivers are calling SET_*_PM_OPS() functions and passing non-defined symbols as argument. It's not triggering compilation issue currently because the drivers synchronize when the symbols are not defined with SET_*_PM_OPS() not using them. But IMHO it's a violation of scopes: all drivers calling SET_*_PM_OPS() should give valid symbols, since it creates an unwanted cross-dependence between those PM functions and their users (why SET_*_PM_OPS() can't use symbols given to them as argument?). I can work on fixing SET_*_PM_OPS() users beforehand in case my proposal here is accepted. Br, David Cohen > > Br, David Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- test_makemenull.S 2013-12-15 11:07:02.635992492 -0800 +++ test_no_makemenull.S 2013-12-15 11:07:10.639992359 -0800 @@ -1,7 +1,7 @@ .file "test1.c" .section .rodata.str1.1,"aMS",@progbits,1 .LC0: - .string "MAKE_ME_NULL is defined" + .string "MAKE_ME_NULL is NOT defined" .LC1: .string "%p\n" .section .text.startup,"ax",@progbits @@ -9,7 +9,7 @@ .globl main .type main, @function main: -.LFB12: +.LFB11: .cfi_startproc subq $8, %rsp .cfi_def_cfa_offset 16 @@ -24,7 +24,7 @@ .cfi_def_cfa_offset 8 ret .cfi_endproc -.LFE12: +.LFE11: .size main, .-main .ident "GCC: (Debian 4.8.2-1) 4.8.2" .section .note.GNU-stack,"",@progbits