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

["Andrzej Ostruszka [C]" <aostruszka@marvell.com>]

First of all thank you Vipin for taking a look at this.

On 4/1/20 7:29 AM, Varghese, Vipin wrote:
> 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)`?

Might not be necessary.  While it is true that if you want to create
proxy via this lib, then currently it is only KNI or TAP.  However any
DPDK port can act as a proxy - as long as it is visible to the system
and reports non-zero if_index in its dev_info.

However it is true that if we allow building of if_proxy even if TAP/KNI
is not enabled then I should add conditionals to the proxy creation
function that would show some meaningful warning when they are not
enabled.  Will take a look at this.

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

I'm not sure I follow what you wanted to ask.  I want to return bitmask
with each bit being lit for every event type.  I could go with or'ing of
all (1ULL << RTE_IFPX_MAC_CHANGE) | (1ULL << RTE_IFPX_MTU_CHANGE) ...
but deemed that this would be simpler.

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

ifpx_events_notify() is called under the same lock.  So either someone
calls this unregister and then notify will not find any callback or the
other way.  Note that notify drops the lock for the time of callback
call (to allow modifications from the callback) but the pointer is first
copied - so the behaviour would be as if the unregister was called later.

I'm not sure I answered your question though - if not then please ask
again with some more details.

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

Here port_id is an input from user - (s)he can make an error.
Internally this should never happen.

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

This is a good point.  Currently ifpx_ports is not protected by the
lock.  Since this is a slow/control path I'll go with moving this under
lock instead of trying to make this lockless.

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

This is a practice sometimes used to have a meaningful error message
printed (together with an expression) while assertion fires.  The value
of expression does not depend on this string but the expression is
"stringified" in macro and printed on console so that way you can add
some message to the condition being checked.  I think this is the only
public function where I've used this - all internal ASSERTS have no
message so I might drop it here if you want.

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

Not really - that was actually a question of mine recently on this list.
This is a slow/control path, so maybe we should save hugepage memory for
the fast path?  I have no strong opinion here and can switch to
rte_malloc() if that is thought as a better option.

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

This is fully under control of application - if application wants
certain events to be notified by the queues then either it should not
register callback for that event type or, if it registers, then this
callback should not return non-zero value (just do some common
preparation or something like that).

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

I'm not sure I understand the question here.  Any port can be bound to a
proxy (vdev or not) and any port visible to system (having non-zero
if_index in dev_info) can be used as a proxy.  Does that answers your
question?  If not please explain.

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

Yes that was my assumption however at the moment I'm using:
- rte_eth_iterator_init/next/cleanup()
- rte_eth_dev_info_get()
- rte_eth_dev_get_mtu()
- rte_eth_macaddr_get()
- rte_eth_dev_mac_addr_add()
- rte_dev_probe/remove()

Is there a problem with these?  If it is, then I'll think about adding
check for secondary.

> 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')`?

This is the same as for Makefile - I think I'll go with allowing it to
build but adding conditionals in proxy creation.  However if you and/or
others think it would be better to skip build then I will adapt.

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

Thanks for reviewing this.

With regards
Andrzej