From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bruce Richardson <bruce.richardson@intel.com>
Subject: Re: [PATCH V13 1/3] eal: add uevent monitor api and
	callback func
Date: Fri, 26 Jan 2018 16:53:41 +0000
Message-ID: <20180126165340.GA21468@bricha3-MOBL3.ger.corp.intel.com>
References: <1516248723-16985-3-git-send-email-jia.guo@intel.com>
 <1516938577-27662-1-git-send-email-jia.guo@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: stephen@networkplumber.org, gaetan.rivet@6wind.com,
 jingjing.wu@intel.com, thomas@monjalon.net, 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: Jeff Guo <jia.guo@intel.com>
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 C07081B2ED
 for <dev@dpdk.org>; Fri, 26 Jan 2018 17:53:48 +0100 (CET)
Content-Disposition: inline
In-Reply-To: <1516938577-27662-1-git-send-email-jia.guo@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 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.

> +	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