* [PATCH v2] staging: greybus: use inline function for macros @ 2023-03-21 18:34 Menna Mahmoud 2023-03-21 18:50 ` Alex Elder 2023-03-22 9:13 ` Greg KH 0 siblings, 2 replies; 18+ messages in thread From: Menna Mahmoud @ 2023-03-21 18:34 UTC (permalink / raw) To: gregkh Cc: outreachy, johan, elder, linux-kernel, linux-staging, eng.mennamahmoud.mm, Julia Lawall Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a static inline function. It is not great to have macros that use the `container_of` macro, because from looking at the definition one cannot tell what type it applies to. One can get the same benefit from an efficiency point of view by making an inline function. Suggested-by: Julia Lawall <julia.lawall@inria.fr> Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> --- changes in v2: -send patch as a single patch. -edit the name of struct object. -edit commit message. --- drivers/staging/greybus/gbphy.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h index d4a225b76338..e7ba232bada1 100644 --- a/drivers/staging/greybus/gbphy.h +++ b/drivers/staging/greybus/gbphy.h @@ -15,7 +15,10 @@ struct gbphy_device { struct list_head list; struct device dev; }; -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) +static inline struct gbphy_device *to_gbphy_dev(const struct device *_dev) +{ + return container_of(_dev, struct gbphy_device, dev); +} static inline void *gb_gbphy_get_data(struct gbphy_device *gdev) { @@ -43,7 +46,10 @@ struct gbphy_driver { struct device_driver driver; }; -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver) +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *drv) +{ + return container_of(drv, struct gbphy_driver, driver); +} int gb_gbphy_register_driver(struct gbphy_driver *driver, struct module *owner, const char *mod_name); -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-21 18:34 [PATCH v2] staging: greybus: use inline function for macros Menna Mahmoud @ 2023-03-21 18:50 ` Alex Elder 2023-03-21 20:43 ` Julia Lawall 2023-03-22 9:13 ` Greg KH 1 sibling, 1 reply; 18+ messages in thread From: Alex Elder @ 2023-03-21 18:50 UTC (permalink / raw) To: Menna Mahmoud, gregkh Cc: outreachy, johan, elder, linux-kernel, linux-staging, Julia Lawall On 3/21/23 1:34 PM, Menna Mahmoud wrote: > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a > static inline function. > > It is not great to have macros that use the `container_of` macro, > because from looking at the definition one cannot tell what type > it applies to. > > One can get the same benefit from an efficiency point of view > by making an inline function. > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> I'm sorry if this conflicts with what others have said. But the use of a macro (with a container_of() right-hand side) to get at the structure containing a field pointer is a widely-used idiom throughout the kernel. What you propose achieves the same result but I would lean toward keeping it as a macro, mainly because it is so common. -Alex > --- > changes in v2: > -send patch as a single patch. > -edit the name of struct object. > -edit commit message. > --- > drivers/staging/greybus/gbphy.h | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h > index d4a225b76338..e7ba232bada1 100644 > --- a/drivers/staging/greybus/gbphy.h > +++ b/drivers/staging/greybus/gbphy.h > @@ -15,7 +15,10 @@ struct gbphy_device { > struct list_head list; > struct device dev; > }; > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > +static inline struct gbphy_device *to_gbphy_dev(const struct device *_dev) > +{ > + return container_of(_dev, struct gbphy_device, dev); > +} > > static inline void *gb_gbphy_get_data(struct gbphy_device *gdev) > { > @@ -43,7 +46,10 @@ struct gbphy_driver { > > struct device_driver driver; > }; > -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver) > +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *drv) > +{ > + return container_of(drv, struct gbphy_driver, driver); > +} > > int gb_gbphy_register_driver(struct gbphy_driver *driver, > struct module *owner, const char *mod_name); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-21 18:50 ` Alex Elder @ 2023-03-21 20:43 ` Julia Lawall 2023-03-21 21:09 ` Alex Elder 0 siblings, 1 reply; 18+ messages in thread From: Julia Lawall @ 2023-03-21 20:43 UTC (permalink / raw) To: Alex Elder Cc: Menna Mahmoud, gregkh, outreachy, johan, elder, linux-kernel, linux-staging On Tue, 21 Mar 2023, Alex Elder wrote: > On 3/21/23 1:34 PM, Menna Mahmoud wrote: > > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a > > static inline function. > > > > It is not great to have macros that use the `container_of` macro, > > because from looking at the definition one cannot tell what type > > it applies to. > > > > One can get the same benefit from an efficiency point of view > > by making an inline function. > > > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> > > I'm sorry if this conflicts with what others have said. > > But the use of a macro (with a container_of() right-hand > side) to get at the structure containing a field pointer > is a widely-used idiom throughout the kernel. > > What you propose achieves the same result but I would > lean toward keeping it as a macro, mainly because it > is so common. Common is not necessarily good. Macros are less safe and less informative. julia > > -Alex > > --- > > changes in v2: > > -send patch as a single patch. > > -edit the name of struct object. > > -edit commit message. > > --- > > drivers/staging/greybus/gbphy.h | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/greybus/gbphy.h > > b/drivers/staging/greybus/gbphy.h > > index d4a225b76338..e7ba232bada1 100644 > > --- a/drivers/staging/greybus/gbphy.h > > +++ b/drivers/staging/greybus/gbphy.h > > @@ -15,7 +15,10 @@ struct gbphy_device { > > struct list_head list; > > struct device dev; > > }; > > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > > +static inline struct gbphy_device *to_gbphy_dev(const struct device *_dev) > > +{ > > + return container_of(_dev, struct gbphy_device, dev); > > +} > > static inline void *gb_gbphy_get_data(struct gbphy_device *gdev) > > { > > @@ -43,7 +46,10 @@ struct gbphy_driver { > > struct device_driver driver; > > }; > > -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver) > > +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver > > *drv) > > +{ > > + return container_of(drv, struct gbphy_driver, driver); > > +} > > int gb_gbphy_register_driver(struct gbphy_driver *driver, > > struct module *owner, const char *mod_name); > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-21 20:43 ` Julia Lawall @ 2023-03-21 21:09 ` Alex Elder 2023-03-21 21:29 ` Julia Lawall 0 siblings, 1 reply; 18+ messages in thread From: Alex Elder @ 2023-03-21 21:09 UTC (permalink / raw) To: Julia Lawall Cc: Menna Mahmoud, gregkh, outreachy, johan, elder, linux-kernel, linux-staging On 3/21/23 3:43 PM, Julia Lawall wrote: > > > On Tue, 21 Mar 2023, Alex Elder wrote: > >> On 3/21/23 1:34 PM, Menna Mahmoud wrote: >>> Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a >>> static inline function. >>> >>> It is not great to have macros that use the `container_of` macro, >>> because from looking at the definition one cannot tell what type >>> it applies to. >>> >>> One can get the same benefit from an efficiency point of view >>> by making an inline function. >>> >>> Suggested-by: Julia Lawall <julia.lawall@inria.fr> >>> Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> >> >> I'm sorry if this conflicts with what others have said. >> >> But the use of a macro (with a container_of() right-hand >> side) to get at the structure containing a field pointer >> is a widely-used idiom throughout the kernel. >> >> What you propose achieves the same result but I would >> lean toward keeping it as a macro, mainly because it >> is so common. > > Common is not necessarily good. Macros are less safe and less > informative. I do agree that the inline function is better, and is functionally equivalent (while being explicit with types and avoiding any macro expansion funny business). Do you think we should make changes like this throughout the kernel (along the lines of the flexible array fixes, to make things safer)? I don't think it's a terrible idea, but it's likely a big undertaking and I predict push-back. Bottom line on this is that I don't think the proposed change is wrong, but there is value in consistently adhering to conventions. Others can comment; I have no real objection. -Alex > > julia > > >> >> -Alex >>> --- >>> changes in v2: >>> -send patch as a single patch. >>> -edit the name of struct object. >>> -edit commit message. >>> --- >>> drivers/staging/greybus/gbphy.h | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/staging/greybus/gbphy.h >>> b/drivers/staging/greybus/gbphy.h >>> index d4a225b76338..e7ba232bada1 100644 >>> --- a/drivers/staging/greybus/gbphy.h >>> +++ b/drivers/staging/greybus/gbphy.h >>> @@ -15,7 +15,10 @@ struct gbphy_device { >>> struct list_head list; >>> struct device dev; >>> }; >>> -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) >>> +static inline struct gbphy_device *to_gbphy_dev(const struct device *_dev) >>> +{ >>> + return container_of(_dev, struct gbphy_device, dev); >>> +} >>> static inline void *gb_gbphy_get_data(struct gbphy_device *gdev) >>> { >>> @@ -43,7 +46,10 @@ struct gbphy_driver { >>> struct device_driver driver; >>> }; >>> -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver) >>> +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver >>> *drv) >>> +{ >>> + return container_of(drv, struct gbphy_driver, driver); >>> +} >>> int gb_gbphy_register_driver(struct gbphy_driver *driver, >>> struct module *owner, const char *mod_name); >> >> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-21 21:09 ` Alex Elder @ 2023-03-21 21:29 ` Julia Lawall 2023-03-21 22:42 ` Alex Elder 0 siblings, 1 reply; 18+ messages in thread From: Julia Lawall @ 2023-03-21 21:29 UTC (permalink / raw) To: Alex Elder Cc: Menna Mahmoud, gregkh, outreachy, johan, elder, linux-kernel, linux-staging On Tue, 21 Mar 2023, Alex Elder wrote: > On 3/21/23 3:43 PM, Julia Lawall wrote: > > > > > > On Tue, 21 Mar 2023, Alex Elder wrote: > > > > > On 3/21/23 1:34 PM, Menna Mahmoud wrote: > > > > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a > > > > static inline function. > > > > > > > > It is not great to have macros that use the `container_of` macro, > > > > because from looking at the definition one cannot tell what type > > > > it applies to. > > > > > > > > One can get the same benefit from an efficiency point of view > > > > by making an inline function. > > > > > > > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > > > > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> > > > > > > I'm sorry if this conflicts with what others have said. > > > > > > But the use of a macro (with a container_of() right-hand > > > side) to get at the structure containing a field pointer > > > is a widely-used idiom throughout the kernel. > > > > > > What you propose achieves the same result but I would > > > lean toward keeping it as a macro, mainly because it > > > is so common. > > > > Common is not necessarily good. Macros are less safe and less > > informative. > > I do agree that the inline function is better, and > is functionally equivalent (while being explicit > with types and avoiding any macro expansion funny > business). > > Do you think we should make changes like this throughout > the kernel (along the lines of the flexible array fixes, > to make things safer)? I don't think it's a terrible idea, > but it's likely a big undertaking and I predict push-back. I agree that it's not a terrible idea and that it's a big undertaking. The code would be a little safer if people had the habit of making functions, as it avoids the risk of parameterizing over the field name in the third argument. But the impact is probably lesser than for the flexible array fixes. But if we have the chance to clean up the staging code, at least, then why not. There do seem to be more than 100 definitions like: #define to_dove_clk(hw) container_of(hw, struct dove_clk, hw) where we have to hope that at all of the usage sites the argument is called hw. Probably anything else would cause a compiler error, but still it looks strange. julia > > Bottom line on this is that I don't think the proposed > change is wrong, but there is value in consistently > adhering to conventions. > > Others can comment; I have no real objection. > > -Alex > > > > > julia > > > > > > > > > > -Alex > > > > --- > > > > changes in v2: > > > > -send patch as a single patch. > > > > -edit the name of struct object. > > > > -edit commit message. > > > > --- > > > > drivers/staging/greybus/gbphy.h | 10 ++++++++-- > > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/staging/greybus/gbphy.h > > > > b/drivers/staging/greybus/gbphy.h > > > > index d4a225b76338..e7ba232bada1 100644 > > > > --- a/drivers/staging/greybus/gbphy.h > > > > +++ b/drivers/staging/greybus/gbphy.h > > > > @@ -15,7 +15,10 @@ struct gbphy_device { > > > > struct list_head list; > > > > struct device dev; > > > > }; > > > > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > > > > +static inline struct gbphy_device *to_gbphy_dev(const struct device > > > > *_dev) > > > > +{ > > > > + return container_of(_dev, struct gbphy_device, dev); > > > > +} > > > > static inline void *gb_gbphy_get_data(struct gbphy_device *gdev) > > > > { > > > > @@ -43,7 +46,10 @@ struct gbphy_driver { > > > > struct device_driver driver; > > > > }; > > > > -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver) > > > > +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver > > > > *drv) > > > > +{ > > > > + return container_of(drv, struct gbphy_driver, driver); > > > > +} > > > > int gb_gbphy_register_driver(struct gbphy_driver *driver, > > > > struct module *owner, const char > > > > *mod_name); > > > > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-21 21:29 ` Julia Lawall @ 2023-03-21 22:42 ` Alex Elder 2023-03-22 10:00 ` Julia Lawall 0 siblings, 1 reply; 18+ messages in thread From: Alex Elder @ 2023-03-21 22:42 UTC (permalink / raw) To: Julia Lawall Cc: Menna Mahmoud, gregkh, outreachy, johan, elder, linux-kernel, linux-staging On 3/21/23 4:29 PM, Julia Lawall wrote: > There do seem to be more than 100 definitions like: > > #define to_dove_clk(hw) container_of(hw, struct dove_clk, hw) > > where we have to hope that at all of the usage sites the argument is > called hw. Probably anything else would cause a compiler error, but still > it looks strange. > > julia Yeah, that's poorly done and unfortunate. The argument to the parameter should be named something different. I didn't understand what you meant before but yes, this isn't good. These should be fixed first. -Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-21 22:42 ` Alex Elder @ 2023-03-22 10:00 ` Julia Lawall 2023-03-22 12:39 ` Alex Elder 2023-03-23 4:58 ` Greg KH 0 siblings, 2 replies; 18+ messages in thread From: Julia Lawall @ 2023-03-22 10:00 UTC (permalink / raw) To: Alex Elder Cc: Menna Mahmoud, gregkh, outreachy, johan, elder, linux-kernel, linux-staging Greg raised the question of whether the inline function is really as efficient as a macro. I tried the following definitions: #define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) static inline struct gbphy_device *extra_to_gbphy_dev(const struct device *_dev) { return container_of(_dev, struct gbphy_device, dev); } And the following uses: ssize_t macro_protocol_id_show(struct device *dev, struct device_attribute *attr, char *buf) { struct gbphy_device *gbphy_dev = to_gbphy_dev(dev); return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); } ssize_t extra_protocol_id_show(struct device *dev, struct device_attribute *attr, char *buf) { struct gbphy_device *gbphy_dev = extra_to_gbphy_dev(dev); return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); } They are a little bit different to avoid too much compiler optimization. After doing make drivers/staging/greybus/gbphy.s, I get similar looking code in both cases: Macro version: .type macro_protocol_id_show, @function macro_protocol_id_show: endbr64 1: call __fentry__ .section __mcount_loc, "a",@progbits .quad 1b .previous pushq %rbp # movq %rdx, %rbp # tmp96, buf pushq %rbx # # drivers/staging/greybus/gbphy.c:40: { movq %rdi, %rbx # tmp95, dev # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); call __sanitizer_cov_trace_pc # # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); movzbl 0(%rbp), %edx # *buf_9(D), *buf_9(D) movq %rbp, %rdi # buf, movq $.LC18, %rsi #, movzbl 3(%rax), %ecx # _1->protocol_id, _1->protocol_id call sprintf # # drivers/staging/greybus/gbphy.c:44: } movl $13, %eax #, popq %rbx # popq %rbp # jmp __x86_return_thunk .size macro_protocol_id_show, .-macro_protocol_id_show Function version: .type extra_protocol_id_show, @function extra_protocol_id_show: endbr64 1: call __fentry__ .section __mcount_loc, "a",@progbits .quad 1b .previous pushq %rbp # movq %rdx, %rbp # tmp96, buf pushq %rbx # # drivers/staging/greybus/gbphy.c:47: { movq %rdi, %rbx # tmp95, dev # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); call __sanitizer_cov_trace_pc # # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); movzbl 0(%rbp), %ecx # *buf_9(D), *buf_9(D) movq %rbp, %rdi # buf, movq $.LC19, %rsi #, movzbl 3(%rax), %edx # _3->protocol_id, _3->protocol_id call sprintf # # drivers/staging/greybus/gbphy.c:51: } movl $13, %eax #, popq %rbx # popq %rbp # jmp __x86_return_thunk .size extra_protocol_id_show, .-extra_protocol_id_show Both seem to access the memory directly. Maybe the example is too simple, and the compiler is more likely to optimize aggressively? julia ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-22 10:00 ` Julia Lawall @ 2023-03-22 12:39 ` Alex Elder 2023-03-23 4:58 ` Greg KH 1 sibling, 0 replies; 18+ messages in thread From: Alex Elder @ 2023-03-22 12:39 UTC (permalink / raw) To: Julia Lawall Cc: Menna Mahmoud, gregkh, outreachy, johan, elder, linux-kernel, linux-staging On 3/22/23 5:00 AM, Julia Lawall wrote: > > Both seem to access the memory directly. Maybe the example is too simple, > and the compiler is more likely to optimize aggressively? Yes I think so. This is a little unrelated but the "inline" keyword isn't very useful because the compiler (at least when optimizing) already takes liberties of inlining code that it "knows" will be better done that way. Same thing here. This function is so trivial that it's almost certainly going to be inlined. So the benefit of a little helper function like this over a macro is that its types are specified explicitly, and there is no chance of macro arguments be duplicated or improperly used in the right hand side. If it's not inlined it also would normally generate stuff on the stack. The benefit of the macro is you can do things with the arguments because they're pass-by-name. But you can't expect there to be any efficiency benefit. -Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-22 10:00 ` Julia Lawall 2023-03-22 12:39 ` Alex Elder @ 2023-03-23 4:58 ` Greg KH 2023-03-23 5:05 ` Deepak R Varma 2023-03-23 9:52 ` Julia Lawall 1 sibling, 2 replies; 18+ messages in thread From: Greg KH @ 2023-03-23 4:58 UTC (permalink / raw) To: Julia Lawall Cc: Alex Elder, Menna Mahmoud, outreachy, johan, elder, linux-kernel, linux-staging On Wed, Mar 22, 2023 at 11:00:41AM +0100, Julia Lawall wrote: > Greg raised the question of whether the inline function is really as > efficient as a macro. > > I tried the following definitions: > > #define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > > static inline struct gbphy_device *extra_to_gbphy_dev(const struct device *_dev) > { > return container_of(_dev, struct gbphy_device, dev); > } > > And the following uses: > > ssize_t macro_protocol_id_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct gbphy_device *gbphy_dev = to_gbphy_dev(dev); > > return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > } > ssize_t extra_protocol_id_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct gbphy_device *gbphy_dev = extra_to_gbphy_dev(dev); > > return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > } > > They are a little bit different to avoid too much compiler optimization. > > After doing make drivers/staging/greybus/gbphy.s, I get similar looking > code in both cases: > > Macro version: > > .type macro_protocol_id_show, @function > macro_protocol_id_show: > endbr64 > 1: call __fentry__ > .section __mcount_loc, "a",@progbits > .quad 1b > .previous > pushq %rbp # > movq %rdx, %rbp # tmp96, buf > pushq %rbx # > # drivers/staging/greybus/gbphy.c:40: { > movq %rdi, %rbx # tmp95, dev > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > call __sanitizer_cov_trace_pc # > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > movzbl 0(%rbp), %edx # *buf_9(D), *buf_9(D) > movq %rbp, %rdi # buf, > movq $.LC18, %rsi #, > movzbl 3(%rax), %ecx # _1->protocol_id, _1->protocol_id > call sprintf # > # drivers/staging/greybus/gbphy.c:44: } > movl $13, %eax #, > popq %rbx # > popq %rbp # > jmp __x86_return_thunk > .size macro_protocol_id_show, .-macro_protocol_id_show > > Function version: > > .type extra_protocol_id_show, @function > extra_protocol_id_show: > endbr64 > 1: call __fentry__ > .section __mcount_loc, "a",@progbits > .quad 1b > .previous > pushq %rbp # > movq %rdx, %rbp # tmp96, buf > pushq %rbx # > # drivers/staging/greybus/gbphy.c:47: { > movq %rdi, %rbx # tmp95, dev > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > call __sanitizer_cov_trace_pc # > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > movzbl 0(%rbp), %ecx # *buf_9(D), *buf_9(D) > movq %rbp, %rdi # buf, > movq $.LC19, %rsi #, > movzbl 3(%rax), %edx # _3->protocol_id, _3->protocol_id > call sprintf # > # drivers/staging/greybus/gbphy.c:51: } > movl $13, %eax #, > popq %rbx # > popq %rbp # > jmp __x86_return_thunk > .size extra_protocol_id_show, .-extra_protocol_id_show > > Both seem to access the memory directly. Maybe the example is too simple, > and the compiler is more likely to optimize aggressively? Nice, that shows that it is the same both ways as the compiler version you are using is smart enough Which compiler and version is this? Does it work the same for all of the supported versions we have to support (i.e. really old gcc?) For the most part, sysfs files are not on any sort of "fast path" so a function call is fine, but as I mentioned before, sometimes we are forced to move calls to container_of() to container_of_const() and that can not be an inline function, but must remain a macro :( thanks, greg k-h ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-23 4:58 ` Greg KH @ 2023-03-23 5:05 ` Deepak R Varma 2023-03-23 5:22 ` Greg KH 2023-03-23 9:52 ` Julia Lawall 1 sibling, 1 reply; 18+ messages in thread From: Deepak R Varma @ 2023-03-23 5:05 UTC (permalink / raw) To: Greg KH Cc: Julia Lawall, Alex Elder, Menna Mahmoud, outreachy, johan, elder, linux-kernel, linux-staging On Thu, Mar 23, 2023 at 05:58:02AM +0100, Greg KH wrote: > Nice, that shows that it is the same both ways as the compiler version > you are using is smart enough > > Which compiler and version is this? Does it work the same for all of > the supported versions we have to support (i.e. really old gcc?) > > For the most part, sysfs files are not on any sort of "fast path" so a Hello, Is there a guideline/documentation on how to identify if a code is part of fast/slow path? > function call is fine, but as I mentioned before, sometimes we are > forced to move calls to container_of() to container_of_const() and that > can not be an inline function, but must remain a macro :( > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-23 5:05 ` Deepak R Varma @ 2023-03-23 5:22 ` Greg KH 2023-03-23 19:46 ` Deepak R Varma 0 siblings, 1 reply; 18+ messages in thread From: Greg KH @ 2023-03-23 5:22 UTC (permalink / raw) To: Deepak R Varma Cc: Julia Lawall, Alex Elder, Menna Mahmoud, outreachy, johan, elder, linux-kernel, linux-staging On Thu, Mar 23, 2023 at 10:35:59AM +0530, Deepak R Varma wrote: > On Thu, Mar 23, 2023 at 05:58:02AM +0100, Greg KH wrote: > > Nice, that shows that it is the same both ways as the compiler version > > you are using is smart enough > > > > Which compiler and version is this? Does it work the same for all of > > the supported versions we have to support (i.e. really old gcc?) > > > > For the most part, sysfs files are not on any sort of "fast path" so a > > Hello, > Is there a guideline/documentation on how to identify if a code is part of > fast/slow path? Not really, the general rule is if the code is used in a function that is time criticial, then it is in the "fast path". Normally the code path for I/O or for determining what process to schedule next is a "fast path" as you want to do the least amount of work in the kernel so as to get the I/O to the hardware or caller, or you want to schedule the next process faster so that it can do the real work that is wanted. But for some I/O, where the hardware is the limiting factor (like slow USB or serial connections), no matter how optimized the kernel is, the data can not get to the user any faster because the hardware just can not provide it, so for that, it's not really that critical. Note that "time critical" can change depending on the user of the system and as hardware evolves. One example would be the creation of a number of sysfs entries for the disks in the systems. On a "normal" system, creating them all is trivial as there are not many disks. But on some "big" systems with many tens of thousands of disks, it can become a boot time bottleneck. So it's usually "you know it when you see it show up on a profile", use the perf tool to test your workload on, to see where in the kernel things are taking up too much time so you know what needs to be made faster. hope this helps, greg k-h ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-23 5:22 ` Greg KH @ 2023-03-23 19:46 ` Deepak R Varma 0 siblings, 0 replies; 18+ messages in thread From: Deepak R Varma @ 2023-03-23 19:46 UTC (permalink / raw) To: Greg KH Cc: Julia Lawall, Alex Elder, Menna Mahmoud, outreachy, johan, elder, linux-kernel, linux-staging On Thu, Mar 23, 2023 at 06:22:55AM +0100, Greg KH wrote: > On Thu, Mar 23, 2023 at 10:35:59AM +0530, Deepak R Varma wrote: > > On Thu, Mar 23, 2023 at 05:58:02AM +0100, Greg KH wrote: > > > Nice, that shows that it is the same both ways as the compiler version > > > you are using is smart enough > > > > > > Which compiler and version is this? Does it work the same for all of > > > the supported versions we have to support (i.e. really old gcc?) > > > > > > For the most part, sysfs files are not on any sort of "fast path" so a > > > > Hello, > > Is there a guideline/documentation on how to identify if a code is part of > > fast/slow path? > > Not really, the general rule is if the code is used in a function that > is time criticial, then it is in the "fast path". > > Normally the code path for I/O or for determining what process to > schedule next is a "fast path" as you want to do the least amount of > work in the kernel so as to get the I/O to the hardware or caller, or > you want to schedule the next process faster so that it can do the real > work that is wanted. > > But for some I/O, where the hardware is the limiting factor (like slow > USB or serial connections), no matter how optimized the kernel is, the > data can not get to the user any faster because the hardware just can > not provide it, so for that, it's not really that critical. > > Note that "time critical" can change depending on the user of the system > and as hardware evolves. > > One example would be the creation of a number of sysfs entries for the > disks in the systems. On a "normal" system, creating them all is trivial > as there are not many disks. But on some "big" systems with many tens > of thousands of disks, it can become a boot time bottleneck. > > So it's usually "you know it when you see it show up on a profile", use > the perf tool to test your workload on, to see where in the kernel > things are taking up too much time so you know what needs to be made > faster. > > hope this helps, Thank you Greg. Yes, it definitely is very helpful. I sincerely appreciate the explanation and the time you took to write it in so detail. I am Obliged. Regards, Deepak. > > greg k-h ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-23 4:58 ` Greg KH 2023-03-23 5:05 ` Deepak R Varma @ 2023-03-23 9:52 ` Julia Lawall 2023-03-25 8:49 ` Greg KH 1 sibling, 1 reply; 18+ messages in thread From: Julia Lawall @ 2023-03-23 9:52 UTC (permalink / raw) To: Greg KH Cc: Alex Elder, Menna Mahmoud, outreachy, johan, elder, linux-kernel, linux-staging On Thu, 23 Mar 2023, Greg KH wrote: > On Wed, Mar 22, 2023 at 11:00:41AM +0100, Julia Lawall wrote: > > Greg raised the question of whether the inline function is really as > > efficient as a macro. > > > > I tried the following definitions: > > > > #define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > > > > static inline struct gbphy_device *extra_to_gbphy_dev(const struct device *_dev) > > { > > return container_of(_dev, struct gbphy_device, dev); > > } > > > > And the following uses: > > > > ssize_t macro_protocol_id_show(struct device *dev, > > struct device_attribute *attr, char *buf) > > { > > struct gbphy_device *gbphy_dev = to_gbphy_dev(dev); > > > > return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > } > > ssize_t extra_protocol_id_show(struct device *dev, > > struct device_attribute *attr, char *buf) > > { > > struct gbphy_device *gbphy_dev = extra_to_gbphy_dev(dev); > > > > return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > } > > > > They are a little bit different to avoid too much compiler optimization. > > > > After doing make drivers/staging/greybus/gbphy.s, I get similar looking > > code in both cases: > > > > Macro version: > > > > .type macro_protocol_id_show, @function > > macro_protocol_id_show: > > endbr64 > > 1: call __fentry__ > > .section __mcount_loc, "a",@progbits > > .quad 1b > > .previous > > pushq %rbp # > > movq %rdx, %rbp # tmp96, buf > > pushq %rbx # > > # drivers/staging/greybus/gbphy.c:40: { > > movq %rdi, %rbx # tmp95, dev > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > call __sanitizer_cov_trace_pc # > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > movzbl 0(%rbp), %edx # *buf_9(D), *buf_9(D) > > movq %rbp, %rdi # buf, > > movq $.LC18, %rsi #, > > movzbl 3(%rax), %ecx # _1->protocol_id, _1->protocol_id > > call sprintf # > > # drivers/staging/greybus/gbphy.c:44: } > > movl $13, %eax #, > > popq %rbx # > > popq %rbp # > > jmp __x86_return_thunk > > .size macro_protocol_id_show, .-macro_protocol_id_show > > > > Function version: > > > > .type extra_protocol_id_show, @function > > extra_protocol_id_show: > > endbr64 > > 1: call __fentry__ > > .section __mcount_loc, "a",@progbits > > .quad 1b > > .previous > > pushq %rbp # > > movq %rdx, %rbp # tmp96, buf > > pushq %rbx # > > # drivers/staging/greybus/gbphy.c:47: { > > movq %rdi, %rbx # tmp95, dev > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > call __sanitizer_cov_trace_pc # > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > movzbl 0(%rbp), %ecx # *buf_9(D), *buf_9(D) > > movq %rbp, %rdi # buf, > > movq $.LC19, %rsi #, > > movzbl 3(%rax), %edx # _3->protocol_id, _3->protocol_id > > call sprintf # > > # drivers/staging/greybus/gbphy.c:51: } > > movl $13, %eax #, > > popq %rbx # > > popq %rbp # > > jmp __x86_return_thunk > > .size extra_protocol_id_show, .-extra_protocol_id_show > > > > Both seem to access the memory directly. Maybe the example is too simple, > > and the compiler is more likely to optimize aggressively? > > Nice, that shows that it is the same both ways as the compiler version > you are using is smart enough > > Which compiler and version is this? Does it work the same for all of > the supported versions we have to support (i.e. really old gcc?) gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0 I got a similar result for gcc-5: macro_protocol_id_show: 1: call __fentry__ .section __mcount_loc, "a",@progbits .quad 1b .previous movq %rdx, %rax # buf, buf movq -32(%rdi), %rdx # MEM[(struct gbphy_device *)dev_1(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_1(D) + -40B].cport_desc movq $.LC19, %rsi #, movq %rax, %rdi # buf, movzbl 3(%rdx), %ecx # _3->protocol_id, D.44996 movzbl (%rax), %edx # *buf_6(D), D.44996 call sprintf # cltq jmp __x86_return_thunk .size macro_protocol_id_show, .-macro_protocol_id_show .section .text.unlikely .LCOLDE20: .text .LHOTE20: .section .rodata.str1.1 .LC21: .string "extra 0x%02x %c\n" .section .text.unlikely .LCOLDB22: .text .LHOTB22: .p2align 6,,63 .globl extra_protocol_id_show .type extra_protocol_id_show, @function extra_protocol_id_show: 1: call __fentry__ .section __mcount_loc, "a",@progbits .quad 1b .previous movq %rdx, %rax # buf, buf movzbl (%rdx), %ecx # *buf_3(D), D.45003 movq -32(%rdi), %rdx # MEM[(struct gbphy_device *)dev_2(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_2(D) + -40B].cport_desc movq $.LC21, %rsi #, movq %rax, %rdi # buf, movzbl 3(%rdx), %edx # _6->protocol_id, D.45003 call sprintf # cltq jmp __x86_return_thunk .size extra_protocol_id_show, .-extra_protocol_id_show .section .text.unlikely > > For the most part, sysfs files are not on any sort of "fast path" so a > function call is fine, but as I mentioned before, sometimes we are > forced to move calls to container_of() to container_of_const() and that > can not be an inline function, but must remain a macro :( It seems that this is because there is not a unique return type, but not a performance issue? julia ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-23 9:52 ` Julia Lawall @ 2023-03-25 8:49 ` Greg KH 2023-03-25 9:28 ` Julia Lawall 0 siblings, 1 reply; 18+ messages in thread From: Greg KH @ 2023-03-25 8:49 UTC (permalink / raw) To: Julia Lawall Cc: Alex Elder, Menna Mahmoud, outreachy, johan, elder, linux-kernel, linux-staging On Thu, Mar 23, 2023 at 10:52:35AM +0100, Julia Lawall wrote: > > > On Thu, 23 Mar 2023, Greg KH wrote: > > > On Wed, Mar 22, 2023 at 11:00:41AM +0100, Julia Lawall wrote: > > > Greg raised the question of whether the inline function is really as > > > efficient as a macro. > > > > > > I tried the following definitions: > > > > > > #define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > > > > > > static inline struct gbphy_device *extra_to_gbphy_dev(const struct device *_dev) > > > { > > > return container_of(_dev, struct gbphy_device, dev); > > > } > > > > > > And the following uses: > > > > > > ssize_t macro_protocol_id_show(struct device *dev, > > > struct device_attribute *attr, char *buf) > > > { > > > struct gbphy_device *gbphy_dev = to_gbphy_dev(dev); > > > > > > return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > > } > > > ssize_t extra_protocol_id_show(struct device *dev, > > > struct device_attribute *attr, char *buf) > > > { > > > struct gbphy_device *gbphy_dev = extra_to_gbphy_dev(dev); > > > > > > return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > > } > > > > > > They are a little bit different to avoid too much compiler optimization. > > > > > > After doing make drivers/staging/greybus/gbphy.s, I get similar looking > > > code in both cases: > > > > > > Macro version: > > > > > > .type macro_protocol_id_show, @function > > > macro_protocol_id_show: > > > endbr64 > > > 1: call __fentry__ > > > .section __mcount_loc, "a",@progbits > > > .quad 1b > > > .previous > > > pushq %rbp # > > > movq %rdx, %rbp # tmp96, buf > > > pushq %rbx # > > > # drivers/staging/greybus/gbphy.c:40: { > > > movq %rdi, %rbx # tmp95, dev > > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > > call __sanitizer_cov_trace_pc # > > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > > movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc > > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > > movzbl 0(%rbp), %edx # *buf_9(D), *buf_9(D) > > > movq %rbp, %rdi # buf, > > > movq $.LC18, %rsi #, > > > movzbl 3(%rax), %ecx # _1->protocol_id, _1->protocol_id > > > call sprintf # > > > # drivers/staging/greybus/gbphy.c:44: } > > > movl $13, %eax #, > > > popq %rbx # > > > popq %rbp # > > > jmp __x86_return_thunk > > > .size macro_protocol_id_show, .-macro_protocol_id_show > > > > > > Function version: > > > > > > .type extra_protocol_id_show, @function > > > extra_protocol_id_show: > > > endbr64 > > > 1: call __fentry__ > > > .section __mcount_loc, "a",@progbits > > > .quad 1b > > > .previous > > > pushq %rbp # > > > movq %rdx, %rbp # tmp96, buf > > > pushq %rbx # > > > # drivers/staging/greybus/gbphy.c:47: { > > > movq %rdi, %rbx # tmp95, dev > > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > > call __sanitizer_cov_trace_pc # > > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > > movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc > > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > > movzbl 0(%rbp), %ecx # *buf_9(D), *buf_9(D) > > > movq %rbp, %rdi # buf, > > > movq $.LC19, %rsi #, > > > movzbl 3(%rax), %edx # _3->protocol_id, _3->protocol_id > > > call sprintf # > > > # drivers/staging/greybus/gbphy.c:51: } > > > movl $13, %eax #, > > > popq %rbx # > > > popq %rbp # > > > jmp __x86_return_thunk > > > .size extra_protocol_id_show, .-extra_protocol_id_show > > > > > > Both seem to access the memory directly. Maybe the example is too simple, > > > and the compiler is more likely to optimize aggressively? > > > > Nice, that shows that it is the same both ways as the compiler version > > you are using is smart enough > > > > Which compiler and version is this? Does it work the same for all of > > the supported versions we have to support (i.e. really old gcc?) > > gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0 > > I got a similar result for gcc-5: > > macro_protocol_id_show: > 1: call __fentry__ > .section __mcount_loc, "a",@progbits > .quad 1b > .previous > movq %rdx, %rax # buf, buf > movq -32(%rdi), %rdx # MEM[(struct gbphy_device *)dev_1(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_1(D) + -40B].cport_desc > movq $.LC19, %rsi #, > movq %rax, %rdi # buf, > movzbl 3(%rdx), %ecx # _3->protocol_id, D.44996 > movzbl (%rax), %edx # *buf_6(D), D.44996 > call sprintf # > cltq > jmp __x86_return_thunk > .size macro_protocol_id_show, .-macro_protocol_id_show > .section .text.unlikely > .LCOLDE20: > .text > .LHOTE20: > .section .rodata.str1.1 > .LC21: > .string "extra 0x%02x %c\n" > .section .text.unlikely > .LCOLDB22: > .text > .LHOTB22: > .p2align 6,,63 > .globl extra_protocol_id_show > .type extra_protocol_id_show, @function > extra_protocol_id_show: > 1: call __fentry__ > .section __mcount_loc, "a",@progbits > .quad 1b > .previous > movq %rdx, %rax # buf, buf > movzbl (%rdx), %ecx # *buf_3(D), D.45003 > movq -32(%rdi), %rdx # MEM[(struct gbphy_device *)dev_2(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_2(D) + -40B].cport_desc > movq $.LC21, %rsi #, > movq %rax, %rdi # buf, > movzbl 3(%rdx), %edx # _6->protocol_id, D.45003 > call sprintf # > cltq > jmp __x86_return_thunk > .size extra_protocol_id_show, .-extra_protocol_id_show > .section .text.unlikely Ah nice! Thanks for checking. Ok, so it's not a performance issue at all either way, compilers are smarter than we think :) > > For the most part, sysfs files are not on any sort of "fast path" so a > > function call is fine, but as I mentioned before, sometimes we are > > forced to move calls to container_of() to container_of_const() and that > > can not be an inline function, but must remain a macro :( > > It seems that this is because there is not a unique return type, but not a > performance issue? Yes, container_of_const() will return a different type (const or not) depending on what is passed into it. This allows the "const-ness" of a pointer to be kept across the call, while as container_of() will always cast away the const which can be dangerous if you don't know what you are doing. thanks, greg k-h ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-25 8:49 ` Greg KH @ 2023-03-25 9:28 ` Julia Lawall 0 siblings, 0 replies; 18+ messages in thread From: Julia Lawall @ 2023-03-25 9:28 UTC (permalink / raw) To: Greg KH Cc: Alex Elder, Menna Mahmoud, outreachy, johan, elder, linux-kernel, linux-staging On Sat, 25 Mar 2023, Greg KH wrote: > On Thu, Mar 23, 2023 at 10:52:35AM +0100, Julia Lawall wrote: > > > > > > On Thu, 23 Mar 2023, Greg KH wrote: > > > > > On Wed, Mar 22, 2023 at 11:00:41AM +0100, Julia Lawall wrote: > > > > Greg raised the question of whether the inline function is really as > > > > efficient as a macro. > > > > > > > > I tried the following definitions: > > > > > > > > #define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > > > > > > > > static inline struct gbphy_device *extra_to_gbphy_dev(const struct device *_dev) > > > > { > > > > return container_of(_dev, struct gbphy_device, dev); > > > > } > > > > > > > > And the following uses: > > > > > > > > ssize_t macro_protocol_id_show(struct device *dev, > > > > struct device_attribute *attr, char *buf) > > > > { > > > > struct gbphy_device *gbphy_dev = to_gbphy_dev(dev); > > > > > > > > return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > > > } > > > > ssize_t extra_protocol_id_show(struct device *dev, > > > > struct device_attribute *attr, char *buf) > > > > { > > > > struct gbphy_device *gbphy_dev = extra_to_gbphy_dev(dev); > > > > > > > > return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > > > } > > > > > > > > They are a little bit different to avoid too much compiler optimization. > > > > > > > > After doing make drivers/staging/greybus/gbphy.s, I get similar looking > > > > code in both cases: > > > > > > > > Macro version: > > > > > > > > .type macro_protocol_id_show, @function > > > > macro_protocol_id_show: > > > > endbr64 > > > > 1: call __fentry__ > > > > .section __mcount_loc, "a",@progbits > > > > .quad 1b > > > > .previous > > > > pushq %rbp # > > > > movq %rdx, %rbp # tmp96, buf > > > > pushq %rbx # > > > > # drivers/staging/greybus/gbphy.c:40: { > > > > movq %rdi, %rbx # tmp95, dev > > > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > > > call __sanitizer_cov_trace_pc # > > > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > > > movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_7(D) + -40B].cport_desc > > > > # drivers/staging/greybus/gbphy.c:43: return sprintf(buf, "%c macro 0x%02x\n", *buf, gbphy_dev->cport_desc->protocol_id); > > > > movzbl 0(%rbp), %edx # *buf_9(D), *buf_9(D) > > > > movq %rbp, %rdi # buf, > > > > movq $.LC18, %rsi #, > > > > movzbl 3(%rax), %ecx # _1->protocol_id, _1->protocol_id > > > > call sprintf # > > > > # drivers/staging/greybus/gbphy.c:44: } > > > > movl $13, %eax #, > > > > popq %rbx # > > > > popq %rbp # > > > > jmp __x86_return_thunk > > > > .size macro_protocol_id_show, .-macro_protocol_id_show > > > > > > > > Function version: > > > > > > > > .type extra_protocol_id_show, @function > > > > extra_protocol_id_show: > > > > endbr64 > > > > 1: call __fentry__ > > > > .section __mcount_loc, "a",@progbits > > > > .quad 1b > > > > .previous > > > > pushq %rbp # > > > > movq %rdx, %rbp # tmp96, buf > > > > pushq %rbx # > > > > # drivers/staging/greybus/gbphy.c:47: { > > > > movq %rdi, %rbx # tmp95, dev > > > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > > > call __sanitizer_cov_trace_pc # > > > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > > > movq -32(%rbx), %rax # MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_8(D) + -40B].cport_desc > > > > # drivers/staging/greybus/gbphy.c:50: return sprintf(buf, "extra 0x%02x %c\n", gbphy_dev->cport_desc->protocol_id, *buf); > > > > movzbl 0(%rbp), %ecx # *buf_9(D), *buf_9(D) > > > > movq %rbp, %rdi # buf, > > > > movq $.LC19, %rsi #, > > > > movzbl 3(%rax), %edx # _3->protocol_id, _3->protocol_id > > > > call sprintf # > > > > # drivers/staging/greybus/gbphy.c:51: } > > > > movl $13, %eax #, > > > > popq %rbx # > > > > popq %rbp # > > > > jmp __x86_return_thunk > > > > .size extra_protocol_id_show, .-extra_protocol_id_show > > > > > > > > Both seem to access the memory directly. Maybe the example is too simple, > > > > and the compiler is more likely to optimize aggressively? > > > > > > Nice, that shows that it is the same both ways as the compiler version > > > you are using is smart enough > > > > > > Which compiler and version is this? Does it work the same for all of > > > the supported versions we have to support (i.e. really old gcc?) > > > > gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0 > > > > I got a similar result for gcc-5: > > > > macro_protocol_id_show: > > 1: call __fentry__ > > .section __mcount_loc, "a",@progbits > > .quad 1b > > .previous > > movq %rdx, %rax # buf, buf > > movq -32(%rdi), %rdx # MEM[(struct gbphy_device *)dev_1(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_1(D) + -40B].cport_desc > > movq $.LC19, %rsi #, > > movq %rax, %rdi # buf, > > movzbl 3(%rdx), %ecx # _3->protocol_id, D.44996 > > movzbl (%rax), %edx # *buf_6(D), D.44996 > > call sprintf # > > cltq > > jmp __x86_return_thunk > > .size macro_protocol_id_show, .-macro_protocol_id_show > > .section .text.unlikely > > .LCOLDE20: > > .text > > .LHOTE20: > > .section .rodata.str1.1 > > .LC21: > > .string "extra 0x%02x %c\n" > > .section .text.unlikely > > .LCOLDB22: > > .text > > .LHOTB22: > > .p2align 6,,63 > > .globl extra_protocol_id_show > > .type extra_protocol_id_show, @function > > extra_protocol_id_show: > > 1: call __fentry__ > > .section __mcount_loc, "a",@progbits > > .quad 1b > > .previous > > movq %rdx, %rax # buf, buf > > movzbl (%rdx), %ecx # *buf_3(D), D.45003 > > movq -32(%rdi), %rdx # MEM[(struct gbphy_device *)dev_2(D) + -40B].cport_desc, MEM[(struct gbphy_device *)dev_2(D) + -40B].cport_desc > > movq $.LC21, %rsi #, > > movq %rax, %rdi # buf, > > movzbl 3(%rdx), %edx # _6->protocol_id, D.45003 > > call sprintf # > > cltq > > jmp __x86_return_thunk > > .size extra_protocol_id_show, .-extra_protocol_id_show > > .section .text.unlikely > > Ah nice! Thanks for checking. > > Ok, so it's not a performance issue at all either way, compilers are > smarter than we think :) I would imagine that the inline would happen first, and then all of the normal optimizations would apply. Given that the function has an inline keyword and is very simple, there is no reason for the inlining not to happen. Naive inlining might introduce new variables for the parameters, but though should be easy for the compiler to get rid of in such a simple case. So intuitively it should be ok. julia > > > > For the most part, sysfs files are not on any sort of "fast path" so a > > > function call is fine, but as I mentioned before, sometimes we are > > > forced to move calls to container_of() to container_of_const() and that > > > can not be an inline function, but must remain a macro :( > > > > It seems that this is because there is not a unique return type, but not a > > performance issue? > > Yes, container_of_const() will return a different type (const or not) > depending on what is passed into it. This allows the "const-ness" of a > pointer to be kept across the call, while as container_of() will always > cast away the const which can be dangerous if you don't know what you > are doing. > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-21 18:34 [PATCH v2] staging: greybus: use inline function for macros Menna Mahmoud 2023-03-21 18:50 ` Alex Elder @ 2023-03-22 9:13 ` Greg KH 1 sibling, 0 replies; 18+ messages in thread From: Greg KH @ 2023-03-22 9:13 UTC (permalink / raw) To: Menna Mahmoud Cc: outreachy, johan, elder, linux-kernel, linux-staging, Julia Lawall On Tue, Mar 21, 2023 at 08:34:56PM +0200, Menna Mahmoud wrote: > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a > static inline function. > > It is not great to have macros that use the `container_of` macro, > because from looking at the definition one cannot tell what type > it applies to. Note, the compiler will tell you if you get this wrong, so this really is not an issue. The container_of() macro is "special" in that it is very type safe and is very commonly used just as a #define to make it simpler and the compiler can just do the pointer math automatically without ever needing a function call to be involved. > One can get the same benefit from an efficiency point of view > by making an inline function. It's actually more efficient to be a macro, so this isn't correct. But all of this is really moot, and I would normally just take this patch, but it's not correct: > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> > --- > changes in v2: > -send patch as a single patch. > -edit the name of struct object. > -edit commit message. > --- > drivers/staging/greybus/gbphy.h | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h > index d4a225b76338..e7ba232bada1 100644 > --- a/drivers/staging/greybus/gbphy.h > +++ b/drivers/staging/greybus/gbphy.h > @@ -15,7 +15,10 @@ struct gbphy_device { > struct list_head list; > struct device dev; > }; > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > +static inline struct gbphy_device *to_gbphy_dev(const struct device *_dev) > +{ > + return container_of(_dev, struct gbphy_device, dev); > +} You need a newline right before your new function to properly set it off. > > static inline void *gb_gbphy_get_data(struct gbphy_device *gdev) > { > @@ -43,7 +46,10 @@ struct gbphy_driver { > > struct device_driver driver; > }; > -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver) > +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *drv) > +{ > + return container_of(drv, struct gbphy_driver, driver); > +} I'm going to be a stickler here, and say this really should be using the new container_of_const() macro instead, and with that, you can NOT turn it into an inline function at all due to the fun use of Generic in that macro. So I wouldn't recommend changing this macro at this time at all, sorry. thanks, greg k-h ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] staging: greybus: use inline function for macros @ 2023-03-19 20:49 Menna Mahmoud 2023-03-19 20:56 ` Julia Lawall 0 siblings, 1 reply; 18+ messages in thread From: Menna Mahmoud @ 2023-03-19 20:49 UTC (permalink / raw) To: gregkh Cc: outreachy, johan, elder, linux-kernel, linux-staging, eng.mennamahmoud.mm, Julia Lawall Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a static inline function. it is not great to have macro that use `container_of` macro, because from looking at the definition one cannot tell what type it applies to. One can get the same benefit from an efficiency point of view by making an inline function. Suggested-by: Julia Lawall <julia.lawall@inria.fr> Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> --- changes in v2: remove newlines added in previous patch. --- drivers/staging/greybus/gbphy.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h index 1de510499480..03a977056637 100644 --- a/drivers/staging/greybus/gbphy.h +++ b/drivers/staging/greybus/gbphy.h @@ -15,8 +15,10 @@ struct gbphy_device { struct list_head list; struct device dev; }; - -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) +static inline struct gbphy_device *to_gbphy_dev(const struct device *d) +{ + return container_of(d, struct gbphy_device, dev); +} static inline void *gb_gbphy_get_data(struct gbphy_device *gdev) { @@ -44,8 +46,10 @@ struct gbphy_driver { struct device_driver driver; }; - -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver) +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *d) +{ + return container_of(d, struct gbphy_driver, driver); +} int gb_gbphy_register_driver(struct gbphy_driver *driver, struct module *owner, const char *mod_name); -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] staging: greybus: use inline function for macros 2023-03-19 20:49 Menna Mahmoud @ 2023-03-19 20:56 ` Julia Lawall 0 siblings, 0 replies; 18+ messages in thread From: Julia Lawall @ 2023-03-19 20:56 UTC (permalink / raw) To: Menna Mahmoud Cc: gregkh, outreachy, johan, elder, linux-kernel, linux-staging On Sun, 19 Mar 2023, Menna Mahmoud wrote: > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a > static inline function. > > it is not great to have macro that use `container_of` macro, > because from looking at the definition one cannot tell what type > it applies to. > > One can get the same benefit from an efficiency point of view > by making an inline function. > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> > --- > changes in v2: > remove newlines added in previous patch. This is not the right solution. Greg is supposed to ignore your previous patch, so he won't have the newlines that you are removing. julia > --- > drivers/staging/greybus/gbphy.h | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h > index 1de510499480..03a977056637 100644 > --- a/drivers/staging/greybus/gbphy.h > +++ b/drivers/staging/greybus/gbphy.h > @@ -15,8 +15,10 @@ struct gbphy_device { > struct list_head list; > struct device dev; > }; > - > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev) > +static inline struct gbphy_device *to_gbphy_dev(const struct device *d) > +{ > + return container_of(d, struct gbphy_device, dev); > +} > > static inline void *gb_gbphy_get_data(struct gbphy_device *gdev) > { > @@ -44,8 +46,10 @@ struct gbphy_driver { > > struct device_driver driver; > }; > - > -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver) > +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *d) > +{ > + return container_of(d, struct gbphy_driver, driver); > +} > > int gb_gbphy_register_driver(struct gbphy_driver *driver, > struct module *owner, const char *mod_name); > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-03-25 9:29 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-21 18:34 [PATCH v2] staging: greybus: use inline function for macros Menna Mahmoud 2023-03-21 18:50 ` Alex Elder 2023-03-21 20:43 ` Julia Lawall 2023-03-21 21:09 ` Alex Elder 2023-03-21 21:29 ` Julia Lawall 2023-03-21 22:42 ` Alex Elder 2023-03-22 10:00 ` Julia Lawall 2023-03-22 12:39 ` Alex Elder 2023-03-23 4:58 ` Greg KH 2023-03-23 5:05 ` Deepak R Varma 2023-03-23 5:22 ` Greg KH 2023-03-23 19:46 ` Deepak R Varma 2023-03-23 9:52 ` Julia Lawall 2023-03-25 8:49 ` Greg KH 2023-03-25 9:28 ` Julia Lawall 2023-03-22 9:13 ` Greg KH -- strict thread matches above, loose matches on Subject: below -- 2023-03-19 20:49 Menna Mahmoud 2023-03-19 20:56 ` Julia Lawall
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).