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.
Hi Eli,
Here are some more AI review comments. They all seem legit. I added a
comment to some of them.
Cheers,
Eelco
commit 9d682f12c1 ("netdev-doca: Introduce doca netdev.")
Author: Eli Britstein <[email protected]>
This patch introduces a DOCA-based netdev implementation for
NVIDIA hardware offload, including ESW manager port handling,
per-port RX ring dispatch, sysfs attribute reading, and custom
statistics.
> if (rx_batch.count == 0) {
> return 0;
> }
>
> dispatch_rx_packets_by_port(&rx_batch, dev->esw_ctx->port_queues,
> rxq->queue_id);
> [...]
> pq = &dev->esw_ctx->port_queues[port_id][rxq->queue_id];
> batch->count =
> rte_ring_sc_dequeue_burst(pq->ring, (void **) batch->packets,
> NETDEV_MAX_BURST, NULL);
In netdev_doca_rxq_recv(), when the port is the ESW manager and
rte_eth_rx_burst returns zero packets, the function returns 0
(success) instead of falling through to the ring dequeue at
line 2605. Should this return EAGAIN instead of 0?
Returning 0 with an empty batch violates the rxq_recv contract
that the existing DPDK implementation follows: netdev_dpdk_rxq_recv
returns EAGAIN when no packets are available. The caller in
dp_netdev_process_rxq_port treats return 0 as "at least one
packet received" and increments batch counters for an empty batch.
More importantly, packets previously dispatched to this port's
own ring from an earlier burst are stranded until the next
successful HW burst, since the ring dequeue is skipped entirely.
---
> /* 'fgets' terminates the string with \n. Passing 'len', which
> * includes the \n, as the size to ovs_strlcpy() causes it to copy
> * len-1 characters, dropping the newline. */
> [...]
> len = strnlen(str, maxlen);
> [...]
> ovs_strlcpy(outp, str, len);
In get_sysfs_attr(), if the sysfs file content has no trailing
newline, does ovs_strlcpy(outp, str, len) truncate the last real
character? The comment explains the intent is to strip the
trailing newline, but strnlen returns the string length without
distinguishing whether a newline is present. For a value like
"p0" (no newline), len is 2, and ovs_strlcpy copies at most
len-1 = 1 byte, producing "p" instead of "p0".
This feeds into get_phys_port_name and then
netdev_doca_generate_devargs, where a truncated port name would
cause incorrect sscanf parsing. While sysfs attributes typically
end with a newline, the kernel API does not guarantee it.
EC> I guess just adding a simple newline strip, rather than
EC> unconditionally truncating the last character, would fix this.
---
> if (!netdev_doca_is_esw_mgr(&common->up)) {
> rte_eth_dev_configure(common->port_id, 0, 0, &conf);
> return doca_eth_dev_port_config_complete(dev, n_rxq, n_txq);
> }
>
> diag = rte_eth_dev_configure(common->port_id, n_rxq,
> n_txq, &conf);
> if (diag) {
> VLOG_WARN("Interface %s eth_dev setup error %s",
> common->up.name, rte_strerror(-diag));
> return diag;
> }
In doca_eth_dev_port_config(), the return value of
rte_eth_dev_configure is checked for ESW manager ports but
silently discarded for representor ports. If the call fails for
a representor, doca_eth_dev_port_config_complete proceeds to set
up MTU and allocate stats on a potentially misconfigured port.
Was the missing error check intentional?
EC> For representor ports, the return value of rte_eth_dev_configure
EC> is silently discarded. Should we return an error here, as the
EC> port might not have been set up correctly?
---
> atomic_add_relaxed(&pq[port_id][queue_id].n_bytes, pkt_size,
> &old_count);
> [...]
> atomic_add_relaxed(&pq->n_packets, batch->count, &old_count);
The byte count (n_bytes) is incremented at enqueue time in
dispatch_rx_packets_by_port, while the packet count (n_packets)
is incremented at dequeue time in netdev_doca_rxq_recv. Since
netdev_doca_get_custom_stats reads both atomics together, a
stats snapshot taken between enqueue and dequeue will show bytes
without corresponding packets.
Additionally, during cleanup in netdev_doca_esw_port_uninit:
> while (true) {
> deq = rte_ring_dequeue(*pring, (void **) &pkt);
> if (deq) {
> break;
> }
> dp_packet_delete(pkt);
> }
Packets dequeued here are deleted without incrementing n_packets,
so the final byte count will be permanently higher than what the
packet count reflects.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
