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=-8.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 3ECFDC433FF for ; Mon, 5 Aug 2019 06:13:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 18CE6206C1 for ; Mon, 5 Aug 2019 06:13:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1564985610; bh=DEza7LipQz/yKwMh3a7hoO43mdHVY4vPsu+c7eOpmmc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=c7/ZaEFI0O+ATG6oU0LJ8uufcq5Mkf7BGlx4sNoiG3OimKKXsDmPw5/5G4wIXaNaY XXwRUCkyH9R+NC39YmUvpWtMaDnxfcvhT3UxVCSYyckxDv/POdAZ1m8lZhF+t7QMd1 WjYav8Y3sCDhWeRB8u04gjilwbS6tiaOKc6M5zIY= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727233AbfHEGN0 (ORCPT ); Mon, 5 Aug 2019 02:13:26 -0400 Received: from mail.kernel.org ([198.145.29.99]:46720 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725976AbfHEGN0 (ORCPT ); Mon, 5 Aug 2019 02:13:26 -0400 Received: from localhost (unknown [77.137.115.125]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 11EEB206C1; Mon, 5 Aug 2019 06:13:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1564985604; bh=DEza7LipQz/yKwMh3a7hoO43mdHVY4vPsu+c7eOpmmc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=2TbYzjm38SOUrCNApTPuG9DfijA4/lcG5skmEv6yuQ6p5TbrxWoHMyk9AxcdpV9Pr 2WNAdl3jPIYDLzg//Qs492WC4fmMd8P8DlMV7nkUFZn8G0kKuc1QLC5nM5WQqKXOIB uz+gUCFUNXjkFp0BZ6Oo52sRGUdFl1yZvavpuXz4= Date: Mon, 5 Aug 2019 09:13:20 +0300 From: Leon Romanovsky To: Chuhong Yuan Cc: Saeed Mahameed , "David S . Miller" , Netdev , linux-rdma@vger.kernel.org, LKML Subject: Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount Message-ID: <20190805061320.GN4832@mtr-leonro.mtl.com> References: <20190802164828.20243-1-hslester96@gmail.com> <20190804125858.GJ4832@mtr-leonro.mtl.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.0 (2019-05-25) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > > Regards, > Chuhong > > > > > > > Signed-off-by: Chuhong Yuan > > > --- > > > Changes in v2: > > > - Add #include. > > > > > > drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c > > > index b9d4f4e19ff9..148b55c3db7a 100644 > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c > > > @@ -32,6 +32,7 @@ > > > > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include "mlx5_core.h" > > > @@ -48,7 +49,7 @@ struct mlx5_vxlan { > > > > > > struct mlx5_vxlan_port { > > > struct hlist_node hlist; > > > - atomic_t refcount; > > > + refcount_t refcount; > > > u16 udp_port; > > > }; > > > > > > @@ -113,7 +114,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port) > > > > > > vxlanp = mlx5_vxlan_lookup_port(vxlan, port); > > > if (vxlanp) { > > > - atomic_inc(&vxlanp->refcount); > > > + refcount_inc(&vxlanp->refcount); > > > return 0; > > > } > > > > > > @@ -137,7 +138,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port) > > > } > > > > > > vxlanp->udp_port = port; > > > - atomic_set(&vxlanp->refcount, 1); > > > + refcount_set(&vxlanp->refcount, 1); > > > > > > spin_lock_bh(&vxlan->lock); > > > hash_add(vxlan->htable, &vxlanp->hlist, port); > > > @@ -170,7 +171,7 @@ int mlx5_vxlan_del_port(struct mlx5_vxlan *vxlan, u16 port) > > > goto out_unlock; > > > } > > > > > > - if (atomic_dec_and_test(&vxlanp->refcount)) { > > > + if (refcount_dec_and_test(&vxlanp->refcount)) { > > > hash_del(&vxlanp->hlist); > > > remove = true; > > > } > > > -- > > > 2.20.1 > > >