[dpdk-dev] [PATCH v8 03/19] ethdev: enable hotplug on multi-process

Thomas Monjalon thomas at monjalon.net
Tue Jul 3 11:44:56 CEST 2018


Hi,

02/07/2018 07:44, Qi Zhang:
> --- /dev/null
> +++ b/lib/librte_ethdev/ethdev_mp.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#ifndef _RTE_ETHDEV_MP_H_
> +#define _RTE_ETHDEV_MP_H_
> +
> +#define MAX_DEV_ARGS_LEN 0x80
> +
> +#define ETH_DEV_MP_ACTION_REQUEST	"eth_dev_mp_request"
> +#define ETH_DEV_MP_ACTION_RESPONSE	"eth_dev_mp_response"
> +
> +enum eth_dev_req_type {
> +	REQ_TYPE_ATTACH,
> +	REQ_TYPE_PRE_DETACH,
> +	REQ_TYPE_DETACH,
> +	REQ_TYPE_ATTACH_ROLLBACK,
> +};

These constants are missing an ethdev prefix.

> +
> +struct eth_dev_mp_req {
> +	enum eth_dev_req_type t;
> +	char devargs[MAX_DEV_ARGS_LEN];
> +	uint16_t port_id;
> +	int result;
> +};
> +
> +/**
> + * this is a synchronous wrapper for secondary process send
> + * request to primary process, this is invoked when an attach
> + * or detach request issued from primary.
> + */
> +int eth_dev_request_to_primary(struct eth_dev_mp_req *req);
> +
> +/**
> + * this is a synchronous wrapper for primary process send
> + * request to secondary process, this is invoked when an attach
> + * or detach request issued from secondary process.
> + */
> +int eth_dev_request_to_secondary(struct eth_dev_mp_req *req);


Why do we need ethdev functions for IPC (mp request/response)?
How this model can reasonnably scale to other device classes
(crypto, compression, bbdev, eventdev, etc)?


> --- /dev/null
> +++ b/lib/librte_ethdev/ethdev_private.h

What is the purpose of a file ethdev_private.h?

> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation

Are you sure about the years?

> +/**
> + * Attach a new Ethernet device in current process.
> + *
> + * @param devargs
> + *  A pointer to a strings array describing the new device
> + *  to be attached. The strings should be a pci address like
> + *  '0000:01:00.0' or virtual device name like 'net_pcap0'.

No, no. The devargs syntax is being changed, so you should not duplicate
its description here. Better to reference an unique source of doc.

> + *
> + * @param port_id
> + *  A pointer to a port identifier actually attached.
> + *
> + * @return
> + *  0 on success and port_id is filled, negative on error
> + */
> +int do_eth_dev_attach(const char *devargs, uint16_t *port_id);

So you are duplicating rte_eth_dev_attach which is flawed in its design
and should be deprecated...

As you may have noticed, rte_eth_dev_attach() is calling
rte_eal_hotplug_add() which manages the EAL device.
It is wrong because the relation between an ethdev port and
an EAL device is not a 1:1 mapping.
We must manage the ethdev port as one of the possible abstractions
of a device represented by rte_device.




More information about the dev mailing list