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

when tagpool is exhausted, grow up to RFC 6 limits #806

Merged
merged 14 commits into from Sep 14, 2016

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Sep 13, 2016

Rather than failing at 64K matchtags, allow the non-group tagpool to grow up to 1M in size.

Also: change kvs_watch to use the non-group tagpool to hopefully address wreck scalability issues discussed in #802.

This took a while due to some disturbing behavior by libveb where initialization to "allocated" did not work, depending on the size of the veb tree - this is why I've added so many assertions to the code. I did verify that the old implementation worked as it should. In fact this condition was caught by an existing unit test that tries to run out the pool and verify that it really got all the tags. I couldn't find any memory clobbering with valgrind, so I assume this is an internal bug in libveb, and it doesn't seem to occur when the tree is initialized to "not allocated". That is how it's used in nodeset.c and here now as well.

@garlick garlick added the review label Sep 13, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+0.04%) to 75.159% when pulling 8a20300 on garlick:issue_802 into 3719509 on flux-framework:master.

@trws

This comment has been minimized.

Copy link
Member

trws commented Sep 13, 2016

Interesting about veb... hope that resolved ok. The update here looks good, but one thing we talked about the other day that might be useful is to have a diagnostic when the tag pool is forced to grow over a given threshold, actually over 4096 seems reasonable, so we can tell when something fishy is going on. Do you think it would be reasonable to include something like that here?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 13, 2016

Sorry I submitted this PR prematurely. I've gone down a rat hole debugging this, as it still seems not to be working.

I'll ponder the diagnostic - it's a bit tricky figuring how to do it in a way that works for all contexts (command, broker, module, etc).

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 13, 2016

Current coverage is 75.01% (diff: 95.58%)

Merging #806 into master will increase coverage by 0.20%

@@             master       #806   diff @@
==========================================
  Files           145        145          
  Lines         24878      24910    +32   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          18609      18685    +76   
+ Misses         6269       6225    -44   
  Partials          0          0          
Diff Coverage File Path
••••••• 75% src/common/libflux/handle.c
•••••••• 85% src/common/libutil/log.c
•••••••••• 100% src/modules/kvs/libkvs.c
•••••••••• 100% src/modules/kvs/kvs.c
•••••••••• 100% src/common/libflux/tagpool.c
•••••••••• 100% src/common/libflux/flog.c
•••••••••• 100% src/common/libutil/veb.c

Powered by Codecov. Last update 3719509...547b37f

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+0.2%) to 75.307% when pulling 71ddef8 on garlick:issue_802 into 3719509 on flux-framework:master.

garlick added some commits Sep 13, 2016

libflux/tagpool: allow tagpool to grow on demand
Start tagpool (group and regular) at 1K, and grow each by
doubling as they run out, up to the RFC 6 maximums.
test/veb: pull in upstream unit tests
Adapt the plan9 style tests supplied with libveb
to work as TAP unit tests.

Source: https://github.com/mrdomino/libveb
master e7c23f425ed0ed39938f2c6ada610c3e8a5cb93f
libutil/veb: avoid buggy mkfull()
When initializing a veb tree, always initialize it "empty".
Then if the "full" flag is set, fill it by calling vebput
on each entry.

@garlick garlick force-pushed the garlick:issue_802 branch from 71ddef8 to bcea9ec Sep 13, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+0.1%) to 75.244% when pulling bcea9ec on garlick:issue_802 into 3719509 on flux-framework:master.

garlick added some commits Sep 13, 2016

modules/kvs: use regular tagpool for kvs.watch
Using the group matchtag pool (limited to 4K entries)
is not serving any purpose in kvs_watch() since the non-group
bits aren't being used.

Switch it back to the regular pool so > 4K kvs.watches
can be managed simultaneously.

Fixes #802 sort of.
modules/kvs: fix kvs.unwach regression
Problem: kvs.unwatch is not matching kvs.watch messages,
causing kvs "watch-unwatch" sharness test failure when
non-group tagpool was used in kvs.watch.

In the recently merged pr #788, the kvs.watch protocol
changed.  The kvs.unwatch handler was partially decoding
queued kvs.watch messages in order to find them for removal,
and was not using decode functions in proto.c that were updated.

Solution: use the proto decode function.
test/kvs: watch-unwatch use reg matchtag pool
The test for tagpool leaks in a watch/unwatch tight
loop should now check the regular pool not the group pool.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+0.1%) to 75.237% when pulling 4b9e233 on garlick:issue_802 into 3719509 on flux-framework:master.

test/kvs: add test for simultaneous watchers
Try to create 2^13 (8K) simultaneous kvs watchers on
one handle.  The theoretical limit is 2^20-1 (1M),
limited by the matchtag pool size.  the previous
limit was 2^12 (4K).
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 13, 2016

OK this is finally getting close I think.

I fixed a bug in libveb where vebnew() was improperly setting up the veb tree when its "full" parameter was set. I should use the term "fixed" loosely - I just made it handle that case by creating the tree "empty", then walking the tree setting each bit. Will get a bug open upstream, although the code hasn't changed since 2010.

In the process of running this down, I pulled in the upstream veb unit tests and added some more including tests for the failing case fixed above.

I also fixed a problem with kvs.unwatch handling, introduced in pr #788.

Having fixed those things, changing the kvs to the non-group matchtag pool, and allowing the non-group pool to grow to the full size allowed by RFC 6 (1M) works. I added a test that checks how many kvs watches can be started on one handle, and ran it up to 402589 before my cloud9 VM ran out of memory. I added this to travis-ci at 8192 watchers, just to prove it goes past the previous limit of 4096.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+0.1%) to 75.261% when pulling 7c6e612 on garlick:issue_802 into 3719509 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 13, 2016

On the diagnostic, writing to stderr is likely to be a problem in some contexts (for example if stdio has been closed and the fd's reused). Would calling flux_log at LOG_CRIT each time we double the size of the tagpool be useful?

@trws

This comment has been minimized.

Copy link
Member

trws commented Sep 13, 2016

That sounds reasonable to me.

libflux/tagpool: log message when pool is expanded
It is thought that some sort of diagnostic would be useful
when a Flux component is using an excessive number of matchtags.
It could mean a resource leak or a design that lacks scalability
or flow control.

Log messages ot the handle at LOG_INFO level.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 13, 2016

OK, I made the logging level LOG_INFO though. LOG_CRIT is probably overkill. At least if things are going slowly a little clue may be waiting for you in the logs, e.g.

2016-09-13T22:33:05.345695Z -.info[0]: tagpool-normal expanded from 1024 to 2048 entries
2016-09-13T22:33:05.712806Z -.info[0]: tagpool-normal expanded from 2048 to 4096 entries
2016-09-13T22:33:06.829736Z -.info[0]: tagpool-normal expanded from 4096 to 8192 entries
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 13, 2016

OK, I made the logging level LOG_INFO though. LOG_CRIT is probably overkill. At least if things are going slowly a little clue may be waiting for you in the logs, e.g.

I think that is perfect, I was also going to suggest that LOG_INFO is adequate.

Is there any way to tag the log message coming from an API user with process name or UUID so when/if we do see one of the messages we can guess at the source?

One idea for scalability testsuite would be to check or filter for these messages after runs.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+0.1%) to 75.24% when pulling 137b4a2 on garlick:issue_802 into 3719509 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 13, 2016

I was just wondering about the "-" in the log message. That is the default "appname" if flux_log_set_appname() has not been called. I had put together a quick fix to make the pid the default; then I thought I'd better look a little more carefully to see why the pid isn't already in the logs since it's part of the message.

Actually, making the default appname = process name is an excellent idea.

libflux/flog: use __progname for default "appname"
If a handle user has not called flux_log_set_appnum(),
ensure that some identifying name gets into the logs.
Use the supposedly portable *__progname.

The previous default was "-", utterly unhelpful.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 13, 2016

Easier than I thought:

2016-09-13T22:49:21.625546Z lt-watch.info[0]: tagpool-normal expanded from 1024 to 2048 entries
2016-09-13T22:49:21.900167Z lt-watch.info[0]: tagpool-normal expanded from 2048 to 4096 entries
2016-09-13T22:49:22.465946Z lt-watch.info[0]: tagpool-normal expanded from 4096 to 8192 entries
2016-09-13T22:49:23.627800Z lt-watch.info[0]: tagpool-normal expanded from 8192 to 16384 entries
libflux/flog: log at LOG_ERR if out of matchtags
Since we're logging matchtag pool expansion, add a log
message at LOG_ERR when matchtags are temporarily unavailble.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+0.1%) to 75.248% when pulling 9f9f810 on garlick:issue_802 into 3719509 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage increased (+0.1%) to 75.246% when pulling 9f9f810 on garlick:issue_802 into 3719509 on flux-framework:master.

garlick added some commits Sep 14, 2016

test/kvs: check kvs_unwatch removes module state
t/kvs/watch simulwatch, driven by t1000-kvs-basic.t sharness
test, now checks the kvs stats to see that the expected
number of watchers has been registered, then calls
kvs_unwatch() and checks that all the watchers have
been unregistered.  A single kvs_unwatch() call unregisters
them all since they are all watching the same key.

This checks for the bug just fixed, where kvs module
state was not being cleaned up by the kvs.unwatch state.
libutil/log: default program name to __progname
If log_init() is not called with another name,
use __progname.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.2%) to 75.32% when pulling 245f8c6 on garlick:issue_802 into 3719509 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.2%) to 75.326% when pulling 547b37f on garlick:issue_802 into 3719509 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 14, 2016

This is ready to be considered for a merge.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 14, 2016

Really nice work here @garlick! I'll poke at it but it looks great to me.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 14, 2016

This is perfect. Merging.

@grondo grondo merged commit 430bedb into flux-framework:master Sep 14, 2016

4 checks passed

codecov/patch 95.58% of diff hit (target 74.80%)
Details
codecov/project 75.01% (+0.20%) compared to 3719509
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 75.326%
Details

@grondo grondo removed the review label Sep 14, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 14, 2016

Thanks!

On Sep 14, 2016 8:19 AM, "Mark Grondona" notifications@github.com wrote:

Merged #806 #806.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#806 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKX2w4uEbaIjBs6sSEiNu6fYDoQW8miks5qqBCagaJpZM4J7Sub
.

@garlick garlick deleted the garlick:issue_802 branch Sep 14, 2016

@garlick garlick referenced this pull request Oct 26, 2016

Closed

0.5.0 release notes #879

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