From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 1E92028FD for ; Thu, 8 Nov 2018 09:49:04 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Nov 2018 00:49:04 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,478,1534834800"; d="scan'208";a="89519797" Received: from jguo15x-mobl.ccr.corp.intel.com (HELO [10.67.68.69]) ([10.67.68.69]) by orsmga006.jf.intel.com with ESMTP; 08 Nov 2018 00:49:02 -0800 To: Matan Azrad , "Ananyev, Konstantin" , "Burakov, Anatoly" , Thomas Monjalon , "Iremonger, Bernard" , "Wu, Jingjing" , "Lu, Wenzhuo" Cc: "Yigit, Ferruh" , "dev@dpdk.org" , "Zhang, Helin" , "He, Shaopeng" References: <1541484436-91320-1-git-send-email-jia.guo@intel.com> <1541484436-91320-4-git-send-email-jia.guo@intel.com> <2601191342CEEE43887BDE71AB9772580103069B86@irsmsx105.ger.corp.intel.com> From: Jeff Guo Message-ID: Date: Thu, 8 Nov 2018 16:49:01 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH 3/3] app/testpmd: fix callback issue for hot-unplug X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Nov 2018 08:49:05 -0000 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 ; Ananyev, Konstantin >>> ; Burakov, Anatoly >>> ; Thomas Monjalon >> ; >>> Iremonger, Bernard ; Wu, Jingjing >>> ; Lu, Wenzhuo >>> Cc: Yigit, Ferruh ; dev@dpdk.org; Zhang, Helin >>> ; He, Shaopeng >>> 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 >>>>> 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 >>>>> --- >>>>> 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. >> 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