From patchwork Fri Jun 17 00:38:31 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jamal Hadi Salim X-Patchwork-Id: 636734 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3rW1bK3jmSz9t20 for ; Fri, 17 Jun 2016 10:39:09 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=mojatatu-com.20150623.gappssmtp.com header.i=@mojatatu-com.20150623.gappssmtp.com header.b=XixYX7fd; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755044AbcFQAin (ORCPT ); Thu, 16 Jun 2016 20:38:43 -0400 Received: from mail-io0-f194.google.com ([209.85.223.194]:32971 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754990AbcFQAik (ORCPT ); Thu, 16 Jun 2016 20:38:40 -0400 Received: by mail-io0-f194.google.com with SMTP id 5so8731493ioy.0 for ; Thu, 16 Jun 2016 17:38:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mojatatu-com.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to; bh=JUYGwittuDGgl6a8SVOy5XZNak46eJiDHIV0ETsRMR4=; b=XixYX7fdI8/ZZYFLb1nyBn3HY3Jiu5yHvYtLBJxrfnxTsDA6PoUTHOyCtkl6iOr0wm TfTIFO+pDpw7i2CJF9U9RkKg+MOWQ1i9kzGXZoDVA4y6cQO21cORAMEQqrV20pImpLh+ oN7Ix1ZkH4h5qJ/RgbzFkIRok5hK9i3ZPGtgNiSLgRf89xFNeLbIT8pRvt6sXcxVcT9E TKcTnP6MkeT6P+sBsLWVBySKy2XfS7/leOdV9IE+RwEeC89YBTSJrqJZAEiaUHv93/5O eCeGs5njy7hWWzuYw59J/nO7VHsG0DZ7sndrKCkBAUcZUvx+CPYNh6VH8H1XUjzqC5DA Y39A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to; bh=JUYGwittuDGgl6a8SVOy5XZNak46eJiDHIV0ETsRMR4=; b=EBRdqHlBMs59YdFjlPeXpRyajigbrtIlLSAaXGqK54PhS8UlQO/P/fGvUQEM/ksgwT awLtF2H7v12K286qSgPCiV0tWhZgIOeH8YHuEnAZdhoHCzV1EXnu8c2LDJXYyghpWDQR 0jlwBaZZDhrgn7l+/LJzV9Otjze16/LGiF9YJ+aMiaimupmATltbUwUD26LU6vdFiTvl 8oXm0lvABLfslPC5GKvF5mLFWXyDJ8IhjXVvEy5u83MYKlO1KaERlrMZ5uG1jUuPtqoJ oQtjD7uLC1Z+rhhOPvbm+13UHA2zaDgmTRrnhXmv+gm60tVd5+2lGjtwh+YleCS7xLss HwXg== X-Gm-Message-State: ALyK8tKm5a41MPcxuQ+7doYpznFdNGIwejNGfxHf7B4T158QPw3mCenzVlH718dllT5mPg== X-Received: by 10.107.27.131 with SMTP id b125mr12242405iob.163.1466123919994; Thu, 16 Jun 2016 17:38:39 -0700 (PDT) Received: from [10.0.0.10] ([23.233.30.50]) by smtp.googlemail.com with ESMTPSA id 41sm3829774iod.43.2016.06.16.17.38.38 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 16 Jun 2016 17:38:39 -0700 (PDT) Subject: Re: [BUG] act_ife: sleeping functions called in atomic context To: Cong Wang , Alexey Khoroshilov References: <1466110219-4825-1-git-send-email-khoroshilov@ispras.ru> Cc: "David S. Miller" , Linux Kernel Network Developers , LKML , ldv-project@linuxtesting.org From: Jamal Hadi Salim Message-ID: <57634687.3050107@mojatatu.com> Date: Thu, 16 Jun 2016 20:38:31 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 16-06-16 05:43 PM, Cong Wang wrote: > On Thu, Jun 16, 2016 at 1:50 PM, Alexey Khoroshilov > wrote: >> tcf_ife_init() contains a big chunk of code executed with >> ife->tcf_lock spinlock held. But that code contains several calls >> to sleeping functions: >> populate_metalist() and use_all_metadata() >> -> add_metainfo() >> -> find_ife_oplist(metaid) >> -> read_lock() >> -> try_module_get(o->owner) >> -> kzalloc(sizeof(*mi), GFP_KERNEL); > > Hmm, we don't need to hold that spinlock when we create a new ife action, > since we haven't inserted it yet. We do need it when we modify an existing > one. So I am thinking if we can refactor that code to avoid spinlock > whenever possible. > Does attached (compile tested) patch help? >> -> ops->alloc(mi, metaval); >> -> module_put(ops->owner); >> _tcf_ife_cleanup() >> -> module_put() >> >> The same problem is actual for tcf_ife_cleanup() as well. >> > > Huh? Both module_put() and kfree() should not sleep, right? > I dont think there is any sleeping there ;-> cheers, jamal diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c index 6bbc518..e341bef 100644 --- a/net/sched/act_ife.c +++ b/net/sched/act_ife.c @@ -302,7 +302,9 @@ static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval, } } + spin_lock_bh(&ife->tcf_lock); list_add_tail(&mi->metalist, &ife->metalist); + spin_unlock_bh(&ife->tcf_lock); return ret; } @@ -474,7 +476,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla, saddr = nla_data(tb[TCA_IFE_SMAC]); } - spin_lock_bh(&ife->tcf_lock); ife->tcf_action = parm->action; if (parm->flags & IFE_ENCODE) { @@ -504,7 +505,6 @@ metadata_parse_err: if (ret == ACT_P_CREATED) _tcf_ife_cleanup(a, bind); - spin_unlock_bh(&ife->tcf_lock); return err; } @@ -523,13 +523,10 @@ metadata_parse_err: if (ret == ACT_P_CREATED) _tcf_ife_cleanup(a, bind); - spin_unlock_bh(&ife->tcf_lock); return err; } } - spin_unlock_bh(&ife->tcf_lock); - if (ret == ACT_P_CREATED) tcf_hash_insert(tn, a);