DPDK patches and discussions
 help / color / mirror / Atom feed
From: Matan Azrad <matan@mellanox.com>
To: Jeff Guo <jia.guo@intel.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"Burakov, Anatoly" <anatoly.burakov@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	"Iremonger, Bernard" <bernard.iremonger@intel.com>,
	"Wu, Jingjing" <jingjing.wu@intel.com>,
	"Lu, Wenzhuo" <wenzhuo.lu@intel.com>
Cc: "Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"Zhang, Helin" <helin.zhang@intel.com>,
	"He, Shaopeng" <shaopeng.he@intel.com>
Subject: Re: [dpdk-dev] [PATCH 3/3] app/testpmd: fix callback issue for hot-unplug
Date: Fri, 9 Nov 2018 05:24:45 +0000
Message-ID: <AM0PR0502MB40194825DCA7EDEF64132C81D2C60@AM0PR0502MB4019.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <11af735e-7e8a-fb16-3ea8-2b269d8437b1@intel.com>



 From: Jeff Guo 
> On 11/8/2018 5:35 PM, Matan Azrad wrote:
> >
> > From: Jeff Guo
> >> On 11/8/2018 3:28 PM, Matan Azrad wrote:
> >>> From: Ananyev, Konstantin
> >>>>> -----Original Message-----
> >>>>> From: Guo, Jia
> >>>>> Sent: Wednesday, November 7, 2018 7:30 AM
> >>>>> To: Matan Azrad <matan@mellanox.com>; Ananyev, Konstantin
> >>>>> <konstantin.ananyev@intel.com>; Burakov, Anatoly
> >>>>> <anatoly.burakov@intel.com>; Thomas Monjalon
> >>>> <thomas@monjalon.net>;
> >>>>> Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> >>>>> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> >>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; Zhang,
> >>>>> Helin <helin.zhang@intel.com>; He, Shaopeng
> >> <shaopeng.he@intel.com>
> >>>>> Subject: Re: [PATCH 3/3] app/testpmd: fix callback issue for
> >>>>> hot-unplug
> >>>>>
> >>>>> matan
> >>>>>
> >>>>> On 11/6/2018 2:36 PM, Matan Azrad wrote:
> >>>>>> Hi Jeff
> >>>>>>
> >>>>>>     From: Jeff Guo <jia.guo@intel.com>
> >>>>>>> Before detach device when device be hot-unplugged, the failure
> >>>>>>> process in user space and kernel space both need to be finished,
> >>>>>>> such as eal interrupt callback need to be inactive before the
> >>>>>>> callback be unregistered when device is being cleaned. This
> >>>>>>> patch add rte alarm for device detaching, with that it could
> >>>>>>> finish interrupt callback soon and give time to let the failure
> >>>>>>> process done
> >>>> before detaching.
> >>>>>>> Fixes: 2049c5113fe8 ("app/testpmd: use hotplug failure handler")
> >>>>>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> >>>>>>> ---
> >>>>>>>     app/test-pmd/testpmd.c | 13 ++++++++++++-
> >>>>>>>     1 file changed, 12 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >>>>>>> index 9c0edca..9c673cf 100644
> >>>>>>> --- a/app/test-pmd/testpmd.c
> >>>>>>> +++ b/app/test-pmd/testpmd.c
> >>>>>>> @@ -2620,7 +2620,18 @@ eth_dev_event_callback(const char
> >>>>>>> *device_name, enum rte_dev_event_type type,
> >>>>>>>     				device_name);
> >>>>>>>     			return;
> >>>>>>>     		}
> >>>>>>> -		rmv_event_callback((void *)(intptr_t)port_id);
> >>>>>>> +		/*
> >>>>>>> +		 * Before detach device, the hot-unplug failure
> >> process in
> >>>>>>> +		 * user space and kernel space both need to be
> >> finished,
> >>>>>>> +		 * such as eal interrupt callback need to be inactive
> >> before
> >>>>>>> +		 * the callback be unregistered when device is being
> >> cleaned.
> >>>>>>> +		 * So finished interrupt callback soon here and give
> >> time to
> >>>>>>> +		 * let the work done before detaching.
> >>>>>>> +		 */
> >>>>>>> +		if (rte_eal_alarm_set(100000,
> >>>>>>> +				rmv_event_callback, (void
> >>>>>>> *)(intptr_t)port_id))
> >>>>>>> +			RTE_LOG(ERR, EAL,
> >>>>>>> +				"Could not set up deferred device
> >>>>>> It looks me strange to use callback and alarm to remove a device:
> >>>>>> Why not to use callback and that is it?
> >>>>>>
> >>>>>> I think that it's better to let the EAL to detach the device
> >>>>>> after all the
> >>>> callbacks were done and not to do it by the user callback.
> >>>>>> So the application\callback owners just need to clean its
> >>>>>> resources with understanding that after the callback the
> >>>>>> device(and the callback
> >>>>> itself) will be detached by the EAL.
> >>>>>
> >>>>>
> >>>>> Firstly, at the currently framework and solution, such as callback
> >>>>> for RTE_ETH_EVENT_INTR_RMV, still need to use the deferred device
> >>>> removal,
> >>>>> we tend to give the control of detaching device to the
> >>>>> application, and the whole process is located on the user's
> >>>>> callback. Notify app to detach device by callback but make it deferred,
> i think it is fine.
> >>> But the device must be detached in remove event, why not to do it in
> EAL?
> >>
> >> I think it because of before detached the device, application should
> >> be stop the forwarding at first, then stop the device, then close
> >>
> >> the device, finally call eal unplug API to detach device. If eal
> >> directly detach device at the first step, there will be mountain user
> >> space error need to handle, so that is one reason why need to
> >> provider the remove notification to app, and let app to process it.
> >
> > This is why the EAL need to detach the device only after all the user
> callbacks were done.
> 
> 
> If i correctly got your meaning, you suppose to let eal to mandatory detach
> device but not app, app just need to stop/close port, right?

Yes, the app should stop,close,clean its own resources of the removed device,
Then, EAL to detach the device.

> 
> If so, it will need to modify rmv_event_callback by not invoke the detaching
> func and add some detaching logic to hotplug framework in eal.
> 
rmv_event_callback is using by other hotplug mechanism (ETHDEV RMV event), so you need to use another one\ add parameter to it.
And yes, you need to detach the device from EAL, should be simple.

> It is hardly say better or worse but this new propose need to discuss more in
> public. And it might be better to use another specific patch to handler it.
> What do you or other guys think so?

Since you are fixing issue here, it can be done by a fix series.

Other feedbacks are welcome all the time 😊

> 
> 
> >>
> >>>> It is also unclear to me my we need an alarm here.
> >>>> First (probably wrong) impression we just try to hide some
> >>>> synchronization Problem by introducing delay.
> >>> Looks like, the issue is that the callback function memory will be
> >>> removed
> >> from the function itself (by the detach call), no?
> >>
> >>
> >> Answer here for both Konstantin and Matan.
> >>
> >> Yes, i think matan is right, the interrupt callback will be destroy
> >> in the app callback itself, the sequence is that as below
> >>
> >> hot-unplug interrupt -> interrupt callback -> app callback(return to
> >> finish interrupt callback, delay detaching) -> detach
> >> device(unregister interrupt
> >> callback)
> >>
> >>
> >>>> Konstantin
> >>>>
> >>>>> Secondly, the vfio is different with igb_uio for hot-unplug, it
> >>>>> register/unregister hotplug interrupt callback for each device, so
> >>>>> need to make  the callback done before unregister the callback.
> >>>>>
> >>>>> So I think it should be considerate as an workaround here, before
> >>>>> we find a better way.
> >>>>>
> >>>>>
> >>>>>>> removal\n");
> >>>>>>>     		break;
> >>>>>>>     	case RTE_DEV_EVENT_ADD:
> >>>>>>>     		RTE_LOG(ERR, EAL, "The device: %s has been
> >> added!\n",
> >>>>>>> --
> >>>>>>> 2.7.4

  reply	other threads:[~2018-11-09  5:24 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-06  6:07 [dpdk-dev] [PATCH 0/3] fix vfio hot-unplug issue Jeff Guo
2018-11-06  6:07 ` [dpdk-dev] [PATCH 1/3] eal: fix lock issue for hot-unplug Jeff Guo
2018-11-06  6:22   ` Matan Azrad
2018-11-07  5:49     ` Jeff Guo
2018-11-08  7:08       ` Matan Azrad
2018-11-06  6:07 ` [dpdk-dev] [PATCH 2/3] vfio: fix to add handler lock " Jeff Guo
2018-11-06  6:23   ` Matan Azrad
2018-11-07  6:15     ` Jeff Guo
2018-11-08  7:20       ` Matan Azrad
2018-11-08  8:30         ` Jeff Guo
2018-11-06  6:07 ` [dpdk-dev] [PATCH 3/3] app/testpmd: fix callback issue " Jeff Guo
2018-11-06  6:36   ` Matan Azrad
2018-11-07  7:30     ` Jeff Guo
2018-11-07 11:05       ` Ananyev, Konstantin
2018-11-08  7:28         ` Matan Azrad
2018-11-08  8:49           ` Jeff Guo
2018-11-08  9:35             ` Matan Azrad
2018-11-09  3:55               ` Jeff Guo
2018-11-09  5:24                 ` Matan Azrad [this message]
2018-11-09  6:17                   ` Jeff Guo
     [not found]                     ` <AM0PR0502MB401938411A7E2BA9E76576A2D2C00@AM0PR0502MB4019.eurprd05.prod.outlook.com>
2018-11-12  1:35                       ` Thomas Monjalon
2018-11-14  9:32                         ` Jeff Guo
2018-11-15  9:18 ` [dpdk-dev] [PATCH V2 0/3] fix vfio hot-unplug issue Jeff Guo
2018-11-15  9:18   ` Jeff Guo
2018-11-18 16:19     ` Thomas Monjalon
2018-11-15  9:18   ` [dpdk-dev] [PATCH V2 1/3] eal: fix lock issue for hot-unplug Jeff Guo
2018-11-15  9:18   ` [dpdk-dev] [PATCH V2 2/3] vfio: fix to add handler lock " Jeff Guo
2018-11-15  9:18   ` [dpdk-dev] [PATCH V2 3/3] app/testpmd: fix callback issue " Jeff Guo

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=AM0PR0502MB40194825DCA7EDEF64132C81D2C60@AM0PR0502MB4019.eurprd05.prod.outlook.com \
    --to=matan@mellanox.com \
    --cc=anatoly.burakov@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=helin.zhang@intel.com \
    --cc=jia.guo@intel.com \
    --cc=jingjing.wu@intel.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=shaopeng.he@intel.com \
    --cc=thomas@monjalon.net \
    --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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git