Re: [PATCH 33/38] act_api: Convert action_idr to XArray
From: Matthew Wilcox <willy@infradead.org>
Date: 2019-08-21 20:35:16
On Wed, Aug 21, 2019 at 07:41:19PM +0000, Vlad Buslov wrote:
quoted
@@ -301,18 +292,18 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb, if (nla_put_string(skb, TCA_KIND, ops->kind)) goto nla_put_failure; - mutex_lock(&idrinfo->lock); - idr_for_each_entry_ul(idr, p, tmp, id) { + xa_lock(&idrinfo->actions); + xa_for_each(&idrinfo->actions, index, p) { ret = tcf_idr_release_unsafe(p);I like the simplification of reusing builtin xarray spinlock for this, but the reason we are using mutex here is because the following call chain: tcf_idr_release_unsafe() -> tcf_action_cleanup() -> free_tcf() -> tcf_chain_put_by_act() -> __tcf_chain_put() -> mutex_lock(&block->lock);
Ah, this one is buggy anyway because we call xa_erase() inside the loop, so it'll deadlock. We could just drop the xa_lock() / xa_unlock() around the loop. There's no need to hold the lock unless we're trying to prevent other simultaneous additions/deletions, which shouldn't be happening? ie this:
@@ -268,8 +268,10 @@ static int tcf_idr_release_unsafe(struct tc_action *p) if (atomic_read(&p->tcfa_bindcnt) > 0) return -EPERM; - if (refcount_dec_and_test(&p->tcfa_refcnt)) { - xa_erase(&p->idrinfo->actions, p->tcfa_index); + if (refcount_dec_and_lock(&p->tcfa_refcnt, + &p->idrinfo->actions.xa_lock)) { + __xa_erase(&p->idrinfo->actions, p->tcfa_index); + xa_unlock(&p->idrinfo->actions); tcf_action_cleanup(p); return ACT_P_DELETED; }
@@ -292,18 +294,15 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb, if (nla_put_string(skb, TCA_KIND, ops->kind)) goto nla_put_failure; - xa_lock(&idrinfo->actions); xa_for_each(&idrinfo->actions, index, p) { ret = tcf_idr_release_unsafe(p); if (ret == ACT_P_DELETED) { module_put(ops->owner); n_i++; } else if (ret < 0) { - xa_unlock(&idrinfo->actions); goto nla_put_failure; } } - xa_unlock(&idrinfo->actions); if (nla_put_u32(skb, TCA_FCNT, n_i)) goto nla_put_failure;