On 7/1/24 13:52, Sunyang Wu wrote:
> 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`.
True.
> 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.
I'm not sure I understand what you mean here.
Consider the following sequence of events:
1. OFPACT_SET_IP_ECN is executed.
wc->masks.nw_tos |= IP_ECN_MASK;
flow->nw_tos |= ofpact_get_SET_IP_ECN(a)->ecn;
2. flow and wc are translated into ovs_key_ipv4 elements in key and mask.
So, key->ipv4_tos == flow->nw_tos and mask->ipv4_tos == IP_ECN_MASK.
3. We call add_set_flow_action(ipv4_tos, RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP)
We check if mask->ipv4_tos is all-zero -- it is not.
We check if it is all-ones -- it is not.
We check if the action is RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP -- yes!
4. add_flow_action() is called with value flow->nw_tos, which is our
original ofpact_get_SET_IP_ECN(a)->ecn. Since ECN bits are higher
than DSCP, that is equivalent ot setting DSCP to zero.
5. mask->ipv4_tos is set to all zeroes.
6. Offload suceeds.
Original action was to set ECN to some value, what we offloaded is
seting DSCP to zero, which is a completely different action.
Offloading should have failed instead.
>
> 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.
It will, because we should have failed offloading, since we can't
offload setting of ECN bits.
>
> 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
