| Message ID | 20211209092806.12336-9-simon.horman@corigine.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | allow user to offload tc action to net device | expand |
On 2021-12-09 04:28, Simon Horman wrote: > include/net/act_api.h | 1 + > include/net/pkt_cls.h | 18 ++++++++++-------- > net/sched/act_api.c | 34 ++++++++++++++++++++++++++++++++++ > 3 files changed, 45 insertions(+), 8 deletions(-) > > diff --git a/include/net/act_api.h b/include/net/act_api.h > index 7e4e79b50216..ce094e79f722 100644 > --- a/include/net/act_api.h > +++ b/include/net/act_api.h > @@ -253,6 +253,7 @@ void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets, > u64 drops, bool hw); > int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int); > > +int tcf_action_update_hw_stats(struct tc_action *action); > int tcf_action_check_ctrlact(int action, struct tcf_proto *tp, > struct tcf_chain **handle, > struct netlink_ext_ack *newchain); > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > index 13f0e4a3a136..1942fe72b3e3 100644 > --- a/include/net/pkt_cls.h > +++ b/include/net/pkt_cls.h > @@ -269,18 +269,20 @@ tcf_exts_stats_update(const struct tcf_exts *exts, > #ifdef CONFIG_NET_CLS_ACT > int i; > > - preempt_disable(); > - > for (i = 0; i < exts->nr_actions; i++) { > struct tc_action *a = exts->actions[i]; > > - tcf_action_stats_update(a, bytes, packets, drops, > - lastuse, true); > - a->used_hw_stats = used_hw_stats; > - a->used_hw_stats_valid = used_hw_stats_valid; > - } > + /* if stats from hw, just skip */ > + if (tcf_action_update_hw_stats(a)) { > + preempt_disable(); > + tcf_action_stats_update(a, bytes, packets, drops, > + lastuse, true); > + preempt_enable(); > > - preempt_enable(); > + a->used_hw_stats = used_hw_stats; > + a->used_hw_stats_valid = used_hw_stats_valid; > + } > + } > #endif > } Sorry - didnt quiet follow this one even after reading to the end. I may have missed the obvious in the equivalence: In the old code we did the preempt first then collect. The changed version only does it if tcf_action_update_hw_stats() is true. cheers, jamal
On December 12, 2021 3:52 AM, Jamal Hadi Salim wrote: >On 2021-12-09 04:28, Simon Horman wrote: >> include/net/act_api.h | 1 + >> include/net/pkt_cls.h | 18 ++++++++++-------- >> net/sched/act_api.c | 34 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 45 insertions(+), 8 deletions(-) >> >> diff --git a/include/net/act_api.h b/include/net/act_api.h index >> 7e4e79b50216..ce094e79f722 100644 >> --- a/include/net/act_api.h >> +++ b/include/net/act_api.h >> @@ -253,6 +253,7 @@ void tcf_action_update_stats(struct tc_action *a, >u64 bytes, u64 packets, >> u64 drops, bool hw); >> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, >> int); >> >> +int tcf_action_update_hw_stats(struct tc_action *action); >> int tcf_action_check_ctrlact(int action, struct tcf_proto *tp, >> struct tcf_chain **handle, >> struct netlink_ext_ack *newchain); diff --git >> a/include/net/pkt_cls.h b/include/net/pkt_cls.h index >> 13f0e4a3a136..1942fe72b3e3 100644 >> --- a/include/net/pkt_cls.h >> +++ b/include/net/pkt_cls.h >> @@ -269,18 +269,20 @@ tcf_exts_stats_update(const struct tcf_exts *exts, >> #ifdef CONFIG_NET_CLS_ACT >> int i; >> >> - preempt_disable(); >> - >> for (i = 0; i < exts->nr_actions; i++) { >> struct tc_action *a = exts->actions[i]; >> >> - tcf_action_stats_update(a, bytes, packets, drops, >> - lastuse, true); >> - a->used_hw_stats = used_hw_stats; >> - a->used_hw_stats_valid = used_hw_stats_valid; >> - } >> + /* if stats from hw, just skip */ >> + if (tcf_action_update_hw_stats(a)) { >> + preempt_disable(); >> + tcf_action_stats_update(a, bytes, packets, drops, >> + lastuse, true); >> + preempt_enable(); >> >> - preempt_enable(); >> + a->used_hw_stats = used_hw_stats; >> + a->used_hw_stats_valid = used_hw_stats_valid; >> + } >> + } >> #endif >> } > >Sorry - didnt quiet follow this one even after reading to the end. >I may have missed the obvious in the equivalence: >In the old code we did the preempt first then collect. The changed version only >does it if tcf_action_update_hw_stats() is true. Hi Jamal, for this change, this is because for the function of tcf_action_update_hw_stats, it will try to retrieve hw stats fron hardware. But in he process of retrieving stats information, the driver may have Lock or other sleeping function. So we should not call tcf_action_update_hw_stats function in context of preempt_disable. Actually, since there is no vendor to support update single action stats from hardware, so it is not obvious, we will post our implement support after these patches set. Do you think if it make sense? > >cheers, >jamal
On 2021-12-12 04:00, Baowen Zheng wrote: > On December 12, 2021 3:52 AM, Jamal Hadi Salim wrote: >> On 2021-12-09 04:28, Simon Horman wrote: >>> include/net/act_api.h | 1 + >>> include/net/pkt_cls.h | 18 ++++++++++-------- >>> net/sched/act_api.c | 34 ++++++++++++++++++++++++++++++++++ >>> 3 files changed, 45 insertions(+), 8 deletions(-) >>> >>> diff --git a/include/net/act_api.h b/include/net/act_api.h index >>> 7e4e79b50216..ce094e79f722 100644 >>> --- a/include/net/act_api.h >>> +++ b/include/net/act_api.h >>> @@ -253,6 +253,7 @@ void tcf_action_update_stats(struct tc_action *a, >> u64 bytes, u64 packets, >>> u64 drops, bool hw); >>> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, >>> int); >>> >>> +int tcf_action_update_hw_stats(struct tc_action *action); >>> int tcf_action_check_ctrlact(int action, struct tcf_proto *tp, >>> struct tcf_chain **handle, >>> struct netlink_ext_ack *newchain); diff --git >>> a/include/net/pkt_cls.h b/include/net/pkt_cls.h index >>> 13f0e4a3a136..1942fe72b3e3 100644 >>> --- a/include/net/pkt_cls.h >>> +++ b/include/net/pkt_cls.h >>> @@ -269,18 +269,20 @@ tcf_exts_stats_update(const struct tcf_exts *exts, >>> #ifdef CONFIG_NET_CLS_ACT >>> int i; >>> >>> - preempt_disable(); >>> - >>> for (i = 0; i < exts->nr_actions; i++) { >>> struct tc_action *a = exts->actions[i]; >>> >>> - tcf_action_stats_update(a, bytes, packets, drops, >>> - lastuse, true); >>> - a->used_hw_stats = used_hw_stats; >>> - a->used_hw_stats_valid = used_hw_stats_valid; >>> - } >>> + /* if stats from hw, just skip */ >>> + if (tcf_action_update_hw_stats(a)) { >>> + preempt_disable(); >>> + tcf_action_stats_update(a, bytes, packets, drops, >>> + lastuse, true); >>> + preempt_enable(); >>> >>> - preempt_enable(); >>> + a->used_hw_stats = used_hw_stats; >>> + a->used_hw_stats_valid = used_hw_stats_valid; >>> + } >>> + } >>> #endif >>> } >> >> Sorry - didnt quiet follow this one even after reading to the end. >> I may have missed the obvious in the equivalence: >> In the old code we did the preempt first then collect. The changed version only >> does it if tcf_action_update_hw_stats() is true. > Hi Jamal, for this change, this is because for the function of tcf_action_update_hw_stats, it will try to retrieve hw stats fron hardware. But in he process of retrieving stats information, the driver may have > Lock or other sleeping function. So we should not call tcf_action_update_hw_stats function in context of preempt_disable. Still confused probably because this is one of those functions that are badly named. Initially i thought that it was useful to call this function for both offloaded vs non-offloaded stats. But it seems it is only useful for hw offloaded stats? If so, please consider a patch for renaming this appropriately for readability. Regardless, two things: 1) In the old code the last two lines + a->used_hw_stats = used_hw_stats; + a->used_hw_stats_valid = used_hw_stats_valid; inside the preempt check and with this they are outside. This is fine if the only reason we have this function is for h/w offload. 2) You introduced tcf_action_update_hw_stats() which also does preempt disable/enable and seems to repeat some of the things you are doing as well in this function? > Actually, since there is no vendor to support update single action stats from hardware, so it is not obvious, we will post our implement support after these patches set. > Do you think if it make sense? Since you plan to have more patches: If it doesnt affect your current goals then i would suggest you leave it to later. The question is, with what you already have in this patchset, do we get something functional and standalone? cheers, jamal >> cheers, >> jamal
On December 14, 2021 8:01 PM, Jamal Hadi Salim wrote: >On 2021-12-12 04:00, Baowen Zheng wrote: >> On December 12, 2021 3:52 AM, Jamal Hadi Salim wrote: >>> On 2021-12-09 04:28, Simon Horman wrote: >>>> include/net/act_api.h | 1 + >>>> include/net/pkt_cls.h | 18 ++++++++++-------- >>>> net/sched/act_api.c | 34 ++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 45 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/include/net/act_api.h b/include/net/act_api.h index >>>> 7e4e79b50216..ce094e79f722 100644 >>>> --- a/include/net/act_api.h >>>> +++ b/include/net/act_api.h >>>> @@ -253,6 +253,7 @@ void tcf_action_update_stats(struct tc_action >>>> *a, >>> u64 bytes, u64 packets, >>>> u64 drops, bool hw); >>>> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, >>>> int); >>>> >>>> +int tcf_action_update_hw_stats(struct tc_action *action); >>>> int tcf_action_check_ctrlact(int action, struct tcf_proto *tp, >>>> struct tcf_chain **handle, >>>> struct netlink_ext_ack *newchain); diff --git >>>> a/include/net/pkt_cls.h b/include/net/pkt_cls.h index >>>> 13f0e4a3a136..1942fe72b3e3 100644 >>>> --- a/include/net/pkt_cls.h >>>> +++ b/include/net/pkt_cls.h >>>> @@ -269,18 +269,20 @@ tcf_exts_stats_update(const struct tcf_exts >*exts, >>>> #ifdef CONFIG_NET_CLS_ACT >>>> int i; >>>> >>>> - preempt_disable(); >>>> - >>>> for (i = 0; i < exts->nr_actions; i++) { >>>> struct tc_action *a = exts->actions[i]; >>>> >>>> - tcf_action_stats_update(a, bytes, packets, drops, >>>> - lastuse, true); >>>> - a->used_hw_stats = used_hw_stats; >>>> - a->used_hw_stats_valid = used_hw_stats_valid; >>>> - } >>>> + /* if stats from hw, just skip */ >>>> + if (tcf_action_update_hw_stats(a)) { >>>> + preempt_disable(); >>>> + tcf_action_stats_update(a, bytes, packets, drops, >>>> + lastuse, true); >>>> + preempt_enable(); >>>> >>>> - preempt_enable(); >>>> + a->used_hw_stats = used_hw_stats; >>>> + a->used_hw_stats_valid = used_hw_stats_valid; >>>> + } >>>> + } >>>> #endif >>>> } >>> >>> Sorry - didnt quiet follow this one even after reading to the end. >>> I may have missed the obvious in the equivalence: >>> In the old code we did the preempt first then collect. The changed >>> version only does it if tcf_action_update_hw_stats() is true. >> Hi Jamal, for this change, this is because for the function of >> tcf_action_update_hw_stats, it will try to retrieve hw stats fron hardware. >But in he process of retrieving stats information, the driver may have Lock or >other sleeping function. So we should not call tcf_action_update_hw_stats >function in context of preempt_disable. > >Still confused probably because this is one of those functions that are badly >named. Initially i thought that it was useful to call this function for both >offloaded vs non-offloaded stats. But it seems it is only useful for hw >offloaded stats? If so, please consider a patch for renaming this appropriately >for readability. Yes, it is only for hw offload stats and is used to sync the stats information from the Offloaded filter to the actions the filter referred to. We will consider to add a patch to rename this function for readability. > >Regardless, two things: > >1) In the old code the last two lines >+ a->used_hw_stats = used_hw_stats; >+ a->used_hw_stats_valid = used_hw_stats_valid; >inside the preempt check and with this they are outside. > >This is fine if the only reason we have this function is for h/w offload. > >2) You introduced tcf_action_update_hw_stats() which also does preempt >disable/enable and seems to repeat some of the things you are doing as well >in this function? As I mentioned above, the function of tcf_exts_stats_update is used to sync the stats information from the offloaded filter to the actions the filter referred to. Then the new added function tcf_action_update_hw_stats() is used to sync the stats Information from the hw device that offloads this action. So if the action is offloaded to hw as a single action, then it will not sync the stats from the hw filter. > >> Actually, since there is no vendor to support update single action stats from >hardware, so it is not obvious, we will post our implement support after these >patches set. >> Do you think if it make sense? > >Since you plan to have more patches: >If it doesnt affect your current goals then i would suggest you leave it to later. >The question is, with what you already have in this patchset, do we get >something functional and standalone? > What we will post later to support update single action stats from hardware is code for driver side, It will mainly implement the flow_offload_act_command of stats an action from hw in driver. So i think it will proper to post the whole framework code in act_api and cls_api in this series. Then when we post the driver patch, we will not need to change the act/cls implement. WDYT? >cheers, >jamal > > > > > >>> cheers, >>> jamal
On 2021-12-14 08:43, Baowen Zheng wrote: > On December 14, 2021 8:01 PM, Jamal Hadi Salim wrote: >> On 2021-12-12 04:00, Baowen Zheng wrote: [..] >> >> Still confused probably because this is one of those functions that are badly >> named. Initially i thought that it was useful to call this function for both >> offloaded vs non-offloaded stats. But it seems it is only useful for hw >> offloaded stats? If so, please consider a patch for renaming this appropriately >> for readability. > Yes, it is only for hw offload stats and is used to sync the stats information from the > Offloaded filter to the actions the filter referred to. > > We will consider to add a patch to rename this function for readability. >> >> Regardless, two things: >> >> 1) In the old code the last two lines >> + a->used_hw_stats = used_hw_stats; >> + a->used_hw_stats_valid = used_hw_stats_valid; >> inside the preempt check and with this they are outside. >> >> This is fine if the only reason we have this function is for h/w offload. >> >> 2) You introduced tcf_action_update_hw_stats() which also does preempt >> disable/enable and seems to repeat some of the things you are doing as well >> in this function? > As I mentioned above, the function of tcf_exts_stats_update is used to sync the stats > information from the offloaded filter to the actions the filter referred to. > Then the new added function tcf_action_update_hw_stats() is used to sync the stats > Information from the hw device that offloads this action. So if the action is offloaded > to hw as a single action, then it will not sync the stats from the hw filter. >> >>> Actually, since there is no vendor to support update single action stats from >> hardware, so it is not obvious, we will post our implement support after these >> patches set. >>> Do you think if it make sense? >> >> Since you plan to have more patches: >> If it doesnt affect your current goals then i would suggest you leave it to later. >> The question is, with what you already have in this patchset, do we get >> something functional and standalone? >> > What we will post later to support update single action stats from hardware is code for driver side, > It will mainly implement the flow_offload_act_command of stats an action from hw in driver. > > So i think it will proper to post the whole framework code in act_api and cls_api in this series. > Then when we post the driver patch, we will not need to change the act/cls implement. > > WDYT? ok. cheers, jamal
diff --git a/include/net/act_api.h b/include/net/act_api.h index 7e4e79b50216..ce094e79f722 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -253,6 +253,7 @@ void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets, u64 drops, bool hw); int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int); +int tcf_action_update_hw_stats(struct tc_action *action); int tcf_action_check_ctrlact(int action, struct tcf_proto *tp, struct tcf_chain **handle, struct netlink_ext_ack *newchain); diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 13f0e4a3a136..1942fe72b3e3 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -269,18 +269,20 @@ tcf_exts_stats_update(const struct tcf_exts *exts, #ifdef CONFIG_NET_CLS_ACT int i; - preempt_disable(); - for (i = 0; i < exts->nr_actions; i++) { struct tc_action *a = exts->actions[i]; - tcf_action_stats_update(a, bytes, packets, drops, - lastuse, true); - a->used_hw_stats = used_hw_stats; - a->used_hw_stats_valid = used_hw_stats_valid; - } + /* if stats from hw, just skip */ + if (tcf_action_update_hw_stats(a)) { + preempt_disable(); + tcf_action_stats_update(a, bytes, packets, drops, + lastuse, true); + preempt_enable(); - preempt_enable(); + a->used_hw_stats = used_hw_stats; + a->used_hw_stats_valid = used_hw_stats_valid; + } + } #endif } diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 1d469029f2cd..4e309b8e49bb 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -245,6 +245,37 @@ static int tcf_action_offload_add(struct tc_action *action, return err; } +int tcf_action_update_hw_stats(struct tc_action *action) +{ + struct flow_offload_action fl_act = {}; + int err; + + if (!tc_act_in_hw(action)) + return -EOPNOTSUPP; + + err = flow_action_init(&fl_act, action, FLOW_ACT_STATS, NULL); + if (err) + return err; + + err = tcf_action_offload_cmd(&fl_act, NULL, NULL); + if (!err) { + preempt_disable(); + tcf_action_stats_update(action, fl_act.stats.bytes, + fl_act.stats.pkts, + fl_act.stats.drops, + fl_act.stats.lastused, + true); + preempt_enable(); + action->used_hw_stats = fl_act.stats.used_hw_stats; + action->used_hw_stats_valid = true; + } else { + return -EOPNOTSUPP; + } + + return 0; +} +EXPORT_SYMBOL(tcf_action_update_hw_stats); + static int tcf_action_offload_del(struct tc_action *action) { struct flow_offload_action fl_act = {}; @@ -1317,6 +1348,9 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p, if (p == NULL) goto errout; + /* update hw stats for this action */ + tcf_action_update_hw_stats(p); + /* compat_mode being true specifies a call that is supposed * to add additional backward compatibility statistic TLVs. */