[ovs-dev] [PATCH 2/2] Implement OpenFlow 1.3 queue stats duration feature.
Ben Pfaff
blp at nicira.com
Fri Jul 26 17:25:40 UTC 2013
Thanks for the reviews!
I apologize: I forgot to add your acks.
On Fri, Jul 26, 2013 at 10:03:25AM -0700, Andy Zhou wrote:
> acked-by: Andy Zhou <azhou at nicira.com>
>
>
> On Wed, Jul 17, 2013 at 3:58 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > OPENFLOW-1.1+ | 3 ---
> > lib/netdev-linux.c | 11 ++++++++---
> > lib/netdev.c | 8 ++++++--
> > lib/netdev.h | 3 +++
> > lib/ofp-print.c | 14 +++++++++++---
> > lib/ofp-util.c | 44 ++++++++++++++++++++++++--------------------
> > lib/ofp-util.h | 10 +++++++++-
> > ofproto/ofproto.c | 19 ++++++++++++++-----
> > tests/ofp-print.at | 50
> > ++++++++++++++++++++++++++------------------------
> > 9 files changed, 101 insertions(+), 61 deletions(-)
> >
> > diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
> > index d08a46e..aea689e 100644
> > --- a/OPENFLOW-1.1+
> > +++ b/OPENFLOW-1.1+
> > @@ -143,9 +143,6 @@ didn't compare the specs carefully yet.)
> > * Rework tag order. I'm not sure whether we need to do anything
> > for this.
> >
> > - * Duration for queue stats. (Duration for port stats is already
> > - implemented.)
> > -
> > * On-demand flow counters. I think this might be a real
> > optimization in some cases for the software switch.
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 8790f14..83d3d67 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -148,6 +148,7 @@ struct tc {
> > struct tc_queue {
> > struct hmap_node hmap_node; /* In struct tc's "queues" hmap. */
> > unsigned int queue_id; /* OpenFlow queue ID. */
> > + long long int created; /* Time queue was created, in msecs. */
> > };
> >
> > /* A particular kind of traffic control. Each implementation generally
> > maps to
> > @@ -2010,9 +2011,11 @@ netdev_linux_get_queue_stats(const struct netdev
> > *netdev_,
> > return EOPNOTSUPP;
> > } else {
> > const struct tc_queue *queue = tc_find_queue(netdev_, queue_id);
> > - return (queue
> > - ? netdev->tc->ops->class_get_stats(netdev_, queue, stats)
> > - : ENOENT);
> > + if (!queue) {
> > + return ENOENT;
> > + }
> > + stats->created = queue->created;
> > + return netdev->tc->ops->class_get_stats(netdev_, queue, stats);
> > }
> > }
> >
> > @@ -2819,6 +2822,7 @@ htb_update_queue__(struct netdev *netdev, unsigned
> > int queue_id,
> > hcp = xmalloc(sizeof *hcp);
> > queue = &hcp->tc_queue;
> > queue->queue_id = queue_id;
> > + queue->created = time_msec();
> > hmap_insert(&htb->tc.queues, &queue->hmap_node, hash);
> > }
> >
> > @@ -3052,6 +3056,7 @@ hfsc_update_queue__(struct netdev *netdev, unsigned
> > int queue_id,
> > hcp = xmalloc(sizeof *hcp);
> > queue = &hcp->tc_queue;
> > queue->queue_id = queue_id;
> > + queue->created = time_msec();
> > hmap_insert(&hfsc->tc.queues, &queue->hmap_node, hash);
> > }
> >
> > diff --git a/lib/netdev.c b/lib/netdev.c
> > index 727c538..6e2a92f 100644
> > --- a/lib/netdev.c
> > +++ b/lib/netdev.c
> > @@ -1246,7 +1246,8 @@ netdev_delete_queue(struct netdev *netdev, unsigned
> > int queue_id)
> > /* Obtains statistics about 'queue_id' on 'netdev'. On success, returns
> > 0 and
> > * fills 'stats' with the queue's statistics; individual members of
> > 'stats' may
> > * be set to all-1-bits if the statistic is unavailable. On failure,
> > returns a
> > - * positive errno value and fills 'stats' with all-1-bits. */
> > + * positive errno value and fills 'stats' with values indicating
> > unsupported
> > + * statistics. */
> > int
> > netdev_get_queue_stats(const struct netdev *netdev, unsigned int queue_id,
> > struct netdev_queue_stats *stats)
> > @@ -1258,7 +1259,10 @@ netdev_get_queue_stats(const struct netdev *netdev,
> > unsigned int queue_id,
> > ? class->get_queue_stats(netdev, queue_id, stats)
> > : EOPNOTSUPP);
> > if (retval) {
> > - memset(stats, 0xff, sizeof *stats);
> > + stats->tx_bytes = UINT64_MAX;
> > + stats->tx_packets = UINT64_MAX;
> > + stats->tx_errors = UINT64_MAX;
> > + stats->created = LLONG_MIN;
> > }
> > return retval;
> > }
> > diff --git a/lib/netdev.h b/lib/netdev.h
> > index b1cc319..eb1870b 100644
> > --- a/lib/netdev.h
> > +++ b/lib/netdev.h
> > @@ -227,6 +227,9 @@ struct netdev_queue_stats {
> > uint64_t tx_bytes;
> > uint64_t tx_packets;
> > uint64_t tx_errors;
> > +
> > + /* Time at which the queue was created, in msecs, LLONG_MIN if
> > unknown. */
> > + long long int created;
> > };
> >
> > int netdev_set_policing(struct netdev *, uint32_t kbits_rate,
> > diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> > index 76bd09c..aabb168 100644
> > --- a/lib/ofp-print.c
> > +++ b/lib/ofp-print.c
> > @@ -1737,9 +1737,17 @@ ofp_print_ofpst_queue_reply(struct ds *string,
> > const struct ofp_header *oh,
> > ofp_print_queue_name(string, qs.queue_id);
> > ds_put_cstr(string, ": ");
> >
> > - print_port_stat(string, "bytes=", qs.stats.tx_bytes, 1);
> > - print_port_stat(string, "pkts=", qs.stats.tx_packets, 1);
> > - print_port_stat(string, "errors=", qs.stats.tx_errors, 0);
> > + print_port_stat(string, "bytes=", qs.tx_bytes, 1);
> > + print_port_stat(string, "pkts=", qs.tx_packets, 1);
> > + print_port_stat(string, "errors=", qs.tx_errors, 1);
> > +
> > + ds_put_cstr(string, "duration=");
> > + if (qs.duration_sec != UINT32_MAX) {
> > + ofp_print_duration(string, qs.duration_sec, qs.duration_nsec);
> > + } else {
> > + ds_put_char(string, '?');
> > + }
> > + ds_put_char(string, '\n');
> > }
> > }
> >
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > index f1ba19c..95eb3e0 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -5400,9 +5400,10 @@ ofputil_queue_stats_from_ofp10(struct
> > ofputil_queue_stats *oqs,
> > {
> > oqs->port_no = u16_to_ofp(ntohs(qs10->port_no));
> > oqs->queue_id = ntohl(qs10->queue_id);
> > - oqs->stats.tx_bytes = ntohll(get_32aligned_be64(&qs10->tx_bytes));
> > - oqs->stats.tx_packets = ntohll(get_32aligned_be64(&qs10->tx_packets));
> > - oqs->stats.tx_errors = ntohll(get_32aligned_be64(&qs10->tx_errors));
> > + oqs->tx_bytes = ntohll(get_32aligned_be64(&qs10->tx_bytes));
> > + oqs->tx_packets = ntohll(get_32aligned_be64(&qs10->tx_packets));
> > + oqs->tx_errors = ntohll(get_32aligned_be64(&qs10->tx_errors));
> > + oqs->duration_sec = oqs->duration_nsec = UINT32_MAX;
> >
> > return 0;
> > }
> > @@ -5419,9 +5420,10 @@ ofputil_queue_stats_from_ofp11(struct
> > ofputil_queue_stats *oqs,
> > }
> >
> > oqs->queue_id = ntohl(qs11->queue_id);
> > - oqs->stats.tx_bytes = ntohll(qs11->tx_bytes);
> > - oqs->stats.tx_packets = ntohll(qs11->tx_packets);
> > - oqs->stats.tx_errors = ntohll(qs11->tx_errors);
> > + oqs->tx_bytes = ntohll(qs11->tx_bytes);
> > + oqs->tx_packets = ntohll(qs11->tx_packets);
> > + oqs->tx_errors = ntohll(qs11->tx_errors);
> > + oqs->duration_sec = oqs->duration_nsec = UINT32_MAX;
> >
> > return 0;
> > }
> > @@ -5430,11 +5432,10 @@ static enum ofperr
> > ofputil_queue_stats_from_ofp13(struct ofputil_queue_stats *oqs,
> > const struct ofp13_queue_stats *qs13)
> > {
> > - enum ofperr error
> > - = ofputil_queue_stats_from_ofp11(oqs, &qs13->qs);
> > + enum ofperr error = ofputil_queue_stats_from_ofp11(oqs, &qs13->qs);
> > if (!error) {
> > - /* FIXME: Get qs13->duration_sec and qs13->duration_nsec,
> > - * Add to netdev_queue_stats? */
> > + oqs->duration_sec = ntohl(qs13->duration_sec);
> > + oqs->duration_nsec = ntohl(qs13->duration_nsec);
> > }
> >
> > return error;
> > @@ -5506,9 +5507,9 @@ ofputil_queue_stats_to_ofp10(const struct
> > ofputil_queue_stats *oqs,
> > qs10->port_no = htons(ofp_to_u16(oqs->port_no));
> > memset(qs10->pad, 0, sizeof qs10->pad);
> > qs10->queue_id = htonl(oqs->queue_id);
> > - put_32aligned_be64(&qs10->tx_bytes, htonll(oqs->stats.tx_bytes));
> > - put_32aligned_be64(&qs10->tx_packets, htonll(oqs->stats.tx_packets));
> > - put_32aligned_be64(&qs10->tx_errors, htonll(oqs->stats.tx_errors));
> > + put_32aligned_be64(&qs10->tx_bytes, htonll(oqs->tx_bytes));
> > + put_32aligned_be64(&qs10->tx_packets, htonll(oqs->tx_packets));
> > + put_32aligned_be64(&qs10->tx_errors, htonll(oqs->tx_errors));
> > }
> >
> > static void
> > @@ -5517,9 +5518,9 @@ ofputil_queue_stats_to_ofp11(const struct
> > ofputil_queue_stats *oqs,
> > {
> > qs11->port_no = ofputil_port_to_ofp11(oqs->port_no);
> > qs11->queue_id = htonl(oqs->queue_id);
> > - qs11->tx_bytes = htonll(oqs->stats.tx_bytes);
> > - qs11->tx_packets = htonll(oqs->stats.tx_packets);
> > - qs11->tx_errors = htonll(oqs->stats.tx_errors);
> > + qs11->tx_bytes = htonll(oqs->tx_bytes);
> > + qs11->tx_packets = htonll(oqs->tx_packets);
> > + qs11->tx_errors = htonll(oqs->tx_errors);
> > }
> >
> > static void
> > @@ -5527,10 +5528,13 @@ ofputil_queue_stats_to_ofp13(const struct
> > ofputil_queue_stats *oqs,
> > struct ofp13_queue_stats *qs13)
> > {
> > ofputil_queue_stats_to_ofp11(oqs, &qs13->qs);
> > - /* OF 1.3 adds duration fields */
> > - /* FIXME: Need to implement queue alive duration (sec + nsec) */
> > - qs13->duration_sec = htonl(~0);
> > - qs13->duration_nsec = htonl(~0);
> > + if (oqs->duration_sec != UINT32_MAX) {
> > + qs13->duration_sec = htonl(oqs->duration_sec);
> > + qs13->duration_nsec = htonl(oqs->duration_nsec);
> > + } else {
> > + qs13->duration_sec = htonl(UINT32_MAX);
> > + qs13->duration_nsec = htonl(UINT32_MAX);
> > + }
> > }
> >
> > /* Encode a queue stat for 'oqs' and append it to 'replies'. */
> > diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> > index 0385a57..f94982d 100644
> > --- a/lib/ofp-util.h
> > +++ b/lib/ofp-util.h
> > @@ -832,7 +832,15 @@ ofputil_encode_queue_stats_request(enum ofp_version
> > ofp_version,
> > struct ofputil_queue_stats {
> > ofp_port_t port_no;
> > uint32_t queue_id;
> > - struct netdev_queue_stats stats;
> > +
> > + /* Values of unsupported statistics are set to all-1-bits
> > (UINT64_MAX). */
> > + uint64_t tx_bytes;
> > + uint64_t tx_packets;
> > + uint64_t tx_errors;
> > +
> > + /* UINT32_MAX if unknown. */
> > + uint32_t duration_sec;
> > + uint32_t duration_nsec;
> > };
> >
> > size_t ofputil_count_queue_stats(const struct ofp_header *);
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 3f4b8a7..98155ac 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -3170,18 +3170,26 @@ handle_aggregate_stats_request(struct ofconn
> > *ofconn,
> > struct queue_stats_cbdata {
> > struct ofport *ofport;
> > struct list replies;
> > + long long int now;
> > };
> >
> > static void
> > put_queue_stats(struct queue_stats_cbdata *cbdata, uint32_t queue_id,
> > const struct netdev_queue_stats *stats)
> > {
> > + struct ofputil_queue_stats oqs;
> >
> > - struct ofputil_queue_stats oqs = {
> > - .port_no = cbdata->ofport->pp.port_no,
> > - .queue_id = queue_id,
> > - .stats = *stats,
> > - };
> > + oqs.port_no = cbdata->ofport->pp.port_no;
> > + oqs.queue_id = queue_id;
> > + oqs.tx_bytes = stats->tx_bytes;
> > + oqs.tx_packets = stats->tx_packets;
> > + oqs.tx_errors = stats->tx_errors;
> > + if (stats->created != LLONG_MIN) {
> > + calc_duration(stats->created, cbdata->now,
> > + &oqs.duration_sec, &oqs.duration_nsec);
> > + } else {
> > + oqs.duration_sec = oqs.duration_nsec = UINT32_MAX;
> > + }
> > ofputil_append_queue_stat(&cbdata->replies, &oqs);
> > }
> >
> > @@ -3228,6 +3236,7 @@ handle_queue_stats_request(struct ofconn *ofconn,
> > COVERAGE_INC(ofproto_queue_req);
> >
> > ofpmp_init(&cbdata.replies, rq);
> > + cbdata.now = time_msec();
> >
> > error = ofputil_decode_queue_stats_request(rq, &oqsr);
> > if (error) {
> > diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> > index 63a89ec..986b931 100644
> > --- a/tests/ofp-print.at
> > +++ b/tests/ofp-print.at
> > @@ -1512,12 +1512,12 @@ AT_CHECK([ovs-ofctl ofp-print "\
> > 00 00 00 00 00 00 00 00 00 00 00 00 \
> > "], [0], [dnl
> > OFPST_QUEUE reply (xid=0x1): 6 queues
> > - port 3 queue 1: bytes=302, pkts=1, errors=0
> > - port 3 queue 2: bytes=0, pkts=0, errors=0
> > - port 2 queue 1: bytes=2100, pkts=20, errors=0
> > - port 2 queue 2: bytes=0, pkts=0, errors=0
> > - port 1 queue 1: bytes=0, pkts=0, errors=0
> > - port 1 queue 2: bytes=0, pkts=0, errors=0
> > + port 3 queue 1: bytes=302, pkts=1, errors=0, duration=?
> > + port 3 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > + port 2 queue 1: bytes=2100, pkts=20, errors=0, duration=?
> > + port 2 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > + port 1 queue 1: bytes=0, pkts=0, errors=0, duration=?
> > + port 1 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > ])
> > AT_CLEANUP
> >
> > @@ -1546,12 +1546,12 @@ AT_CHECK([ovs-ofctl ofp-print "\
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 \
> > "], [0], [dnl
> > OFPST_QUEUE reply (OF1.1) (xid=0x1): 6 queues
> > - port 3 queue 1: bytes=302, pkts=1, errors=0
> > - port 3 queue 2: bytes=0, pkts=0, errors=0
> > - port 2 queue 1: bytes=2100, pkts=20, errors=0
> > - port 2 queue 2: bytes=0, pkts=0, errors=0
> > - port 1 queue 1: bytes=0, pkts=0, errors=0
> > - port 1 queue 2: bytes=0, pkts=0, errors=0
> > + port 3 queue 1: bytes=302, pkts=1, errors=0, duration=?
> > + port 3 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > + port 2 queue 1: bytes=2100, pkts=20, errors=0, duration=?
> > + port 2 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > + port 1 queue 1: bytes=0, pkts=0, errors=0, duration=?
> > + port 1 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > ])
> > AT_CLEANUP
> >
> > @@ -1573,12 +1573,14 @@ AT_CHECK([ovs-ofctl ofp-print "\
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 \
> > "], [0], [dnl
> > OFPST_QUEUE reply (OF1.2) (xid=0x1): 6 queues
> > - port 3 queue 1: bytes=302, pkts=1, errors=0
> > - port 3 queue 2: bytes=0, pkts=0, errors=0
> > - port 2 queue 1: bytes=2100, pkts=20, errors=0
> > - port 2 queue 2: bytes=0, pkts=0, errors=0
> > - port 1 queue 1: bytes=0, pkts=0, errors=0
> > - port 1 queue 2: bytes=0, pkts=0, errors=0
> > + port 3 queue 1: bytes=302, pkts=1, errors=0, duration=?
> > + port 3 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > + port 2 queue 1: bytes=2100, pkts=20, errors=0, duration=?
> > + port 2 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > + port 1 queue 1: bytes=0, pkts=0, errors=0, duration=?
> > + port 1 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > +])
> > +AT_CLEANUP
> >
> > AT_SETUP([OFPST_QUEUE reply - OF1.3])
> > AT_KEYWORDS([ofp-print OFPT_STATS_REPLY])
> > @@ -1604,12 +1606,12 @@ AT_CHECK([ovs-ofctl ofp-print "\
> > ff ff ff ff ff ff ff ff \
> > "], [0], [dnl
> > OFPST_QUEUE reply (OF1.3) (xid=0x1): 6 queues
> > - port 3 queue 1: bytes=302, pkts=1, errors=0
> > - port 3 queue 2: bytes=0, pkts=0, errors=0
> > - port 2 queue 1: bytes=2100, pkts=20, errors=0
> > - port 2 queue 2: bytes=0, pkts=0, errors=0
> > - port 1 queue 1: bytes=0, pkts=0, errors=0
> > - port 1 queue 2: bytes=0, pkts=0, errors=0
> > + port 3 queue 1: bytes=302, pkts=1, errors=0, duration=100.5s
> > + port 3 queue 2: bytes=0, pkts=0, errors=0, duration=100.5s
> > + port 2 queue 1: bytes=2100, pkts=20, errors=0, duration=100.5s
> > + port 2 queue 2: bytes=0, pkts=0, errors=0, duration=100.5s
> > + port 1 queue 1: bytes=0, pkts=0, errors=0, duration=100.5s
> > + port 1 queue 2: bytes=0, pkts=0, errors=0, duration=?
> > ])
> > AT_CLEANUP
> >
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
More information about the dev
mailing list