From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Guo, Jia" <jia.guo@intel.com>
Subject: Re: [PATCH V13 1/3] eal: add uevent monitor api and
	callback func
Date: Sat, 27 Jan 2018 11:48:17 +0800
Message-ID: <4dba572b-38e1-33b8-2009-c7d977b8b012@intel.com>
References: <1516248723-16985-3-git-send-email-jia.guo@intel.com>
 <1516938577-27662-1-git-send-email-jia.guo@intel.com>
 <20180126165340.GA21468@bricha3-MOBL3.ger.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Cc: stephen@networkplumber.org, gaetan.rivet@6wind.com,
 jingjing.wu@intel.com, motih@mellanox.com, ferruh.yigit@intel.com,
 konstantin.ananyev@intel.com, jblunck@infradead.org, shreyansh.jain@nxp.com,
 dev@dpdk.org, helin.zhang@intel.com
To: Bruce Richardson <bruce.richardson@intel.com>, thomas@monjalon.net
Return-path: <dev-bounces@dpdk.org>
Received: from mga14.intel.com (mga14.intel.com [192.55.52.115])
 by dpdk.org (Postfix) with ESMTP id 7D102A493
 for <dev@dpdk.org>; Sat, 27 Jan 2018 04:48:21 +0100 (CET)
In-Reply-To: <20180126165340.GA21468@bricha3-MOBL3.ger.corp.intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>



On 1/27/2018 12:53 AM, Bruce Richardson wrote:
> On Fri, Jan 26, 2018 at 11:49:35AM +0800, Jeff Guo wrote:
>> This patch aim to add a general uevent mechanism in eal device layer,
>> to enable all linux kernel object uevent monitoring, user could use these
>> APIs to monitor and read out the device status info that sent from the
>> kernel side, then corresponding to handle it, such as when detect hotplug
>> uevent type, user could detach or attach the device, and more it benefit
>> to use to do smoothly fail safe work.
>>
>> About uevent monitoring:
>> a: add one epolling to poll the netlink socket, to monitor the uevent of
>>     the device.
>> b: add enum of rte_eal_dev_event_type and struct of rte_eal_uevent.
>> c: add below APIs in rte eal device layer.
>>     rte_dev_callback_register
>>     rte_dev_callback_unregister
>>     _rte_dev_callback_process
>>     rte_dev_event_monitor_start
>>     rte_dev_event_monitor_stop
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> Hi Jeff,
>
>
>> ---
>> v13->v12:
>> fix some logic issue and null check issue
>> fix monitor stop func issue and bsp build issue
> <snip>
>
>> +int
>> +rte_dev_event_monitor_start(void)
>> +{
>> +	int ret;
>> +	struct rte_service_spec service;
>> +	const uint32_t sid = 0;
>> +
>> +	if (!service_no_init)
>> +		return 0;
>> +
>> +	slcore = rte_get_next_lcore(/* start core */ -1,
>> +					       /* skip master */ 1,
>> +					       /* wrap */ 0);
>> +
>> +	ret = rte_service_lcore_add(slcore);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "dev event monitor lcore add fail");
>> +		return ret;
>> +	}
>> +
> I don't think you should be taking another service core for this purpose
> without the user asking for it. I also don't think service cores is the
> right "tool" for monitoring the epoll. Rather than using a non-blocking
> poll on a service core, I think you should look to reuse the existing
> infrastructure for handling interrupts in the EAL, which relies on a
> separate thread blocked on fd's awaiting input.
bruce, seems that you might be see the other view of the mountain, so if 
service cores tools basically be born to  need user knowledge and 
control it, and it is no need to add user to control service tool in the 
case, i thinks we might not use the existing interrupts infrastructure 
because it is the device uevent not interrupt as the same functional 
scope ,  we could use a separate thread which i have used before in v7 
to specialize poll the uevent, please check v7 part to see if it is good.

@tomas, do you agree with that above , or other suggestion, could it be 
got agreement all or let it improvement later?
>> +	memset(&service, 0, sizeof(service));
>> +	snprintf(service.name, sizeof(service.name), DEV_EV_MNT_SERVICE_NAME);
>> +
>> +	service.socket_id = rte_socket_id();
>> +	service.callback = dev_uev_monitoring;
>> +	service.callback_userdata = NULL;
>> +	service.capabilities = 0;
>> +	ret = rte_service_component_register(&service, &sevice_id);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "Failed to register service %s "
>> +			"err = %" PRId32,
>> +			service.name, ret);
>> +		return ret;
>> +	}
>> +	ret = rte_service_runstate_set(sid, 1);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "Failed to set the runstate of "
>> +			"the service");
>> +		goto err_done;
>> +	}
>> +	ret = rte_service_component_runstate_set(sevice_id, 1);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "Failed to set the backend runstate"
>> +			" of a component");
>> +		return ret;
>> +	}
>> +	ret = rte_service_map_lcore_set(sid, slcore, 1);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "Failed to enable lcore 1 on "
>> +			"dev event monitor service");
>> +		return ret;
>> +	}
>> +	rte_service_lcore_start(slcore);
>> +	service_no_init = false;
>> +	return 0;
>> +
>> +err_done:
>> +	rte_service_component_unregister(sevice_id);
>> +	return ret;
>> +}
>> +
> Regards,
> /Bruce