Hi, Ilya, Thank you for your valuable advice!
In lines 7390-7406 of the 'ofproto_dpif xlate.c' file, show below:
case OFPACT_SET_IP_DSCP:
if (is_ip_any(flow)) {
WC_MASK_FIELD(wc, nw_proto);
wc->masks.nw_tos |= IP_DSCP_MASK;
flow->nw_tos &= ~IP_DSCP_MASK;
flow->nw_tos |= ofpact_get_SET_IP_DSCP(a)->dscp;
}
break;
case OFPACT_SET_IP_ECN:
if (is_ip_any(flow)) {
WC_MASK_FIELD(wc, nw_proto);
wc->masks.nw_tos |= IP_ECN_MASK;
flow->nw_tos &= ~IP_ECN_MASK;
flow->nw_tos |= ofpact_get_SET_IP_ECN(a)->ecn;
}
break;
when parsing and setting the DSCP and ECN actions, different masks are applied
to the `nw_tos` field respectively,
and a mask operation is performed on `nw_tos`. Additionally, the same action
identifier is used for offloading both DSCP
and ECN, ensuring that setting DSCP and setting ECN do not interfere with each
other.
Regarding the `add_set_flow_action__` function, the mask is cleared to zero.
This clearing operation is intended
for subsequent checks to determine if all the fields issued are supported. This
mask will not be passed to the DPDK
side, so it will not affect the offloading operation.
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
