On 9/18/2017 9:06 PM, Van Haaren, Harry wrote:
From: Rao, Nikhil
+++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
@@ -0,0 +1,1239 @@
+#include <rte_cycles.h>
+#include <rte_common.h>
+#include <rte_dev.h>
+#include <rte_errno.h>
+#include <rte_ethdev.h>
+#include <rte_log.h>
+#include <rte_malloc.h>
+#include <rte_service_component.h>
+#include <rte_thash.h>
+
+#include "rte_eventdev.h"
+#include "rte_eventdev_pmd.h"
+#include "rte_event_eth_rx_adapter.h"
+
+#define BATCH_SIZE 32
+#define BLOCK_CNT_THRESHOLD 10
+#define ETH_EVENT_BUFFER_SIZE (4*BATCH_SIZE)
+
+static const char adapter_mem_name[] = "rx_adapter_mem_";
This bit isn't really DPDK style - usually we allocate a specific size for a
buffer,
and then print into it using a safe method such as snprintf()
<snip>
+struct rte_event_eth_rx_adapter {
+ /* event device identifier */
+ uint8_t eventdev_id;
+ /* per ethernet device structure */
+ struct eth_device_info *eth_devices;
+ /* malloc name */
+ char mem_name[sizeof(adapter_mem_name) + 4];
See above comment, and why the magic + 4?
If we had a statically sized buffer, then this wouldn't be an issue.
There are multiple other instances of adapter_mem_name being accessed,
they have this strange "+ 4" throughout. Please refactor.
Assuming this code is all initialization and configuration only, lets
not save a few bytes at the expense of a few bugs :D
OK.
+
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+#define BE_16(x) (uint16_t)((x) >> 8 | (x) << 8)
+#else
+#define BE_16(x) (x)
+#endif
+
+#define NETWORK_ORDER(x) BE_16(x)
There is a rte_byteorder.h header file, which handles details such as the above.
I don't have much experience with it - but it looks like there's some
duplication here.
Agreed, looks redundant. These are compile time conversions, and I see
that rte_byteorder does handle that case.
+static int
+_rte_event_eth_rx_adapter_queue_del(struct rte_event_eth_rx_adapter
*rx_adapter,
+ struct eth_device_info *dev_info,
+ uint16_t rx_queue_id)
+{
Why the _ prefix before the function? This is already a static function, so
it won't be available outside this translation unit. If it is not meant to be
externally visible, don't use the "rte" prefix and voila, you have an internal
visibility function..? Perhaps I missed something.
A few more _functions() like this below.
No particular reason for the _ prefix, as I was refactoring common code,
it looks like I may have cut & pasted the queue_add/del function names.
Will implement your suggestion.
+static int
+_rx_adapter_ctrl(struct rte_event_eth_rx_adapter *rx_adapter, int start)
+{
This function is only called from one place?
It can probably be placed in that function itself, given it is the non
_prefixed version?
The idea here was to separate out the error checking from the core
logic. I will put it back.
+int
+rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
+ rx_adapter_conf_cb conf_cb, void *conf_arg)
+{
+ struct rte_event_eth_rx_adapter *rx_adapter;
+ int ret;
+ int socket_id;
+ uint8_t i;
+ char mem_name[sizeof(adapter_mem_name) + 4];
Same as before.
+ RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
+ RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+ if (!conf_cb)
+ return -EINVAL;
+
+ if (rte_event_eth_rx_adapter == NULL) {
+ ret = rte_event_eth_rx_adapter_init();
+ if (ret)
+ return ret;
+ }
+
+ rx_adapter = id_to_rx_adapter(id);
+ if (rx_adapter != NULL) {
+ RTE_EDEV_LOG_ERR("Eth Rx adapter exists id = %" PRIu8, id);
+ return -EEXIST;
+ }
+
+ socket_id = rte_event_dev_socket_id(dev_id);
+ snprintf(mem_name, sizeof(adapter_mem_name) + 4, "%s%d",
+ adapter_mem_name, id);
+
+ rx_adapter = rte_zmalloc_socket(mem_name, sizeof(*rx_adapter),
+ RTE_CACHE_LINE_SIZE, socket_id);
+ if (rx_adapter == NULL) {
+ RTE_EDEV_LOG_ERR("failed to get mem for rx adapter");
+ return -ENOMEM;
+ }
+
+ rx_adapter->eventdev_id = dev_id;
+ rx_adapter->socket_id = socket_id;
+ rx_adapter->conf_cb = conf_cb;
+ rx_adapter->conf_arg = conf_arg;
+ strcpy(rx_adapter->mem_name, mem_name);
+ rx_adapter->eth_devices = rte_zmalloc_socket(rx_adapter->mem_name,
+ rte_eth_dev_count() *
+ sizeof(struct eth_device_info), 0,
+ socket_id);
+ if (rx_adapter->eth_devices == NULL) {
+ RTE_EDEV_LOG_ERR("failed to get mem for eth devices\n");
+ rte_free(rx_adapter);
+ return -ENOMEM;
+ }
This (and the previous other -ERROR returns) don't free the resources
that have been taken by this function up to this point. Improved tidying
up a after an error would be good.
The rte_free() for rx_adapter is the only cleanup needed, unless you are
talking about the cleanup for rte_event_eth_rx_adapter_init() function.
There's also some checkpatch warnings on the patch page:
http://dpdk.org/ml/archives/test-report/2017-July/024634.html
These warnings are for the previous version.
In general - looks like really good work - these are just improvements, nothing
major discovered.
Thanks for the review.
Nikhil
