Messages in this thread | | | From | Anna-Maria Behnsen <> | Subject | Re: [PATCH v10 18/20] timers: Implement the hierarchical pull model | Date | Mon, 05 Feb 2024 16:59:45 +0100 |
| |
Frederic Weisbecker <frederic@kernel.org> writes:
> Le Mon, Jan 15, 2024 at 03:37:41PM +0100, Anna-Maria Behnsen a écrit : >> +static bool tmigr_handle_remote_up(struct tmigr_group *group, >> + struct tmigr_group *child, >> + void *ptr) >> +{ >> + struct tmigr_remote_data *data = ptr; >> + u64 now, next = KTIME_MAX; >> + struct tmigr_event *evt; >> + unsigned long jif; >> + u8 childmask; >> + >> + jif = data->basej; >> + now = data->now; >> + >> + childmask = data->childmask; >> + >> +again: >> + /* >> + * Handle the group only if @childmask is the migrator or if the >> + * group has no migrator. Otherwise the group is active and is >> + * handled by its own migrator. >> + */ >> + if (!tmigr_check_migrator(group, childmask)) >> + return true; >> + >> + raw_spin_lock_irq(&group->lock); >> + >> + evt = tmigr_next_expired_groupevt(group, now); >> + >> + if (evt) { >> + unsigned int remote_cpu = evt->cpu; >> + >> + raw_spin_unlock_irq(&group->lock); >> + >> + next = tmigr_handle_remote_cpu(remote_cpu, now, jif); >> + >> + /* check if there is another event, that needs to be handled */ >> + goto again; >> + } else { >> + raw_spin_unlock_irq(&group->lock); >> + } >> + >> + /* >> + * Update of childmask for the next level and keep track of the expiry >> + * of the first event that needs to be handled >> + */ >> + data->childmask = group->childmask; >> + data->firstexp = next; > > So assume we have: > > [GRP1:0] > migrator = [GRP0:0] > active = [GRP0:0] > nextevt = TIMER3 > / \ > [GRP0:0] [GRP0:1] > migrator = CPU0 migrator = NONE > active = CPU0 active = NONE > nextevt = KTIME_MAX nextevt = TIMER3 > / \ / \ > 0 1 2 3 > idle idle idle idle (TIMER3) > > Then CPU 0 goes idle: > > [GRP1:0] > migrator = NONE > active = NONE > nextevt = TIMER3 > / \ > [GRP0:0] [GRP0:1] > migrator = NONE migrator = NONE > active = NONE active = NONE > nextevt = KTIME_MAX nextevt = TIMER3 > / \ / \ > 0 1 2 3 > idle idle idle idle (TIMER3) > > CPU 0 is the idle migrator and its tmc->wakeup is TIMER3. > But CPU 0 has a local timer that expires before TIMER3. > > When that timer interrupt fires, it raises the softirq, which > executes on IRQ tail. So CPU0 eventually calls tmigr_handle_remote() > before TIMER3 has expired. > > This leads to tmigr_next_expired_groupevt() to return NULL and then > data->firstexp = KTIME_MAX and then tmc->wakeup = KTIME_MAX. > > Later on, tmigr_new_timer() is called with a KTIME_MAX global > event and so tmc->wakeup stays with KTIME_MAX, ignoring TIMER3. > > It looks like you need to handle the tmigr_next_expired_groupevt() > case returning NULL.
Yes. You are right. group->next_expiry is updated via tmigr_next_expired_groupevt(). So I should take this value to rely on for the firstevt.
But with this, we do not need the return value of tmigr_handle_remote_cpu(). When the upperst level is inactive and there is a timer (e.g. propagated there by tmigr_handle_remote_cpu()), the return value of tmigr_handle_remote_cpu() is the next_expiry of the top level group. But this value is also noticed by walking the hierarchy up to the top level with tmigr_handle_remote_up().
Thanks,
Anna-Maria
| |