On Fri, Jan 14, 2022 at 10:19:50AM IST, Kumar Kartikeya Dwivedi wrote:

On Fri, Jan 14, 2022 at 04:02:11AM IST, Alexei Starovoitov wrote:

quoted

On Tue, Jan 11, 2022 at 11:34:20PM +0530, Kumar Kartikeya Dwivedi wrote:

quoted

[...]
+	/* Make sure all updates are visible before we go to MODULE_STATE_LIVE,
+	 * pairs with smp_rmb in btf_try_get_module (for success case).
+	 *
+	 * btf_populate_kfunc_set(...)
+	 * smp_wmb()	<-----------.
+	 * mod->state = LIVE	    |		if (mod->state == LIVE)
+	 *			    |		  atomic_inc_nz(mod)
+	 *			    `--------->	  smp_rmb()
+	 *					  btf_kfunc_id_set_contains(...)
+	 */
+	smp_wmb();

This comment somehow implies that mod->state = LIVE
and if (mod->state == LIVE && try_mod_get) can race.
That's not the case.
The patch 1 closed the race.
btf_kfunc_id_set_contains() will be called only on LIVE modules.
At that point all __init funcs of the module including register_btf_kfunc_id_set()
have completed.
This smp_wmb/rmb pair serves no purpose.
Unless I'm missing something?

Right, I'm no expert on memory ordering, but even if we closed the race, to me
there seems to be no reason why the CPU cannot reorder the stores to tab (or its
hook/type slot) with mod->state = LIVE store.

Usually, the visibility is handled by whatever lock is used to register the
module somewhere in some subsystem, as the next acquirer can see all updates
from the previous registration.

In this case, we're directly assigning a pointer without holding any locks etc.
While it won't be concurrently accessed until module state is LIVE, it is
necessary to make all updates visible in the right order (that is, once state is
LIVE, everything stored previously in struct btf for module is also visible).

Once mod->state = LIVE is visible, we will start accessing kfunc_set_tab, but if
previous stores to it were not visible by then, we'll access a badly-formed
kfunc_set_tab.

For this particular case, you can think of mod->state = LIVE acting as a release
store, and the read for mod->state == LIVE acting as an acquire load.

Also, to be more precise, we're now synchronizing using btf_mod->flags, not
mod->state, so I should atleast update the comment, but the idea is the same.

But I'm probably being overtly cautious, please let me know why.

--
Kartikeya