Re: [Intel-xe] [PATCH] drm/xe: Prevent flooding the kernel log with XE_IOCTL_ERR

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, May 31, 2023 at 09:45:07AM -0700, Lucas De Marchi wrote:
> On Thu, May 25, 2023 at 04:16:43PM +0000, Francois Dugast wrote:
> > Lower log level of XE_IOCTL_ERR macro to debug in order to prevent
> > flooding kernel log. Rely on drm_dbg macro output which includes
> > function name, so removing file and name macros.
> > 
> > Reported-by: Oded Gabbay <ogabbay@kernel.org>
> > Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_macros.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
> > index 0d24c124d202..2b8a46ffb93e 100644
> > --- a/drivers/gpu/drm/xe/xe_macros.h
> > +++ b/drivers/gpu/drm/xe/xe_macros.h
> > @@ -13,8 +13,8 @@
> > #define XE_BUG_ON BUG_ON
> > 
> > #define XE_IOCTL_ERR(xe, cond) \
> 
> 		   ^^^
> 
> > -	((cond) && (drm_info(&(xe)->drm, \
> > -			    "Ioctl argument check failed at %s:%d: %s", \
> > -			    __FILE__, __LINE__, #cond), 1))
> > +	((cond) && (drm_dbg(&(xe)->drm, \
> 
> 			^^^
> 
> now you have a mismatch.
> 
> From a quick look we already have
> several places with mixed usage. See e.g. engine_set_timeslice(),
> engine_set_preemption_timeout(), engine_set_priority(). There are also a
> few places that will log on ENOMEM, which should not be there, but is
> not caught by checkpatch since it's obfuscated in the macro (see
> scripts/checkpatch.pl - "# check for unnecessary "Out of Memory"
> messages")
> 
> Can we get away simply removing it?  retsnoop is a thing nowadays and to
> understand where the error is coming from, userspace could simply run it
> on the side... something like
> 
> 	# ./retsnoop -e '*sys_ioctl' -a ':drivers/gpu/drm/xe/*.c'
> 
> which returns more useful things if the error is farther down the
> call stack.

That requires manual execution though...I think there's still value in
having debug-level messages for failures like these so that we get
meaningful CI logs when something goes wrong.  It's not always quick or
easy to grab a suitable machine to reproduce a CI-reported failure
locally with retsnoop.


Matt

> 
> Lucas De Marchi
> 
> > +			    "Ioctl argument check failed: %s", \
> > +			    #cond), 1))
> > 
> > #endif
> > -- 
> > 2.34.1
> > 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation