Welcome! Log In Create A New Profile

Advanced

[PATCH] BUG/MINOR, lua/sockets, make lua tasks that are waiting for io suspend until woken up by the a corresponding event.

Posted by PiBa-NL 
Hi List, WiIly, Thierry, Emeric,

Tried a little patch for my 100% cpu usage issue.
https://www.mail-archive.com/[email protected]/msg29762.html

It stops the cpu usage reported in above thread.. Just wondering if
there are any culprits that might now 'hang' a lua applet instead.?.

I think this issue was introduced here (haven't tried to bisect it..):
http://git.haproxy.org/?p=haproxy.git;a=commitdiff;h=253e53e661c49fb9723535319cf511152bf09bc7

Possibly due to some TICK_ETERNITY that shouldn't actually wait a
eternity.?.

Thoughts are welcome :) Or perhaps the 'all okay' so it can be merged.?
If commit message needs tweaking please feel free to do so :).

Regards,
PiBa-NL (Pieter)
From a0b01cdc8ccc4ae95c5c03bc98bf859b6115d2f9 Mon Sep 17 00:00:00 2001
From: PiBa-NL <[email protected]>
Date: Wed, 2 May 2018 22:27:14 +0200
Subject: [PATCH] BUG/MINOR, lua/sockets, make lua tasks that are waiting for
io suspend until woken up by the a corresponding event.

If a lua socket is waiting for data it currently spins at 100% cpu usage. This because the TICK_ETERNITY returned by the socket is ignored when setting the 'expire' time of the task.

Fixed by removing the check for yields that return TICK_ETERNITY.

This should be backported to at least 1.8.
---
src/hlua.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/hlua.c b/src/hlua.c
index 32199c9..4c56409 100644
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -5552,8 +5552,7 @@ static struct task *hlua_process_task(struct task *task)

case HLUA_E_AGAIN: /* co process or timeout wake me later. */
notification_gc(&hlua->com);
- if (hlua->wake_time != TICK_ETERNITY)
- task->expire = hlua->wake_time;
+ task->expire = hlua->wake_time;
break;

/* finished with error. */
--
2.10.1.windows.1
Pieter,

Am 02.05.2018 um 23:54 schrieb PiBa-NL:
> If commit message needs tweaking please feel free to do so :).
>

obviously not authoritative for this, but I noticed directly that the
first line of your message is very long. It should generally be about 60
characters, otherwise it might get truncated. The following lines should
be no more than 76 characters to avoid wrapping with the common terminal
width of 80.

Also after the severity and the component a colon should be used, not a
comma.

I suggest something like this for the first line. It is succinct and
gives a good idea of what the bug might me. But please check whether I
grasped the issue properly.

BUG/MINOR: lua: Put tasks to sleep when waiting for data

Best regards
Tim Düsterhus
Hi Tim,

Op 3-5-2018 om 0:26 schreef Tim Düsterhus:
> Pieter,
>
> Am 02.05.2018 um 23:54 schrieb PiBa-NL:
>> If commit message needs tweaking please feel free to do so :).
>>
> obviously not authoritative for this, but I noticed directly that the
> first line of your message is very long. It should generally be about 60
> characters, otherwise it might get truncated. The following lines should
> be no more than 76 characters to avoid wrapping with the common terminal
> width of 80.
>
> Also after the severity and the component a colon should be used, not a
> comma.
>
> I suggest something like this for the first line. It is succinct and
> gives a good idea of what the bug might me. But please check whether I
> grasped the issue properly.
>
> BUG/MINOR: lua: Put tasks to sleep when waiting for data
>
> Best regards
> Tim Düsterhus

Thanks, valid comments, the colons i should have noticed could have
sworn i did those correctly.. but no ;). i thought i carefully
constructed the commit message looking at a few others, while cramming
in as much 'info' about the change/issue as possible on a line (better
than 'fixed #123' as the whole subject&message as some committers do in
a other project i contribute to., but thats off-topic.). As for line
lengths, it didn't even cross my mind to look at that. (i also didn't
lookup the contributing either sorry, though it doesn't seem to specify
specific line lengths.?.)

Anyhow changed the title as suggested, it still covers the change. And
adjusted line wrapping in the message to not exceed 76.

Regards,

PiBa-NL (Pieter)

From a0b01cdc8ccc4ae95c5c03bc98bf859b6115d2f9 Mon Sep 17 00:00:00 2001
From: PiBa-NL <[email protected]>
Date: Wed, 2 May 2018 22:27:14 +0200
Subject: [PATCH] BUG/MINOR, BUG/MINOR: lua: Put tasks to sleep when waiting for data

If a lua socket is waiting for data it currently spins at 100% cpu usage.
This because the TICK_ETERNITY returned by the socket is ignored when
setting the 'expire' time of the task.

Fixed by removing the check for yields that return TICK_ETERNITY.

This should be backported to at least 1.8.
---
src/hlua.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/hlua.c b/src/hlua.c
index 32199c9..4c56409 100644
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -5552,8 +5552,7 @@ static struct task *hlua_process_task(struct task *task)

case HLUA_E_AGAIN: /* co process or timeout wake me later. */
notification_gc(&hlua->com);
- if (hlua->wake_time != TICK_ETERNITY)
- task->expire = hlua->wake_time;
+ task->expire = hlua->wake_time;
break;

/* finished with error. */
--
2.10.1.windows.1
Hi guys,

