From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id 50E733254 for ; Tue, 5 Sep 2017 14:02:09 +0200 (CEST) Received: by mail-wm0-f43.google.com with SMTP id i145so18274559wmf.1 for ; Tue, 05 Sep 2017 05:02:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=+uKclaGPdZupH23R05FC8ZNwgjlxDIXI1KpIuQP7r2I=; b=W1K/WMVUau/4kOEFgzAZVQ6ZxnWRFmIEE8h6xCc9c1TW4B6Gt3FOsapvMLzQSCrdXk dsATWW4CJ/QVfThoVKVlDOzdQTHmz7f9KBAJIZzfQkp/3DuHC9CWvi1BOSh8J389PGf+ 2ocqrHoE/TjESAztDIeGYTg08B8o7J2It8gRNq7kpBCxwHsyUVqdJZQCmK5oBt0QQQUP tSzMq+0PRlHp1gIhxsFJcaSsvQlh20RABySxgY9Io0LFeH9jGNSqmIKWDiaOvHvblfHO EzXa66bgYz8TX0Pq4AIjZVuf3Ho5aESTPTxZtyy2LIPqVhoLDo61ZI5LqeJ+rG5I37IW KR7g== 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:content-transfer-encoding :in-reply-to; bh=+uKclaGPdZupH23R05FC8ZNwgjlxDIXI1KpIuQP7r2I=; b=EPDuUW4u4q8CHon/3uDbi9iTel4bXBX5CCmnMrt/sjDRLsgb1cqvlwO20IMcKdJbyp CWUMV4azVlhbTXxQ0ufjyIKtTXvqp2xR/l26V14fj1VZXQjhmrmmJ43p/cY/0ni5pljK e0JoI6/z48dySNuYNYRdqIFmI55IkBREyr6+zyce/TTs+b547p3W4+yLaBwC8vkZvf7+ x+gcVc2Vmlt1/JknnJRzHYEwwui3Mz4EgbLgK64XHGd+8kG+QK0+LK5Rh2nfVcVcRhLs TBvVFtZWSVd4ttIcgyLeP3MHwku01v38LVFhn9pT+2a9yl7YZo25CaxwBriEwlhRJXgQ YV3w== X-Gm-Message-State: AHPjjUik9TFWSAAmofntmGb+VK9Xqn+eiXpLyLMG/p5SMzjoi+ff2oUt /bKbm3izLpdzP5Pv1IU= X-Google-Smtp-Source: ADKCNb6Q+xFvEQEVCs43+YUVb8fpwZEnQs9ScALA95CcTGYusU76BK+4i+pWFj2/eNJ7resZmKA3mA== X-Received: by 10.28.24.138 with SMTP id 132mr2301432wmy.61.1504612928733; Tue, 05 Sep 2017 05:02:08 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id a69sm377452wma.44.2017.09.05.05.02.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 05 Sep 2017 05:02:07 -0700 (PDT) Date: Tue, 5 Sep 2017 14:01:58 +0200 From: Adrien Mazarguil To: Matan Azrad Cc: =?utf-8?B?TsOpbGlv?= Laranjeiro , "dev@dpdk.org" Message-ID: <20170905120158.GC4301@6wind.com> References: <20170904124943.2pep4kbglu4q5qg4@localhost> <1504533353-38337-1-git-send-email-matan@mellanox.com> <20170904153308.GY4301@6wind.com> <20170905092802.GA4301@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device removal event X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Sep 2017 12:02:09 -0000 Hi Matan, On Tue, Sep 05, 2017 at 10:38:21AM +0000, Matan Azrad wrote: > Hi Adrien > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Tuesday, September 5, 2017 12:28 PM > > To: Matan Azrad > > Cc: Nélio Laranjeiro ; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device removal event > > > > Hi Matan, > > > > On Mon, Sep 04, 2017 at 05:52:55PM +0000, Matan Azrad wrote: > > > Hi Adrien, > > > > > > > -----Original Message----- > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > > > Sent: Monday, September 4, 2017 6:33 PM > > > > To: Matan Azrad > > > > Cc: Nélio Laranjeiro ; dev@dpdk.org > > > > Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device removal > > > > event > > > > > > > > Hi Matan, > > > > > > > > One comment I have is, while this patch adds support for RMV, it > > > > also silently addresses a bug (see large comment you added to > > > > priv_link_status_update()). > > > > > > > > This should be split in two commits, with the fix part coming first > > > > and CC stable@dpdk.org, and a second commit adding RMV support > > proper. > > > > > > > > > > Actually, the mlx4 bug was not appeared in the mlx5 previous code, > > > Probably because the RMV interrupt was not implemented in mlx5 before > > this patch. > > > > Good point, no RMV could occur before it is implemented, however a > > dedicated commit for the fix itself (i.e. alarm callback not supposed to end up > > calling ibv_get_async_event()) might better explain the logic behind these > > changes. What I mean is, if there was no problem, you wouldn't need to > > make > > priv_link_status_update() a separate function, right? > > > > The separation was done mainly because of the new interrupt implementation, > else, there was bug here. > The unnecessary alarm ibv_get_async_event calling was harmless in > the previous code. > I gets your point for the logic explanation behind these changes and I can add it in this > patch commit log to be clearer, something like: > The link update operation was separated from the interrupt callback > to avoid RMV interrupt disregard and unnecessary event acknowledgment > caused by the inconsistent link status alarm callback. Yes, it's better to explain why you did this in the commit log, but see below. > > > The big comment just explains the link inconsistent issue and was > > > added here since Nelio and I think the new function, > > > priv_link_status_update(), justifies this comment for future review. > > > > I understand, this could also have been part of the commit log of the > > dedicated commit. > > > Are you sure we need to describe the code comment reason in the commit log? It's a change you did to address a possible bug otherwise so we have to, however remember that a commit should, as much as possible, do exactly one thing. If you need to explain that you did this in order to do that, "this" and "that" can often be identified as two separate commits. Doing so makes it much easier for reviewers to understand the reasoning behind changes and leads to quicker reviews (makes instant-acks even possible). It'd still like a separate commit if you don't mind. -- Adrien Mazarguil 6WIND