From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id E3FB323C for ; Mon, 16 Jul 2018 10:35:10 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jul 2018 01:35:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,360,1526367600"; d="scan'208";a="67259140" Received: from orsmsx103.amr.corp.intel.com ([10.22.225.130]) by orsmga003.jf.intel.com with ESMTP; 16 Jul 2018 01:34:28 -0700 Received: from orsmsx112.amr.corp.intel.com (10.22.240.13) by ORSMSX103.amr.corp.intel.com (10.22.225.130) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 16 Jul 2018 01:34:27 -0700 Received: from orsmsx110.amr.corp.intel.com ([169.254.10.90]) by ORSMSX112.amr.corp.intel.com ([169.254.3.12]) with mapi id 14.03.0319.002; Mon, 16 Jul 2018 01:34:27 -0700 From: "Rao, Nikhil" To: Jerin Jacob CC: "olivier.matz@6wind.com" , "dev@dpdk.org" , "anoob.joseph@cavium.com" Thread-Topic: [PATCH 1/4] eventdev: add eth Tx adapter APIs Thread-Index: AQHUFPSEvE+AyFgccEiZ3zBKa6JaO6SI22EAgAiSygA= Date: Mon, 16 Jul 2018 08:34:26 +0000 Message-ID: <1F668163772FA946975B9466A9DFF729ED315CAA@ORSMSX110.amr.corp.intel.com> References: <1530859329-160189-1-git-send-email-nikhil.rao@intel.com> <20180710121728.GA11131@jerin> In-Reply-To: <20180710121728.GA11131@jerin> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [10.22.254.140] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/4] eventdev: add eth Tx adapter APIs 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: , X-List-Received-Date: Mon, 16 Jul 2018 08:35:11 -0000 > -----Original Message----- > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com] > Sent: Tuesday, July 10, 2018 5:48 PM > To: Rao, Nikhil > Cc: olivier.matz@6wind.com; dev@dpdk.org; anoob.joseph@cavium.com > Subject: Re: [PATCH 1/4] eventdev: add eth Tx adapter APIs >=20 > --- >=20 > 1) Update doc/api/doxy-api-index.md OK. > 2) Update lib/librte_eventdev/Makefile > +SYMLINK-y-include +=3D rte_event_eth_tx_adapter.h >=20 This is done in patch 3 of this series. >=20 > I think, the following working is _pending_ >=20 > 1) Update app/test-eventdev/ for Tx adapter > 2) Update examples/eventdev_pipeline/ for Tx adapter > 3) Add Tx adapter documentation > 4) Add Tx adapter ops for octeontx driver > 5) Add Tx adapter ops for dpaa driver(if need) >=20 > Nikhil, > If you are OK then Cavium would like to take up (1), (2) and (4) activiti= es. >=20 > Let me know your thoughts. >=20 Fine with me. > Since this patch set already crossed the RC1 deadline. We will complete a= ll > the _pending_ work and push to next-eventdev tree in the very beginning > of > v18.11 so that Anoob's adapter helper function work can be added v18.11. >=20 >=20 > > > > This patch series adds the event ethernet Tx adapter which is based on > > a previous RFC > > * RFCv1 - http://mails.dpdk.org/archives/dev/2018-May/102936.html > > * RFCv2 - http://mails.dpdk.org/archives/dev/2018-June/104075.html > > > > RFC -> V1: > > =3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > * Move port and tx queue id to mbuf from mbuf private area. (Jerin > > Jacob) > > > > * Support for PMD transmit function. (Jerin Jacob) > > > > * mbuf change has been replaced with > rte_event_eth_tx_adapter_txq_set(). > > The goal is to align with the mbuf change for a qid field. > > (http://mails.dpdk.org/archives/dev/2018-February/090651.html). Once > > the mbuf change is available, the function can be replaced with a > > macro with no impact to applications. > > > > * Various cleanups (Jerin Jacob) > > > > lib/librte_eventdev/rte_event_eth_tx_adapter.h | 497 > +++++++++++++++++++++++++ > > lib/librte_mbuf/rte_mbuf.h | 4 +- > > MAINTAINERS | 5 + > > 3 files changed, 505 insertions(+), 1 deletion(-) create mode 100644 > > lib/librte_eventdev/rte_event_eth_tx_adapter.h > > > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * A structure used to retrieve statistics for an eth tx adapter insta= nce. > > + */ > > +struct rte_event_eth_tx_adapter_stats { > > + uint64_t tx_retry; > > + /**< Number of transmit retries */ > > + uint64_t tx_packets; > > + /**< Number of packets transmitted */ > > + uint64_t tx_dropped; > > + /**< Number of packets dropped */ }; > > + > > +/** Event Eth Tx Adapter Structure */ struct rte_event_eth_tx_adapter > > +{ > > + uint8_t id; > > + /**< Adapter Identifier */ > > + uint8_t eventdev_id; > > + /**< Max mbufs processed in any service function invocation */ > > + uint32_t max_nb_tx; > > + /**< The adapter can return early if it has processed at least > > + * max_nb_tx mbufs. This isn't treated as a requirement; batchi= ng > may > > + * cause the adapter to process more than max_nb_tx mbufs. > > + */ > > + uint32_t nb_queues; > > + /**< Number of Tx queues in adapter */ > > + int socket_id; > > + /**< socket id */ > > + rte_spinlock_t tx_lock; > > + /**< Synchronization with data path */ > > + void *dev_private; > > + /**< PMD private data */ > > + char > mem_name[RTE_EVENT_ETH_TX_ADAPTER_SERVICE_NAME_LEN]; > > + /**< Memory allocation name */ > > + rte_event_eth_tx_adapter_conf_cb conf_cb; > > + /** Configuration callback */ > > + void *conf_arg; > > + /**< Configuration callback argument */ > > + uint16_t dev_count; > > + /**< Highest port id supported + 1 */ > > + struct rte_event_eth_tx_adapter_ethdev *txa_ethdev; > > + /**< Per ethernet device structure */ > > + struct rte_event_eth_tx_adapter_stats stats; } > > +__rte_cache_aligned; >=20 > Can you move this structure to .c file as implementation, Reasons are - > a) It should not be under ABI deprecation > b) INTERNAL_PORT based adapter may have different values.i.e the above > structure is implementation defined. > > > + > > +struct rte_event_eth_tx_adapters { > > + struct rte_event_eth_tx_adapter **data; }; > > + >=20 > same as above >=20 > > +/* Per eth device structure */ > > +struct rte_event_eth_tx_adapter_ethdev { > > + /* Pointer to ethernet device */ > > + struct rte_eth_dev *dev; > > + /* Number of queues added */ > > + uint16_t nb_queues; > > + /* PMD specific queue data */ > > + void *queues; > > +}; >=20 > same as above >=20 > > + > > +extern struct rte_event_eth_tx_adapters rte_event_eth_tx_adapters; > > + >=20 > same as above > OK, if these fields are not going to be used within the other adapter, I wi= ll move these to the .c file. =20 > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Create a new event ethernet Tx adapter with the specified identifie= r. > > + * > > + * @param id > > + * The identifier of the event ethernet Tx adapter. > > + * @param dev_id > > + * The event device identifier. > > + * @param port_config > > + * Event port configuration, the adapter uses this configuration to > > + * create an event port if needed. > > + * @return > > + * - 0: Success > > + * - <0: Error code on failure > > + */ > > +int __rte_experimental > > +rte_event_eth_tx_adapter_create(uint8_t id, uint8_t dev_id, > > + struct rte_event_port_conf > > +*port_config); > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Create a new event ethernet Tx adapter with the specified identifie= r. > > + * > > + * @param id > > + * The identifier of the event ethernet Tx adapter. > > + * @param dev_id > > + * The event device identifier. > > + * @param conf_cb > > + * Callback function that initalizes members of the >=20 > s/initalizes/initializes >=20 > > + * struct rte_event_eth_tx_adapter_conf struct passed into > > + * it. > > + * @param conf_arg > > + * Argument that is passed to the conf_cb function. > > + * @return > > + * - 0: Success > > + * - <0: Error code on failure > > + */ > > +int __rte_experimental > > +rte_event_eth_tx_adapter_create_ext(uint8_t id, uint8_t dev_id, > > + rte_event_eth_tx_adapter_conf_cb conf_c= b, > > + void *conf_arg); > > + > > +/** > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Add a Tx queue to the adapter. > > + * A queue value of -1 is used to indicate all > > + * queues within the device. > > + * > > + * @param id > > + * Adapter identifier. > > + * @param eth_dev_id > > + * Ethernet Port Identifier. > > + * @param queue > > + * Tx queue index. > > + * @return > > + * - 0: Success, Queues added succcessfully. >=20 > s/succcessfully/successfully >=20 >=20 > > + * - <0: Error code on failure. > > + */ > > +int __rte_experimental > > +rte_event_eth_tx_adapter_queue_add(uint8_t id, > > + uint16_t eth_dev_id, > > + int32_t queue); > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * > > + * Set Tx queue in the mbuf. > > + * > > + * @param pkt > > + * Pointer to the mbuf. > > + * @param queue > > + * Tx queue index. > > + */ > > +void __rte_experimental > > +rte_event_eth_tx_adapter_txq_set(struct rte_mbuf *pkt, uint16_t > > +queue); >=20 > 1) Can you make this as static inline for better performance(as it is jus= t a > mbuf field access)? OK. This would also move the private definition of struct txa_mbuf_txq_id to= the adapter header file, which would be needed to deprecated once the fiel= d is available in rte_mbuf.h. >=20 > 2) Please add _get function, It will be useful for application and Tx ada= pter > op implementation. >=20 >=20 OK. > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Retrieve the adapter event port. The adapter creates an event port > > +if > > + * the RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT is not set in > the > > + * eth Tx capabilities of the event device. > > + * > > + * @param id > > + * Adapter Identifier. > > + * @param[out] event_port_id > > + * Event port pointer. > > + * @return > > + * - 0: Success. > > + * - <0: Error code on failure. > > + */ > > +int __rte_experimental > > +rte_event_eth_tx_adapter_event_port_get(uint8_t id, uint8_t > > +*event_port_id); > > + > > +static __rte_always_inline uint16_t __rte_experimental > > +__rte_event_eth_tx_adapter_enqueue(uint8_t id, uint8_t dev_id, > uint8_t port_id, > > + struct rte_event ev[], > > + uint16_t nb_events, > > + const event_tx_adapter_enqueue fn) { > > + const struct rte_eventdev *dev =3D &rte_eventdevs[dev_id]; >=20 > Access to *dev twice(see below rte_event_eth_tx_adapter_enqueue()) >=20 > > + struct rte_event_eth_tx_adapter *txa =3D > > + > > + rte_event_eth_tx_adapters.data[id]; >=20 > Just like common Tx adapter implementation, We can manage ethdev > queue to adapter mapping internally. So this deference is not required in > fastpath. >=20 > Please simply call the following, just like other eventdev ops. > fn(dev->data->ports[port_id], ev, nb_events) >=20 >=20 OK. > > + > > +#ifdef RTE_LIBRTE_EVENTDEV_DEBUG > > + if (id >=3D RTE_EVENT_ETH_TX_ADAPTER_MAX_INSTANCE || > > + dev_id >=3D RTE_EVENT_MAX_DEVS || > > + !rte_eventdevs[dev_id].attached) { > > + rte_errno =3D -EINVAL; > > + return 0; > > + } > > + > > + if (port_id >=3D dev->data->nb_ports) { > > + rte_errno =3D -EINVAL; > > + return 0; > > + } > > +#endif > > + return fn((void *)txa, dev, dev->data->ports[port_id], ev, > > +nb_events); } > > + > > +/** > > + * Enqueue a burst of events objects or an event object supplied in > > +*rte_event* > > + * structure on an event device designated by its *dev_id* through > > +the event > > + * port specified by *port_id*. This function is supported if the > > +eventdev PMD > > + * has the RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT > capability flag set. > > + * > > + * The *nb_events* parameter is the number of event objects to > > +enqueue which are > > + * supplied in the *ev* array of *rte_event* structure. > > + * > > + * The rte_event_eth_tx_adapter_enqueue() function returns the > number > > +of > > + * events objects it actually enqueued. A return value equal to > > +*nb_events* > > + * means that all event objects have been enqueued. > > + * > > + * @param id > > + * The identifier of the tx adapter. > > + * @param dev_id > > + * The identifier of the device. > > + * @param port_id > > + * The identifier of the event port. > > + * @param ev > > + * Points to an array of *nb_events* objects of type *rte_event* > > +structure > > + * which contain the event object enqueue operations to be processed. > > + * @param nb_events > > + * The number of event objects to enqueue, typically number of > > + * rte_event_port_enqueue_depth() available for this port. > > + * > > + * @return > > + * The number of event objects actually enqueued on the event device= . > The > > + * return value can be less than the value of the *nb_events* > parameter when > > + * the event devices queue is full or if invalid parameters are spec= ified > in a > > + * *rte_event*. If the return value is less than *nb_events*, the > remaining > > + * events at the end of ev[] are not consumed and the caller has to = take > care > > + * of them, and rte_errno is set accordingly. Possible errno values > include: > > + * - -EINVAL The port ID is invalid, device ID is invalid, an event= 's queue > > + * ID is invalid, or an event's sched type doesn't match = the > > + * capabilities of the destination queue. > > + * - -ENOSPC The event port was backpressured and unable to enqueue > > + * one or more events. This error code is only applicable= to > > + * closed systems. > > + */ > > +static inline uint16_t __rte_experimental > > +rte_event_eth_tx_adapter_enqueue(uint8_t id, uint8_t dev_id, > > + uint8_t port_id, > > + struct rte_event ev[], > > + uint16_t nb_events) { > > + const struct rte_eventdev *dev =3D &rte_eventdevs[dev_id]; > > + > > + return __rte_event_eth_tx_adapter_enqueue(id, dev_id, port_id, > ev, > > + nb_events, > > + dev->txa_enqueue); >=20 > As per above, Since the function call logic is simplified you can add the > above function logic here. >=20 OK, I will also delete the id parameter. > > +} > > + > > index dabb12d..ab23503 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -388,6 +388,11 @@ F: lib/librte_eventdev/*crypto_adapter* > > F: test/test/test_event_crypto_adapter.c > > F: doc/guides/prog_guide/event_crypto_adapter.rst > > > > +Eventdev Ethdev Tx Adapter API - EXPERIMENTAL > > +M: Nikhil Rao > > +T: git://dpdk.org/next/dpdk-next-eventdev > > +F: lib/librte_eventdev/*eth_tx_adapter* >=20 > Add the testcase also. >=20 I have made that update in patch 4 of this series. > Overall it looks good. No more comments on specification. >=20 Thanks for the review, Nikhil