From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: "Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device removal event
Date: Tue, 5 Sep 2017 14:01:58 +0200 [thread overview]
Message-ID: <20170905120158.GC4301@6wind.com> (raw)
In-Reply-To: <DB6PR0502MB304893E38927D4784301AFD4D2960@DB6PR0502MB3048.eurprd05.prod.outlook.com>
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 <matan@mellanox.com>
> > Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; 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 <matan@mellanox.com>
> > > > Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; 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
next prev parent reply other threads:[~2017-09-05 12:02 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-13 12:25 [dpdk-dev] [PATCH 1/2] " Matan Azrad
2017-08-13 12:25 ` [dpdk-dev] [PATCH 2/2] net/mlx5: fix probe failure report Matan Azrad
2017-08-23 9:44 ` Nélio Laranjeiro
2017-09-01 10:40 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2017-08-23 9:40 ` [dpdk-dev] [PATCH 1/2] net/mlx5: support device removal event Nélio Laranjeiro
2017-08-23 19:44 ` Matan Azrad
2017-08-24 7:38 ` Nélio Laranjeiro
2017-08-24 14:33 ` Matan Azrad
2017-08-25 8:29 ` Nélio Laranjeiro
2017-08-29 8:30 ` [dpdk-dev] [PATCH v2] " Matan Azrad
2017-09-04 12:49 ` Nélio Laranjeiro
2017-09-04 13:55 ` [dpdk-dev] [PATCH v3] " Matan Azrad
2017-09-04 15:33 ` Adrien Mazarguil
2017-09-04 17:52 ` Matan Azrad
2017-09-05 9:28 ` Adrien Mazarguil
2017-09-05 10:38 ` Matan Azrad
2017-09-05 12:01 ` Adrien Mazarguil [this message]
2017-09-05 13:36 ` Matan Azrad
2017-09-06 7:12 ` Adrien Mazarguil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170905120158.GC4301@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=matan@mellanox.com \
--cc=nelio.laranjeiro@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).