Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

examples: ephiperfifo: weird timerfd_settime call #3632

Closed
elboulangero opened this issue Mar 1, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@elboulangero
Copy link
Contributor

commented Mar 1, 2019

static int multi_timer_cb(CURLM *multi, long timeout_ms, GlobalInfo *g)
{
struct itimerspec its;
CURLMcode rc;
fprintf(MSG_OUT, "multi_timer_cb: Setting timeout to %ld ms\n", timeout_ms);
timerfd_settime(g->tfd, /*flags=*/0, &its, NULL);

It looks to me like its is not initialized at this point. I would even say that this line is here by mistake?

@cheshirekow should know in a glance!

@cheshirekow

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

My immediate impression is that there's a missing Zero-init of its. I think it's supposed to be covering the default case. I will investigate further and try to recall.

@elboulangero

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

I see that just below you actually test the value of timeout and set its accordingly, before calling timerfd_settime. So it seems to me that this line 156 is not wanted, but then again I'm not familiar with libcurl yet, so maybe I'm missing something.

@cheshirekow

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Yeah it looks fishy. I think you might be right. I think it might be leftovers of some code that was moved into the else block. I feel like the timer should be disarmed in the ==0 case too though...

@cheshirekow

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

I seem to recall having to iterate on this function a few times. I think there was some trickiness in here where I didn't fully understand the semantics of timeout_ms==0. Based on the description of CURLMOPT_TIMERFUNCTION

A timeout_ms value of -1 passed to this callback means you should delete the timer. All other values are valid expire times in number of milliseconds.

I think it's a little ambiguous what to do in a timout_ms == 0 case. I'm pretty sure we need to service curl_multi_socket_action in this case. Perhaps we also want to disarm the timer. So should we disarm if timeout_ms <= 0 instead of just < 0? It's not a big deal either way, we'll just service the curl_multi_socket_action one time more than it wanted to in the worst case.

Anyhow, I think that yes, line 156 should be removed.

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

In the timout_ms == 0 case, that's a valid timer expiry meaning "now". But unless the event system needs it, there's no need to special-case the zero. In fact, I've updated other examples to avoid the recursive call back into libcurl from within the callback itself so I would prefer if we could rewrite the callback (including your above mentioned bug-fix) to look like this:

/* Update the timer after curl_multi library does it's thing. Curl will
 * inform us through this callback what it wants the new timeout to be,
 * after it does some work. */
static int multi_timer_cb(CURLM *multi, long timeout_ms, GlobalInfo *g)
{
  struct itimerspec its;
  CURLMcode rc;

  fprintf(MSG_OUT, "multi_timer_cb: Setting timeout to %ld ms\n", timeout_ms);

  if(timeout_ms >= 0) {
    its.it_interval.tv_sec = 1;
    its.it_interval.tv_nsec = 0;
    its.it_value.tv_sec = timeout_ms / 1000;
    its.it_value.tv_nsec = (timeout_ms % 1000) * 1000;
    timerfd_settime(g->tfd, /*flags=*/0, &its, NULL);
  }
  else {
    memset(&its, 0, sizeof(struct itimerspec));
    timerfd_settime(g->tfd, /*flags=*/0, &its, NULL);
  }
  return 0;
}

@bagder bagder added the documentation label Mar 1, 2019

@elboulangero

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

But unless the event system needs it, there's no need to special-case the zero.

Actually you need a special case here, because from timerfd_create(2): Setting both fields of new_value.it_value to zero disarms the timer.

One possible way is to to set its.it_value.tv_nsec = 1, i've seen that in systemd event loop for example:

https://github.com/systemd/systemd/blob/8e4fbe3f2dbbe1a358976316057e52145e102b64/src/libsystemd/sd-event/sd-event.c#L2990-L2997

On the other hand, libevent handles it as a special case:

https://github.com/libevent/libevent/blob/master/epoll.c#L434-L438

elboulangero added a commit to elboulangero/curl that referenced this issue Mar 1, 2019

example: various fixes in ephiperfifo.c
The main change here is the timer value that was wrong, it was given in
usecs (ms * 1000), while the itimerspec struct wants nsecs
(ms * 1000 * 1000). This resulted in the callback being invoked WAY TOO
OFTEN.

As a quick check you can run this command before and after applying this
commit:

    # shell 1
    ./ephiperfifo 2>&1 | tee ephiperfifio.log
    # shell 2
    echo http://hacking.elboulangero.com > hiper.fifo

Then just compare the size of the logs files.

Closes: curl#3632
Signed-off-by: Arnaud Rebillout <arnaud.rebillout@collabora.com>

elboulangero added a commit to elboulangero/curl that referenced this issue Mar 1, 2019

example: various fixes in ephiperfifo.c
The main change here is the timer value that was wrong, it was given in
usecs (ms * 1000), while the itimerspec struct wants nsecs
(ms * 1000 * 1000). This resulted in the callback being invoked WAY TOO
OFTEN.

As a quick check you can run this command before and after applying this
commit:

    # shell 1
    ./ephiperfifo 2>&1 | tee ephiperfifo.log
    # shell 2
    echo http://hacking.elboulangero.com > hiper.fifo

Then just compare the size of the logs files.

Closes: curl#3632
Signed-off-by: Arnaud Rebillout <arnaud.rebillout@collabora.com>

elboulangero added a commit to elboulangero/curl that referenced this issue Mar 1, 2019

examples: various fixes in ephiperfifo.c
The main change here is the timer value that was wrong, it was given in
usecs (ms * 1000), while the itimerspec struct wants nsecs
(ms * 1000 * 1000). This resulted in the callback being invoked WAY TOO
OFTEN.

As a quick check you can run this command before and after applying this
commit:

    # shell 1
    ./ephiperfifo 2>&1 | tee ephiperfifo.log
    # shell 2
    echo http://hacking.elboulangero.com > hiper.fifo

Then just compare the size of the logs files.

Closes: curl#3632
Signed-off-by: Arnaud Rebillout <arnaud.rebillout@collabora.com>

@bagder bagder closed this in a977d93 Mar 1, 2019

@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.