On Fri, Mar 13, 2015 at 11:31 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Mar 10, 2015 at 09:28:22PM -0700, Ansis Atteka wrote:
>> On Tue, Mar 10, 2015 at 1:37 PM, Ben Pfaff <b...@nicira.com> wrote:
>> > Otherwise the policing limits could make no sense if large rates were
>> > specified.
>> >
>> > Reported-by: Zhangguanghui <zhang.guang...@h3c.com>
>> > Signed-off-by: Ben Pfaff <b...@nicira.com>
>> > ---
>> > AUTHORS | 1 +
>> > lib/netdev-linux.c | 13 +++++++------
>> > vswitchd/bridge.c | 4 ++--
>> > 3 files changed, 10 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/AUTHORS b/AUTHORS
>> > index 6e8b4da..00627ef 100644
>> > --- a/AUTHORS
>> > +++ b/AUTHORS
>> > @@ -344,6 +344,7 @@ Voravit T. vora...@kth.se
>> > Yeming Zhao zhaoyem...@gmail.com
>> > Ying Chen yingc...@vmware.com
>> > Yongqiang Liu liuyq7...@gmail.com
>> > +Zhangguanghui zhang.guang...@h3c.com
>> > Ziyou Wang ziy...@vmware.com
>> > Zolt??n Balogh zoltan.bal...@ericsson.com
>> > ankur dwivedi ankurengg2...@gmail.com
>> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> > index 920c884..199e6ac 100644
>> > --- a/lib/netdev-linux.c
>> > +++ b/lib/netdev-linux.c
>> > @@ -1,5 +1,5 @@
>> > /*
>> > - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
>> > + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
>> > *
>> > * Licensed under the Apache License, Version 2.0 (the "License");
>> > * you may not use this file except in compliance with the License.
>> > @@ -405,8 +405,8 @@ static struct tcmsg *tc_make_request(const struct
>> > netdev *, int type,
>> > unsigned int flags, struct ofpbuf *);
>> > static int tc_transact(struct ofpbuf *request, struct ofpbuf **replyp);
>> > static int tc_add_del_ingress_qdisc(struct netdev *netdev, bool add);
>> > -static int tc_add_policer(struct netdev *netdev, int kbits_rate,
>> > - int kbits_burst);
>> > +static int tc_add_policer(struct netdev *,
>> > + uint32_t kbits_rate, uint32_t kbits_burst);
>> >
>> > static int tc_parse_qdisc(const struct ofpbuf *, const char **kind,
>> > struct nlattr **options);
>> > @@ -4525,7 +4525,8 @@ tc_add_del_ingress_qdisc(struct netdev *netdev, bool
>> > add)
>> > * Returns 0 if successful, otherwise a positive errno value.
>> > */
>> > static int
>> > -tc_add_policer(struct netdev *netdev, int kbits_rate, int kbits_burst)
>> > +tc_add_policer(struct netdev *netdev,
>> > + uint32_t kbits_rate, uint32_t kbits_burst)
>> > {
>> > struct tc_police tc_police;
>> > struct ofpbuf request;
>> > @@ -4539,8 +4540,8 @@ tc_add_policer(struct netdev *netdev, int
>> > kbits_rate, int kbits_burst)
>> > tc_police.action = TC_POLICE_SHOT;
>> > tc_police.mtu = mtu;
>> > tc_fill_rate(&tc_police.rate, ((uint64_t) kbits_rate * 1000)/8, mtu);
>> > - tc_police.burst = tc_bytes_to_ticks(tc_police.rate.rate,
>> > - kbits_burst * 1024);
>> > + tc_police.burst = tc_bytes_to_ticks(
>> > + tc_police.rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * 1024);
>>
>> Maybe silly questions, since I actually did not try out the code, but
>> 1. is it correct that above kbits_burst is multiplied with 1024, but
>> in tc_fill_rate() kbits_rate is multiplied with 1000?
>> 2. also, tc_fill_rate() above seems to init tc_police.rate in bytes
>> not bits, but tc_bytes_to_ticks() expects both arguments to be in same
>> units to cancel out?
>
> It's definitely weird, but testing shows that it does the right thing
> at least from "tc filter show"'s point of view. How about if I add
> this comment?
>
> /* The following appears wrong in two ways:
> *
> * - tc_bytes_to_ticks() should take "bytes" as quantity for both of its
> * arguments (or at least consistently "bytes" as both or "bits" as
> * both), but this supplies bytes for the first argument and bits for
> the
> * second.
> *
> * - 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, so while this actually ends up doing the right thing
> * from tc's point of view. Whatever. */
Acked-by: Ansis Atteka <aatt...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
