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=-2.4 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 D8169C433FF for ; Tue, 6 Aug 2019 18:38:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9FC5520818 for ; Tue, 6 Aug 2019 18:38:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=ziepe.ca header.i=@ziepe.ca header.b="DzULNXQE" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728558AbfHFSi0 (ORCPT ); Tue, 6 Aug 2019 14:38:26 -0400 Received: from mail-qk1-f196.google.com ([209.85.222.196]:42150 "EHLO mail-qk1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728189AbfHFSiY (ORCPT ); Tue, 6 Aug 2019 14:38:24 -0400 Received: by mail-qk1-f196.google.com with SMTP id 201so63666571qkm.9 for ; Tue, 06 Aug 2019 11:38:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ziepe.ca; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=t0FZaLhTQo8X5m9sJH3jai3ugnitzUTUhXxicv7YDk8=; b=DzULNXQEJNZwg345/aqx14lziCowwmZZ111AycKfFi5XM1I301Ud8rtZZt1wgeF5Pl UU6wbBcMywCdYe2A00lP3rFFRmRHkverVEso5c/xpaLDR8J1XMlG7KG83Vs1L8jQdWhi o82MwcEtBdtL0DJveIufG1vt+LkvzW6Fe5LtJGtcF3gNTJmIummsJdbcVlAV3fyEpO46 6VVPQLT6TIe9NED3IcRJk8e+lTOltkPSvmuu6jEvNkJHdRpRrIsHpypmQJKpFVqidOGc 9UEGDh2qogUsM6aedOr0W515t9WTNWR1c68j4XvUSFM/riUM3zuVVTRliMQEO0XOPTUX EA3Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=t0FZaLhTQo8X5m9sJH3jai3ugnitzUTUhXxicv7YDk8=; b=pgo3ddvifrbVhe6MR3J8dsr8xp3TR6TRKLr6CPepscfgF1i/IxAN5VtDzwbV96Ujsk iO4pISQ6bOXm96oQKRvvWZZysudpBVnBw4BqnD5NEeEhH5dclEbPpct1vRwcwZ9p5VJ1 G+sSV5/ZrLnsFLbw4FTO5hgGATxNNqJwy4/SvM5bgBa7zYuZd0U1q4LV5dnm9DfxPzsh dbu2/X5aZfdqMoQYkcTcF9bzmqqWH79pQpY36/AO6H1AT0XtJ1diZnyio5V9woCYcx5S HS+RTWv/7WMy+CDk2dSIcdiolu1mCXQr4Dl+bUFyIpNm06IJ6kJ4J7Au1DO6XVMwlvtQ QXjw== X-Gm-Message-State: APjAAAUfqd9jTMs86yqm/N5/VgMObohmxIC61kHLeaAUJNXXr0npXEHE Y533o/mMrZSabTiZv0oCUmUNBg== X-Google-Smtp-Source: APXvYqyTCcAt6GJ8H61tbGk2kUskSoko6Ijz7dJpmZwAc8BT25Kjo621gYUI55QkHQ/0656a/D+FUw== X-Received: by 2002:a05:620a:124f:: with SMTP id a15mr4630085qkl.173.1565116702615; Tue, 06 Aug 2019 11:38:22 -0700 (PDT) Received: from ziepe.ca (hlfxns017vw-156-34-55-100.dhcp-dynamic.fibreop.ns.bellaliant.net. [156.34.55.100]) by smtp.gmail.com with ESMTPSA id d141sm37790010qke.3.2019.08.06.11.38.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 06 Aug 2019 11:38:21 -0700 (PDT) Received: from jgg by mlx.ziepe.ca with local (Exim 4.90_1) (envelope-from ) id 1hv4Ll-0004NN-6J; Tue, 06 Aug 2019 15:38:21 -0300 Date: Tue, 6 Aug 2019 15:38:21 -0300 From: Jason Gunthorpe To: Leon Romanovsky Cc: Saeed Mahameed , "hslester96@gmail.com" , "davem@davemloft.net" , "netdev@vger.kernel.org" , "linux-rdma@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount Message-ID: <20190806183821.GR11627@ziepe.ca> References: <20190802164828.20243-1-hslester96@gmail.com> <20190804125858.GJ4832@mtr-leonro.mtl.com> <20190805061320.GN4832@mtr-leonro.mtl.com> <20190806065958.GQ4832@mtr-leonro.mtl.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190806065958.GQ4832@mtr-leonro.mtl.com> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org On Tue, Aug 06, 2019 at 09:59:58AM +0300, Leon Romanovsky wrote: > On Mon, Aug 05, 2019 at 08:06:36PM +0000, Saeed Mahameed wrote: > > On Mon, 2019-08-05 at 14:55 +0800, Chuhong Yuan wrote: > > > On Mon, Aug 5, 2019 at 2:13 PM Leon Romanovsky > > > wrote: > > > > On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote: > > > > > On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky > > > > > wrote: > > > > > > On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote: > > > > > > > refcount_t is better for reference counters since its > > > > > > > implementation can prevent overflows. > > > > > > > So convert atomic_t ref counters to refcount_t. > > > > > > > > > > > > I'm not thrilled to see those automatic conversion patches, > > > > > > especially > > > > > > for flows which can't overflow. There is nothing wrong in using > > > > > > atomic_t > > > > > > type of variable, do you have in mind flow which will cause to > > > > > > overflow? > > > > > > > > > > > > Thanks > > > > > > > > > > I have to say that these patches are not done automatically... > > > > > Only the detection of problems is done by a script. > > > > > All conversions are done manually. > > > > > > > > Even worse, you need to audit usage of atomic_t and replace there > > > > it can overflow. > > > > > > > > > I am not sure whether the flow can cause an overflow. > > > > > > > > It can't. > > > > > > > > > But I think it is hard to ensure that a data path is impossible > > > > > to have problems in any cases including being attacked. > > > > > > > > It is not data path, and I doubt that such conversion will be > > > > allowed > > > > in data paths without proving that no performance regression is > > > > introduced. > > > > > So I think it is better to do this minor revision to prevent > > > > > potential risk, just like we have done in mlx5/core/cq.c. > > > > > > > > mlx5/core/cq.c is a different beast, refcount there means actual > > > > users > > > > of CQ which are limited in SW, so in theory, they have potential > > > > to be overflown. > > > > > > > > It is not the case here, there your are adding new port. > > > > There is nothing wrong with atomic_t. > > > > > > > > > > Thanks for your explanation! > > > I will pay attention to this point in similar cases. > > > But it seems that the semantic of refcount is not always as clear as > > > here... > > > > > > > Semantically speaking, there is nothing wrong with moving to refcount_t > > in the case of vxlan ports.. it also seems more accurate and will > > provide the type protection, even if it is not necessary. Please let me > > know what is the verdict here, i can apply this patch to net-next-mlx5. > > There is no verdict here, it is up to you., if you like code churn, go > for it. IMHO CONFIG_REFCOUNT_FULL is a valuable enough reason to not open code with an atomic even if overflow is not possible. Races resulting in incrs on 0 refcounts is a common enough mistake. Jason