DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
To: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	"Chen, Jing D" <jing.d.chen@intel.com>,
	"Liang, Cunming" <cunming.liang@intel.com>,
	"Wu, Jingjing" <jingjing.wu@intel.com>,
	"Zhang, Helin" <helin.zhang@intel.com>,
	"thomas.monjalon@6wind.com" <thomas.monjalon@6wind.com>
Subject: Re: [dpdk-dev] [PATCH v6 1/4] lib/librte_ether: support device reset
Date: Wed, 22 Jun 2016 08:07:47 +0530	[thread overview]
Message-ID: <20160622023745.GA4437@localhost.localdomain> (raw)
In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09090348971F@shsmsx102.ccr.corp.intel.com>

On Wed, Jun 22, 2016 at 01:35:37AM +0000, Lu, Wenzhuo wrote:
> Hi Jerin,
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Tuesday, June 21, 2016 10:29 PM
> > To: Ananyev, Konstantin
> > Cc: Lu, Wenzhuo; Stephen Hemminger; dev@dpdk.org; Richardson, Bruce; Chen,
> > Jing D; Liang, Cunming; Wu, Jingjing; Zhang, Helin;
> > thomas.monjalon@6wind.com
> > Subject: Re: [dpdk-dev] [PATCH v6 1/4] lib/librte_ether: support device reset
> > 
> > On Tue, Jun 21, 2016 at 02:03:15PM +0000, Ananyev, Konstantin wrote:
> > >
> > >
> > > > > > > > Hi Wenzhuo,
> > > > > > > >
> > > > > > > > > > > > > > On Mon, Jun 20, 2016 at 02:24:27PM +0800, Wenzhuo Lu
> > wrote:
> > > > > > > > > > > > > > > Add an API to reset the device.
> > > > > > > > > > > > > > > It's for VF device in this scenario, kernel PF + DPDK VF.
> > > > > > > > > > > > > > > When the PF port down->up, APP should call
> > > > > > > > > > > > > > > this API to reset VF port. Most likely, APP
> > > > > > > > > > > > > > > should call it in its management thread and
> > > > > > > > > > > > > > > guarantee the thread safe. It means APP should
> > > > > > > > > > > > > > > stop the rx/tx and the device, then reset the device, then
> > recover the device and rx/tx.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Following is _a_ use-case for Device reset. But
> > > > > > > > > > > > > > may be not be _the_ use case. IMO, We need to
> > > > > > > > > > > > > > first say expected behavior of this API and add a use-case
> > later.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Other use-case would be, PCIe VF with functional
> > > > > > > > > > > > > > level reset for SRIOV migration.
> > > > > > > > > > > > > > Are we on same page?
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > In my experience with Linux devices, this is
> > > > > > > > > > > > > normally handled by the device driver in the start
> > > > > > > > > > > > > routine.  Since any use case which needs this is
> > > > > > > > > > > > > going to do a stop/reset/start sequence, why not just have
> > the VF device driver do this in the start routine?.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Adding yet another API and state transistion if
> > > > > > > > > > > > > not necessary increases the complexity and required test
> > cases for all devices.
> > > > > > > > > > > >
> > > > > > > > > > > > I agree with Stephen here.I think if application
> > > > > > > > > > > > needs to call start after the device reset then we
> > > > > > > > > > > > could add this logic in start itself rather exposing
> > > > > > > > > > > > a yet another API
> > > > > > > > > > > Do you mean changing the device_start to include all
> > > > > > > > > > > these actions, stop
> > > > > > > > > > device -> stop queue -> re-setup queue -> start queue -> start
> > device ?
> > > > > > > > > >
> > > > > > > > > > What was the expected API call sequence when you were
> > introduced this API?
> > > > > > > > > >
> > > > > > > > > > Point was to have implicit device reset in the API call
> > > > > > > > > > sequence(Wherever make sense for specific PMD)
> > > > > > > > > I think the API call sequence depends on the
> > > > > > > > > implementation of the APP. Let's say if there's not this
> > > > > > > > > reset API, APP can use
> > > > this
> > > > > > API
> > > > > > > > call sequence to handle the PF link down/up event,
> > > > > > > > rte_eth_dev_close -> rte_eth_rx_queue_setup ->
> > > > rte_eth_tx_queue_setup -
> > > > > > >
> > > > > > > > rte_eth_dev_start.
> > > > > > > > > Actually our purpose is to use this reset API instead of
> > > > > > > > > the API call sequence. You can see the reset API is not
> > > > > > > > > necessary. The
> > > > > > benefit
> > > > > > > > is to save the code for APP.
> > > > > > > >
> > > > > > > > Then I am bit confused with original commit log description.
> > > > > > > > |
> > > > > > > > |It means APP should stop the rx/tx and the device, then
> > > > > > > > |reset the device, then recover the device and rx/tx.
> > > > > > > > |
> > > > > > > > I was under impression that it a low level reset API for
> > > > > > > > this device? Is n't it?
> > > > > > > >
> > > > > > > > The other issue is generalized outlook of the API, Certain
> > > > > > > > PMD will not have PF link down/up event? Link down/up and
> > > > > > > > only connected to VF and PF only for configuration.
> > > > > > > >
> > > > > > > > How about fixing it more transparently in PMD driver itself
> > > > > > > > as PMD driver knows the PF link up/down event, Is it
> > > > > > > > possible to recover the VF on that event if its only matter of resetting
> > it?
> > > > > > >
> > > > > > > I think we already went through that discussion on the list.
> > > > > > > Unfortunately with current dpdk design it is hardly possible.
> > > > > > > To achieve that we need to introduce some sort of
> > > > > > > synchronisation between IO and control APIs (locking or so).
> > > > > > > Actually I am not sure why having a special reset function will be a
> > problem.
> > > > > >
> > > > > > |
> > > > > > |It means APP should stop the rx/tx and the device, then reset
> > > > > > |the device, then recover the device and rx/tx.
> > > > > > |
> > > > > > Just to understand, If application still need  to do the stop
> > > > > > then what value addtion reset API brings on the table?
> > > > >
> > > > > If application calls dev_reset() it doesn't need to call dev_stop() before it.
> > > > > dev_reset() will take care of it.
> > > > > But it needs to make sure that no other thread will try to modify
> > > > > that device state (either dev_stop/start, or eth_rx_busrst/eth_tx_burst)
> > while the reset op is in place.
> > > >
> > > > OK. This description looks different than commit log and API doxygen
> > comment. Please fix it.
> > > > How about a different name for this API. Device reset is too generic?
> Any suggestion? I use this name because I believe what this API do is to reset the device.
> 
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > > Yes, it would exist only for VFs, for PF it could be left unimplemented.
> > > > > > > Though it definitely seems more convenient from user point of
> > > > > > > view, they would know: to handle VF reset event, they just
> > > > > > > need to call that particular function, not to re-implement their own.
> > > > > > What if driver returns "not implemented" then application will
> > > > > >have do  generic rte_eth_dev_stop/rte_eth_dev_start.
> > > > > >That way in application  perspective we are NOT solving any problem.
> > > > >
> > > > > True, but as I said for PF application would just never receive such event.
> > > > What is this event ? Is it VF Link up/down event?
> > > >
> > > > No I was referring to VF itself, Other VF PMD drivers in drivers/net
> > > > where this callback is not implemented.
> > >
> > > Hmm, the only suggestion I have here - Maintainers/developers of
> > > non-Intel PMD will implement it for their VFs?
> > 
> > That's fine. But, We have to know what to implement here in PMD perspective?
> > That's reason being asking about the API expectation and application usage :-)
> > 
> > > In case of course they do need to handle similar event.
> > Which is this event and How application get notify it.
> When the PF link is down/up, the PF will use the mailbox to send a message to VF. The event here means the VF receives that message from PF. So VF can know the physical link state changed. You see it's only for VF. PF will not receive such kind of message.
> And we use the callback mechanism to let APP notified. APP should register a callback function. When VF driver receives the message it will call the callback function, then APP can know that.

