DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Andrzej Ostruszka [C]" <aostruszka@marvell.com>
To: "Varghese, Vipin" <vipin.varghese@intel.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 20:08:52 +0000
Message-ID: <19d195cd-cb73-6388-0a85-aca74e963003@marvell.com> (raw)
In-Reply-To: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D49923E@BGSMSX101.gar.corp.intel.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

  reply	other threads:[~2020-04-01 20:08 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
2020-04-01 20:08     ` Andrzej Ostruszka [C] [this message]
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=19d195cd-cb73-6388-0a85-aca74e963003@marvell.com \
    --to=aostruszka@marvell.com \
    --cc=dev@dpdk.org \
    --cc=thomas@monjalon.net \
    --cc=vipin.varghese@intel.com \
    /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 http://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/ http://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