From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vasily Averin Subject: Re: [PATCH v3 08/16] memcg: enable accounting of ipc resources Date: Sat, 24 Apr 2021 14:17:50 +0300 Message-ID: References: <4ed65beb-bda3-1c93-fadf-296b760a32b2@virtuozzo.com> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=virtuozzo.com; s=relay; h=Content-Type:MIME-Version:Date:Message-ID:From: Subject; bh=UXNNUoARoOvWpr054pjURR+Eo7jDmpH/x3SEVXjHYTY=; b=Lgaj2Nk/GvJVU6saH HCLOMYFtPDUPzRqUgGaeQNDlqnyl7+1ggqETMO2v1EszcBjPZ25oMERu1gM4e/LAYHKM4lgGVFAqc Jn3JWjdmzDxkkNZ09FnSdSGxrmz8yx9Pgvc3LFaGTPtnbVfO8cgzRoLGfQn7GOhPonqWZOvKD7Eks =; In-Reply-To: Content-Language: en-US List-ID: Content-Type: text/plain; charset="us-ascii" To: Michal Hocko Cc: Alexey Dobriyan , cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Shakeel Butt , Johannes Weiner , Vladimir Davydov , Roman Gushchin , Andrew Morton , Dmitry Safonov <0x7f454c46-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> On 4/23/21 4:49 PM, Michal Hocko wrote: > On Fri 23-04-21 15:40:58, Michal Hocko wrote: >> On Fri 23-04-21 15:32:01, Vasily Averin wrote: >>> On 4/23/21 3:16 PM, Alexey Dobriyan wrote: >>>> On Thu, Apr 22, 2021 at 01:37:02PM +0300, Vasily Averin wrote: >>>>> When user creates IPC objects it forces kernel to allocate memory for >>>>> these long-living objects. >>>>> >>>>> It makes sense to account them to restrict the host's memory consumption >>>>> from inside the memcg-limited container. >>>>> >>>>> This patch enables accounting for IPC shared memory segments, messages >>>>> semaphores and semaphore's undo lists. >>>> >>>>> --- a/ipc/msg.c >>>>> +++ b/ipc/msg.c >>>>> @@ -147,7 +147,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) >>>>> key_t key = params->key; >>>>> int msgflg = params->flg; >>>>> >>>>> - msq = kvmalloc(sizeof(*msq), GFP_KERNEL); >>>>> + msq = kvmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT); >>>> >>>> Why this requires vmalloc? struct msg_queue is not big at all. >>>> >>>>> --- a/ipc/shm.c >>>>> +++ b/ipc/shm.c >>>>> @@ -619,7 +619,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) >>>>> ns->shm_tot + numpages > ns->shm_ctlall) >>>>> return -ENOSPC; >>>>> >>>>> - shp = kvmalloc(sizeof(*shp), GFP_KERNEL); >>>>> + shp = kvmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT); >>>> >>>> Same question. >>>> Kmem caches can be GFP_ACCOUNT by default. >>> >>> It is side effect: previously all these objects was allocated via ipc_alloc/ipc_alloc_rcu >>> function called kvmalloc inside. >>> >>> Should I replace it to kmalloc right in this patch? >> >> I would say those are two independent things. I would agree that >> kvmalloc is bogus here. The allocation would try SLAB allocator first >> but if it fails (as kvmalloc doesn't try really hard) then it would >> fragment memory without a good reason which looks like a bug to me. > > I have dug into history and this all seems to be just code pattern > copy&paste thing. In the past it was ipc_alloc_rcu which was a common > code for all IPCs and that code was conditional on rcu_use_vmalloc > (HDRLEN_KMALLOC + size > PAGE_SIZE). Later changed to kvmalloc and then > helper removed and all users to use kvmalloc. It seems that only > semaphores can grow large enough to care about kvmalloc though. I have one more cleanup for ipc, so if no one objects, I'll fix this case, add my patch and send them together as a separate patch set a bit later. Thank you, Vasily Averin