How about the standardizing  a name for that event like
RTE_ETH_EVENT_INTR_DOWNSTREAM_LSC or
RTE_ETH_EVENT_INTR_PF_LSC or similar (like RTE_ETH_EVENT_INTR_RESET)
and counter API in VF to handle the specific event whose API name
similar to selected event name not eth_dev_reset(reset sounds like more
like HW reset, In PCIe device perspective FLR etc)

OR

How about handling in more generic way where a generic alert message
send by PF to VF like RTE_ETH_EVENT_INTR_PF_ALERT or similar.
And have only one handle functions in VF side so that in future
we can keep adding new functionality with out introducing new counter API in VF

Jerin

> 
> > 
> > > if not I suppose there is no harm to left it unimplemented.
> > OK. If it is for VF/PF link down-up event then I will make it as 'nop'.
> As explained above, the event is not VF/PF link down-up. Actually it's that VF is notified the PF link is down-up.
> 
> And to my opinion, although now we only implement the reset API for VF, I believe there's nothing preventing us to implement this API for PF if we can find some scenario that we need to reset the PF link. The reset API is reset API, it can be used for the event described above. But it's not bound to this event.
> > 
> > Jerin
> > 
> > > Konstantin
> > >

  reply	other threads:[~2016-06-22  2:38 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-06  5:40 [dpdk-dev] [PATCH 0/8] support reset of VF link Wenzhuo Lu
2016-06-06  5:40 ` [dpdk-dev] [PATCH 1/8] lib/librte_ether: support device reset Wenzhuo Lu
2016-06-06  5:40 ` [dpdk-dev] [PATCH 2/8] lib/librte_ether: defind RX/TX lock mode Wenzhuo Lu
2016-06-08  2:15   ` Stephen Hemminger
2016-06-08  7:34     ` Lu, Wenzhuo
2016-06-09  7:50       ` Olivier Matz
2016-06-12  5:25         ` Lu, Wenzhuo
2016-06-10 18:12       ` Stephen Hemminger
2016-06-12  5:27         ` Lu, Wenzhuo
2016-06-06  5:40 ` [dpdk-dev] [PATCH 3/8] ixgbe: RX/TX with lock on VF Wenzhuo Lu
2016-06-06  5:40 ` [dpdk-dev] [PATCH 4/8] ixgbe: implement device reset " Wenzhuo Lu
2016-06-06  5:40 ` [dpdk-dev] [PATCH 5/8] igb: RX/TX with lock " Wenzhuo Lu
2016-06-06  5:40 ` [dpdk-dev] [PATCH 6/8] igb: implement device reset " Wenzhuo Lu
2016-06-06  5:40 ` [dpdk-dev] [PATCH 7/8] i40e:RX/TX with lock " Wenzhuo Lu
2016-06-06  5:40 ` [dpdk-dev] [PATCH 8/8] i40e: implement device reset " Wenzhuo Lu
2016-06-15  3:03 ` [dpdk-dev] [PATCH v5 0/4] support reset of VF link Wenzhuo Lu
2016-06-15  3:03   ` [dpdk-dev] [PATCH v5 1/4] lib/librte_ether: support device reset Wenzhuo Lu
2016-06-16 15:31     ` Bruce Richardson
2016-06-16 15:36     ` Thomas Monjalon
2016-06-15  3:03   ` [dpdk-dev] [PATCH v5 2/4] ixgbe: implement device reset on VF Wenzhuo Lu
2016-06-15  3:03   ` [dpdk-dev] [PATCH v5 3/4] igb: " Wenzhuo Lu
2016-06-15  3:03   ` [dpdk-dev] [PATCH v5 4/4] i40e: " Wenzhuo Lu
2016-06-20  6:24 ` [dpdk-dev] [PATCH v6 0/4] support reset of VF link Wenzhuo Lu
2016-06-20  6:24   ` [dpdk-dev] [PATCH v6 1/4] lib/librte_ether: support device reset Wenzhuo Lu
2016-06-20  9:14     ` Jerin Jacob
2016-06-20 16:17       ` Stephen Hemminger
2016-06-21  3:51         ` Jerin Jacob
2016-06-21  6:14           ` Lu, Wenzhuo
2016-06-21  7:37             ` Jerin Jacob
2016-06-21  8:24               ` Lu, Wenzhuo
2016-06-21  8:55                 ` Jerin Jacob
2016-06-21  9:26                   ` Ananyev, Konstantin
2016-06-21 10:57                     ` Jerin Jacob
2016-06-21 13:10                       ` Ananyev, Konstantin
2016-06-21 13:30                         ` Jerin Jacob
2016-06-21 14:03                           ` Ananyev, Konstantin
2016-06-21 14:29                             ` Jerin Jacob
2016-06-22  1:35                               ` Lu, Wenzhuo
2016-06-22  2:37                                 ` Jerin Jacob [this message]
2016-06-22  3:32                                   ` Lu, Wenzhuo
2016-06-22  4:14                                     ` Jerin Jacob
2016-06-22  5:05                                       ` Lu, Wenzhuo
2016-06-22  6:10                                         ` Jerin Jacob
2016-06-22  6:42                                           ` Lu, Wenzhuo
2016-06-22  7:59                                             ` Jerin Jacob
2016-06-22  8:17                                               ` Thomas Monjalon
2016-06-22  8:25                                                 ` Lu, Wenzhuo
2016-06-22  9:18                                                   ` Thomas Monjalon
2016-06-22 11:06                                                     ` Jerin Jacob
2016-06-23  0:45                                                       ` Lu, Wenzhuo
2016-06-23  0:39                                                     ` Lu, Wenzhuo
2016-06-21  0:51       ` Lu, Wenzhuo
2016-06-20  6:24   ` [dpdk-dev] [PATCH v6 2/4] ixgbe: implement device reset on VF Wenzhuo Lu
2016-06-20  6:24   ` [dpdk-dev] [PATCH v6 3/4] igb: " Wenzhuo Lu
2016-06-20  6:24   ` [dpdk-dev] [PATCH v6 4/4] i40e: " Wenzhuo Lu
2016-07-04 15:48   ` [dpdk-dev] [PATCH v6 0/4] support reset of VF link Luca Boccassi
2016-07-05  0:52     ` Lu, Wenzhuo
2016-07-05  9:52       ` Luca Boccassi
2016-07-06  0:45         ` Lu, Wenzhuo
2016-07-06 16:26           ` Luca Boccassi
     [not found]           ` <1467822182.32466.34.camel@brocade.com>
2016-07-07  1:09             ` Lu, Wenzhuo
2016-07-07 10:20               ` Luca Boccassi
2016-07-07 13:12                 ` Lu, Wenzhuo
2016-07-07 16:19                   ` Luca Boccassi
2016-07-08  0:14                     ` Lu, Wenzhuo
2016-07-08 17:15                       ` Luca Boccassi
2016-07-11  1:32                         ` Lu, Wenzhuo
2016-07-11 12:02                           ` Luca Boccassi
2016-07-11 15:43                             ` Luca Boccassi
2016-07-12  1:19                               ` Lu, Wenzhuo
2016-08-26 12:58                               ` Luca Boccassi
2016-08-29  1:04                                 ` Lu, Wenzhuo

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=20160622023745.GA4437@localhost.localdomain \
    --to=jerin.jacob@caviumnetworks.com \
    --cc=bruce.richardson@intel.com \
    --cc=cunming.liang@intel.com \
    --cc=dev@dpdk.org \
    --cc=helin.zhang@intel.com \
    --cc=jing.d.chen@intel.com \
    --cc=jingjing.wu@intel.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas.monjalon@6wind.com \
    --cc=wenzhuo.lu@intel.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).