| Message ID | 20240815122245.975440-1-dongml2@chinatelecom.cn |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [ovs-dev,net-next] net: ovs: fix ovs_drop_reasons error | expand |
On Thu, 15 Aug 2024 20:22:45 +0800 Menglong Dong wrote: > I'm sure if I understand it correctly, but it seems that there is > something wrong with ovs_drop_reasons. > > ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but > OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that > ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION". > > Fix this by initializing ovs_drop_reasons with index. > > Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason") > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn> Could you include output? Presumably from drop monitor? I think it should go to net rather than net-next.
On Sat, Aug 17, 2024 at 9:01 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 15 Aug 2024 20:22:45 +0800 Menglong Dong wrote: > > I'm sure if I understand it correctly, but it seems that there is > > something wrong with ovs_drop_reasons. > > > > ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but > > OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that > > ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION". > > > > Fix this by initializing ovs_drop_reasons with index. > > > > Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason") > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn> > > Could you include output? Presumably from drop monitor? > I think it should go to net rather than net-next. I have not tested it yet. I just copy all the code for drop reason subsystem from openvswitch to vxlan, and this happens in the vxlan code. I'm on vacation right now, and I'll test it out next Monday. Thanks! Menglong Dong > -- > pw-bot: cr
On Sat, Aug 17, 2024 at 9:01 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 15 Aug 2024 20:22:45 +0800 Menglong Dong wrote: > > I'm sure if I understand it correctly, but it seems that there is > > something wrong with ovs_drop_reasons. > > > > ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but > > OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that > > ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION". > > > > Fix this by initializing ovs_drop_reasons with index. > > > > Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason") > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn> > > Could you include output? Presumably from drop monitor? I think I'm right. I'm not familiar with ovs, and it's hard to create a drop case for me. So, I did some modification to ovs_vport_receive link this: @@ -510,6 +511,9 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb, tun_info = NULL; } + ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION); + return -ENOMEM; + /* Extract flow from 'skb' into 'key'. */ error = ovs_flow_key_extract(tun_info, skb, &key); if (unlikely(error)) { In the drop watch, I can have the output like this: drop at: ovs_vport_receive+0x78/0xc0 [openvswitc (0xffffffffc068fa78) origin: software input port ifindex: 18 timestamp: Sun Aug 18 03:28:00 2024 142999108 nsec protocol: 0x86dd length: 70 original length: 70 drop reason: OVS_DROP_ACTION_ERROR Obviously, the output is wrong, just like what I suspect. > I think it should go to net rather than net-next. Should I resend this patch to the net branch? Thanks! Menglong Dong > -- > pw-bot: cr
On Sun, 18 Aug 2024 11:35:48 +0800 Menglong Dong wrote: > > I think it should go to net rather than net-next. > > Should I resend this patch to the net branch? Yes please! And with the results of your experiment added to the commit message.
On Thu, Aug 15, 2024 at 08:22:45PM GMT, Menglong Dong wrote: > I'm sure if I understand it correctly, but it seems that there is > something wrong with ovs_drop_reasons. > > ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but > OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that > ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION". > > Fix this by initializing ovs_drop_reasons with index. > > Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason") > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn> The patch looks good to me. Also, tested and verified that, without the patch, adding flow to drop packets results in: drop at: do_execute_actions+0x197/0xb20 [openvsw (0xffffffffc0db6f97) origin: software input port ifindex: 8 timestamp: Tue Aug 20 10:19:17 2024 859853461 nsec protocol: 0x800 length: 98 original length: 98 drop reason: OVS_DROP_ACTION_ERROR With the patch, the same results in: drop at: do_execute_actions+0x197/0xb20 [openvsw (0xffffffffc0db6f97) origin: software input port ifindex: 8 timestamp: Tue Aug 20 10:16:13 2024 475856608 nsec protocol: 0x800 length: 98 original length: 98 drop reason: OVS_DROP_LAST_ACTION Tested-by: Adrian Moreno <amorenoz@redhat.com> Reviewed-by: Adrian Moreno <amorenoz@redhat.com>
On Tue, Aug 20, 2024 at 10:23 PM Adrián Moreno <amorenoz@redhat.com> wrote: > > On Thu, Aug 15, 2024 at 08:22:45PM GMT, Menglong Dong wrote: > > I'm sure if I understand it correctly, but it seems that there is > > something wrong with ovs_drop_reasons. > > > > ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but > > OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that > > ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION". > > > > Fix this by initializing ovs_drop_reasons with index. > > > > Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason") > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn> > > The patch looks good to me. Also, tested and verified that, > without the patch, adding flow to drop packets results in: > > drop at: do_execute_actions+0x197/0xb20 [openvsw (0xffffffffc0db6f97) > origin: software > input port ifindex: 8 > timestamp: Tue Aug 20 10:19:17 2024 859853461 nsec > protocol: 0x800 > length: 98 > original length: 98 > drop reason: OVS_DROP_ACTION_ERROR > > With the patch, the same results in: > > drop at: do_execute_actions+0x197/0xb20 [openvsw (0xffffffffc0db6f97) > origin: software > input port ifindex: 8 > timestamp: Tue Aug 20 10:16:13 2024 475856608 nsec > protocol: 0x800 > length: 98 > original length: 98 > drop reason: OVS_DROP_LAST_ACTION > > Tested-by: Adrian Moreno <amorenoz@redhat.com> > Reviewed-by: Adrian Moreno <amorenoz@redhat.com> > Thanks! I'll resend this patch to the net branch with your testing.
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index 99d72543abd3..249210958f0b 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -2706,7 +2706,7 @@ static struct pernet_operations ovs_net_ops = { }; static const char * const ovs_drop_reasons[] = { -#define S(x) (#x), +#define S(x) [(x) & ~SKB_DROP_REASON_SUBSYS_MASK] = (#x), OVS_DROP_REASONS(S) #undef S };
I'm sure if I understand it correctly, but it seems that there is something wrong with ovs_drop_reasons. ovs_drop_reasons[0] is "OVS_DROP_LAST_ACTION", but OVS_DROP_LAST_ACTION == __OVS_DROP_REASON + 1, which means that ovs_drop_reasons[1] should be "OVS_DROP_LAST_ACTION". Fix this by initializing ovs_drop_reasons with index. Fixes: 9d802da40b7c ("net: openvswitch: add last-action drop reason") Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn> --- net/openvswitch/datapath.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)