| Message ID | 20240624144530.690545-8-marcin.szycik@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Switch API optimizations | expand |
On 6/24/24 16:45, Marcin Szycik wrote: > Track the number of rules and recipes added to switch. Add a tracepoint to > ice_aq_sw_rules(), which shows both rule and recipe count. This information > can be helpful when designing a set of rules to program to the hardware, as > it shows where the practical limit is. Actual limits are known (64 recipes, > 32k rules), but it's hard to translate these values to how many rules the > *user* can actually create, because of extra metadata being implicitly > added, and recipe/rule chaining. Chaining combines several recipes/rules to > create a larger recipe/rule, so one large rule added by the user might > actually consume multiple rules from hardware perspective. > > Rule counter is simply incremented/decremented in ice_aq_sw_rules(), since > all rules are added or removed via it. > > Counting recipes is harder, as recipes can't be removed (only overwritten). > Recipes added via ice_aq_add_recipe() could end up being unused, when > there is an error in later stages of rule creation. Instead, track the > allocation and freeing of recipes, which should reflect the actual usage of > recipes (if something fails after recipe(s) were created, caller should > free them). Also, a number of recipes are loaded from NVM by default - > initialize the recipe counter with the number of these recipes on switch > initialization. > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> > --- > drivers/net/ethernet/intel/ice/ice_common.c | 3 +++ > drivers/net/ethernet/intel/ice/ice_switch.c | 22 +++++++++++++++++++-- > drivers/net/ethernet/intel/ice/ice_trace.h | 18 +++++++++++++++++ > drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ > 4 files changed, 43 insertions(+), 2 deletions(-) > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Dear Marcin, Thank you for your patch. Am 24.06.24 um 16:45 schrieb Marcin Szycik: > Track the number of rules and recipes added to switch. Add a tracepoint to > ice_aq_sw_rules(), which shows both rule and recipe count. This information > can be helpful when designing a set of rules to program to the hardware, as > it shows where the practical limit is. Actual limits are known (64 recipes, > 32k rules), but it's hard to translate these values to how many rules the > *user* can actually create, because of extra metadata being implicitly > added, and recipe/rule chaining. Chaining combines several recipes/rules to > create a larger recipe/rule, so one large rule added by the user might > actually consume multiple rules from hardware perspective. > > Rule counter is simply incremented/decremented in ice_aq_sw_rules(), since > all rules are added or removed via it. > > Counting recipes is harder, as recipes can't be removed (only overwritten). > Recipes added via ice_aq_add_recipe() could end up being unused, when > there is an error in later stages of rule creation. Instead, track the > allocation and freeing of recipes, which should reflect the actual usage of > recipes (if something fails after recipe(s) were created, caller should > free them). Also, a number of recipes are loaded from NVM by default - > initialize the recipe counter with the number of these recipes on switch > initialization. Can you please add an example how to use the tracepoint? > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> > --- > drivers/net/ethernet/intel/ice/ice_common.c | 3 +++ > drivers/net/ethernet/intel/ice/ice_switch.c | 22 +++++++++++++++++++-- > drivers/net/ethernet/intel/ice/ice_trace.h | 18 +++++++++++++++++ > drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ > 4 files changed, 43 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c > index 6abd1b3796ab..009716a12a26 100644 > --- a/drivers/net/ethernet/intel/ice/ice_common.c > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > @@ -934,6 +934,9 @@ static int ice_init_fltr_mgmt_struct(struct ice_hw *hw) > INIT_LIST_HEAD(&sw->vsi_list_map_head); > sw->prof_res_bm_init = 0; > > + /* Initialize recipe count with default recipes read from NVM */ > + sw->recp_cnt = ICE_SW_LKUP_LAST; > + > status = ice_init_def_sw_recp(hw); > if (status) { > devm_kfree(ice_hw_to_dev(hw), hw->switch_info); > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c > index 27828cdfe085..3caafcdc301f 100644 > --- a/drivers/net/ethernet/intel/ice/ice_switch.c > +++ b/drivers/net/ethernet/intel/ice/ice_switch.c > @@ -3,6 +3,7 @@ > > #include "ice_lib.h" > #include "ice_switch.h" > +#include "ice_trace.h" > > #define ICE_ETH_DA_OFFSET 0 > #define ICE_ETH_ETHTYPE_OFFSET 12 > @@ -1961,6 +1962,15 @@ ice_aq_sw_rules(struct ice_hw *hw, void *rule_list, u16 rule_list_sz, > hw->adminq.sq_last_status == ICE_AQ_RC_ENOENT) > status = -ENOENT; > > + if (!status) { > + if (opc == ice_aqc_opc_add_sw_rules) > + hw->switch_info->rule_cnt += num_rules; > + else if (opc == ice_aqc_opc_remove_sw_rules) > + hw->switch_info->rule_cnt -= num_rules; > + } > + > + trace_ice_aq_sw_rules(hw->switch_info); > + > return status; > } > > @@ -2181,8 +2191,10 @@ int ice_alloc_recipe(struct ice_hw *hw, u16 *rid) > sw_buf->res_type = cpu_to_le16(res_type); > status = ice_aq_alloc_free_res(hw, sw_buf, buf_len, > ice_aqc_opc_alloc_res); > - if (!status) > + if (!status) { > *rid = le16_to_cpu(sw_buf->elem[0].e.sw_resp); > + hw->switch_info->recp_cnt++; > + } > > return status; > } > @@ -2196,7 +2208,13 @@ int ice_alloc_recipe(struct ice_hw *hw, u16 *rid) > */ > static int ice_free_recipe_res(struct ice_hw *hw, u16 rid) > { > - return ice_free_hw_res(hw, ICE_AQC_RES_TYPE_RECIPE, 1, &rid); > + int status; > + > + status = ice_free_hw_res(hw, ICE_AQC_RES_TYPE_RECIPE, 1, &rid); > + if (!status) > + hw->switch_info->recp_cnt--; > + > + return status; > } > > /** > diff --git a/drivers/net/ethernet/intel/ice/ice_trace.h b/drivers/net/ethernet/intel/ice/ice_trace.h > index 244cddd2a9ea..07aab6e130cd 100644 > --- a/drivers/net/ethernet/intel/ice/ice_trace.h > +++ b/drivers/net/ethernet/intel/ice/ice_trace.h > @@ -330,6 +330,24 @@ DEFINE_EVENT(ice_esw_br_port_template, > TP_ARGS(port) > ); > > +DECLARE_EVENT_CLASS(ice_switch_stats_template, > + TP_PROTO(struct ice_switch_info *sw_info), > + TP_ARGS(sw_info), > + TP_STRUCT__entry(__field(u16, rule_cnt) > + __field(u8, recp_cnt)), > + TP_fast_assign(__entry->rule_cnt = sw_info->rule_cnt; > + __entry->recp_cnt = sw_info->recp_cnt;), > + TP_printk("rules=%u recipes=%u", > + __entry->rule_cnt, > + __entry->recp_cnt) > +); > + > +DEFINE_EVENT(ice_switch_stats_template, > + ice_aq_sw_rules, > + TP_PROTO(struct ice_switch_info *sw_info), > + TP_ARGS(sw_info) > +); > + > /* End tracepoints */ > > #endif /* _ICE_TRACE_H_ */ > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h > index c330a436d11a..b6bc2de53b0a 100644 > --- a/drivers/net/ethernet/intel/ice/ice_type.h > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > @@ -764,6 +764,8 @@ struct ice_switch_info { > struct ice_sw_recipe *recp_list; > u16 prof_res_bm_init; > u16 max_used_prof_index; > + u16 rule_cnt; > + u8 recp_cnt; > > DECLARE_BITMAP(prof_res_bm[ICE_MAX_NUM_PROFILES], ICE_MAX_FV_WORDS); > }; Kind regards, Paul
On 25.06.2024 10:31, Paul Menzel wrote: > Dear Marcin, > > > Thank you for your patch. > > Am 24.06.24 um 16:45 schrieb Marcin Szycik: >> Track the number of rules and recipes added to switch. Add a tracepoint to >> ice_aq_sw_rules(), which shows both rule and recipe count. This information >> can be helpful when designing a set of rules to program to the hardware, as >> it shows where the practical limit is. Actual limits are known (64 recipes, >> 32k rules), but it's hard to translate these values to how many rules the >> *user* can actually create, because of extra metadata being implicitly >> added, and recipe/rule chaining. Chaining combines several recipes/rules to >> create a larger recipe/rule, so one large rule added by the user might >> actually consume multiple rules from hardware perspective. >> >> Rule counter is simply incremented/decremented in ice_aq_sw_rules(), since >> all rules are added or removed via it. >> >> Counting recipes is harder, as recipes can't be removed (only overwritten). >> Recipes added via ice_aq_add_recipe() could end up being unused, when >> there is an error in later stages of rule creation. Instead, track the >> allocation and freeing of recipes, which should reflect the actual usage of >> recipes (if something fails after recipe(s) were created, caller should >> free them). Also, a number of recipes are loaded from NVM by default - >> initialize the recipe counter with the number of these recipes on switch >> initialization. > > Can you please add an example how to use the tracepoint? Sure, will add to next version. Example configuration: cd /sys/kernel/tracing echo function > current_tracer echo ice_aq_sw_rules > set_ftrace_filter echo ice_aq_sw_rules > set_event echo 1 > tracing_on cat trace Sample output: tc-4097 [069] ...1. 787.595536: ice_aq_sw_rules <-ice_rem_adv_rule tc-4097 [069] ..... 787.595705: ice_aq_sw_rules: rules=9 recipes=15 tc-4098 [057] ...1. 787.652033: ice_aq_sw_rules <-ice_add_adv_rule tc-4098 [057] ..... 787.652201: ice_aq_sw_rules: rules=10 recipes=16 Thanks, Marcin >> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> >> --- >> drivers/net/ethernet/intel/ice/ice_common.c | 3 +++ >> drivers/net/ethernet/intel/ice/ice_switch.c | 22 +++++++++++++++++++-- >> drivers/net/ethernet/intel/ice/ice_trace.h | 18 +++++++++++++++++ >> drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ >> 4 files changed, 43 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c >> index 6abd1b3796ab..009716a12a26 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_common.c >> +++ b/drivers/net/ethernet/intel/ice/ice_common.c >> @@ -934,6 +934,9 @@ static int ice_init_fltr_mgmt_struct(struct ice_hw *hw) >> INIT_LIST_HEAD(&sw->vsi_list_map_head); >> sw->prof_res_bm_init = 0; >> + /* Initialize recipe count with default recipes read from NVM */ >> + sw->recp_cnt = ICE_SW_LKUP_LAST; >> + >> status = ice_init_def_sw_recp(hw); >> if (status) { >> devm_kfree(ice_hw_to_dev(hw), hw->switch_info); >> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c >> index 27828cdfe085..3caafcdc301f 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_switch.c >> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c >> @@ -3,6 +3,7 @@ >> #include "ice_lib.h" >> #include "ice_switch.h" >> +#include "ice_trace.h" >> #define ICE_ETH_DA_OFFSET 0 >> #define ICE_ETH_ETHTYPE_OFFSET 12 >> @@ -1961,6 +1962,15 @@ ice_aq_sw_rules(struct ice_hw *hw, void *rule_list, u16 rule_list_sz, >> hw->adminq.sq_last_status == ICE_AQ_RC_ENOENT) >> status = -ENOENT; >> + if (!status) { >> + if (opc == ice_aqc_opc_add_sw_rules) >> + hw->switch_info->rule_cnt += num_rules; >> + else if (opc == ice_aqc_opc_remove_sw_rules) >> + hw->switch_info->rule_cnt -= num_rules; >> + } >> + >> + trace_ice_aq_sw_rules(hw->switch_info); >> + >> return status; >> } >> @@ -2181,8 +2191,10 @@ int ice_alloc_recipe(struct ice_hw *hw, u16 *rid) >> sw_buf->res_type = cpu_to_le16(res_type); >> status = ice_aq_alloc_free_res(hw, sw_buf, buf_len, >> ice_aqc_opc_alloc_res); >> - if (!status) >> + if (!status) { >> *rid = le16_to_cpu(sw_buf->elem[0].e.sw_resp); >> + hw->switch_info->recp_cnt++; >> + } >> return status; >> } >> @@ -2196,7 +2208,13 @@ int ice_alloc_recipe(struct ice_hw *hw, u16 *rid) >> */ >> static int ice_free_recipe_res(struct ice_hw *hw, u16 rid) >> { >> - return ice_free_hw_res(hw, ICE_AQC_RES_TYPE_RECIPE, 1, &rid); >> + int status; >> + >> + status = ice_free_hw_res(hw, ICE_AQC_RES_TYPE_RECIPE, 1, &rid); >> + if (!status) >> + hw->switch_info->recp_cnt--; >> + >> + return status; >> } >> /** >> diff --git a/drivers/net/ethernet/intel/ice/ice_trace.h b/drivers/net/ethernet/intel/ice/ice_trace.h >> index 244cddd2a9ea..07aab6e130cd 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_trace.h >> +++ b/drivers/net/ethernet/intel/ice/ice_trace.h >> @@ -330,6 +330,24 @@ DEFINE_EVENT(ice_esw_br_port_template, >> TP_ARGS(port) >> ); >> +DECLARE_EVENT_CLASS(ice_switch_stats_template, >> + TP_PROTO(struct ice_switch_info *sw_info), >> + TP_ARGS(sw_info), >> + TP_STRUCT__entry(__field(u16, rule_cnt) >> + __field(u8, recp_cnt)), >> + TP_fast_assign(__entry->rule_cnt = sw_info->rule_cnt; >> + __entry->recp_cnt = sw_info->recp_cnt;), >> + TP_printk("rules=%u recipes=%u", >> + __entry->rule_cnt, >> + __entry->recp_cnt) >> +); >> + >> +DEFINE_EVENT(ice_switch_stats_template, >> + ice_aq_sw_rules, >> + TP_PROTO(struct ice_switch_info *sw_info), >> + TP_ARGS(sw_info) >> +); >> + >> /* End tracepoints */ >> #endif /* _ICE_TRACE_H_ */ >> diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h >> index c330a436d11a..b6bc2de53b0a 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_type.h >> +++ b/drivers/net/ethernet/intel/ice/ice_type.h >> @@ -764,6 +764,8 @@ struct ice_switch_info { >> struct ice_sw_recipe *recp_list; >> u16 prof_res_bm_init; >> u16 max_used_prof_index; >> + u16 rule_cnt; >> + u8 recp_cnt; >> DECLARE_BITMAP(prof_res_bm[ICE_MAX_NUM_PROFILES], ICE_MAX_FV_WORDS); >> }; > > > Kind regards, > > Paul
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 6abd1b3796ab..009716a12a26 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -934,6 +934,9 @@ static int ice_init_fltr_mgmt_struct(struct ice_hw *hw) INIT_LIST_HEAD(&sw->vsi_list_map_head); sw->prof_res_bm_init = 0; + /* Initialize recipe count with default recipes read from NVM */ + sw->recp_cnt = ICE_SW_LKUP_LAST; + status = ice_init_def_sw_recp(hw); if (status) { devm_kfree(ice_hw_to_dev(hw), hw->switch_info); diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c index 27828cdfe085..3caafcdc301f 100644 --- a/drivers/net/ethernet/intel/ice/ice_switch.c +++ b/drivers/net/ethernet/intel/ice/ice_switch.c @@ -3,6 +3,7 @@ #include "ice_lib.h" #include "ice_switch.h" +#include "ice_trace.h" #define ICE_ETH_DA_OFFSET 0 #define ICE_ETH_ETHTYPE_OFFSET 12 @@ -1961,6 +1962,15 @@ ice_aq_sw_rules(struct ice_hw *hw, void *rule_list, u16 rule_list_sz, hw->adminq.sq_last_status == ICE_AQ_RC_ENOENT) status = -ENOENT; + if (!status) { + if (opc == ice_aqc_opc_add_sw_rules) + hw->switch_info->rule_cnt += num_rules; + else if (opc == ice_aqc_opc_remove_sw_rules) + hw->switch_info->rule_cnt -= num_rules; + } + + trace_ice_aq_sw_rules(hw->switch_info); + return status; } @@ -2181,8 +2191,10 @@ int ice_alloc_recipe(struct ice_hw *hw, u16 *rid) sw_buf->res_type = cpu_to_le16(res_type); status = ice_aq_alloc_free_res(hw, sw_buf, buf_len, ice_aqc_opc_alloc_res); - if (!status) + if (!status) { *rid = le16_to_cpu(sw_buf->elem[0].e.sw_resp); + hw->switch_info->recp_cnt++; + } return status; } @@ -2196,7 +2208,13 @@ int ice_alloc_recipe(struct ice_hw *hw, u16 *rid) */ static int ice_free_recipe_res(struct ice_hw *hw, u16 rid) { - return ice_free_hw_res(hw, ICE_AQC_RES_TYPE_RECIPE, 1, &rid); + int status; + + status = ice_free_hw_res(hw, ICE_AQC_RES_TYPE_RECIPE, 1, &rid); + if (!status) + hw->switch_info->recp_cnt--; + + return status; } /** diff --git a/drivers/net/ethernet/intel/ice/ice_trace.h b/drivers/net/ethernet/intel/ice/ice_trace.h index 244cddd2a9ea..07aab6e130cd 100644 --- a/drivers/net/ethernet/intel/ice/ice_trace.h +++ b/drivers/net/ethernet/intel/ice/ice_trace.h @@ -330,6 +330,24 @@ DEFINE_EVENT(ice_esw_br_port_template, TP_ARGS(port) ); +DECLARE_EVENT_CLASS(ice_switch_stats_template, + TP_PROTO(struct ice_switch_info *sw_info), + TP_ARGS(sw_info), + TP_STRUCT__entry(__field(u16, rule_cnt) + __field(u8, recp_cnt)), + TP_fast_assign(__entry->rule_cnt = sw_info->rule_cnt; + __entry->recp_cnt = sw_info->recp_cnt;), + TP_printk("rules=%u recipes=%u", + __entry->rule_cnt, + __entry->recp_cnt) +); + +DEFINE_EVENT(ice_switch_stats_template, + ice_aq_sw_rules, + TP_PROTO(struct ice_switch_info *sw_info), + TP_ARGS(sw_info) +); + /* End tracepoints */ #endif /* _ICE_TRACE_H_ */ diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index c330a436d11a..b6bc2de53b0a 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -764,6 +764,8 @@ struct ice_switch_info { struct ice_sw_recipe *recp_list; u16 prof_res_bm_init; u16 max_used_prof_index; + u16 rule_cnt; + u8 recp_cnt; DECLARE_BITMAP(prof_res_bm[ICE_MAX_NUM_PROFILES], ICE_MAX_FV_WORDS); };