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 [thread overview]
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
next prev 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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).