From: Peter Zijlstra <hidden> Date: 2006-08-12 14:15:05
Hi,
here the latest effort, it includes a whole new trivial allocator with a
horrid name and an almost full rewrite of the deadlock prevention core.
This version does not do anything per device and hence does not depend
on the new netdev_alloc_skb() API.
The reason to add a second allocator to the receive side is twofold:
1) it allows easy detection of the memory pressure / OOM situation;
2) it allows the receive path to be unbounded and go at full speed when
resources permit.
The choice of using the global memalloc reserve as a mempool makes that
the new allocator has to release pages as soon as possible; if we were
to hoard pages in the allocator the memalloc reserve would not get
replenished readily.
Peter
From: Peter Zijlstra <hidden> Date: 2006-08-12 14:15:45
The core of the VM deadlock avoidance framework.
From the 'user' side of things it provides a function to mark a 'struct sock'
as SOCK_MEMALLOC, meaning this socket may dip into the memalloc reserves on
the receive side.
When *dev_alloc_skb() finds it cannot allocate a struct sk_buff the regular
way it will grab some memory from the memalloc reserve.
Network paths will drop !SOCK_MEMALLOC packets ASAP when reserve is being used.
Memalloc sk_buff allocations are not done from the SLAB but are done using
the new SROG allocator. sk_buff::memalloc records this exception so that
kfree_skbmem()/skb_clone() and others can do the right thing.
Signed-off-by: Peter Zijlstra <redacted>
Signed-off-by: Daniel Phillips <redacted>
---
include/linux/gfp.h | 3 -
include/linux/mmzone.h | 1
include/linux/skbuff.h | 6 +-
include/net/sock.h | 40 +++++++++++++++
mm/page_alloc.c | 41 ++++++++++++++-
net/core/skbuff.c | 127 ++++++++++++++++++++++++++++++++++++++++++++-----
net/core/sock.c | 74 ++++++++++++++++++++++++++++
net/ipv4/af_inet.c | 3 +
net/ipv4/icmp.c | 3 +
net/ipv4/tcp_ipv4.c | 3 +
net/ipv4/udp.c | 8 ++-
11 files changed, 290 insertions(+), 19 deletions(-)
Index: linux-2.6/include/linux/gfp.h
===================================================================
@@ -54,7 +55,7 @@ struct vm_area_struct;#define GFP_LEVEL_MASK (__GFP_WAIT|__GFP_HIGH|__GFP_IO|__GFP_FS| \__GFP_COLD|__GFP_NOWARN|__GFP_REPEAT|\__GFP_NOFAIL|__GFP_NORETRY|__GFP_NO_GROW|__GFP_COMP|\-__GFP_NOMEMALLOC|__GFP_HARDWALL)+__GFP_NOMEMALLOC|__GFP_HARDWALL|__GFP_MEMALLOC)/* This equals 0, but use constants in case they ever change */#define GFP_NOWAIT (GFP_ATOMIC & ~__GFP_HIGH)
@@ -420,6 +420,7 @@ int percpu_pagelist_fraction_sysctl_handvoid__user*,size_t*,loff_t*);intsysctl_min_unmapped_ratio_sysctl_handler(structctl_table*,int,structfile*,void__user*,size_t*,loff_t*);+intadjust_memalloc_reserve(intbytes);#include<linux/topology.h>/* Returns the number of the current Node. */
@@ -970,8 +971,8 @@ restart:/* This allocation should allow future memory freeing. */-if(((p->flags&PF_MEMALLOC)||unlikely(test_thread_flag(TIF_MEMDIE)))-&&!in_interrupt()){+if((((p->flags&PF_MEMALLOC)||unlikely(test_thread_flag(TIF_MEMDIE)))+&&!in_interrupt())||(gfp_mask&__GFP_MEMALLOC)){if(!(gfp_mask&__GFP_NOMEMALLOC)){nofail_alloc:/* go through the zonelist yet again, ignoring mins */
@@ -139,14 +140,45 @@ EXPORT_SYMBOL(skb_truesize_bug);*Buffersmayonlybeallocatedfrominterruptsusinga@gfp_maskof*%GFP_ATOMIC.*/-structsk_buff*__alloc_skb(unsignedintsize,gfp_tgfp_mask,-intfclone)+static+structsk_buff*___alloc_skb(unsignedintsize,gfp_tgfp_mask,intfclone){kmem_cache_t*cache;structskb_shared_info*shinfo;structsk_buff*skb;u8*data;+size=SKB_DATA_ALIGN(size);++if(gfp_mask&__GFP_MEMALLOC){+cache=NULL;+skb=NULL;+if(!memalloc_skbs_try_inc())+gotoout;++/*+*Allocatethedatasectionfirstbecauseweknowthefirst+*SROGallocisavalidSROGentrypointandskb->headis+*sharedbetweenclones.ThissavesusfromtrackingSROGs.+*/+data=srog_alloc(NULL,size+sizeof(structskb_shared_info),+gfp_mask);+if(!data)+gotodec_out;++skb=srog_alloc(data,fclone+?2*sizeof(structsk_buff)+sizeof(atomic_t)+:sizeof(structsk_buff),gfp_mask);+if(!skb){+srog_free(NULL,data);+dec_out:+memalloc_skbs_dec();+gotoout;+}++gotoallocated;+}+cache=fclone?skbuff_fclone_cache:skbuff_head_cache;/* Get the HEAD */
@@ -155,12 +187,13 @@ struct sk_buff *__alloc_skb(unsigned intgotoout;/* Get the DATA. Size must match skb_add_mtu(). */-size=SKB_DATA_ALIGN(size);data=____kmalloc(size+sizeof(structskb_shared_info),gfp_mask);if(!data)gotonodata;+allocated:memset(skb,0,offsetof(structsk_buff,truesize));+skb->memalloc=!cache;skb->truesize=size+sizeof(structsk_buff);atomic_set(&skb->users,1);skb->head=data;
@@ -1136,7 +1136,12 @@ int udp_rcv(struct sk_buff *skb)sk=udp_v4_lookup(saddr,uh->source,daddr,uh->dest,skb->dev->ifindex);if(sk!=NULL){-intret=udp_queue_rcv_skb(sk,skb);+intret;++if(unlikely(skb->memalloc&&!sk_is_memalloc(sk)))+gotodrop_noncritical;++ret=udp_queue_rcv_skb(sk,skb);sock_put(sk);/* a return value > 0 means to resubmit the input, but
@@ -1147,6 +1152,7 @@ int udp_rcv(struct sk_buff *skb)return0;}+drop_noncritical:if(!xfrm4_policy_check(NULL,XFRM_POLICY_IN,skb))gotodrop;nf_reset(skb);
@@ -195,6 +197,78 @@ __u32 sysctl_rmem_default = SK_RMEM_MAX;/* Maximal space eaten by iovec or ancilliary data plus some space */intsysctl_optmem_max=sizeof(unsignedlong)*(2*UIO_MAXIOV+512);+staticDEFINE_SPINLOCK(memalloc_lock);+staticintmemalloc_socks;+staticunsignedlongmemalloc_reserve;++atomic_tmemalloc_skbs_used;+EXPORT_SYMBOL_GPL(memalloc_skbs_used);++/**+*sk_adjust_memalloc-adjusttheglobalmemallocreserveforthisdevice+*@dev:devicethathasmemallocdemands+*@nr_socks:numberofnew%SOCK_MEMALLOCsockets+*+*Thisfunctionadjuststhememallocreservebasedondevice+*demand.Foreach%SOCK_MEMALLOCsocketthisdevicewillreserve+*2*%MAX_PHYS_SEGMENTSpagesforoutboundtraffic(assumption:+*each%SOCK_MEMALLOCsocketwillhavea%request_queueassociated)+*and5*%MAX_CONCURRENT_SKBSpages.+*+*2*%MAX_PHYS_SEGMENTS-therequestqueuecanholdupto150%the+*remaining50%goestobeingsurewecanwritepacketsfor+*theoutgoingpages.+*+*5*%MAX_CONCURRENT_SKBS-foreachskb4pagesforhighorder+*jumboframeallocs,and1forgoodmeasure.+*/+intsk_adjust_memalloc(intnr_socks)+{+unsignedlongflags;+unsignedlongreserve;+interr;++spin_lock_irqsave(&memalloc_lock,flags);++memalloc_socks+=nr_socks;+BUG_ON(memalloc_socks<0);++reserve=memalloc_socks*2*MAX_PHYS_SEGMENTS+/* outbound */+MAX_CONCURRENT_SKBS*5;/* inbound */++err=adjust_memalloc_reserve(reserve-memalloc_reserve);+if(err){+printk(KERN_WARNING+"Unable to change RX reserve to: %lu, error: %d\n",+reserve,err);+gotounlock;+}+memalloc_reserve=reserve;++unlock:+spin_unlock_irqrestore(&memalloc_lock,flags);+returnerr;+}+EXPORT_SYMBOL_GPL(sk_adjust_memalloc);++/**+*sk_set_memalloc-sets%SOCK_MEMALLOC+*@sk:sockettosetiton+*+*Set%SOCK_MEMALLOConasocketandincreasethememallocreserve+*accordingly.+*/+intsk_set_memalloc(structsock*sk)+{+interr=0;++if(!(err=sk_adjust_memalloc(1)))+sock_set_flag(sk,SOCK_MEMALLOC);++returnerr;+}+EXPORT_SYMBOL_GPL(sk_set_memalloc);+staticintsock_set_timeout(long*timeo_p,char__user*optval,intoptlen){structtimevaltv;
From: Peter Zijlstra <hidden> Date: 2006-08-12 14:15:54
A simple memory allocator with a horrid name.
(if someone knows the proper name of this thing, please speak up.
It's too trivial to not be described somewhere)
Its use is for cases where you have multiple objects of various sizes
that have similar livetimes and should release pages as soon as possible.
In a few words, it allocates pages and packs the objects in them, the
pages are kept on a list and the free space blocks are kept in address
order. On free insertion sort is used and front and back merges are tried.
Signed-off-by: Peter Zijlstra <redacted>
---
include/linux/srog.h | 14 ++
mm/Makefile | 3
mm/srog.c | 290 +++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 306 insertions(+), 1 deletion(-)
Index: linux-2.6/include/linux/srog.h
===================================================================
From: Peter Zijlstra <hidden> Date: 2006-08-12 14:16:36
Use sk_set_memalloc() on the nbd socket.
Limit each request to 1 page, so that the request throttling also limits the
number of in-flight pages and force the IO scheduler to NOOP as anything else
doesn't make sense anyway.
Signed-off-by: Peter Zijlstra <redacted>
Signed-off-by: Daniel Phillips <redacted>
---
block/elevator.c | 5 +++++
block/ll_rw_blk.c | 12 ++++++++++--
drivers/block/nbd.c | 12 +++++++++++-
include/linux/blkdev.h | 9 +++++++++
4 files changed, 35 insertions(+), 3 deletions(-)
Index: linux-2.6/block/ll_rw_blk.c
===================================================================
@@ -361,8 +361,13 @@ static void nbd_do_it(struct nbd_device BUG_ON(lo->magic!=LO_MAGIC);+if(sk_set_memalloc(lo->sock->sk))+printk(KERN_WARNING+"failed to set SO_MEMALLOC on NBD socket\n");+while((req=nbd_read_stat(lo))!=NULL)nbd_end_request(req);+return;}
@@ -628,11 +633,16 @@ static int __init nbd_init(void)*everygendisktohaveitsveryownrequest_queuestruct.*Thesestructsarebigsowedynamicallyallocatethem.*/-disk->queue=blk_init_queue(do_nbd_request,&nbd_lock);+disk->queue=blk_init_queue_node_elv(do_nbd_request,+&nbd_lock,-1,"noop");if(!disk->queue){put_disk(disk);gotoout;}+blk_queue_pin_elevator(disk->queue);+blk_queue_max_segment_size(disk->queue,PAGE_SIZE);+blk_queue_max_hw_segments(disk->queue,1);+blk_queue_max_phys_segments(disk->queue,1);}if(register_blkdev(NBD_MAJOR,"nbd")){
This symbol name has nothing to do with its purpose. The entire area of
code you are modifying could be described as having something to do with
'memalloc'.
GFP_EMERGENCY or GFP_USE_RESERVES or somesuch would be a far better
symbol name.
I recognize that is matches with GFP_NOMEMALLOC, but that doesn't change
the situation anyway. In fact, a cleanup patch to rename GFP_NOMEMALLOC
would be nice.
Jeff
This symbol name has nothing to do with its purpose. The entire area of
code you are modifying could be described as having something to do with
'memalloc'.
GFP_EMERGENCY or GFP_USE_RESERVES or somesuch would be a far better
symbol name.
I recognize that is matches with GFP_NOMEMALLOC, but that doesn't change
the situation anyway. In fact, a cleanup patch to rename GFP_NOMEMALLOC
would be nice.
I'm rather bad at picking names, but here goes:
PF_MEMALLOC -> PF_EMERGALLOC
__GFP_NOMEMALLOC -> __GFP_NOEMERGALLOC
__GFP_MEMALLOC -> __GFP_EMERGALLOC
Is that suitable and shall I prepare patches? Or do we want more ppl to
chime in and have a few more rounds?
+#define __GFP_MEMALLOC ((__force gfp_t)0x40000u) /* Use emergency reserves */
This symbol name has nothing to do with its purpose. The entire area of
code you are modifying could be described as having something to do with
'memalloc'.
GFP_EMERGENCY or GFP_USE_RESERVES or somesuch would be a far better
symbol name.
I recognize that is matches with GFP_NOMEMALLOC, but that doesn't change
the situation anyway. In fact, a cleanup patch to rename GFP_NOMEMALLOC
would be nice.
I'm rather bad at picking names, but here goes:
PF_MEMALLOC -> PF_EMERGALLOC
__GFP_NOMEMALLOC -> __GFP_NOEMERGALLOC
__GFP_MEMALLOC -> __GFP_EMERGALLOC
Is that suitable and shall I prepare patches? Or do we want more ppl to
chime in and have a few more rounds?
Pardon my ignorance, but if we're doing cleanup anyway, why not use only one flag instead of two?
Why is __GFP_NOMEMALLOC needed when not setting __GFP_MEMALLOC could mean the same? Or else what
is the expected behaviour if both flags are set?
+#define __GFP_MEMALLOC ((__force gfp_t)0x40000u) /* Use emergency reserves */
This symbol name has nothing to do with its purpose. The entire area of
code you are modifying could be described as having something to do with
'memalloc'.
GFP_EMERGENCY or GFP_USE_RESERVES or somesuch would be a far better
symbol name.
I recognize that is matches with GFP_NOMEMALLOC, but that doesn't change
the situation anyway. In fact, a cleanup patch to rename GFP_NOMEMALLOC
would be nice.
I'm rather bad at picking names, but here goes:
PF_MEMALLOC -> PF_EMERGALLOC
__GFP_NOMEMALLOC -> __GFP_NOEMERGALLOC
__GFP_MEMALLOC -> __GFP_EMERGALLOC
SOCK_MEMALLOC -> SOCK_EMERGALLOC
quoted
Is that suitable and shall I prepare patches? Or do we want more ppl to
chime in and have a few more rounds?
Pardon my ignorance, but if we're doing cleanup anyway, why not use only one flag instead of two?
Why is __GFP_NOMEMALLOC needed when not setting __GFP_MEMALLOC could mean the same? Or else what
is the expected behaviour if both flags are set?
__GFP_NOMEMALLOC is most authorative; its use is (afaik) to negate
PF_MEMALLOC.
I agree that having both seems odd, but I haven't spend any significant
time on trying to find a 'nicer' solution.
On Sat, August 12, 2006 16:14, Peter Zijlstra said:
Hi,
here the latest effort, it includes a whole new trivial allocator with a
horrid name and an almost full rewrite of the deadlock prevention core.
This version does not do anything per device and hence does not depend
on the new netdev_alloc_skb() API.
The reason to add a second allocator to the receive side is twofold:
1) it allows easy detection of the memory pressure / OOM situation;
2) it allows the receive path to be unbounded and go at full speed when
resources permit.
The choice of using the global memalloc reserve as a mempool makes that
the new allocator has to release pages as soon as possible; if we were
to hoard pages in the allocator the memalloc reserve would not get
replenished readily.
Version 2 had about 250 new lines of code, while v3 has close to 600, when
including the SROG code. And that while things should have become simpler.
So why use SROG instead of the old alloc_pages() based code? And why couldn't
you use a slightly modified SLOB instead of writing a new allocator?
It looks like overkill to me.
Greetings,
Indan
From: Peter Zijlstra <hidden> Date: 2006-08-12 17:34:25
On Sat, 2006-08-12 at 18:51 +0200, Indan Zupancic wrote:
On Sat, August 12, 2006 16:14, Peter Zijlstra said:
quoted
Hi,
here the latest effort, it includes a whole new trivial allocator with a
horrid name and an almost full rewrite of the deadlock prevention core.
This version does not do anything per device and hence does not depend
on the new netdev_alloc_skb() API.
The reason to add a second allocator to the receive side is twofold:
1) it allows easy detection of the memory pressure / OOM situation;
2) it allows the receive path to be unbounded and go at full speed when
resources permit.
The choice of using the global memalloc reserve as a mempool makes that
the new allocator has to release pages as soon as possible; if we were
to hoard pages in the allocator the memalloc reserve would not get
replenished readily.
Version 2 had about 250 new lines of code, while v3 has close to 600, when
including the SROG code. And that while things should have become simpler.
So why use SROG instead of the old alloc_pages() based code? And why couldn't
you use a slightly modified SLOB instead of writing a new allocator?
It looks like overkill to me.
Of the 611 new lines about 150 are new comments.
Simpler yes, but also more complete; the old patches had serious issues
with the alternative allocation scheme.
As for why SROG, because trying to stick all the semantics needed for
all skb operations into the old approach was nasty, I had it almost
complete but it was horror (and more code than the SROG approach).
Why not SLOB, well, I mistakenly assumed that it was a simpler SLAB
allocator, my bad. However after having had a quick peek at it; whereas
it seems similar in intent it does not provide the things I look for.
Making it so and keep the old semantics and make it compile along side
of SLAB will take quite some effort.
I'd drop the memalloc_skbs_available() check, as that's already done by
___alloc_skb.
Right, thanks. Hmm, its the last occurence of that function, even
better.
quoted
+static DEFINE_SPINLOCK(memalloc_lock);
+static int memalloc_socks;
+static unsigned long memalloc_reserve;
Why is this a long? adjust_memalloc_reserve() takes an int.
Euhm, right :-) long comes naturaly when I think about quantities op
pages. The adjust_memalloc_reserve() argument is an increment, a delta;
perhaps I should change that to long.
Is it needed at all, considering var_free_kbytes already exists?
Having them separate would allow ajust_memalloc_reserve() to be used by
other callers too (would need some extra locking).
On Sat, August 12, 2006 19:44, Peter Zijlstra said:
Euhm, right :-) long comes naturaly when I think about quantities op
pages. The adjust_memalloc_reserve() argument is an increment, a delta;
perhaps I should change that to long.
Maybe, but having 16 TB of reserved memory seems plenty for a while.
Having them separate would allow ajust_memalloc_reserve() to be used by
other callers too (would need some extra locking).
True, but currently memalloc_reserve isn't used in a sensible way,
or I'm missing something.
Greetings,
Indan
From: Peter Zijlstra <hidden> Date: 2006-08-12 18:09:30
On Sat, 2006-08-12 at 19:54 +0200, Indan Zupancic wrote:
On Sat, August 12, 2006 19:44, Peter Zijlstra said:
quoted
Euhm, right :-) long comes naturaly when I think about quantities op
pages. The adjust_memalloc_reserve() argument is an increment, a delta;
perhaps I should change that to long.
Maybe, but having 16 TB of reserved memory seems plenty for a while.
Oh, for sure, but since it doesn't really matter all that much, I'd
rather go for proper.
quoted
Having them separate would allow ajust_memalloc_reserve() to be used by
other callers too (would need some extra locking).
True, but currently memalloc_reserve isn't used in a sensible way,
or I'm missing something.
Well, I'm somewhat reluctant to stick network related code into mm/, it
seems well separated now.
On Sat, August 12, 2006 19:33, Peter Zijlstra said:
Simpler yes, but also more complete; the old patches had serious issues
with the alternative allocation scheme.
It sure is more complete, and looks nicer, but the price is IMHO too high.
I'm curious what those serious issues are, and if they can't be fixed.
As for why SROG, because trying to stick all the semantics needed for
all skb operations into the old approach was nasty, I had it almost
complete but it was horror (and more code than the SROG approach).
What was missing or wrong in the old approach? Can't you use the new
approach, but use alloc_pages() instead of SROG?
Sorry if I bug you so, but I'm also trying to increase my knowledge here. ;-)
Greetings,
Indan
On Sat, August 12, 2006 20:08, Peter Zijlstra said:
On Sat, 2006-08-12 at 19:54 +0200, Indan Zupancic wrote:
quoted
True, but currently memalloc_reserve isn't used in a sensible way,
or I'm missing something.
Well, I'm somewhat reluctant to stick network related code into mm/, it
seems well separated now.
What I had in mind was something like:
+static DEFINE_SPINLOCK(memalloc_lock);
+static int memalloc_socks;
+
+atomic_t memalloc_skbs_used;
+EXPORT_SYMBOL_GPL(memalloc_skbs_used);
+
+int sk_adjust_memalloc(int nr_socks)
+{
+ unsigned long flags;
+ unsigned int reserve;
+ int err;
+
+ spin_lock_irqsave(&memalloc_lock, flags);
+
+ memalloc_socks += nr_socks;
+ BUG_ON(memalloc_socks < 0);
+
+ reserve = nr_socks * (2 * MAX_PHYS_SEGMENTS + /* outbound */
+ 5 * MAX_CONCURRENT_SKBS); /* inbound */
+
+ err = adjust_memalloc_reserve(reserve);
+ spin_unlock_irqrestore(&memalloc_lock, flags);
+ if (err) {
+ printk(KERN_WARNING
+ "Unable to change RX reserve to: %lu, error: %d\n",
+ reserve, err);
+ }
+ return err;
+}
The original code missed the brackets, so 5 * MAX_CONCURRENT_SKBS wasn't done
per socket. But the comment said it was per socket, so I added in this version.
Greetings,
Indan
From: Peter Zijlstra <hidden> Date: 2006-08-12 18:48:04
On Sat, 2006-08-12 at 20:32 +0200, Indan Zupancic wrote:
On Sat, August 12, 2006 20:08, Peter Zijlstra said:
quoted
On Sat, 2006-08-12 at 19:54 +0200, Indan Zupancic wrote:
quoted
True, but currently memalloc_reserve isn't used in a sensible way,
or I'm missing something.
Well, I'm somewhat reluctant to stick network related code into mm/, it
seems well separated now.
What I had in mind was something like:
+static DEFINE_SPINLOCK(memalloc_lock);
+static int memalloc_socks;
+
+atomic_t memalloc_skbs_used;
+EXPORT_SYMBOL_GPL(memalloc_skbs_used);
+
+int sk_adjust_memalloc(int nr_socks)
+{
+ unsigned long flags;
+ unsigned int reserve;
+ int err;
+
+ spin_lock_irqsave(&memalloc_lock, flags);
+
+ memalloc_socks += nr_socks;
+ BUG_ON(memalloc_socks < 0);
+
+ reserve = nr_socks * (2 * MAX_PHYS_SEGMENTS + /* outbound */
+ 5 * MAX_CONCURRENT_SKBS); /* inbound */
+
+ err = adjust_memalloc_reserve(reserve);
+ spin_unlock_irqrestore(&memalloc_lock, flags);
+ if (err) {
+ printk(KERN_WARNING
+ "Unable to change RX reserve to: %lu, error: %d\n",
+ reserve, err);
+ }
+ return err;
+}
The original code missed the brackets, so 5 * MAX_CONCURRENT_SKBS wasn't done
per socket. But the comment said it was per socket, so I added in this version.
Ah right, I did that in v3, with a similar comment, but I realised that
the inbound reserve need not be per socket, and that the comment was
ambiguous enough to allow this reading.
Why not per socket, its used to place an upper bound, its not
calculating one, its setting one.
Like you can see in memalloc_skbs_inc(), it limits to
MAX_CONCURRENT_SKBS.
From: Peter Zijlstra <hidden> Date: 2006-08-12 18:54:57
On Sat, 2006-08-12 at 20:16 +0200, Indan Zupancic wrote:
On Sat, August 12, 2006 19:33, Peter Zijlstra said:
quoted
Simpler yes, but also more complete; the old patches had serious issues
with the alternative allocation scheme.
It sure is more complete, and looks nicer, but the price is IMHO too high.
I'm curious what those serious issues are, and if they can't be fixed.
quoted
As for why SROG, because trying to stick all the semantics needed for
all skb operations into the old approach was nasty, I had it almost
complete but it was horror (and more code than the SROG approach).
What was missing or wrong in the old approach? Can't you use the new
approach, but use alloc_pages() instead of SROG?
Sorry if I bug you so, but I'm also trying to increase my knowledge here. ;-)
I'm almost sorry I threw that code out, you'd understand instantly..
Lemme see what I can do to explain; what I need/want is:
- single allocation group per packet - that is, when I free a packet
and all its associated object I get my memory back.
- not waste too much space managing the various objects
skb operations want to allocate various sk_buffs for the same data
clones. Also, it wants to be able to break the COW or realloc the data.
The trivial approach would be one page (or higher alloc page) per
object, and that will work quite well, except that it'll waste a _lot_
of memory.
So I tried manual packing (parts of that you have seen in previous
attempts). This gets hard when you want to do unlimited clones and COW
breaks. To do either you need to go link several pages.
So needing a list of pages and wanting packing gave me SROG. The biggest
wart is having to deal with higher order pages. Explicitly coding in
knowledge of the object you're packing just makes the code bigger - such
is the power of abstraction.
On Sat, August 12, 2006 20:47, Peter Zijlstra said:
Ah right, I did that in v3, with a similar comment, but I realised that
the inbound reserve need not be per socket, and that the comment was
ambiguous enough to allow this reading.
True, but better to change the comment than to confuse people.
Lots of it is outdated because reservations aren't per device anymore.
Changes to your version:
- Got rid of memalloc_socks.
- Don't include inetdevice.h (it isn't needed anymore, right?)
- Updated comment.
(I'm editing the diff, so this won't apply)
Index: linux-2.6/net/core/sock.c
===================================================================
@@ -195,6 +197,78 @@ __u32 sysctl_rmem_default = SK_RMEM_MAX;/* Maximal space eaten by iovec or ancilliary data plus some space */intsysctl_optmem_max=sizeof(unsignedlong)*(2*UIO_MAXIOV+512);+staticDEFINE_SPINLOCK(memalloc_lock);+staticintmemalloc_socks;+staticunsignedlongmemalloc_reserve;++atomic_tmemalloc_skbs_used;+EXPORT_SYMBOL_GPL(memalloc_skbs_used);++/**+*sk_adjust_memalloc-adjusttheglobalmemallocreserve+*@nr_socks:numberofnew%SOCK_MEMALLOCsockets+*+*Thisfunctionadjuststhememallocreservebasedonsystemdemand.+*Foreach%SOCK_MEMALLOCsocket2*%MAX_PHYS_SEGMENTSpagesare+*reservedforoutboundtraffic(assumption:each%SOCK_MEMALLOC+*socketwillhavea%request_queueassociated).+*+*Pagesforinboundtrafficarealreadyreserved.+*+*2*%MAX_PHYS_SEGMENTS-therequestqueuecanholdupto150%,+*theremaining50%goestobeingsurewecanwritepackets+*fortheoutgoingpages.+*/+staticDEFINE_SPINLOCK(memalloc_lock);+staticintmemalloc_socks;++atomic_tmemalloc_skbs_used;+EXPORT_SYMBOL_GPL(memalloc_skbs_used);++intsk_adjust_memalloc(intnr_socks)+{+unsignedlongflags;+unsignedintreserve;+interr;++spin_lock_irqsave(&memalloc_lock,flags);++memalloc_socks+=nr_socks;+BUG_ON(memalloc_socks<0);++reserve=nr_socks*2*MAX_PHYS_SEGMENTS;/* outbound */++err=adjust_memalloc_reserve(reserve);+spin_unlock_irqrestore(&memalloc_lock,flags);+if(err){+printk(KERN_WARNING+"Unable to adjust RX reserve by %lu, error: %d\n",+reserve,err);+}+returnerr;+}+EXPORT_SYMBOL_GPL(sk_adjust_memalloc);
What's missing now is an adjust_memalloc_reserve(5 * MAX_CONCURRENT_SKBS)
call in some init code.
Greetings,
Indan
On Sat, August 12, 2006 20:54, Peter Zijlstra said:
- single allocation group per packet - that is, when I free a packet
and all its associated object I get my memory back.
This is easy.
- not waste too much space managing the various objects
This too, when ignoring clones and COW.
skb operations want to allocate various sk_buffs for the same data
clones. Also, it wants to be able to break the COW or realloc the data.
So this seems to be what adds all the complexity.
So I tried manual packing (parts of that you have seen in previous
attempts). This gets hard when you want to do unlimited clones and COW
breaks. To do either you need to go link several pages.
It gets messy quite quickly, yes.
So needing a list of pages and wanting packing gave me SROG. The biggest
wart is having to deal with higher order pages. Explicitly coding in
knowledge of the object you're packing just makes the code bigger - such
is the power of abstraction.
I assume you meant "Not explicitly coding in", or else I'm tempted to disagree.
Abstraction that has only one user which uses it in one way only adds bloat.
But looking at the code a bit more I'm afraid you're right.
Greetings,
Indan
This symbol name has nothing to do with its purpose. The entire area of
code you are modifying could be described as having something to do with
'memalloc'.
GFP_EMERGENCY or GFP_USE_RESERVES or somesuch would be a far better
symbol name.
I recognize that is matches with GFP_NOMEMALLOC, but that doesn't change
the situation anyway. In fact, a cleanup patch to rename GFP_NOMEMALLOC
would be nice.
I'm rather bad at picking names, but here goes:
PF_MEMALLOC -> PF_EMERGALLOC
__GFP_NOMEMALLOC -> __GFP_NOEMERGALLOC
__GFP_MEMALLOC -> __GFP_EMERGALLOC
Is that suitable and shall I prepare patches? Or do we want more ppl to
chime in and have a few more rounds?
MEMALLOC is the name Linus chose to name exactly the reserve from which we
are allocating. Perhaps that was just Linus being denser than jgarzik and
not realizing that he should have called it EMERGALLOC right from the start.
BUT since Linus did call it MEMALLOC, we should too. Or just email Linus
and tell him how much better EMERGALLOC rolls off the tongue, and could we
please change all occurances of MEMALLOC to EMERGALLOC. Then don't read
your email for a week ;-)
Inventing a new name for an existing thing is very poor taste on grounds of
grepability alone.
Regards,
Daniel
From: Daniel Phillips <hidden> Date: 2006-08-14 00:43:11
Peter Zijlstra wrote:
On Sat, 2006-08-12 at 20:16 +0200, Indan Zupancic wrote:
quoted
What was missing or wrong in the old approach? Can't you use the new
approach, but use alloc_pages() instead of SROG?
Sorry if I bug you so, but I'm also trying to increase my knowledge here. ;-)
I'm almost sorry I threw that code out...
Good instinct :-)
Lemme see what I can do to explain; what I need/want is:
- single allocation group per packet - that is, when I free a packet
and all its associated object I get my memory back.
First, try to recast all your objects as pages, as Evgeniy Polyakov
suggested. Then if there is some place where that just doesn't work
(please point it out) put a mempool there and tweak the high level
reservation setup accordingly.
- not waste too much space managing the various objects
If we waste a little space only where the network would have otherwise
dropped a packet, that is still a big win. We just need to be sure the
normal path does not become more wasteful.
skb operations want to allocate various sk_buffs for the same data
clones. Also, it wants to be able to break the COW or realloc the data.
The trivial approach would be one page (or higher alloc page) per
object, and that will work quite well, except that it'll waste a _lot_
of memory.
High order allocations are just way too undependable without active
defragmentation, which isn't even on the horizon at the moment. We
just need to treat any network hardware that can't scatter/gather into
single pages as too broken to use for network block io.
As for sk_buff cow break, we need to look at which network paths do it
(netfilter obviously, probably others) and decide whether we just want
to declare that the feature breaks network block IO, or fix the feature
so it plays well with reserve accounting.
So I tried manual packing (parts of that you have seen in previous
attempts). This gets hard when you want to do unlimited clones and COW
breaks. To do either you need to go link several pages.
You need to avoid the temptation to fix the entire world on the first
attempt. Sure, there will be people who call for gobs of overengineering
right from the start, but simple has always worked better for Linux than
lathering on layers of complexity just to support some feature that may
arguably be broken by design. For example, swapping through a firewall.
Changing from per-interface to a global block IO reserve was a great
simplification, we need more of those.
Looking forward to -v5 ;-)
Regards,
Daniel
From: Paul Jackson <hidden> Date: 2006-08-14 01:01:32
Daniel wrote:
Inventing a new name for an existing thing is very poor taste on grounds of
grepability alone.
I wouldn't say 'very poor taste' -- just something that should be
done infrequently, with good reason, and with reasonable concensus,
especially from the key maintainers in the affected area.
Good names are good taste, in my book. But stable naming is good too.
I wonder what Nick thinks of this? Looks like he added
__GFP_NOMEMALLOC a year ago, following the naming style of PF_MEMALLOC.
I added him to the cc list.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson [off-list ref] 1.925.600.0401
From: Nick Piggin <hidden> Date: 2006-08-14 03:43:39
Paul Jackson wrote:
Daniel wrote:
quoted
Inventing a new name for an existing thing is very poor taste on grounds of
grepability alone.
I wouldn't say 'very poor taste' -- just something that should be
done infrequently, with good reason, and with reasonable concensus,
especially from the key maintainers in the affected area.
Good names are good taste, in my book. But stable naming is good too.
I wonder what Nick thinks of this? Looks like he added
__GFP_NOMEMALLOC a year ago, following the naming style of PF_MEMALLOC.
I added him to the cc list.
__GFP_NOMEMALLOC was added to prevent mempool backed allocations from
accessing the emergency reserve. Because that would just shift deadlocks
from mempool "safe" sites to those which have not been converted.
PF_MEMALLOC is a good name: PF_MEMALLOC says that the task is currently
allocating memory. It does not say anything about the actual allocator
implementation details to handle this (1. don't recurse into reclaim; 2.
allow access to reserves), but that is a good thing.
__GFP_NOMEMALLOC and __GFP_MEMALLOC are poorly named (I take the blame).
It isn't that the task is suddenly no longer allocating in the context
of an allocation, it is just that you want to allow or deny access to
the reserve.
__GFP_NOMEMALLOC should be something like __GFP_EMERG_NEVER and
__GFP_MEMALLOC should be _ALWAYS. Or something like that.
NOMEMALLOC is specific enough that I don't mind a rename at this stage.
Renaming PF_MEMALLOC would be wrong, however.
--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com
On Sun, Aug 13, 2006 at 05:42:47PM -0700, Daniel Phillips (phillips@google.com) wrote:
High order allocations are just way too undependable without active
defragmentation, which isn't even on the horizon at the moment. We
just need to treat any network hardware that can't scatter/gather into
single pages as too broken to use for network block io.
A bit of network tree allocator free advertisement - per-CPU self
defragmentation works reliably in that allocator, one could even find a
graphs of memory usage for NTA and SLAB-like allocator.
As for sk_buff cow break, we need to look at which network paths do it
(netfilter obviously, probably others) and decide whether we just want
to declare that the feature breaks network block IO, or fix the feature
so it plays well with reserve accounting.
I would suggest to consider skb cow (cloning) as a must.
From: Rik van Riel <hidden> Date: 2006-08-14 12:21:46
Evgeniy Polyakov wrote:
On Sun, Aug 13, 2006 at 05:42:47PM -0700, Daniel Phillips (phillips@google.com) wrote:
quoted
As for sk_buff cow break, we need to look at which network paths do it
(netfilter obviously, probably others) and decide whether we just want
to declare that the feature breaks network block IO, or fix the feature
so it plays well with reserve accounting.
I would suggest to consider skb cow (cloning) as a must.
That should not be any problem, since skb's (including cowed ones)
are short lived anyway. Allocating a little bit more memory is
fine when we have a guarantee that the memory will be freed again
shortly.
--
What is important? What you want to be true, or what is true?
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2006-08-14 12:52:20
Rik van Riel [off-list ref] wrote:
That should not be any problem, since skb's (including cowed ones)
are short lived anyway. Allocating a little bit more memory is
fine when we have a guarantee that the memory will be freed again
shortly.
From: Rik van Riel <hidden> Date: 2006-08-14 14:23:07
Herbert Xu wrote:
Rik van Riel [off-list ref] wrote:
quoted
That should not be any problem, since skb's (including cowed ones)
are short lived anyway. Allocating a little bit more memory is
fine when we have a guarantee that the memory will be freed again
shortly.
I'm not sure about the context the comment applies to, but skb's are
not necessarily short-lived. For example, they could be queued for
a few seconds for ARP/NDISC and even longer for IPsec SA resolution.
That's still below the threshold where it should cause problems
with the VM going OOM. Especially if there aren't too many of
these packets.
--
All Rights Reversed
From: Pavel Machek <hidden> Date: 2006-08-24 14:43:48
Hi!
Limit each request to 1 page, so that the request throttling also limits the
number of in-flight pages and force the IO scheduler to NOOP as anything else
doesn't make sense anyway.
I'd like to understand why it breaks with other schedulers before
merging this. Maybe the failure in NOOP is just harder to trigger?
Pavel
--
Thanks for all the (sleeping) penguins.