On 7 Jan 2026, at 20:29, Aaron Conole wrote:
> Eelco Chaudron via dev <[email protected]> writes:
>
>> This patch adds support for configuring the priority of
>> offload providers. When multiple providers exist and
>> support the same port, the 'hw-offload-priority' option
>> allows specifying the order in which they are tried.
>>
>> Signed-off-by: Eelco Chaudron <[email protected]>
>> ---
[...]
>> static int
>> dpif_offload_attach_providers_(struct dpif *dpif)
>> OVS_REQUIRES(dpif_offload_mutex)
>> {
>> + struct ovs_list provider_list = OVS_LIST_INITIALIZER(&provider_list);
>> const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif));
>> struct dp_offload *dp_offload;
>> + struct dpif_offload *offload;
>> struct shash_node *node;
>> + char *tokens, *saveptr;
>>
>> /* Allocate and attach dp_offload to dpif. */
>> dp_offload = xmalloc(sizeof *dp_offload);
>> @@ -206,37 +241,51 @@ dpif_offload_attach_providers_(struct dpif *dpif)
>> ovs_list_init(&dp_offload->offload_providers);
>> shash_add(&dpif_offload_providers, dp_offload->dpif_name, dp_offload);
>>
>> - /* Attach all the providers supporting this dpif type. */
>> + /* Open all the providers supporting this dpif type. */
>> SHASH_FOR_EACH (node, &dpif_offload_classes) {
>> const struct dpif_offload_class *class = node->data;
>>
>> for (size_t i = 0; class->supported_dpif_types[i] != NULL; i++) {
>> if (!strcmp(class->supported_dpif_types[i], dpif_type_str)) {
>> - struct dpif_offload *offload;
>> - int error;
>> -
>> - error = class->open(class, dpif, &offload);
>> - if (!error) {
>> - error = dpif_offload_attach_provider_to_dp_offload(
>> - dp_offload, offload);
>> - if (error) {
>> - VLOG_WARN("failed to add dpif offload provider "
>> - "%s to %s: %s",
>> - class->type, dpif_name(dpif),
>> - ovs_strerror(error));
>> - class->close(offload);
>> - }
>> - } else {
>> + int error = class->open(class, dpif, &offload);
>> +
>> + if (error) {
>> VLOG_WARN("failed to initialize dpif offload provider "
>> "%s for %s: %s",
>> class->type, dpif_name(dpif),
>> ovs_strerror(error));
>> + } else {
>> + ovs_list_push_back(&provider_list,
>> + &offload->dpif_list_node);
>> }
>> break;
>
> Weird - I must be misunderstanding, but I would expect that this
> wouldn't actually properly seek through the list, because the first
> match would land on this break. Did I miss something?
Let me put the actual code here, as the diff is hard to read :)
/* Open all the providers supporting this dpif type. */
SHASH_FOR_EACH (node, &dpif_offload_classes) {
const struct dpif_offload_class *class = node->data;
for (size_t i = 0; class->supported_dpif_types[i] != NULL; i++) {
if (!strcmp(class->supported_dpif_types[i], dpif_type_str)) {
int error = class->open(class, dpif, &offload);
if (error) {
VLOG_WARN("failed to initialize dpif offload provider "
"%s for %s: %s",
class->type, dpif_name(dpif),
ovs_strerror(error));
} else {
ovs_list_push_back(&provider_list,
&offload->dpif_list_node);
}
break;
}
}
}
An offload provider can support multiple dpif types. Here we are looking for a
specific dpif type by name, so once we find a match with strcmp(), we only
need to process that one and can safely ignore the rest, hence the break.
Hope this clears it up.
>>>>> }
[...]
>>>>> +void
>> +dpif_offload_set_global_cfg(const struct smap *other_cfg)
>> +{
>> + static struct ovsthread_once init_once = OVSTHREAD_ONCE_INITIALIZER;
>> + const char *priority = smap_get(other_cfg, "hw-offload-priority");
>> +
>> + /* The 'hw-offload-priority' parameter can only be set at startup,
>> + * any successive change needs a restart. */
>> + if (ovsthread_once_start(&init_once)) {
>> + /* Initialize the dpif-offload layer in case it's not yet
>> initialized
>> + * at the first invocation of setting the configuration. */
>> + dp_offload_initialize();
>> +
>> + /* If priority is not set keep the default value. */
>> + if (priority) {
>> + char *tokens = xstrdup(priority);
>> + char *saveptr;
>> +
>> + free(dpif_offload_provider_priority_list);
>> + dpif_offload_provider_priority_list = xstrdup(priority);
>> +
>> + /* Log a warning for unknown offload providers. */
>> + for (char *name = strtok_r(tokens, ",", &saveptr);
>> + name;
>> + name = strtok_r(NULL, ",", &saveptr)) {
>> +
>> + if (!shash_find(&dpif_offload_classes, name)) {
>> + VLOG_WARN("'hw-offload-priority' configuration has an "
>> + "unknown type; %s", name);
>
> If a weird config gets into this, maybe it's better to also remove it
> from the in-memory list. WDYT?
Yes, we could. It would slightly speed up the creation of new dpifs; however,
it would require us to keep two copies (the original and a modified one),
otherwise the VLOG below would be triggered each time.
My preference is to keep this as-is and not store the additional configuration
string. Let me know your thoughts.
>> + }
>> + }
>> + free(tokens);
>> + }
>> + ovsthread_once_done(&init_once);
>> + } else {
>> + if (priority && strcmp(priority,
>> + dpif_offload_provider_priority_list)) {
>> + VLOG_INFO_ONCE("'hw-offload-priority' configuration changed; "
>> + "restart required");
>> + }
>> + }
>> +}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
