| Message ID | 20240620073520.47804-1-sunyang.wu@jaguarmicro.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev,v3] netdev-offload-dpdk: Support offload of set dscp action. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Thu, Jun 20, 2024 at 03:35:20PM +0800, 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 <sunyang.wu@jaguarmicro.com> Acked-by: Simon Horman <horms@ovn.org>
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 <sunyang.wu@jaguarmicro.com> > --- > 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. > 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");
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 <sunyang.wu@jaguarmicro.com> >> --- >> 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"); >
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) { 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); 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");
Add the "set dscp action" parsing function, so that the "set dscp action" can be offloaded. Signed-off-by: Sunyang Wu <sunyang.wu@jaguarmicro.com> --- lib/netdev-offload-dpdk.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)