| Message ID | 20230314065802.1532741-3-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: fix parsing of TCA_EXT_WARN_MSG for tc action | expand |
On Tue, Mar 14, 2023 at 2:58 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > In my previous commit 0349b8779cc9 ("sched: add new attr TCA_EXT_WARN_MSG to > report tc extact message") I didn't notice the tc action use different > enum with filter. So we can't use TCA_EXT_WARN_MSG directly for tc action. > Let's add a TCA_ACT_EXT_WARN_MSG for tc action specifically and put this > param before going to the TCA_ACT_TAB nest. > > Fixes: 0349b8779cc9 ("sched: add new attr TCA_EXT_WARN_MSG to report tc extact message") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > include/uapi/linux/rtnetlink.h | 1 + > net/sched/act_api.c | 8 ++++---- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > index 25a0af57dd5e..5ad3448a1fa7 100644 > --- a/include/uapi/linux/rtnetlink.h > +++ b/include/uapi/linux/rtnetlink.h > @@ -789,6 +789,7 @@ enum { > TCA_ROOT_FLAGS, > TCA_ROOT_COUNT, > TCA_ROOT_TIME_DELTA, /* in msecs */ > + TCA_ACT_EXT_WARN_MSG, > __TCA_ROOT_MAX, > #define TCA_ROOT_MAX (__TCA_ROOT_MAX - 1) > }; > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index fce522886099..f960cb534ca0 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -1589,6 +1589,10 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[], > t->tca__pad1 = 0; > t->tca__pad2 = 0; > > + if (extack && extack->_msg && > + nla_put_string(skb, TCA_ACT_EXT_WARN_MSG, extack->_msg)) > + goto out_nlmsg_trim; > + > nest = nla_nest_start_noflag(skb, TCA_ACT_TAB); > if (!nest) > goto out_nlmsg_trim; > @@ -1598,10 +1602,6 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[], > > nla_nest_end(skb, nest); > > - if (extack && extack->_msg && > - nla_put_string(skb, TCA_EXT_WARN_MSG, extack->_msg)) > - goto out_nlmsg_trim; > - > nlh->nlmsg_len = skb_tail_pointer(skb) - b; > > return skb->len; Sorry, only thing i should have mentioned earlier - not clear from here: Do you get two ext warns now in the same netlink message? One for the action and one for the cls? Something to check: on terminal1 > tc monitor on terminal2 > run a command which will get the offload to fail and see what response you get My concern is you may be getting two warnings in one message. cheers, jamal > -- > 2.38.1 >
On Tue, 14 Mar 2023 14:58:02 +0800 Hangbin Liu wrote: > --- a/include/uapi/linux/rtnetlink.h > +++ b/include/uapi/linux/rtnetlink.h > @@ -789,6 +789,7 @@ enum { > TCA_ROOT_FLAGS, > TCA_ROOT_COUNT, > TCA_ROOT_TIME_DELTA, /* in msecs */ > + TCA_ACT_EXT_WARN_MSG, Not TCA_ROOT_EXT_... ? All other attrs in this set are called TCA_ROOT_x > __TCA_ROOT_MAX, > #define TCA_ROOT_MAX (__TCA_ROOT_MAX - 1)
On Wed, Mar 15, 2023 at 12:45:32AM -0700, Jakub Kicinski wrote: > On Tue, 14 Mar 2023 14:58:02 +0800 Hangbin Liu wrote: > > --- a/include/uapi/linux/rtnetlink.h > > +++ b/include/uapi/linux/rtnetlink.h > > @@ -789,6 +789,7 @@ enum { > > TCA_ROOT_FLAGS, > > TCA_ROOT_COUNT, > > TCA_ROOT_TIME_DELTA, /* in msecs */ > > + TCA_ACT_EXT_WARN_MSG, > > Not TCA_ROOT_EXT_... ? > All other attrs in this set are called TCA_ROOT_x Hmm, when we discussed this issue, Jamal suggested to use TCAA_EXT_WARN_MSG. I expand it to TCA_ACT_EXT_WARN_MSG to correspond with the format of TCA_*. But your suggest TCA_ROOT_EXT_ also makes sense. I'm OK to change it. Jamal, what do you think? Thanks Hangbin
On Tue, Mar 14, 2023 at 06:35:29PM -0400, Jamal Hadi Salim wrote: > Sorry, only thing i should have mentioned earlier - not clear from here: > Do you get two ext warns now in the same netlink message? One for the > action and one for the cls? > Something to check: > on terminal1 > tc monitor > on terminal2 > run a command which will get the offload to fail and > see what response you get > > My concern is you may be getting two warnings in one message. From the result we only got 1 warning message. # tc qdisc add dev enp4s0f0np0 ingress # tc filter add dev enp4s0f0np0 ingress flower verbose ct_state +trk+new action drop Warning: mlx5_core: matching on ct_state +new isn't supported. # tc monitor qdisc ingress ffff: dev enp4s0f0np0 parent ffff:fff1 ---------------- added chain dev enp4s0f0np0 parent ffff: chain 0 added filter dev enp4s0f0np0 ingress protocol all pref 49152 flower chain 0 handle 0x1 ct_state +trk+new not_in_hw action order 1: gact action drop random type none pass val 0 index 1 ref 1 bind 1 mlx5_core: matching on ct_state +new isn't supported ^C Thanks Hangbin
On Wed, Mar 15, 2023 at 5:40 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > On Wed, Mar 15, 2023 at 12:45:32AM -0700, Jakub Kicinski wrote: > > On Tue, 14 Mar 2023 14:58:02 +0800 Hangbin Liu wrote: > > > --- a/include/uapi/linux/rtnetlink.h > > > +++ b/include/uapi/linux/rtnetlink.h > > > @@ -789,6 +789,7 @@ enum { > > > TCA_ROOT_FLAGS, > > > TCA_ROOT_COUNT, > > > TCA_ROOT_TIME_DELTA, /* in msecs */ > > > + TCA_ACT_EXT_WARN_MSG, > > > > Not TCA_ROOT_EXT_... ? > > All other attrs in this set are called TCA_ROOT_x > > Hmm, when we discussed this issue, Jamal suggested to use TCAA_EXT_WARN_MSG. > I expand it to TCA_ACT_EXT_WARN_MSG to correspond with the format of TCA_*. > But your suggest TCA_ROOT_EXT_ also makes sense. I'm OK to change it. > > Jamal, what do you think? Yeah, sticking to the TCA_ROOT_ prefixes makes it neater. TCA_ROOT_EXT_WARN_MSG? cheers, jamal
On Wed, Mar 15, 2023 at 5:47 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > On Tue, Mar 14, 2023 at 06:35:29PM -0400, Jamal Hadi Salim wrote: > > Sorry, only thing i should have mentioned earlier - not clear from here: > > Do you get two ext warns now in the same netlink message? One for the > > action and one for the cls? > > Something to check: > > on terminal1 > tc monitor > > on terminal2 > run a command which will get the offload to fail and > > see what response you get > > > > My concern is you may be getting two warnings in one message. > > From the result we only got 1 warning message. > > # tc qdisc add dev enp4s0f0np0 ingress > # tc filter add dev enp4s0f0np0 ingress flower verbose ct_state +trk+new action drop > Warning: mlx5_core: matching on ct_state +new isn't supported. > > # tc monitor > qdisc ingress ffff: dev enp4s0f0np0 parent ffff:fff1 ---------------- > added chain dev enp4s0f0np0 parent ffff: chain 0 > added filter dev enp4s0f0np0 ingress protocol all pref 49152 flower chain 0 handle 0x1 > ct_state +trk+new > not_in_hw > action order 1: gact action drop > random type none pass val 0 > index 1 ref 1 bind 1 > > mlx5_core: matching on ct_state +new isn't supported > ^C Thanks for checking. I was worried from the quick glance that you will end up calling the action code with extack from cls and that the warning will be duplicated. cheers, jamal
On Wed, Mar 15, 2023 at 02:49:43PM -0400, Jamal Hadi Salim wrote: > On Wed, Mar 15, 2023 at 5:47 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > > > On Tue, Mar 14, 2023 at 06:35:29PM -0400, Jamal Hadi Salim wrote: > > > Sorry, only thing i should have mentioned earlier - not clear from here: > > > Do you get two ext warns now in the same netlink message? One for the > > > action and one for the cls? > > > Something to check: > > > on terminal1 > tc monitor > > > on terminal2 > run a command which will get the offload to fail and > > > see what response you get > > > > > > My concern is you may be getting two warnings in one message. > > > > From the result we only got 1 warning message. > > > > # tc qdisc add dev enp4s0f0np0 ingress > > # tc filter add dev enp4s0f0np0 ingress flower verbose ct_state +trk+new action drop > > Warning: mlx5_core: matching on ct_state +new isn't supported. > > > > # tc monitor > > qdisc ingress ffff: dev enp4s0f0np0 parent ffff:fff1 ---------------- > > added chain dev enp4s0f0np0 parent ffff: chain 0 > > added filter dev enp4s0f0np0 ingress protocol all pref 49152 flower chain 0 handle 0x1 > > ct_state +trk+new > > not_in_hw > > action order 1: gact action drop > > random type none pass val 0 > > index 1 ref 1 bind 1 > > > > mlx5_core: matching on ct_state +new isn't supported > > ^C > > Thanks for checking. I was worried from the quick glance that you will > end up calling the action code with extack from cls and that the > warning will be duplicated. The action info should be filled via dump function, which will not call tca_get_fill(). So I think it should be safe. Please correct me if I missed anything. Thanks Hangbin
On Wed, Mar 15, 2023 at 11:33 PM Hangbin Liu <liuhangbin@gmail.com> wrote: > > On Wed, Mar 15, 2023 at 02:49:43PM -0400, Jamal Hadi Salim wrote: > > On Wed, Mar 15, 2023 at 5:47 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > > > > > On Tue, Mar 14, 2023 at 06:35:29PM -0400, Jamal Hadi Salim wrote: > > > > Sorry, only thing i should have mentioned earlier - not clear from here: > > > > Do you get two ext warns now in the same netlink message? One for the > > > > action and one for the cls? > > > > Something to check: > > > > on terminal1 > tc monitor > > > > on terminal2 > run a command which will get the offload to fail and > > > > see what response you get > > > > > > > > My concern is you may be getting two warnings in one message. > > > > > > From the result we only got 1 warning message. > > > > > > # tc qdisc add dev enp4s0f0np0 ingress > > > # tc filter add dev enp4s0f0np0 ingress flower verbose ct_state +trk+new action drop > > > Warning: mlx5_core: matching on ct_state +new isn't supported. > > > > > > # tc monitor > > > qdisc ingress ffff: dev enp4s0f0np0 parent ffff:fff1 ---------------- > > > added chain dev enp4s0f0np0 parent ffff: chain 0 > > > added filter dev enp4s0f0np0 ingress protocol all pref 49152 flower chain 0 handle 0x1 > > > ct_state +trk+new > > > not_in_hw > > > action order 1: gact action drop > > > random type none pass val 0 > > > index 1 ref 1 bind 1 > > > > > > mlx5_core: matching on ct_state +new isn't supported > > > ^C > > > > Thanks for checking. I was worried from the quick glance that you will > > end up calling the action code with extack from cls and that the > > warning will be duplicated. > > The action info should be filled via dump function, which will not call > tca_get_fill(). So I think it should be safe. Please correct me if I missed > anything. Right - for a similar scenario, it will only be called when you offload an action independent of the filter. cheers, jamal
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index 25a0af57dd5e..5ad3448a1fa7 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -789,6 +789,7 @@ enum { TCA_ROOT_FLAGS, TCA_ROOT_COUNT, TCA_ROOT_TIME_DELTA, /* in msecs */ + TCA_ACT_EXT_WARN_MSG, __TCA_ROOT_MAX, #define TCA_ROOT_MAX (__TCA_ROOT_MAX - 1) }; diff --git a/net/sched/act_api.c b/net/sched/act_api.c index fce522886099..f960cb534ca0 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1589,6 +1589,10 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[], t->tca__pad1 = 0; t->tca__pad2 = 0; + if (extack && extack->_msg && + nla_put_string(skb, TCA_ACT_EXT_WARN_MSG, extack->_msg)) + goto out_nlmsg_trim; + nest = nla_nest_start_noflag(skb, TCA_ACT_TAB); if (!nest) goto out_nlmsg_trim; @@ -1598,10 +1602,6 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[], nla_nest_end(skb, nest); - if (extack && extack->_msg && - nla_put_string(skb, TCA_EXT_WARN_MSG, extack->_msg)) - goto out_nlmsg_trim; - nlh->nlmsg_len = skb_tail_pointer(skb) - b; return skb->len;
In my previous commit 0349b8779cc9 ("sched: add new attr TCA_EXT_WARN_MSG to report tc extact message") I didn't notice the tc action use different enum with filter. So we can't use TCA_EXT_WARN_MSG directly for tc action. Let's add a TCA_ACT_EXT_WARN_MSG for tc action specifically and put this param before going to the TCA_ACT_TAB nest. Fixes: 0349b8779cc9 ("sched: add new attr TCA_EXT_WARN_MSG to report tc extact message") Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- include/uapi/linux/rtnetlink.h | 1 + net/sched/act_api.c | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-)