Re: [dpdk-dev] [PATCH 1/4] lib: introduce IF Proxy library

["Varghese, Vipin" <vipin.varghese@intel.com>]

snipped
> diff --git a/lib/librte_if_proxy/Makefile b/lib/librte_if_proxy/Makefile
> new file mode 100644
> index 000000000..43cb702a2
> --- /dev/null
> +++ b/lib/librte_if_proxy/Makefile
> @@ -0,0 +1,29 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(C) 2020 Marvell International Ltd.
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +# library name
> +LIB = librte_if_proxy.a
> +
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR)
> +LDLIBS += -lrte_eal -lrte_ethdev
> +
> +EXPORT_MAP := rte_if_proxy_version.map
> +
> +LIBABIVER := 1
> +
> +# all source are stored in SRCS-y
> +SRCS-$(CONFIG_RTE_LIBRTE_IF_PROXY) := if_proxy_common.c
> +
> +SYSDIR := $(patsubst "%app",%,$(CONFIG_RTE_EXEC_ENV))
> +include $(SRCDIR)/$(SYSDIR)/Makefile
> +
Should there be check `ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)` and `ifeq ($(CONFIG_RTE_LIBRTE_TAP),y)`?

> +SRCS-$(CONFIG_RTE_LIBRTE_IF_PROXY) += $(addprefix $(SYSDIR)/,$(SRCS))
> +
> +# install this header file
> +SYMLINK-$(CONFIG_RTE_LIBRTE_IF_PROXY)-include := rte_if_proxy.h
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
Snipped

> +
> +uint64_t rte_ifpx_events_available(void)
> +{
> +	/* All events are supported on Linux. */
> +	return (1ULL << RTE_IFPX_NUM_EVENTS) - 1;
Should we give the available from the used count?

> +}
> +

Snipped

> +
> +void rte_ifpx_callbacks_unregister(void)
> +{
> +	rte_spinlock_lock(&ifpx_lock);
> +	memset(&ifpx_callbacks.cbs, 0, sizeof(ifpx_callbacks.cbs));
What would happen to pending events, are agreeing to drop all?

> +	rte_spinlock_unlock(&ifpx_lock);
> +}
> +
> +uint16_t rte_ifpx_proxy_get(uint16_t port_id)
> +{
> +	if (port_id >= RTE_MAX_ETHPORTS)
> +		return RTE_MAX_ETHPORTS;
In the init function, the default value is set with RTE_MAX_ETHPORTS. Will there be a scenario port_id can be greater?

> +
> +	return ifpx_ports[port_id];
> +}
> +
> +unsigned int rte_ifpx_port_get(uint16_t proxy_id,
> +			       uint16_t *ports, unsigned int num)
> +{
> +	unsigned int p, cnt = 0;
> +
> +	for (p = 0; p < RTE_DIM(ifpx_ports); ++p) {
> +		if (ifpx_ports[p] == proxy_id && ifpx_ports[p] != p) {
> +			++cnt;
> +			if (ports && num > 0) {
> +				*ports++ = p;
> +				--num;
> +			}
> +		}
> +	}
Application can dynamically ports to DPDK. if this is correct, will this require lock to make this thread safe?

> +	return cnt;
> +}
> +
> +const struct rte_ifpx_info *rte_ifpx_info_get(uint16_t port_id)
> +{
> +	struct ifpx_proxy_node *px;
> +
> +	if (port_id >= RTE_MAX_ETHPORTS ||
> +	    ifpx_ports[port_id] == RTE_MAX_ETHPORTS)
> +		return NULL;
> +
> +	rte_spinlock_lock(&ifpx_lock);
> +	TAILQ_FOREACH(px, &ifpx_proxies, elem) {
> +		if (px->proxy_id == ifpx_ports[port_id])
> +			break;
> +	}
> +	rte_spinlock_unlock(&ifpx_lock);
> +	RTE_ASSERT(px && "Internal IF Proxy library error");

Can you help me understand the assert logic with const string?

> +
> +	return &px->info;
> +}
> +
> +static
> +void queue_event(const struct rte_ifpx_event *ev, struct rte_ring *r)
> +{
> +	struct rte_ifpx_event *e = malloc(sizeof(*ev));
Is there specific reason not to use rte_malloc?

> +
> +	if (!e) {
> +		IFPX_LOG(ERR, "Failed to allocate event!");
> +		return;
> +	}
> +	RTE_ASSERT(r);
> +
> +	*e = *ev;
> +	rte_ring_sp_enqueue(r, e);
> +}
> +
> +void ifpx_notify_event(struct rte_ifpx_event *ev, struct ifpx_proxy_node *px)
> +{
> +	struct ifpx_queue_node *q;
> +	int done = 0;
> +	uint16_t p, proxy_id;
> +
> +	if (px) {
> +		if (px->state & DEL_PENDING)
> +			return;
> +		proxy_id = px->proxy_id;
> +		RTE_ASSERT(proxy_id != RTE_MAX_ETHPORTS);
> +		px->state |= IN_USE;
> +	} else
> +		proxy_id = RTE_MAX_ETHPORTS;
> +
> +	RTE_ASSERT(ev);
> +	/* This function is expected to be called with a lock held. */
> +	RTE_ASSERT(rte_spinlock_trylock(&ifpx_lock) == 0);
> +
> +	if (ifpx_callbacks.funcs[ev->type].f_ptr) {
> +		union cb_ptr_t cb = ifpx_callbacks.funcs[ev->type];
> +
> +		/* Drop the lock for the time of callback call. */
> +		rte_spinlock_unlock(&ifpx_lock);
> +		if (px) {
> +			for (p = 0; p < RTE_DIM(ifpx_ports); ++p) {
> +				if (ifpx_ports[p] != proxy_id ||
> +				    ifpx_ports[p] == p)
> +					continue;
> +				ev->data.port_id = p;
> +				done = cb.f_ptr(&ev->data) || done;
> +			}
> +		} else {
> +			RTE_ASSERT(ev->type == RTE_IFPX_CFG_DONE);
> +			done = cb.cfg_done();
> +		}
> +		rte_spinlock_lock(&ifpx_lock);
> +	}
> +	if (done)
> +		goto exit;
> +
> +	/* Event not "consumed" yet so try to notify via queues. */

Is there a chance when trying to use queues the events are consumed by method above by listener?

> +	TAILQ_FOREACH(q, &ifpx_queues, elem) {
> +		if (px) {
> +			for (p = 0; p < RTE_DIM(ifpx_ports); ++p) {
> +				if (ifpx_ports[p] != proxy_id ||
> +				    ifpx_ports[p] == p)
> +					continue;
> +				/* Set the port_id - the remaining params
> should
> +				 * be filled before calling this function.
> +				 */
> +				ev->data.port_id = p;
> +				queue_event(ev, q->r);
> +			}
> +		} else
> +			queue_event(ev, q->r);
> +	}
> +exit:
> +	if (px)
> +		px->state &= ~IN_USE;
> +}