On Thu, May 03, 2018 at 12:58:55AM +0200, PiBa-NL wrote:
> Hi Tim,
>
> Op 3-5-2018 om 0:26 schreef Tim Düsterhus:
> > Pieter,
> >
> > Am 02.05.2018 um 23:54 schrieb PiBa-NL:
> > > If commit message needs tweaking please feel free to do so :).
> > >
> > obviously not authoritative for this, but I noticed directly that the
> > first line of your message is very long. It should generally be about 60
> > characters, otherwise it might get truncated. The following lines should
> > be no more than 76 characters to avoid wrapping with the common terminal
> > width of 80.
> >
> > Also after the severity and the component a colon should be used, not a
> > comma.
> >
> > I suggest something like this for the first line. It is succinct and
> > gives a good idea of what the bug might me. But please check whether I
> > grasped the issue properly.
> >
> > BUG/MINOR: lua: Put tasks to sleep when waiting for data
> >
> > Best regards
> > Tim Düsterhus
>
> Thanks, valid comments, the colons i should have noticed could have sworn i
> did those correctly.. but no ;). i thought i carefully constructed the
> commit message looking at a few others, while cramming in as much 'info'
> about the change/issue as possible on a line (better than 'fixed #123' as
> the whole subject&message as some committers do in a other project i
> contribute to., but thats off-topic.). As for line lengths, it didn't even
> cross my mind to look at that. (i also didn't lookup the contributing either
> sorry, though it doesn't seem to specify specific line lengths.?.)

Tim is absolutely right. I'm picky about the formating of the subject line
because it helps us a lot to do backports to stable branches. Just to give
you an idea, here's a typical command run in 1.8 branch :

haproxy-1.8$ ./scripts/git-show-backports -q -m -r tip/master HEAD | grep BUG/

abeaff2d - | BUG/MINOR: fd/threads: properly dereference fdcache as volatile
1ff91041 - | BUG/MINOR: fd/threads: properly lock the FD before adding it to the fd cache.
41ccb194 - | BUG/MEDIUM: threads: fix the double CAS implementation for ARMv7
f161d0f5 - | BUG/MINOR: pools/threads: don't ignore DEBUG_UAF on double-word CAS capable archs
6e8a41d8 - | BUG/MINOR: cli: Ensure all command outputs end with a LF
26fb5d84 - | BUG/MEDIUM: fd/threads: ensure the fdcache_mask always reflects the cache contents
1a1dd606 - | BUG/MINOR: h2: remove accidental debug code introduced with show_fd function
b7426d15 - | BUG/MINOR: spoe: Register the variable to set when an error occurred
(...)

Here you can easily see that it's important to have the most info and context
in the fewest words, and it's very convenient to quickly evict commits we're
not interested in for a given version. Writing optimal commit messages is a
difficult exercise first, but over time it becomes natural and you quickly
figure that it also helps for other projects because "git log --oneline" and
"git rebase -i" start to give you a lot of information that help do the best
work.

By the way I've just noticed while running this command that your subject
now mistakenly contains "BUG/MINOR, BUG/MINOR" and I didn't notice it before
pushing. Bah I should have looked closer, too late now :-)

Regarding your fix, good job! You're right, it's a mistake. At first I
thought your fix was wrong until I figured that the current code results
in not updating an expired timeout, hence the 100% CPU loop. It's not
surprising we ended up with this given that the scheduler semantics have
changed in 1.8 and this part got broke after the change.

Now merged, thank you!

Willy
Pieter,

Am 03.05.2018 um 00:58 schrieb PiBa-NL:
> *snip* As for line
> lengths, it didn't even cross my mind to look at that. (i also didn't
> lookup the contributing either sorry, though it doesn't seem to specify
> specific line lengths.?.)
>

Indeed they are not specified for haproxy, but the line lengths are kind
of an unwritten standard in git usage.

The more important length is the one of the subject line. Both

gitweb: http://git.haproxy.org/?p=haproxy.git;a=shortlog

as well as

GitHub:
https://github.com/haproxy/haproxy/commit/ebaba754297f39b86959fbdc13f66e4534aadeae

truncate the first line after a certain length. Usually a length of 50
is recommended (e.g. the vim highlighter colors the line differently
after 50 characters). However for haproxy you lose about 10 characters
for the type + severity of the patch (BUG/MINOR).

The body is recommended to wrap at lengths between 72 and 76 characters.
Personally I just wrap so that it fits my default terminal size of 80x24
characters nicely.

And one last thing: Copied, literal, output of tools such as gdb for
stack traces should not be wrapped. It should simply exceed the width.

Best regards
Tim Düsterhus
Hi Tim, Willy,

Apparently even a simple copy/paste is to difficult for me to do right
sometimes really sorry about that.. :/
Thanks for merging and explaining, ill try and do better next time :)

Regards,
PiBa-NL
On Thu, May 03, 2018 at 08:37:09PM +0200, PiBa-NL wrote:
> Hi Tim, Willy,
>
> Apparently even a simple copy/paste is to difficult for me to do right
> sometimes really sorry about that.. :/

Bah don't worry, if you want to re-inflate your ego, just look in the
logs for cleanups I've done just after commiting an atrocity that I
should be ashamed of, and you'll figure that we all do that once in a
while (typically a leftover printf or build warning).

> Thanks for merging and explaining, ill try and do better next time :)

You're welcome!

Willy
Sorry, only registered users may post in this forum.

Click here to login