[dpdk-dev] [PATCH 3/4] eventdev: Add eventdev ethernet Rx adapter

Rao, Nikhil nikhil.rao at intel.com
Tue Sep 19 17:25:22 CEST 2017


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


More information about the dev mailing list