From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B6C13C433E0 for ; Wed, 17 Jun 2020 03:36:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9C2B820810 for ; Wed, 17 Jun 2020 03:36:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726728AbgFQDgj (ORCPT ); Tue, 16 Jun 2020 23:36:39 -0400 Received: from m9784.mail.qiye.163.com ([220.181.97.84]:54509 "EHLO m9784.mail.qiye.163.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726497AbgFQDgj (ORCPT ); Tue, 16 Jun 2020 23:36:39 -0400 Received: from [192.168.188.14] (unknown [106.75.220.2]) by m9784.mail.qiye.163.com (Hmail) with ESMTPA id 449FF41BA0; Wed, 17 Jun 2020 11:36:24 +0800 (CST) Subject: Re: [PATCH net v3 2/4] flow_offload: fix incorrect cb_priv check for flow_block_cb To: Pablo Neira Ayuso , Simon Horman Cc: netdev@vger.kernel.org, davem@davemloft.net, vladbu@mellanox.com References: <1592277580-5524-1-git-send-email-wenxu@ucloud.cn> <1592277580-5524-3-git-send-email-wenxu@ucloud.cn> <20200616105123.GA21396@netronome.com> <20200616143427.GA8084@netronome.com> <565dd609-1e20-16f4-f38d-8a0b15816f50@ucloud.cn> <20200616154716.GA16382@netronome.com> <20200616203834.GA27394@salvia> From: wenxu Message-ID: Date: Wed, 17 Jun 2020 11:36:19 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: <20200616203834.GA27394@salvia> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US X-HM-Spam-Status: e1kfGhgUHx5ZQUtXWQgYFAkeWUFZVklVSU5KS0tLSk1OQkhCSEhZV1koWU FJQjdXWS1ZQUlXWQ8JGhUIEh9ZQVkXIjULOBw4SEgdGk8RUBceSzg5QzocVlZVTUkoSVlXWQkOFx 4IWUFZNTQpNjo3JCkuNz5ZV1kWGg8SFR0UWUFZNDBZBg++ X-HM-Sender-Digest: e1kMHhlZQR0aFwgeV1kSHx4VD1lBWUc6Nz46Ggw4Pjg5SjYOKyI3HAxM DBcKCx9VSlVKTkJJSE1PQkNPTUtNVTMWGhIXVQweFQMOOw4YFxQOH1UYFUVZV1kSC1lBWUpLTVVM TlVJSUtVSVlXWQgBWUFOTU9KNwY+ X-HM-Tid: 0a72c05968b32086kuqy449ff41ba0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 6/17/2020 4:38 AM, Pablo Neira Ayuso wrote: > On Tue, Jun 16, 2020 at 05:47:17PM +0200, Simon Horman wrote: >> On Tue, Jun 16, 2020 at 11:18:16PM +0800, wenxu wrote: >>> 在 2020/6/16 22:34, Simon Horman 写道: >>>> On Tue, Jun 16, 2020 at 10:20:46PM +0800, wenxu wrote: >>>>> 在 2020/6/16 18:51, Simon Horman 写道: >>>>>> On Tue, Jun 16, 2020 at 11:19:38AM +0800, wenxu@ucloud.cn wrote: >>>>>>> From: wenxu >>>>>>> >>>>>>> In the function __flow_block_indr_cleanup, The match stataments >>>>>>> this->cb_priv == cb_priv is always false, the flow_block_cb->cb_priv >>>>>>> is totally different data with the flow_indr_dev->cb_priv. >>>>>>> >>>>>>> Store the representor cb_priv to the flow_block_cb->indr.cb_priv in >>>>>>> the driver. >>>>>>> >>>>>>> Fixes: 1fac52da5942 ("net: flow_offload: consolidate indirect flow_block infrastructure") >>>>>>> Signed-off-by: wenxu >>>>>> Hi Wenxu, >>>>>> >>>>>> I wonder if this can be resolved by using the cb_ident field of struct >>>>>> flow_block_cb. >>>>>> >>>>>> I observe that mlx5e_rep_indr_setup_block() seems to be the only call-site >>>>>> where the value of the cb_ident parameter of flow_block_cb_alloc() is >>>>>> per-block rather than per-device. So part of my proposal is to change >>>>>> that. >>>>> I check all the xxdriver_indr_setup_block. It seems all the cb_ident parameter of >>>>> >>>>> flow_block_cb_alloc is per-block. Both in the nfp_flower_setup_indr_tc_block >>>>> >>>>> and bnxt_tc_setup_indr_block. >>>>> >>>>> >>>>> nfp_flower_setup_indr_tc_block: >>>>> >>>>> struct nfp_flower_indr_block_cb_priv *cb_priv; >>>>> >>>>> block_cb = flow_block_cb_alloc(nfp_flower_setup_indr_block_cb, >>>>>                                                cb_priv, cb_priv, >>>>>                                                nfp_flower_setup_indr_tc_release); >>>>> >>>>> >>>>> bnxt_tc_setup_indr_block: >>>>> >>>>> struct bnxt_flower_indr_block_cb_priv *cb_priv; >>>>> >>>>> block_cb = flow_block_cb_alloc(bnxt_tc_setup_indr_block_cb, >>>>>                                                cb_priv, cb_priv, >>>>>                                                bnxt_tc_setup_indr_rel); >>>>> >>>>> >>>>> And the function flow_block_cb_is_busy called in most place. Pass the >>>>> >>>>> parameter as cb_priv but not cb_indent . >>>> Thanks, I see that now. But I still think it would be useful to understand >>>> the purpose of cb_ident. It feels like it would lead to a clean solution >>>> to the problem you have highlighted. >>> I think The cb_ident means identify.  It is used to identify the each flow block cb. >>> >>> In the both flow_block_cb_is_busy and flow_block_cb_lookup function check >>> >>> the block_cb->cb_ident == cb_ident. >> Thanks, I think that I now see what you mean about the different scope of >> cb_ident and your proposal to allow cleanup by flow_indr_dev_unregister(). >> >> I do, however, still wonder if there is a nicer way than reaching into >> the structure and manually setting block_cb->indr.cb_priv >> at each call-site. >> >> Perhaps a variant of flow_block_cb_alloc() for indirect blocks >> would be nicer? > A follow up patch to add this new variant would be good. Probably > __flow_block_indr_binding() can go away with this new variant to set > up the indirect flow block. Maybe __flow_block_indr_binding() can't go away. The data and cleanup callback which should init the flow_block_indr is only in the flow_indr_dev_setup_offload. This can't be gotten in flow_indr_block_cb_alloc. >