Re: [ovs-dev] [PATCH] Optimize the poll loop for poll_immediate_wake()
I'm impressed by the improvement. I didn't really expect any.
On Tue, Jul 13, 2021 at 09:19:34AM +0100, Anton Ivanov wrote:
> I ran the revised patch series (v2) which addresses your comments on the
> ovn-heater benchmark.
>
> With 9 fake nodes, 50 fake pods per node I get ~ 2% reproducible improvement.
> IMHO it should be even more noticeable at high scale.
>
> Best regards,
>
> A
>
> On 09/07/2021 19:45, Ben Pfaff wrote:
> > On Fri, Jul 09, 2021 at 06:19:06PM +0100, anton.iva...@cambridgegreys.com
> > wrote:
> > > From: Anton Ivanov
> > >
> > > If we are not obtaining any useful information out of the poll(),
> > > such as is a fd busy or not, we do not need to do a poll() if
> > > an immediate_wake() has been requested.
> > >
> > > This cuts out all the pollfd hash additions, forming the poll
> > > arguments and the actual poll() after a call to
> > > poll_immediate_wake()
> > >
> > > Signed-off-by: Anton Ivanov
> > Thanks for the patch.
> >
> > I think that this will have some undesirable side effects because it
> > avoids calling time_poll() if the wakeup should happen immediately, and
> > time_poll() does some thing that we always want to happen between one
> > main loop and another. In particular the following calls from
> > time_poll() are important:
> >
> > coverage_clear();
> > coverage_run();
> > if (*last_wakeup && !thread_is_pmd()) {
> > log_poll_interval(*last_wakeup);
> > }
> >
> > ...
> >
> > if (!time_left) {
> > ovsrcu_quiesce();
> > } else {
> > ovsrcu_quiesce_start();
> > }
> >
> > ...
> >
> > if (deadline <= time_msec()) {
> > #ifndef _WIN32
> > fatal_signal_handler(SIGALRM);
> > #else
> > VLOG_ERR("wake up from WaitForMultipleObjects after deadline");
> > fatal_signal_handler(SIGTERM);
> > #endif
> > if (retval < 0) {
> > retval = 0;
> > }
> > break;
> > }
> >
> > ...
> >
> > *last_wakeup = time_msec();
> > refresh_rusage();
> >
> > Instead of this change, I'd suggest something more like the following,
> > along with the changes you made to suppress the file descriptors if the
> > timeout is already zero:
> >
> > diff --git a/lib/timeval.c b/lib/timeval.c
> > index 193c7bab1781..f080a742 100644
> > --- a/lib/timeval.c
> > +++ b/lib/timeval.c
> > @@ -323,7 +323,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE
> > *handles OVS_UNUSED,
> > }
> > #ifndef _WIN32
> > -retval = poll(pollfds, n_pollfds, time_left);
> > +retval = time_left ? poll(pollfds, n_pollfds, time_left) : 0;
> > if (retval < 0) {
> > retval = -errno;
> > }
> >
> >
> --
> Anton R. Ivanov
> Cambridgegreys Limited. Registered in England. Company Number 10273661
> https://www.cambridgegreys.com/
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Optimize the poll loop for poll_immediate_wake()
I ran the revised patch series (v2) which addresses your comments on the
ovn-heater benchmark.
With 9 fake nodes, 50 fake pods per node I get ~ 2% reproducible improvement.
IMHO it should be even more noticeable at high scale.
Best regards,
A
On 09/07/2021 19:45, Ben Pfaff wrote:
On Fri, Jul 09, 2021 at 06:19:06PM +0100, anton.iva...@cambridgegreys.com wrote:
From: Anton Ivanov
If we are not obtaining any useful information out of the poll(),
such as is a fd busy or not, we do not need to do a poll() if
an immediate_wake() has been requested.
This cuts out all the pollfd hash additions, forming the poll
arguments and the actual poll() after a call to
poll_immediate_wake()
Signed-off-by: Anton Ivanov
Thanks for the patch.
I think that this will have some undesirable side effects because it
avoids calling time_poll() if the wakeup should happen immediately, and
time_poll() does some thing that we always want to happen between one
main loop and another. In particular the following calls from
time_poll() are important:
coverage_clear();
coverage_run();
if (*last_wakeup && !thread_is_pmd()) {
log_poll_interval(*last_wakeup);
}
...
if (!time_left) {
ovsrcu_quiesce();
} else {
ovsrcu_quiesce_start();
}
...
if (deadline <= time_msec()) {
#ifndef _WIN32
fatal_signal_handler(SIGALRM);
#else
VLOG_ERR("wake up from WaitForMultipleObjects after deadline");
fatal_signal_handler(SIGTERM);
#endif
if (retval < 0) {
retval = 0;
}
break;
}
...
*last_wakeup = time_msec();
refresh_rusage();
Instead of this change, I'd suggest something more like the following,
along with the changes you made to suppress the file descriptors if the
timeout is already zero:
diff --git a/lib/timeval.c b/lib/timeval.c
index 193c7bab1781..f080a742 100644
--- a/lib/timeval.c
+++ b/lib/timeval.c
@@ -323,7 +323,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE
*handles OVS_UNUSED,
}
#ifndef _WIN32
-retval = poll(pollfds, n_pollfds, time_left);
+retval = time_left ? poll(pollfds, n_pollfds, time_left) : 0;
if (retval < 0) {
retval = -errno;
}
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Optimize the poll loop for poll_immediate_wake()
Good point,
I forgot about the rcu.
I will redo the patch as per your suggestions and resubmit it next week.
A.
On 09/07/2021 19:45, Ben Pfaff wrote:
On Fri, Jul 09, 2021 at 06:19:06PM +0100, anton.iva...@cambridgegreys.com wrote:
From: Anton Ivanov
If we are not obtaining any useful information out of the poll(),
such as is a fd busy or not, we do not need to do a poll() if
an immediate_wake() has been requested.
This cuts out all the pollfd hash additions, forming the poll
arguments and the actual poll() after a call to
poll_immediate_wake()
Signed-off-by: Anton Ivanov
Thanks for the patch.
I think that this will have some undesirable side effects because it
avoids calling time_poll() if the wakeup should happen immediately, and
time_poll() does some thing that we always want to happen between one
main loop and another. In particular the following calls from
time_poll() are important:
coverage_clear();
coverage_run();
if (*last_wakeup && !thread_is_pmd()) {
log_poll_interval(*last_wakeup);
}
...
if (!time_left) {
ovsrcu_quiesce();
} else {
ovsrcu_quiesce_start();
}
...
if (deadline <= time_msec()) {
#ifndef _WIN32
fatal_signal_handler(SIGALRM);
#else
VLOG_ERR("wake up from WaitForMultipleObjects after deadline");
fatal_signal_handler(SIGTERM);
#endif
if (retval < 0) {
retval = 0;
}
break;
}
...
*last_wakeup = time_msec();
refresh_rusage();
Instead of this change, I'd suggest something more like the following,
along with the changes you made to suppress the file descriptors if the
timeout is already zero:
diff --git a/lib/timeval.c b/lib/timeval.c
index 193c7bab1781..f080a742 100644
--- a/lib/timeval.c
+++ b/lib/timeval.c
@@ -323,7 +323,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE
*handles OVS_UNUSED,
}
#ifndef _WIN32
-retval = poll(pollfds, n_pollfds, time_left);
+retval = time_left ? poll(pollfds, n_pollfds, time_left) : 0;
if (retval < 0) {
retval = -errno;
}
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Optimize the poll loop for poll_immediate_wake()
On Fri, Jul 09, 2021 at 06:19:06PM +0100, anton.iva...@cambridgegreys.com wrote:
> From: Anton Ivanov
>
> If we are not obtaining any useful information out of the poll(),
> such as is a fd busy or not, we do not need to do a poll() if
> an immediate_wake() has been requested.
>
> This cuts out all the pollfd hash additions, forming the poll
> arguments and the actual poll() after a call to
> poll_immediate_wake()
>
> Signed-off-by: Anton Ivanov
Thanks for the patch.
I think that this will have some undesirable side effects because it
avoids calling time_poll() if the wakeup should happen immediately, and
time_poll() does some thing that we always want to happen between one
main loop and another. In particular the following calls from
time_poll() are important:
coverage_clear();
coverage_run();
if (*last_wakeup && !thread_is_pmd()) {
log_poll_interval(*last_wakeup);
}
...
if (!time_left) {
ovsrcu_quiesce();
} else {
ovsrcu_quiesce_start();
}
...
if (deadline <= time_msec()) {
#ifndef _WIN32
fatal_signal_handler(SIGALRM);
#else
VLOG_ERR("wake up from WaitForMultipleObjects after deadline");
fatal_signal_handler(SIGTERM);
#endif
if (retval < 0) {
retval = 0;
}
break;
}
...
*last_wakeup = time_msec();
refresh_rusage();
Instead of this change, I'd suggest something more like the following,
along with the changes you made to suppress the file descriptors if the
timeout is already zero:
diff --git a/lib/timeval.c b/lib/timeval.c
index 193c7bab1781..f080a742 100644
--- a/lib/timeval.c
+++ b/lib/timeval.c
@@ -323,7 +323,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE
*handles OVS_UNUSED,
}
#ifndef _WIN32
-retval = poll(pollfds, n_pollfds, time_left);
+retval = time_left ? poll(pollfds, n_pollfds, time_left) : 0;
if (retval < 0) {
retval = -errno;
}
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
