[PATCH 1/1] RDMA/rxe: Add function name to the logs

[PATCH 1/1] RDMA/rxe: Add function name to the logs

[Zhu Yanjun]

From: Zhu Yanjun <yanjun.zhu@linux.dev>

Add the function names to the pr_ logs. As such, if some bugs occur,
with function names, it is easy to locate the bugs.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
 drivers/infiniband/sw/rxe/rxe.h       |  2 +-
 drivers/infiniband/sw/rxe/rxe_queue.h | 12 ++++--------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
index 2415f3704f57..43742d2f32de 100644
--- a/drivers/infiniband/sw/rxe/rxe.h
+++ b/drivers/infiniband/sw/rxe/rxe.h
@@ -10,7 +10,7 @@
 #ifdef pr_fmt
 #undef pr_fmt
 #endif
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
 
 #include <linux/skbuff.h>
 
diff --git a/drivers/infiniband/sw/rxe/rxe_queue.h b/drivers/infiniband/sw/rxe/rxe_queue.h
index c711cb98b949..5d6e17b00e60 100644
--- a/drivers/infiniband/sw/rxe/rxe_queue.h
+++ b/drivers/infiniband/sw/rxe/rxe_queue.h
@@ -185,8 +185,7 @@ static inline void queue_advance_producer(struct rxe_queue *q,
 	case QUEUE_TYPE_FROM_CLIENT:
 		/* used by rxe, client owns the index */
 		if (WARN_ON(1))
-			pr_warn("%s: attempt to advance client index\n",
-				__func__);
+			pr_warn("attempt to advance client index\n");
 		break;
 	case QUEUE_TYPE_TO_CLIENT:
 		/* used by rxe which owns the index */
@@ -206,8 +205,7 @@ static inline void queue_advance_producer(struct rxe_queue *q,
 	case QUEUE_TYPE_TO_ULP:
 		/* used by ulp, rxe owns the index */
 		if (WARN_ON(1))
-			pr_warn("%s: attempt to advance driver index\n",
-				__func__);
+			pr_warn("attempt to advance driver index\n");
 		break;
 	}
 }
