xiaoxiang781216 commented on code in PR #18131: URL: https://github.com/apache/nuttx/pull/18131#discussion_r2724277979
########## sched/hrtimer/hrtimer.h: ########## @@ -33,9 +33,18 @@ #include <nuttx/hrtimer.h> #include <nuttx/seqlock.h> +#include "sched/sched.h" #ifdef CONFIG_HRTIMER +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* Delay used while waiting for a running hrtimer callback to complete */ + +#define HRTIMER_CANCEL_SYNC_DELAY_MS 5 Review Comment: too large? let's sleep one tick ########## sched/hrtimer/hrtimer_gettime.c: ########## @@ -0,0 +1,57 @@ +/**************************************************************************** + * sched/hrtimer/hrtimer_gettime.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 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> Review Comment: add blank line ########## sched/hrtimer/hrtimer.h: ########## @@ -309,5 +309,112 @@ static inline_function bool hrtimer_is_first(FAR hrtimer_t *hrtimer) #endif } +/**************************************************************************** + * Name: hrtimer_mark_running + * + * Description: + * Mark the timer as running. + * + * Input Parameters: + * timer - The timer to be marked. + * cpu - The CPU core Id. + * + * Returned Value: + * None. + * + * Assumption: + * The caller must hold the lock. + * + ****************************************************************************/ + +#ifdef CONFIG_SMP +# define hrtimer_mark_running(timer, cpu) \ + (g_hrtimer_running[cpu] = (uintptr_t)(timer)) +#else +# define hrtimer_mark_running(timer, cpu) UNUSED(cpu) +#endif + +/**************************************************************************** + * Name: hrtimer_has_ownership + * + * Description: + * Check if the CPU core has ownership of the timer. + * + * Input Parameters: + * timer - The timer to be marked. + * cpu - The CPU core Id. + * + * Returned Value: + * true if the CPU core has ownership of the timer, false otherwise. + * + * Assumption: + * The caller must hold the lock. + * + ****************************************************************************/ + +#ifdef CONFIG_SMP +# define hrtimer_has_ownership(timer, cpu) \ + (g_hrtimer_running[cpu] == (uintptr_t)(timer)) +#else +# define hrtimer_has_ownership(timer, cpu) (true) +#endif + +/**************************************************************************** + * Name: hrtimer_revoke_ownership + * + * Description: + * Revoke the ownership of the cancelled timer and return the references + * count to the timer. + * + * Input Parameters: + * queue - The timer queue. + * cancelled - The cancelled timer upt + * + * Returned Value: + * The references count to the timer. + * + * Assumption: + * The caller must hold the queue lock. + * + ****************************************************************************/ + +static inline_function +int hrtimer_revoke_ownership(uintptr_t cancelled) Review Comment: change to hrtimer_cancel_running ########## sched/hrtimer/hrtimer_process.c: ########## @@ -119,16 +119,14 @@ void hrtimer_process(uint64_t now) flags = write_seqlock_irqsave(&g_hrtimer_lock); - hrtimer_mark_running(NULL, cpu); Review Comment: why not remove directly from https://github.com/apache/nuttx/pull/18131/changes/734c80aa1092bb87c4260361d45610f1cf2fef13#diff-f9e41d058bde844c806f58468dfe80d07319014601b4bbf1d9cdf6c79388c797R144 ########## sched/hrtimer/hrtimer.h: ########## @@ -32,6 +32,8 @@ #include <nuttx/clock.h> #include <nuttx/hrtimer.h> Review Comment: remove blank line ########## sched/hrtimer/hrtimer.h: ########## @@ -33,9 +33,18 @@ #include <nuttx/hrtimer.h> #include <nuttx/seqlock.h> Review Comment: add blank line ########## sched/hrtimer/hrtimer_process.c: ########## @@ -148,6 +141,8 @@ void hrtimer_process(uint64_t now) hrtimer = hrtimer_get_first(); } + hrtimer_mark_running(NULL, cpu); Review Comment: let's add hrtimer_unmark_running macro? ########## sched/hrtimer/hrtimer.h: ########## @@ -309,5 +309,112 @@ static inline_function bool hrtimer_is_first(FAR hrtimer_t *hrtimer) #endif } +/**************************************************************************** + * Name: hrtimer_mark_running + * + * Description: + * Mark the timer as running. + * + * Input Parameters: + * timer - The timer to be marked. + * cpu - The CPU core Id. + * + * Returned Value: + * None. + * + * Assumption: + * The caller must hold the lock. + * + ****************************************************************************/ + +#ifdef CONFIG_SMP +# define hrtimer_mark_running(timer, cpu) \ + (g_hrtimer_running[cpu] = (uintptr_t)(timer)) +#else +# define hrtimer_mark_running(timer, cpu) UNUSED(cpu) +#endif + +/**************************************************************************** + * Name: hrtimer_has_ownership + * + * Description: + * Check if the CPU core has ownership of the timer. + * + * Input Parameters: + * timer - The timer to be marked. + * cpu - The CPU core Id. + * + * Returned Value: + * true if the CPU core has ownership of the timer, false otherwise. + * + * Assumption: + * The caller must hold the lock. + * + ****************************************************************************/ + +#ifdef CONFIG_SMP +# define hrtimer_has_ownership(timer, cpu) \ Review Comment: change to hrtimer_is_running ########## sched/hrtimer/hrtimer_cancel.c: ########## @@ -60,11 +60,12 @@ #ifdef CONFIG_SMP static inline_function bool hrtimer_is_active(FAR hrtimer_t *hrtimer) { - bool is_active = false; + bool is_active = false; + uintptr_t cancelled = (uintptr_t)hrtimer | 0x1u; - for (int i = 0; i < CONFIG_SMP_NCPUS; i++) + for (int cpu = 0; cpu < CONFIG_SMP_NCPUS; cpu++) Review Comment: move declaration out of loop ########## sched/hrtimer/hrtimer.h: ########## @@ -309,5 +309,112 @@ static inline_function bool hrtimer_is_first(FAR hrtimer_t *hrtimer) #endif } +/**************************************************************************** + * Name: hrtimer_mark_running + * + * Description: + * Mark the timer as running. + * + * Input Parameters: + * timer - The timer to be marked. + * cpu - The CPU core Id. + * + * Returned Value: + * None. + * + * Assumption: + * The caller must hold the lock. + * + ****************************************************************************/ + +#ifdef CONFIG_SMP +# define hrtimer_mark_running(timer, cpu) \ + (g_hrtimer_running[cpu] = (uintptr_t)(timer)) +#else +# define hrtimer_mark_running(timer, cpu) UNUSED(cpu) +#endif + +/**************************************************************************** + * Name: hrtimer_has_ownership + * + * Description: + * Check if the CPU core has ownership of the timer. + * + * Input Parameters: + * timer - The timer to be marked. + * cpu - The CPU core Id. + * + * Returned Value: + * true if the CPU core has ownership of the timer, false otherwise. + * + * Assumption: + * The caller must hold the lock. + * + ****************************************************************************/ + +#ifdef CONFIG_SMP +# define hrtimer_has_ownership(timer, cpu) \ + (g_hrtimer_running[cpu] == (uintptr_t)(timer)) +#else +# define hrtimer_has_ownership(timer, cpu) (true) +#endif + +/**************************************************************************** + * Name: hrtimer_revoke_ownership + * + * Description: + * Revoke the ownership of the cancelled timer and return the references + * count to the timer. + * + * Input Parameters: + * queue - The timer queue. + * cancelled - The cancelled timer upt + * + * Returned Value: + * The references count to the timer. + * + * Assumption: + * The caller must hold the queue lock. + * + ****************************************************************************/ + +static inline_function +int hrtimer_revoke_ownership(uintptr_t cancelled) Review Comment: let's or 1 inside this function instead ``` int hrtimer_revoke_ownership(FAR hrtimer_t *hrtimer) ``` ########## sched/hrtimer/hrtimer_cancel.c: ########## @@ -60,11 +60,12 @@ #ifdef CONFIG_SMP static inline_function bool hrtimer_is_active(FAR hrtimer_t *hrtimer) { - bool is_active = false; + bool is_active = false; + uintptr_t cancelled = (uintptr_t)hrtimer | 0x1u; Review Comment: move `| 1` into hrtimer_has_ownership -- 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]
