[PATCH 0/1] SUNRPC: convert RPC_TASK_* constants to enum

[PATCH 0/1] SUNRPC: convert RPC_TASK_* constants to enum

[Stephen Brennan]

Hi folks,

For context on this change, I work on the drgn debugger [1] and also
maintain a growing set of helpers [2] that can examine live systems & core
dumps and give detailed information about the state of different
subsystems. One such helper (not written by me) shows all RPC tasks and
their states. For example:

=============================== RPC_TASK =================================
---- <rpc_task: 0xffff8dc383548a00>  tk_op: nfs4_delegreturn_ops  tk_action: call_status
     tk_client: 0xffff8dc385c0fe00  tk_client.cl_xprt: 0xffff8dc4f79f9000
     tk_xprt: 0xffff8dc4f79f9000
     tk_status: 0 (OK) tk_rpc_status[0] (OK)
     tk_runstate: 0x16  (RPC_TASK_QUEUED|RPC_TASK_ACTIVE|RPC_TASK_MSG_RECV_WAIT)
     tk_priority: 1  tk_timeout(ticks): 6095245983  tk_timeouts(major): 0
     <tk_rqstp: 0xffff8dc47025d000>  rq_xid: 0x96143ed3  rq_retries: 0

---- <rpc_task: 0xffff8dc383548400>  tk_op: nfs4_close_ops  tk_action: call_status
     tk_client: 0xffff8dc385c0fe00  tk_client.cl_xprt: 0xffff8dc4f79f9000
     tk_xprt: 0xffff8dc4f79f9000
     tk_status: 0 (OK) tk_rpc_status[0] (OK)
     tk_runstate: 0x16  (RPC_TASK_QUEUED|RPC_TASK_ACTIVE|RPC_TASK_MSG_RECV_WAIT)
     tk_priority: 1  tk_timeout(ticks): 6095245983  tk_timeouts(major): 0
     <tk_rqstp: 0xffff8dc47025ca00>  rq_xid: 0x96143ed4  rq_retries: 0

To do that it needs to be able to interpret the RPC_TASK_* constants back
to their human readable names. Of course these can be hard-coded and this
has been done, but in this particular case, the RPC state codes have been
updated and changed several times:

729749bb8da1 ("SUNRPC: Don't hold the transport lock across socket copy operations")
7ebbbc6e7bd0 ("SUNRPC: Simplify identification of when the message send/receive is complete")
cf9946cd6144 ("SUNRPC: Refactor the transport request pinning")
ae67bd3821bb ("SUNRPC: Fix up task signalling")

Most of these simply add to the end of the list, but at least one (7ebbbc6e7bd0)
shuffles around existing codes. Creating maintainable debugging scripts that can
be used over a range of kernel versions is difficult when you need to detect
these sorts of shuffles. We *can* do this by detecting the presence or existence
of other code changes that occurred in the same commit, but this tends to get
tedious, and it's not very reliable. It certainly won't help in case a similar
change happens in the future.

Converting constants from macros to enums is a great way to avoid this for the
future. While macros aren't typically encoded in debuginfo, enum definitions
are. Of course macros aren't always suitable for 64-bit constants. But in this
case, the RPC_TASK_* constants are bit numbers, so they aren't impacted by this
limitation. So this change shouldn't have any impact except making the code
easier to debug.

Thanks,
Stephen

[1]: https://drgn.readthedocs.io/en/latest/
[2]: https://github.com/oracle-samples/drgn-tools

Stephen Brennan (1):
  SUNRPC: convert RPC_TASK_* constants to enum

 include/linux/sunrpc/sched.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

-- 
2.43.5

[PATCH 1/1] SUNRPC: convert RPC_TASK_* constants to enum

[Stephen Brennan]

