Message ID | 57634687.3050107@mojatatu.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, Jun 16, 2016 at 5:38 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 16-06-16 05:43 PM, Cong Wang wrote: >> >> On Thu, Jun 16, 2016 at 1:50 PM, Alexey Khoroshilov >> <khoroshilov@ispras.ru> 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? You at least miss the unlock in load_metaops_and_vet()? I think we can just remove that tcf_lock, I am testing a patch now.
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);