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,URIBL_BLOCKED, 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 40605C433E1 for ; Wed, 17 Jun 2020 10:10:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 264E62053B for ; Wed, 17 Jun 2020 10:10:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726541AbgFQKKN (ORCPT ); Wed, 17 Jun 2020 06:10:13 -0400 Received: from m9784.mail.qiye.163.com ([220.181.97.84]:33174 "EHLO m9784.mail.qiye.163.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725536AbgFQKKK (ORCPT ); Wed, 17 Jun 2020 06:10:10 -0400 Received: from [192.168.188.14] (unknown [106.75.220.2]) by m9784.mail.qiye.163.com (Hmail) with ESMTPA id 5BAAC41FAC; Wed, 17 Jun 2020 18:10:00 +0800 (CST) Subject: Re: [PATCH net v3 2/4] flow_offload: fix incorrect cb_priv check for flow_block_cb To: Pablo Neira Ayuso Cc: Simon Horman , 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> <20200617083817.GA1744@salvia> From: wenxu Message-ID: <3d770837-e316-e12e-e455-8a23077cf3ae@ucloud.cn> Date: Wed, 17 Jun 2020 18:09:34 +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: <20200617083817.GA1744@salvia> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US X-HM-Spam-Status: e1kfGhgUHx5ZQUtXWQgYFAkeWUFZVkpVSEhNS0tLS0NPQ0xMS0pZV1koWU FJQjdXWS1ZQUlXWQ8JGhUIEh9ZQVkXIjULOBw*M04kHU9NUBceOU4hUDocVlZVSEsoSVlXWQkOFx 4IWUFZNTQpNjo3JCkuNz5ZV1kWGg8SFR0UWUFZNDBZBg++ X-HM-Sender-Digest: e1kMHhlZQR0aFwgeV1kSHx4VD1lBWUc6NlE6LSo4HTg8LzVCEVZKIikK LUMwChZVSlVKTkJJSENDTUtLTUlCVTMWGhIXVQweFQMOOw4YFxQOH1UYFUVZV1kSC1lBWUpLTVVM TlVJSUtVSVlXWQgBWUFNSEpONwY+ X-HM-Tid: 0a72c1c1c2c62086kuqy5baac41fac Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 6/17/2020 4:38 PM, Pablo Neira Ayuso wrote: > On Wed, Jun 17, 2020 at 11:36:19AM +0800, wenxu wrote: >> 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. > Probably flow_indr_block_bind_cb_t can be updated to include the data > and the cleanup callback. Yes this can setup the indr_block info in the flow_indr_block_cb_alloc. it also needs a flow_indr_block_cb_remove to handle the UNBIND setup. >