RE: Question about ip_defrag

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;