(#499) Minor cleanup of INVALID_TID, and MAX_THREADS in task-system

[gapisback]

This commit applies some minor clean-up to task system as a follow-on to the larger rework done under PR #497. Consistently use STATUS_BUSY, as a way to report when all concurrent threads are in-use. This will result in a Resource temporarily unavailable message to the client. Minor changes to task system unit-test code & cleanup of comments.

--- NOTE to the reviewer:

Nothing is changing substantially; minor corrections to keep code & test code consistent.

[netlify[bot]]

✅ Deploy Preview for splinterdb canceled.

Name	Link
🔨 Latest commit	8577b58e0e1dd0b17a97dfcad79b8b83d2b895c0
🔍 Latest deploy log	https://app.netlify.com/sites/splinterdb/deploys/63bf412675be20000a3ace44

[gapisback]

@rtjohnso - I had tried to implement 'LIMIT_EXCEEDED' in earlier rounds of my own dev. The error message reported by strerror() is very strange:

TEST 5/6 task_system:test_use_all_but_one_threads_for_bg_threads
Verbose mode on.  This test exercises an error case, so on sucess it will print a message that appears to be an error.
[...]
Cannot create a new thread as the limit on concurrent threads, 64, will be exceeded.
Could not create a thread: No space left on device

In above test case, when I induced this error when trying to start one thread more than MAX_THREADS, the error message comes out as: No space left on device -- which I think is quite misleading.

This is occurring because we have mapped this as follows in platform_linux/platform_types.h:

 25 #define STATUS_LIMIT_EXCEEDED CONST_STATUS(ENOSPC)

while STATUS_BUSY is mapped as:

 24 #define STATUS_BUSY           CONST_STATUS(EAGAIN)

... This seems more consistent.

[gapisback]

@rtjohnso - I think the underlying problem is that STATUS_LIMIT_EXCEEDED is mapped to something that may not apply in all cases.

Checking this errno man page, I could not find any E<token> that maps cleanly to our current usages of STATUS_LIMIT_EXCEEDED in our code base.

If you can suggest that is closely correct, we can amend the mapping in a different PR.

[rtjohnso]

You're right. I did a quick skim and didn't see any thing better. Proceed as you had planned. Thank you.

Best, Rob

On Wed, Jan 11, 2023, 3:05 PM gapisback @.***> wrote:

@.**** commented on this pull request.

In src/task.c https://github.com/vmware/splinterdb/pull/529#discussion_r1067546845:

@@ -255,7 +255,7 @@ task_create_thread_with_hooks(platform_thread *thread, platform_error_log("Cannot create a new thread as the limit on" " concurrent threads, %d, will be exceeded.\n", MAX_THREADS);

• return STATUS_LIMIT_EXCEEDED;
• return STATUS_BUSY;

Unfortunately, we have this mapping in our code base:

`` 25 #define STATUS_LIMIT_EXCEEDED CONST_STATUS(ENOSPC)

which ends up spitting out a [misleading] Could not create a thread: No space left on device message.

I think we should look into properly remapping STATUS_LIMIT_EXCEEDED to a more appropriate errno-code, outside this PR (if you don't mind).

— Reply to this email directly, view it on GitHub https://github.com/vmware/splinterdb/pull/529#discussion_r1067546845, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA45FRIBZWX26GMRHVVLOJTWR44DTANCNFSM6AAAAAATYEBL2U . You are receiving this because you were mentioned.Message ID: @.***>