Message ID | 1504115149-26864-3-git-send-email-ktraynor@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,1/3] dpif-netdev: Rename rxq_interval. | expand |
Thanks for the patch Kevin
On 8/30/17, 10:45 AM, "Kevin Traynor" <ktraynor@redhat.com> wrote:
rxq_cycle_sort summed the latest cycles from each queue for sorting.
While each comparison was correct with the latest cycles, the cycles
could change between calls to rxq_cycle_sort. In order to use
consistent values through each call to rxq_cycle_sort, sum the cycles
prior to rxq_cycle_sort being called.
As discussed, these changes are optional and have some tradeoffs, but
overall, I don’t see any major issue introduced here, since this is understood to be a
rough comparison anyways.
Also, change return to 0 when values are equal.
As discussed, this means the equal tie-breaker is done by qsort instead of
the compare function; the net practical effect of this is nil, but this is more
standard.
Reported-by: Ilya Maximets <i.maximets@samsung.com>
Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
lib/dpif-netdev.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1db9f10..7c21ee5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3432,18 +3432,14 @@ rxq_cycle_sort(const void *a, const void *b)
struct dp_netdev_rxq *qb;
uint64_t total_qa, total_qb;
- unsigned i;
qa = *(struct dp_netdev_rxq **) a;
qb = *(struct dp_netdev_rxq **) b;
- total_qa = total_qb = 0;
- for (i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) {
- total_qa += dp_netdev_rxq_get_intrvl_cycles(qa, i);
- total_qb += dp_netdev_rxq_get_intrvl_cycles(qb, i);
- }
- dp_netdev_rxq_set_cycles(qa, RXQ_CYCLES_PROC_HIST, total_qa);
- dp_netdev_rxq_set_cycles(qb, RXQ_CYCLES_PROC_HIST, total_qb);
+ total_qa = dp_netdev_rxq_get_cycles(qa, RXQ_CYCLES_PROC_HIST);
+ total_qb = dp_netdev_rxq_get_cycles(qb, RXQ_CYCLES_PROC_HIST);
- if (total_qa >= total_qb) {
+ if (total_qa == total_qb) {
+ return 0;
+ } else if (total_qa > total_qb) {
return -1;
}
@@ -3493,4 +3489,6 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) OVS_REQUIRES(dp->port_mutex)
}
} else if (!pinned && q->core_id == OVS_CORE_UNSPEC) {
+ uint64_t cycle_hist = 0;
+
if (n_rxqs == 0) {
rxqs = xmalloc(sizeof *rxqs);
@@ -3498,4 +3496,10 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) OVS_REQUIRES(dp->port_mutex)
rxqs = xrealloc(rxqs, sizeof *rxqs * (n_rxqs + 1));
}
+
+ for (unsigned i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) {
+ cycle_hist += dp_netdev_rxq_get_intrvl_cycles(q, i);
+ }
+ dp_netdev_rxq_set_cycles(q, RXQ_CYCLES_PROC_HIST, cycle_hist);
+
/* Store the queue. */
rxqs[n_rxqs++] = q;
--
1.8.3.1
Are there any other comments for this patch?
On 8/30/17, 11:01 AM, "Darrell Ball" <dball@vmware.com> wrote:
Thanks for the patch Kevin
On 8/30/17, 10:45 AM, "Kevin Traynor" <ktraynor@redhat.com> wrote:
rxq_cycle_sort summed the latest cycles from each queue for sorting.
While each comparison was correct with the latest cycles, the cycles
could change between calls to rxq_cycle_sort. In order to use
consistent values through each call to rxq_cycle_sort, sum the cycles
prior to rxq_cycle_sort being called.
As discussed, these changes are optional and have some tradeoffs, but
overall, I don’t see any major issue introduced here, since this is understood to be a
rough comparison anyways.
Also, change return to 0 when values are equal.
As discussed, this means the equal tie-breaker is done by qsort instead of
the compare function; the net practical effect of this is nil, but this is more
standard.
Reported-by: Ilya Maximets <i.maximets@samsung.com>
Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
lib/dpif-netdev.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1db9f10..7c21ee5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3432,18 +3432,14 @@ rxq_cycle_sort(const void *a, const void *b)
struct dp_netdev_rxq *qb;
uint64_t total_qa, total_qb;
- unsigned i;
qa = *(struct dp_netdev_rxq **) a;
qb = *(struct dp_netdev_rxq **) b;
- total_qa = total_qb = 0;
- for (i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) {
- total_qa += dp_netdev_rxq_get_intrvl_cycles(qa, i);
- total_qb += dp_netdev_rxq_get_intrvl_cycles(qb, i);
- }
- dp_netdev_rxq_set_cycles(qa, RXQ_CYCLES_PROC_HIST, total_qa);
- dp_netdev_rxq_set_cycles(qb, RXQ_CYCLES_PROC_HIST, total_qb);
+ total_qa = dp_netdev_rxq_get_cycles(qa, RXQ_CYCLES_PROC_HIST);
+ total_qb = dp_netdev_rxq_get_cycles(qb, RXQ_CYCLES_PROC_HIST);
- if (total_qa >= total_qb) {
+ if (total_qa == total_qb) {
+ return 0;
+ } else if (total_qa > total_qb) {
return -1;
}
@@ -3493,4 +3489,6 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) OVS_REQUIRES(dp->port_mutex)
}
} else if (!pinned && q->core_id == OVS_CORE_UNSPEC) {
+ uint64_t cycle_hist = 0;
+
if (n_rxqs == 0) {
rxqs = xmalloc(sizeof *rxqs);
@@ -3498,4 +3496,10 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) OVS_REQUIRES(dp->port_mutex)
rxqs = xrealloc(rxqs, sizeof *rxqs * (n_rxqs + 1));
}
+
+ for (unsigned i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) {
+ cycle_hist += dp_netdev_rxq_get_intrvl_cycles(q, i);
+ }
+ dp_netdev_rxq_set_cycles(q, RXQ_CYCLES_PROC_HIST, cycle_hist);
+
/* Store the queue. */
rxqs[n_rxqs++] = q;
--
1.8.3.1
Ping again. This was requested to make the sort function behave in a more standard manner. I don't think there's anything controversial in it. thanks, Kevin. On 09/22/2017 08:21 PM, Darrell Ball wrote: > Are there any other comments for this patch? > > > On 8/30/17, 11:01 AM, "Darrell Ball" <dball@vmware.com> wrote: > > Thanks for the patch Kevin > > On 8/30/17, 10:45 AM, "Kevin Traynor" <ktraynor@redhat.com> wrote: > > rxq_cycle_sort summed the latest cycles from each queue for sorting. > While each comparison was correct with the latest cycles, the cycles > could change between calls to rxq_cycle_sort. In order to use > consistent values through each call to rxq_cycle_sort, sum the cycles > prior to rxq_cycle_sort being called. > > As discussed, these changes are optional and have some tradeoffs, but > overall, I don’t see any major issue introduced here, since this is understood to be a > rough comparison anyways. > > Also, change return to 0 when values are equal. > > As discussed, this means the equal tie-breaker is done by qsort instead of > the compare function; the net practical effect of this is nil, but this is more > standard. > > Reported-by: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > --- > lib/dpif-netdev.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 1db9f10..7c21ee5 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -3432,18 +3432,14 @@ rxq_cycle_sort(const void *a, const void *b) > struct dp_netdev_rxq *qb; > uint64_t total_qa, total_qb; > - unsigned i; > > qa = *(struct dp_netdev_rxq **) a; > qb = *(struct dp_netdev_rxq **) b; > > - total_qa = total_qb = 0; > - for (i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) { > - total_qa += dp_netdev_rxq_get_intrvl_cycles(qa, i); > - total_qb += dp_netdev_rxq_get_intrvl_cycles(qb, i); > - } > - dp_netdev_rxq_set_cycles(qa, RXQ_CYCLES_PROC_HIST, total_qa); > - dp_netdev_rxq_set_cycles(qb, RXQ_CYCLES_PROC_HIST, total_qb); > + total_qa = dp_netdev_rxq_get_cycles(qa, RXQ_CYCLES_PROC_HIST); > + total_qb = dp_netdev_rxq_get_cycles(qb, RXQ_CYCLES_PROC_HIST); > > - if (total_qa >= total_qb) { > + if (total_qa == total_qb) { > + return 0; > + } else if (total_qa > total_qb) { > return -1; > } > @@ -3493,4 +3489,6 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) OVS_REQUIRES(dp->port_mutex) > } > } else if (!pinned && q->core_id == OVS_CORE_UNSPEC) { > + uint64_t cycle_hist = 0; > + > if (n_rxqs == 0) { > rxqs = xmalloc(sizeof *rxqs); > @@ -3498,4 +3496,10 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) OVS_REQUIRES(dp->port_mutex) > rxqs = xrealloc(rxqs, sizeof *rxqs * (n_rxqs + 1)); > } > + > + for (unsigned i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) { > + cycle_hist += dp_netdev_rxq_get_intrvl_cycles(q, i); > + } > + dp_netdev_rxq_set_cycles(q, RXQ_CYCLES_PROC_HIST, cycle_hist); > + > /* Store the queue. */ > rxqs[n_rxqs++] = q; > -- > 1.8.3.1 > > > > >
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 1db9f10..7c21ee5 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3432,18 +3432,14 @@ rxq_cycle_sort(const void *a, const void *b) struct dp_netdev_rxq *qb; uint64_t total_qa, total_qb; - unsigned i; qa = *(struct dp_netdev_rxq **) a; qb = *(struct dp_netdev_rxq **) b; - total_qa = total_qb = 0; - for (i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) { - total_qa += dp_netdev_rxq_get_intrvl_cycles(qa, i); - total_qb += dp_netdev_rxq_get_intrvl_cycles(qb, i); - } - dp_netdev_rxq_set_cycles(qa, RXQ_CYCLES_PROC_HIST, total_qa); - dp_netdev_rxq_set_cycles(qb, RXQ_CYCLES_PROC_HIST, total_qb); + total_qa = dp_netdev_rxq_get_cycles(qa, RXQ_CYCLES_PROC_HIST); + total_qb = dp_netdev_rxq_get_cycles(qb, RXQ_CYCLES_PROC_HIST); - if (total_qa >= total_qb) { + if (total_qa == total_qb) { + return 0; + } else if (total_qa > total_qb) { return -1; } @@ -3493,4 +3489,6 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) OVS_REQUIRES(dp->port_mutex) } } else if (!pinned && q->core_id == OVS_CORE_UNSPEC) { + uint64_t cycle_hist = 0; + if (n_rxqs == 0) { rxqs = xmalloc(sizeof *rxqs); @@ -3498,4 +3496,10 @@ rxq_scheduling(struct dp_netdev *dp, bool pinned) OVS_REQUIRES(dp->port_mutex) rxqs = xrealloc(rxqs, sizeof *rxqs * (n_rxqs + 1)); } + + for (unsigned i = 0; i < PMD_RXQ_INTERVAL_MAX; i++) { + cycle_hist += dp_netdev_rxq_get_intrvl_cycles(q, i); + } + dp_netdev_rxq_set_cycles(q, RXQ_CYCLES_PROC_HIST, cycle_hist); + /* Store the queue. */ rxqs[n_rxqs++] = q;
rxq_cycle_sort summed the latest cycles from each queue for sorting. While each comparison was correct with the latest cycles, the cycles could change between calls to rxq_cycle_sort. In order to use consistent values through each call to rxq_cycle_sort, sum the cycles prior to rxq_cycle_sort being called. Also, change return to 0 when values are equal. Reported-by: Ilya Maximets <i.maximets@samsung.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- lib/dpif-netdev.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)