[PATCH 1/3] netlink: use the macro min(x,y) provided by <linux/kernel.h> instead

[PATCH 1/3] netlink: use the macro min(x,y) provided by <linux/kernel.h> instead

[Denis Cheng]

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 net/netlink/af_netlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 5681ce3..8bb14e3 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1905,7 +1905,7 @@ static int __init netlink_proto_init(void)
 
 	order = get_bitmask_order(max) - 1 + PAGE_SHIFT;
 	max = (1UL << order) / sizeof(struct hlist_head);
-	order = get_bitmask_order(max > UINT_MAX ? UINT_MAX : max) - 1;
+	order = get_bitmask_order(min(max, (unsigned long)UINT_MAX)) - 1;
 
 	for (i = 0; i < MAX_LINKS; i++) {
 		struct nl_pid_hash *hash = &nl_table[i].hash;
-- 
1.5.3.rc7

[PATCH 2/3] netlink: the temp variable name max is ambiguous

[Denis Cheng]

with the macro max provided by <linux/kernel.h>, so changed its name to a more proper one: limit

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 net/netlink/af_netlink.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 8bb14e3..ac3b100 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1885,7 +1885,7 @@ static int __init netlink_proto_init(void)
 {
 	struct sk_buff *dummy_skb;
 	int i;
-	unsigned long max;
+	unsigned long limit;
 	unsigned int order;
 	int err = proto_register(&netlink_proto, 0);
 
@@ -1899,13 +1899,13 @@ static int __init netlink_proto_init(void)
 		goto panic;
 
 	if (num_physpages >= (128 * 1024))
-		max = num_physpages >> (21 - PAGE_SHIFT);
+		limit = num_physpages >> (21 - PAGE_SHIFT);
 	else
-		max = num_physpages >> (23 - PAGE_SHIFT);
+		limit = num_physpages >> (23 - PAGE_SHIFT);
 
-	order = get_bitmask_order(max) - 1 + PAGE_SHIFT;
-	max = (1UL << order) / sizeof(struct hlist_head);
-	order = get_bitmask_order(min(max, (unsigned long)UINT_MAX)) - 1;
+	order = get_bitmask_order(limit) - 1 + PAGE_SHIFT;
+	limit = (1UL << order) / sizeof(struct hlist_head);
+	order = get_bitmask_order(min(limit, (unsigned long)UINT_MAX)) - 1;
 
 	for (i = 0; i < MAX_LINKS; i++) {
 		struct nl_pid_hash *hash = &nl_table[i].hash;
-- 
1.5.3.rc7

[PATCH 3/3] netlink: use a statically allocated nl_table instead

[Denis Cheng]

if the table is always fixed size with MAX_LINKS entries, why not use a statically
allocated table straightforwardly?

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 net/netlink/af_netlink.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index ac3b100..c527f87 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -115,7 +115,7 @@ struct netlink_table {
 	int registered;
 };
 
-static struct netlink_table *nl_table;
+static struct netlink_table nl_table[MAX_LINKS];
 
 static DECLARE_WAIT_QUEUE_HEAD(nl_table_wait);
 
@@ -1894,10 +1894,6 @@ static int __init netlink_proto_init(void)
 
 	BUILD_BUG_ON(sizeof(struct netlink_skb_parms) > sizeof(dummy_skb->cb));
 
-	nl_table = kcalloc(MAX_LINKS, sizeof(*nl_table), GFP_KERNEL);
-	if (!nl_table)
-		goto panic;
-
 	if (num_physpages >= (128 * 1024))
 		limit = num_physpages >> (21 - PAGE_SHIFT);
 	else
@@ -1915,8 +1911,7 @@ static int __init netlink_proto_init(void)
 			while (i-- > 0)
 				nl_pid_hash_free(nl_table[i].hash.table,
 						 1 * sizeof(*hash->table));
-			kfree(nl_table);
-			goto panic;
+			goto out;
 		}
 		memset(hash->table, 0, 1 * sizeof(*hash->table));
 		hash->max_shift = order;
@@ -1933,8 +1928,6 @@ static int __init netlink_proto_init(void)
 	rtnetlink_init();
 out:
 	return err;
-panic:
-	panic("netlink_init: Cannot allocate nl_table\n");
 }
 
 core_initcall(netlink_proto_init);
-- 
1.5.3.rc7

Re: [PATCH 1/3] netlink: use the macro min(x,y) provided by <linux/kernel.h> instead

[rae l]

On 9/2/07, David Newall <david@davidnewall.com> wrote:
> Denis Cheng wrote
> > +     order = get_bitmask_order(min(max, (unsigned long)UINT_MAX)) - 1;
> >
>
> Why doesn't this clash with the max define, also in linux/kernel.h?
They indeed don't clash,
the cpp included by gcc is intelligent enough, it know the
function-style definition of max in kernel.h, that's different from
the auto variable max here, so they don't clash with each other,

But I think the variable name "max" here is ambiguous, I changed it to
"limit", see my following patch [PATCH 2/3].

-- 
Denis Cheng

Re: [PATCH 1/3] netlink: use the macro min(x,y) provided by <linux/kernel.h> instead

[David Miller]

From: Denis Cheng <crquan@gmail.com>
Date: Sun,  2 Sep 2007 03:45:57 +0800

> Signed-off-by: Denis Cheng <crquan@gmail.com>

Applied to net-2.6.24, thanks.

Re: [PATCH 2/3] netlink: the temp variable name max is ambiguous

[David Miller]

From: Denis Cheng <crquan@gmail.com>
Date: Sun,  2 Sep 2007 03:45:58 +0800

> with the macro max provided by <linux/kernel.h>, so changed its name to a more proper one: limit
> 
> Signed-off-by: Denis Cheng <crquan@gmail.com>

Not strictly necessary because CPP knows to differentiate between
'max(' and plain 'max' when evaluating if a CPP macro should be
expanded or not.

Nonetheless, applied to net-2.6.24, thanks.

Re: [PATCH 3/3] netlink: use a statically allocated nl_table instead

[David Miller]

From: Denis Cheng <crquan@gmail.com>
Date: Sun,  2 Sep 2007 03:45:59 +0800

> if the table is always fixed size with MAX_LINKS entries, why not use a statically
> allocated table straightforwardly?
> 
> Signed-off-by: Denis Cheng <crquan@gmail.com>

I made the explicit decision to dynamically allocate because
many systems have limits on how large the kernel image can
be and therefore the less we statically allocate huge tables
(constant size or not) the better.

Lockdep is the worst offender, for example, it's completely awful.  It
consumes 4MB of kernel BSS space when enabled on a 64-bit platform.

Patch not applied.

Re: [PATCH 2/3] netlink: the temp variable name max is ambiguous

[rae l]

On 9/17/07, David Miller <davem@davemloft.net> wrote:
> From: Denis Cheng <crquan@gmail.com>
> Date: Sun,  2 Sep 2007 03:45:58 +0800
>
> > with the macro max provided by <linux/kernel.h>, so changed its name to a more proper one: limit
> >
> > Signed-off-by: Denis Cheng <crquan@gmail.com>
>
> Not strictly necessary because CPP knows to differentiate between
> 'max(' and plain 'max' when evaluating if a CPP macro should be
> expanded or not.
I also know the GNU CPP is intelligent, but people are often not.
I just think the avoidance to use human ambiguous names could give
more readability.

>
> Nonetheless, applied to net-2.6.24, thanks.
>

-- 
Denis Cheng