[v2 PATCH 1/2] netprio_cgroup: Remove update_netdev_tables() since it is unnecessary

[v2 PATCH 1/2] netprio_cgroup: Remove update_netdev_tables() since it is unnecessary

[Srivatsa S. Bhat]

The update_netdev_tables() function appears to be unnecessary, since the
write_update_netdev_table() function will adjust the priomaps as and when
required anyway. So drop the usage of update_netdev_tables() entirely.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 net/core/netprio_cgroup.c |   32 --------------------------------
 1 files changed, 0 insertions(+), 32 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index c75e3f9..fd339bb0 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -109,32 +109,6 @@ static int write_update_netdev_table(struct net_device *dev)
 	return ret;
 }
 
-static int update_netdev_tables(void)
-{
-	int ret = 0;
-	struct net_device *dev;
-	u32 max_len;
-	struct netprio_map *map;
-
-	rtnl_lock();
-	max_len = atomic_read(&max_prioidx) + 1;
-	for_each_netdev(&init_net, dev) {
-		map = rtnl_dereference(dev->priomap);
-		/*
-		 * don't allocate priomap if we didn't
-		 * change net_prio.ifpriomap (map == NULL),
-		 * this will speed up skb_update_prio.
-		 */
-		if (map && map->priomap_len < max_len) {
-			ret = extend_netdev_table(dev, max_len);
-			if (ret < 0)
-				break;
-		}
-	}
-	rtnl_unlock();
-	return ret;
-}
-
 static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp)
 {
 	struct cgroup_netprio_state *cs;
@@ -153,12 +127,6 @@ static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp)
 		goto out;
 	}
 
-	ret = update_netdev_tables();
-	if (ret < 0) {
-		put_prioidx(cs->prioidx);
-		goto out;
-	}
-
 	return &cs->css;
 out:
 	kfree(cs);

[v2 PATCH 2/2] netprio_cgroup: Use memcpy instead of the for-loop to copy priomap

[Srivatsa S. Bhat]

Replace the current (inefficient) for-loop with memcpy, to copy priomap.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 net/core/netprio_cgroup.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index fd339bb0..83bbd0e 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -73,7 +73,6 @@ static int extend_netdev_table(struct net_device *dev, u32 new_len)
 			   ((sizeof(u32) * new_len));
 	struct netprio_map *new_priomap = kzalloc(new_size, GFP_KERNEL);
 	struct netprio_map *old_priomap;
-	int i;
 
 	old_priomap  = rtnl_dereference(dev->priomap);
 
@@ -82,10 +81,10 @@ static int extend_netdev_table(struct net_device *dev, u32 new_len)
 		return -ENOMEM;
 	}
 
-	for (i = 0;
-	     old_priomap && (i < old_priomap->priomap_len);
-	     i++)
-		new_priomap->priomap[i] = old_priomap->priomap[i];
+	if (old_priomap)
+		memcpy(new_priomap->priomap, old_priomap->priomap,
+			old_priomap->priomap_len *
+					sizeof(old_priomap->priomap[0]));
 
 	new_priomap->priomap_len = new_len;
 

Re: [v2 PATCH 2/2] netprio_cgroup: Use memcpy instead of the for-loop to copy priomap

[David Miller]

From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
Date: Wed, 12 Sep 2012 11:37:47 +0530

> +		memcpy(new_priomap->priomap, old_priomap->priomap,
> +			old_priomap->priomap_len *
> +					sizeof(old_priomap->priomap[0]));

This argument indentation is ridiculous.  Try:

		memcpy(new_priomap->priomap, old_priomap->priomap,
		       old_priomap->priomap_len *
		       sizeof(old_priomap->priomap[0]));

Using TABs exclusively for argumentat indentation is not the goal.

Rather, lining the arguments up properly so that they sit at the first
column after the first line's openning parenthesis is what you should
be trying to achieve.

And ignoring whatever stylistic convention we may or may not have, I
find it impossibly hard to believe that the code quoted above looks
good even to you.

Re: [v2 PATCH 2/2] netprio_cgroup: Use memcpy instead of the for-loop to copy priomap

[Srivatsa S. Bhat]

