| Message ID | 20210729142239.2022546-1-mark.d.gray@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ovs-dev] rconn: Add additional coverage counters rconn_run and rconn_retry | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
On 7/29/21 4:22 PM, Mark Gray wrote: > Add counters to measure the number of times rconn_run() is > executed and the number of times rconn_send() must retry > due to an overflow at the vconn layer. > > These counters allow for more effective debugging of buffer > overflows from rconn_send(). > > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- Looks good to me, thanks! Acked-by: Dumitru Ceara <dceara@redhat.com>
On 7/29/21 16:22, Mark Gray wrote: > Add counters to measure the number of times rconn_run() is > executed and the number of times rconn_send() must retry > due to an overflow at the vconn layer. > > These counters allow for more effective debugging of buffer > overflows from rconn_send(). > > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- > lib/rconn.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/lib/rconn.c b/lib/rconn.c > index a96b2eb8bf43..e635a7ff6284 100644 > --- a/lib/rconn.c > +++ b/lib/rconn.c > @@ -40,6 +40,8 @@ COVERAGE_DEFINE(rconn_discarded); > COVERAGE_DEFINE(rconn_overflow); > COVERAGE_DEFINE(rconn_queued); > COVERAGE_DEFINE(rconn_sent); > +COVERAGE_DEFINE(rconn_run); > +COVERAGE_DEFINE(rconn_retry); > > /* The connection states have the following meanings: > * > @@ -624,6 +626,8 @@ rconn_run(struct rconn *rc) > int old_state; > size_t i; > > + COVERAGE_INC(rconn_run); Hi, Mark. I'm not sure what is a value of counting this? rconn_run() supposed to be called unconditionally and it doesn't seem very useful to count these calls. What do you think? > + > ovs_mutex_lock(&rc->mutex); > if (rc->vconn) { > int error; > @@ -1132,6 +1136,8 @@ try_send(struct rconn *rc) > if (retval != EAGAIN) { > report_error(rc, retval); > disconnect(rc, retval); > + } else { > + COVERAGE_INC(rconn_retry); It might be better to rename to something like 'rconn_send_retry'. > } > return retval; > } >
On 15/09/2021 22:49, Ilya Maximets wrote: > On 7/29/21 16:22, Mark Gray wrote: >> Add counters to measure the number of times rconn_run() is >> executed and the number of times rconn_send() must retry >> due to an overflow at the vconn layer. >> >> These counters allow for more effective debugging of buffer >> overflows from rconn_send(). >> >> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >> --- >> lib/rconn.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/lib/rconn.c b/lib/rconn.c >> index a96b2eb8bf43..e635a7ff6284 100644 >> --- a/lib/rconn.c >> +++ b/lib/rconn.c >> @@ -40,6 +40,8 @@ COVERAGE_DEFINE(rconn_discarded); >> COVERAGE_DEFINE(rconn_overflow); >> COVERAGE_DEFINE(rconn_queued); >> COVERAGE_DEFINE(rconn_sent); >> +COVERAGE_DEFINE(rconn_run); >> +COVERAGE_DEFINE(rconn_retry); >> >> /* The connection states have the following meanings: >> * >> @@ -624,6 +626,8 @@ rconn_run(struct rconn *rc) >> int old_state; >> size_t i; >> >> + COVERAGE_INC(rconn_run); > > Hi, Mark. I'm not sure what is a value of counting this? > rconn_run() supposed to be called unconditionally and it > doesn't seem very useful to count these calls. > > What do you think? > I needed it because I was trying to understand the ratio of the number of rconn_retry() calls to the number of rconn_run() calls. I suppose (depending on how rconn was being used) you may be able determine this from another counter but it just seemed more convenient like this. If you think this is not valuable, I can leave it out. >> + >> ovs_mutex_lock(&rc->mutex); >> if (rc->vconn) { >> int error; >> @@ -1132,6 +1136,8 @@ try_send(struct rconn *rc) >> if (retval != EAGAIN) { >> report_error(rc, retval); >> disconnect(rc, retval); >> + } else { >> + COVERAGE_INC(rconn_retry); > > It might be better to rename to something like 'rconn_send_retry'. Ok. I will rename this when I get a reply to above. > >> } >> return retval; >> } >> >
diff --git a/lib/rconn.c b/lib/rconn.c index a96b2eb8bf43..e635a7ff6284 100644 --- a/lib/rconn.c +++ b/lib/rconn.c @@ -40,6 +40,8 @@ COVERAGE_DEFINE(rconn_discarded); COVERAGE_DEFINE(rconn_overflow); COVERAGE_DEFINE(rconn_queued); COVERAGE_DEFINE(rconn_sent); +COVERAGE_DEFINE(rconn_run); +COVERAGE_DEFINE(rconn_retry); /* The connection states have the following meanings: * @@ -624,6 +626,8 @@ rconn_run(struct rconn *rc) int old_state; size_t i; + COVERAGE_INC(rconn_run); + ovs_mutex_lock(&rc->mutex); if (rc->vconn) { int error; @@ -1132,6 +1136,8 @@ try_send(struct rconn *rc) if (retval != EAGAIN) { report_error(rc, retval); disconnect(rc, retval); + } else { + COVERAGE_INC(rconn_retry); } return retval; }
Add counters to measure the number of times rconn_run() is executed and the number of times rconn_send() must retry due to an overflow at the vconn layer. These counters allow for more effective debugging of buffer overflows from rconn_send(). Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- lib/rconn.c | 6 ++++++ 1 file changed, 6 insertions(+)