| Message ID | 30191db0-4d99-0349-b66a-c7354ef90d50@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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. -- 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
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 > -- 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
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 >> -- 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
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
> 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 -- 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
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 -- 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
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 > -- 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
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> > --- Acked-by: Tariq Toukan <tariqt@mellanox.com> Thanks. -- 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
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; }