The RPC_TASK_* constants are defined as macros, which means that most
kernel builds will not contain their definitions in the debuginfo.
However, it's quite useful for debuggers to be able to view the task
state constant and interpret it correctly. Conversion to an enum will
ensure the constants are present in debuginfo and can be interpreted by
debuggers without needing to hard-code them and track their changes.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 include/linux/sunrpc/sched.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 0c77ba488bbae..177220524eb5d 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -151,13 +151,15 @@ struct rpc_task_setup {
 #define RPC_WAS_SENT(t)		((t)->tk_flags & RPC_TASK_SENT)
 #define RPC_IS_MOVEABLE(t)	((t)->tk_flags & RPC_TASK_MOVEABLE)
 
-#define RPC_TASK_RUNNING	0
-#define RPC_TASK_QUEUED		1
-#define RPC_TASK_ACTIVE		2
-#define RPC_TASK_NEED_XMIT	3
-#define RPC_TASK_NEED_RECV	4
-#define RPC_TASK_MSG_PIN_WAIT	5
-#define RPC_TASK_SIGNALLED	6
+enum {
+	RPC_TASK_RUNNING	= 0,
+	RPC_TASK_QUEUED		= 1,
+	RPC_TASK_ACTIVE		= 2,
+	RPC_TASK_NEED_XMIT	= 3,
+	RPC_TASK_NEED_RECV	= 4,
+	RPC_TASK_MSG_PIN_WAIT	= 5,
+	RPC_TASK_SIGNALLED	= 6,
+};
 
 #define rpc_test_and_set_running(t) \
 				test_and_set_bit(RPC_TASK_RUNNING, &(t)->tk_runstate)
-- 
2.43.5

Re: [PATCH 1/1] SUNRPC: convert RPC_TASK_* constants to enum

[NeilBrown]

On Sat, 17 Aug 2024, Stephen Brennan wrote:
> The RPC_TASK_* constants are defined as macros, which means that most
> kernel builds will not contain their definitions in the debuginfo.
> However, it's quite useful for debuggers to be able to view the task
> state constant and interpret it correctly. Conversion to an enum will
> ensure the constants are present in debuginfo and can be interpreted by
> debuggers without needing to hard-code them and track their changes.
> 
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---
>  include/linux/sunrpc/sched.h | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
> index 0c77ba488bbae..177220524eb5d 100644
> --- a/include/linux/sunrpc/sched.h
> +++ b/include/linux/sunrpc/sched.h
> @@ -151,13 +151,15 @@ struct rpc_task_setup {
>  #define RPC_WAS_SENT(t)		((t)->tk_flags & RPC_TASK_SENT)
>  #define RPC_IS_MOVEABLE(t)	((t)->tk_flags & RPC_TASK_MOVEABLE)
>  
> -#define RPC_TASK_RUNNING	0
> -#define RPC_TASK_QUEUED		1
> -#define RPC_TASK_ACTIVE		2
> -#define RPC_TASK_NEED_XMIT	3
> -#define RPC_TASK_NEED_RECV	4
> -#define RPC_TASK_MSG_PIN_WAIT	5
> -#define RPC_TASK_SIGNALLED	6
> +enum {
> +	RPC_TASK_RUNNING	= 0,
> +	RPC_TASK_QUEUED		= 1,
> +	RPC_TASK_ACTIVE		= 2,
> +	RPC_TASK_NEED_XMIT	= 3,
> +	RPC_TASK_NEED_RECV	= 4,
> +	RPC_TASK_MSG_PIN_WAIT	= 5,
> +	RPC_TASK_SIGNALLED	= 6,
> +};

I am strongly in favour of converting these #defines to an enum, but
having the explicit assignments in the enum is pure noise adding no
value at all.
Would you consider resubmiting as a simple enum that uses the default
values?

Thanks,
NeilBrown


>  
>  #define rpc_test_and_set_running(t) \
>  				test_and_set_bit(RPC_TASK_RUNNING, &(t)->tk_runstate)
> -- 
> 2.43.5
> 
> 

Re: [PATCH 1/1] SUNRPC: convert RPC_TASK_* constants to enum

[Stephen Brennan]

"NeilBrown" <neilb@suse.de> writes:
> On Sat, 17 Aug 2024, Stephen Brennan wrote:
>> The RPC_TASK_* constants are defined as macros, which means that most
>> kernel builds will not contain their definitions in the debuginfo.
>> However, it's quite useful for debuggers to be able to view the task
>> state constant and interpret it correctly. Conversion to an enum will
>> ensure the constants are present in debuginfo and can be interpreted by
>> debuggers without needing to hard-code them and track their changes.
>> 
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> ---
>>  include/linux/sunrpc/sched.h | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>> 
>> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
>> index 0c77ba488bbae..177220524eb5d 100644
>> --- a/include/linux/sunrpc/sched.h
>> +++ b/include/linux/sunrpc/sched.h
>> @@ -151,13 +151,15 @@ struct rpc_task_setup {
>>  #define RPC_WAS_SENT(t)		((t)->tk_flags & RPC_TASK_SENT)
>>  #define RPC_IS_MOVEABLE(t)	((t)->tk_flags & RPC_TASK_MOVEABLE)
>>  
>> -#define RPC_TASK_RUNNING	0
>> -#define RPC_TASK_QUEUED		1
>> -#define RPC_TASK_ACTIVE		2
>> -#define RPC_TASK_NEED_XMIT	3
>> -#define RPC_TASK_NEED_RECV	4
>> -#define RPC_TASK_MSG_PIN_WAIT	5
>> -#define RPC_TASK_SIGNALLED	6
>> +enum {
>> +	RPC_TASK_RUNNING	= 0,
>> +	RPC_TASK_QUEUED		= 1,
>> +	RPC_TASK_ACTIVE		= 2,
>> +	RPC_TASK_NEED_XMIT	= 3,
>> +	RPC_TASK_NEED_RECV	= 4,
>> +	RPC_TASK_MSG_PIN_WAIT	= 5,
>> +	RPC_TASK_SIGNALLED	= 6,
>> +};
>
> I am strongly in favour of converting these #defines to an enum, but
> having the explicit assignments in the enum is pure noise adding no
> value at all.

I agree, I only included it in case reviewers would prefer to be able to
see the values, as they were with the #defines. But I think it's common
knowledge that enums start at 0 and increment by 1.

> Would you consider resubmiting as a simple enum that uses the default
> values?

Definitely!

Thanks,
Stephen