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

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

On Thu, Jun 01, 2023 at 10:28:41AM -0700, Lucas De Marchi wrote:
> On Thu, Jun 01, 2023 at 09:41:06AM -0700, Matt Roper wrote:
> > 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.
> 
> so XE_IOCTL_DBG()? XE_IOCTL_CHECK()?
> 
> Not a pattern I see much in the kernel, but other projects use is below.
> It removes the dbg/err mismatch and makes the code shorter to read, but
> the return inside the macro is not something very common... but hey,
> using the comma operator is not that common neither :). Thoughts?

One concern with "_ASSERT" is that people may incorrectly assume that
it's something that only gets compiled into debug builds.  But with a
different name like "_CHECK" or "_EXPECT" your proposal below looks okay
to me.

We may find cases in the future where we also want a second macro that
takes a printf-like string for a custom error message (e.g., to include
the actual value of something rather than just the expression that
failed).  But we can cross that bridge if/when we get to it.


Matt

> 
> 
> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
> index 1a9082db8f1b..7070912edd96 100644
> --- a/drivers/gpu/drm/xe/xe_engine.c
> +++ b/drivers/gpu/drm/xe/xe_engine.c
> @@ -249,11 +249,8 @@ static int engine_set_compute_mode(struct xe_device *xe, struct xe_engine *e,
>  static int engine_set_persistence(struct xe_device *xe, struct xe_engine *e,
>  				  u64 value, bool create)
>  {
> -	if (XE_IOCTL_ERR(xe, !create))
> -		return -EINVAL;
> -
> -	if (XE_IOCTL_ERR(xe, e->flags & ENGINE_FLAG_COMPUTE_MODE))
> -		return -EINVAL;
> +	XE_IOCTL_ASSERT(xe, create, -EINVAL);
> +	XE_IOCTL_ASSERT(xe, !(e->flags & ENGINE_FLAG_COMPUTE_MODE), -EINVAL);
>  	if (value)
>  		e->flags |= ENGINE_FLAG_PERSISTENT;
> @@ -266,11 +263,8 @@ static int engine_set_persistence(struct xe_device *xe, struct xe_engine *e,
>  static int engine_set_job_timeout(struct xe_device *xe, struct xe_engine *e,
>  				  u64 value, bool create)
>  {
> -	if (XE_IOCTL_ERR(xe, !create))
> -		return -EINVAL;
> -
> -	if (!capable(CAP_SYS_NICE))
> -		return -EPERM;
> +	XE_IOCTL_ASSERT(xe, create, -EINVAL);
> +	XE_IOCTL_ASSERT(xe, capable(CAP_SYS_NICE), -EPERM);
>  	return e->ops->set_job_timeout(e, value);
>  }
> @@ -278,11 +272,8 @@ static int engine_set_job_timeout(struct xe_device *xe, struct xe_engine *e,
>  static int engine_set_acc_trigger(struct xe_device *xe, struct xe_engine *e,
>  				  u64 value, bool create)
>  {
> -	if (XE_IOCTL_ERR(xe, !create))
> -		return -EINVAL;
> -
> -	if (XE_IOCTL_ERR(xe, !xe->info.supports_usm))
> -		return -EINVAL;
> +	XE_IOCTL_ASSERT(xe, create, -EINVAL);
> +	XE_IOCTL_ASSERT(xe, xe->info.supports_usm, -EINVAL);
>  	e->usm.acc_trigger = value;
> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
> index 0d24c124d202..6660ab49c8a8 100644
> --- a/drivers/gpu/drm/xe/xe_macros.h
> +++ b/drivers/gpu/drm/xe/xe_macros.h
> @@ -17,4 +17,11 @@
>  			    "Ioctl argument check failed at %s:%d: %s", \
>  			    __FILE__, __LINE__, #cond), 1))
> +#define XE_IOCTL_ASSERT(xe__, cond__, retcode__)				\
> +	if (unlikely(!(cond__))) {						\
> +		drm_dbg(&(xe__)->drm, "Ioctl argument check failed: %s",	\
> +			#cond__);						\
> +		return retcode__;						\
> +	}
> +
>  #endif
> 
> > 
> > 
> > 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

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