On 28/07/2026 13:28, Eelco Chaudron wrote:
External email: Use caution opening links or attachments
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.
Ack
+ 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.
Good catch. Ack.
+ 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.
right. will fix. btw, the same in netdev-dpdk.c.
+ }
+
+ 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;
}
Ack
+ 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.
Ack. btw, same in dpdk.c
+ 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.
Ack
+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
