net: fix functions and variables related to netns_ipvs->sysctl_sync_qlen_max

Since the type of netns_ipvs->sysctl_sync_qlen_max has been changed to unsigned long, type of its related proc var sync_qlen_max should be changed to unsigned long, too. Also the return type of function sysctl_sync_qlen_max().

Besides, the type of ipvs_master_sync_state->sync_queue_len should also be changed to unsigned long.

Changelog from V1:
- change type of ipvs_master_sync_state->sync_queue_len to unsigned long as Simon addressed.

Cc Andrew Morton <[email protected]>

Cc David Miller <[email protected]>

Cc Julian Anastasov <[email protected]>

Cc Simon Horman <[email protected]>

Signed-off-by Zhang Yanfei <[email protected]>

---
 include/net/ip_vs.h            |    6 +++---
 net/netfilter/ipvs/ip_vs_ctl.c |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

include/net/ip_vs.h +3 -3

index 68c69d5..1d56f92 100644
@@ -874,7 +874,7 @@ struct ip_vs_app {
 struct ipvs_master_sync_state {
 	struct list_head	sync_queue;
 	struct ip_vs_sync_buff	*sync_buff;
-	int			sync_queue_len;
+	unsigned long		sync_queue_len;
 	unsigned int		sync_queue_delay;
 	struct task_struct	*master_thread;
 	struct delayed_work	master_wakeup_work;

@@ -1052,7 +1052,7 @@ static inline int sysctl_sync_ports(struct netns_ipvs *ipvs)
 	return ACCESS_ONCE(ipvs->sysctl_sync_ports);
 }
 
-static inline int sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
+static inline unsigned long sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
 {
 	return ipvs->sysctl_sync_qlen_max;
 }

@@ -1099,7 +1099,7 @@ static inline int sysctl_sync_ports(struct netns_ipvs *ipvs)
 	return 1;
 }
 
-static inline int sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
+static inline unsigned long sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
 {
 	return IPVS_SYNC_QLEN_MAX;
 }

net/netfilter/ipvs/ip_vs_ctl.c +2 -2

