From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: Fwd: u32 ht filters Date: Mon, 12 Feb 2018 14:23:52 +0100 Message-ID: <20180212132352.GA2153@nanopsycho> References: <9c8f997d-339c-5088-0bb5-124e9f55f02d@itcare.pl> <20180207070148.GA2149@nanopsycho.orion> <20180208073846.GA2041@nanopsycho> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Jiri Pirko , Linux Kernel Network Developers , =?utf-8?B?UGF3ZcWC?= Staszewski To: Cong Wang Return-path: Received: from mail-wr0-f178.google.com ([209.85.128.178]:37317 "EHLO mail-wr0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934054AbeBLNXz (ORCPT ); Mon, 12 Feb 2018 08:23:55 -0500 Received: by mail-wr0-f178.google.com with SMTP id k32so8679583wrk.4 for ; Mon, 12 Feb 2018 05:23:54 -0800 (PST) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Sat, Feb 10, 2018 at 09:41:57PM CET, xiyou.wangcong@gmail.com wrote: >On Wed, Feb 7, 2018 at 11:38 PM, Jiri Pirko wrote: >> Thu, Feb 08, 2018 at 12:08:36AM CET, xiyou.wangcong@gmail.com wrote: >>>On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko wrote: >>>> Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangcong@gmail.com wrote: >>>>>Hi, Jiri >>>>> >>>>>Your commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c >>>>>breaks the tc script by Paweł. Please find below for details. >>>> >>>> Did you do the bisection? >>>> The commit just uses block struct instead of q, but since they >>>> are in 1:1 relation, that should be equvivalent. So basically you still >>>> have per-qdisc hashtables for u32. >>> >>>Well, at least the following fixes the problem here. But I am not sure >>>if it is expected too for shared block among multiple qdiscs. >> >> For shared block, block->q is null. > >According to this comment: >/* block_index not 0 means the shared block is requested */ > >and the code, > > if (!block) { > block = tcf_block_create(net, q, extack); > >block->q is set to q, and q is always non-NULL AFAIU. Yep, that is a bug. Fixing it now. > >Also, I don't know if it is intended, but block->q always points to >the parent qdisc rather than the qdisc attached to a class. You are right. That is incorrect. Fixing now. > > >>> >>> >>>@@ -338,7 +330,7 @@ static struct hlist_head *tc_u_common_hash; >>> >>> static unsigned int tc_u_hash(const struct tcf_proto *tp) >>> { >>>- return hash_ptr(tp->chain->block, U32_HASH_SHIFT); >>>+ return hash_ptr(tp->chain->block->q, U32_HASH_SHIFT); >>> } >>> >>> static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp) >>>@@ -348,7 +340,7 @@ static struct tc_u_common *tc_u_common_find(const >>>struct tcf_proto *tp) >>> >>> h = tc_u_hash(tp); >>> hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { >>>- if (tc->block == tp->chain->block) >>>+ if (tc->block->q == tp->chain->block->q) >> >> :O I don't get it. tc->block is pointer, tc->block->q is pointer. And >> they are different at the same time for non-shared block. > >If you look into Pawel's script, a new block is created for each class >therefore a different tc_u_common is created which causes the >ht 9:22 can't be found. Yeah :/ this tc_u_common thing is going to haunt me. This is resolvable now by using block->q. There is no support for block sharing for other qdiscs than ingress and clsact so we don't have to take care of the multi-class&block sharing now. Will fix. Thanks!