[dpdk-dev] [PATCH v3] net/mlx5: support device removal event

Adrien Mazarguil adrien.mazarguil at 6wind.com
Tue Sep 5 14:01:58 CEST 2017


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 at 6wind.com]
> > Sent: Tuesday, September 5, 2017 12:28 PM
> > To: Matan Azrad <matan at mellanox.com>
> > Cc: Nélio Laranjeiro <nelio.laranjeiro at 6wind.com>; dev at 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 at 6wind.com]
> > > > Sent: Monday, September 4, 2017 6:33 PM
> > > > To: Matan Azrad <matan at mellanox.com>
> > > > Cc: Nélio Laranjeiro <nelio.laranjeiro at 6wind.com>; dev at 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 at 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


More information about the dev mailing list