On 8 Oct 2026, at 15:43, Eli Britstein via dev wrote:

> On 06/10/2026 17:30, Eelco Chaudron wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Exercise the alt_actions parameter added to the netdev_hw_post_process()
>> callback.  When a flow contains only OUTPUT actions with a mix of
>> dummy-pmd and non-dummy-pmd egress ports, the dummy driver executes the
>> dummy-pmd outputs in the simulated hardware pipeline and returns the
>> non-dummy-pmd outputs as alternative actions for the PMD thread.
>> A new test case validates the new API.

Thanks for the review Eli!

//Eelco

[...]

>> @@ -589,10 +596,10 @@ dummy_offload_hw_post_process(const struct 
>> dpif_offload *offload_,
>>       void *flow_reference = NULL;
>>       uint32_t flow_mark;
>>
>> +    *alt_actions = NULL;
> move this part to the previous commit

Will do.

>>       port = dummy_offload_get_port_by_netdev(offload_, netdev);
>>       if (!port || !dp_packet_has_flow_mark(packet, &flow_mark)) {
>>           *flow_reference_ = NULL;
>> -        *alt_actions = NULL;
>>           return 0;
>>       }
>>
>> @@ -605,13 +612,15 @@ dummy_offload_hw_post_process(const struct 
>> dpif_offload *offload_,
>>               if (pmd_data) {
>>                   flow_reference = pmd_data->flow_reference;
>>               }
>> +            if (off_flow->alt_actions) {
>> +                *alt_actions = off_flow->alt_actions;
>> +            }
> missing blank lines after }

Added.

>>               break;
>>           }
>>       }
>>       ovs_mutex_unlock(&port->port_mutex);
>>
>>       *flow_reference_ = flow_reference;
>> -    *alt_actions = NULL;
>>       return 0;
>>   }
>>
>> @@ -724,22 +733,23 @@ dummy_offload_are_all_actions_supported(const struct 
>> dpif_offload *offload_,
>>       return true;
>>   }
>>
>> -static bool
>> +static int
>>   dummy_offload_hw_process_pkt(const struct dpif_offload *offload_,
>>                                struct dummy_offloaded_flow *flow,
>>                                struct dp_packet *pkt)
>>   {
>> +    bool partial_act = flow->alt_actions != NULL;
>>       uint32_t hash = dp_packet_get_rss_hash(pkt);
>>       uint32_t pkt_size = dp_packet_size(pkt);
>>       const struct nlattr *nla;
>>       size_t left;
>>
>>       if (!flow->actions) {
>> -        return false;
>> +        return -ENOENT;
> Shouldn't return values be positive errno?

Good catch! You are right OVS style guide wants positive error numbers.
I was confused with DPDK.

[...]

>> +/* Build a set of alternative actions to exercise the alt_actions support in
>> + * the netdev_hw_post_process() API.  The dummy implementation returns
>> + * alternative actions only when the action list consists solely of
>> + * OVS_ACTION_ATTR_OUTPUT actions, and at least one egress port is not of
>> + * type dummy-pmd.  In that case, the dummy-pmd outputs are handled in
>> + * 'hardware', while the non-dummy-pmd outputs are returned as alternative
>> + * actions to be executed by the PMD thread. */
>> +static struct offload_actions *
>> +dummy_build_alt_actions(const struct dpif_offload *offload_,
>> +                        odp_port_t in_odp,
>> +                        const struct nlattr *actions,
>> +                        size_t actions_len)
>> +{
>> +    struct offload_actions *alt_actions = NULL;
>> +    const struct nlattr *nla;
>> +    struct ofpbuf buf;
>> +    size_t left;
>> +
>> +    ofpbuf_init(&buf, 0);
>> +
>> +    NL_ATTR_FOR_EACH (nla, left, actions, actions_len) {
>> +        enum ovs_action_attr action = nl_attr_type(nla);
>> +        struct dummy_offload_port *out_port;
>> +        odp_port_t out_odp;
>> +
>> +        if (action != OVS_ACTION_ATTR_OUTPUT) {
>> +            goto out;
>> +        }
>> +
>> +        out_odp = nl_attr_get_odp_port(nla);
>> +        out_port = dummy_offload_get_port_by_odp_port(offload_, out_odp);
>> +        if (out_odp != in_odp
>> +            && (!out_port
>> +                || strcmp("dummy-pmd",
>> +                          netdev_get_type(out_port->pm_port.netdev)))) {
>> +            ofpbuf_put(&buf, nla, NLA_ALIGN(nla->nla_len));
>> +        }
>> +    }
>> +
>> +    if (buf.size) {
>> +        alt_actions = xmalloc(sizeof *alt_actions + buf.size);
>> +        alt_actions->size = buf.size;
>> +        memcpy(alt_actions->actions, buf.data, buf.size);
>> +    }
>
> Can be simplified just to skip the first action, if there are more (draft, 
> need to add maybe some validity checks):
>
> NL_ATTR_FOR_EACH_UNSAFE (nla, left, nl_actions, nl_actions_len) {
>     if (nla == nl_actions) {
>         continue;
>
>         }
>
>
>
>         break;
>
>     }
>
> alt_actions = xmalloc(sizeof *alt_actions + left);
> alt_actions->size = left;
> memcpy(alt_actions->actions, nla, left);

The action list can contain a mix of dummy-pmd and non-dummy-pmd
output ports in any order, the suggestion does not handle this,
it only skips the first action, missing any non-dummy-pmd ports
later in the list.

[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to