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

[Lucas De Marchi]

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 at kernel.org>
>> > Link: https://lists.freedesktop.org/archives/intel-xe/2023-May/004704.html
>> > Signed-off-by: Francois Dugast <francois.dugast at 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?


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