[ndctl PATCH] test/common: document magic number CXL_TEST_QOS_CLASS=42

[ndctl PATCH] test/common: document magic number CXL_TEST_QOS_CLASS=42

[marc.herbert]

From: Marc Herbert <marc.herbert@linux.intel.com>

This magic number must match kernel code. Make that corresponding kernel
code much less time-consuming to find.

Additionally, that same one-line reference indirectly documents the
minimum kernel version required by the test(s) using this value (only
cxl-qos-class.sh at this time)

Signed-off-by: Marc Herbert <marc.herbert@linux.intel.com>
---
 test/common | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/common b/test/common
index 2d076402ef7c..1ab62be6994f 100644
--- a/test/common
+++ b/test/common
@@ -155,4 +155,5 @@ check_dmesg()
 }
 
 # CXL COMMON
+# Test constant defined in kernel commit v6.8-rc2-9-g117132edc690
 CXL_TEST_QOS_CLASS=42
-- 
2.50.1

Re: [ndctl PATCH] test/common: document magic number CXL_TEST_QOS_CLASS=42

[Dave Jiang]

On 8/21/25 7:55 PM, marc.herbert@linux.intel.com wrote:
> From: Marc Herbert <marc.herbert@linux.intel.com>
> 
> This magic number must match kernel code. Make that corresponding kernel
> code much less time-consuming to find.
> 
> Additionally, that same one-line reference indirectly documents the
> minimum kernel version required by the test(s) using this value (only
> cxl-qos-class.sh at this time)
> 
> Signed-off-by: Marc Herbert <marc.herbert@linux.intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  test/common | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/test/common b/test/common
> index 2d076402ef7c..1ab62be6994f 100644
> --- a/test/common
> +++ b/test/common
> @@ -155,4 +155,5 @@ check_dmesg()
>  }
>  
>  # CXL COMMON
> +# Test constant defined in kernel commit v6.8-rc2-9-g117132edc690
>  CXL_TEST_QOS_CLASS=42

Re: [ndctl PATCH] test/common: document magic number CXL_TEST_QOS_CLASS=42

[Alison Schofield]

On Fri, Aug 22, 2025 at 02:55:34AM +0000, marc.herbert@linux.intel.com wrote:
> From: Marc Herbert <marc.herbert@linux.intel.com>
> 
> This magic number must match kernel code. Make that corresponding kernel
> code much less time-consuming to find.

The 'must match' is the important part. Include that in the comment.
Why expect the user to parse a git describe string and go fishing.
Just tell them it is defined in the cxl-test module.

See below...

> 
> Additionally, that same one-line reference indirectly documents the
> minimum kernel version required by the test(s) using this value (only
> cxl-qos-class.sh at this time)
> 
> Signed-off-by: Marc Herbert <marc.herbert@linux.intel.com>
> ---
>  test/common | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/test/common b/test/common
> index 2d076402ef7c..1ab62be6994f 100644
> --- a/test/common
> +++ b/test/common
> @@ -155,4 +155,5 @@ check_dmesg()
>  }
>  
>  # CXL COMMON
> +# Test constant defined in kernel commit v6.8-rc2-9-g117132edc690

# Must match the FAKE_QTG_ID defined in the cxl-test module

>  CXL_TEST_QOS_CLASS=42
> -- 
> 2.50.1
> 

Re: [ndctl PATCH] test/common: document magic number CXL_TEST_QOS_CLASS=42

[Marc Herbert]

On 2025-08-22 11:13, Alison Schofield wrote:
> On Fri, Aug 22, 2025 at 02:55:34AM +0000, marc.herbert@linux.intel.com wrote:
>> From: Marc Herbert <marc.herbert@linux.intel.com>
>>
>> This magic number must match kernel code. Make that corresponding kernel
>> code much less time-consuming to find.
> 
> The 'must match' is the important part. Include that in the comment.

Good point, will do!

> Why expect the user to parse a git describe string and go fishing.
> Just tell them it is defined in the cxl-test module.
> 

git knows how to parse back the git describe string; readers don't need
to parse anything. As explained in the commit message, it's convenient
because it holds in a single string both the immutable commit ID _and_
an indication of the minimum kernel version required.

Why tell users to "go fishing" for the cxl-test module when they can
find the location directly with a single git command.  This is real: I
actually wasted a fair amount of time searching for that constant in
drivers/cxl/ because I assumed the cxl-test driver was there. This
comment is not meant for experts; if they needed it then it would not
have been missing for so long.

Last but not least, code and files move around and get renamed. This
commit will never change, so it provides an immutable starting point in
case things change.

I'll add both, it should still fit on one line.

Re: [ndctl PATCH] test/common: document magic number CXL_TEST_QOS_CLASS=42

[Alison Schofield]

On Fri, Aug 22, 2025 at 08:07:12PM -0700, Marc Herbert wrote:
> 
> 
> On 2025-08-22 11:13, Alison Schofield wrote:
> > On Fri, Aug 22, 2025 at 02:55:34AM +0000, marc.herbert@linux.intel.com wrote:
> >> From: Marc Herbert <marc.herbert@linux.intel.com>
> >>
> >> This magic number must match kernel code. Make that corresponding kernel
> >> code much less time-consuming to find.
> > 
> > The 'must match' is the important part. Include that in the comment.
> 
> Good point, will do!
> 
> > Why expect the user to parse a git describe string and go fishing.
> > Just tell them it is defined in the cxl-test module.
> > 
> 
> git knows how to parse back the git describe string; readers don't need
> to parse anything. As explained in the commit message, it's convenient
> because it holds in a single string both the immutable commit ID _and_
> an indication of the minimum kernel version required.
> 
> Why tell users to "go fishing" for the cxl-test module when they can
> find the location directly with a single git command.  This is real: I
> actually wasted a fair amount of time searching for that constant in
> drivers/cxl/ because I assumed the cxl-test driver was there. This
> comment is not meant for experts; if they needed it then it would not
> have been missing for so long.
> 
> Last but not least, code and files move around and get renamed. This
> commit will never change, so it provides an immutable starting point in
> case things change.
> 
> I'll add both, it should still fit on one line.

Marc,

I didn't infer the pain you go on to describe here in the original
commit message. I read this as a trivial patch, and even questioned
(but didn't comment) usage of 'magic number' language on value that
had a define.

ICYMI 
"Describe your problem. Whether your patch is a one-line bug fix or
5000 lines of a new feature, there must be an underlying problem that
motivated you to do this work. Convince the reviewer that there is a
problem worth fixing and that it makes sense for them to read past the
first paragraph."

That's from
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
and ndctl/Contributing.md notes that ndctl follows those guidelines.

There's a lot more guidance at that link, but following the basic 1-2-3,
emphasis on #2 for this patch, will help me review without missing the
point:

1- State current situation
2- State why #1 is lacking, painful, broken, needs enhancing, etc.
3- State how this patch addresses #2.

Perhaps:
[ndctl PATCH] test/common: document the CXL_TEST_QOS_CLASS kernel dependency

1-
The CXL_TEST_QOS_CLASS define lacks documentation about its
kernel counterpart, making it difficult to locate the corresponding
kernel code when debugging or verifying compatibility.

2-
When tests depend on kernel-specific constants, like this one does,
the lack of clear documentation leads to tedious manual searching.

3-
Add a comment that identifies...

May also be worth mentioning that this issue is arising during the
testing of kernels with backported features where test failures
may be due to missing kernel support rather than actual bugs.
If that is indeed what is happening and is part of the pain.

I'll watch for v2.

--Alison