IS_ERR(), IS_ERR_OR_NULL(), IS_ERR_VALUE() and WARN*() already contain
unlikely() optimization internally. Thus, there is no point in calling
these functions and defines under likely()/unlikely().
This check is based on the coccinelle rule developed by Enrico Weigelt
https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Denis Efremov <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f05..56969ce06df4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6480,6 +6480,12 @@ sub process {
"Using $1 should generally have parentheses around the comparison\n" . $herecurr);
}
+# nested likely/unlikely calls
+ if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
+ WARN("LIKELY_MISUSE",
+ "nested (un)?likely() calls, $1 already uses unlikely() internally\n" . $herecurr);
+ }
+
# whine mightly about in_atomic
if ($line =~ /\bin_atomic\s*\(/) {
if ($realfile =~ m@^drivers/@) {
--
2.21.0
"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Sean Paul <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c | 4 ++--
drivers/gpu/drm/msm/disp/mdp_format.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
index 4804cf40de14..030279d7b64b 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
@@ -253,7 +253,7 @@ int mdp5_ctl_set_cursor(struct mdp5_ctl *ctl, struct mdp5_pipeline *pipeline,
u32 blend_cfg;
struct mdp5_hw_mixer *mixer = pipeline->mixer;
- if (unlikely(WARN_ON(!mixer))) {
+ if (WARN_ON(!mixer)) {
DRM_DEV_ERROR(ctl_mgr->dev->dev, "CTL %d cannot find LM",
ctl->id);
return -EINVAL;
@@ -695,7 +695,7 @@ struct mdp5_ctl_manager *mdp5_ctlm_init(struct drm_device *dev,
goto fail;
}
- if (unlikely(WARN_ON(ctl_cfg->count > MAX_CTL))) {
+ if (WARN_ON(ctl_cfg->count > MAX_CTL)) {
DRM_DEV_ERROR(dev->dev, "Increase static pool size to at least %d\n",
ctl_cfg->count);
ret = -ENOSPC;
diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c b/drivers/gpu/drm/msm/disp/mdp_format.c
index 8afb0f9c04bb..5495d8b3f5b9 100644
--- a/drivers/gpu/drm/msm/disp/mdp_format.c
+++ b/drivers/gpu/drm/msm/disp/mdp_format.c
@@ -174,7 +174,7 @@ const struct msm_format *mdp_get_format(struct msm_kms *kms, uint32_t format,
struct csc_cfg *mdp_get_default_csc_cfg(enum csc_type type)
{
- if (unlikely(WARN_ON(type >= CSC_MAX)))
+ if (WARN_ON(type >= CSC_MAX))
return NULL;
return &csc_convert[type];
--
2.21.0
"unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
unlikely() internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: Boris Pismenny <[email protected]>
Cc: Saeed Mahameed <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
index 7833ddef0427..e5222d17df35 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
@@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct net_device *netdev,
goto out;
tls_ctx = tls_get_ctx(skb->sk);
- if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
+ if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
goto err_out;
priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
--
2.21.0
"unlikely(WARN(x))" is excessive. WARN() already uses unlikely()
internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
drivers/xen/events/events_base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 2e8570c09789..6c8843968a52 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -247,7 +247,7 @@ static void xen_irq_info_cleanup(struct irq_info *info)
*/
unsigned int evtchn_from_irq(unsigned irq)
{
- if (unlikely(WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)))
+ if (WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq))
return 0;
return info_for_irq(irq)->evtchn;
--
2.21.0
"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: "Darrick J. Wong" <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
fs/xfs/xfs_buf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index ca0849043f54..4389d87ff0f0 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -2096,7 +2096,7 @@ xfs_verify_magic(
int idx;
idx = xfs_sb_version_hascrc(&mp->m_sb);
- if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
+ if (WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx]))
return false;
return dmagic == bp->b_ops->magic[idx];
}
@@ -2114,7 +2114,7 @@ xfs_verify_magic16(
int idx;
idx = xfs_sb_version_hascrc(&mp->m_sb);
- if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx])))
+ if (WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx]))
return false;
return dmagic == bp->b_ops->magic16[idx];
}
--
2.21.0
"unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
unlikely() internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: Mike Marciniszyn <[email protected]>
Cc: Dennis Dalessandro <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
drivers/infiniband/hw/hfi1/verbs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
index 646f61545ed6..1c52b19dd0f8 100644
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ b/drivers/infiniband/hw/hfi1/verbs.c
@@ -1040,7 +1040,7 @@ int hfi1_verbs_send_pio(struct rvt_qp *qp, struct hfi1_pkt_state *ps,
if (cb)
iowait_pio_inc(&priv->s_iowait);
pbuf = sc_buffer_alloc(sc, plen, cb, qp);
- if (unlikely(IS_ERR_OR_NULL(pbuf))) {
+ if (IS_ERR_OR_NULL(pbuf)) {
if (cb)
verbs_pio_complete(qp, 0);
if (IS_ERR(pbuf)) {
--
2.21.0
"unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
unlikely() internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: "Pali Rohár" <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
drivers/input/mouse/alps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 34700eda0429..ed1661434899 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
/* On V2 devices the DualPoint Stick reports bare packets */
dev = priv->dev2;
dev2 = psmouse->dev;
- } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
+ } else if (IS_ERR_OR_NULL(priv->dev3)) {
/* Register dev3 mouse if we received PS/2 packet first time */
if (!IS_ERR(priv->dev3))
psmouse_queue_work(psmouse, &priv->dev3_register_work,
--
2.21.0
"likely(!IS_ERR(x))" is excessive. IS_ERR() already uses
unlikely() internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: Anton Altaparmakov <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
fs/ntfs/mft.c | 12 ++++++------
fs/ntfs/namei.c | 2 +-
fs/ntfs/runlist.c | 2 +-
fs/ntfs/super.c | 2 +-
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index 20c841a906f2..3aac5c917afe 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -71,7 +71,7 @@ static inline MFT_RECORD *map_mft_record_page(ntfs_inode *ni)
}
/* Read, map, and pin the page. */
page = ntfs_map_page(mft_vi->i_mapping, index);
- if (likely(!IS_ERR(page))) {
+ if (!IS_ERR(page)) {
/* Catch multi sector transfer fixup errors. */
if (likely(ntfs_is_mft_recordp((le32*)(page_address(page) +
ofs)))) {
@@ -154,7 +154,7 @@ MFT_RECORD *map_mft_record(ntfs_inode *ni)
mutex_lock(&ni->mrec_lock);
m = map_mft_record_page(ni);
- if (likely(!IS_ERR(m)))
+ if (!IS_ERR(m))
return m;
mutex_unlock(&ni->mrec_lock);
@@ -271,7 +271,7 @@ MFT_RECORD *map_extent_mft_record(ntfs_inode *base_ni, MFT_REF mref,
m = map_mft_record(ni);
/* map_mft_record() has incremented this on success. */
atomic_dec(&ni->count);
- if (likely(!IS_ERR(m))) {
+ if (!IS_ERR(m)) {
/* Verify the sequence number. */
if (likely(le16_to_cpu(m->sequence_number) == seq_no)) {
ntfs_debug("Done 1.");
@@ -1303,7 +1303,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(ntfs_volume *vol)
read_unlock_irqrestore(&mftbmp_ni->size_lock, flags);
rl = ntfs_attr_find_vcn_nolock(mftbmp_ni,
(ll - 1) >> vol->cluster_size_bits, NULL);
- if (unlikely(IS_ERR(rl) || !rl->length || rl->lcn < 0)) {
+ if (IS_ERR(rl) || unlikely(!rl->length || rl->lcn < 0)) {
up_write(&mftbmp_ni->runlist.lock);
ntfs_error(vol->sb, "Failed to determine last allocated "
"cluster of mft bitmap attribute.");
@@ -1734,7 +1734,7 @@ static int ntfs_mft_data_extend_allocation_nolock(ntfs_volume *vol)
read_unlock_irqrestore(&mft_ni->size_lock, flags);
rl = ntfs_attr_find_vcn_nolock(mft_ni,
(ll - 1) >> vol->cluster_size_bits, NULL);
- if (unlikely(IS_ERR(rl) || !rl->length || rl->lcn < 0)) {
+ if (IS_ERR(rl) || unlikely(!rl->length || rl->lcn < 0)) {
up_write(&mft_ni->runlist.lock);
ntfs_error(vol->sb, "Failed to determine last allocated "
"cluster of mft data attribute.");
@@ -1776,7 +1776,7 @@ static int ntfs_mft_data_extend_allocation_nolock(ntfs_volume *vol)
do {
rl2 = ntfs_cluster_alloc(vol, old_last_vcn, nr, lcn, MFT_ZONE,
true);
- if (likely(!IS_ERR(rl2)))
+ if (!IS_ERR(rl2))
break;
if (PTR_ERR(rl2) != -ENOSPC || nr == min_nr) {
ntfs_error(vol->sb, "Failed to allocate the minimal "
diff --git a/fs/ntfs/namei.c b/fs/ntfs/namei.c
index 2d3cc9e3395d..4e6a44bc654c 100644
--- a/fs/ntfs/namei.c
+++ b/fs/ntfs/namei.c
@@ -115,7 +115,7 @@ static struct dentry *ntfs_lookup(struct inode *dir_ino, struct dentry *dent,
dent_ino = MREF(mref);
ntfs_debug("Found inode 0x%lx. Calling ntfs_iget.", dent_ino);
dent_inode = ntfs_iget(vol->sb, dent_ino);
- if (likely(!IS_ERR(dent_inode))) {
+ if (!IS_ERR(dent_inode)) {
/* Consistency check. */
if (is_bad_inode(dent_inode) || MSEQNO(mref) ==
NTFS_I(dent_inode)->seq_no ||
diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c
index 508744a93180..97932fb5179c 100644
--- a/fs/ntfs/runlist.c
+++ b/fs/ntfs/runlist.c
@@ -951,7 +951,7 @@ runlist_element *ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
}
/* Now combine the new and old runlists checking for overlaps. */
old_rl = ntfs_runlists_merge(old_rl, rl);
- if (likely(!IS_ERR(old_rl)))
+ if (!IS_ERR(old_rl))
return old_rl;
ntfs_free(rl);
ntfs_error(vol->sb, "Failed to merge runlists.");
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 29621d40f448..7dc3bc604f78 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -1475,7 +1475,7 @@ static bool load_and_init_usnjrnl(ntfs_volume *vol)
kfree(name);
/* Get the inode. */
tmp_ino = ntfs_iget(vol->sb, MREF(mref));
- if (unlikely(IS_ERR(tmp_ino) || is_bad_inode(tmp_ino))) {
+ if (IS_ERR(tmp_ino) || unlikely(is_bad_inode(tmp_ino))) {
if (!IS_ERR(tmp_ino))
iput(tmp_ino);
ntfs_error(vol->sb, "Failed to load $UsnJrnl.");
--
2.21.0
"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: Inaky Perez-Gonzalez <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
drivers/net/wimax/i2400m/tx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wimax/i2400m/tx.c b/drivers/net/wimax/i2400m/tx.c
index ebd64e083726..1255302e251e 100644
--- a/drivers/net/wimax/i2400m/tx.c
+++ b/drivers/net/wimax/i2400m/tx.c
@@ -654,8 +654,7 @@ void i2400m_tx_close(struct i2400m *i2400m)
padding = aligned_size - tx_msg_moved->size;
if (padding > 0) {
pad_buf = i2400m_tx_fifo_push(i2400m, padding, 0, 0);
- if (unlikely(WARN_ON(pad_buf == NULL
- || pad_buf == TAIL_FULL))) {
+ if (WARN_ON(pad_buf == NULL || pad_buf == TAIL_FULL)) {
/* This should not happen -- append should verify
* there is always space left at least to append
* tx_block_size */
--
2.21.0
"unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
unlikely() internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
include/net/udp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/udp.h b/include/net/udp.h
index 79d141d2103b..bad74f780831 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -480,7 +480,7 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
* CB fragment
*/
segs = __skb_gso_segment(skb, features, false);
- if (unlikely(IS_ERR_OR_NULL(segs))) {
+ if (IS_ERR_OR_NULL(segs)) {
int segs_nr = skb_shinfo(skb)->gso_segs;
atomic_add(segs_nr, &sk->sk_drops);
--
2.21.0
"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.
Signed-off-by: Denis Efremov <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
fs/open.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/open.c b/fs/open.c
index a59abe3c669a..9432cd5a8294 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -776,7 +776,7 @@ static int do_dentry_open(struct file *f,
f->f_mode |= FMODE_ATOMIC_POS;
f->f_op = fops_get(inode->i_fop);
- if (unlikely(WARN_ON(!f->f_op))) {
+ if (WARN_ON(!f->f_op)) {
error = -ENODEV;
goto cleanup_all;
}
--
2.21.0
On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> unlikely() internally.
The keyword here is _internally_.
https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
So please no.
>
> Signed-off-by: Denis Efremov <[email protected]>
> Cc: "Pali Roh?r" <[email protected]>
> Cc: Dmitry Torokhov <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> ---
> drivers/input/mouse/alps.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 34700eda0429..ed1661434899 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> /* On V2 devices the DualPoint Stick reports bare packets */
> dev = priv->dev2;
> dev2 = psmouse->dev;
> - } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> + } else if (IS_ERR_OR_NULL(priv->dev3)) {
> /* Register dev3 mouse if we received PS/2 packet first time */
> if (!IS_ERR(priv->dev3))
> psmouse_queue_work(psmouse, &priv->dev3_register_work,
> --
> 2.21.0
>
--
Dmitry
On Thu, Aug 29, 2019 at 07:50:21PM +0300, Denis Efremov wrote:
> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.
>
> Signed-off-by: Denis Efremov <[email protected]>
> Cc: "Darrick J. Wong" <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
LGTM,
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/xfs/xfs_buf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ca0849043f54..4389d87ff0f0 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -2096,7 +2096,7 @@ xfs_verify_magic(
> int idx;
>
> idx = xfs_sb_version_hascrc(&mp->m_sb);
> - if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
> + if (WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx]))
> return false;
> return dmagic == bp->b_ops->magic[idx];
> }
> @@ -2114,7 +2114,7 @@ xfs_verify_magic16(
> int idx;
>
> idx = xfs_sb_version_hascrc(&mp->m_sb);
> - if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx])))
> + if (WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx]))
> return false;
> return dmagic == bp->b_ops->magic16[idx];
> }
> --
> 2.21.0
>
On Thu, 2019-08-29 at 19:50 +0300, Denis Efremov wrote:
> "unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
> unlikely() internally.
>
> Signed-off-by: Denis Efremov <[email protected]>
> Cc: Boris Pismenny <[email protected]>
> Cc: Saeed Mahameed <[email protected]>
> Cc: Leon Romanovsky <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> index 7833ddef0427..e5222d17df35 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> @@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct
> net_device *netdev,
> goto out;
>
> tls_ctx = tls_get_ctx(skb->sk);
> - if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
> + if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
> goto err_out;
>
> priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
Acked-by: Saeed Mahameed <[email protected]>
Dave, you can take this one.
On Thu, Aug 29, 2019 at 07:50:21PM +0300, Denis Efremov wrote:
> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
From: Saeed Mahameed <[email protected]>
Date: Thu, 29 Aug 2019 21:23:30 +0000
> On Thu, 2019-08-29 at 19:50 +0300, Denis Efremov wrote:
>> "unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
>> unlikely() internally.
>>
>> Signed-off-by: Denis Efremov <[email protected]>
>> Cc: Boris Pismenny <[email protected]>
>> Cc: Saeed Mahameed <[email protected]>
>> Cc: Leon Romanovsky <[email protected]>
>> Cc: Joe Perches <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: [email protected]
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git
>> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> index 7833ddef0427..e5222d17df35 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> @@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct
>> net_device *netdev,
>> goto out;
>>
>> tls_ctx = tls_get_ctx(skb->sk);
>> - if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
>> + if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
>> goto err_out;
>>
>> priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
>
> Acked-by: Saeed Mahameed <[email protected]>
>
> Dave, you can take this one.
Ok, applied to net-next as well as the UDP patch.
> pad_buf = i2400m_tx_fifo_push(i2400m, padding, 0, 0);
> - if (unlikely(WARN_ON(pad_buf == NULL
> - || pad_buf == TAIL_FULL))) {
> + if (WARN_ON(pad_buf == NULL || pad_buf == TAIL_FULL)) {
How do you think about to use the following code variant?
+ if (WARN_ON(!pad_buf || pad_buf == TAIL_FULL)) {
Regards,
Markus
On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > unlikely() internally.
>
> The keyword here is _internally_.
>
> https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
>
> So please no.
Dmitry and I already rejected this patch, see also linked-list:
https://lore.kernel.org/lkml/20190820111719.7blyk5jstgwde2ae@pali/
> >
> > Signed-off-by: Denis Efremov <[email protected]>
> > Cc: "Pali Rohár" <[email protected]>
> > Cc: Dmitry Torokhov <[email protected]>
> > Cc: Joe Perches <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/input/mouse/alps.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > index 34700eda0429..ed1661434899 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> > /* On V2 devices the DualPoint Stick reports bare packets */
> > dev = priv->dev2;
> > dev2 = psmouse->dev;
> > - } else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> > + } else if (IS_ERR_OR_NULL(priv->dev3)) {
> > /* Register dev3 mouse if we received PS/2 packet first time */
> > if (!IS_ERR(priv->dev3))
> > psmouse_queue_work(psmouse, &priv->dev3_register_work,
> > --
> > 2.21.0
> >
>
--
Pali Rohár
[email protected]
On 31.08.2019 18:25, Pali Rohár wrote:
> On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
>> On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
>>> "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
>>> unlikely() internally.
>>
>> The keyword here is _internally_.
>>
>> https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
>>
>> So please no.
>
> Dmitry and I already rejected this patch, see also linked-list:
> https://lore.kernel.org/lkml/20190820111719.7blyk5jstgwde2ae@pali/
>
Looks like this is a very long recurring story with this patch.
Thanks, for the clarification.
Regards,
Denis
On 31.08.2019 12:15, Markus Elfring wrote:
>> +# nested likely/unlikely calls
>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>> + WARN("LIKELY_MISUSE",
>
> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
> in this regular expression?
Hmm,
(?: <- Catch group is required here, since it is used in diagnostic message,
see $1
IS_ERR
(?:_ <- Another atomic group just to show that '_' is a common prefix?
I'm not sure about this. Usually, Perl interpreter is very good at optimizing such things.
You could see this optimization if you run perl with -Mre=debug.
(?:OR_NULL|VALUE))?|WARN)
Regards,
Denis
On 31.08.2019 14:25, Markus Elfring wrote:
>> pad_buf = i2400m_tx_fifo_push(i2400m, padding, 0, 0);
>> - if (unlikely(WARN_ON(pad_buf == NULL
>> - || pad_buf == TAIL_FULL))) {
>> + if (WARN_ON(pad_buf == NULL || pad_buf == TAIL_FULL)) {
>
> How do you think about to use the following code variant?
>
> + if (WARN_ON(!pad_buf || pad_buf == TAIL_FULL)) {
>
Well, I thought about it, because coccinelle warns about style here.
But this condition looks more symmetric with direct comparison.
I've decided that it will be better to save the original style.
Thanks,
Denis
>>> +# nested likely/unlikely calls
>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>> + WARN("LIKELY_MISUSE",
>>
>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>> in this regular expression?
…
> IS_ERR
> (?:_ <- Another atomic group just to show that '_' is a common prefix?
Yes. - I hope that this specification detail can help a bit.
> Usually, Perl interpreter is very good at optimizing such things.
Would you like to help this software component by omitting a pair of
non-capturing parentheses at the beginning?
\b(?:un)?likely\s*
Regards,
Markus
On 31.08.2019 19:45, Markus Elfring wrote:
>>>> +# nested likely/unlikely calls
>>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>> + WARN("LIKELY_MISUSE",
>>>
>>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>>> in this regular expression?
> …
>> IS_ERR
>> (?:_ <- Another atomic group just to show that '_' is a common prefix?
>
> Yes. - I hope that this specification detail can help a bit.
I'm not sure that another pair of brackets for a single char worth it.
>> Usually, Perl interpreter is very good at optimizing such things.
The interpreter optimizes it internally:
echo 'IS_ERR_OR_NULL' | perl -Mre=debug -ne '/IS_ERR(?:_OR_NULL|_VALUE)?/ && print'
Compiling REx "IS_ERR(?:_OR_NULL|_VALUE)?"
Final program:
1: EXACT <IS_ERR> (4)
4: CURLYX[0]{0,1} (16)
6: EXACT <_> (8) <-- common prefix
8: TRIE-EXACT[OV] (15)
<OR_NULL>
<VALUE>
...
>
> Would you like to help this software component by omitting a pair of
> non-capturing parentheses at the beginning?
>
> \b(?:un)?likely\s*
This pair of brackets is required to match "unlikely" and it's
optional in order to match "likely".
Regards,
Denis
>>>>> +# nested likely/unlikely calls
>>>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>>> + WARN("LIKELY_MISUSE",
…
>> \b(?:un)?likely\s*
>
> This pair of brackets is required to match "unlikely"
> and it's optional in order to match "likely".
I agree also to this view if you refer to the shortened regular expression here.
But I got an other development opinion for an extra pair of non-capturing parentheses
at the front (from the version which you suggested).
Regards,
Markus
On Sat, 2019-08-31 at 17:25 +0200, Pali Roh?r wrote:
> On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> > On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > > unlikely() internally.
> >
> > The keyword here is _internally_.
> >
> > https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
> >
> > So please no.
I think it poor form not to simply restate your original
objection from 4 message levels below this link
https://lists.gt.net/linux/kernel/2269724
Hm... I do not like this change. If I read code
if (unlikely(IS_ERR_OR_NULL(priv->dev3)))
then I know that it is really unlikely that condition will be truth and
so this is some case of error/exception or something that normally does
not happen too much.
But if I read code
if (IS_ERR_OR_NULL(priv->dev3))
I know nothing about chance that this condition will be truth. Explicit
unlikely in previous example give me more information.
I alslo think this argument is dubious as it also applies
to any IS_ERR and all the unlikely uses have been removed
from those.
On Sat, Aug 31, 2019 at 01:32:02PM -0700, Joe Perches wrote:
> On Sat, 2019-08-31 at 17:25 +0200, Pali Roh?r wrote:
> > On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> > > On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > > > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > > > unlikely() internally.
> > >
> > > The keyword here is _internally_.
> > >
> > > https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
> > >
> > > So please no.
>
> I think it poor form not to simply restate your original
> objection from 4 message levels below this link
Thank you for the lesson in etiquette, but I posted reference to the
very message I wanted.
>
> https://lists.gt.net/linux/kernel/2269724
>
> Hm... I do not like this change. If I read code
>
> if (unlikely(IS_ERR_OR_NULL(priv->dev3)))
>
> then I know that it is really unlikely that condition will be truth and
> so this is some case of error/exception or something that normally does
> not happen too much.
>
> But if I read code
>
> if (IS_ERR_OR_NULL(priv->dev3))
>
> I know nothing about chance that this condition will be truth. Explicit
> unlikely in previous example give me more information.
>
> I alslo think this argument is dubious as it also applies
> to any IS_ERR and all the unlikely uses have been removed
> from those.
No, if you read the reference I posted, the argument does not apply to
all IS_ERR() instances. Majority of them are in probe() paths where we
do not really care about likely/unlikely. Here we are dealing with
IS_ERR in a [fairly] hot path.
Thanks.
--
Dmitry
On Thu 29 Aug 09:50 PDT 2019, Denis Efremov wrote:
> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.
>
> Signed-off-by: Denis Efremov <[email protected]>
> Cc: Rob Clark <[email protected]>
> Cc: Sean Paul <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c | 4 ++--
> drivers/gpu/drm/msm/disp/mdp_format.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> index 4804cf40de14..030279d7b64b 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> @@ -253,7 +253,7 @@ int mdp5_ctl_set_cursor(struct mdp5_ctl *ctl, struct mdp5_pipeline *pipeline,
> u32 blend_cfg;
> struct mdp5_hw_mixer *mixer = pipeline->mixer;
>
> - if (unlikely(WARN_ON(!mixer))) {
> + if (WARN_ON(!mixer)) {
> DRM_DEV_ERROR(ctl_mgr->dev->dev, "CTL %d cannot find LM",
> ctl->id);
> return -EINVAL;
> @@ -695,7 +695,7 @@ struct mdp5_ctl_manager *mdp5_ctlm_init(struct drm_device *dev,
> goto fail;
> }
>
> - if (unlikely(WARN_ON(ctl_cfg->count > MAX_CTL))) {
> + if (WARN_ON(ctl_cfg->count > MAX_CTL)) {
> DRM_DEV_ERROR(dev->dev, "Increase static pool size to at least %d\n",
> ctl_cfg->count);
> ret = -ENOSPC;
> diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c b/drivers/gpu/drm/msm/disp/mdp_format.c
> index 8afb0f9c04bb..5495d8b3f5b9 100644
> --- a/drivers/gpu/drm/msm/disp/mdp_format.c
> +++ b/drivers/gpu/drm/msm/disp/mdp_format.c
> @@ -174,7 +174,7 @@ const struct msm_format *mdp_get_format(struct msm_kms *kms, uint32_t format,
>
> struct csc_cfg *mdp_get_default_csc_cfg(enum csc_type type)
> {
> - if (unlikely(WARN_ON(type >= CSC_MAX)))
> + if (WARN_ON(type >= CSC_MAX))
> return NULL;
>
> return &csc_convert[type];
> --
> 2.21.0
>