core: add functionality to check queue state of another thread

[miri64]

Contribution description

This adds functions to get the message slots still available in a queue as well as its total length from another thread. Using those a GNRC module can gauge how congested the input queue of a module it wants to send to is.

Testing procedure

make -C tests/msg_avail -j flash test
make -C tests/msg_queue_len -j flash test

should both pass, msg_avail does not increase in size when build with RIOT_CI_BUILD=1 compared to master (I tested on nrf52dk, z1, and arduino-mega2560).

Issues/PRs references

None

[kaspar030]

Mostly LGTM.

I've been trying to find any concurrency issues.
cib_size() shouldn't have any, as the size of a cib_t's backing array is unlikely to change.
Other than that, the new functions will fail or return bogus data (like many others) if the underlying thread dies / the thread_t gets re-used). This is a general problem and doesn't have to be addressed here.

[miri64]

Addressed comments.

[maribu]

lgtm. One comment inline

[maribu]

Have you checked if there is a benefit in providing only msg_avail_thread() as C function and msg_avail() as static inline header-only function on top? My gut feeling is that there is a small ROM benefit in this.

[miri64]

Yepp. The problem was that this requires me to include the thread.h header for the common thread_t parameter type of msg_avail_thread() and the call to thread_get_active(), leading to a circular include (as that header includes msg.h). Alternatively, kernel_pid_t could be used as a parameter as I do in this version now. However, he current version (as pointed out in OP) does not increase the ROM, so even if there is new ROM bytes to be won would be lost, I think, by the added thread->pid operation in msg_avail(), especially since that would be multiplied with every inclusion of #include "msg.h", when made inline.

[maribu]

since that would be multiplied with every inclusion of #include "msg.h", when made inline.

You mean, by every call of msg_avail(), right?

If you prefer using kernel_pid_t as argument, you could use:

static inline int msg_avail(void)
{
    return msg_avail_thread(thread_getpid());
}

This should be one instruction more (one load). If there are few calls to msg_avail(), this should be less ROM. Right now there are zero calls in sys, drivers, core, pkg, and examples to msg_avail() - only tests are using it.

Also note that for pointers, the compiler doesn't really need to know the exact type. thread_t is typedefed in sched.h (as forward declaration). And since kernel_pid_t is also typedefed there, sched.h has to be included (at least indirectly) anyway. I see no reason against using thread_t * as argument without including thread.h.

[miri64]

If you prefer using kernel_pid_t as argument, you could use:

static inline int msg_avail(void)
{
    return msg_avail_thread(thread_getpid());
}

For that I need to include thread.h as thread_getpid() is defined there...

[miri64]

Same goes for something like this

diff --git a/core/include/msg.h b/core/include/msg.h
index 3c9c8ca02a..bacadebffc 100644
--- a/core/include/msg.h
+++ b/core/include/msg.h
@@ -364,7 +364,12 @@ int msg_reply_int(msg_t *m, msg_t *reply);
  * @return Number of messages available in queue of @p pid on success
  * @return -1, if no caller's message queue is initialized
  */
diff --git a/core/include/msg.h b/core/include/msg.h
index 3c9c8ca02a..bacadebffc 100644
--- a/core/include/msg.h
+++ b/core/include/msg.h
@@ -364,7 +364,12 @@ int msg_reply_int(msg_t *m, msg_t *reply);
  * @return Number of messages available in queue of @p pid on success
  * @return -1, if no caller's message queue is initialized
  */
-int msg_avail_thread(kernel_pid_t pid);
+int _msg_avail_thread(thread_t *thread);
+
+static inline int msg_avail_thread(kernel_pid_t pid)
+{
+    return _msg_avail_thread(thread_get(pid));
+}
 
 /**
  * @brief Check how many messages are available (waiting) in the message queue
@@ -372,7 +377,10 @@ int msg_avail_thread(kernel_pid_t pid);
  * @return Number of messages available in our queue on success
  * @return -1, if no caller's message queue is initialized
  */
