Why can't this or these check(s) be done after callout_lock(), when we know c_cpu is valid?

You should probably also add a check for CPU_ABSENT() like CPU_FOREACH() does.

Can you refactor this code a bit and use "cc_cce_cleanup()". I suggest you move "cc_exec_curr(cc, direct) = NULL;" out of cc_cce_cleanup() because the places where it should be NULL, is already NULL'ed and use cc_cce_cleanup() instead of C&P'ing the code.

--HPS

Can we avoid adding this variable and instead check if the c_cpu == CPUBLOCK and then don't override c_cpu ?

There is no need, if the CPU is absent it would not be INIT'd i.e. the init var would be 0.

I don't think that would work since CPUBLOCK gives us special locking characteristics and we
need it set to single gate the lockers during migration.

What if a callout clients set an absent CPU value?

If the callout is being migrated, then surely we should not set the CPU number back to the old one, by means of l_cpu? I think it would make more sense to use the new one, I.E. ignore a CPU change upon cpu == CPUBLOCK.

They can't.. if the CPU is not initialized then
you would hit the else of line 970.

I will have this in the next patch.. I like that idea and had
missed the fact that we NULL cc_exec after we are done so
we can take that out .. of course we have to add it to the init routine.

This really becomes a toss up. You either go back to the old one or go
to the new one.. and of course its only in effect *if* the CPU passed
in is invalid. So you are talking about a corner of a corner case

1. The cpu passed by the callout_reset_on() is invalid

and

2. The CPU is currently migrating.

What do you do?

Leave it alone? i.e. make the cpu passed be equal to the new target CPU

or

migrate back.

Either choice is probably not want the caller wanted.. they wanted a different
CPU then either of those options.

So I don't really think it matters IMO.

OK, I see, and have you checked that timeout_cpu is valid, which sets up cc_inited? It is a tunable?

Hi,

In the cases where c->c_cpu is passed, it is obvious, the driver wants the current or last selected CPU. Only then we can have CPUBLOCK passed.

In the other cases where the driver gives a specific CPU, CPUBLOCK will not be passed, simply.

--HPS

s/cc_inited/timeout_cpu/

Move these checks after the callout_lock() just below when c_cpu is stable, then use "cpu = c->c_cpu" instead of "cpu = c->l_cpu" and eliminate the l_cpu variable ??

No you can *not* do that. I don't see what you are
so worried about here. If the caller specifies an invalid CPU
then you will crash when you try to lock an invalid non-init'd mutex.
These tests are catching two things, the migrating CPU but also that
a caller is *not* specifying an invalid CPU.

The l_cpu variable adds one uint32_t.. and I took out a uint32_t when
I moved the cc_exec_next out of the array.. so overall there is *no* change
in size as compared to what was there previously (discounting holes of course
not sure where those are in this structure).

No its set as it always as been when the O/S starts.. the timeout_cpu is always
CPU 0, and I don't see no reason to change that. We could, I suppose, have a sysctl
that could allow changing it.. just a matter of adding a small function to take in the
timeout CPU, assure its valid and then set it.. But I won't do that has part of this patch
since this is only fixing "issues".. and thats a feature.

Hi,
Maybe I misunderstand something.

The "l_cpu" field you added is per callout. cc_exec_next() is per CPU. We add 4 bytes to each callout everywhere in the kernel to catch a race the probably will not happen that frequently.

Did you misplace l_cpu, intending to put it into the cc_exec structure?

--HPS

Nope its not in the wrong place.. I just was not thinking.. it can't be in cc_exec it has
to be in the callout structure.. So yes it adds 4 bytes.. for a race that does not often
happen or, besides the race, a case where the caller specifies:

callout_reset_sbt_on(... , cpu=N) where N is below MAXCPU but is an *invalid* cpu.

This checks server two purposes and if the caller specifies a wrong CPU (or you hit
the race) then you would be toast if you continue.

I don't see how the locks would work either without this. You need to let cpu become
CPUBLOCK in order to assure the case where a race spins waiting for the cpu switch
to complete.

If you want to protect against all conditions you need the variable..

why no CC_LOCK(cc) here?

Well what happens when one does a callout_lock(c) is you are
actually doing a CC_LOCK().. but gated so that two thread do
*not* do a callout_cpu_switch() on the same c at the same time.

Its possible that c->c_cpu is CPUBLOCK, this indicates to the caller
of callout_lock() .. spin don't lock since the CC_LOCK is changing to
a new CPU..

Its a very subtle but needed thing. I am still not convinced that this
is a better mechanism then splitting the flags in two, I will run this under
a heavy load and compare the results with running it with a split flag.

The extra lock's unlocks may be in the noise factor.. or they may stand out.

Eh, I would just panic on an invalid CPU. Guessing what valid CPU to use when someone has made a mistake is just hiding their bug. It also simplifies your handling somewhat.

That CPUBLOCK gets passed in is most unfortunate. :)

OTOH, when that happens, you know that the caller isn't trying to migrate the callout and that it is passing in c->c_cpu, so you could depend on that and just use regular callout_lock(), yes? (In fact, this argues for the non-*on() methods actually passing a cookie to say "don't move me" rather than accesssing c_cpu directly since in some cases they may "undo" a migration.) Given that, I think you don't need l_cpu at all. When you see CPUBLOCK, just ignore it. If you fix that, I would probably then change the non-on methods to always pass CPUBLOCK to fix the other issue of undoing a migration.