On 6/28/24 23:08, Ilya Maximets wrote:
> On 6/20/24 09:35, Sunyang Wu via dev wrote:
>> Add the "set dscp action" parsing function,
>> so that the "set dscp action" can be offloaded.
>>
>> Signed-off-by: Sunyang Wu <[email protected]>
>> ---
>> lib/netdev-offload-dpdk.c | 19 ++++++++++++++++++-
>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>
>
> Hi, Sunyang Wu. Thanks for the patch!
> See some comments inline.
>
> Best regards, Ilya Maximets.
>
>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>> index 623005b1c..524942457 100644
>> --- a/lib/netdev-offload-dpdk.c
>> +++ b/lib/netdev-offload-dpdk.c
>> @@ -791,6 +791,17 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
>> ds_put_format(s, "port %"PRIu16" ", ntohs(set_tp->port));
>> }
>> ds_put_cstr(s, "/ ");
>> + } else if (actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP ||
>> + actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP) {
>> + const struct rte_flow_action_set_dscp *set_dscp = actions->conf;
>> + char *dirstr = actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP
>> + ? "set_ipv4_dscp " : "set_ipv6_dscp ";
>> +
>> + ds_put_cstr(s, dirstr);
>> + if (set_dscp) {
>> + ds_put_format(s, "dscp_value %d ", set_dscp->dscp);
>> + }
>> + ds_put_cstr(s, "/ ");
>> } else if (actions->type == RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN) {
>> const struct rte_flow_action_of_push_vlan *of_push_vlan =
>> actions->conf;
>> @@ -1835,7 +1846,10 @@ add_set_flow_action__(struct flow_actions *actions,
>> if (is_all_zeros(mask, size)) {
>> return 0;
>> }
>> - if (!is_all_ones(mask, size)) {
>> + if (!is_all_ones(mask, size) &&
>> + /* set dscp need patial mask */
>> + attr != RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP &&
>> + attr != RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP) {
>
> This doesn't seem right. We need to check that the mask contains
> all the DSCP bits and that it doesn't have anything else inside.
>
> For example, if we try to set ECN bits, we'll have them in a mask.
> In this case is_all_zeros() check will fail and then this check
> will allow us to proceed as well. In the end, we'll end up setting
> DSCP bits to all zeroes, or worse, to whatever was in the first
> six bits of nw_tos.
>
Also, this function will clear the whole mask below, but we should
only clear DSCP bits in this case.
>> VLOG_DBG_RL(&rl, "Partial mask is not supported");
>> return -1;
>> }
>> @@ -1912,6 +1926,7 @@ parse_set_actions(struct flow_actions *actions,
>> add_set_flow_action(ipv4_src,
>> RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC);
>> add_set_flow_action(ipv4_dst,
>> RTE_FLOW_ACTION_TYPE_SET_IPV4_DST);
>> add_set_flow_action(ipv4_ttl, RTE_FLOW_ACTION_TYPE_SET_TTL);
>> + add_set_flow_action(ipv4_tos,
>> RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP);z
>>
>> if (mask && !is_all_zeros(mask, sizeof *mask)) {
>> VLOG_DBG_RL(&rl, "Unsupported IPv4 set action");
>> @@ -1924,6 +1939,8 @@ parse_set_actions(struct flow_actions *actions,
>> add_set_flow_action(ipv6_src,
>> RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC);
>> add_set_flow_action(ipv6_dst,
>> RTE_FLOW_ACTION_TYPE_SET_IPV6_DST);
>> add_set_flow_action(ipv6_hlimit, RTE_FLOW_ACTION_TYPE_SET_TTL);
>> + add_set_flow_action(ipv6_tclass,
>> + RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP);
>>
>> if (mask && !is_all_zeros(mask, sizeof *mask)) {
>> VLOG_DBG_RL(&rl, "Unsupported IPv6 set action");
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
