On 9/7/2016 3:07 PM, Tvrtko Ursulin wrote:
On 07/09/16 09:44, Chris Wilson wrote:
On Wed, Sep 07, 2016 at 01:40:27PM +0530, Goel, Akash wrote:
On 9/6/2016 9:22 PM, Tvrtko Ursulin wrote:
[snip]
+ while (!stop_logging)
+ {
+ if (test_duration && (igt_seconds_elapsed(&start) >
test_duration)) {
If you agree to allow no poll period the this would not work
right? In
that case you would need to use alarm(2) or something.
Can calculate the timeout value for poll call as,
if (poll_timeout < 0) {
timeout = test_duration - igt_seconds_elapsed(&start))
}
My point was that with indefinite poll loop will not run if there is
not
log data so timeout will not work implemented like this.
I understood your concern in first place but probably didn't put
forth my point clearly.
For more clarity, this is how think it can be addressed.
--- a/tools/intel_guc_logger.c
+++ b/tools/intel_guc_logger.c
@@ -370,6 +370,8 @@ int main(int argc, char **argv)
{
struct pollfd relay_poll_fd;
struct timespec start={};
+ uint32_t time_elapsed;
+ int timeout;
int nfds;
int ret;
@@ -395,10 +397,17 @@ int main(int argc, char **argv)
while (!stop_logging)
{
- if (test_duration && (igt_seconds_elapsed(&start) >
test_duration)) {
- igt_debug("Ran for stipulated %d seconds, exit now\n",
test_duration);
- stop_logging = true;
- break;
+ timeout = poll_timeout;
+ if (test_duration) {
+ time_elapsed = igt_seconds_elapsed(&start);
+ if (time_elapsed >= test_duration) {
+ igt_debug("Ran for stipulated %d seconds, exit
now\n", test_duration);
+ stop_logging = true;
+ break;
+ }
+ if (poll_timeout < 0)
+ timeout = (test_duration - time_elapsed) * 1000;
}
/* Wait/poll for the new data to be available, relay doesn't
@@ -412,7 +421,7 @@ int main(int argc, char **argv)
* than a jiffy gap between 2 flush interrupts) and relay runs
* out of sub buffers to store the new logs.
*/
- ret = poll(&relay_poll_fd, nfds, poll_timeout);
+ ret = poll(&relay_poll_fd, nfds, timeout);
if (ret < 0) {
if (errno == EINTR)
break;
So will not do polling with indefinite timeout and adjust the
timeout value as per test's duration.
Does it look ok ?
Since the comment still refers to a kernel bug that you've fixed, it can
just go. The timeout calculation is indeed more simply expressed as
alarm(timeout).
Yes I wrote privately that's especially true since there is already a
handler for SIGINT which would do the right thing for SIGALRM as well. I
don't feel so strongly about this but now that we both think the same
maybe go for the simpler implementation if you don't mind Akash?
Thanks much for suggestion.
Will use 'alarm(timeout)', its definitely much simpler.
And fixing the blocking read() is about 10 lines in the kernel...
Haven't checked but if that is the case, since we are already fixing
relayfs issues, it would be good to do that one as well since it would
simplify the logger. Because if we do it straight away then we know
logger can use it, and if we leave it for later then it gets uglier for
the logger.
But if we cannot make the fix go in the same kernel version (or earlier)
than the GuC logging then I think we don't need to block on that.
Sorry not sure that whether we would gain much by trying to add the
support for blocking read in relay.
For a regular disk file, which is of a fixed size, it makes sense to
have a provision to block the reader until file's data is paged in from
the disk into RAM.
But for relay, data to be read would invariably be generated dynamically
which can stop at anytime and thus the reader could get blocked for ever.
I think the current relay semantics are fine that if there is no data
left to be read in channel buffers zero will be returned and Clients
can get to know about the generation of new data through poll (using a
timeout).
Best regards
Akash
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
