| Message ID | 20230421151651.3032616-7-amorenoz@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improve linux QoS for exotic and fast links | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Fri, Apr 21, 2023 at 05:16:50PM +0200, Adrian Moreno wrote: > In preparation for supporting 64-bit rates in tc policies, move the > allocation and initialization of struct tc_police object inside > nl_msg_put_act_police(). That way, the function is now called with the > actual rates. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> Tested-by: Simon Horman <simon.horman@corigine.com> > --- > lib/netdev-linux.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 8ee75981b..a2bae300c 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2653,21 +2653,26 @@ nl_msg_act_police_end_nest(struct ofpbuf *request, size_t offset, > } > > static void > -nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, > +nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, > + uint64_t kbits_rate, uint64_t kbits_burst, > uint64_t pkts_rate, uint64_t pkts_burst, > uint32_t notexceed_act, bool single_action) nit: not that I have a good idea of a better approach, but this has a lot of parameters now :(
On 4/26/23 15:39, Simon Horman wrote: > On Fri, Apr 21, 2023 at 05:16:50PM +0200, Adrian Moreno wrote: >> In preparation for supporting 64-bit rates in tc policies, move the >> allocation and initialization of struct tc_police object inside >> nl_msg_put_act_police(). That way, the function is now called with the >> actual rates. >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > > Tested-by: Simon Horman <simon.horman@corigine.com> > >> --- >> lib/netdev-linux.c | 37 ++++++++++++++++++------------------- >> 1 file changed, 18 insertions(+), 19 deletions(-) >> >> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >> index 8ee75981b..a2bae300c 100644 >> --- a/lib/netdev-linux.c >> +++ b/lib/netdev-linux.c >> @@ -2653,21 +2653,26 @@ nl_msg_act_police_end_nest(struct ofpbuf *request, size_t offset, >> } >> >> static void >> -nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, >> +nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, >> + uint64_t kbits_rate, uint64_t kbits_burst, >> uint64_t pkts_rate, uint64_t pkts_burst, >> uint32_t notexceed_act, bool single_action) > > nit: not that I have a good idea of a better approach, > but this has a lot of parameters now :( > I know, I felt the same but I don't see a clean way of improving this.
On Wed, May 03, 2023 at 03:53:14PM +0200, Adrian Moreno wrote: > > > On 4/26/23 15:39, Simon Horman wrote: > > On Fri, Apr 21, 2023 at 05:16:50PM +0200, Adrian Moreno wrote: > > > In preparation for supporting 64-bit rates in tc policies, move the > > > allocation and initialization of struct tc_police object inside > > > nl_msg_put_act_police(). That way, the function is now called with the > > > actual rates. > > > > > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > > > > Tested-by: Simon Horman <simon.horman@corigine.com> Sorry, I think I may have hit the wrong button there. I don't recall testing this. So I must have meant. Reviewed-by: Simon Horman <simon.horman@corigine.com> > > > lib/netdev-linux.c | 37 ++++++++++++++++++------------------- > > > 1 file changed, 18 insertions(+), 19 deletions(-) > > > > > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > > > index 8ee75981b..a2bae300c 100644 > > > --- a/lib/netdev-linux.c > > > +++ b/lib/netdev-linux.c > > > @@ -2653,21 +2653,26 @@ nl_msg_act_police_end_nest(struct ofpbuf *request, size_t offset, > > > } > > > static void > > > -nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, > > > +nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, > > > + uint64_t kbits_rate, uint64_t kbits_burst, > > > uint64_t pkts_rate, uint64_t pkts_burst, > > > uint32_t notexceed_act, bool single_action) > > > > nit: not that I have a good idea of a better approach, > > but this has a lot of parameters now :( > > > > I know, I felt the same but I don't see a clean way of improving this. Ack
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 8ee75981b..a2bae300c 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2653,21 +2653,26 @@ nl_msg_act_police_end_nest(struct ofpbuf *request, size_t offset, } static void -nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, +nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, + uint64_t kbits_rate, uint64_t kbits_burst, uint64_t pkts_rate, uint64_t pkts_burst, uint32_t notexceed_act, bool single_action) { + struct tc_police police; size_t offset, act_offset; uint32_t prio = 0; - if (!police->rate.rate && !pkts_rate) { + if (!kbits_rate && !pkts_rate) { return; } + tc_policer_init(&police, kbits_rate, kbits_burst); + police.index = index; + nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset, single_action); - if (police->rate.rate) { - tc_put_rtab(request, TCA_POLICE_RATE, &police->rate, 0); + if (police.rate.rate) { + tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, 0); } if (pkts_rate) { uint64_t pkt_burst_ticks; @@ -2677,7 +2682,7 @@ nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, nl_msg_put_u64(request, TCA_POLICE_PKTRATE64, pkts_rate); nl_msg_put_u64(request, TCA_POLICE_PKTBURST64, pkt_burst_ticks); } - nl_msg_put_unspec(request, TCA_POLICE_TBF, police, sizeof *police); + nl_msg_put_unspec(request, TCA_POLICE_TBF, &police, sizeof police); nl_msg_act_police_end_nest(request, offset, act_offset, notexceed_act); } @@ -2690,7 +2695,6 @@ tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, size_t basic_offset, action_offset; uint16_t prio = TC_RESERVED_PRIORITY_POLICE; int ifindex, err = 0; - struct tc_police pol_act; struct ofpbuf request; struct ofpbuf *reply; struct tcmsg *tcmsg; @@ -2707,12 +2711,12 @@ tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, tcmsg->tcm_info = tc_make_handle(prio, eth_type); tcmsg->tcm_handle = handle; - tc_policer_init(&pol_act, kbits_rate, kbits_burst); nl_msg_put_string(&request, TCA_KIND, "matchall"); basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS); action_offset = nl_msg_start_nested(&request, TCA_MATCHALL_ACT); - nl_msg_put_act_police(&request, &pol_act, kpkts_rate * 1000, - kpkts_burst * 1000, TC_ACT_UNSPEC, false); + nl_msg_put_act_police(&request, 0, kbits_rate, kbits_burst, + kpkts_rate * 1000, kpkts_burst * 1000, TC_ACT_UNSPEC, + false); nl_msg_end_nested(&request, action_offset); nl_msg_end_nested(&request, basic_offset); @@ -5704,7 +5708,6 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, uint32_t kpkts_burst) { size_t basic_offset, police_offset; - struct tc_police tc_police; struct ofpbuf request; struct tcmsg *tcmsg; int error; @@ -5721,9 +5724,9 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS); police_offset = nl_msg_start_nested(&request, TCA_BASIC_ACT); - tc_policer_init(&tc_police, kbits_rate, kbits_burst); - nl_msg_put_act_police(&request, &tc_police, kpkts_rate * 1000ULL, - kpkts_burst * 1000ULL, TC_ACT_UNSPEC, false); + nl_msg_put_act_police(&request, 0, kbits_rate, kbits_burst, + kpkts_rate * 1000ULL, kpkts_burst * 1000ULL, + TC_ACT_UNSPEC, false); nl_msg_end_nested(&request, police_offset); nl_msg_end_nested(&request, basic_offset); @@ -5740,16 +5743,12 @@ tc_add_policer_action(uint32_t index, uint32_t kbits_rate, uint32_t kbits_burst, uint32_t pkts_rate, uint32_t pkts_burst, bool update) { - struct tc_police tc_police; struct ofpbuf request; struct tcamsg *tcamsg; size_t offset; int flags; int error; - tc_policer_init(&tc_police, kbits_rate, kbits_burst); - tc_police.index = index; - flags = (update ? NLM_F_REPLACE : NLM_F_EXCL) | NLM_F_CREATE; tcamsg = tc_make_action_request(RTM_NEWACTION, flags, &request); if (!tcamsg) { @@ -5757,8 +5756,8 @@ tc_add_policer_action(uint32_t index, uint32_t kbits_rate, } offset = nl_msg_start_nested(&request, TCA_ACT_TAB); - nl_msg_put_act_police(&request, &tc_police, pkts_rate, pkts_burst, - TC_ACT_PIPE, true); + nl_msg_put_act_police(&request, index, kbits_rate, kbits_burst, pkts_rate, + pkts_burst, TC_ACT_PIPE, true); nl_msg_end_nested(&request, offset); error = tc_transact(&request, NULL);
In preparation for supporting 64-bit rates in tc policies, move the allocation and initialization of struct tc_police object inside nl_msg_put_act_police(). That way, the function is now called with the actual rates. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- lib/netdev-linux.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-)