On 09/12/2012 01:19 PM, David Miller wrote:
> From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> Date: Wed, 12 Sep 2012 11:37:47 +0530
> 
>> +		memcpy(new_priomap->priomap, old_priomap->priomap,
>> +			old_priomap->priomap_len *
>> +					sizeof(old_priomap->priomap[0]));
> 
> This argument indentation is ridiculous.  Try:
> 
> 		memcpy(new_priomap->priomap, old_priomap->priomap,
> 		       old_priomap->priomap_len *
> 		       sizeof(old_priomap->priomap[0]));
> 
> Using TABs exclusively for argumentat indentation is not the goal.
> 
> Rather, lining the arguments up properly so that they sit at the first
> column after the first line's openning parenthesis is what you should
> be trying to achieve.

OK, will fix it, thanks!

> 
> And ignoring whatever stylistic convention we may or may not have, I
> find it impossibly hard to believe that the code quoted above looks
> good even to you.
>

On second thoughts, I think the memcpy in this case will actually be worse
since it will copy the contents in chunks of smaller size than the for-loop.

Or, did you mean to say that this code is plain wrong for some reason?

Regards,
Srivatsa S. Bhat

Re: [v2 PATCH 2/2] netprio_cgroup: Use memcpy instead of the for-loop to copy priomap

[Srivatsa S. Bhat]

On 09/12/2012 01:54 PM, Srivatsa S. Bhat wrote:
> On 09/12/2012 01:19 PM, David Miller wrote:
>> From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>> Date: Wed, 12 Sep 2012 11:37:47 +0530
>>
>>> +		memcpy(new_priomap->priomap, old_priomap->priomap,
>>> +			old_priomap->priomap_len *
>>> +					sizeof(old_priomap->priomap[0]));
>>
>> This argument indentation is ridiculous.  Try:
>>
>> 		memcpy(new_priomap->priomap, old_priomap->priomap,
>> 		       old_priomap->priomap_len *
>> 		       sizeof(old_priomap->priomap[0]));
>>
>> Using TABs exclusively for argumentat indentation is not the goal.
>>
>> Rather, lining the arguments up properly so that they sit at the first
>> column after the first line's openning parenthesis is what you should
>> be trying to achieve.
> 
> OK, will fix it, thanks!
> 
>>
>> And ignoring whatever stylistic convention we may or may not have, I
>> find it impossibly hard to believe that the code quoted above looks
>> good even to you.
>>
> 
> On second thoughts, I think the memcpy in this case will actually be worse
> since it will copy the contents in chunks of smaller size than the for-loop.

Oops, I missed the __HAVE_ARCH_MEMCPY and was looking at the wrong memcpy
implementation.. And in any case, I went totally off-track by your last comment.
I hadn't realized that you were still referring to the way the code looks, rather
than questioning the switch to memcpy. Sorry about that!

I'll fix the odd-looking indentation and repost the patch.
 
Regards,
Srivatsa S. Bhat

Re: [v2 PATCH 2/2] netprio_cgroup: Use memcpy instead of the for-loop to copy priomap

[David Miller]

From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
Date: Wed, 12 Sep 2012 13:54:05 +0530

> On 09/12/2012 01:19 PM, David Miller wrote:
>> From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>> Date: Wed, 12 Sep 2012 11:37:47 +0530
>> 
>>> +		memcpy(new_priomap->priomap, old_priomap->priomap,
>>> +			old_priomap->priomap_len *
>>> +					sizeof(old_priomap->priomap[0]));
>> 
>> This argument indentation is ridiculous.  Try:
>> 
>> 		memcpy(new_priomap->priomap, old_priomap->priomap,
>> 		       old_priomap->priomap_len *
>> 		       sizeof(old_priomap->priomap[0]));
>> 
>> Using TABs exclusively for argumentat indentation is not the goal.
>> 
>> Rather, lining the arguments up properly so that they sit at the first
>> column after the first line's openning parenthesis is what you should
>> be trying to achieve.
> 
> OK, will fix it, thanks!
> 
>> 
>> And ignoring whatever stylistic convention we may or may not have, I
>> find it impossibly hard to believe that the code quoted above looks
>> good even to you.
>>
> 
> On second thoughts, I think the memcpy in this case will actually be worse
> since it will copy the contents in chunks of smaller size than the for-loop.

I meant just coding style wise, not semantically.