[-next,v3,1/2] sunrpc: Wrap read accesses to rpc_task.tk_pid

Commit Message

Geert Uytterhoeven Oct. 16, 2023, 1:09 p.m. UTC

The tk_pid member in the rpc_task structure exists conditionally on
debug or tracing being enabled.

Introduce and use a wapper to read the value of this member, so users
outside tracing no longer have to care about these conditions.

Reported-by: kernel test robot <[e-mail]>
Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
Signed-off-by: Geert Uytterhoeven <[e-mail]>
---
v3:
  - New.
---
 fs/nfs/filelayout/filelayout.c         | 12 ++++++------
 fs/nfs/flexfilelayout/flexfilelayout.c |  9 +++------
 include/linux/sunrpc/sched.h           | 10 ++++++++++
 net/sunrpc/clnt.c                      |  2 +-
 net/sunrpc/debugfs.c                   |  2 +-
 5 files changed, 21 insertions(+), 14 deletions(-)

Comments

Benjamin Coddington Oct. 16, 2023, 9:15 p.m. UTC | #1

On 16 Oct 2023, at 9:09, Geert Uytterhoeven wrote:

> The tk_pid member in the rpc_task structure exists conditionally on
> debug or tracing being enabled.
>
> Introduce and use a wapper to read the value of this member, so users
> outside tracing no longer have to care about these conditions.
>
> Reported-by: kernel test robot <[e-mail]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
> Signed-off-by: Geert Uytterhoeven <[e-mail]>

I never work on kernels that don't have tk_pid, but I can say its so useful
that for 2 out of the 224 bytes that rpc_task uses (on aarch64), I'd be
inclined to just include it all the time.  That way its around for folks to
reference with realtime tools (like bpftrace, stap).

Does anyone know if there is a good reason not to include it for all
configurations?

Ben

..also:
Reviewed-by: Benjamin Coddington <[e-mail]>

Geert Uytterhoeven Oct. 17, 2023, 6:35 a.m. UTC | #2

Hi Ben.

On Mon, Oct 16, 2023 at 11:15 PM Benjamin Coddington
<[e-mail]> wrote:
> On 16 Oct 2023, at 9:09, Geert Uytterhoeven wrote:
> > The tk_pid member in the rpc_task structure exists conditionally on
> > debug or tracing being enabled.
> >
> > Introduce and use a wapper to read the value of this member, so users
> > outside tracing no longer have to care about these conditions.
> >
> > Reported-by: kernel test robot <[e-mail]>
> > Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
> > Signed-off-by: Geert Uytterhoeven <[e-mail]>
>
> I never work on kernels that don't have tk_pid, but I can say its so useful
> that for 2 out of the 224 bytes that rpc_task uses (on aarch64), I'd be
> inclined to just include it all the time.  That way its around for folks to
> reference with realtime tools (like bpftrace, stap).

That would work, too.
In fact always including it should not increase the size of struct rpc_task,
as there's still unused spaced in the gap at the end.

> Does anyone know if there is a good reason not to include it for all
> configurations?
>
> Ben
>
> ..also:
> Reviewed-by: Benjamin Coddington <[e-mail]>

Thanks!

Gr{oetje,eeting}s,

                        Geert

Patch

diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
index ce8f8934bca517c0..5af545f49c54db4f 100644
--- a/fs/nfs/filelayout/filelayout.c
+++ b/fs/nfs/filelayout/filelayout.c
@@ -93,8 +93,7 @@  static void filelayout_reset_write(struct nfs_pgio_header *hdr)
 	if (!test_and_set_bit(NFS_IOHDR_REDO, &hdr->flags)) {
 		dprintk("%s Reset task %5u for i/o through MDS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
@@ -110,8 +109,7 @@  static void filelayout_reset_read(struct nfs_pgio_header *hdr)
 	if (!test_and_set_bit(NFS_IOHDR_REDO, &hdr->flags)) {
 		dprintk("%s Reset task %5u for i/o through MDS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
@@ -274,7 +272,8 @@  static void filelayout_read_prepare(struct rpc_task *task, void *data)
 		return;
 	}
 	if (filelayout_reset_to_mds(hdr->lseg)) {
-		dprintk("%s task %u reset io to MDS\n", __func__, task->tk_pid);
+		dprintk("%s task %u reset io to MDS\n", __func__,
+			rpc_tk_pid(task));
 		filelayout_reset_read(hdr);
 		rpc_exit(task, 0);
 		return;
@@ -372,7 +371,8 @@  static void filelayout_write_prepare(struct rpc_task *task, void *data)
 		return;
 	}
 	if (filelayout_reset_to_mds(hdr->lseg)) {
-		dprintk("%s task %u reset io to MDS\n", __func__, task->tk_pid);
+		dprintk("%s task %u reset io to MDS\n", __func__,
+			rpc_tk_pid(task));
 		filelayout_reset_write(hdr);
 		rpc_exit(task, 0);
 		return;
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index a1dc338649062de3..3dd17f675d433f4d 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -1017,8 +1017,7 @@  static void ff_layout_reset_write(struct nfs_pgio_header *hdr, bool retry_pnfs)
 	if (retry_pnfs) {
 		dprintk("%s Reset task %5u for i/o through pNFS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
@@ -1030,8 +1029,7 @@  static void ff_layout_reset_write(struct nfs_pgio_header *hdr, bool retry_pnfs)
 	if (!test_and_set_bit(NFS_IOHDR_REDO, &hdr->flags)) {
 		dprintk("%s Reset task %5u for i/o through MDS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
@@ -1066,8 +1064,7 @@  static void ff_layout_reset_read(struct nfs_pgio_header *hdr)
 	if (!test_and_set_bit(NFS_IOHDR_REDO, &hdr->flags)) {
 		dprintk("%s Reset task %5u for i/o through MDS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 8ada7dc802d30507..0f39e60d28ed0132 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -270,6 +270,11 @@  void		rpc_prepare_task(struct rpc_task *task);
 gfp_t		rpc_task_gfp_mask(void);
 
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG) || IS_ENABLED(CONFIG_TRACEPOINTS)
+static inline unsigned short rpc_tk_pid(const struct rpc_task *task)
+{
+	return task->tk_pid;
+}
+
 static inline const char * rpc_qname(const struct rpc_wait_queue *q)
 {
 	return ((q && q->name) ? q->name : "unknown");
@@ -281,6 +286,11 @@  static inline void rpc_assign_waitqueue_name(struct rpc_wait_queue *q,
 	q->name = name;
 }
 #else
+static inline unsigned short rpc_tk_pid(const struct rpc_task *task)
+{
+	return 0;
+}
+
 static inline void rpc_assign_waitqueue_name(struct rpc_wait_queue *q,
 		const char *name)
 {
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 9c210273d06b7f51..2f37e4143789b0cc 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -3320,7 +3320,7 @@  static void rpc_show_task(const struct rpc_clnt *clnt,
 		rpc_waitq = rpc_qname(task->tk_waitqueue);
 
 	printk(KERN_INFO "%5u %04x %6d %8p %8p %8ld %8p %sv%u %s a:%ps q:%s\n",
-		task->tk_pid, task->tk_flags, task->tk_status,
+		rpc_tk_pid(task), task->tk_flags, task->tk_status,
 		clnt, task->tk_rqstp, rpc_task_timeout(task), task->tk_ops,
 		clnt->cl_program->name, clnt->cl_vers, rpc_proc_name(task),
 		task->tk_action, rpc_waitq);
diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index a176d5a0b0ee9a2c..8896518dd6f3ce0e 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -31,7 +31,7 @@  tasks_show(struct seq_file *f, void *v)
 		xid = be32_to_cpu(task->tk_rqstp->rq_xid);
 
 	seq_printf(f, "%5u %04x %6d 0x%x 0x%x %8ld %ps %sv%u %s a:%ps q:%s\n",
-		task->tk_pid, task->tk_flags, task->tk_status,
+		rpc_tk_pid(task), task->tk_flags, task->tk_status,
 		clnt->cl_clid, xid, rpc_task_timeout(task), task->tk_ops,
 		clnt->cl_program->name, clnt->cl_vers, rpc_proc_name(task),
 		task->tk_action, rpc_waitq);