On Fri 02-12-16 09:54:17, Anatoly Stepanov wrote:
> Alex, Vlasimil, Michal, thanks for your responses!
>
> On Fri, Dec 02, 2016 at 10:19:33AM +0100, Michal Hocko wrote:
> > Thanks for CCing me Vlastimil
> >
> > On Fri 02-12-16 09:44:23, Vlastimil Babka wrote:
> > > On 12/01/2016 02:16 AM, Anatoly Stepanov wrote:
> > > > As memcg array size can be up to:
> > > > sizeof(struct memcg_cache_array) + kmemcg_id * sizeof(void *);
> > > >
> > > > where kmemcg_id can be up to MEMCG_CACHES_MAX_SIZE.
> > > >
> > > > When a memcg instance count is large enough it can lead
> > > > to high order allocations up to order 7.
> >
> > This is definitely not nice and worth fixing! I am just wondering
> > whether this is something you have encountered in the real life. Having
> > thousands of memcgs sounds quite crazy^Wscary to me. I am not at all
> > sure we are prepared for that and some controllers would have real
> > issues with it AFAIR.
>
> In our company we use custom-made lightweight container technology, the thing is
> we can have up to several thousands of them on a server.
> So those high-order allocations were observed on a real production workload.
OK, this is interesting. Definitely worth mentioning in the changelog!
[...]
> > /*
> > * Do not invoke OOM killer for larger requests as we can fall
> > * back to the vmalloc
> > */
> > if (size > PAGE_SIZE)
> > gfp_mask |= __GFP_NORETRY | __GFP_NOWARN;
>
> I think we should check against PAGE_ALLOC_COSTLY_ORDER anyway, as
> there's no big need to allocate large contiguous chunks here, at the
> same time someone in the kernel might really need them.
PAGE_ALLOC_COSTLY_ORDER is and should remain the page allocator internal
implementation detail and shouldn't spread out much outside. GFP_NORETRY
will already make sure we do not push hard here.
>
> >
> > ret = kzalloc(size, gfp_mask);
> > if (ret)
> > return ret;
> > return vzalloc(size);
> >
>
> > I also do not like memcg_alloc helper name. It suggests we are
> > allocating a memcg while it is used for cache arrays and slab LRUS.
> > Anyway this pattern is quite widespread in the kernel so I would simply
> > suggest adding kvmalloc function instead.
>
> Agreed, it would be nice to have a generic call.
> I would suggest an impl. like this:
>
> void *kvmalloc(size_t size)
gfp_t gfp_mask should be a parameter as this should be a generic helper.
> {
> gfp_t gfp_mask = GFP_KERNEL;
> void *ret;
>
> if (size > PAGE_SIZE)
> gfp_mask |= __GFP_NORETRY | __GFP_NOWARN;
>
>
> if (size <= (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER)) {
> ret = kzalloc(size, gfp_mask);
> if (ret)
> return ret;
> }
No, please just do as suggested above. Tweak the gfp_mask for higher
order requests and do kmalloc first with vmalloc as a fallback.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>