From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v3] net/mlx5: support device removal event Date: Wed, 6 Sep 2017 09:12:52 +0200 Message-ID: <20170906071252.GJ4301@6wind.com> References: <20170904124943.2pep4kbglu4q5qg4@localhost> <1504533353-38337-1-git-send-email-matan@mellanox.com> <20170904153308.GY4301@6wind.com> <20170905092802.GA4301@6wind.com> <20170905120158.GC4301@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: =?utf-8?B?TsOpbGlv?= Laranjeiro , "dev@dpdk.org" To: Matan Azrad Return-path: Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41]) by dpdk.org (Postfix) with ESMTP id 6932DF04 for ; Wed, 6 Sep 2017 09:13:03 +0200 (CEST) Received: by mail-wm0-f41.google.com with SMTP id u26so27285258wma.0 for ; Wed, 06 Sep 2017 00:13:03 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Matan, On Tue, Sep 05, 2017 at 01:36:13PM +0000, Matan Azrad wrote: > Hi Adrien > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Tuesday, September 5, 2017 3:02 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 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. > > Sorry, but I think it is an infinite order. > I have just added RMV interrupt, I did a lot of things in this patch for it. > I think I don't need to separate each thing done for this support. > I prefer to stay it in one patch if you don't mind. I understand that's a lot of work, so let's cut the talk. Since I'm the one requesting for patches to be split, I'll offer to re-spin yours and submit the result as v4, is that OK? -- Adrien Mazarguil 6WIND