Re: Question about ip_defrag

14 messages, 3 authors, 2017-08-30 · open the first message on its own page

Re: Question about ip_defrag

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-24 18:59:38

On Thu, 24 Aug 2017 16:04:41 +0000 "liujian (CE)" [off-list ref] wrote:
quoted
What kernel version have you seen this issue with?  
3.10,with some backport.

 >As far as I remember, this issue have been fixed before...  

which one patch? I didnot find out the patch:(
AFAIK it was some bugs in the percpu_counter code.  If you need to
backport look at the git commits:

 git log lib/percpu_counter.c include/linux/percpu_counter.h

Are you maintaining your own 3.10 kernel?

I know that for RHEL7 (also kernel 3.10) we backported the
percpu_counter fixes...

--Jesper

发件人: Jesper Dangaard Brouer
收件人: liujian (CE)<liujian56@huawei.com<mailto:liujian56@huawei.com>>
抄送: davem@davemloft.net<mailto:davem@davemloft.net>;kuznet@ms2.inr.ac.ru<mailto:kuznet@ms2.inr.ac.ru>;yoshfuji@linux-ipv6.org<mailto:yoshfuji@linux-ipv6.org>;elena.reshetova@intel.com<mailto:elena.reshetova@intel.com>;edumazet@google.com<mailto:edumazet@google.com>;netdev@vger.kernel.org<mailto:netdev@vger.kernel.org>;brouer@redhat.com<mailto:brouer@redhat.com>
主题: Re: Question about ip_defrag
时间: 2017-08-24 21:53:17


On Thu, 24 Aug 2017 13:15:33 +0000 "liujian (CE)" [off-list ref] wrote:
quoted
Hello,

With below patch we met one issue.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.13-rc6&id=6d7b857d541e

the issue:
Ip_defrag fail caused by frag_mem_limit reached 4M(frags.high_thresh).
At this moment,sum_frag_mem_limit is about 10K.
and my test machine's cpu num is 64.

Can i only change frag_mem_limit to sum_ frag_mem_limit?

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 96e95e8..f09c00b 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -120,7 +120,7 @@ static void inet_frag_secret_rebuild(struct inet_frags *f)
 static bool inet_fragq_should_evict(const struct inet_frag_queue *q)
 {
        return q->net->low_thresh == 0 ||
-              frag_mem_limit(q->net) >= q->net->low_thresh;
+              sum_frag_mem_limit(q->net) >= q->net->low_thresh;
 }

 static unsigned int
@@ -355,7 +355,7 @@ static struct inet_frag_queue *inet_frag_alloc(struct netns_frags *nf,
 {
        struct inet_frag_queue *q;

-       if (!nf->high_thresh || frag_mem_limit(nf) > nf->high_thresh) {
+       if (!nf->high_thresh || sum_frag_mem_limit(nf) > nf->high_thresh) {
                inet_frag_schedule_worker(f);
                return NULL;
        }
@@ -396,7 +396,7 @@ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf,
        struct inet_frag_queue *q;
        int depth = 0;

-       if (frag_mem_limit(nf) > nf->low_thresh)
+       if (sum_frag_mem_limit(nf) > nf->low_thresh)
                inet_frag_schedule_worker(f);

        hash &= (INETFRAGS_HASHSZ - 1);
--
Thank you for your time.  
What kernel version have you seen this issue with?

As far as I remember, this issue have been fixed before...

--
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

RE: Question about ip_defrag

From: liujian (CE) <hidden>
Date: 2017-08-25 01:33:28

-----Original Message-----
From: Jesper Dangaard Brouer [mailto:brouer@redhat.com]
Sent: Friday, August 25, 2017 2:59 AM
To: liujian (CE)
Cc: davem@davemloft.net; kuznet@ms2.inr.ac.ru; yoshfuji@linux-ipv6.org;
elena.reshetova@intel.com; edumazet@google.com; netdev@vger.kernel.org;
brouer@redhat.com
Subject: Re: Question about ip_defrag


On Thu, 24 Aug 2017 16:04:41 +0000 "liujian (CE)" [off-list ref]
wrote:
quoted
quoted
What kernel version have you seen this issue with?
3.10,with some backport.

 >As far as I remember, this issue have been fixed before...

which one patch? I didnot find out the patch:(
AFAIK it was some bugs in the percpu_counter code.  If you need to backport
look at the git commits:

 git log lib/percpu_counter.c include/linux/percpu_counter.h

Are you maintaining your own 3.10 kernel?

I know that for RHEL7 (also kernel 3.10) we backported the percpu_counter
fixes...
Could you tell me which one patch?  we have backported most of the two files's change. 
Thank you ~

--Jesper

quoted
发件人: Jesper Dangaard Brouer
收件人: liujian
(CE)<liujian56@huawei.com<mailto:liujian56@huawei.com>>
quoted
抄送:
davem@davemloft.net<mailto:davem@davemloft.net>;kuznet@ms2.inr.ac.ru
<m
quoted
ailto:kuznet@ms2.inr.ac.ru>;yoshfuji@linux-ipv6.org<mailto:yoshfuji@li
nux-ipv6.org>;elena.reshetova@intel.com<mailto:elena.reshetova@intel.c
om>;edumazet@google.com<mailto:edumazet@google.com>;netdev@vger.k
ernel
quoted
.org<mailto:netdev@vger.kernel.org>;brouer@redhat.com<mailto:brouer@r
e
quoted
dhat.com>
主题: Re: Question about ip_defrag
时间: 2017-08-24 21:53:17


On Thu, 24 Aug 2017 13:15:33 +0000 "liujian (CE)" [off-list ref]
wrote:
quoted
quoted
Hello,

With below patch we met one issue.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/c
ommit/?h=v4.13-rc6&id=6d7b857d541e

the issue:
Ip_defrag fail caused by frag_mem_limit reached 4M(frags.high_thresh).
At this moment,sum_frag_mem_limit is about 10K.
and my test machine's cpu num is 64.

Can i only change frag_mem_limit to sum_ frag_mem_limit?

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 96e95e8..f09c00b 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -120,7 +120,7 @@ static void inet_frag_secret_rebuild(struct
inet_frags *f)  static bool inet_fragq_should_evict(const struct
inet_frag_queue *q)  {
        return q->net->low_thresh == 0 ||
-              frag_mem_limit(q->net) >= q->net->low_thresh;
+              sum_frag_mem_limit(q->net) >= q->net->low_thresh;
 }

 static unsigned int
@@ -355,7 +355,7 @@ static struct inet_frag_queue
*inet_frag_alloc(struct netns_frags *nf,  {
        struct inet_frag_queue *q;

-       if (!nf->high_thresh || frag_mem_limit(nf) > nf->high_thresh) {
+       if (!nf->high_thresh || sum_frag_mem_limit(nf) >
+ nf->high_thresh) {
                inet_frag_schedule_worker(f);
                return NULL;
        }
@@ -396,7 +396,7 @@ struct inet_frag_queue *inet_frag_find(struct
netns_frags *nf,
quoted
quoted
        struct inet_frag_queue *q;
        int depth = 0;

-       if (frag_mem_limit(nf) > nf->low_thresh)
+       if (sum_frag_mem_limit(nf) > nf->low_thresh)
                inet_frag_schedule_worker(f);

        hash &= (INETFRAGS_HASHSZ - 1);
--

Thank you for your time.
What kernel version have you seen this issue with?

As far as I remember, this issue have been fixed before...

--
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


--
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

RE: Question about ip_defrag

From: liujian (CE) <hidden>
Date: 2017-08-28 08:12:05

Hi

I checked our 3.10 kernel, we had backported all percpu_counter bug fix in lib/percpu_counter.c and include/linux/percpu_counter.h.
And I check 4.13-rc6, also has the issue if NIC's rx cpu num big enough.
quoted
quoted
quoted
the issue:
Ip_defrag fail caused by frag_mem_limit reached 4M(frags.high_thresh).
At this moment,sum_frag_mem_limit is about 10K.
So should we change ipfrag high/low thresh to a reasonable value ? 
And if it is, is there a standard to change the value?


root@RH8100-V3:/proc/net# cat sockstat
sockets: used 1485
TCP: inuse 4 orphan 0 tw 0 alloc 5 mem 1
UDP: inuse 203 mem 201
UDPLITE: inuse 0
RAW: inuse 0
FRAG: inuse 1 memory 16048, 3156696.
root@RH8100-V3:/proc/net#

In order to print frag_mem_limit, change the code as below:
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 43eb6567..38bfb20 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -73,7 +73,7 @@ static int sockstat_seq_show(struct seq_file *seq, void *v)
        seq_printf(seq, "RAW: inuse %d\n",
                   sock_prot_inuse_get(net, &raw_prot));
        frag_mem = ip_frag_mem(net);
-       seq_printf(seq,  "FRAG: inuse %u memory %u\n", !!frag_mem, frag_mem);
+       seq_printf(seq,  "FRAG: inuse %u memory %u, %u.\n", !!frag_mem, frag_mem, frag_mem_limit(&net->ipv4.frags));
        return 0;
 }
Best Regards,
liujian

-----Original Message-----
From: liujian (CE)
Sent: Friday, August 25, 2017 9:33 AM
To: 'Jesper Dangaard Brouer'
Cc: davem@davemloft.net; kuznet@ms2.inr.ac.ru; yoshfuji@linux-ipv6.org;
elena.reshetova@intel.com; edumazet@google.com; netdev@vger.kernel.org;
Wangkefeng (Kevin); weiyongjun (A)
Subject: RE: Question about ip_defrag

quoted
-----Original Message-----
From: Jesper Dangaard Brouer [mailto:brouer@redhat.com]
Sent: Friday, August 25, 2017 2:59 AM
To: liujian (CE)
Cc: davem@davemloft.net; kuznet@ms2.inr.ac.ru;
yoshfuji@linux-ipv6.org; elena.reshetova@intel.com;
edumazet@google.com; netdev@vger.kernel.org; brouer@redhat.com
Subject: Re: Question about ip_defrag


On Thu, 24 Aug 2017 16:04:41 +0000 "liujian (CE)"
[off-list ref]
wrote:
quoted
quoted
What kernel version have you seen this issue with?
3.10,with some backport.

 >As far as I remember, this issue have been fixed before...

which one patch? I didnot find out the patch:(
AFAIK it was some bugs in the percpu_counter code.  If you need to
backport look at the git commits:

 git log lib/percpu_counter.c include/linux/percpu_counter.h

Are you maintaining your own 3.10 kernel?

I know that for RHEL7 (also kernel 3.10) we backported the
percpu_counter fixes...
Could you tell me which one patch?  we have backported most of the two
files's change.
Thank you ~

quoted
--Jesper

quoted
发件人: Jesper Dangaard Brouer
收件人: liujian
(CE)<liujian56@huawei.com<mailto:liujian56@huawei.com>>
quoted
抄送:
davem@davemloft.net<mailto:davem@davemloft.net>;kuznet@ms2.inr.ac.ru
quoted
<m
quoted
ailto:kuznet@ms2.inr.ac.ru>;yoshfuji@linux-ipv6.org<mailto:yoshfuji@
li
nux-ipv6.org>;elena.reshetova@intel.com<mailto:elena.reshetova@intel
.c
om>;edumazet@google.com<mailto:edumazet@google.com>;netdev@vger.k
quoted
ernel
quoted
.org<mailto:netdev@vger.kernel.org>;brouer@redhat.com<mailto:brouer
@
quoted
quoted
r
e
quoted
dhat.com>
主题: Re: Question about ip_defrag
时间: 2017-08-24 21:53:17


On Thu, 24 Aug 2017 13:15:33 +0000 "liujian (CE)"
[off-list ref]
wrote:
quoted
quoted
Hello,

With below patch we met one issue.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
/c ommit/?h=v4.13-rc6&id=6d7b857d541e

the issue:
Ip_defrag fail caused by frag_mem_limit reached 4M(frags.high_thresh).
At this moment,sum_frag_mem_limit is about 10K.
and my test machine's cpu num is 64.

Can i only change frag_mem_limit to sum_ frag_mem_limit?

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 96e95e8..f09c00b 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -120,7 +120,7 @@ static void inet_frag_secret_rebuild(struct
inet_frags *f)  static bool inet_fragq_should_evict(const struct
inet_frag_queue *q)  {
        return q->net->low_thresh == 0 ||
-              frag_mem_limit(q->net) >= q->net->low_thresh;
+              sum_frag_mem_limit(q->net) >= q->net->low_thresh;
 }

 static unsigned int
@@ -355,7 +355,7 @@ static struct inet_frag_queue
*inet_frag_alloc(struct netns_frags *nf,  {
        struct inet_frag_queue *q;

-       if (!nf->high_thresh || frag_mem_limit(nf) > nf->high_thresh) {
+       if (!nf->high_thresh || sum_frag_mem_limit(nf) >
+ nf->high_thresh) {
                inet_frag_schedule_worker(f);
                return NULL;
        }
@@ -396,7 +396,7 @@ struct inet_frag_queue *inet_frag_find(struct
netns_frags *nf,
quoted
quoted
        struct inet_frag_queue *q;
        int depth = 0;

-       if (frag_mem_limit(nf) > nf->low_thresh)
+       if (sum_frag_mem_limit(nf) > nf->low_thresh)
                inet_frag_schedule_worker(f);

        hash &= (INETFRAGS_HASHSZ - 1);
--

Thank you for your time.
What kernel version have you seen this issue with?

As far as I remember, this issue have been fixed before...

--
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


--
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: Question about ip_defrag

From: Florian Westphal <fw@strlen.de>
Date: 2017-08-28 14:03:43

liujian (CE) [off-list ref] wrote:
Hi

I checked our 3.10 kernel, we had backported all percpu_counter bug fix in lib/percpu_counter.c and include/linux/percpu_counter.h.
And I check 4.13-rc6, also has the issue if NIC's rx cpu num big enough.
quoted
quoted
quoted
quoted
the issue:
Ip_defrag fail caused by frag_mem_limit reached 4M(frags.high_thresh).
At this moment,sum_frag_mem_limit is about 10K.
So should we change ipfrag high/low thresh to a reasonable value ? 
And if it is, is there a standard to change the value?
Each cpu can have frag_percpu_counter_batch bytes rest doesn't know
about so with 64 cpus that is ~8 mbyte.

possible solutions:
1. reduce frag_percpu_counter_batch to 16k or so
2. make both low and high thresh depend on NR_CPUS

liujian, does this change help in any way?
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -123,6 +123,17 @@ static bool inet_fragq_should_evict(const struct inet_frag_queue *q)
 	       frag_mem_limit(q->net) >= q->net->low_thresh;
 }
 
+/* ->mem batch size is huge, this can cause severe discrepancies
+ * between actual value (sum of pcpu values) and the global estimate.
+ *
+ * Use a smaller batch to give an opportunity for the global estimate
+ * to more accurately reflect current state.
+ */
+static void update_frag_mem_limit(struct netns_frags *nf, unsigned int batch)
+{
+	 percpu_counter_add_batch(&nf->mem, 0, batch);
+}
+
 static unsigned int
 inet_evict_bucket(struct inet_frags *f, struct inet_frag_bucket *hb)
 {
@@ -146,8 +157,12 @@ inet_evict_bucket(struct inet_frags *f, struct inet_frag_bucket *hb)
 
 	spin_unlock(&hb->chain_lock);
 
-	hlist_for_each_entry_safe(fq, n, &expired, list_evictor)
+	hlist_for_each_entry_safe(fq, n, &expired, list_evictor) {
+		struct netns_frags *nf = fq->net;
+
 		f->frag_expire((unsigned long) fq);
+		update_frag_mem_limit(nf, 1);
+	}
 
 	return evicted;
 }
@@ -396,8 +411,10 @@ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf,
 	struct inet_frag_queue *q;
 	int depth = 0;
 
-	if (frag_mem_limit(nf) > nf->low_thresh)
+	if (frag_mem_limit(nf) > nf->low_thresh) {
 		inet_frag_schedule_worker(f);
+		update_frag_mem_limit(nf, SKB_TRUESIZE(1500) * 16);
+	}
 
 	hash &= (INETFRAGS_HASHSZ - 1);
 	hb = &f->hash[hash];
@@ -416,6 +433,8 @@ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf,
 	if (depth <= INETFRAGS_MAXDEPTH)
 		return inet_frag_create(nf, f, key);
 
+	update_frag_mem_limit(nf, 1);
+
 	if (inet_frag_may_rebuild(f)) {
 		if (!f->rebuild)
 			f->rebuild = true;

Re: Question about ip_defrag

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-29 07:20:29

On Mon, 28 Aug 2017 16:00:32 +0200
Florian Westphal [off-list ref] wrote:
liujian (CE) [off-list ref] wrote:
quoted
Hi

I checked our 3.10 kernel, we had backported all percpu_counter bug fix in lib/percpu_counter.c and include/linux/percpu_counter.h.
And I check 4.13-rc6, also has the issue if NIC's rx cpu num big enough.
  
quoted
quoted
quoted
quoted
the issue:
Ip_defrag fail caused by frag_mem_limit reached 4M(frags.high_thresh).
At this moment,sum_frag_mem_limit is about 10K.  
So should we change ipfrag high/low thresh to a reasonable value ? 
And if it is, is there a standard to change the value?  
Each cpu can have frag_percpu_counter_batch bytes rest doesn't know
about so with 64 cpus that is ~8 mbyte.

possible solutions:
1. reduce frag_percpu_counter_batch to 16k or so
2. make both low and high thresh depend on NR_CPUS
To me it looks like we/I have been using the wrong API for comparing
against percpu_counters.  I guess we should have used __percpu_counter_compare().

/*
 * Compare counter against given value.
 * Return 1 if greater, 0 if equal and -1 if less
 */
int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch)
{
	s64	count;

	count = percpu_counter_read(fbc);
	/* Check to see if rough count will be sufficient for comparison */
	if (abs(count - rhs) > (batch * num_online_cpus())) {
		if (count > rhs)
			return 1;
		else
			return -1;
	}
	/* Need to use precise count */
	count = percpu_counter_sum(fbc);
	if (count > rhs)
		return 1;
	else if (count < rhs)
		return -1;
	else
		return 0;
}
EXPORT_SYMBOL(__percpu_counter_compare);


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

RE: Question about ip_defrag

From: liujian (CE) <hidden>
Date: 2017-08-29 07:41:19

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Florian Westphal
Sent: Monday, August 28, 2017 10:01 PM
To: liujian (CE)
Cc: Jesper Dangaard Brouer; davem@davemloft.net; kuznet@ms2.inr.ac.ru;
yoshfuji@linux-ipv6.org; elena.reshetova@intel.com; edumazet@google.com;
netdev@vger.kernel.org; Wangkefeng (Kevin); weiyongjun (A)
Subject: Re: Question about ip_defrag

liujian (CE) [off-list ref] wrote:
quoted
Hi

I checked our 3.10 kernel, we had backported all percpu_counter bug fix in
lib/percpu_counter.c and include/linux/percpu_counter.h.
quoted
And I check 4.13-rc6, also has the issue if NIC's rx cpu num big enough.
quoted
quoted
quoted
quoted
the issue:
Ip_defrag fail caused by frag_mem_limit reached
4M(frags.high_thresh).
quoted
quoted
quoted
quoted
quoted
At this moment,sum_frag_mem_limit is about 10K.
So should we change ipfrag high/low thresh to a reasonable value ?
And if it is, is there a standard to change the value?
Each cpu can have frag_percpu_counter_batch bytes rest doesn't know about
so with 64 cpus that is ~8 mbyte.

possible solutions:
1. reduce frag_percpu_counter_batch to 16k or so 2. make both low and high
thresh depend on NR_CPUS
Thank you for your reply.
 
liujian, does this change help in any way?
I will have a try.
quoted hunk
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -123,6 +123,17 @@ static bool inet_fragq_should_evict(const struct
inet_frag_queue *q)
 	       frag_mem_limit(q->net) >= q->net->low_thresh;  }

+/* ->mem batch size is huge, this can cause severe discrepancies
+ * between actual value (sum of pcpu values) and the global estimate.
+ *
+ * Use a smaller batch to give an opportunity for the global estimate
+ * to more accurately reflect current state.
+ */
+static void update_frag_mem_limit(struct netns_frags *nf, unsigned int
+batch) {
+	 percpu_counter_add_batch(&nf->mem, 0, batch); }
+
 static unsigned int
 inet_evict_bucket(struct inet_frags *f, struct inet_frag_bucket *hb)  { @@
-146,8 +157,12 @@ inet_evict_bucket(struct inet_frags *f, struct
inet_frag_bucket *hb)

 	spin_unlock(&hb->chain_lock);

-	hlist_for_each_entry_safe(fq, n, &expired, list_evictor)
+	hlist_for_each_entry_safe(fq, n, &expired, list_evictor) {
+		struct netns_frags *nf = fq->net;
+
 		f->frag_expire((unsigned long) fq);
+		update_frag_mem_limit(nf, 1);
quoted hunk
+	}

 	return evicted;
 }
@@ -396,8 +411,10 @@ struct inet_frag_queue *inet_frag_find(struct
netns_frags *nf,
 	struct inet_frag_queue *q;
 	int depth = 0;

-	if (frag_mem_limit(nf) > nf->low_thresh)
+	if (frag_mem_limit(nf) > nf->low_thresh) {
 		inet_frag_schedule_worker(f);
+		update_frag_mem_limit(nf, SKB_TRUESIZE(1500) * 16); 
+	}

 	hash &= (INETFRAGS_HASHSZ - 1);
 	hb = &f->hash[hash];
@@ -416,6 +433,8 @@ struct inet_frag_queue *inet_frag_find(struct
netns_frags *nf,
 	if (depth <= INETFRAGS_MAXDEPTH)
 		return inet_frag_create(nf, f, key);

+	update_frag_mem_limit(nf, 1);
+
 	if (inet_frag_may_rebuild(f)) {
 		if (!f->rebuild)
 			f->rebuild = true;

RE: Question about ip_defrag

From: liujian (CE) <hidden>
Date: 2017-08-29 07:44:42

-----Original Message-----
From: Jesper Dangaard Brouer [mailto:brouer@redhat.com]
Sent: Tuesday, August 29, 2017 3:20 PM
To: Florian Westphal
Cc: liujian (CE); davem@davemloft.net; kuznet@ms2.inr.ac.ru;
yoshfuji@linux-ipv6.org; elena.reshetova@intel.com; edumazet@google.com;
netdev@vger.kernel.org; Wangkefeng (Kevin); weiyongjun (A);
brouer@redhat.com
Subject: Re: Question about ip_defrag

On Mon, 28 Aug 2017 16:00:32 +0200
Florian Westphal [off-list ref] wrote:
quoted
liujian (CE) [off-list ref] wrote:
quoted
Hi

I checked our 3.10 kernel, we had backported all percpu_counter bug fix in
lib/percpu_counter.c and include/linux/percpu_counter.h.
quoted
quoted
And I check 4.13-rc6, also has the issue if NIC's rx cpu num big enough.
quoted
quoted
quoted
quoted
the issue:
Ip_defrag fail caused by frag_mem_limit reached
4M(frags.high_thresh).
quoted
quoted
quoted
quoted
quoted
quoted
At this moment,sum_frag_mem_limit is about 10K.
So should we change ipfrag high/low thresh to a reasonable value ?
And if it is, is there a standard to change the value?
Each cpu can have frag_percpu_counter_batch bytes rest doesn't know
about so with 64 cpus that is ~8 mbyte.

possible solutions:
1. reduce frag_percpu_counter_batch to 16k or so 2. make both low and
high thresh depend on NR_CPUS
To me it looks like we/I have been using the wrong API for comparing against
percpu_counters.  I guess we should have used
__percpu_counter_compare().
Are you means?
Change 
if (frag_mem_limit(nf) > nf->low_thresh)
to
__percpu_counter_compare(&nf->mem, nf->low_thresh, frag_percpu_counter_batch)
/*
 * Compare counter against given value.
 * Return 1 if greater, 0 if equal and -1 if less  */ int
__percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) {
	s64	count;

	count = percpu_counter_read(fbc);
	/* Check to see if rough count will be sufficient for comparison */
	if (abs(count - rhs) > (batch * num_online_cpus())) {
		if (count > rhs)
			return 1;
		else
			return -1;
	}
	/* Need to use precise count */
	count = percpu_counter_sum(fbc);
	if (count > rhs)
		return 1;
	else if (count < rhs)
		return -1;
	else
		return 0;
}
EXPORT_SYMBOL(__percpu_counter_compare);


--
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: Question about ip_defrag

From: Florian Westphal <fw@strlen.de>
Date: 2017-08-29 07:56:13

Jesper Dangaard Brouer [off-list ref] wrote:
On Mon, 28 Aug 2017 16:00:32 +0200
Florian Westphal [off-list ref] wrote:
quoted
liujian (CE) [off-list ref] wrote:
quoted
Hi

I checked our 3.10 kernel, we had backported all percpu_counter bug fix in lib/percpu_counter.c and include/linux/percpu_counter.h.
And I check 4.13-rc6, also has the issue if NIC's rx cpu num big enough.
  
quoted
quoted
quoted
quoted
the issue:
Ip_defrag fail caused by frag_mem_limit reached 4M(frags.high_thresh).
At this moment,sum_frag_mem_limit is about 10K.  
So should we change ipfrag high/low thresh to a reasonable value ? 
And if it is, is there a standard to change the value?  
Each cpu can have frag_percpu_counter_batch bytes rest doesn't know
about so with 64 cpus that is ~8 mbyte.

possible solutions:
1. reduce frag_percpu_counter_batch to 16k or so
2. make both low and high thresh depend on NR_CPUS
I take 2) back.  Its wrong to do this, for large NR_CPU values it
would even overflow.
To me it looks like we/I have been using the wrong API for comparing
against percpu_counters.  I guess we should have used __percpu_counter_compare().
Are you sure?  For liujian use case (64 cores) it looks like we would
always fall through to percpu_counter_sum() so we eat spinlock_irqsave
cost for all compares.

Before we entertain this we should consider reducing frag_percpu_counter_batch
to a smaller value.

RE: Question about ip_defrag

From: liujian (CE) <hidden>
Date: 2017-08-29 13:02:25



Best Regards,
liujian

-----Original Message-----
From: liujian (CE)
Sent: Tuesday, August 29, 2017 3:39 PM
To: 'Florian Westphal'
Cc: Jesper Dangaard Brouer; davem@davemloft.net; kuznet@ms2.inr.ac.ru;
yoshfuji@linux-ipv6.org; elena.reshetova@intel.com; edumazet@google.com;
netdev@vger.kernel.org; Wangkefeng (Kevin); weiyongjun (A)
Subject: RE: Question about ip_defrag


quoted
-----Original Message-----
From: netdev-owner@vger.kernel.org
[mailto:netdev-owner@vger.kernel.org]
On Behalf Of Florian Westphal
Sent: Monday, August 28, 2017 10:01 PM
To: liujian (CE)
Cc: Jesper Dangaard Brouer; davem@davemloft.net; kuznet@ms2.inr.ac.ru;
yoshfuji@linux-ipv6.org; elena.reshetova@intel.com;
edumazet@google.com; netdev@vger.kernel.org; Wangkefeng (Kevin);
weiyongjun (A)
Subject: Re: Question about ip_defrag

liujian (CE) [off-list ref] wrote:
quoted
Hi

I checked our 3.10 kernel, we had backported all percpu_counter bug
fix in
lib/percpu_counter.c and include/linux/percpu_counter.h.
quoted
And I check 4.13-rc6, also has the issue if NIC's rx cpu num big enough.
quoted
quoted
quoted
quoted
the issue:
Ip_defrag fail caused by frag_mem_limit reached
4M(frags.high_thresh).
quoted
quoted
quoted
quoted
quoted
At this moment,sum_frag_mem_limit is about 10K.
So should we change ipfrag high/low thresh to a reasonable value ?
And if it is, is there a standard to change the value?
Each cpu can have frag_percpu_counter_batch bytes rest doesn't know
about so with 64 cpus that is ~8 mbyte.

possible solutions:
1. reduce frag_percpu_counter_batch to 16k or so 2. make both low and
high thresh depend on NR_CPUS
Thank you for your reply.
quoted
liujian, does this change help in any way?
I will have a try.
Now, I have not the real environment. 
I use iperf generate fragment packets; 
and I always change NIC rx irq's affinity cpu, to make sure frag_mem_limit reach to thresh.
my test machine, CPU num is 384.
As above , test the patch , seemingly , there is no improving...
Check /proc/net/snmp, there is no significant difference.
maybe we should find a good test method!

root@RH8100-V3:/proc/net# cat sockstat
sockets: used 1386
TCP: inuse 3 orphan 0 tw 0 alloc 4 mem 1
UDP: inuse 44 mem 42
UDPLITE: inuse 0
RAW: inuse 0
FRAG: inuse 1 memory 34336, 3144424.
 
quoted
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -123,6 +123,17 @@ static bool inet_fragq_should_evict(const struct
inet_frag_queue *q)
 	       frag_mem_limit(q->net) >= q->net->low_thresh;  }

+/* ->mem batch size is huge, this can cause severe discrepancies
+ * between actual value (sum of pcpu values) and the global estimate.
+ *
+ * Use a smaller batch to give an opportunity for the global estimate
+ * to more accurately reflect current state.
+ */
+static void update_frag_mem_limit(struct netns_frags *nf, unsigned
+int
+batch) {
+	 percpu_counter_add_batch(&nf->mem, 0, batch); }
+
 static unsigned int
 inet_evict_bucket(struct inet_frags *f, struct inet_frag_bucket *hb)
{ @@
-146,8 +157,12 @@ inet_evict_bucket(struct inet_frags *f, struct
inet_frag_bucket *hb)

 	spin_unlock(&hb->chain_lock);

-	hlist_for_each_entry_safe(fq, n, &expired, list_evictor)
+	hlist_for_each_entry_safe(fq, n, &expired, list_evictor) {
+		struct netns_frags *nf = fq->net;
+
 		f->frag_expire((unsigned long) fq);
+		update_frag_mem_limit(nf, 1);
quoted
+	}

 	return evicted;
 }
@@ -396,8 +411,10 @@ struct inet_frag_queue *inet_frag_find(struct
netns_frags *nf,
 	struct inet_frag_queue *q;
 	int depth = 0;

-	if (frag_mem_limit(nf) > nf->low_thresh)
+	if (frag_mem_limit(nf) > nf->low_thresh) {
 		inet_frag_schedule_worker(f);
+		update_frag_mem_limit(nf, SKB_TRUESIZE(1500) * 16);
+	}

 	hash &= (INETFRAGS_HASHSZ - 1);
 	hb = &f->hash[hash];
@@ -416,6 +433,8 @@ struct inet_frag_queue *inet_frag_find(struct
netns_frags *nf,
 	if (depth <= INETFRAGS_MAXDEPTH)
 		return inet_frag_create(nf, f, key);

+	update_frag_mem_limit(nf, 1);
+
 	if (inet_frag_may_rebuild(f)) {
 		if (!f->rebuild)
 			f->rebuild = true;

Re: Question about ip_defrag

From: Florian Westphal <fw@strlen.de>
Date: 2017-08-29 13:49:21

liujian (CE) [off-list ref] wrote:

[ trimming cc list ]
Now, I have not the real environment. 
I use iperf generate fragment packets; 
and I always change NIC rx irq's affinity cpu, to make sure frag_mem_limit reach to thresh.
my test machine, CPU num is 384.
Oh well, that explains it.
quoted
quoted
+	if (frag_mem_limit(nf) > nf->low_thresh) {
 		inet_frag_schedule_worker(f);
+		update_frag_mem_limit(nf, SKB_TRUESIZE(1500) * 16);
+	}
You need to reduce this to a lower value.
Your cpu count * batch_value needs to be less than
low_thresh to avoid problems.

Wtih 384 cpus its close to 12 mbyte...

Perhaps do this:

update_frag_mem_limit(nf, 2 * 1024*1024 / NR_CPUS);


However, I think its better to revert the percpu counter change and
move back to a single atomic_t count.

RE: Question about ip_defrag

From: liujian (CE) <hidden>
Date: 2017-08-30 01:52:49



Best Regards,
liujian

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Florian Westphal
Sent: Tuesday, August 29, 2017 9:47 PM
To: liujian (CE)
Cc: Florian Westphal; Jesper Dangaard Brouer; netdev@vger.kernel.org;
Wangkefeng (Kevin); weiyongjun (A)
Subject: Re: Question about ip_defrag

liujian (CE) [off-list ref] wrote:

[ trimming cc list ]
quoted
Now, I have not the real environment.
I use iperf generate fragment packets; and I always change NIC rx
irq's affinity cpu, to make sure frag_mem_limit reach to thresh.
my test machine, CPU num is 384.
Oh well, that explains it.
quoted
quoted
quoted
+	if (frag_mem_limit(nf) > nf->low_thresh) {
 		inet_frag_schedule_worker(f);
+		update_frag_mem_limit(nf, SKB_TRUESIZE(1500) * 16);
+	}
You need to reduce this to a lower value.
Your cpu count * batch_value needs to be less than low_thresh to avoid
problems.

Wtih 384 cpus its close to 12 mbyte...

Perhaps do this:

update_frag_mem_limit(nf, 2 * 1024*1024 / NR_CPUS);


However, I think its better to revert the percpu counter change and move back
to a single atomic_t count.
Ok. 
Florian and Jesper, many thanks for this issue. 

Re: Question about ip_defrag

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-30 10:58:58

(trimmed CC list a bit)

On Tue, 29 Aug 2017 09:53:15 +0200 Florian Westphal [off-list ref] wrote:
Jesper Dangaard Brouer [off-list ref] wrote:
quoted
On Mon, 28 Aug 2017 16:00:32 +0200
Florian Westphal [off-list ref] wrote:
  
quoted
liujian (CE) [off-list ref] wrote:  
quoted
Hi

I checked our 3.10 kernel, we had backported all percpu_counter
bug fix in lib/percpu_counter.c and include/linux/percpu_counter.h.
And I check 4.13-rc6, also has the issue if NIC's rx cpu num big enough.
    
quoted
quoted
quoted
quoted
the issue:
Ip_defrag fail caused by frag_mem_limit reached 4M(frags.high_thresh).
At this moment,sum_frag_mem_limit is about 10K.    
So should we change ipfrag high/low thresh to a reasonable value ? 
And if it is, is there a standard to change the value?    
Each cpu can have frag_percpu_counter_batch bytes rest doesn't know
about so with 64 cpus that is ~8 mbyte.

possible solutions:
1. reduce frag_percpu_counter_batch to 16k or so
2. make both low and high thresh depend on NR_CPUS  
I take 2) back.  Its wrong to do this, for large NR_CPU values it
would even overflow.
Alternatively solution 3:
Why do we want to maintain a (4MBytes) memory limit, across all CPUs?
Couldn't we just allow each CPU to have a memory limit?

quoted
To me it looks like we/I have been using the wrong API for comparing
against percpu_counters.  I guess we should have used __percpu_counter_compare().  
Are you sure?  For liujian use case (64 cores) it looks like we would
always fall through to percpu_counter_sum() so we eat spinlock_irqsave
cost for all compares.

Before we entertain this we should consider reducing frag_percpu_counter_batch
to a smaller value.
Yes, I agree, we really need to lower/reduce the frag_percpu_counter_batch.
As you say, else the __percpu_counter_compare() call will be useless
(around systems with >= 32 CPUs).

I think the bug is in frag_mem_limit().  It just reads the global
counter (fbc->count), without considering other CPUs can have upto 130K
that haven't been subtracted yet (due to 3M low limit, become dangerous
at >=24 CPUs).  The  __percpu_counter_compare() does the right thing,
and takes into account the number of (online) CPUs and batch size, to
account for this.

If we choose 16K (16384), and use __percpu_counter_compare(), then we
can scale to systems with 256 CPUs (4*1024*1024/16384=256), before this
memory accounting becomes more expensive (than not using percpu_counters).
But Liujian, reports he have a 384 CPU system, so he would still need
to increase the lower+high threshold.


$ grep -H .  /proc/sys/net/ipv*/ip*frag_*_thresh /proc/sys/net/netfilter/nf_conntrack_frag6_*_thresh
/proc/sys/net/ipv4/ipfrag_high_thresh:4194304
/proc/sys/net/ipv4/ipfrag_low_thresh:3145728
/proc/sys/net/ipv6/ip6frag_high_thresh:4194304
/proc/sys/net/ipv6/ip6frag_low_thresh:3145728
/proc/sys/net/netfilter/nf_conntrack_frag6_high_thresh:4194304
/proc/sys/net/netfilter/nf_conntrack_frag6_low_thresh:3145728

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: Question about ip_defrag

From: Florian Westphal <fw@strlen.de>
Date: 2017-08-30 12:01:11

Jesper Dangaard Brouer [off-list ref] wrote:
quoted
I take 2) back.  Its wrong to do this, for large NR_CPU values it
would even overflow.
Alternatively solution 3:
Why do we want to maintain a (4MBytes) memory limit, across all CPUs?
Couldn't we just allow each CPU to have a memory limit?
Consider ipv4, ipv6, nf ipv6 defrag, 6lowpan, and 8k cpus... This will
render any limit useless.
quoted
quoted
To me it looks like we/I have been using the wrong API for comparing
against percpu_counters.  I guess we should have used __percpu_counter_compare().  
Are you sure?  For liujian use case (64 cores) it looks like we would
always fall through to percpu_counter_sum() so we eat spinlock_irqsave
cost for all compares.

Before we entertain this we should consider reducing frag_percpu_counter_batch
to a smaller value.
Yes, I agree, we really need to lower/reduce the frag_percpu_counter_batch.
As you say, else the __percpu_counter_compare() call will be useless
(around systems with >= 32 CPUs).

I think the bug is in frag_mem_limit().  It just reads the global
counter (fbc->count), without considering other CPUs can have upto 130K
that haven't been subtracted yet (due to 3M low limit, become dangerous
at >=24 CPUs).  The  __percpu_counter_compare() does the right thing,
and takes into account the number of (online) CPUs and batch size, to
account for this.
Right, I think we should at very least use __percpu_counter_compare
before denying a new frag queue allocation request.

I'll create a patch.

Re: Question about ip_defrag

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-30 12:22:15

On Wed, 30 Aug 2017 13:58:20 +0200
Florian Westphal [off-list ref] wrote:
Jesper Dangaard Brouer [off-list ref] wrote:
quoted
quoted
I take 2) back.  Its wrong to do this, for large NR_CPU values it
would even overflow.  
Alternatively solution 3:
Why do we want to maintain a (4MBytes) memory limit, across all CPUs?
Couldn't we just allow each CPU to have a memory limit?  
Consider ipv4, ipv6, nf ipv6 defrag, 6lowpan, and 8k cpus... This will
render any limit useless.
With 8K CPUs I agree, that this might be a bad idea!
quoted
quoted
quoted
To me it looks like we/I have been using the wrong API for comparing
against percpu_counters.  I guess we should have used __percpu_counter_compare().    
Are you sure?  For liujian use case (64 cores) it looks like we would
always fall through to percpu_counter_sum() so we eat spinlock_irqsave
cost for all compares.

Before we entertain this we should consider reducing frag_percpu_counter_batch
to a smaller value.  
Yes, I agree, we really need to lower/reduce the frag_percpu_counter_batch.
As you say, else the __percpu_counter_compare() call will be useless
(around systems with >= 32 CPUs).

I think the bug is in frag_mem_limit().  It just reads the global
counter (fbc->count), without considering other CPUs can have upto 130K
that haven't been subtracted yet (due to 3M low limit, become dangerous
at >=24 CPUs).  The  __percpu_counter_compare() does the right thing,
and takes into account the number of (online) CPUs and batch size, to
account for this.  
Right, I think we should at very least use __percpu_counter_compare
before denying a new frag queue allocation request.

I'll create a patch.
Oh, I've already started working on a patch, that I'm testing now.  But
if you want to take the assignment then I'm fine with that!.  I just
though that it was my responsibility to fix, given I introduced
percpu_counter usage (back in 2013-01-28 / 6d7b857d541e).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help