| Message ID | 20220503030836.28783-4-jianbol@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for ovs metering with tc offload | 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 3 May 2022, at 5:08, Jianbo Liu via dev wrote: > To reuse the code for manipulating police action, move the common > initialization code to a function, and change PPS parameters as meter > pktps is in unit of packet per second. > > null_police is redundant because either BPS or PPS, not both, can be > configured in one message. So the police passed in to > nl_msg_put_act_police can be reused as its rate is zero for PPS, and > it also provides the index for police action to be created. See comments below… > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > --- > lib/netdev-linux.c | 82 ++++++++++++++++++++++++---------------------- > 1 file changed, 42 insertions(+), 40 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 9d125029d..eb05153c0 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2629,37 +2629,27 @@ 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, > - uint32_t kpkts_rate, uint32_t kpkts_burst) > +nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, > + uint32_t pkts_rate, uint32_t pkts_burst) > { > size_t offset, act_offset; > uint32_t prio = 0; > - /* used for PPS, set rate as 0 to act as a single action */ > - struct tc_police null_police; > - > - memset(&null_police, 0, sizeof null_police); > - > - if (police.rate.rate) { > - nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset); > - tc_put_rtab(request, TCA_POLICE_RATE, &police.rate); > - nl_msg_put_unspec(request, TCA_POLICE_TBF, &police, sizeof police); > - nl_msg_act_police_end_nest(request, offset, act_offset); > - } > - if (kpkts_rate) { > - unsigned int pkt_burst_ticks, pps_rate, size; > - nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset); > - pps_rate = kpkts_rate * 1000; > - size = MIN(UINT32_MAX / 1000, kpkts_burst) * 1000; > + > + nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset); > + if (police->rate.rate) { > + tc_put_rtab(request, TCA_POLICE_RATE, &police->rate); > + } > + if (pkts_rate) { > + unsigned int pkt_burst_ticks; If you make this uint64_t pkt_burst_ticks, you can get rid of the ugly typecast below. > /* Here tc_bytes_to_ticks is used to convert packets rather than bytes > to ticks. */ > - pkt_burst_ticks = tc_bytes_to_ticks(pps_rate, size); > - nl_msg_put_u64(request, TCA_POLICE_PKTRATE64, (uint64_t) pps_rate); > + pkt_burst_ticks = tc_bytes_to_ticks(pkts_rate, pkts_burst); > + nl_msg_put_u64(request, TCA_POLICE_PKTRATE64, (uint64_t) pkts_rate); > nl_msg_put_u64(request, TCA_POLICE_PKTBURST64, > (uint64_t) pkt_burst_ticks); nl_msg_put_u64(request, TCA_POLICE_PKTBURST64, pkt_burst_ticks); > - nl_msg_put_unspec(request, TCA_POLICE_TBF, &null_police, > - sizeof null_police); > - nl_msg_act_police_end_nest(request, offset, act_offset); > } > + nl_msg_put_unspec(request, TCA_POLICE_TBF, police, sizeof *police); > + nl_msg_act_police_end_nest(request, offset, act_offset); You moved this out, but is there a potential way I can configure both kpkts_rate = 0 and police->rate.rate = 0? If so we should not do the add out of the if statements. Maybe just in case at this as the beginning of the function: if (!kpkts_rate && !police->rate.rate) { return; } > } > > static int > @@ -2692,7 +2682,8 @@ tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, > 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, kpkts_burst); > + nl_msg_put_act_police(&request, &pol_act, kpkts_rate * 1000, > + kpkts_burst * 1000); This was earlier calculated as: MIN(UINT32_MAX / 1000, kpkts_burst) * 1000; Are we ok with only doing * 1000? Only!? Maybe even better is to change the uint32_t pkts_rate, uint32_t pkts_burst in nl_msg_put_act_police() to uint64_t as this is the size that gets added to the NetLink message. > nl_msg_end_nested(&request, action_offset); > nl_msg_end_nested(&request, basic_offset); > > @@ -5599,6 +5590,29 @@ netdev_linux_tc_make_request(const struct netdev *netdev, int type, > return tc_make_request(ifindex, type, flags, request); > } > > +static void > +tc_policer_init(struct tc_police *tc_police, uint32_t kbits_rate, > + uint32_t kbits_burst) Same as above, as TC has uint64_t values for rate and burst, why not use it to avoid all this casting and checking? > +{ > + int mtu = 65535; > + > + memset(tc_police, 0, sizeof *tc_police); > + > + tc_police->action = TC_POLICE_SHOT; > + tc_police->mtu = mtu; > + tc_fill_rate(&tc_police->rate, ((uint64_t) kbits_rate * 1000) / 8, mtu); > + > + /* The following appears wrong in one way: In networking a kilobit is > + * usually 1000 bits but this uses 1024 bits. > + * > + * However if you "fix" those problems then "tc filter show ..." shows > + * "125000b", meaning 125,000 bits, when OVS configures it for 1000 kbit == > + * 1,000,000 bits, whereas this actually ends up doing the right thing from > + * tc's point of view. Whatever. */ > + tc_police->burst = tc_bytes_to_ticks( > + tc_police->rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8); > +} > + > /* Adds a policer to 'netdev' with a rate of 'kbits_rate' and a burst size > * of 'kbits_burst', with a rate of 'kpkts_rate' and a burst size of > * 'kpkts_burst'. > @@ -5623,22 +5637,8 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, > struct ofpbuf request; > struct tcmsg *tcmsg; > int error; > - int mtu = 65535; > > - memset(&tc_police, 0, sizeof tc_police); > - tc_police.action = TC_POLICE_SHOT; > - tc_police.mtu = mtu; > - tc_fill_rate(&tc_police.rate, ((uint64_t) kbits_rate * 1000)/8, mtu); > - > - /* The following appears wrong in one way: In networking a kilobit is > - * usually 1000 bits but this uses 1024 bits. > - * > - * However if you "fix" those problems then "tc filter show ..." shows > - * "125000b", meaning 125,000 bits, when OVS configures it for 1000 kbit == > - * 1,000,000 bits, whereas this actually ends up doing the right thing from > - * tc's point of view. Whatever. */ > - tc_police.burst = tc_bytes_to_ticks( > - tc_police.rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8); > + tc_policer_init(&tc_police, kbits_rate, kbits_burst); > tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWTFILTER, > NLM_F_EXCL | NLM_F_CREATE, &request); > if (!tcmsg) { > @@ -5648,9 +5648,11 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, > tcmsg->tcm_info = tc_make_handle(49, > (OVS_FORCE uint16_t) htons(ETH_P_ALL)); > nl_msg_put_string(&request, TCA_KIND, "basic"); > + > basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS); > police_offset = nl_msg_start_nested(&request, TCA_BASIC_ACT); I would move the tc_policer_init() call from above to here, but that’s just my preference, as it’s not used until here. > - nl_msg_put_act_police(&request, tc_police, kpkts_rate, kpkts_burst); > + nl_msg_put_act_police(&request, &tc_police, kpkts_rate * 1000, > + kpkts_burst * 1000); Same remark here about the missing “MIN(UINT32_MAX / 1000, kpkts_burst) * 1000;” maybe use uint64_t in this function as an input and it will work all the way down to the other functions. > nl_msg_end_nested(&request, police_offset); > nl_msg_end_nested(&request, basic_offset); > > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 9d125029d..eb05153c0 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2629,37 +2629,27 @@ 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, - uint32_t kpkts_rate, uint32_t kpkts_burst) +nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, + uint32_t pkts_rate, uint32_t pkts_burst) { size_t offset, act_offset; uint32_t prio = 0; - /* used for PPS, set rate as 0 to act as a single action */ - struct tc_police null_police; - - memset(&null_police, 0, sizeof null_police); - - if (police.rate.rate) { - nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset); - tc_put_rtab(request, TCA_POLICE_RATE, &police.rate); - nl_msg_put_unspec(request, TCA_POLICE_TBF, &police, sizeof police); - nl_msg_act_police_end_nest(request, offset, act_offset); - } - if (kpkts_rate) { - unsigned int pkt_burst_ticks, pps_rate, size; - nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset); - pps_rate = kpkts_rate * 1000; - size = MIN(UINT32_MAX / 1000, kpkts_burst) * 1000; + + nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset); + if (police->rate.rate) { + tc_put_rtab(request, TCA_POLICE_RATE, &police->rate); + } + if (pkts_rate) { + unsigned int pkt_burst_ticks; /* Here tc_bytes_to_ticks is used to convert packets rather than bytes to ticks. */ - pkt_burst_ticks = tc_bytes_to_ticks(pps_rate, size); - nl_msg_put_u64(request, TCA_POLICE_PKTRATE64, (uint64_t) pps_rate); + pkt_burst_ticks = tc_bytes_to_ticks(pkts_rate, pkts_burst); + nl_msg_put_u64(request, TCA_POLICE_PKTRATE64, (uint64_t) pkts_rate); nl_msg_put_u64(request, TCA_POLICE_PKTBURST64, (uint64_t) pkt_burst_ticks); - nl_msg_put_unspec(request, TCA_POLICE_TBF, &null_police, - sizeof null_police); - nl_msg_act_police_end_nest(request, offset, act_offset); } + nl_msg_put_unspec(request, TCA_POLICE_TBF, police, sizeof *police); + nl_msg_act_police_end_nest(request, offset, act_offset); } static int @@ -2692,7 +2682,8 @@ tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, 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, kpkts_burst); + nl_msg_put_act_police(&request, &pol_act, kpkts_rate * 1000, + kpkts_burst * 1000); nl_msg_end_nested(&request, action_offset); nl_msg_end_nested(&request, basic_offset); @@ -5599,6 +5590,29 @@ netdev_linux_tc_make_request(const struct netdev *netdev, int type, return tc_make_request(ifindex, type, flags, request); } +static void +tc_policer_init(struct tc_police *tc_police, uint32_t kbits_rate, + uint32_t kbits_burst) +{ + int mtu = 65535; + + memset(tc_police, 0, sizeof *tc_police); + + tc_police->action = TC_POLICE_SHOT; + tc_police->mtu = mtu; + tc_fill_rate(&tc_police->rate, ((uint64_t) kbits_rate * 1000) / 8, mtu); + + /* The following appears wrong in one way: In networking a kilobit is + * usually 1000 bits but this uses 1024 bits. + * + * However if you "fix" those problems then "tc filter show ..." shows + * "125000b", meaning 125,000 bits, when OVS configures it for 1000 kbit == + * 1,000,000 bits, whereas this actually ends up doing the right thing from + * tc's point of view. Whatever. */ + tc_police->burst = tc_bytes_to_ticks( + tc_police->rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8); +} + /* Adds a policer to 'netdev' with a rate of 'kbits_rate' and a burst size * of 'kbits_burst', with a rate of 'kpkts_rate' and a burst size of * 'kpkts_burst'. @@ -5623,22 +5637,8 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, struct ofpbuf request; struct tcmsg *tcmsg; int error; - int mtu = 65535; - memset(&tc_police, 0, sizeof tc_police); - tc_police.action = TC_POLICE_SHOT; - tc_police.mtu = mtu; - tc_fill_rate(&tc_police.rate, ((uint64_t) kbits_rate * 1000)/8, mtu); - - /* The following appears wrong in one way: In networking a kilobit is - * usually 1000 bits but this uses 1024 bits. - * - * However if you "fix" those problems then "tc filter show ..." shows - * "125000b", meaning 125,000 bits, when OVS configures it for 1000 kbit == - * 1,000,000 bits, whereas this actually ends up doing the right thing from - * tc's point of view. Whatever. */ - tc_police.burst = tc_bytes_to_ticks( - tc_police.rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8); + tc_policer_init(&tc_police, kbits_rate, kbits_burst); tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWTFILTER, NLM_F_EXCL | NLM_F_CREATE, &request); if (!tcmsg) { @@ -5648,9 +5648,11 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, tcmsg->tcm_info = tc_make_handle(49, (OVS_FORCE uint16_t) htons(ETH_P_ALL)); nl_msg_put_string(&request, TCA_KIND, "basic"); + basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS); police_offset = nl_msg_start_nested(&request, TCA_BASIC_ACT); - nl_msg_put_act_police(&request, tc_police, kpkts_rate, kpkts_burst); + nl_msg_put_act_police(&request, &tc_police, kpkts_rate * 1000, + kpkts_burst * 1000); nl_msg_end_nested(&request, police_offset); nl_msg_end_nested(&request, basic_offset);
To reuse the code for manipulating police action, move the common initialization code to a function, and change PPS parameters as meter pktps is in unit of packet per second. null_police is redundant because either BPS or PPS, not both, can be configured in one message. So the police passed in to nl_msg_put_act_police can be reused as its rate is zero for PPS, and it also provides the index for police action to be created. Signed-off-by: Jianbo Liu <jianbol@nvidia.com> --- lib/netdev-linux.c | 82 ++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 40 deletions(-)