Hi Cong,
Thanks for reviewing!
On Fri 15 Feb 2019 at 22:21, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> (Sorry for joining this late.)
>
> On Mon, Feb 11, 2019 at 12:56 AM Vlad Buslov <vla...@mellanox.com> wrote:
>> @@ -2432,7 +2474,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct
>> netlink_callback *cb)
>> index_start = cb->args[0];
>> index = 0;
>>
>> - list_for_each_entry(chain, &block->chain_list, list) {
>> + for (chain = __tcf_get_next_chain(block, NULL);
>> + chain;
>> + chain_prev = chain,
>> + chain = __tcf_get_next_chain(block, chain),
>> + tcf_chain_put(chain_prev)) {
>
> Why do you want to take the block->lock in each iteration
> of the loop rather than taking once for the whole loop?
This loop calls classifier ops callback in tc_chain_fill_node(). I don't
call any classifier ops callbacks while holding block or chain lock in
this change because the goal is to achieve fine-grained locking for data
structures used by filter update path. Locking per-block or per-chain is
much coarser than taking reference counters to parent structures and
allowing classifiers to implement their own locking.
In this case call to ops->tmplt_dump() is probably quite fast and its
execution time doesn't depend on number of filters on the classifier, so
releasing block->lock on each iteration doesn't provide much benefit, if
at all. However, it is easier for me to reason about locking correctness
in this refactoring by following a simple rule that no locks (besides
rtnl mutex) can be held when calling classifier ops callbacks.
