| Message ID | alpine.DEB.2.20.1803292109180.2750@hadrien (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Looks okay to me, I'll squash this into the original patch. Julia Lawall <julia.lawall@lip6.fr> writes: > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE > for debugfs files. > > Semantic patch information: > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() > imposes some significant overhead as compared to > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). > > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > Fixes: 9eec7989e762 ("OPTIONAL: cpufreq/intel_pstate: Expose LP controller parameters via debugfs.") > CC: Francisco Jerez <currojerez@riseup.net> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr> > --- > > I don't actually know anything about this issue. The change was suggested > by kbuild. > > intel_pstate.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -885,7 +885,7 @@ static int lp_param_get(void *data, u64 > *val = *(u32 *)data; > return 0; > } > -DEFINE_SIMPLE_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n"); > > static struct dentry *debugfs_parent; > > @@ -922,9 +922,10 @@ static void intel_pstate_debug_expose_pa > for (i = 0; lp_files[i].name; i++) { > struct dentry *dentry; > > - dentry = debugfs_create_file(lp_files[i].name, 0660, > - debugfs_parent, lp_files[i].value, > - &fops_lp_param); > + dentry = debugfs_create_file_unsafe(lp_files[i].name, 0660, > + debugfs_parent, > + lp_files[i].value, > + &fops_lp_param); > if (!IS_ERR(dentry)) > lp_files[i].dentry = dentry; > }
Fabio Estevam <festevam@gmail.com> writes: > Hi Julia, > > On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: >> Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE >> for debugfs files. >> >> Semantic patch information: >> Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() >> imposes some significant overhead as compared to >> DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). > > Just curious: could you please expand on what "imposes some > significant overhead" means? > Probably negligible given that this code will only be run once at system boot and then never used again in production systems. But I guess the micro-optimization doesn't hurt either. > Thanks
Hi Julia, On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE > for debugfs files. > > Semantic patch information: > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() > imposes some significant overhead as compared to > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). Just curious: could you please expand on what "imposes some significant overhead" means? Thanks
On Thu, 29 Mar 2018, Fabio Estevam wrote: > Hi Julia, > > On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE > > for debugfs files. > > > > Semantic patch information: > > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() > > imposes some significant overhead as compared to > > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). > > Just curious: could you please expand on what "imposes some > significant overhead" means? I don't know. I didn't write this rule. Nicolai, can you explain? thanks, julia
Julia Lawall <julia.lawall@lip6.fr> writes: > On Thu, 29 Mar 2018, Fabio Estevam wrote: > >> Hi Julia, >> >> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: >> > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE >> > for debugfs files. >> > >> > Semantic patch information: >> > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() >> > imposes some significant overhead as compared to >> > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). >> >> Just curious: could you please expand on what "imposes some >> significant overhead" means? > > I don't know. I didn't write this rule. Nicolai, can you explain? From commit 49d200deaa68 ("debugfs: prevent access to removed files' private data"): Upon return of debugfs_remove()/debugfs_remove_recursive(), it might still be attempted to access associated private file data through previously opened struct file objects. If that data has been freed by the caller of debugfs_remove*() in the meanwhile, the reading/writing process would either encounter a fault or, if the memory address in question has been reassigned again, unrelated data structures could get overwritten. [...] Currently, there are ~1000 call sites of debugfs_create_file() spread throughout the whole tree and touching all of those struct file_operations in order to make them file removal aware by means of checking the result of debugfs_use_file_start() from within their methods is unfeasible. Instead, wrap the struct file_operations by a lifetime managing proxy at file open [...] The additional overhead comes in terms of additional memory needed: for debugs files created through debugfs_create_file(), one such struct file_operations proxy is allocated for each struct file instantiation, c.f. full_proxy_open(). This was needed to "repair" the ~1000 call sites without touching them. New debugfs users should make their file_operations removal aware themselves by means of DEFINE_DEBUGFS_ATTRIBUTE() and signal that fact to the debugfs core by instantiating them through debugfs_create_file_unsafe(). See commit c64688081490 ("debugfs: add support for self-protecting attribute file fops") for further information. Thanks, Nicolai
On Fri, 30 Mar 2018, Nicolai Stange wrote: > Julia Lawall <julia.lawall@lip6.fr> writes: > > > On Thu, 29 Mar 2018, Fabio Estevam wrote: > > > >> Hi Julia, > >> > >> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: > >> > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE > >> > for debugfs files. > >> > > >> > Semantic patch information: > >> > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() > >> > imposes some significant overhead as compared to > >> > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). > >> > >> Just curious: could you please expand on what "imposes some > >> significant overhead" means? > > > > I don't know. I didn't write this rule. Nicolai, can you explain? > > From commit 49d200deaa68 ("debugfs: prevent access to removed files' private > data"): > > Upon return of debugfs_remove()/debugfs_remove_recursive(), it might > still be attempted to access associated private file data through > previously opened struct file objects. If that data has been freed by > the caller of debugfs_remove*() in the meanwhile, the reading/writing > process would either encounter a fault or, if the memory address in > question has been reassigned again, unrelated data structures could get > overwritten. > [...] > Currently, there are ~1000 call sites of debugfs_create_file() spread > throughout the whole tree and touching all of those struct file_operations > in order to make them file removal aware by means of checking the result of > debugfs_use_file_start() from within their methods is unfeasible. > > Instead, wrap the struct file_operations by a lifetime managing proxy at > file open [...] > > The additional overhead comes in terms of additional memory needed: for > debugs files created through debugfs_create_file(), one such struct > file_operations proxy is allocated for each struct file instantiation, > c.f. full_proxy_open(). > > This was needed to "repair" the ~1000 call sites without touching them. > > New debugfs users should make their file_operations removal aware > themselves by means of DEFINE_DEBUGFS_ATTRIBUTE() and signal that fact to > the debugfs core by instantiating them through > debugfs_create_file_unsafe(). > > See commit c64688081490 ("debugfs: add support for self-protecting > attribute file fops") for further information. Thanks. Perhaps it would be good to add a reference to this commit in the message generated by the semantic patch. Would it be sufficient to just apply the semantic patch everywhere and submit the patches? julia > > > Thanks, > > Nicolai > > > -- > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, > HRB 21284 (AG Nürnberg) >
Hi Julia, On Thursday, March 29, 2018 9:12:06 PM CEST Julia Lawall wrote: > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE > for debugfs files. > > Semantic patch information: > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() > imposes some significant overhead as compared to > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). > > Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci We've dropped the debugfs bits from intel_pstate entirely, so this change is not applicable any more. Thanks! > Fixes: 9eec7989e762 ("OPTIONAL: cpufreq/intel_pstate: Expose LP controller parameters via debugfs.") > CC: Francisco Jerez <currojerez@riseup.net> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr> > --- > > I don't actually know anything about this issue. The change was suggested > by kbuild. > > intel_pstate.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -885,7 +885,7 @@ static int lp_param_get(void *data, u64 > *val = *(u32 *)data; > return 0; > } > -DEFINE_SIMPLE_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n"); > > static struct dentry *debugfs_parent; > > @@ -922,9 +922,10 @@ static void intel_pstate_debug_expose_pa > for (i = 0; lp_files[i].name; i++) { > struct dentry *dentry; > > - dentry = debugfs_create_file(lp_files[i].name, 0660, > - debugfs_parent, lp_files[i].value, > - &fops_lp_param); > + dentry = debugfs_create_file_unsafe(lp_files[i].name, 0660, > + debugfs_parent, > + lp_files[i].value, > + &fops_lp_param); > if (!IS_ERR(dentry)) > lp_files[i].dentry = dentry; > } >
On Fri, Mar 30, 2018 at 3:22 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: >> From commit 49d200deaa68 ("debugfs: prevent access to removed files' private >> data"): >> >> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might >> still be attempted to access associated private file data through >> previously opened struct file objects. If that data has been freed by >> the caller of debugfs_remove*() in the meanwhile, the reading/writing >> process would either encounter a fault or, if the memory address in >> question has been reassigned again, unrelated data structures could get >> overwritten. >> [...] >> Currently, there are ~1000 call sites of debugfs_create_file() spread >> throughout the whole tree and touching all of those struct file_operations >> in order to make them file removal aware by means of checking the result of >> debugfs_use_file_start() from within their methods is unfeasible. >> >> Instead, wrap the struct file_operations by a lifetime managing proxy at >> file open [...] >> >> The additional overhead comes in terms of additional memory needed: for >> debugs files created through debugfs_create_file(), one such struct >> file_operations proxy is allocated for each struct file instantiation, >> c.f. full_proxy_open(). >> >> This was needed to "repair" the ~1000 call sites without touching them. >> >> New debugfs users should make their file_operations removal aware >> themselves by means of DEFINE_DEBUGFS_ATTRIBUTE() and signal that fact to >> the debugfs core by instantiating them through >> debugfs_create_file_unsafe(). >> >> See commit c64688081490 ("debugfs: add support for self-protecting >> attribute file fops") for further information. Thanks for the detailed explanation, Nicolai! > Thanks. Perhaps it would be good to add a reference to this commit in > the message generated by the semantic patch. Yes, that would be very helpful indeed. Thanks
Julia Lawall <julia.lawall@lip6.fr> writes: > On Fri, 30 Mar 2018, Nicolai Stange wrote: > >> Julia Lawall <julia.lawall@lip6.fr> writes: >> >> > On Thu, 29 Mar 2018, Fabio Estevam wrote: >> > >> >> Hi Julia, >> >> >> >> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: >> >> > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE >> >> > for debugfs files. >> >> > >> >> > Semantic patch information: >> >> > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() >> >> > imposes some significant overhead as compared to >> >> > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). >> >> >> >> Just curious: could you please expand on what "imposes some >> >> significant overhead" means? >> > >> > I don't know. I didn't write this rule. Nicolai, can you explain? >> >> From commit 49d200deaa68 ("debugfs: prevent access to removed files' private >> data"): >> >> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might >> still be attempted to access associated private file data through >> previously opened struct file objects. If that data has been freed by >> the caller of debugfs_remove*() in the meanwhile, the reading/writing >> process would either encounter a fault or, if the memory address in >> question has been reassigned again, unrelated data structures could get >> overwritten. >> [...] >> Currently, there are ~1000 call sites of debugfs_create_file() spread >> throughout the whole tree and touching all of those struct file_operations >> in order to make them file removal aware by means of checking the result of >> debugfs_use_file_start() from within their methods is unfeasible. >> >> Instead, wrap the struct file_operations by a lifetime managing proxy at >> file open [...] >> >> The additional overhead comes in terms of additional memory needed: for >> debugs files created through debugfs_create_file(), one such struct >> file_operations proxy is allocated for each struct file instantiation, >> c.f. full_proxy_open(). >> >> This was needed to "repair" the ~1000 call sites without touching them. >> >> New debugfs users should make their file_operations removal aware >> themselves by means of DEFINE_DEBUGFS_ATTRIBUTE() and signal that fact to >> the debugfs core by instantiating them through >> debugfs_create_file_unsafe(). >> >> See commit c64688081490 ("debugfs: add support for self-protecting >> attribute file fops") for further information. > > Thanks. Perhaps it would be good to add a reference to this commit in > the message generated by the semantic patch. Thanks for doing this! > > Would it be sufficient to just apply the semantic patch everywhere and > submit the patches? In principle yes. But I'm note sure whether such a mass application is worth it: the proxy allocation happens only at file open and the expectation is that there aren't that many debugfs files kept open at a time. OTOH, a struct file_operation consumes 256 bytes with sizeof(long) == 8. In my opinion, new users should avoid this overhead as it's easily doable. For existing ones, I don't know. Thanks, Nicolai
--- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -885,7 +885,7 @@ static int lp_param_get(void *data, u64 *val = *(u32 *)data; return 0; } -DEFINE_SIMPLE_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n"); static struct dentry *debugfs_parent; @@ -922,9 +922,10 @@ static void intel_pstate_debug_expose_pa for (i = 0; lp_files[i].name; i++) { struct dentry *dentry; - dentry = debugfs_create_file(lp_files[i].name, 0660, - debugfs_parent, lp_files[i].value, - &fops_lp_param); + dentry = debugfs_create_file_unsafe(lp_files[i].name, 0660, + debugfs_parent, + lp_files[i].value, + &fops_lp_param); if (!IS_ERR(dentry)) lp_files[i].dentry = dentry; }
Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for debugfs files. Semantic patch information: Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() imposes some significant overhead as compared to DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci Fixes: 9eec7989e762 ("OPTIONAL: cpufreq/intel_pstate: Expose LP controller parameters via debugfs.") CC: Francisco Jerez <currojerez@riseup.net> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr> --- I don't actually know anything about this issue. The change was suggested by kbuild. intel_pstate.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)