Skip to content

Remove redundant memory barrier #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thwuedwin
Copy link

This pull request removes an extra memory barrier before queuing AI work. As explained in the commit message, the queue_work() function provides the necessary memory-ordering guarantees, making the explicit barrier redundant under this project’s usage pattern.

The change was tested by running over 1000 games to verify that turn ordering remains strictly alternating between 'O' and 'X'.
However, this testing is not exhaustive. If you know more rigorous methods to validate memory visibility across CPUs, suggestions are welcome.

The memory barrier before queuing AI work was intended to ensure that
changes to the global variables 'finish' and 'turn' are visible to other
CPUs. However, as documented in workqueue.h, 'queue_work()' provides
the required memory-ordering guarantees: all stores preceding the call
to 'queue_work()' will be visible to the CPU that executes the work
before the work function begins.

In this project, two AI work items take turns making moves. A work item
is only queued after the other has finished executing,
ensuring that at any given time, at most one AI work item is on the
workqueue. Under this usage pattern, 'queue_work()' always returns true,
which implies that the ordering guarantees are valid and the work was
not already queued. Therefore, extra memory barriers are unnecessary.

Tested by executing over 1000 games to verify that the turn ordering
is always correct (i.e., strictly alternating between O and X).
@jserv
Copy link
Contributor

jserv commented Jun 26, 2025

Write the rationale for the proposed changes based on Linux kernel documentation and source code.

@thwuedwin thwuedwin closed this Jun 26, 2025
@thwuedwin thwuedwin reopened this Jun 26, 2025
@thwuedwin
Copy link
Author

Sorry, I accidentally closed the pull request.
Here is the explanation for the proposed change, based on my understanding of the Linux kernel documentation and source code:

The function queue_work() is defined in /include/linux/workqueue.h, which mentions that it has Memory-ordering properties.

/**
 * queue_work - queue work on a workqueue
 * Memory-ordering properties:  If it returns %true, guarantees that all stores
 * preceding the call to queue_work() in the program order will be visible from
 * the CPU which will execute @work by the time such work executes, e.g.,
 * ...
 */
static inline bool queue_work(struct workqueue_struct *wq,
			      struct work_struct *work)
{
	return queue_work_on(WORK_CPU_UNBOUND, wq, work);
}

The underlying function queue_work_on() is defined in /kernel/workqueue.c. It queues the given work on the specified CPU, if it is not already pending. Its return value is false if the work is already in a queue, and true otherwise.

    if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work)) &&
        !clear_pending_if_disabled(work)) {
        __queue_work(cpu, wq, work);
        ret = true;
    }

Inside clear_pending_if_disabled(), we can find a call to set_work_pool_and_clear_pending(), which uses a memory barrier smp_wmb() to ensure proper visibility of prior stores.

However, the first condition uses test_and_set_bit() to check and set the pending bit. Suppose the bit was already set (i.e., the work item is already enqueued). In that case, the short-circuiting behavior of the logical && operator prevents evaluation of the second condition and therefore skips the memory barrier. In such a case, queue_work_on() returns false, and the memory-ordering guarantee does not apply.

This means the memory-ordering property holds when the work is successfully queued, which is guaranteed in this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants