[PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[Gaurav Singh]

parent cannot be NULL here since its in the else part
of the if (parent == NULL) condition. Remove the extra
check on parent pointer.

Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
---
 net/sched/sch_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 9a3449b56bd6..8c92d00c5c8e 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1094,7 +1094,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 
 		/* Only support running class lockless if parent is lockless */
 		if (new && (new->flags & TCQ_F_NOLOCK) &&
-		    parent && !(parent->flags & TCQ_F_NOLOCK))
+		    && !(parent->flags & TCQ_F_NOLOCK))
 			qdisc_clear_nolock(new);
 
 		if (!cops || !cops->graft)
-- 
2.17.1

Re: [PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[Jakub Kicinski]

On Wed, 17 Jun 2020 20:55:26 -0400 Gaurav Singh wrote:
> parent cannot be NULL here since its in the else part
> of the if (parent == NULL) condition. Remove the extra
> check on parent pointer.
> 
> Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>

Change seems legit, but it obviously doesn't build...

>  net/sched/sch_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 9a3449b56bd6..8c92d00c5c8e 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1094,7 +1094,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  
>  		/* Only support running class lockless if parent is lockless */
>  		if (new && (new->flags & TCQ_F_NOLOCK) &&
> -		    parent && !(parent->flags & TCQ_F_NOLOCK))
> +		    && !(parent->flags & TCQ_F_NOLOCK))
>  			qdisc_clear_nolock(new);
>  
>  		if (!cops || !cops->graft)

[PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[Gaurav Singh]

Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>

Fix build errors

Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
---
 net/sched/sch_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 9a3449b56bd6..be93ebcdb18d 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1094,7 +1094,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 
 		/* Only support running class lockless if parent is lockless */
 		if (new && (new->flags & TCQ_F_NOLOCK) &&
-		    parent && !(parent->flags & TCQ_F_NOLOCK))
+			!(parent->flags & TCQ_F_NOLOCK))
 			qdisc_clear_nolock(new);
 
 		if (!cops || !cops->graft)
-- 
2.17.1

Re: [PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[Eric Dumazet]

On 6/17/20 6:23 PM, Gaurav Singh wrote:
> Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>

Two Signed-off-by ?

> 
> Fix build errors

Your patch does not fix a build error.

> 
> Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
> ---
>  net/sched/sch_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 9a3449b56bd6..be93ebcdb18d 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1094,7 +1094,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  
>  		/* Only support running class lockless if parent is lockless */
>  		if (new && (new->flags & TCQ_F_NOLOCK) &&
> -		    parent && !(parent->flags & TCQ_F_NOLOCK))
> +			!(parent->flags & TCQ_F_NOLOCK))
>  			qdisc_clear_nolock(new);
>  
>  		if (!cops || !cops->graft)
> 

[PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[Gaurav Singh]

parent cannot be NULL here since its in the else part
of the if (parent == NULL) condition. Remove the extra
check on parent pointer.

Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
---
 net/sched/sch_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 9a3449b56bd6..be93ebcdb18d 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1094,7 +1094,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 
 		/* Only support running class lockless if parent is lockless */
 		if (new && (new->flags & TCQ_F_NOLOCK) &&
-		    parent && !(parent->flags & TCQ_F_NOLOCK))
+			!(parent->flags & TCQ_F_NOLOCK))
 			qdisc_clear_nolock(new);
 
 		if (!cops || !cops->graft)
-- 
2.17.1

Re: [PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[kernel test robot]

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

Hi Gaurav,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.8-rc1 next-20200617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Gaurav-Singh/Remove-redundant-condition-in-qdisc_graft/20200618-085703
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1b5044021070efa3259f3e9548dc35d1eb6aa844
config: s390-randconfig-r016-20200618 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):


vim +1097 net/sched/sch_api.c

  1019	
  1020	/* Graft qdisc "new" to class "classid" of qdisc "parent" or
  1021	 * to device "dev".
  1022	 *
  1023	 * When appropriate send a netlink notification using 'skb'
  1024	 * and "n".
  1025	 *
  1026	 * On success, destroy old qdisc.
  1027	 */
  1028	
  1029	static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
  1030			       struct sk_buff *skb, struct nlmsghdr *n, u32 classid,
  1031			       struct Qdisc *new, struct Qdisc *old,
  1032			       struct netlink_ext_ack *extack)
  1033	{
  1034		struct Qdisc *q = old;
  1035		struct net *net = dev_net(dev);
  1036	
  1037		if (parent == NULL) {
  1038			unsigned int i, num_q, ingress;
  1039	
  1040			ingress = 0;
  1041			num_q = dev->num_tx_queues;
  1042			if ((q && q->flags & TCQ_F_INGRESS) ||
  1043			    (new && new->flags & TCQ_F_INGRESS)) {
  1044				num_q = 1;
  1045				ingress = 1;
  1046				if (!dev_ingress_queue(dev)) {
  1047					NL_SET_ERR_MSG(extack, "Device does not have an ingress queue");
  1048					return -ENOENT;
  1049				}
  1050			}
  1051	
  1052			if (dev->flags & IFF_UP)
  1053				dev_deactivate(dev);
  1054	
  1055			qdisc_offload_graft_root(dev, new, old, extack);
  1056	
  1057			if (new && new->ops->attach)
  1058				goto skip;
  1059	
  1060			for (i = 0; i < num_q; i++) {
  1061				struct netdev_queue *dev_queue = dev_ingress_queue(dev);
  1062	
  1063				if (!ingress)
  1064					dev_queue = netdev_get_tx_queue(dev, i);
  1065	
  1066				old = dev_graft_qdisc(dev_queue, new);
  1067				if (new && i > 0)
  1068					qdisc_refcount_inc(new);
  1069	
  1070				if (!ingress)
  1071					qdisc_put(old);
  1072			}
  1073	
  1074	skip:
  1075			if (!ingress) {
  1076				notify_and_destroy(net, skb, n, classid,
  1077						   dev->qdisc, new);
  1078				if (new && !new->ops->attach)
  1079					qdisc_refcount_inc(new);
  1080				dev->qdisc = new ? : &noop_qdisc;
  1081	
  1082				if (new && new->ops->attach)
  1083					new->ops->attach(new);
  1084			} else {
  1085				notify_and_destroy(net, skb, n, classid, old, new);
  1086			}
  1087	
  1088			if (dev->flags & IFF_UP)
  1089				dev_activate(dev);
  1090		} else {
  1091			const struct Qdisc_class_ops *cops = parent->ops->cl_ops;
  1092			unsigned long cl;
  1093			int err;
  1094	
  1095			/* Only support running class lockless if parent is lockless */
  1096			if (new && (new->flags & TCQ_F_NOLOCK) &&
> 1097			    && !(parent->flags & TCQ_F_NOLOCK))
  1098				qdisc_clear_nolock(new);
  1099	
  1100			if (!cops || !cops->graft)
  1101				return -EOPNOTSUPP;
  1102	
  1103			cl = cops->find(parent, classid);
  1104			if (!cl) {
  1105				NL_SET_ERR_MSG(extack, "Specified class not found");
  1106				return -ENOENT;
  1107			}
  1108	
  1109			err = cops->graft(parent, cl, new, &old, extack);
  1110			if (err)
  1111				return err;
  1112			notify_and_destroy(net, skb, n, classid, old, new);
  1113		}
  1114		return 0;
  1115	}
  1116	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21348 bytes --]

Re: [PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[David Miller]

From: Gaurav Singh <gaurav1086@gmail.com>
Date: Thu, 18 Jun 2020 00:00:56 -0400

> parent cannot be NULL here since its in the else part
> of the if (parent == NULL) condition. Remove the extra
> check on parent pointer.
> 
> Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
> ---
>  net/sched/sch_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 9a3449b56bd6..be93ebcdb18d 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1094,7 +1094,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  
>  		/* Only support running class lockless if parent is lockless */
>  		if (new && (new->flags & TCQ_F_NOLOCK) &&
> -		    parent && !(parent->flags & TCQ_F_NOLOCK))
> +			!(parent->flags & TCQ_F_NOLOCK))

You've broken the indentation of this line, it was correctly indented
before your change.

[PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[Gaurav Singh]

parent cannot be NULL here since its in the else part
of the if (parent == NULL) condition. Remove the extra
check on parent pointer.

Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
---
 net/sched/sch_api.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 9a3449b56bd6..11ebba60da3b 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1093,8 +1093,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 		int err;
 
 		/* Only support running class lockless if parent is lockless */
-		if (new && (new->flags & TCQ_F_NOLOCK) &&
-		    parent && !(parent->flags & TCQ_F_NOLOCK))
+		if (new && (new->flags & TCQ_F_NOLOCK) && !(parent->flags & TCQ_F_NOLOCK))
 			qdisc_clear_nolock(new);
 
 		if (!cops || !cops->graft)
-- 
2.17.1

Re: [PATCH] [net/sched]: Remove redundant condition in qdisc_graft

[David Miller]

From: Gaurav Singh <gaurav1086@gmail.com>
Date: Thu, 18 Jun 2020 16:36:31 -0400

> parent cannot be NULL here since its in the else part
> of the if (parent == NULL) condition. Remove the extra
> check on parent pointer.
> 
> Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>

Applied to net-next, thanks.