On 5 Jan 2022, at 15:35, Ilya Maximets wrote:
> On 1/3/22 11:19, Eelco Chaudron wrote:
>> Hi Ilya,
>>
>> As you reviewed the v1, I’m wondering if this can get into the next release?
>
> Hi. Sorry, this one fell through the cracks.
No problem, same here, I noticed it when picking up some related work :)
> It seems to be a bug fix, so we'll need to backport it anyway.
> I'll try to get to this patch in a near future.
ACK.
> Best regards, Ilya Maximets.
>
>>
>> I've also added the original authors of the fixes patches; maybe they can
>> review?
>>
>> Eelco
>>
>> On 21 Jun 2021, at 11:20, Eelco Chaudron wrote:
>>
>>> For patch ports, the is_last_action value is not propagated and is
>>> always set to true. This causes non-reversible actions to modify the
>>> packet, and the original content is not preserved when processing
>>> the remaining actions.
>>>
>>> This patch propagates the is_last_action flag for patch port related
>>> actions. In addition, it also fixes a general last action propagation
>>> to the individual actions.
>>>
>>> Fixed check_pkt_larger as last action, as it is a valid case for the
>>> drop action, so it should not be skipped.
>>>
>>> Fixes: feee58b95 ("ofproto-dpif-xlate: Keep track of the last action")
>>> Fixes: 5b34f8fc3 ("Add a new OVS action check_pkt_larger")
>>> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>>>
>>> ---
>>> v2: Fixed additional last action propagation to individual actions.
>>> Fixed problem with the check_pkt_larger now that that the correct
>>> last action state is passed.
>>>
>>> ofproto/ofproto-dpif-xlate.c | 27 ++++++++++++---------------
>>> tests/ofproto-dpif.at | 28 ++++++++++++++++++++++++++++
>>> 2 files changed, 40 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 7108c8a30..7b35f6a32 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -460,7 +460,7 @@ static void xlate_commit_actions(struct xlate_ctx *ctx);
>>>
>>> static void
>>> patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
>>> - struct xport *out_dev);
>>> + struct xport *out_dev, bool is_last_action);
>>>
>>> static void
>>> ctx_trigger_freeze(struct xlate_ctx *ctx)
>>> @@ -3598,7 +3598,7 @@ propagate_tunnel_data_to_flow(struct xlate_ctx *ctx,
>>> struct eth_addr dmac,
>>> static int
>>> native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
>>> const struct flow *flow, odp_port_t tunnel_odp_port,
>>> - bool truncate)
>>> + bool truncate, bool is_last_action)
>>> {
>>> struct netdev_tnl_build_header_params tnl_params;
>>> struct ovs_action_push_tnl tnl_push_data;
>>> @@ -3728,7 +3728,7 @@ native_tunnel_output(struct xlate_ctx *ctx, const
>>> struct xport *xport,
>>> entry->tunnel_hdr.hdr_size = tnl_push_data.header_len;
>>> entry->tunnel_hdr.operation = ADD;
>>>
>>> - patch_port_output(ctx, xport, out_dev);
>>> + patch_port_output(ctx, xport, out_dev, is_last_action);
>>>
>>> /* Similar to the stats update in revalidation, the x_cache entries
>>> * are populated by the previous translation are used to update the
>>> @@ -3822,7 +3822,7 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx,
>>> const struct flow *flow, co
>>> */
>>> static void
>>> patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
>>> - struct xport *out_dev)
>>> + struct xport *out_dev, bool is_last_action)
>>> {
>>> struct flow *flow = &ctx->xin->flow;
>>> struct flow old_flow = ctx->xin->flow;
>>> @@ -3864,8 +3864,9 @@ patch_port_output(struct xlate_ctx *ctx, const struct
>>> xport *in_dev,
>>> if (!process_special(ctx, out_dev) && may_receive(out_dev, ctx)) {
>>> if (xport_stp_forward_state(out_dev) &&
>>> xport_rstp_forward_state(out_dev)) {
>>> +
>>> xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
>>> - false, true, clone_xlate_actions);
>>> + false, is_last_action, clone_xlate_actions);
>>> if (!ctx->freezing) {
>>> xlate_action_set(ctx);
>>> }
>>> @@ -3880,7 +3881,7 @@ patch_port_output(struct xlate_ctx *ctx, const struct
>>> xport *in_dev,
>>> mirror_mask_t old_mirrors2 = ctx->mirrors;
>>>
>>> xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
>>> - false, true, clone_xlate_actions);
>>> + false, is_last_action, clone_xlate_actions);
>>> ctx->mirrors = old_mirrors2;
>>> ctx->base_flow = old_base_flow;
>>> ctx->odp_actions->size = old_size;
>>> @@ -4107,7 +4108,7 @@ terminate_native_tunnel(struct xlate_ctx *ctx, struct
>>> flow *flow,
>>> static void
>>> compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
>>> const struct xlate_bond_recirc *xr, bool check_stp,
>>> - bool is_last_action OVS_UNUSED, bool truncate)
>>> + bool is_last_action, bool truncate)
>>> {
>>> const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
>>> struct flow_wildcards *wc = ctx->wc;
>>> @@ -4144,7 +4145,7 @@ compose_output_action__(struct xlate_ctx *ctx,
>>> ofp_port_t ofp_port,
>>> if (truncate) {
>>> xlate_report_error(ctx, "Cannot truncate output to patch port");
>>> }
>>> - patch_port_output(ctx, xport, xport->peer);
>>> + patch_port_output(ctx, xport, xport->peer, is_last_action);
>>> return;
>>> }
>>>
>>> @@ -4239,7 +4240,8 @@ compose_output_action__(struct xlate_ctx *ctx,
>>> ofp_port_t ofp_port,
>>> xr->recirc_id);
>>> } else if (is_native_tunnel) {
>>> /* Output to native tunnel port. */
>>> - native_tunnel_output(ctx, xport, flow, odp_port, truncate);
>>> + native_tunnel_output(ctx, xport, flow, odp_port, truncate,
>>> + is_last_action);
>>> flow->tunnel = flow_tnl; /* Restore tunnel metadata */
>>>
>>> } else if (terminate_native_tunnel(ctx, flow, wc,
>>> @@ -6742,7 +6744,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
>>> ofpacts_len,
>>> const struct ofpact_set_field *set_field;
>>> const struct mf_field *mf;
>>> bool last = is_last_action && ofpact_last(a, ofpacts, ofpacts_len)
>>> - && ctx->action_set.size;
>>> + && !ctx->action_set.size;
>>>
>>> if (ctx->error) {
>>> break;
>>> @@ -7145,11 +7147,6 @@ do_xlate_actions(const struct ofpact *ofpacts,
>>> size_t ofpacts_len,
>>> break;
>>>
>>> case OFPACT_CHECK_PKT_LARGER: {
>>> - if (last) {
>>> - /* If this is last action, then there is no need to
>>> - * translate the action. */
>>> - break;
>>> - }
>>> const struct ofpact *remaining_acts = ofpact_next(a);
>>> size_t remaining_acts_len =
>>> ofpact_remaining_len(remaining_acts,
>>> ofpacts,
>>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>>> index 31064ed95..adf24d671 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/ofproto-dpif.at
>>> @@ -8416,6 +8416,34 @@ AT_CHECK([sed -n
>>> 's/=[[0-9]][[0-9]]\(\.[[0-9]][[0-9]]*\)\{0,1\}s/=?s/p' stdout],
>>> OVS_VSWITCHD_STOP
>>> AT_CLEANUP
>>>
>>> +
>>> +AT_SETUP([ofproto-dpif - patch ports - meter (clone)])
>>> +
>>> +OVS_VSWITCHD_START(
>>> + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \
>>> + add-port br0 p1 -- set Interface p1 type=patch \
>>> + options:peer=p2 ofport_request=2 --
>>> \
>>> + add-br br1 -- \
>>> + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
>>> + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
>>> + fail-mode=secure -- \
>>> + add-port br1 p2 -- set Interface p2 type=patch \
>>> + options:peer=p1 -- \
>>> + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3])
>>> +
>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br1 'meter=1 pktps stats
>>> bands=type=drop rate=2'])
>>> +AT_CHECK([ovs-ofctl del-flows br0])
>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0
>>> in_port=local,ip,actions=2,1])
>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1
>>> in_port=1,ip,actions=meter:1,3])
>>> +
>>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
>>> 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'],
>>> [0], [stdout])
>>> +AT_CHECK([tail -1 stdout], [0],
>>> + [Datapath actions: clone(meter(0),3),1
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP
>>> +AT_CLEANUP
>>> +
>>> dnl ----------------------------------------------------------------------
>>> AT_BANNER([ofproto-dpif -- megaflows])
>>>
>>>
>>> _______________________________________________
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
