DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Varghese, Vipin" <vipin.varghese@intel.com>
To: Andrzej Ostruszka <aostruszka@marvell.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	 Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH 1/4] lib: introduce IF Proxy library
Date: Wed, 1 Apr 2020 05:29:32 +0000
Message-ID: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D49923E@BGSMSX101.gar.corp.intel.com> (raw)
In-Reply-To: <20200306164104.15528-2-aostruszka@marvell.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

  parent reply	other threads:[~2020-04-01  5:29 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-06 16:41 [dpdk-dev] [PATCH 0/4] Introduce IF proxy library Andrzej Ostruszka
2020-03-06 16:41 ` [dpdk-dev] [PATCH 1/4] lib: introduce IF Proxy library Andrzej Ostruszka
2020-03-31 12:36   ` Harman Kalra
2020-03-31 15:37     ` Andrzej Ostruszka [C]
2020-04-01  5:29   ` Varghese, Vipin [this message]
2020-04-01 20:08     ` Andrzej Ostruszka [C]
2020-04-08  3:04       ` Varghese, Vipin
2020-04-08 18:13         ` Andrzej Ostruszka [C]
2020-03-06 16:41 ` [dpdk-dev] [PATCH 2/4] if_proxy: add library documentation Andrzej Ostruszka
2020-03-06 16:41 ` [dpdk-dev] [PATCH 3/4] if_proxy: add simple functionality test Andrzej Ostruszka
2020-03-06 16:41 ` [dpdk-dev] [PATCH 4/4] if_proxy: add example application Andrzej Ostruszka
2020-03-06 17:17 ` [dpdk-dev] [PATCH 0/4] Introduce IF proxy library Andrzej Ostruszka
2020-03-10 11:10 ` [dpdk-dev] [PATCH v2 " Andrzej Ostruszka
2020-03-10 11:10   ` [dpdk-dev] [PATCH v2 1/4] lib: introduce IF Proxy library Andrzej Ostruszka
2020-07-02  0:34     ` Stephen Hemminger
2020-07-07 20:13       ` Andrzej Ostruszka [C]
2020-07-08 16:07         ` Morten Brørup
2020-07-09  8:43           ` Andrzej Ostruszka [C]
2020-07-22  0:40             ` Thomas Monjalon
2020-07-22  8:45               ` Jerin Jacob
2020-07-22  8:56                 ` Thomas Monjalon
2020-07-22  9:09                   ` Jerin Jacob
2020-07-22  9:27                     ` Thomas Monjalon
2020-07-22  9:54                       ` Jerin Jacob
2020-07-23 14:09                         ` [dpdk-dev] [EXT] " Andrzej Ostruszka [C]
2020-03-10 11:10   ` [dpdk-dev] [PATCH v2 2/4] if_proxy: add library documentation Andrzej Ostruszka
2020-03-10 11:10   ` [dpdk-dev] [PATCH v2 3/4] if_proxy: add simple functionality test Andrzej Ostruszka
2020-03-10 11:10   ` [dpdk-dev] [PATCH v2 4/4] if_proxy: add example application Andrzej Ostruszka
2020-03-25  8:08   ` [dpdk-dev] [PATCH v2 0/4] Introduce IF proxy library David Marchand
2020-03-25 11:11     ` Morten Brørup
2020-03-26 17:42       ` Andrzej Ostruszka
2020-04-02 13:48         ` Andrzej Ostruszka [C]
2020-04-03 17:19           ` Thomas Monjalon
2020-04-03 19:09             ` Jerin Jacob
2020-04-03 21:18               ` Morten Brørup
2020-04-03 21:57                 ` Thomas Monjalon
2020-04-04 10:18                   ` Jerin Jacob
2020-04-10 10:41                     ` Morten Brørup
2020-04-04 18:30             ` Andrzej Ostruszka [C]
2020-04-04 19:58               ` Thomas Monjalon
2020-04-10 10:03                 ` Morten Brørup
2020-04-10 12:28                   ` Jerin Jacob
2020-03-26 12:41     ` Andrzej Ostruszka
2020-03-30 19:23       ` Andrzej Ostruszka
2020-04-03 21:42   ` Thomas Monjalon
2020-04-04 18:07     ` Andrzej Ostruszka [C]
2020-04-04 19:51       ` Thomas Monjalon
2020-04-16 16:11 ` [dpdk-dev] [PATCH " Stephen Hemminger
2020-04-16 16:49   ` Jerin Jacob
2020-04-16 17:04     ` Stephen Hemminger
2020-04-16 17:26       ` Andrzej Ostruszka [C]
2020-04-16 17:27       ` Jerin Jacob
2020-04-16 17:12     ` Andrzej Ostruszka [C]
2020-04-16 17:19       ` Stephen Hemminger
2020-05-04  8:53 ` [dpdk-dev] [PATCH v3 " Andrzej Ostruszka
2020-05-04  8:53   ` [dpdk-dev] [PATCH v3 1/4] lib: introduce IF Proxy library Andrzej Ostruszka
2020-05-04  8:53   ` [dpdk-dev] [PATCH v3 2/4] if_proxy: add library documentation Andrzej Ostruszka
2020-05-04  8:53   ` [dpdk-dev] [PATCH v3 3/4] if_proxy: add simple functionality test Andrzej Ostruszka
2020-05-04  8:53   ` [dpdk-dev] [PATCH v3 4/4] if_proxy: add example application Andrzej Ostruszka
2020-06-22  9:21 ` [dpdk-dev] [PATCH v4 0/4] Introduce IF proxy library Andrzej Ostruszka
2020-06-22  9:21   ` [dpdk-dev] [PATCH v4 1/4] lib: introduce IF Proxy library Andrzej Ostruszka
2020-06-22  9:21   ` [dpdk-dev] [PATCH v4 2/4] if_proxy: add library documentation Andrzej Ostruszka
2020-06-22  9:21   ` [dpdk-dev] [PATCH v4 3/4] if_proxy: add simple functionality test Andrzej Ostruszka
2020-06-22  9:21   ` [dpdk-dev] [PATCH v4 4/4] if_proxy: add example application Andrzej Ostruszka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4C9E0AB70F954A408CC4ADDBF0F8FA7D4D49923E@BGSMSX101.gar.corp.intel.com \
    --to=vipin.varghese@intel.com \
    --cc=aostruszka@marvell.com \
    --cc=dev@dpdk.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git