From mboxrd@z Thu Jan 1 00:00:00 1970 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) 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=hm6Uonx+yp54x3kGncYW+8M/N8aJ3XDRVHi6qmMBtIc=; b=0BWZVj7A8mD6YhlcUVo02G1jgzzliyJXVWgrehmEDTP1uKB4E7CK086UqoAg/83CSP wMQ7+0oEL3sbZO6+3QZZOJVjhE+tl+HixPq9D+0LEXrXzOzVf2ta9H+a966gOXHMlGUQ tBlis74MPcQKiyXvtEMyY0+0Wd6bAz5dmdLTqkbqGIlqN12Ryw2aYxWt3ru8qqUB7GKQ K28XGxQ3w+jmMW91B0Cky5gDKao5xUVq+Sp2e+OA7gd1FPUQKUubl/kHEdK0CJWZkH9P c0GHh+oKCwAx4tEDmwZAUBU8sXTDuGnmxqRpG9CNQ0WO4sNuStkTv5GFi8t2aV3xwZpe J16A== 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=hm6Uonx+yp54x3kGncYW+8M/N8aJ3XDRVHi6qmMBtIc=; b=jueVc5f7vLRr7vazOhsH42NhK4Bph32qAurYzX1Ar4Q0GFRIzIInnzq5Y9sNyaaRHL kt3JK/+txpI0vNBD/QMagnOblEA8Ty0PlSSOPjuGiYW+tHIsX24VftBEbhWyeSNtwJJX vx5q0kWWniCPpGOsrE+DnwLFcSZQd8JlO3eAjD0LzIq5D9I6rTcwXDYXBFUAAKCJtjZo vEy8QIzLN+2YbIxHs9j9v3mYaTjlOT6+IVpiwVZM/X09igJFTg9FQTHMTCtK6mwQbNTv /aXDT33KOIxwVVrsZ7MHwWRQdrj5FpX2/GftGmfKxkAUGkEQXOIsjypFasRe3hUSK0DG KPXg== X-Gm-Message-State: AHPjjUg42ov7i9zElIfj0YpeorKcYI1w2BTtfP2VudwrOIP/7ZZxNd// w5jAmKONZ4dWph6P X-Google-Smtp-Source: ADKCNb6kQk2EJDYqpRanogl19BJEiqR3KCoG8/lbAG8FLWud0dANgp3tA8pU6NS1fLHs6vePElLDJA== X-Received: by 10.28.213.15 with SMTP id m15mr798255wmg.99.1504681982796; Wed, 06 Sep 2017 00:13:02 -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 k52sm5331439wrf.62.2017.09.06.00.13.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 06 Sep 2017 00:13:01 -0700 (PDT) Date: Wed, 6 Sep 2017 09:12:52 +0200 From: Adrien Mazarguil To: Matan Azrad Cc: =?utf-8?B?TsOpbGlv?= Laranjeiro , "dev@dpdk.org" 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-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: Wed, 06 Sep 2017 07:13:03 -0000 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