On 02.08.21 12:27, Jan Kiszka wrote:
> On 02.08.21 12:06, Florian Bezdeka wrote:
>> On 02.08.21 11:45, Jan Kiszka wrote:
>>> On 31.07.21 09:08, Florian Bezdeka wrote:
>>>> This patch was based on the patch sent out by Song and reworked.
>>>> Especially
>>>> - switched from CLOCK_MONOTONIC to CLOCK_REALTIME
>>>> According to POSIX this service is based on CLOCK_REALTIME
>>>>
>>>> - Fixed some mutex leaks / missing pthread_mutex_destroy()
>>>>
>>>> - Removed some magic numbers used for making sure the syscall does
>>>> not return too early
>>>>
>>>> - Fixed several mutex deadlocks. Once mutex_timedlock() was
>>>> successful following calls will deadlock (as long as no special
>>>> DEADLOCK mutex is being used)
>>>>
>>>> Signed-off-by: Florian Bezdeka <florian.bezd...@siemens.com>
>>>> ---
>>>> testsuite/smokey/y2038/syscall-tests.c | 187 +++++++++++++++++++++++++
>>>> 1 file changed, 187 insertions(+)
>>>>
>>>> diff --git a/testsuite/smokey/y2038/syscall-tests.c
>>>> b/testsuite/smokey/y2038/syscall-tests.c
>>>> index 08506d086..2e8add864 100644
>>>> --- a/testsuite/smokey/y2038/syscall-tests.c
>>>> +++ b/testsuite/smokey/y2038/syscall-tests.c
>>>> @@ -115,6 +115,45 @@ static inline bool ts_less(const struct xn_timespec64
>>>> *a,
>>>> return false;
>>>> }
>>>>
>>>> +/**
>>>> + * Simple helper data structure for holding a thread context
>>>> + */
>>>> +struct thread_context {
>>>> + int sc_nr;
>>>> + pthread_mutex_t *mutex;
>>>> + struct xn_timespec64 *ts;
>>>> + bool timedwait_timecheck;
>>>> +};
>>>> +
>>>> +/**
>>>> + * Start the supplied function inside a separate thread, wait for
>>>> completion
>>>> + * and check the thread return value.
>>>> + *
>>>> + * @param thread The thread entry point
>>>> + * @param arg The thread arguments
>>>> + * @param exp_result The expected return value
>>>> + *
>>>> + * @return 0 if the thread reported @exp_result as return value, the
>>>> thread's
>>>> + * return value otherwise
>>>> + */
>>>> +static int run_thread(void *(*thread)(void *), void *arg, int exp_result)
>>>> +{
>>>> + pthread_t tid;
>>>> + void *status;
>>>> + long res;
>>>> + int ret;
>>>> +
>>>> + if (!__T(ret, pthread_create(&tid, NULL, thread, arg)))
>>>> + return ret;
>>>> +
>>>> + if (!__T(ret, pthread_join(tid, &status)))
>>>> + return ret;
>>>> +
>>>> + res = *((long *) status);
>>>> +
>>>> + return (res == exp_result) ? 0 : ret;
>>>> +}
>>>> +
>>>> static int test_sc_cobalt_sem_timedwait64(void)
>>>> {
>>>> int ret;
>>>> @@ -404,6 +443,150 @@ static int test_sc_cobalt_clock_adjtime64(void)
>>>> return 0;
>>>> }
>>>>
>>>> +static void *timedlock64_thread(void *arg)
>>>> +{
>>>> + struct thread_context *ctx = (struct thread_context *) arg;
>>>> + struct xn_timespec64 t1, t2;
>>>> + struct timespec ts_nat;
>>>> + int ret;
>>>> +
>>>> + if (ctx->timedwait_timecheck) {
>>>> + if (!__T(ret, clock_gettime(CLOCK_REALTIME, &ts_nat)))
>>>> + return (void *)(long)ret;
>>>> +
>>>> + t1.tv_sec = ts_nat.tv_sec;
>>>> + t1.tv_nsec = ts_nat.tv_nsec;
>>>> + ts_add_ns(&t1, ctx->ts->tv_nsec);
>>>> + ts_add_ns(&t1, ctx->ts->tv_sec * NSEC_PER_SEC);
>>>> + }
>>>> +
>>>> + ret = XENOMAI_SYSCALL2(ctx->sc_nr, ctx->mutex, (void *) ctx->ts);
>>>> + if (ret)
>>>> + return (void *)(long)ret;
>>>> +
>>>> + if (ctx->timedwait_timecheck) {
>>>> + if (!__T(ret, clock_gettime(CLOCK_REALTIME, &ts_nat)))
>>>> + return (void *)(long)ret;
>>>> +
>>>> + t2.tv_sec = ts_nat.tv_sec;
>>>> + t2.tv_nsec = ts_nat.tv_nsec;
>>>> +
>>>> + if (ts_less(&t2, &t1))
>>>> + smokey_warning("mutex_timedlock64 returned to early!\n"
>>>> + "Expected wakeup at: %lld sec %lld
>>>> nsec\n"
>>>> + "Back at : %lld sec %lld
>>>> nsec\n",
>>>> + t1.tv_sec, t1.tv_nsec, t2.tv_sec,
>>>> + t2.tv_nsec);
>>>> + }
>>>> +
>>>> + return (void *)(long)pthread_mutex_unlock(ctx->mutex);
>>>> +}
>>>> +
>>>> +static int test_sc_cobalt_mutex_timedlock64(void)
>>>> +{
>>>> + int ret;
>>>> + pthread_mutex_t mutex;
>>>> + int sc_nr = sc_cobalt_mutex_timedlock64;
>>>> + struct xn_timespec64 ts64;
>>>> + struct thread_context ctx = {0};
>>>> +
>>>> + ret = pthread_mutex_init(&mutex, NULL);
>>>> +
>>>> + /* Make sure we don't crash because of NULL pointers */
>>>> + ret = XENOMAI_SYSCALL2(sc_nr, NULL, NULL);
>>>> + if (ret == -ENOSYS) {
>>>> + smokey_note("mutex_timedlock64: skipped. (no kernel support)");
>>>> + return 0; // Not implemented, nothing to test, success
>>>> + }
>>>> + if (!smokey_assert(ret == -EINVAL))
>>>> + return ret ? ret : -EINVAL;
>>>> +
>>>> + /*
>>>> + * mutex can be taken immediately, no need to validate
>>>> + * NULL should be allowed
>>>> + */
>>>> + ret = XENOMAI_SYSCALL2(sc_nr, &mutex, NULL);
>>>> + if (!smokey_assert(!ret))
>>>> + return ret;
>>>> +
>>>> + if (!__T(ret, pthread_mutex_unlock(&mutex)))
>>>> + return ret;
>>>> +
>>>> + /*
>>>> + * mutex can be taken immediately, no need to validate
>>>> + * an invalid address should be allowed
>>>> + */
>>>> + ret = XENOMAI_SYSCALL2(sc_nr, &mutex, 0xdeadbeef);
>>>> + if (!smokey_assert(!ret))
>>>> + return ret;
>>>> +
>>>> + /*
>>>> + * mutex still locked, second thread has to fail with -EINVAL when
>>>> + * submitting NULL as timeout
>>>> + */
>>>> + ctx.sc_nr = sc_nr;
>>>> + ctx.mutex = &mutex;
>>>> + ctx.ts = NULL;
>>>> + if (!__T(ret, run_thread(timedlock64_thread, &ctx, -EINVAL)))
>>>> + return ret;
>>>> +
>>>> + /*
>>>> + * mutex still locked, second thread has to fail with -EFAULT when
>>>> + * submitting an invalid address as timeout
>>>> + */
>>>> + ctx.ts = (void *) 0xdeadbeef;
>>>> + if (!__T(ret, run_thread(timedlock64_thread, &ctx, -EFAULT)))
>>>> + return ret;
>>>> +
>>>> + /*
>>>> + * mutex still locked, second thread has to fail with -EFAULT when
>>>> + * submitting an invalid timeout (while the address is valid)
>>>> + */
>>>> + ts64.tv_sec = -1;
>>>> + ctx.ts = &ts64;
>>>> + if (!__T(ret, run_thread(timedlock64_thread, &ctx, -EFAULT)))
>>>> + return ret;
>>>> +
>>>> + /*
>>>> + * mutex still locked, second thread has to fail with -ETIMEOUT when
>>>> + * submitting a valid timeout
>>>> + */
>>>> + ts64.tv_sec = 0;
>>>> + ts64.tv_nsec = 500;
>>>> + if (!__T(ret, run_thread(timedlock64_thread, &ctx, -ETIMEDOUT)))
>>>> + return ret;
>>>> +
>>>> + if (!__T(ret, pthread_mutex_unlock(&mutex)))
>>>> + return ret;
>>>> +
>>>> + /* mutex available, second thread should be able to lock and unlock */
>>>> + ts64.tv_sec = 0;
>>>> + ts64.tv_nsec = 500;
>>>> + if (!__T(ret, run_thread(timedlock64_thread, &ctx, 0)))
>>>> + return ret;
>>>> +
>>>> + /*
>>>> + * Locking the mutex here so the second thread has to deliver -ETIMEOUT.
>>>> + * Timechecks will now be enabled to make sure we don't give up to early
>>>> + */
>>>> + if (!__T(ret, pthread_mutex_lock(&mutex)))
>>>> + return ret;
>>>> +
>>>> + ts64.tv_sec = 0;
>>>> + ts64.tv_nsec = 500;
>>>> + ctx.timedwait_timecheck = true;
>>>> + if (!__T(ret, run_thread(timedlock64_thread, &ctx, -ETIMEDOUT)))
>>>> + return ret;
>>>> +
>>>> + if (!__T(ret, pthread_mutex_unlock(&mutex)))
>>>> + return ret;
>>>> +
>>>> + if (!__T(ret, pthread_mutex_destroy(&mutex)))
>>>> + return ret;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int run_y2038(struct smokey_test *t, int argc, char *const argv[])
>>>> {
>>>> int ret;
>>>> @@ -432,5 +615,9 @@ static int run_y2038(struct smokey_test *t, int argc,
>>>> char *const argv[])
>>>> if (ret)
>>>> return ret;
>>>>
>>>> + ret = test_sc_cobalt_mutex_timedlock64();
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>
>>> We have build errors, possibly due to this change. See e.g.
>>>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.denx.de%2FXenomai%2Fxenomai-images%2F-%2Fjobs%2F302794&data=04%7C01%7Cflorian.bezdeka%40siemens.com%7Cf3d801d35dcd4c4c73fb08d955a017b8%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637634968345821616%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=aCAWr9GWQT%2BfqQgYyTNMWd0UZBjjUvUcfutdcMH4k0c%3D&reserved=0
>>
>> That's strange. Local build as well as the CI build done by the gitlab
>> hackerspace project for the florian/y2038 is succesfull, but next is
>> failing...
>>
>> florian/y2038:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.com%2FXenomai%2Fxenomai-hacker-space%2F-%2Fpipelines%2F345882439&data=04%7C01%7Cflorian.bezdeka%40siemens.com%7Cf3d801d35dcd4c4c73fb08d955a017b8%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637634968345821616%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Q%2F1JfxX8knBWCCcCDodyQwWvteu4DXr7GTIDxpAlF80%3D&reserved=0
>>
>
> I'm trying to convert ctx->timedwait_timecheck into a local var, maybe
> that helps the compiler.
Seems to be an compiler thing.
index 36475347e..d7784e3da 100644
--- a/testsuite/smokey/y2038/syscall-tests.c
+++ b/testsuite/smokey/y2038/syscall-tests.c
@@ -447,7 +447,7 @@ static int test_sc_cobalt_clock_adjtime64(void)
static void *timedlock64_thread(void *arg)
{
struct thread_context *ctx = (struct thread_context *) arg;
- struct xn_timespec64 t1, t2;
+ struct xn_timespec64 t1 = {0}, t2 = {0};
struct timespec ts_nat;
int ret;
Should do it.
>
> Jan
>
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
