Hello Ben:
I have sent a new patch serial, based on your review comments:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341543.html
https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341544.html
Thanks,
Guoshuai
On Mon, Nov 13, 2017 at 08:23:29PM +0800, Guoshuai Li wrote:
This feature is used to limit the bandwidth of flows, such as floating IP.
ovn-northd changes:
1. add bandwidth column in NB's QOS table.
2. add QOS_METER stages in Logical switch ingress/egress.
3. add set_meter() action in SB's LFlow table.
ovn-controller changes:
add meter_table for meter action process openflow meter table.
Now, This feature is only supported in DPDK.
Signed-off-by: Guoshuai Li <[email protected]>
---
v3: Fix bandwidth mix error.
rebasing.
v2: Fix Ingress/Egress Table id error.
Thank you for working on this feature. New QoS features are welcome in
OVN.
"checkpatch" reports some issues. Some of them can be ignored. The
following seem worth fixing to me:
ERROR: Improper whitespace around control block
#231 FILE: ovn/controller/ofctrl.c:829:
HMAP_FOR_EACH_WITH_HASH(e, hmap_node, target->hmap_node.hash,
ERROR: Improper whitespace around control block
#297 FILE: ovn/controller/ofctrl.c:974:
HMAP_FOR_EACH(desired_meter, hmap_node, &meters->desired_meters) {
ERROR: Improper whitespace around control block
#333 FILE: ovn/controller/ofctrl.c:1124:
HMAP_FOR_EACH_SAFE(installed_meter, next_meter, hmap_node,
WARNING: Line length is >79-characters long
#341 FILE: ovn/controller/ofctrl.c:1132:
ds_put_format(&meter_string, "meter=%u",
installed_meter->meter_id);
ERROR: Improper whitespace around control block
#366 FILE: ovn/controller/ofctrl.c:1157:
HMAP_FOR_EACH_SAFE(desired_meter, next_meter, hmap_node,
WARNING: Line length is >79-characters long
#398 FILE: ovn/controller/ofctrl.h:35:
void ofctrl_init(struct group_table *group_table, struct meter_table
*meter_table);
ERROR: Improper whitespace around control block
#451 FILE: ovn/controller/ovn-controller.c:871:
HMAP_FOR_EACH_SAFE(installed_meter, next_meter, hmap_node,
WARNING: Line length is >79-characters long
#903 FILE: ovn/northd/ovn-northd.c:3885:
/* Ingress table 12 and 13: DHCP options and response, by default goto
next.
WARNING: Line length is >79-characters long
#1030 FILE: ovn/ovn-sb.xml:1642:
<b>Parameters</b>: rate limit int field <var>rate</var>, burst
rate limits
In parse_set_meter_action(), please do not use unnecessary casts.
Conversion to uint32_t occurs the same way regardless of whether a cast
is used. I see similar casts that also appear unneeded in build_qos().
In format_SET_METER(), please put the comma before the space here, not
after it:
+ ds_put_format(s, "set_meter(%d ,%d);", cl->rate, cl->burst);
In the definition of struct ovnact_set_meter, I recommend documenting
the units in use (kbps and kbits?).
I do not understand why set_meter() is not allowed in the last table.
In encode_SET_METER(), please use "%"PRIu32 for formatting uint32_ts,
instead of "%d".
It looks to me like group_table and meter_table are the same data
structure, as are group_info and meter_info. I would prefer to avoid
duplicating data structures and the code that manipulates them, if the
same code and data structures can be used. Would you mind working to
use common code and data here?
In build_qos(), %d and uint8_t don't match up. Maybe the cast should be
dropped (and use "%"PRId64):
+ ds_put_format(&dscp_action, "ip.dscp = %d; next;",
+ (uint8_t)qos->value_action[j]);
In ovn-sb.xml, please describe the units (kbps and kbits?).
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