Snipped

> +
> +RTE_INIT(if_proxy_init)
> +{
> +	unsigned int i;

Is IF_PROXY supported for vdev also? 

> +	for (i = 0; i < RTE_DIM(ifpx_ports); ++i)
> +		ifpx_ports[i] = RTE_MAX_ETHPORTS;
> +
> +	ifpx_log_type = rte_log_register("lib.if_proxy");
> +	if (ifpx_log_type >= 0)
> +		rte_log_set_level(ifpx_log_type, RTE_LOG_WARNING);
> +
> +	if (ifpx_platform.init)
> +		ifpx_platform.init();
> +}
Snipped

> +SRCS += if_proxy.c
> diff --git a/lib/librte_if_proxy/linux/if_proxy.c
> b/lib/librte_if_proxy/linux/if_proxy.c
> new file mode 100644
> index 000000000..bf851c096
> --- /dev/null
> +++ b/lib/librte_if_proxy/linux/if_proxy.c
> @@ -0,0 +1,552 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */

Assuming all the events are executed `if and only if` the current process if Primary? If it is secondary for physical interface certain `rte_eth_api` will fail. Can we have check the events are processed for primary only?

Snipped

> diff --git a/lib/librte_if_proxy/meson.build b/lib/librte_if_proxy/meson.build
> new file mode 100644
> index 000000000..f0c1a6e15
> --- /dev/null
> +++ b/lib/librte_if_proxy/meson.build
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(C) 2020 Marvell International Ltd.
> +
> +# Currently only implemented on Linux
> +if not is_linux
> +	build = false
> +	reason = 'only supported on linux'
> +endif
> +
> +version = 1
> +allow_experimental_apis = true
> +
> +deps += ['ethdev']
> +sources = files('if_proxy_common.c')
> +headers = files('rte_if_proxy.h')

Does the if_proxy have dependency on TAP and KNI. Should not we add check as ` if dpdk_conf.has('RTE_LIBRTE_KNI')` and ` if dpdk_conf.has('RTE_LIBRTE_TAP')`?

> +
> +if is_linux
> +	sources += files('linux/if_proxy.c')
> +endif

Snipped