DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jeff Guo <jia.guo@intel.com>
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" <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 14:17:07 +0800
Message-ID: <8bac8c10-d30b-ab93-1d6c-03e7d93b97c3@intel.com> (raw)
In-Reply-To: <AM0PR0502MB40194825DCA7EDEF64132C81D2C60@AM0PR0502MB4019.eurprd05.prod.outlook.com>


On 11/9/2018 1:24 PM, Matan Azrad wrote:
>
>   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.


I think rmv_event_callback is original use for other hotplug event 
(ETHDEV RMV event), but it still use the common hotplug mechanism(app 
callback and app detach),

so i think it will still need to face this callback issue and you could 
check that eth_event_callback also use the rte alarm to detach device.

so my suggestion is that, you maybe propose a good idea but let we keep 
on current mechanism until we come to a final good solution agreement, 
before that, just let

it functional.


>> 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  6:17 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
2018-11-09  6:17                   ` Jeff Guo [this message]
     [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=8bac8c10-d30b-ab93-1d6c-03e7d93b97c3@intel.com \
    --to=jia.guo@intel.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=jingjing.wu@intel.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=matan@mellanox.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