On 26 Jul 2026, at 17:33, Eli Britstein wrote:
> Introduce a new netdev type - "doca".
> The code is placed in new files.
> - ovs-doca: initialization of doca library and utility functions that
> are used currently by netdev-doca and also will be used for future
> hw-offload code.
> - netdev-doca: implementation of the new netdev.
>
> Supported ports are mlx5 ports in switch-dev mode only that with a NIC
> that supports hw-steering.
>
> The netdev has the concept of ESW manager. A representor port is
> functional only if its ESW manager is attached to OVS. In case it is
> not, the representor appears as functional in ovs-vsctl show, but it is
> not. Upon initializing of an ESW manager port, each representor is
> reconfigured to be functional, and upon destruction, they are first stopped.
>
> Steering infrastructure:
> - RX packets of all ports are steered to a common queue. This queue is
> polled using dpdk API and the packets are classified to a per-port
> memory structure.
> - TX packets are marked with the target port as metadata and sent to a
> common queue. The egress pipe matches on the metadata and forwards the
> packets accordingly.
>
> Signed-off-by: Eli Britstein <[email protected]>
Hi Eli,
Thanks for sending the v6 series. I have some comments below.
I would suggest waiting for feedback from David before sending a v7.
I have acked all of the previous patches in the series, but he may
have additional comments.
Cheers,
Eelco
[...]
> +static int
> +netdev_doca_slowpath_esw_init(struct netdev *netdev)
> +{
> + int rv;
> +
> +#define ESW_INIT_CMD(func) \
> + do { \
> + rv = (func)(netdev); \
> + if (rv == DOCA_SUCCESS) { \
> + break; \
> + } \
> + VLOG_ERR("%s: eSwitch initialization failed, %s() with error: " \
> + "%d (%s)", netdev_get_name(netdev), #func, rv, \
> + doca_error_get_descr(rv)); \
> + return rv; \
> + } while (0)
> +
> + ESW_INIT_CMD(netdev_doca_egress_pipe_init);
> + ESW_INIT_CMD(netdev_doca_rss_pipe_init);
> + ESW_INIT_CMD(netdev_doca_meta_tag0_pipe_init);
> + ESW_INIT_CMD(netdev_doca_meta_tag0_rule_init);
> + ESW_INIT_CMD(netdev_doca_pre_miss_pipe_init);
> + ESW_INIT_CMD(netdev_doca_pre_miss_rules_init);
> + ESW_INIT_CMD(netdev_doca_root_pipe_init);
If a failure occurs in a later call, resources initialized by earlier
successful calls may not be cleaned up. Should the macro call
netdev_doca_slowpath_esw_uninit() before returning on error?
netdev_doca_slowpath_esw_uninit() calls ovs_doca_destroy_pipe() for
each resource, which safely handles NULL pointers.
> + return 0;
> +}
[...]
> +static void
> +netdev_doca_dev_close(struct netdev_doca *dev)
> +{
> + struct netdev_dpdk_common *common = &dev->common;
> + struct netdev_doca_esw_ctx *esw = dev->esw_ctx;
> + struct rte_eth_dev_info dev_info;
> + char *pci_addr;
> + bool last;
> + int err;
> +
> + memset(&dev_info, 0, sizeof dev_info);
> +
> + if (rte_eth_dev_is_valid_port(common->port_id)) {
> + err = rte_eth_dev_info_get(common->port_id, &dev_info);
> + if (err) {
> + VLOG_ERR("Failed to get info of port "DPDK_PORT_ID_FMT": %s",
> + common->port_id, rte_strerror(-err));
> + }
> +
> + err = rte_eth_dev_close(common->port_id);
> + if (err) {
> + VLOG_ERR("Failed to close port "DPDK_PORT_ID_FMT": %s",
> + common->port_id, rte_strerror(-err));
> + }
> + }
> +
> + if (!esw) {
> + return;
> + }
> +
> + pci_addr = xstrdup(esw->pci_addr);
> +
> + last = refmap_unref(netdev_doca_esw_rfm, esw);
> + /* The last is the ESW. */
> + if (last && esw->dev) {
> + if (dev_info.device) {
> + /* The esw->cmd_fd is closed inside. */
> + err = rte_dev_remove(dev_info.device);
> + if (err) {
> + VLOG_ERR("Failed to remove device %s: %s", common->devargs,
> + rte_strerror(-err));
> + }
> +
> + esw->cmd_fd = -1;
> + }
> +
> + VLOG_DBG("Closing '%s'", pci_addr);
> + err = doca_dev_close(esw->dev);
> + if (err != DOCA_SUCCESS) {
> + VLOG_ERR("Failed to close doca dev %s. Error: %d (%s)", pci_addr,
> + err, doca_error_get_descr(err));
> + }
> +
> + esw->dev = NULL;
> + if (esw->cmd_fd != -1) {
> + close(esw->cmd_fd);
I guess this needs;
esw->cmd_fd = -1;
> + } else {
We do not need this else branch.
> + esw->cmd_fd = -1;
> + }
> + }
> +
> + dev->esw_ctx = NULL;
> + free(pci_addr);
> +}
[...]
> +static int
> +netdev_doca_reconfigure(struct netdev *netdev)
> +{
> + struct netdev_doca *dev = netdev_doca_cast(netdev);
> + struct netdev_dpdk_common *common = &dev->common;
> + int err = 0;
> +
> + /* If an ESW manager is not attached to OVS, a representor cannot be
> + * configured. */
> + if (!netdev_doca_is_esw_mgr(netdev) &&
> + netdev_doca_get_esw_mgr_port_id(netdev) ==
> + DPDK_ETH_PORT_ID_INVALID) {
> + return EOPNOTSUPP;
> + }
> +
> + ovs_mutex_lock(&dpdk_common_mutex);
> + ovs_mutex_lock(&dev->common.mutex);
> +
> + common->requested_n_rxq = common->user_n_rxq;
> +
> + if (netdev->n_txq == common->requested_n_txq
> + && netdev->n_rxq == common->requested_n_rxq
> + && common->mtu == common->requested_mtu
> + && common->lsc_interrupt_mode == common->requested_lsc_interrupt_mode
> + && common->rxq_size == common->requested_rxq_size
> + && common->txq_size == common->requested_txq_size
> + && eth_addr_equals(common->hwaddr, common->requested_hwaddr)
> + && common->socket_id == common->requested_socket_id
> + && netdev_dpdk_is_started(common)) {
> + /* Reconfiguration is unnecessary. */
> + goto out;
> + }
> +
> + netdev_doca_port_stop(netdev);
> +
> + err = netdev_doca_mempool_configure(dev);
> + if (err) {
> + goto out;
> + }
> +
> + common->lsc_interrupt_mode = common->requested_lsc_interrupt_mode;
> +
> + netdev->n_txq = common->requested_n_txq;
> + netdev->n_rxq = common->requested_n_rxq;
> + if (!netdev_doca_is_esw_mgr(netdev)) {
> + int esw_n_rxq;
> +
> + esw_n_rxq = dev->esw_ctx->n_rxq;
> + if (common->requested_n_rxq != esw_n_rxq) {
> + VLOG_WARN("%s: requested_n_rxq=%d is ignored. DOCA binds the "
> + "number of rx queues to the esw's n_rxq=%d",
> + netdev_get_name(netdev), common->requested_n_rxq,
> + esw_n_rxq);
> + }
> +
> + netdev->n_rxq = esw_n_rxq;
> + }
> +
> + common->rxq_size = common->requested_rxq_size;
> + common->txq_size = common->requested_txq_size;
> +
> + rte_free(common->tx_q);
> + common->tx_q = NULL;
> +
> + if (!eth_addr_equals(common->hwaddr, common->requested_hwaddr)) {
> + err = netdev_dpdk_set_dev_etheraddr(&dev->common,
> + common->requested_hwaddr);
> + if (err) {
> + goto out;
> + }
> + }
> +
> + err = doca_eth_dev_init(dev);
> + if (err) {
> + goto out;
> + }
> +
> + netdev_dpdk_update_netdev_flags(&dev->common);
> +
> + /* If both requested and actual hw-addr were previously
> + * unset (initialized to 0), then first device init above
> + * will have set actual hw-addr to something new.
> + * This would trigger spurious MAC reconfiguration unless
> + * the requested MAC is kept in sync.
> + *
> + * This is harmless in case requested_hwaddr was
> + * configured by the user, as netdev_dpdk_set_dev_etheraddr()
> + * will have succeeded to get to this point. */
> + common->requested_hwaddr = common->hwaddr;
> +
> + common->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
> + if (!common->tx_q) {
> + err = ENOMEM;
If the allocation fails, the port is left started with a NULL tx_q.
On the next reconfigure call, the "no change needed" guard sees
started=true and skips the allocation, leaving tx_q permanently NULL
and risking a crash in netdev_doca_send() when concurrent_txq is true.
> + }
> +
> + netdev_change_seq_changed(netdev);
> +
> +out:
> + ovs_mutex_unlock(&dev->common.mutex);
> + ovs_mutex_unlock(&dpdk_common_mutex);
> + return err;
> +}
[...]
> +static enum doca_log_level
> +get_buf_log_level(const char **pbuf, size_t *psize)
> +{
> + const char *buf = *pbuf;
> + size_t size = *psize;
> + const char *p = buf;
> + int level;
> +
> + /* Skip [timestamp][thread_id][DOCA], then parse [LEVEL] (INF/WRN/etc.).
> */
> + for (int i = 0; i < 4; i++) {
> + while (size && *p && *p != '[') {
> + size--;
> + p++;
> + }
> +
> + if (!size || !*p) {
> + return DOCA_LOG_LEVEL_DISABLE;
> + }
> +
> + size--;
> + p++;
> + }
> +
Does this code need a "size >= 4" guard to safeguard the *psize
arithmetic below against underflow? Placing it here also ensures
that at least 3 bytes remain for the strncmp in
ovs_doca_parse_log_level().
if (size < 4) {
return DOCA_LOG_LEVEL_DISABLE;
}
> + level = ovs_doca_parse_log_level(NULL, p);
> + if (level < 0) {
> + return DOCA_LOG_LEVEL_DISABLE;
> + }
> +
> + /* 'p' points to the level start which is 3 chars and another ']'
> + * after it. For example "INF]". Skip it. */
> + *pbuf = p + 4;
> + *psize -= *pbuf - buf;
> +
> + return level;
> +}
> +
> +static ssize_t
> +ovs_doca_log_write(void *c OVS_UNUSED, const char *buf, size_t size)
> +{
> + static struct vlog_rate_limit dbg_rl = VLOG_RATE_LIMIT_INIT(600, 600);
> + enum doca_log_level level = get_buf_log_level(&buf, &size);
> +
> + switch (level) {
> + case DOCA_LOG_LEVEL_DISABLE:
> + VLOG_ERR("(Failed to parse level): %.*s", (int) size, buf);
> + break;
The coding style guide says 'case' labels should align with the
'switch' keyword, not be indented an extra level inside it.
> + case DOCA_LOG_LEVEL_TRACE:
> + case DOCA_LOG_LEVEL_DEBUG:
> + VLOG_DBG_RL(&dbg_rl, "%.*s", (int) size, buf);
> + break;
> + case DOCA_LOG_LEVEL_INFO:
> + VLOG_INFO_RL(&dbg_rl, "%.*s", (int) size, buf);
> + break;
> + case DOCA_LOG_LEVEL_WARNING:
> + VLOG_WARN_RL(&dbg_rl, "%.*s", (int) size, buf);
> + break;
> + case DOCA_LOG_LEVEL_ERROR:
> + VLOG_ERR_RL(&dbg_rl, "%.*s", (int) size, buf);
> + break;
> + case DOCA_LOG_LEVEL_CRIT:
> + VLOG_EMER("%.*s", (int) size, buf);
> + break;
> + }
> +
> + return size;
> +}
[...]
> +/* Every doca-entry operation is asynchronous. It must be processed using
> + * doca_flow_entries_process() API. For each processed entry, this callback
> + * is called. The 'qid' argument is the queue-id for which the entry was
> + * processed on (which is the same as the one of the initial operation).
> + * 'queues' is an array of queues. Each entry is accessed only the its own
> + * queue (which is assigned to a thread), no locks are required here. */
The last sentence here needs some rewording.
> +static void
> +ovs_doca_entry_process(struct doca_flow_pipe_entry *entry,
> + uint16_t qid,
> + enum doca_flow_entry_status status,
> + enum doca_flow_entry_op op,
> + void *aux)
> +{
> + static const char *status_desc[] = {
> + [DOCA_FLOW_ENTRY_STATUS_IN_PROCESS] = "in-process",
> + [DOCA_FLOW_ENTRY_STATUS_SUCCESS] = "success",
> + [DOCA_FLOW_ENTRY_STATUS_ERROR] = "failure",
> + };
> + static const char *op_desc[] = {
> + [DOCA_FLOW_ENTRY_OP_ADD] = "add",
> + [DOCA_FLOW_ENTRY_OP_DEL] = "del",
> + [DOCA_FLOW_ENTRY_OP_UPD] = "mod",
> + [DOCA_FLOW_ENTRY_OP_AGED] = "age",
> + };
> + bool error = status == DOCA_FLOW_ENTRY_STATUS_ERROR;
> + struct ovs_doca_steering_queue *queues = aux;
> +
> + ovs_assert(status < ARRAY_SIZE(status_desc));
> + ovs_assert(op < ARRAY_SIZE(op_desc));
> +
> + VLOG_RL(&rl, error ? VLL_ERR : VLL_DBG,
> + "%s: [qid:%" PRIu16 "] %s aux=%p entry %p %s",
> + __func__, qid, op_desc[op], aux, entry, status_desc[status]);
> +
> + if (queues && status != DOCA_FLOW_ENTRY_STATUS_IN_PROCESS) {
> + queues[qid].n_waiting_entries--;
> + }
> +}
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
