Fix-Point commented on PR #17570: URL: https://github.com/apache/nuttx/pull/17570#issuecomment-3675027337
Please pay more respect to parallel programming. It does not look like a simple change can fix this at all. I hope that before you claim you fix the issue, you can review your state-machine more carefully. I must say sorry that I have not too much time to review your commit. I am not your z3 SMT solver who can always automatically generate the counter-examples for you. I think it is impossible (or very hard) to allow restart the timer correctly unless refactoring your state-machine design. At least, I can not find a solution that can ensure functional correctness for your design. For example, When the newly-set timer (triggered at t2) while the old timer callback function (triggered at t1) is executing simultaneously. This is a highly likely SMP scenario of thread interleaving. The old timer callback returns next as `UINT32_MAX`, while the new request callback returns next as `1000`. However, since the old timer callback detects that the hrtimer is in the `running` state, so it sets the next timer to `(t2 + UINT32_MAX)` instead of `(t1 + UINT32_MAX)`. After the new timer callback finishes execution and proceeds to modify the timer, the next timer trigger time ends up being `(t2 + UINT32_MAX + 1000)` rather than the correct value of `(t2 + 1000)`. I suspect that the reason your patch fails to run `ostest` on `rv-virt:smp` is also due to this issue. By the way, here is the another simple test-case that fails that I do not even know why. It stucked at `hrtimer_cancel_sync(&timer);`. Please fix this problem first. I must say that I am not a lab rat or your tester. Please ensure the functional correctness of your patches before merging it to upstream. I fully agree with @anchao, who pointed out my problem in https://github.com/apache/nuttx/pull/16342 before. > emm ... community is not a testing ground, please do not treating everyone like lab rats. After that, I am trying my best to ensure my PR is functional correctness. ```c /**************************************************************************** * apps/examples/hello/hello_main.c * * SPDX-License-Identifier: Apache-2.0 * * Licensed to the Apache Software Foundation (ASF) under one or more * contributor license agreements. See the NOTICE file distributed with * this work for additional information regarding copyright ownership. The * ASF licenses this file to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance with the * License. You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the * License for the specific language governing permissions and limitations * under the License. * ****************************************************************************/ /**************************************************************************** * Included Files ****************************************************************************/ #include <nuttx/config.h> #include <stdio.h> #include <unistd.h> #include <nuttx/hrtimer.h> #define HRTIMER_TEST_THREAD_NR (1) #define HRTIMER_TEST_NR (1000000) /**************************************************************************** * Public Functions ****************************************************************************/ static int volatile tcount = 0; static volatile uint32_t next = 0; static uint32_t test_callback(FAR struct hrtimer_s *hrtimer) { tcount++; up_ndelay(hrtimer->expired % (10 * NSEC_PER_USEC)); return 0; } static uint32_t test_callback_background(FAR struct hrtimer_s *hrtimer) { up_ndelay(hrtimer->expired % NSEC_PER_USEC); return 0; } static void test1(int tid) { hrtimer_t timer; int count = 0; irqstate_t flags; spinlock_t lock; if (tid == 0) { hrtimer_init(&timer, test_callback, NULL); } else { hrtimer_init(&timer, test_callback_background, NULL); } while (count++ < HRTIMER_TEST_NR) { int ret; if (tid == 0) { uint64_t delay = rand() % (10 * NSEC_PER_MSEC); /* Simulate the periodical hrtimer.. */ flags = spin_lock_irqsave(&lock); /* Use as periodical timer */ ret = hrtimer_cancel(&timer); ret = hrtimer_start(&timer, 1000, HRTIMER_MODE_REL); spin_unlock_irqrestore(&lock, flags); up_ndelay(NSEC_PER_MSEC); flags = spin_lock_irqsave(&lock); ret = hrtimer_cancel(&timer); ret = hrtimer_start(&timer, 1000, HRTIMER_MODE_REL); spin_unlock_irqrestore(&lock, flags); up_ndelay(NSEC_PER_MSEC); hrtimer_cancel_sync(&timer); // stucked here???? printf("???\n"); } else { /* Simulate the background hrtimer.. */ uint64_t delay = rand() % (10 * NSEC_PER_MSEC); ret = hrtimer_cancel(&timer); ret = hrtimer_start(&timer, delay, HRTIMER_MODE_REL); } UNUSED(ret); } } static void* test_thread(void *arg) { while (1) { test1((int)arg); } return NULL; } /**************************************************************************** * hello_main ****************************************************************************/ int main(int argc, FAR char *argv[]) { unsigned int thread_id; pthread_attr_t attr; pthread_t pthreads[HRTIMER_TEST_THREAD_NR]; printf("hrtimer_test start...\n"); ASSERT(pthread_attr_init(&attr) == 0); /* Create wdog test thread */ for (thread_id = 0; thread_id < HRTIMER_TEST_THREAD_NR; thread_id++) { ASSERT(pthread_create(&pthreads[thread_id], &attr, test_thread, (void *)thread_id) == 0); } for (thread_id = 0; thread_id < HRTIMER_TEST_THREAD_NR; thread_id++) { pthread_join(pthreads[thread_id], NULL); } ASSERT(pthread_attr_destroy(&attr) == 0); printf("hrtimer_test end...\n"); return 0; } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
