Message ID | 20250417123155.4002358-1-sunil.khatri@amd.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [V6,1/5] drm: add drm_file_err function to add process info | expand |
On 17/04/2025 13:31, Sunil Khatri wrote: > Add a drm helper function which append the process information for appends > the drm_file over drm_err formated output. formatted > v5: change to macro from function (Christian Koenig) > add helper functions for lock/unlock (Christian Koenig) > > v6: remove __maybe_unused and make function inline (Jani Nikula) > remove drm_print.h > > v7: Use va_format and %pV to concatenate fmt and vargs (Jani Nikula) > > Signed-off-by: Sunil Khatri <sunil.khatri@amd.com> > --- > drivers/gpu/drm/drm_file.c | 34 ++++++++++++++++++++++++++++++++++ > include/drm/drm_file.h | 3 +++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > index c299cd94d3f7..7e64d84d4e2d 100644 > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c > @@ -986,6 +986,40 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f) > } > EXPORT_SYMBOL(drm_show_fdinfo); > > +/** > + * drm_file_err - Fill info string with process name and pid Update to something like: drm_file_err - Log error message associated with a drm_file client > + * @file_priv: context of interest for process name and pid > + * @fmt: prinf() like format string printf > + * > + * This update the user provided buffer with process > + * name and pid information for @file_priv Also stale but it may be okay to drop altogether since short description feels enough for a logging helper. > + */ > +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...) > +{ > + struct task_struct *task; > + struct pid *pid; > + struct drm_device *dev = file_priv->minor->dev; > + va_list args; > + struct va_format vaf; You could tidy the declaration block a bit, usually ordering from longer to narrower lines for readability but up to you. > + > + va_start(args, fmt); > + vaf.fmt = fmt; > + vaf.va = &args; > + > + mutex_lock(&file_priv->client_name_lock); > + rcu_read_lock(); > + pid = rcu_dereference(file_priv->pid); > + task = pid_task(pid, PIDTYPE_TGID); > + > + drm_err(dev, "comm: %s pid: %d client: %s %pV", task ? task->comm : "", > + task ? task->pid : 0, file_priv->client_name ?: "Unset", &vaf); > + Hm is task->pid the tid or tgid? Could be the same thing due getting PIDTYPE_GID.. PID namespaces are always endlessly confusing to me since I don't work in that area often enough. And I have some bikeshedding ideas for the format, like maybe consolidating with naming used in drm_clients_info() and adding some separator between the client data and the log message that follows. Like: "Client %s[%d] (%s): %pV", task ? task->comm : "<unknown>", pid_nr(pid), file_priv->client_name ?: "<unset>", ... And even if I am a bit unsure about the "<unknown>", I think it doesn't harm to be consistent. It is fine as is though, so you decide if you want to change anything or not. If you could please double check task->pid is indeed definitely TGID: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Regards, Tvrtko > + va_end(args); > + rcu_read_unlock(); > + mutex_unlock(&file_priv->client_name_lock); > +} > +EXPORT_SYMBOL(drm_file_err); > + > /** > * mock_drm_getfile - Create a new struct file for the drm device > * @minor: drm minor to wrap (e.g. #drm_device.primary) > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 94d365b22505..5c3b2aa3e69d 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -446,6 +446,9 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv) > return file_priv->minor->type == DRM_MINOR_ACCEL; > } > > +__printf(2, 3) > +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...); > + > void drm_file_update_pid(struct drm_file *); > > struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
On 4/17/2025 7:04 PM, Tvrtko Ursulin wrote: > > On 17/04/2025 13:31, Sunil Khatri wrote: >> Add a drm helper function which append the process information for > > appends Noted > >> the drm_file over drm_err formated output. > > formatted 'Noted > >> v5: change to macro from function (Christian Koenig) >> add helper functions for lock/unlock (Christian Koenig) >> >> v6: remove __maybe_unused and make function inline (Jani Nikula) >> remove drm_print.h >> >> v7: Use va_format and %pV to concatenate fmt and vargs (Jani Nikula) >> >> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com> >> --- >> drivers/gpu/drm/drm_file.c | 34 ++++++++++++++++++++++++++++++++++ >> include/drm/drm_file.h | 3 +++ >> 2 files changed, 37 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c >> index c299cd94d3f7..7e64d84d4e2d 100644 >> --- a/drivers/gpu/drm/drm_file.c >> +++ b/drivers/gpu/drm/drm_file.c >> @@ -986,6 +986,40 @@ void drm_show_fdinfo(struct seq_file *m, struct >> file *f) >> } >> EXPORT_SYMBOL(drm_show_fdinfo); >> +/** >> + * drm_file_err - Fill info string with process name and pid > > Update to something like: > > drm_file_err - Log error message associated with a drm_file client > >> + * @file_priv: context of interest for process name and pid >> + * @fmt: prinf() like format string > > printf > >> + * >> + * This update the user provided buffer with process >> + * name and pid information for @file_priv > > Also stale but it may be okay to drop altogether since short > description feels enough for a logging helper. > >> + */ >> +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...) >> +{ >> + struct task_struct *task; >> + struct pid *pid; >> + struct drm_device *dev = file_priv->minor->dev; >> + va_list args; >> + struct va_format vaf; > > You could tidy the declaration block a bit, usually ordering from > longer to narrower lines for readability but up to you. Sure >> + >> + va_start(args, fmt); >> + vaf.fmt = fmt; >> + vaf.va = &args; >> + >> + mutex_lock(&file_priv->client_name_lock); >> + rcu_read_lock(); >> + pid = rcu_dereference(file_priv->pid); >> + task = pid_task(pid, PIDTYPE_TGID); >> + >> + drm_err(dev, "comm: %s pid: %d client: %s %pV", task ? >> task->comm : "", >> + task ? task->pid : 0, file_priv->client_name ?: "Unset", &vaf); >> + > > Hm is task->pid the tid or tgid? Could be the same thing due getting > PIDTYPE_GID.. PID namespaces are always endlessly confusing to me > since I don't work in that area often enough. > There are two parameters in task_struct one is pid and tgid and both are different if i am not wrong. In case of multi thread pid and tgid is different if its one thread i guess in that case pid and tgid are same but even i am not 100% sure. > And I have some bikeshedding ideas for the format, like maybe > consolidating with naming used in drm_clients_info() and adding some > separator between the client data and the log message that follows. Like: > > "Client %s[%d] (%s): %pV", > task ? task->comm : "<unknown>", > pid_nr(pid), > file_priv->client_name ?: "<unset>", > ... Sure > > And even if I am a bit unsure about the "<unknown>", I think it > doesn't harm to be consistent. It is fine as is though, so you decide > if you want to change anything or not. > > If you could please double check task->pid is indeed definitely TGID: > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > Regards, > > Tvrtko > >> + va_end(args); >> + rcu_read_unlock(); >> + mutex_unlock(&file_priv->client_name_lock); >> +} >> +EXPORT_SYMBOL(drm_file_err); >> + >> /** >> * mock_drm_getfile - Create a new struct file for the drm device >> * @minor: drm minor to wrap (e.g. #drm_device.primary) >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h >> index 94d365b22505..5c3b2aa3e69d 100644 >> --- a/include/drm/drm_file.h >> +++ b/include/drm/drm_file.h >> @@ -446,6 +446,9 @@ static inline bool drm_is_accel_client(const >> struct drm_file *file_priv) >> return file_priv->minor->type == DRM_MINOR_ACCEL; >> } >> +__printf(2, 3) >> +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...); >> + >> void drm_file_update_pid(struct drm_file *); >> struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, >> unsigned int minor_id); >
For rest of the patches which are part of the amdgpu tree will push incorporating changes as shared by @Tvrtko Ursulin <tvrtko.ursulin@igalia.com> once drm change is merged. Thanks a lot all for the reviews. Regards Sunil Khatri On 4/17/2025 6:01 PM, Sunil Khatri wrote: > Add a drm helper function which append the process information for > the drm_file over drm_err formated output. > > v5: change to macro from function (Christian Koenig) > add helper functions for lock/unlock (Christian Koenig) > > v6: remove __maybe_unused and make function inline (Jani Nikula) > remove drm_print.h > > v7: Use va_format and %pV to concatenate fmt and vargs (Jani Nikula) > > Signed-off-by: Sunil Khatri <sunil.khatri@amd.com> > --- > drivers/gpu/drm/drm_file.c | 34 ++++++++++++++++++++++++++++++++++ > include/drm/drm_file.h | 3 +++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > index c299cd94d3f7..7e64d84d4e2d 100644 > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c > @@ -986,6 +986,40 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f) > } > EXPORT_SYMBOL(drm_show_fdinfo); > > +/** > + * drm_file_err - Fill info string with process name and pid > + * @file_priv: context of interest for process name and pid > + * @fmt: prinf() like format string > + * > + * This update the user provided buffer with process > + * name and pid information for @file_priv > + */ > +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...) > +{ > + struct task_struct *task; > + struct pid *pid; > + struct drm_device *dev = file_priv->minor->dev; > + va_list args; > + struct va_format vaf; > + > + va_start(args, fmt); > + vaf.fmt = fmt; > + vaf.va = &args; > + > + mutex_lock(&file_priv->client_name_lock); > + rcu_read_lock(); > + pid = rcu_dereference(file_priv->pid); > + task = pid_task(pid, PIDTYPE_TGID); > + > + drm_err(dev, "comm: %s pid: %d client: %s %pV", task ? task->comm : "", > + task ? task->pid : 0, file_priv->client_name ?: "Unset", &vaf); > + > + va_end(args); > + rcu_read_unlock(); > + mutex_unlock(&file_priv->client_name_lock); > +} > +EXPORT_SYMBOL(drm_file_err); > + > /** > * mock_drm_getfile - Create a new struct file for the drm device > * @minor: drm minor to wrap (e.g. #drm_device.primary) > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 94d365b22505..5c3b2aa3e69d 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -446,6 +446,9 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv) > return file_priv->minor->type == DRM_MINOR_ACCEL; > } > > +__printf(2, 3) > +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...); > + > void drm_file_update_pid(struct drm_file *); > > struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index c299cd94d3f7..7e64d84d4e2d 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -986,6 +986,40 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f) } EXPORT_SYMBOL(drm_show_fdinfo); +/** + * drm_file_err - Fill info string with process name and pid + * @file_priv: context of interest for process name and pid + * @fmt: prinf() like format string + * + * This update the user provided buffer with process + * name and pid information for @file_priv + */ +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...) +{ + struct task_struct *task; + struct pid *pid; + struct drm_device *dev = file_priv->minor->dev; + va_list args; + struct va_format vaf; + + va_start(args, fmt); + vaf.fmt = fmt; + vaf.va = &args; + + mutex_lock(&file_priv->client_name_lock); + rcu_read_lock(); + pid = rcu_dereference(file_priv->pid); + task = pid_task(pid, PIDTYPE_TGID); + + drm_err(dev, "comm: %s pid: %d client: %s %pV", task ? task->comm : "", + task ? task->pid : 0, file_priv->client_name ?: "Unset", &vaf); + + va_end(args); + rcu_read_unlock(); + mutex_unlock(&file_priv->client_name_lock); +} +EXPORT_SYMBOL(drm_file_err); + /** * mock_drm_getfile - Create a new struct file for the drm device * @minor: drm minor to wrap (e.g. #drm_device.primary) diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 94d365b22505..5c3b2aa3e69d 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -446,6 +446,9 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_ACCEL; } +__printf(2, 3) +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...); + void drm_file_update_pid(struct drm_file *); struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
Add a drm helper function which append the process information for the drm_file over drm_err formated output. v5: change to macro from function (Christian Koenig) add helper functions for lock/unlock (Christian Koenig) v6: remove __maybe_unused and make function inline (Jani Nikula) remove drm_print.h v7: Use va_format and %pV to concatenate fmt and vargs (Jani Nikula) Signed-off-by: Sunil Khatri <sunil.khatri@amd.com> --- drivers/gpu/drm/drm_file.c | 34 ++++++++++++++++++++++++++++++++++ include/drm/drm_file.h | 3 +++ 2 files changed, 37 insertions(+)