-int msg_avail(void);
+static inline int msg_avail(void)
+{
+    return _msg_avail_thread(thread_get_active());
+}
 
 /**
  * @brief Get maximum capacity of a thread's queue length
diff --git a/core/msg.c b/core/msg.c
index 1a375e441e..9318dc63c5 100644
--- a/core/msg.c
+++ b/core/msg.c
@@ -433,10 +433,10 @@ static int _msg_receive(msg_t *m, int block)
     DEBUG("This should have never been reached!\n");
 }
 
-static int _msg_avail(thread_t *thread)
+int _msg_avail_thread(thread_t *thread)
 {
     DEBUG("msg_available: %" PRIkernel_pid ": msg_available.\n",
-          thread->pid);
+          pid);
 
     int queue_index = -1;
 
@@ -447,16 +447,6 @@ static int _msg_avail(thread_t *thread)
     return queue_index;
 }
 
-int msg_avail_thread(kernel_pid_t pid)
-{
-    return _msg_avail(thread_get(pid));
-}
-
-int msg_avail(void)
-{
-    return _msg_avail(thread_get_active());
-}
-
 int msg_queue_capacity(kernel_pid_t pid)
 {
     DEBUG("msg_queue_capacity: %" PRIkernel_pid ": msg_queue_capacity.\n",

[maribu]

I was thinking like this:

diff --git a/core/include/msg.h b/core/include/msg.h
index 3c9c8ca02a..afb7c4b763 100644
--- a/core/include/msg.h
+++ b/core/include/msg.h
@@ -359,12 +359,12 @@ int msg_reply_int(msg_t *m, msg_t *reply);
  * @brief Check how many messages are available (waiting) in the message queue
  *        of a specific thread
  *
- * @param[in] pid    a PID
+ * @param[in] thread    Thread to get the number of available messages of
  *
  * @return Number of messages available in queue of @p pid on success
  * @return -1, if no caller's message queue is initialized
  */
-int msg_avail_thread(kernel_pid_t pid);
+int msg_avail_thread(thread_t *thread);
 
 /**
  * @brief Check how many messages are available (waiting) in the message queue
@@ -372,7 +372,10 @@ int msg_avail_thread(kernel_pid_t pid);
  * @return Number of messages available in our queue on success
  * @return -1, if no caller's message queue is initialized
  */
-int msg_avail(void);
+static inline int msg_avail(void)
+{
+    return msg_avail_thread(thread_get_active());
+}
 
 /**
  * @brief Get maximum capacity of a thread's queue length
diff --git a/core/include/sched.h b/core/include/sched.h
index ceea4771ba..34ff614eec 100644
--- a/core/include/sched.h
+++ b/core/include/sched.h
@@ -180,6 +180,32 @@ typedef enum {
 #define SCHED_PRIO_LEVELS 16
 #endif
 
+/**
+ * @brief Returns the process ID of the currently running thread
+ *
+ * @return          obviously you are not a golfer.
+ */
+static inline kernel_pid_t thread_getpid(void)
+{
+    extern volatile kernel_pid_t sched_active_pid;
+
+    return sched_active_pid;
+}
+
+/**
+ * @brief   Returns a pointer to the Thread Control Block of the currently
+ *          running thread
+ *
+ * @return  Pointer to the TCB of the currently running thread, or `NULL` if
+ *          no thread is running
+ */
+static inline thread_t *thread_get_active(void)
+{
+    extern volatile thread_t *sched_active_thread;
+
+    return (thread_t *)sched_active_thread;
+}
+
 /**
  * @brief   Triggers the scheduler to schedule the next thread
  *
diff --git a/core/include/thread.h b/core/include/thread.h
index 5bf9e05715..20ec7a100d 100644
--- a/core/include/thread.h
+++ b/core/include/thread.h
@@ -372,32 +372,6 @@ int thread_kill_zombie(kernel_pid_t pid);
  */
 int thread_wakeup(kernel_pid_t pid);
 
