| Message ID | 20220208184208.79303-6-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | locking: Separate lock tracepoints from lockdep/lock_stat (v1) | expand |
On Tue, 08 Feb 2022, Namhyung Kim <namhyung@kernel.org> wrote: > With upcoming lock tracepoints config, it'd define some of lockdep > functions without enabling CONFIG_LOCKDEP actually. The existing code > assumes those functions will be removed by the preprocessor but it's > not the case anymore. Let's protect the code with #ifdef's explicitly. I don't understand why you can't keep the no-op stubs for CONFIG_LOCKDEP=n. BR, Jani. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > drivers/gpu/drm/i915/intel_wakeref.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c > index dfd87d082218..6e4b8d036283 100644 > --- a/drivers/gpu/drm/i915/intel_wakeref.c > +++ b/drivers/gpu/drm/i915/intel_wakeref.c > @@ -106,8 +106,11 @@ void __intel_wakeref_init(struct intel_wakeref *wf, > wf->wakeref = 0; > > INIT_DELAYED_WORK(&wf->work, __intel_wakeref_put_work); > + > +#ifdef CONFIG_LOCKDEP > lockdep_init_map(&wf->work.work.lockdep_map, > "wakeref.work", &key->work, 0); > +#endif > } > > int intel_wakeref_wait_for_idle(struct intel_wakeref *wf)
Hello, On Tue, Feb 8, 2022 at 10:51 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Tue, 08 Feb 2022, Namhyung Kim <namhyung@kernel.org> wrote: > > With upcoming lock tracepoints config, it'd define some of lockdep > > functions without enabling CONFIG_LOCKDEP actually. The existing code > > assumes those functions will be removed by the preprocessor but it's > > not the case anymore. Let's protect the code with #ifdef's explicitly. > > I don't understand why you can't keep the no-op stubs for > CONFIG_LOCKDEP=n. Because I want to use the lockdep annotation for other purposes. But the workqueue lockdep_map was defined under LOCKDEP only. Please see the description in the cover letter. https://lore.kernel.org/all/20220208184208.79303-1-namhyung@kernel.org/ Thanks, Namhyung
On Tue, 08 Feb 2022, Namhyung Kim <namhyung@kernel.org> wrote: > Hello, > > On Tue, Feb 8, 2022 at 10:51 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: >> >> On Tue, 08 Feb 2022, Namhyung Kim <namhyung@kernel.org> wrote: >> > With upcoming lock tracepoints config, it'd define some of lockdep >> > functions without enabling CONFIG_LOCKDEP actually. The existing code >> > assumes those functions will be removed by the preprocessor but it's >> > not the case anymore. Let's protect the code with #ifdef's explicitly. >> >> I don't understand why you can't keep the no-op stubs for >> CONFIG_LOCKDEP=n. > > Because I want to use the lockdep annotation for other purposes. > But the workqueue lockdep_map was defined under LOCKDEP > only. Please see the description in the cover letter. > > https://lore.kernel.org/all/20220208184208.79303-1-namhyung@kernel.org/ So lockdep_init_map() might still be there and build just fine for CONFIG_LOCKDEP=n, but now we're actually required to wrap all call sites in #ifdefs depending on the purpose? I'm not convinced yet. BR, Jani.
On Wed, 09 Feb 2022 15:49:01 +0200 Jani Nikula <jani.nikula@linux.intel.com> wrote: > > Because I want to use the lockdep annotation for other purposes. > > But the workqueue lockdep_map was defined under LOCKDEP > > only. Please see the description in the cover letter. > > > > https://lore.kernel.org/all/20220208184208.79303-1-namhyung@kernel.org/ > > So lockdep_init_map() might still be there and build just fine for > CONFIG_LOCKDEP=n, but now we're actually required to wrap all call sites > in #ifdefs depending on the purpose? I'm not convinced yet. I addressed this already. I suggested to add a "raw" variant that turns to a nop when LOCKDEP is not enabled. That is, for those locations that are only for working with LOCKDEP, the call will be: lockdep_init_map_raw() This will differentiate the locations that are for just lockdep and those that are for both lockdep and tracing. -- Steve
Hello, On Wed, Feb 9, 2022 at 8:27 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 09 Feb 2022 15:49:01 +0200 > Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > > Because I want to use the lockdep annotation for other purposes. > > > But the workqueue lockdep_map was defined under LOCKDEP > > > only. Please see the description in the cover letter. > > > > > > https://lore.kernel.org/all/20220208184208.79303-1-namhyung@kernel.org/ > > > > So lockdep_init_map() might still be there and build just fine for > > CONFIG_LOCKDEP=n, but now we're actually required to wrap all call sites > > in #ifdefs depending on the purpose? I'm not convinced yet. Because work_struct.lockdep_map is there only if CONFIG_LOCKDEP=y. > > I addressed this already. I suggested to add a "raw" variant that turns to > a nop when LOCKDEP is not enabled. That is, for those locations that are > only for working with LOCKDEP, the call will be: > > lockdep_init_map_raw() > > This will differentiate the locations that are for just lockdep and those > that are for both lockdep and tracing. Yep, this should be fine if it's actually defined on CONFIG_LOCKDEP=y. Thanks, Namhyung
diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c index dfd87d082218..6e4b8d036283 100644 --- a/drivers/gpu/drm/i915/intel_wakeref.c +++ b/drivers/gpu/drm/i915/intel_wakeref.c @@ -106,8 +106,11 @@ void __intel_wakeref_init(struct intel_wakeref *wf, wf->wakeref = 0; INIT_DELAYED_WORK(&wf->work, __intel_wakeref_put_work); + +#ifdef CONFIG_LOCKDEP lockdep_init_map(&wf->work.work.lockdep_map, "wakeref.work", &key->work, 0); +#endif } int intel_wakeref_wait_for_idle(struct intel_wakeref *wf)
With upcoming lock tracepoints config, it'd define some of lockdep functions without enabling CONFIG_LOCKDEP actually. The existing code assumes those functions will be removed by the preprocessor but it's not the case anymore. Let's protect the code with #ifdef's explicitly. Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- drivers/gpu/drm/i915/intel_wakeref.c | 3 +++ 1 file changed, 3 insertions(+)