[ovs-dev] [PATCH v3] ovn: OVN Support QoS meter

[Guoshuai Li]

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 <ligs at dtdream.com>
>> ---
>>
>> 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.