From: "Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 1/2] net/mlx5: support device removal event
Date: Thu, 24 Aug 2017 09:38:24 +0200 [thread overview]
Message-ID: <20170824073824.GA4544@autoinstall.dev.6wind.com> (raw)
In-Reply-To: <DB6PR0502MB304871761D7E0F83B67929D4D2850@DB6PR0502MB3048.eurprd05.prod.outlook.com>
On Wed, Aug 23, 2017 at 07:44:45PM +0000, Matan Azrad wrote:
> Hi Nelio
>
> > -----Original Message-----
> > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > Sent: Wednesday, August 23, 2017 12:41 PM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org
> > Subject: Re: [PATCH 1/2] net/mlx5: support device removal event
> >
> > Hi Matan,
> >
> > On Sun, Aug 13, 2017 at 03:25:11PM +0300, Matan Azrad wrote:
> > > Extend the LSC event handling to support the device removal as well.
> > > The Verbs library may send several related events, which are different
> > > from LSC event.
> > >
> > > The mlx5 event handling has been made capable of receiving and
> > > signaling several event types at once.
> > >
> > > This support includes next:
> > > 1. Removal event detection according to the user configuration.
> > > 2. Calling to all registered mlx5 removal callbacks.
> > > 3. Capabilities extension to include removal interrupt handling.
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> > > drivers/net/mlx5/mlx5.c | 2 +-
> > > drivers/net/mlx5/mlx5_ethdev.c | 100
> > > +++++++++++++++++++++++++++--------------
> > > 2 files changed, 68 insertions(+), 34 deletions(-)
> > >
> > > Hi
> > > This patch based on top of last Nelio mlx5 cleanup patches.
> > >
> > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > > bd66a7c..1a3d7f1 100644
> > > --- a/drivers/net/mlx5/mlx5.c
> > > +++ b/drivers/net/mlx5/mlx5.c
> > > @@ -865,7 +865,7 @@ static struct rte_pci_driver mlx5_driver = {
> > > },
> > > .id_table = mlx5_pci_id_map,
> > > .probe = mlx5_pci_probe,
> > > - .drv_flags = RTE_PCI_DRV_INTR_LSC,
> > > + .drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV,
> > > };
> > >
> > > /**
> > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > > b/drivers/net/mlx5/mlx5_ethdev.c index 57f6237..404d8f4 100644
> > > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > > @@ -1112,47 +1112,75 @@ mlx5_ibv_device_to_pci_addr(const struct
> > > ibv_device *device, }
> > >
> > > /**
> > > - * Link status handler.
> > > + * Update the link status.
> > > + * Set alarm if the device link status is inconsistent.
> >
> > Adding such comment should also comment about the issue this alarm is
> > solving i.e. why the link is inconsistent and why the alarm help to fix the
> > issue.
> >
> I didn't see any comments about that in the old code , Hence I didn't write it.
Normal as the alarm is a work around specifically necessary to Mellanox PMD.
Now you explicitly announce that this function program an alarm, the question
is why is it necessary?
> I think you right and this could be added.(even before this patch).
No, in the current code, it update the link, if it inconsistent it tries to
have a link correct ASAP. There is no need to inform this function will
program an alarm, it is internal cooking.
> > > *
> > > * @param priv
> > > * Pointer to private structure.
> > > - * @param dev
> > > - * Pointer to the rte_eth_dev structure.
> > > *
> > > * @return
> > > - * Nonzero if the callback process can be called immediately.
> > > + * Zero if alarm is not set and the link status is consistent.
> > > */
> > > static int
> > > -priv_dev_link_status_handler(struct priv *priv, struct rte_eth_dev
> > > *dev)
> > > +priv_link_status_alarm_update(struct priv *priv)
> >
> > The old name is more accurate, the fact we need to program an alarm is a
> > work around to get the correct status from ethtool. If it was possible to avoid
> > it, this alarm would not exists.
> >
> Probably because of the git +- format and this specific patch you got confuse here.
No I applied your patch and read your code. You did not understand my
comment.
>[...]
When I read:
> void
> mlx5_dev_link_status_handler(void *arg)
> {
> struct rte_eth_dev *dev = arg;
> struct priv *priv = dev->data->dev_private;
> int ret;
>
> priv_lock(priv);
> assert(priv->pending_alarm == 1);
> priv->pending_alarm = 0;
> - ret = priv_dev_link_status_handler(priv, dev);
> + ret = priv_link_status_alarm_update(priv);
> priv_unlock(priv);
> - if (ret)
> + if (!ret)
> _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL,
> - NULL);
> + NULL);
> }
I am expecting to find something related to a link update, what I see is an alarm
update. I don't expect to update an alarm but a link. The names and action
are inconsistent i.e. mlx5_dev_link_status_handler() should handle a link not
an alarm.
I understand there is a need to add more function levels, but the
priv_link_status_alarm_update() should be renamed to something like
priv_link_status_update().
Regards,
--
Nélio Laranjeiro
6WIND
next prev parent reply other threads:[~2017-08-24 7:38 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-13 12:25 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 [this message]
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
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=20170824073824.GA4544@autoinstall.dev.6wind.com \
--to=nelio.laranjeiro@6wind.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=matan@mellanox.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).