From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 33F61A057B; Wed, 1 Apr 2020 07:29:39 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D38E41BEA6; Wed, 1 Apr 2020 07:29:37 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 53FB61BE91 for ; Wed, 1 Apr 2020 07:29:36 +0200 (CEST) IronPort-SDR: 5CTVrWmrNONuTDj9bdOo1QYpse5nrHiKOhM003ERBMEPp+N01Ru2YVq7u3rYM857I7WN580KyN bhZyWqF+XuhA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Mar 2020 22:29:35 -0700 IronPort-SDR: LyrlR+IpJi+HoFRHl3DL1EWvbUkZ+Sb/AUMJOype1tnkDwxFDFGvhOO4eoKsC2youmoPjq24Tg 0Rbg+BMpugkw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,330,1580803200"; d="scan'208";a="359744660" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga001.fm.intel.com with ESMTP; 31 Mar 2020 22:29:35 -0700 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 31 Mar 2020 22:29:35 -0700 Received: from bgsmsx110.gar.corp.intel.com (10.223.4.212) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 31 Mar 2020 22:29:34 -0700 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.36]) by BGSMSX110.gar.corp.intel.com ([169.254.11.98]) with mapi id 14.03.0439.000; Wed, 1 Apr 2020 10:59:33 +0530 From: "Varghese, Vipin" To: Andrzej Ostruszka , "dev@dpdk.org" , Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH 1/4] lib: introduce IF Proxy library Thread-Index: AQHV89YgT9bQ09qV3keT1KLLnUVs3qhjxfJA Date: Wed, 1 Apr 2020 05:29:32 +0000 Message-ID: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D49923E@BGSMSX101.gar.corp.intel.com> References: <20200306164104.15528-1-aostruszka@marvell.com> <20200306164104.15528-2-aostruszka@marvell.com> In-Reply-To: <20200306164104.15528-2-aostruszka@marvell.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.223.10.10] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/4] lib: introduce IF Proxy library X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 =3D librte_if_proxy.a > + > +CFLAGS +=3D -DALLOW_EXPERIMENTAL_API > +CFLAGS +=3D -O3 > +CFLAGS +=3D $(WERROR_FLAGS) -I$(SRCDIR) > +LDLIBS +=3D -lrte_eal -lrte_ethdev > + > +EXPORT_MAP :=3D rte_if_proxy_version.map > + > +LIBABIVER :=3D 1 > + > +# all source are stored in SRCS-y > +SRCS-$(CONFIG_RTE_LIBRTE_IF_PROXY) :=3D if_proxy_common.c > + > +SYSDIR :=3D $(patsubst "%app",%,$(CONFIG_RTE_EXEC_ENV)) > +include $(SRCDIR)/$(SYSDIR)/Makefile > + Should there be check `ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)` and `ifeq ($(CONF= IG_RTE_LIBRTE_TAP),y)`? > +SRCS-$(CONFIG_RTE_LIBRTE_IF_PROXY) +=3D $(addprefix $(SYSDIR)/,$(SRCS)) > + > +# install this header file > +SYMLINK-$(CONFIG_RTE_LIBRTE_IF_PROXY)-include :=3D 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 >=3D 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 =3D 0; > + > + for (p =3D 0; p < RTE_DIM(ifpx_ports); ++p) { > + if (ifpx_ports[p] =3D=3D proxy_id && ifpx_ports[p] !=3D p) { > + ++cnt; > + if (ports && num > 0) { > + *ports++ =3D p; > + --num; > + } > + } > + } Application can dynamically ports to DPDK. if this is correct, will this re= quire 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 >=3D RTE_MAX_ETHPORTS || > + ifpx_ports[port_id] =3D=3D RTE_MAX_ETHPORTS) > + return NULL; > + > + rte_spinlock_lock(&ifpx_lock); > + TAILQ_FOREACH(px, &ifpx_proxies, elem) { > + if (px->proxy_id =3D=3D 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 =3D 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 =3D *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 =3D 0; > + uint16_t p, proxy_id; > + > + if (px) { > + if (px->state & DEL_PENDING) > + return; > + proxy_id =3D px->proxy_id; > + RTE_ASSERT(proxy_id !=3D RTE_MAX_ETHPORTS); > + px->state |=3D IN_USE; > + } else > + proxy_id =3D RTE_MAX_ETHPORTS; > + > + RTE_ASSERT(ev); > + /* This function is expected to be called with a lock held. */ > + RTE_ASSERT(rte_spinlock_trylock(&ifpx_lock) =3D=3D 0); > + > + if (ifpx_callbacks.funcs[ev->type].f_ptr) { > + union cb_ptr_t cb =3D ifpx_callbacks.funcs[ev->type]; > + > + /* Drop the lock for the time of callback call. */ > + rte_spinlock_unlock(&ifpx_lock); > + if (px) { > + for (p =3D 0; p < RTE_DIM(ifpx_ports); ++p) { > + if (ifpx_ports[p] !=3D proxy_id || > + ifpx_ports[p] =3D=3D p) > + continue; > + ev->data.port_id =3D p; > + done =3D cb.f_ptr(&ev->data) || done; > + } > + } else { > + RTE_ASSERT(ev->type =3D=3D RTE_IFPX_CFG_DONE); > + done =3D 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 meth= od above by listener? > + TAILQ_FOREACH(q, &ifpx_queues, elem) { > + if (px) { > + for (p =3D 0; p < RTE_DIM(ifpx_ports); ++p) { > + if (ifpx_ports[p] !=3D proxy_id || > + ifpx_ports[p] =3D=3D p) > + continue; > + /* Set the port_id - the remaining params > should > + * be filled before calling this function. > + */ > + ev->data.port_id =3D p; > + queue_event(ev, q->r); > + } > + } else > + queue_event(ev, q->r); > + } > +exit: > + if (px) > + px->state &=3D ~IN_USE; > +} Snipped > + > +RTE_INIT(if_proxy_init) > +{ > + unsigned int i; Is IF_PROXY supported for vdev also?=20 > + for (i =3D 0; i < RTE_DIM(ifpx_ports); ++i) > + ifpx_ports[i] =3D RTE_MAX_ETHPORTS; > + > + ifpx_log_type =3D rte_log_register("lib.if_proxy"); > + if (ifpx_log_type >=3D 0) > + rte_log_set_level(ifpx_log_type, RTE_LOG_WARNING); > + > + if (ifpx_platform.init) > + ifpx_platform.init(); > +} Snipped > +SRCS +=3D 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 i= f 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 =3D false > + reason =3D 'only supported on linux' > +endif > + > +version =3D 1 > +allow_experimental_apis =3D true > + > +deps +=3D ['ethdev'] > +sources =3D files('if_proxy_common.c') > +headers =3D files('rte_if_proxy.h') Does the if_proxy have dependency on TAP and KNI. Should not we add check a= s ` if dpdk_conf.has('RTE_LIBRTE_KNI')` and ` if dpdk_conf.has('RTE_LIBRTE_= TAP')`? > + > +if is_linux > + sources +=3D files('linux/if_proxy.c') > +endif Snipped