[PATCH] ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[PATCH] ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[SF Markus Elfring]

From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Date: Mon, 1 Jan 2018 21:42:27 +0100

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/net/ethernet/mellanox/mlx4/en_cq.c | 4 +---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c | 4 +---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c | 4 +---
 drivers/net/ethernet/mellanox/mlx4/main.c  | 4 +---
 drivers/net/ethernet/mellanox/mlx4/reset.c | 1 -
 5 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_cq.c b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
index 1e487acb4667..cf5e0d002b3f 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
@@ -55,10 +55,8 @@ int mlx4_en_create_cq(struct mlx4_en_priv *priv,
 	cq = kzalloc_node(sizeof(*cq), GFP_KERNEL, node);
 	if (!cq) {
 		cq = kzalloc(sizeof(*cq), GFP_KERNEL);
-		if (!cq) {
-			en_err(priv, "Failed to allocate CQ structure\n");
+		if (!cq)
 			return -ENOMEM;
-		}
 	}
 
 	cq->size = entries;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 85e28efcda33..103abe1ad0cb 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -272,10 +272,8 @@ int mlx4_en_create_rx_ring(struct mlx4_en_priv *priv,
 	ring = kzalloc_node(sizeof(*ring), GFP_KERNEL, node);
 	if (!ring) {
 		ring = kzalloc(sizeof(*ring), GFP_KERNEL);
-		if (!ring) {
-			en_err(priv, "Failed to allocate RX ring structure\n");
+		if (!ring)
 			return -ENOMEM;
-		}
 	}
 
 	ring->prod = 0;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 6b6853773848..a21d6ffaa244 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -58,10 +58,8 @@ int mlx4_en_create_tx_ring(struct mlx4_en_priv *priv,
 	ring = kzalloc_node(sizeof(*ring), GFP_KERNEL, node);
 	if (!ring) {
 		ring = kzalloc(sizeof(*ring), GFP_KERNEL);
-		if (!ring) {
-			en_err(priv, "Failed allocating TX ring\n");
+		if (!ring)
 			return -ENOMEM;
-		}
 	}
 
 	ring->size = size;
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 4d84cab77105..c793c5b0b0cf 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -3177,10 +3177,8 @@ static u64 mlx4_enable_sriov(struct mlx4_dev *dev, struct pci_dev *pdev,
 	}
 
 	dev->dev_vfs = kzalloc(total_vfs * sizeof(*dev->dev_vfs), GFP_KERNEL);
-	if (NULL == dev->dev_vfs) {
-		mlx4_err(dev, "Failed to allocate memory for VFs\n");
+	if (!dev->dev_vfs)
 		goto disable_sriov;
-	}
 
 	if (!(dev->flags &  MLX4_FLAG_SRIOV)) {
 		if (total_vfs > fw_enabled_sriov_vfs) {
diff --git a/drivers/net/ethernet/mellanox/mlx4/reset.c b/drivers/net/ethernet/mellanox/mlx4/reset.c
index 0076d88587ca..74d77e56cc35 100644
--- a/drivers/net/ethernet/mellanox/mlx4/reset.c
+++ b/drivers/net/ethernet/mellanox/mlx4/reset.c
@@ -72,7 +72,6 @@ int mlx4_reset(struct mlx4_dev *dev)
 	hca_header = kmalloc(256, GFP_KERNEL);
 	if (!hca_header) {
 		err = -ENOMEM;
-		mlx4_err(dev, "Couldn't allocate memory to save HCA PCI header, aborting\n");
 		goto out;
 	}
 
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[Tariq Toukan]

On 01/01/2018 10:46 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 1 Jan 2018 21:42:27 +0100
> 
> Omit an extra message for a memory allocation failure in these functions.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---

Is this an issue? Why? What is your motivation?
These are error messages, very informative, appear only upon errors, and 
in control flow.

Re: [PATCH] ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[Julia Lawall]

On Wed, 3 Jan 2018, Tariq Toukan wrote:

>
>
> On 01/01/2018 10:46 PM, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 1 Jan 2018 21:42:27 +0100
> >
> > Omit an extra message for a memory allocation failure in these functions.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
>
> Is this an issue? Why? What is your motivation?
> These are error messages, very informative, appear only upon errors, and in
> control flow.

Strings take up space.  Since there is a backtrace on an out of memory
problem, if the string does not provide any more information than the
position of the call, then there is not much added value.  I don't know
what was the string in this case.  If it provides some additional
information, then it would be reasonable to keep it.

julia

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[Tariq Toukan]

On 03/01/2018 10:06 AM, Julia Lawall wrote:
> 
> 
> On Wed, 3 Jan 2018, Tariq Toukan wrote:
> 
>>
>>
>> On 01/01/2018 10:46 PM, SF Markus Elfring wrote:
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Mon, 1 Jan 2018 21:42:27 +0100
>>>
>>> Omit an extra message for a memory allocation failure in these functions.
>>>
>>> This issue was detected by using the Coccinelle software.
>>>
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>> ---
>>
>> Is this an issue? Why? What is your motivation?
>> These are error messages, very informative, appear only upon errors, and in
>> control flow.
> 
> Strings take up space.  Since there is a backtrace on an out of memory
> problem, if the string does not provide any more information than the
> position of the call, then there is not much added value.  I don't know
> what was the string in this case.  If it provides some additional
> information, then it would be reasonable to keep it.

I don't really accept this claim...

Short informative strings worth the tiny space they consume. It helps 
the users of our driver understand what went wrong in simple words, 
without the need to understand the role of the functions/callstack or 
being familiar with different parts of the driver code.

In addition, some out-of-memory errors are recoverable, even though 
their backtrace is also printed. For example, in function 
mlx4_en_create_cq (appears in patch) we have a first allocation attempt 
(kzalloc_node) and a fallback (kzalloc). I'd prefer to state a clear 
error message only when both have failed, because otherwise the user 
might be confused whether the backtrace should indicate a malfunctioning 
interface, or not.

Tariq

> 
> julia
> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH] ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

On Wed, Jan 03, 2018 at 01:24:59PM +0200, Tariq Toukan wrote:
>
>
> On 03/01/2018 10:06 AM, Julia Lawall wrote:
> >
> >
> > On Wed, 3 Jan 2018, Tariq Toukan wrote:
> >
> > >
> > >
> > > On 01/01/2018 10:46 PM, SF Markus Elfring wrote:
> > > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > > Date: Mon, 1 Jan 2018 21:42:27 +0100
> > > >
> > > > Omit an extra message for a memory allocation failure in these functions.
> > > >
> > > > This issue was detected by using the Coccinelle software.
> > > >
> > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > > ---
> > >
> > > Is this an issue? Why? What is your motivation?
> > > These are error messages, very informative, appear only upon errors, and in
> > > control flow.
> >
> > Strings take up space.  Since there is a backtrace on an out of memory
> > problem, if the string does not provide any more information than the
> > position of the call, then there is not much added value.  I don't know
> > what was the string in this case.  If it provides some additional
> > information, then it would be reasonable to keep it.
>
> I don't really accept this claim...
>
> Short informative strings worth the tiny space they consume. It helps the
> users of our driver understand what went wrong in simple words, without the
> need to understand the role of the functions/callstack or being familiar
> with different parts of the driver code.
>
> In addition, some out-of-memory errors are recoverable, even though their
> backtrace is also printed. For example, in function mlx4_en_create_cq
> (appears in patch) we have a first allocation attempt (kzalloc_node) and a
> fallback (kzalloc). I'd prefer to state a clear error message only when both
> have failed, because otherwise the user might be confused whether the
> backtrace should indicate a malfunctioning interface, or not.

Tariq,

There is standard way to handle fallback in allocation and it is to
use __GFP_NOWARN flag in first allocation. So actually you pointed to the
"better-to-be-improved" function call.

Thanks

>
> Tariq
>
> >
> > julia
> >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[SF Markus Elfring]

> I don't really accept this claim...
> Short informative strings worth the tiny space they consume.

There can be different opinions for their usefulness.


> In addition, some out-of-memory errors are recoverable, even though their backtrace is also printed.

How do you think about to suppress the backtrace generation for them?


> For example, in function mlx4_en_create_cq (appears in patch) we have a first allocation attempt (kzalloc_node)

Would it be helpful to pass the option “__GFP_NOWARN” there?


> and a fallback (kzalloc). I'd prefer to state a clear error message only when both have failed,
> because otherwise the user might be confused whether the backtrace should indicate a malfunctioning interface, or not.

Can the distinction become easier by any other means?

Regards,
Markus

Re: [PATCH] ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[Jason Gunthorpe]

On Wed, Jan 03, 2018 at 01:24:59PM +0200, Tariq Toukan wrote:

> >Strings take up space.  Since there is a backtrace on an out of memory
> >problem, if the string does not provide any more information than the
> >position of the call, then there is not much added value.  I don't know
> >what was the string in this case.  If it provides some additional
> >information, then it would be reasonable to keep it.
> 
> I don't really accept this claim...

The standard we are moving to is to rely on the backtrace print for
debugging. It is so huge it is unlikely a single print from the driver
will make much difference to the user's view.

Most users think backtrace == oops == bug report.

> In addition, some out-of-memory errors are recoverable, even though their
> backtrace is also printed. For example, in function mlx4_en_create_cq
> (appears in patch) we have a first allocation attempt (kzalloc_node) and a
> fallback (kzalloc).

Please send a patch fixing this as other have suggested, it is clearly
a bug.

Jason

Re: ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[Tariq Toukan]

On 03/01/2018 4:22 PM, SF Markus Elfring wrote:
>> I don't really accept this claim...
>> Short informative strings worth the tiny space they consume.
> 
> There can be different opinions for their usefulness.
> 
> 
>> In addition, some out-of-memory errors are recoverable, even though their backtrace is also printed.
> 
> How do you think about to suppress the backtrace generation for them?
> 
> 
OK, makes sense.

>> For example, in function mlx4_en_create_cq (appears in patch) we have a first allocation attempt (kzalloc_node)
> 
> Would it be helpful to pass the option “__GFP_NOWARN” there?
> 
> 

I'll prepare a patch to use it.
Will ack this patch for now.

>> and a fallback (kzalloc). I'd prefer to state a clear error message only when both have failed,
>> because otherwise the user might be confused whether the backtrace should indicate a malfunctioning interface, or not.
> 
> Can the distinction become easier by any other means?
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] ethernet: mlx4: Delete an error message for a failed memory allocation in five functions

[Tariq Toukan]

On 01/01/2018 10:46 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Date: Mon, 1 Jan 2018 21:42:27 +0100
> 
> Omit an extra message for a memory allocation failure in these functions.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---


Acked-by: Tariq Toukan <tariqt-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html