Response is a bit weird because non-linear editing..
On Tue, Jun 24, 2025 at 08:10:57PM -0700, Boqun Feng wrote:
> + /* Whether the scan kthread has been scheduled to scan */
> + bool scheduled;
> +static int __noreturn shazptr_scan_kthread(void *unused)
> +{
> + for (;;) {
> + swait_event_idle_exclusive(shazptr_scan.wq,
> + READ_ONCE(shazptr_scan.scheduled));
This seems weird; why use a whole wait-queue, in exclusive mode no less,
for something that is one known thread.
Also, I think this thing needs to be FREEZABLE, otherwise suspend might
have issues.
Why not just write it like:
for (;;) {
set_current_state(TASK_IDLE | TASK_FREEZABLE);
if (!list_empty(&scan->queue))
break;
schedule();
}
__set_current_state(TASK_RUNNABLE);
for (;;) {
scoped_guard (mutex, scan->lock) {
if (list_empty(scan->queued) &&
list_empty(scan->scanning))
break;
}
shazptr_do_scan(scan);
}
> + shazptr_do_scan(&shazptr_scan);
> +
> + scoped_guard(mutex, &shazptr_scan.lock) {
> + if (list_empty(&shazptr_scan.queued) &&
> + list_empty(&shazptr_scan.scanning))
> + shazptr_scan.scheduled = false;
This condition, why can't we directly use this condition instead of
scheduled?
> + }
> + }
> +}
> +static void synchronize_shazptr_normal(void *ptr)
> +{
> +
> + /* Found blocking slots, prepare to wait. */
> + if (blocking_grp_mask) {
> + struct shazptr_scan *scan = &shazptr_scan;
> + struct shazptr_wait wait = {
> + .blocking_grp_mask = blocking_grp_mask,
> + };
> +
> + INIT_LIST_HEAD(&wait.list);
> + init_completion(&wait.done);
> +
> + scoped_guard(mutex, &scan->lock) {
> + list_add_tail(&wait.list, &scan->queued);
> +
> + if (!scan->scheduled) {
> + WRITE_ONCE(scan->scheduled, true);
> + swake_up_one(&shazptr_scan.wq);
> + }
Or perhaps; just write this like:
bool was_empty = list_empty(&scan->queued);
list_add_tail(&wait.list, &scan->queued);
if (was_empty)
wake_up_process(scan->thread);
> + }
> +
> + wait_for_completion(&wait.done);
> + }
> +}
