2023-12-26 21:24:07

by Tanzir Hasan

[permalink] [raw]
Subject: [PATCH] xprtrdma: removed unnecessary headers from verbs.c

asm-generic/barrier.h and asm/bitops.h are already brought into the
header and the file can still be built with their removal.

Suggested-by: Al Viro <[email protected]>
Signed-off-by: Tanzir Hasan <[email protected]>
---
net/sunrpc/xprtrdma/verbs.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
index 28c0771c4e8c..5436560dda85 100644
--- a/net/sunrpc/xprtrdma/verbs.c
+++ b/net/sunrpc/xprtrdma/verbs.c
@@ -55,9 +55,6 @@
#include <linux/sunrpc/svc_rdma.h>
#include <linux/log2.h>

-#include <asm-generic/barrier.h>
-#include <asm/bitops.h>
-
#include <rdma/ib_cm.h>

#include "xprt_rdma.h"

---
base-commit: fbafc3e621c3f4ded43720fdb1d6ce1728ec664e
change-id: 20231226-verbs-30800631d3f1

Best regards,
--
Tanzir Hasan <[email protected]>



2023-12-26 23:21:08

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH] xprtrdma: removed unnecessary headers from verbs.c

Hi,

On 12/26/23 13:23, Tanzir Hasan wrote:
> asm-generic/barrier.h and asm/bitops.h are already brought into the
> header and the file can still be built with their removal.

Brought into which header?

Does this conflict with Rule #1 in Documentation/process/submit-checklist.rst ?

> Suggested-by: Al Viro <[email protected]>
> Signed-off-by: Tanzir Hasan <[email protected]>
> ---
> net/sunrpc/xprtrdma/verbs.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
> index 28c0771c4e8c..5436560dda85 100644
> --- a/net/sunrpc/xprtrdma/verbs.c
> +++ b/net/sunrpc/xprtrdma/verbs.c
> @@ -55,9 +55,6 @@
> #include <linux/sunrpc/svc_rdma.h>
> #include <linux/log2.h>
>
> -#include <asm-generic/barrier.h>
> -#include <asm/bitops.h>
> -
> #include <rdma/ib_cm.h>
>
> #include "xprt_rdma.h"
>
> ---
> base-commit: fbafc3e621c3f4ded43720fdb1d6ce1728ec664e
> change-id: 20231226-verbs-30800631d3f1
>
> Best regards,

--
#Randy

2023-12-26 23:35:28

by Tanzir Hasan

[permalink] [raw]
Subject: Re: [PATCH] xprtrdma: removed unnecessary headers from verbs.c

On Tue, Dec 26, 2023 at 3:20 PM Randy Dunlap <[email protected]> wrote:
>
> Hi,
>
> On 12/26/23 13:23, Tanzir Hasan wrote:
> > asm-generic/barrier.h and asm/bitops.h are already brought into the
> > header and the file can still be built with their removal.
>
> Brought into which header?
Hi Randy,

Sorry for the poor explanation. I see that I left out the specific header.
The inclusion of linux/sunrpc/svc_rdma.h brings in linux/sunrpc/rpc_rdma.h
This brings in linux/bitops.h which is preferred over asm/bitops.h

> Does this conflict with Rule #1 in Documentation/process/submit-checklist.rst ?

Yes, this conflicts with Rule #1. A better version of this patch would be to add
linux/bitops.h to this file directly. The main reason this patch
exists is to clear
out the asm-generic file since those are not preferred. I can do this by either
including just linux/bitops.h or including both linux/bitops.h and
asm/barrier.h.
Would the second approach conform better with Rule #1?

Thanks,
Tanzir

2023-12-26 23:54:13

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH] xprtrdma: removed unnecessary headers from verbs.c

Hi Tanzir,

On 12/26/23 15:35, Tanzir Hasan wrote:
> On Tue, Dec 26, 2023 at 3:20 PM Randy Dunlap <[email protected]> wrote:
>>
>> Hi,
>>
>> On 12/26/23 13:23, Tanzir Hasan wrote:
>>> asm-generic/barrier.h and asm/bitops.h are already brought into the
>>> header and the file can still be built with their removal.
>>
>> Brought into which header?
> Hi Randy,
>
> Sorry for the poor explanation. I see that I left out the specific header.
> The inclusion of linux/sunrpc/svc_rdma.h brings in linux/sunrpc/rpc_rdma.h
> This brings in linux/bitops.h which is preferred over asm/bitops.h
>
>> Does this conflict with Rule #1 in Documentation/process/submit-checklist.rst ?
>
> Yes, this conflicts with Rule #1. A better version of this patch would be to add
> linux/bitops.h to this file directly. The main reason this patch
> exists is to clear
> out the asm-generic file since those are not preferred. I can do this by either
> including just linux/bitops.h or including both linux/bitops.h and
> asm/barrier.h.
> Would the second approach conform better with Rule #1?

Yes, it would IMO.

Where can I find your current working list of what/how to #include?

Thanks.

--
#Randy

2023-12-27 00:05:04

by Tanzir Hasan

[permalink] [raw]
Subject: Re: [PATCH] xprtrdma: removed unnecessary headers from verbs.c

Hi Randy,

> Where can I find your current working list of what/how to #include?
Here is my current working list of what to #include.

#include <linux/bitops.h>
#include <linux/interrupt.h>
#include <linux/slab.h>
#include <linux/sunrpc/addr.h>
#include <linux/sunrpc/svc_rdma.h>
#include <linux/log2.h>

#include <asm/barrier.h>

#include <rdma/ib_cm.h>

#include "xprt_rdma.h"
#include <trace/events/rpcrdma.h>

There was a discussion here about when to include asm/asm-generics:
https://lore.kernel.org/llvm/20231215210344.GA3096493@ZenIV/

If I misunderstood your question please let me know.

Best,
Tanzir

2023-12-27 00:11:07

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH] xprtrdma: removed unnecessary headers from verbs.c



On 12/26/23 16:04, Tanzir Hasan wrote:
> Hi Randy,
>
>> Where can I find your current working list of what/how to #include?
> Here is my current working list of what to #include.
>
> #include <linux/bitops.h>
> #include <linux/interrupt.h>
> #include <linux/slab.h>
> #include <linux/sunrpc/addr.h>
> #include <linux/sunrpc/svc_rdma.h>
> #include <linux/log2.h>
>
> #include <asm/barrier.h>
>
> #include <rdma/ib_cm.h>
>
> #include "xprt_rdma.h"
> #include <trace/events/rpcrdma.h>
>
> There was a discussion here about when to include asm/asm-generics:
> https://lore.kernel.org/llvm/20231215210344.GA3096493@ZenIV/
>
> If I misunderstood your question please let me know.

Yes, more the latter link for general info rather than the specific
info for this one source file.

Thanks.

--
#Randy