-/**
- * @brief Returns the process ID of the currently running thread
- *
- * @return          obviously you are not a golfer.
- */
-static inline kernel_pid_t thread_getpid(void)
-{
-    extern volatile kernel_pid_t sched_active_pid;
-
-    return sched_active_pid;
-}
-
-/**
- * @brief   Returns a pointer to the Thread Control Block of the currently
- *          running thread
- *
- * @return  Pointer to the TCB of the currently running thread, or `NULL` if
- *          no thread is running
- */
-static inline thread_t *thread_get_active(void)
-{
-    extern volatile thread_t *sched_active_thread;
-
-    return (thread_t *)sched_active_thread;
-}
-
 /**
  * @brief   Gets called upon thread creation to set CPU registers
  *
diff --git a/core/msg.c b/core/msg.c
index 1a375e441e..461d0beb85 100644
--- a/core/msg.c
+++ b/core/msg.c
@@ -433,7 +433,7 @@ static int _msg_receive(msg_t *m, int block)
     DEBUG("This should have never been reached!\n");
 }
 
-static int _msg_avail(thread_t *thread)
+int msg_avail_thread(thread_t *thread)
 {
     DEBUG("msg_available: %" PRIkernel_pid ": msg_available.\n",
           thread->pid);
@@ -447,16 +447,6 @@ static int _msg_avail(thread_t *thread)
     return queue_index;
 }
 
-int msg_avail_thread(kernel_pid_t pid)
-{
-    return _msg_avail(thread_get(pid));
-}
-
-int msg_avail(void)
-{
-    return _msg_avail(thread_get_active());
-}
-
 int msg_queue_capacity(kernel_pid_t pid)
 {
     DEBUG("msg_queue_capacity: %" PRIkernel_pid ": msg_queue_capacity.\n",

When tests/msg_avail is modified to contain both flavors, it safes 44 bytes. If only msg_avail() is used, it is 4 bytes larger.

[miri64]

TBH, that is to much of a rework for me with the added caveat of having 4 bytes more if only one of the functions is used. As you said, this function is used rarely and in my use case I will only use msg_avail_thread(), so most likely same problem. Also I want there to be a function using kernel_pid_t, as that is the type that is used in the rest of the header to identify a thread and since the msg API is used heavily outside of core IMHO it makes sense to not use a more or less internal type to core as a thread identifier.

[miri64]

Squashed and rebased to current master. Ping @maribu?

[maribu]

Maybe rename to msg_queue_capacity_thread() for naming consistency? And add

static inline int msg_queue_capacity(void)
{
    retrurn msg_queue_capacity_thread(thread_getpid());
}

Even if there is no obvious use case for this now, it is hard to change APIs later on.

[miri64]

This causes a cyclic inclusion, as thread.h (where thread_getpid() is declared) is including msg.h. I would have used sched_active_pid, but that seems not to be exposed anymore. Last alternative would be to make it a non-static inline function.

[miri64]

Squashed and rebased to current master. Ping @maribu?

(forgot to push, so did that now)

[fjmolinas]

ping @maribu @miri64

[miri64]

ping @maribu @miri64

I addressed all change requests in comment and awaiting @maribu's response.

[maribu]

ACK

[miri64]

Core change, so still needs a second ACK. @kaspar030?

[miri64]

Adapted and squashed tests to align with the latest changes from the review.

[miri64]

Shall I rebase? ;-)

[benpicco]

Sure!

[miri64]

This might need a re-review, since the signature of msg_avail() changed in current master.

Needs re-review

Needs re-review

[miri64]

Shall I rebase? ;-)

Done

[benpicco]

The type of msg_avail() just became unsigned in the meantime, the change is still good.

[kfessel]

read and checked previously raise issues (all are resolved)