On 4/3/25 10:02 AM, Roi Dayan wrote:
>
>
> On 02/04/2025 19:24, Ilya Maximets wrote:
>> On 4/2/25 4:04 PM, Eli Britstein wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ilya Maximets <i.maxim...@ovn.org>
>>>> Sent: Wednesday, 2 April 2025 13:51
>>>> To: Roi Dayan <r...@nvidia.com>; d...@openvswitch.org
>>>> Cc: Maor Dickman <ma...@nvidia.com>; Eli Britstein <el...@nvidia.com>;
>>>> i.maxim...@ovn.org
>>>> Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-xlate: Add a drop action
>>>> for
>>>> native tunnel failure.
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 3/24/25 3:58 PM, Roi Dayan via dev wrote:
>>>>> From: Eli Britstein <el...@nvidia.com>
>>>>>
>>>>> Upon tunnel output failure, due to routing failure for example, add an
>>>>> explicit drop action, so it will appear in the dp-flows for better
>>>>> visibility for that case.
>>>>>
>>>>> Signed-off-by: Eli Britstein <el...@nvidia.com>
>>>>> Reviewed-by: Roi Dayan <r...@nvidia.com>
>>>>> ---
>>>>> ofproto/ofproto-dpif-xlate.c | 27 +++++++++++++++++++++++----
>>>>> 1 file changed, 23 insertions(+), 4 deletions(-)
>>>>
>>>> Thanks, Eli and Roi for the updated series!
>>>>
>>>> Please, add a small test for this functionality to the tunnel-push-pop.at.
>>>> It should be easy to simulate the missing neighbor cache entry, for
>>>> example.
>>>> Style-wise, please, use newer tests (at the end of the file) as a model
>>>> for this
>>>> new test. Datapath flow dumps do not print drop reasons, but it should be
>>>> enough to just see an explicit cloned drop in the test.
>>> Ack
>>>>
>>>> See some more comments below.
>>>>
>>>> Best regards, Ilya Maximets.
>>>>
>>>>>
>>>>> diff --git a/ofproto/ofproto-dpif-xlate.c
>>>>> b/ofproto/ofproto-dpif-xlate.c index eeb1853efd29..40ddd426e6f6 100644
>>>>> --- a/ofproto/ofproto-dpif-xlate.c
>>>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>>>> @@ -461,6 +461,10 @@ const char *xlate_strerror(enum xlate_error error)
>>>>> return "Unknown error";
>>>>> }
>>>>>
>>>>> +static void put_drop_action(struct ofproto_dpif *ofproto,
>>>>> + struct ofpbuf *odp_actions,
>>>>> + enum xlate_error error,
>>>>> + bool is_last_action);
>>>>> static void xlate_action_set(struct xlate_ctx *ctx); static void
>>>>> xlate_commit_actions(struct xlate_ctx *ctx);
>>>>>
>>>>> @@ -3895,6 +3899,8 @@ native_tunnel_output(struct xlate_ctx *ctx,
>>>>> const struct xport *xport,
>>>>>
>>>>> err = tnl_route_lookup_flow(ctx, flow, &d_ip6, &s_ip6, &out_dev);
>>>>> if (err) {
>>>>> + put_drop_action(ctx->xbridge->ofproto, ctx->odp_actions,
>>>>> + XLATE_INVALID_TUNNEL_METADATA,
>>>>> + is_last_action);
>>>>
>>>> Please, define new separate translation error values for each case.
>>>> The invalid tunnel metadata error means that the metadata for the tunnel is
>>>> not correct and cannot be transformed into required format.
>>>> It's not the case for the routing failure or a missing MAC or a neighbor
>>>> cache
>>>> miss below. These should all be separate values.
>>>> Please add them to the end of the xlate_error enum. And we'll need
>>>> corresponding coverage counters in the odp-execute.
>>> The reason I didn't want to do it was because they are defined in
>>> include/linux/openvswitch.h and I was not sure it's ok to change from
>>> OVS context or only from kernel side.
>>
>> I think, we initially wanted them to be available for the kernel, but
>> then decided they should be fully opaque. The header inside the kernel
>> doesn't have this enum, we're free to change it in the userspace header.
>> The definition of this enum lives in the public header just so external
>> tools may better guess what the drop value means without having full
>> sources or dissecting a binary package.
>>
>> The only rule is to keep the values of existing errors, so tools can
>> rely on them not changing too much.
>>
>>> I'll add them.
>>> Is this OK? Other names?
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 406d083b3..556239594 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -449,6 +449,12 @@ const char *xlate_strerror(enum xlate_error error)
>>> return "Too many MPLS labels";
>>> case XLATE_INVALID_TUNNEL_METADATA:
>>> return "Invalid tunnel metadata";
>>> + case XLATE_TUNNEL_ROUTING_FAIL:
>>
>> I'd use the full _FAILED instead of _FAIL, that will be in line with
>> XLATE_BRIDGE_NOT_FOUND or XLATE_FORWARDING_DISABLED.
>>
>>> + return "Tunnel routing fail";
>>
>> We may use the string we already have int he xlate_report:
>>
>> "Native tunnel routing failed"
>>
>>> + case XLATE_TUNNEL_OUTPUT_NO_ETHERNET:
>>> + return "Tunnel output no ethernet";
>>
>> "Tunnel output device lacks Ethernet address"
>>
>>> + case XLATE_TUNNEL_NEIGH_LOOKUP_FAIL:
>>
>> Maybe XLATE_TUNNEL_NEIGH_CACHE_MISS ?
>>
>>> + return "Tunnel neigh lookup fail";
>>
>> "Tunnel neighbor cache miss" ?
>>
>>> case XLATE_UNSUPPORTED_PACKET_TYPE:
>>> return "Unsupported packet type";
>>
>>
>> And list the cases in order of their definitions in the header.
>>
>> Also, there is one more drop case in the code. Something like:
>>
>> XLATE_TUNNEL_HEADER_BUILD_FAILED
>> "Native tunnel header build failed"
>>
>>>
>>>>
>>>> Being able to distinguish drops caused by the neighbor lookup failures
>>>> would
>>>> actually be very useful.
>>>>
>>>>> xlate_report(ctx, OFT_WARN, "native tunnel routing failed");
>>>>> return err;
>>>>> }
>>>>> @@ -3906,6 +3912,8 @@ native_tunnel_output(struct xlate_ctx *ctx, const
>>>> struct xport *xport,
>>>>> /* Use mac addr of bridge port of the peer. */
>>>>> err = netdev_get_etheraddr(out_dev->netdev, &smac);
>>>>> if (err) {
>>>>> + put_drop_action(ctx->xbridge->ofproto, ctx->odp_actions,
>>>>> + XLATE_INVALID_TUNNEL_METADATA,
>>>>> + is_last_action);
>>>>> xlate_report(ctx, OFT_WARN,
>>>>> "tunnel output device lacks Ethernet address");
>>>>> return err;
>>>>> @@ -3920,6 +3928,8 @@ native_tunnel_output(struct xlate_ctx *ctx, const
>>>> struct xport *xport,
>>>>> if (err) {
>>>>> struct in6_addr nh_s_ip6 = in6addr_any;
>>>>>
>>>>> + put_drop_action(ctx->xbridge->ofproto, ctx->odp_actions,
>>>>> + XLATE_INVALID_TUNNEL_METADATA,
>>>>> + is_last_action);
>>>>> xlate_report(ctx, OFT_DETAIL,
>>>>> "neighbor cache miss for %s on bridge %s, "
>>>>> "sending %s request", @@ -6501,13 +6511,21 @@
>>>>> put_ct_label(const struct flow *flow, struct ofpbuf *odp_actions,
>>>>>
>>>>> static void
>>>>> put_drop_action(struct ofproto_dpif *ofproto, struct ofpbuf *odp_actions,
>>>>> - enum xlate_error error)
>>>>> + enum xlate_error error, bool is_last_action)
>>>>> {
>>>>> + size_t offset;
>>>>> +
>>>>> if (!ovs_explicit_drop_action_supported(ofproto)) {
>>>>> return;
>>>>> }
>>>>>
>>>>> + if (!is_last_action) {
>>>>> + offset = nl_msg_start_nested(odp_actions,
>>>> OVS_ACTION_ATTR_CLONE);
>>>>> + }
>>>>> nl_msg_put_u32(odp_actions, OVS_ACTION_ATTR_DROP, error);
>>>>> + if (!is_last_action) {
>>>>> + nl_msg_end_nested(odp_actions, offset);
>>>>> + }
>>>>> }
>>>>>
>>>>> static void
>>>>> @@ -8134,7 +8152,7 @@ xlate_tweak_odp_actions(struct xlate_ctx *ctx)
>>>>> }
>>>>>
>>>>> if (!last_action) {
>>>>> - put_drop_action(ctx->xbridge->ofproto, actions, XLATE_OK);
>>>>> + put_drop_action(ctx->xbridge->ofproto, actions, XLATE_OK,
>>>>> + true);
>>>>
>>>> This is confusing as we're in the explicit !last_action case and we're
>>>> calling this
>>>> function with is_last_action == true.
>>> We are not in the explicit !last_action case.
>>
>> There is 'if (!last_action) {' right above this line...
>>
>>> We are here if the action list is empty and the previous loop was not
>>> iterated even once.
>>> I can add a comment to clarify.
>>>>
>>>> I'd suggest to create a new function for the cloned drop, e.g.
>>>> put_cloned_drop_action() and use it for the tunnel case only. It may call
>>>> the
>>> If we have a dedicated function, it is true that current use of
>>> put_drop_action
>>> remains, but in all the new tunnel drop cases, we will have to do this:
>>> if (is_last_action) {
>>> put_drop_action()
>>> } else {
>>> put_cloned_drop_action()
>>> }
>>> This is longer and repetitive code. Are you sure you prefer it this way?
>>
>> We can have the 'is_last_action' argument in the new put_cloned_drop_action()
>> to avoid the extra if condition.
>>
>
>
> Hi Ilya.
>
> I'm suggesting a re-look on the original patch for this part because I think
> it
> becomes strange code and somewhat redundant even without the "shouldn't
> happen"
> clone() action which ovs is still probing for explicit drop support anyway so
> why to assume empty clone() shouldn't happen or we don't care.
>
> It will look like this:
>
> static void
> put_drop_action(args) {
> if (!supported) {
> return
> }
> put DROP
> }
>
> put_clone_drop_action(args, is_last) {
> if (!supported) { // to avoid empty clone if we don't want empty
> clone().
> return
> }
> start_nested()
> put_drop(action)
> end_nested()
> }
>
>
> All new callers must call put_clone_drop_action() because they could be last
> or
> not.
Not really. It depends on whether we actually intend to stop the pipeline and
actually drop the packet or if we want to just report a failure but continue
processing of the packet. The native tunnel processing is actually an odd case
here, because it decides to clone or not clone the packet based on further
processing in the output bridge. Normally we'd emit a clone before calling
the native_tunnel_output() and then all the drop cases inside would be just
normal drops without this dance with the clones. But it would be more work to
figure out if we can then drop the clone after the fact.
Most other action processing code is structured differently from the native
tunneling and so should be using a plain drop action without any cloning
involved. Because when you need to drop the packet, just drop the packet and
it would be very confusing if dropping the packet actually cloned it and
dropped the clone instead of dropping the original.
> We have 3 old callers 4 new callers.
> In my option having put_clone_drop_action() with an is_last arg is more
> confusing
> than just adding is_last to the original put_drop_action().
>
> alternative to the is_last arg to call it clone arg and reverse the callers
> from put_drop_action(args, is_last) to put_drop_action(args, !is_last)
> and put_drop_action might be more readable as the arg is called clone if to
> wrap in clone() action.
>
> So the question is if to keep the above or are you sure you want to continue
> with new put_clone_drop_action(args, is_last) ?
As per above, I think, we should stick with the put_cloned_drop_action() and
only use it for native tunneling today.
Best regards, Ilya Maximets.
>
> Thanks,
> Roi
>
>
>>>
>>>> original put_drop_action(). There will be a case when we may add an empty
>>>> clone() if the datapath doesn't support the explicit drop, but it should
>>>> actually
>>>> never happen and it's not a big problem even if that happens for any
>>>> reason.
>>>>
>>>>> return;
>>>>> }
>>>>>
>>>>> @@ -8167,7 +8185,7 @@ xlate_tweak_odp_actions(struct xlate_ctx *ctx)
>>>>> last_observe_offset != UINT32_MAX &&
>>>>> (unsigned char *) last_action == (unsigned char *) actions->data
>>>>> +
>>>>> last_observe_offset) {
>>>>> - put_drop_action(ctx->xbridge->ofproto, actions, XLATE_OK);
>>>>> + put_drop_action(ctx->xbridge->ofproto, actions, XLATE_OK,
>>>>> + true);
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -8615,7 +8633,8 @@ exit:
>>>>> if (xin->odp_actions) {
>>>>> ofpbuf_clear(xin->odp_actions);
>>>>> /* Make the drop explicit if the datapath supports it. */
>>>>> - put_drop_action(ctx.xbridge->ofproto, xin->odp_actions,
>>>>> ctx.error);
>>>>> + put_drop_action(ctx.xbridge->ofproto, xin->odp_actions,
>>>>> ctx.error,
>>>>> + true);
>>>>> }
>>>>> } else {
>>>>> /* In the non-error case, see if we can further optimize or
>>>>> tweak
>>>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