@@ -228,14 +226,12 @@ static inline void queue_advance_consumer(struct rxe_queue *q,
 	case QUEUE_TYPE_TO_CLIENT:
 		/* used by rxe, client owns the index */
 		if (WARN_ON(1))
-			pr_warn("%s: attempt to advance client index\n",
-				__func__);
+			pr_warn("attempt to advance client index\n");
 		break;
 	case QUEUE_TYPE_FROM_ULP:
 		/* used by ulp, rxe owns the index */
 		if (WARN_ON(1))
-			pr_warn("%s: attempt to advance driver index\n",
-				__func__);
+			pr_warn("attempt to advance driver index\n");
 		break;
 	case QUEUE_TYPE_TO_ULP:
 		/* used by ulp which owns the index */
-- 
2.27.0

Re: [PATCH 1/1] RDMA/rxe: Add function name to the logs

[Zhu Yanjun]

在 2023/4/10 18:21, Zhu Yanjun 写道:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Add the function names to the pr_ logs. As such, if some bugs occur,
> with function names, it is easy to locate the bugs.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Gently ping

Zhu Yanjun

> ---
>   drivers/infiniband/sw/rxe/rxe.h       |  2 +-
>   drivers/infiniband/sw/rxe/rxe_queue.h | 12 ++++--------
>   2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index 2415f3704f57..43742d2f32de 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -10,7 +10,7 @@
>   #ifdef pr_fmt
>   #undef pr_fmt
>   #endif
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
>   
>   #include <linux/skbuff.h>
>   
> diff --git a/drivers/infiniband/sw/rxe/rxe_queue.h b/drivers/infiniband/sw/rxe/rxe_queue.h
> index c711cb98b949..5d6e17b00e60 100644
> --- a/drivers/infiniband/sw/rxe/rxe_queue.h
> +++ b/drivers/infiniband/sw/rxe/rxe_queue.h
> @@ -185,8 +185,7 @@ static inline void queue_advance_producer(struct rxe_queue *q,
>   	case QUEUE_TYPE_FROM_CLIENT:
>   		/* used by rxe, client owns the index */
>   		if (WARN_ON(1))
> -			pr_warn("%s: attempt to advance client index\n",
> -				__func__);
> +			pr_warn("attempt to advance client index\n");
>   		break;
>   	case QUEUE_TYPE_TO_CLIENT:
>   		/* used by rxe which owns the index */
> @@ -206,8 +205,7 @@ static inline void queue_advance_producer(struct rxe_queue *q,
>   	case QUEUE_TYPE_TO_ULP:
>   		/* used by ulp, rxe owns the index */
>   		if (WARN_ON(1))
> -			pr_warn("%s: attempt to advance driver index\n",
> -				__func__);
> +			pr_warn("attempt to advance driver index\n");
>   		break;
>   	}
>   }
> @@ -228,14 +226,12 @@ static inline void queue_advance_consumer(struct rxe_queue *q,
>   	case QUEUE_TYPE_TO_CLIENT:
>   		/* used by rxe, client owns the index */
>   		if (WARN_ON(1))
> -			pr_warn("%s: attempt to advance client index\n",
> -				__func__);
> +			pr_warn("attempt to advance client index\n");
>   		break;
>   	case QUEUE_TYPE_FROM_ULP:
>   		/* used by ulp, rxe owns the index */
>   		if (WARN_ON(1))
> -			pr_warn("%s: attempt to advance driver index\n",
> -				__func__);
> +			pr_warn("attempt to advance driver index\n");
>   		break;
>   	case QUEUE_TYPE_TO_ULP:
>   		/* used by ulp which owns the index */

Re: [PATCH 1/1] RDMA/rxe: Add function name to the logs

[Leon Romanovsky]

On Mon, Apr 10, 2023 at 06:21:05PM +0800, Zhu Yanjun wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Add the function names to the pr_ logs. As such, if some bugs occur,
> with function names, it is easy to locate the bugs.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/sw/rxe/rxe.h       |  2 +-
>  drivers/infiniband/sw/rxe/rxe_queue.h | 12 ++++--------
>  2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index 2415f3704f57..43742d2f32de 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -10,7 +10,7 @@
>  #ifdef pr_fmt
>  #undef pr_fmt
>  #endif
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
>  
>  #include <linux/skbuff.h>
>  
> diff --git a/drivers/infiniband/sw/rxe/rxe_queue.h b/drivers/infiniband/sw/rxe/rxe_queue.h
> index c711cb98b949..5d6e17b00e60 100644
> --- a/drivers/infiniband/sw/rxe/rxe_queue.h
> +++ b/drivers/infiniband/sw/rxe/rxe_queue.h
> @@ -185,8 +185,7 @@ static inline void queue_advance_producer(struct rxe_queue *q,
>  	case QUEUE_TYPE_FROM_CLIENT:
>  		/* used by rxe, client owns the index */
>  		if (WARN_ON(1))
> -			pr_warn("%s: attempt to advance client index\n",
> -				__func__);
> +			pr_warn("attempt to advance client index\n");

Delete all if (WARN_ON(1)) pr_warn(...) in favour of plain WARN_ON().
It will give you all information which you need.

Thanks

Re: [PATCH 1/1] RDMA/rxe: Add function name to the logs

[Zhu Yanjun]

在 2023/4/18 16:08, Leon Romanovsky 写道:
> On Mon, Apr 10, 2023 at 06:21:05PM +0800, Zhu Yanjun wrote:
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> Add the function names to the pr_ logs. As such, if some bugs occur,
>> with function names, it is easy to locate the bugs.
>>
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>>   drivers/infiniband/sw/rxe/rxe.h       |  2 +-
>>   drivers/infiniband/sw/rxe/rxe_queue.h | 12 ++++--------
>>   2 files changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
>> index 2415f3704f57..43742d2f32de 100644
>> --- a/drivers/infiniband/sw/rxe/rxe.h
>> +++ b/drivers/infiniband/sw/rxe/rxe.h
>> @@ -10,7 +10,7 @@
>>   #ifdef pr_fmt
>>   #undef pr_fmt
>>   #endif
>> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
>>   
>>   #include <linux/skbuff.h>
>>   
>> diff --git a/drivers/infiniband/sw/rxe/rxe_queue.h b/drivers/infiniband/sw/rxe/rxe_queue.h
>> index c711cb98b949..5d6e17b00e60 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_queue.h
>> +++ b/drivers/infiniband/sw/rxe/rxe_queue.h
>> @@ -185,8 +185,7 @@ static inline void queue_advance_producer(struct rxe_queue *q,
>>   	case QUEUE_TYPE_FROM_CLIENT:
>>   		/* used by rxe, client owns the index */
>>   		if (WARN_ON(1))
>> -			pr_warn("%s: attempt to advance client index\n",
>> -				__func__);
>> +			pr_warn("attempt to advance client index\n");
> Delete all if (WARN_ON(1)) pr_warn(...) in favour of plain WARN_ON().
> It will give you all information which you need.

Got it. Thanks a lot. I will fix it ASAP.

Zhu Yanjun

>
> Thanks

[PATCH 1/1] RDMA/rxe: Add function name to the logs

[Zhu Yanjun]

From: Zhu Yanjun <yanjun.zhu@linux.dev>

Add the function names to the pr_ logs. As such, if some bugs occur,
with function names, it is easy to locate the bugs.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
 drivers/infiniband/sw/rxe/rxe.h       |  2 +-
 drivers/infiniband/sw/rxe/rxe_queue.h | 16 ++++------------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
index d33dd6cf83d3..f897d0ac1216 100644
--- a/drivers/infiniband/sw/rxe/rxe.h
+++ b/drivers/infiniband/sw/rxe/rxe.h
@@ -10,7 +10,7 @@
 #ifdef pr_fmt
 #undef pr_fmt
 #endif
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
 
 #include <linux/skbuff.h>
 
diff --git a/drivers/infiniband/sw/rxe/rxe_queue.h b/drivers/infiniband/sw/rxe/rxe_queue.h
index c711cb98b949..11bba08ca5f9 100644
--- a/drivers/infiniband/sw/rxe/rxe_queue.h
+++ b/drivers/infiniband/sw/rxe/rxe_queue.h
@@ -184,9 +184,7 @@ static inline void queue_advance_producer(struct rxe_queue *q,
 	switch (type) {
 	case QUEUE_TYPE_FROM_CLIENT:
 		/* used by rxe, client owns the index */
-		if (WARN_ON(1))
-			pr_warn("%s: attempt to advance client index\n",
-				__func__);
+		WARN_ON(1);
 		break;
 	case QUEUE_TYPE_TO_CLIENT:
 		/* used by rxe which owns the index */
@@ -205,9 +203,7 @@ static inline void queue_advance_producer(struct rxe_queue *q,
 		break;
 	case QUEUE_TYPE_TO_ULP:
 		/* used by ulp, rxe owns the index */
-		if (WARN_ON(1))
-			pr_warn("%s: attempt to advance driver index\n",
-				__func__);
+		WARN_ON(1);
 		break;
 	}
 }
@@ -227,15 +223,11 @@ static inline void queue_advance_consumer(struct rxe_queue *q,
 		break;
 	case QUEUE_TYPE_TO_CLIENT:
 		/* used by rxe, client owns the index */
-		if (WARN_ON(1))
-			pr_warn("%s: attempt to advance client index\n",
-				__func__);
+		WARN_ON(1);
 		break;
 	case QUEUE_TYPE_FROM_ULP:
 		/* used by ulp, rxe owns the index */
-		if (WARN_ON(1))
-			pr_warn("%s: attempt to advance driver index\n",
-				__func__);
+		WARN_ON(1);
 		break;
 	case QUEUE_TYPE_TO_ULP:
 		/* used by ulp which owns the index */
-- 
2.27.0

Re: [PATCH 1/1] RDMA/rxe: Add function name to the logs

[Jason Gunthorpe]

On Tue, Apr 18, 2023 at 08:26:11PM +0800, Zhu Yanjun wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Add the function names to the pr_ logs. As such, if some bugs occur,
> with function names, it is easy to locate the bugs.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/sw/rxe/rxe.h       |  2 +-
>  drivers/infiniband/sw/rxe/rxe_queue.h | 16 ++++------------
>  2 files changed, 5 insertions(+), 13 deletions(-)

After you delete the warn_on's there is very little left:

rivers/infiniband/sw/rxe/rxe.c:                pr_err("rxe creation allowed on top of a real device only\n");
drivers/infiniband/sw/rxe/rxe.c:        pr_info("loaded\n");
drivers/infiniband/sw/rxe/rxe.c:        pr_info("unloaded\n");
drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to create IPv4 UDP tunnel\n");
drivers/infiniband/sw/rxe/rxe_net.c:            pr_warn("IPv6 is not supported, can not create a UDPv6 socket\n");
drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to create IPv6 UDP tunnel\n");
drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to register netdev notifier\n");

It is not exactly mysterious where these come from?

Why do we need the function name?

Jason

Re: [PATCH 1/1] RDMA/rxe: Add function name to the logs

[Zhu Yanjun]

在 2023/4/21 23:27, Jason Gunthorpe 写道:
> On Tue, Apr 18, 2023 at 08:26:11PM +0800, Zhu Yanjun wrote:
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> Add the function names to the pr_ logs. As such, if some bugs occur,
>> with function names, it is easy to locate the bugs.
>>
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>>   drivers/infiniband/sw/rxe/rxe.h       |  2 +-
>>   drivers/infiniband/sw/rxe/rxe_queue.h | 16 ++++------------
>>   2 files changed, 5 insertions(+), 13 deletions(-)
> After you delete the warn_on's there is very little left:
>
> rivers/infiniband/sw/rxe/rxe.c:                pr_err("rxe creation allowed on top of a real device only\n");
> drivers/infiniband/sw/rxe/rxe.c:        pr_info("loaded\n");
> drivers/infiniband/sw/rxe/rxe.c:        pr_info("unloaded\n");
> drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to create IPv4 UDP tunnel\n");
> drivers/infiniband/sw/rxe/rxe_net.c:            pr_warn("IPv6 is not supported, can not create a UDPv6 socket\n");
> drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to create IPv6 UDP tunnel\n");
> drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to register netdev notifier\n");
>
> It is not exactly mysterious where these come from?

If I got you correctly, you mean that we can easily locate the above 
pr_xxx logs. As such, it is not necessary to add function names.

 From this perspective, I agree with you.

Why I make this commit is that we can directly use this pr_info("xxxxx") 
when some bug occurs. But in the logs, the function names appear.

With this commit, we can decrease some work loads with some bug occurs.

Now I am debugging a problem. With a similar commit with this, I 
directly use pr_warn("xxx"). But in the logs, function names, line 
number, file names, caller and so on will appear.

This greatly decrease my workloads.

I think this is a smart method. So I introduce this smart method to 
softroce. When some bugs occur in softroce, this method can help us.

Zhu Yanjun

>
> Why do we need the function name?
>
> Jason

Re: [PATCH 1/1] RDMA/rxe: Add function name to the logs

[Jason Gunthorpe]

On Fri, Apr 21, 2023 at 11:51:34PM +0800, Zhu Yanjun wrote:
> 
> 在 2023/4/21 23:27, Jason Gunthorpe 写道:
> > On Tue, Apr 18, 2023 at 08:26:11PM +0800, Zhu Yanjun wrote:
> > > From: Zhu Yanjun <yanjun.zhu@linux.dev>
> > > 
> > > Add the function names to the pr_ logs. As such, if some bugs occur,
> > > with function names, it is easy to locate the bugs.
> > > 
> > > Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> > > ---
> > >   drivers/infiniband/sw/rxe/rxe.h       |  2 +-
> > >   drivers/infiniband/sw/rxe/rxe_queue.h | 16 ++++------------
> > >   2 files changed, 5 insertions(+), 13 deletions(-)
> > After you delete the warn_on's there is very little left:
> > 
> > rivers/infiniband/sw/rxe/rxe.c:                pr_err("rxe creation allowed on top of a real device only\n");
> > drivers/infiniband/sw/rxe/rxe.c:        pr_info("loaded\n");
> > drivers/infiniband/sw/rxe/rxe.c:        pr_info("unloaded\n");
> > drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to create IPv4 UDP tunnel\n");
> > drivers/infiniband/sw/rxe/rxe_net.c:            pr_warn("IPv6 is not supported, can not create a UDPv6 socket\n");
> > drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to create IPv6 UDP tunnel\n");
> > drivers/infiniband/sw/rxe/rxe_net.c:            pr_err("Failed to register netdev notifier\n");
> > 
> > It is not exactly mysterious where these come from?
> 
> If I got you correctly, you mean that we can easily locate the above pr_xxx
> logs. As such, it is not necessary to add function names.
> 
> From this perspective, I agree with you.
> 
> Why I make this commit is that we can directly use this pr_info("xxxxx")
> when some bug occurs. But in the logs, the function names appear.
> 
> With this commit, we can decrease some work loads with some bug occurs.

That doesn't seem like a good reason to merge something so abnormal
for pr_* functions

Jason