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

Matan Azrad matan at mellanox.com
Tue Sep 5 12:38:21 CEST 2017


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.

> > 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?

> Thanks.
> 
> --
> Adrien Mazarguil
> 6WIND


More information about the dev mailing list