From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932631AbdC1VyC (ORCPT ); Tue, 28 Mar 2017 17:54:02 -0400 Received: from mail-yw0-f193.google.com ([209.85.161.193]:36109 "EHLO mail-yw0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932591AbdC1VyA (ORCPT ); Tue, 28 Mar 2017 17:54:00 -0400 Date: Tue, 28 Mar 2017 17:53:29 -0400 From: Tejun Heo To: Jens Axboe Cc: Tahsin Erdogan , linux-block@vger.kernel.org, David Rientjes , linux-kernel@vger.kernel.org Subject: Re: [PATCH] blkcg: allocate struct blkcg_gq outside request queue spinlock Message-ID: <20170328215329.GF28157@htj.duckdns.org> References: <20170324215627.12831-1-tahsin@google.com> <5b9ab59c-4121-d49a-1dc5-bd419f3ac94f@kernel.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5b9ab59c-4121-d49a-1dc5-bd419f3ac94f@kernel.dk> User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 24, 2017 at 04:04:32PM -0600, Jens Axboe wrote: > On 03/24/2017 03:56 PM, Tahsin Erdogan wrote: > > blkg_conf_prep() currently calls blkg_lookup_create() while holding > > request queue spinlock. This means allocating memory for struct > > blkcg_gq has to be made non-blocking. This causes occasional -ENOMEM > > failures in call paths like below: > > > > pcpu_alloc+0x68f/0x710 > > __alloc_percpu_gfp+0xd/0x10 > > __percpu_counter_init+0x55/0xc0 > > cfq_pd_alloc+0x3b2/0x4e0 > > blkg_alloc+0x187/0x230 > > blkg_create+0x489/0x670 > > blkg_lookup_create+0x9a/0x230 > > blkg_conf_prep+0x1fb/0x240 > > __cfqg_set_weight_device.isra.105+0x5c/0x180 > > cfq_set_weight_on_dfl+0x69/0xc0 > > cgroup_file_write+0x39/0x1c0 > > kernfs_fop_write+0x13f/0x1d0 > > __vfs_write+0x23/0x120 > > vfs_write+0xc2/0x1f0 > > SyS_write+0x44/0xb0 > > entry_SYSCALL_64_fastpath+0x18/0xad > > > > In the code path above, percpu allocator cannot call vmalloc() due to > > queue spinlock. > > > > A failure in this call path gives grief to tools which are trying to > > configure io weights. We see occasional failures happen shortly after > > reboots even when system is not under any memory pressure. Machines > > with a lot of cpus are more vulnerable to this condition. > > > > Do struct blkcg_gq allocations outside the queue spinlock to allow > > blocking during memory allocations. > > This looks much simpler/cleaner to me, compared to v5. Tejun, what do > you think? So, this patch in itself looks better but now we end up with two separate mechanisms to handle non-atomic allocations. This drop lock / alloc / relock / check invariants in the main path and preallocation logic used in the init path. Right now, both proposed implementations aren't that satisfactory. Personally, I'd prefer superficial ugliness to structural duplications, but, ideally, we shouldn't have to make this choice. idk, it's a bug fix. We can always clean things up later. Acked-by: Tejun Heo Thanks. -- tejun