index ec664cb..d79a530 100644
@@ -1747,9 +1747,9 @@ static struct ctl_table vs_vars[] = {
 	},
 	{
 		.procname	= "sync_qlen_max",
-		.maxlen		= sizeof(int),
+		.maxlen		= sizeof(unsigned long),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_doulongvec_minmax,
 	},
 	{
 		.procname	= "sync_sock_size",

Signature · 1 line

1.7.1

Since the type of netns_ipvs->sysctl_sync_qlen_max has been changed to unsigned long, type of its related proc var sync_qlen_max should be changed to unsigned long, too. Also the return type of function sysctl_sync_qlen_max().

Besides, the type of ipvs_master_sync_state->sync_queue_len should also be changed to unsigned long.

Changelog from V1:
- change type of ipvs_master_sync_state->sync_queue_len to unsigned long as Simon addressed.

Cc Andrew Morton <[email protected]>

Cc David Miller <[email protected]>

Cc Julian Anastasov <[email protected]>

Cc Simon Horman <[email protected]>

Signed-off-by Zhang Yanfei <[email protected]>

---
 include/net/ip_vs.h            |    6 +++---
 net/netfilter/ipvs/ip_vs_ctl.c |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

include/net/ip_vs.h +3 -3

index 68c69d5..1d56f92 100644
@@ -874,7 +874,7 @@ struct ip_vs_app {
 struct ipvs_master_sync_state {
 	struct list_head	sync_queue;
 	struct ip_vs_sync_buff	*sync_buff;
-	int			sync_queue_len;
+	unsigned long		sync_queue_len;
 	unsigned int		sync_queue_delay;
 	struct task_struct	*master_thread;
 	struct delayed_work	master_wakeup_work;

@@ -1052,7 +1052,7 @@ static inline int sysctl_sync_ports(struct netns_ipvs *ipvs)
 	return ACCESS_ONCE(ipvs->sysctl_sync_ports);
 }
 
-static inline int sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
+static inline unsigned long sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
 {
 	return ipvs->sysctl_sync_qlen_max;
 }

@@ -1099,7 +1099,7 @@ static inline int sysctl_sync_ports(struct netns_ipvs *ipvs)
 	return 1;
 }
 
-static inline int sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
+static inline unsigned long sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
 {
 	return IPVS_SYNC_QLEN_MAX;
 }

net/netfilter/ipvs/ip_vs_ctl.c +2 -2

index ec664cb..d79a530 100644
@@ -1747,9 +1747,9 @@ static struct ctl_table vs_vars[] = {
 	},
 	{
 		.procname	= "sync_qlen_max",
-		.maxlen		= sizeof(int),
+		.maxlen		= sizeof(unsigned long),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_doulongvec_minmax,
 	},
 	{
 		.procname	= "sync_sock_size",

Signature · 15 lines

1.7.1


Another question about the sysctl_sync_qlen_max:
This variable is assigned as:

ipvs->sysctl_sync_qlen_max = nr_free_buffer_pages() / 32;

The function nr_free_buffer_pages actually means: counts of pages
which are beyond high watermark within ZONE_DMA and ZONE_NORMAL.

is it ok to be called here? Some people misused this function because
the function name was misleading them. I am sorry I am totally not
familiar with the ipvs code, so I am just asking you about
this.

Hello,

Quoted 6 lines · Zhang Yanfei

Since the type of netns_ipvs->sysctl_sync_qlen_max has been changed to unsigned long, type of its related proc var sync_qlen_max should be changed to unsigned long, too. Also the return type of function sysctl_sync_qlen_max().

Besides, the type of ipvs_master_sync_state->sync_queue_len should also be changed to unsigned long.

v2 looks fine. Thanks! Regarding your question
see below...

Quoted 77 lines

Changelog from V1:
- change type of ipvs_master_sync_state->sync_queue_len to unsigned long as Simon addressed.

Cc Andrew Morton <[email protected]>

Cc David Miller <[email protected]>

Cc Julian Anastasov <[email protected]>

Cc Simon Horman <[email protected]>

Signed-off-by Zhang Yanfei <[email protected]>

---
 include/net/ip_vs.h            |    6 +++---
 net/netfilter/ipvs/ip_vs_ctl.c |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

include/net/ip_vs.h +3 -3

index 68c69d5..1d56f92 100644
@@ -874,7 +874,7 @@ struct ip_vs_app {
 struct ipvs_master_sync_state {
 	struct list_head	sync_queue;
 	struct ip_vs_sync_buff	*sync_buff;
-	int			sync_queue_len;
+	unsigned long		sync_queue_len;
 	unsigned int		sync_queue_delay;
 	struct task_struct	*master_thread;
 	struct delayed_work	master_wakeup_work;

@@ -1052,7 +1052,7 @@ static inline int sysctl_sync_ports(struct netns_ipvs *ipvs)
 	return ACCESS_ONCE(ipvs->sysctl_sync_ports);
 }
 
-static inline int sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
+static inline unsigned long sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
 {
 	return ipvs->sysctl_sync_qlen_max;
 }

@@ -1099,7 +1099,7 @@ static inline int sysctl_sync_ports(struct netns_ipvs *ipvs)
 	return 1;
 }
 
-static inline int sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
+static inline unsigned long sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
 {
 	return IPVS_SYNC_QLEN_MAX;
 }

net/netfilter/ipvs/ip_vs_ctl.c +2 -2

index ec664cb..d79a530 100644
@@ -1747,9 +1747,9 @@ static struct ctl_table vs_vars[] = {
 	},
 	{
 		.procname	= "sync_qlen_max",
-		.maxlen		= sizeof(int),
+		.maxlen		= sizeof(unsigned long),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_doulongvec_minmax,
 	},
 	{
 		.procname	= "sync_sock_size",

Signature · 15 lines

1.7.1


Another question about the sysctl_sync_qlen_max:
This variable is assigned as:

ipvs->sysctl_sync_qlen_max = nr_free_buffer_pages() / 32;

The function nr_free_buffer_pages actually means: counts of pages
which are beyond high watermark within ZONE_DMA and ZONE_NORMAL.

is it ok to be called here? Some people misused this function because
the function name was misleading them. I am sorry I am totally not
familiar with the ipvs code, so I am just asking you about
this.

Using nr_free_buffer_pages should be fine here.
We are using it as rough estimation for the number of sync buffers we can use in NORMAL zones. We are using dev->mtu for such buffers, so it can take a PAGE_SIZE for a buffer. We are not interested in HIGHMEM size. high watermarks should have negliable effect. I'm even not sure whether we need to clamp it for systems with TBs of memory.

Quoted 5 lines

Another question about the sysctl_sync_qlen_max:
This variable is assigned as:

ipvs->sysctl_sync_qlen_max = nr_free_buffer_pages() / 32;

Quoted 7 lines

The function nr_free_buffer_pages actually means: counts of pages which are beyond high watermark within ZONE_DMA and ZONE_NORMAL.

is it ok to be called here? Some people misused this function because the function name was misleading them. I am sorry I am totally not familiar with the ipvs code, so I am just asking you about this.

Using nr_free_buffer_pages should be fine here.
We are using it as rough estimation for the number of sync buffers we can use in NORMAL zones. We are using dev->mtu for such buffers, so it can take a PAGE_SIZE for a buffer. We are not interested in HIGHMEM size. high watermarks should have negliable effect. I'm even not sure whether we need to clamp it for systems with TBs of memory.

Quoted 13 lines · On Thu, 7 Feb 2013 10:40:26 +0200 (EET) Julian Anastasov <[email protected]> wrote:

Quoted 5 lines

Another question about the sysctl_sync_qlen_max:
This variable is assigned as:

ipvs->sysctl_sync_qlen_max = nr_free_buffer_pages() / 32;

Quoted 7 lines

The function nr_free_buffer_pages actually means: counts of pages which are beyond high watermark within ZONE_DMA and ZONE_NORMAL.

is it ok to be called here? Some people misused this function because the function name was misleading them. I am sorry I am totally not familiar with the ipvs code, so I am just asking you about this.

Quoted 7 lines

Using nr_free_buffer_pages should be fine here.
We are using it as rough estimation for the number of sync buffers we can use in NORMAL zones. We are using dev->mtu for such buffers, so it can take a PAGE_SIZE for a buffer. We are not interested in HIGHMEM size. high watermarks should have negliable effect. I'm even not sure whether we need to clamp it for systems with TBs of memory.

Using nr_free_buffer_pages() is good-enough-for-now. There are questions around the name of this thing and its exact functionality and whether callers are using it appropriately. But if anything is changed there, it will be as part of kernel-wide sweep.

One thing to bear in mind is memory hot[un]plug. Anything which was sized using nr_free_buffer_pages() (or similar) may become inappropriately sized if memory is added or removed. So any site which uses nr_free_buffer_pages() really should be associated with a hotplug handler and a great pile of code to resize the structure at runtime. It's pretty ugly stuff :( I suspect it usually Just Doesn't Matter.

Redarding this patch:
net-change-type-of-netns_ipvs-sysctl_sync_qlen_max.patch and net-fix-functions-and-variables-related-to-netns_ipvs-sysctl_sync_qlen_max.patch are joined at the hip and should be redone as a single patch with a suitable changelog, please. And with a cc:[email protected].

Hello,

Quoted 29 lines · Andrew Morton

Quoted 13 lines · On Thu, 7 Feb 2013 10:40:26 +0200 (EET) Julian Anastasov <[email protected]> wrote:

Quoted 5 lines

Another question about the sysctl_sync_qlen_max:
This variable is assigned as:

ipvs->sysctl_sync_qlen_max = nr_free_buffer_pages() / 32;

Quoted 7 lines

The function nr_free_buffer_pages actually means: counts of pages which are beyond high watermark within ZONE_DMA and ZONE_NORMAL.

is it ok to be called here? Some people misused this function because the function name was misleading them. I am sorry I am totally not familiar with the ipvs code, so I am just asking you about this.

Quoted 7 lines

Using nr_free_buffer_pages should be fine here.
We are using it as rough estimation for the number of sync buffers we can use in NORMAL zones. We are using dev->mtu for such buffers, so it can take a PAGE_SIZE for a buffer. We are not interested in HIGHMEM size. high watermarks should have negliable effect. I'm even not sure whether we need to clamp it for systems with TBs of memory.

Using nr_free_buffer_pages() is good-enough-for-now. There are questions around the name of this thing and its exact functionality and whether callers are using it appropriately. But if anything is changed there, it will be as part of kernel-wide sweep.

Quoted 6 lines

One thing to bear in mind is memory hot[un]plug. Anything which was sized using nr_free_buffer_pages() (or similar) may become inappropriately sized if memory is added or removed. So any site which uses nr_free_buffer_pages() really should be associated with a hotplug handler and a great pile of code to resize the structure at runtime. It's pretty ugly stuff :( I suspect it usually Just Doesn't Matter.

I'll try to think on this hotplug problem
and also on the si_meminfo usage in net/netfilter/ipvs/ip_vs_ctl.c which is quite wrong for systems with HIGHMEM, may be we need there a free+reclaimable+file function for NORMAL zone.

Quoted 5 lines

Redarding this patch:
net-change-type-of-netns_ipvs-sysctl_sync_qlen_max.patch and net-fix-functions-and-variables-related-to-netns_ipvs-sysctl_sync_qlen_max.patch are joined at the hip and should be redone as a single patch with a suitable changelog, please. And with a cc:[email protected].

Agreed, Zhang Yanfei and Simon? I'm just not sure,
may be this combined patch should hit only the ipvs->nf->net trees? Or may be net-next, if we don't have time for 3.8.

Hello,

Quoted 29 lines · Andrew Morton

Quoted 13 lines · On Thu, 7 Feb 2013 10:40:26 +0200 (EET) Julian Anastasov <[email protected]> wrote:

Quoted 5 lines

Another question about the sysctl_sync_qlen_max:
This variable is assigned as:

ipvs->sysctl_sync_qlen_max = nr_free_buffer_pages() / 32;

Quoted 7 lines

The function nr_free_buffer_pages actually means: counts of pages which are beyond high watermark within ZONE_DMA and ZONE_NORMAL.

is it ok to be called here? Some people misused this function because the function name was misleading them. I am sorry I am totally not familiar with the ipvs code, so I am just asking you about this.

Quoted 7 lines

Using nr_free_buffer_pages should be fine here.
We are using it as rough estimation for the number of sync buffers we can use in NORMAL zones. We are using dev->mtu for such buffers, so it can take a PAGE_SIZE for a buffer. We are not interested in HIGHMEM size. high watermarks should have negliable effect. I'm even not sure whether we need to clamp it for systems with TBs of memory.

Using nr_free_buffer_pages() is good-enough-for-now. There are questions around the name of this thing and its exact functionality and whether callers are using it appropriately. But if anything is changed there, it will be as part of kernel-wide sweep.

Quoted 6 lines

One thing to bear in mind is memory hot[un]plug. Anything which was sized using nr_free_buffer_pages() (or similar) may become inappropriately sized if memory is added or removed. So any site which uses nr_free_buffer_pages() really should be associated with a hotplug handler and a great pile of code to resize the structure at runtime. It's pretty ugly stuff :( I suspect it usually Just Doesn't Matter.

I'll try to think on this hotplug problem
and also on the si_meminfo usage in net/netfilter/ipvs/ip_vs_ctl.c which is quite wrong for systems with HIGHMEM, may be we need there a free+reclaimable+file function for NORMAL zone.

Quoted 5 lines

Redarding this patch:
net-change-type-of-netns_ipvs-sysctl_sync_qlen_max.patch and net-fix-functions-and-variables-related-to-netns_ipvs-sysctl_sync_qlen_max.patch are joined at the hip and should be redone as a single patch with a suitable changelog, please. And with a cc:[email protected].

Agreed, Zhang Yanfei and Simon? I'm just not sure,
may be this combined patch should hit only the ipvs->nf->net trees? Or may be net-next, if we don't have time for 3.8.

ao? 2013a1'02ae??16ae?JPY 05:39, Julian Anastasov a??e??:

Quoted 11 lines

Hello,

Quoted 5 lines · Andrew Morton

Redarding this patch:
net-change-type-of-netns_ipvs-sysctl_sync_qlen_max.patch and net-fix-functions-and-variables-related-to-netns_ipvs-sysctl_sync_qlen_max.patch are joined at the hip and should be redone as a single patch with a suitable changelog, please. And with a cc:[email protected].

Quoted 5 lines

Agreed, Zhang Yanfei and Simon? I'm just not sure,
may be this combined patch should hit only the ipvs->nf->net trees? Or may be net-next, if we don't have time for 3.8.

Should I resend the combined patch?