On 26 Jul 2026, at 16:11, Eli Britstein wrote:
> On 23/07/2026 16:11, Eelco Chaudron wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 1 Jul 2026, at 15:58, 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]>
>>> ---
>> Thanks for the new revision Eli! Some minor comments below.
>>
>> //Eelco
>>
>>> Documentation/automake.mk | 2 +
>>> Documentation/howto/doca.rst | 144 ++
>>> Documentation/howto/index.rst | 1 +
> ...
>>> +
>>> + fwd.type = DOCA_FLOW_FWD_RSS;
>>> + fwd.rss_type = DOCA_FLOW_RESOURCE_TYPE_NON_SHARED;
>>> + memset(&fwd.rss.nr_queues, 0xFF, sizeof fwd.rss.nr_queues);
>> I am not very familiar with the DOCA API, but looking at
>> doca_flow.h, struct doca_flow_resource_rss_cfg has both
>> outer_flags and nr_queues. The template marks nr_queues
>> as changeable (0xFF) but leaves outer_flags at 0. In
>> netdev_doca_rss_entries_init(), each of the 11 RSS entries
>> sets a different outer_flags value via
>> netdev_doca_rss_flags(). Does DOCA honour per-entry
>> outer_flags when the template has it at zero, or should
>> it also be set to 0xFF here?
>
> This is doca-flow API.
>
> https://networking-docs.nvidia.com/doca/archive/3-4-0/doca-flow#Setting-Pipe-Forwarding
Thanks for pointing me to the docs. Still not clear to me
whether it needs the mask or not, but I guess you are the
expert ;)
The rest of the comment replies look good. I will try to
skim the v6 and do a full review after my PTO.
//Eelco
>>> + rv = ovs_doca_pipe_create(&dev->common.up, &match, NULL, &monitor,
>>> + &actions, &actions, NULL, &fwd, NULL,
>>> + NETDEV_DOCA_RSS_NUM_ENTRIES *
>>> RTE_MAX_ETHPORTS,
>>> + false, false, UINT64_C(1) << AUX_QUEUE,
>>> "RSS",
> ...
>>> +
>>> + /* A device may report more queues than it makes available (this has
>>> + * been observed for Intel xl710, which reserves some of them for
>> Does this comment make sense in the context of netdev-doca?
>> Maybe consider updating it.
>>
>>> + * SRIOV): rte_eth_*_queue_setup will fail if a queue is not
>>> + * available. When this happens we can retry the configuration
>>> + * and request less queues. */
>>> + while (n_rxq && n_txq) {
>>> + if (diag) {
>>> + VLOG_INFO("Retrying setup with (rxq:%d txq:%d)", n_rxq, n_txq);
> Removed this comment, and also the retry loop.
>>> + }
> ...
>>> + 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);
>> Should we also rate limit these?
> There is no "VLOG_EMER_RL". I think it should abort, but anyway, this follows
> dpdk.c.
>>
>>> + break;
>>> + }
>>> +
>>> + return size;
>>> +}
>>> +
>>> +static cookie_io_functions_t ovs_doca_log_func = {
> ...
>>> + queues[qid].n_waiting_entries--;
>>> + }
>> If an async entry ADD fails at the HW level, the only
>> indication is a VLL_ERR log from the callback. The callers
>> have no way to detect this failure; ovs_doca_complete_queue_esw()
>> returns DOCA_SUCCESS regardless. This means the steering
>> pipeline could be partially configured with no error propagated
>> to the caller. Should we check doca_flow_pipe_entry_get_status()
>> after completion, or have the callback set a failure flag?
>> This will also be needed when installing actual offloaded flows,
>> where the caller must know whether the flow was committed to HW.
> Nice catch. Added.
>>
>>> +}
>>> +
>>> +static int
>>> +ovs_doca_init__(const struct smap *ovs_other_config)
>>> +{
>>> + struct doca_flow_definitions_cfg *defs_cfg = NULL;
>>> + struct doca_flow_definitions *defs = NULL;
>>> + struct doca_flow_cfg *cfg;
>>> + doca_error_t err;
>>> +
>>> + if (!dpdk_available()) {
>>> + VLOG_ERR("DOCA requires DPDK. Set other_config:dpdk-init=true.");
>>> + return ENODEV;
>>> + }
>>> +
>>> + if (rte_flow_dynf_metadata_register() < 0) {
>>> + VLOG_ERR("Failed to register dynamic metadata, Error: %d (%s)",
>>> + rte_errno, rte_strerror(rte_errno));
>>> + return ENOTSUP;
>>> + }
>>> +
>>> + log_stream = fopencookie(NULL, "w+", ovs_doca_log_func);
>>> + if (!log_stream) {
>>> + VLOG_ERR("Can't redirect DOCA log: %s.", ovs_strerror(errno));
>>> + } else {
>>> + /* Create a logger back-end that prints to the redirected log. */
>>> + doca_log_backend_create_with_file_sdk(log_stream, &ovs_doca_log);
>>> + doca_log_level_set_global_sdk_limit(DOCA_LOG_LEVEL_WARNING);
>>> + }
>>> +
>>> + unixctl_command_register("doca/log-set", "{level}. "
>>> + "level=crit/err/warn/info/dbg/trc", 0, 1,
>>> + ovs_doca_unixctl_log_set, NULL);
>>> + unixctl_command_register("doca/log-get", "", 0, 0,
>>> + unixctl_mem_stream, ovs_doca_log_get);
>>> +
>>> + /* DOCA configuration happens earlier than dpif-netdev's.
>>> + * To avoid reorganizing them, read the relevant item directly. */
>>> + ovs_doca_max_megaflows_counters =
>>> + smap_get_uint(ovs_other_config, "flow-limit",
>>> + OFPROTO_FLOW_LIMIT_DEFAULT);
>>> +
>>> +#define RV_TEST(call) \
>>> + do { \
>>> + err = (call); \
>>> + if (err != DOCA_SUCCESS) { \
>>> + VLOG_ERR("DOCA initialization failed, %s(). Error: %d (%s)", \
>>> + #call, err, doca_error_get_descr(err)); \
>>> + return ENODEV; \
>>> + } \
>>> + } while (0)
>>> +
>>> + RV_TEST(doca_flow_cfg_create(&cfg));
>>> + RV_TEST(doca_flow_cfg_set_pipe_queues(cfg,
>>> OVS_DOCA_MAX_STEERING_QUEUES));
>>> + RV_TEST(doca_flow_cfg_set_resource_mode(cfg,
>>> + DOCA_FLOW_RESOURCE_MODE_PORT));
>>> + RV_TEST(doca_flow_cfg_set_mode_args(cfg,
>>> "switch,hws,isolated,expert"));
>>> + RV_TEST(doca_flow_cfg_set_queue_depth(cfg, OVS_DOCA_QUEUE_DEPTH));
>>> + RV_TEST(doca_flow_cfg_set_cb_entry_process(cfg,
>>> ovs_doca_entry_process));
>>> + RV_TEST(ovs_doca_init_defs(cfg, &defs, &defs_cfg));
>>> + RV_TEST(doca_flow_init(cfg));
>>> + ovs_doca_destroy_defs(defs, defs_cfg);
>>> + RV_TEST(doca_flow_cfg_destroy(cfg));
>>> +
>> The RV_TEST macro returns immediately on failure, so if
>> doca_flow_cfg_create() succeeds but any subsequent setter
>> fails, cfg is never destroyed. Likewise if doca_flow_init()
>> fails after ovs_doca_init_defs(), both defs and defs_cfg are
>> leaked.
> Ack
>>
>>> +#undef RV_TEST
>>> +
>>> + netdev_doca_register();
>>> + return 0;
> ...
>>> + room = OVS_DOCA_QUEUE_DEPTH - n_waiting;
>>> + if (n_processed == 0 && retries-- <= 0) {
>>> + COVERAGE_INC(ovs_doca_queue_block);
>>> + break;
>>> + }
>> When 100 consecutive iterations produce zero progress the
>> loop breaks, but err is still DOCA_SUCCESS. n_waiting_entries
>> may be nonzero. The caller receives success but the entry
>> may not be committed to HW. Should set an error before
>> breaking:
>>
>> if (n_processed == 0 && retries-- <= 0) {
>> COVERAGE_INC(ovs_doca_queue_block);
>> err = DOCA_ERROR_AGAIN;
>> break;
>> }
>>
>> Or should we just document we will ignore the error, and
>> a successive call will retry?
> Addressed as part of the previous comment about
> doca_flow_pipe_entry_get_status().
>>
>>> +
>>> + if (timeout_ms && time_msec() > timeout_ms) {
>> timeout_ms is always nonzero (set to time_msec() + 1000
>> earlier), so "if (timeout_ms && ...)" is always true,
>> and can be removed.
>>
>>> + err = DOCA_ERROR_TIME_OUT;
>>> + VLOG_EMER("Timeout reached trying to complete queue %u: "
>>> + "%u remaining entries", qid, n_waiting);
>> Should we rate-limit this in case the queue is persistently
>> timing out? This is called from the flow add/remove path,
>> so a stuck queue could flood the log.
>>
>> VLOG_EMER_RL(%rl, "...");
> There is no VLOG_EMER_RL. Changed to VLOG_ERR_RL.
>>
>>> + }
>>> + } while (err == DOCA_SUCCESS && room < OVS_DOCA_QUEUE_DEPTH);
>>> +
>>> + return err;
>>> +}
> ...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
