Re: [RFC PATCH 2/4] Rename lock_cpu_hotplug to get_online_cpus


Okay. But other than pseries_add_processor and pseries_remove_processor,
are there any other places where we _change_ the cpu_present_map ?

Other arch code e.g. ia64 changes it for add/remove also. But I fail
to see how it matters.


I agree that we need some kind of synchronization for threads which
read the cpu_present_map. But probably we can use a seperate mutex
for that.

That would be needless complexity.


The naming is a problem IMO for two reasons:

- lock_cpu_hotplug() protects cpu_present_map as well as
cpu_online_map (sigh, I see that Documentation/cpu-hotplug.txt
disagrees with me, but my statement holds for powerpc, at least).

- get_online_cpus() implies reference count semantics (as stated in
the changelog) but AFAICT it really has a reference count
implementation with read-write locking semantics.

Hmm, I think there's another problem here. With your changes, code
which relies on the mutual exclusion behavior of lock_cpu_hotplug()
(such as pseries_add/remove_processor) will now be able to run
concurrently. Probably those functions should use
cpu_hotplug_begin/end instead.

One of the primary reasons to move away from the mutex semantics for
cpu-hotplug protection, was that there were a lot of places where
lock_cpu_hotplug() was used for protecting local data structures too,
when they had nothing to do with cpu-hotplug as such, and it resulted
in a whole mess. It took people quite sometime to sort things out
with the cpufreq subsystem.

cpu_present_map isn't a "local data structure" any more than
cpu_online_map, and it is quite relevant to cpu hotplug. We have to
maintain the invariant that the set of cpus online is a subset of cpus
present.

Probably it would be a lot cleaner if we use get_online_cpus() for
protection against cpu-hotplug and use specific mutexes for serializing
accesses to local data structures. Thoughts?

I don't feel like I'm getting through here. Let me restate.

If I'm reading them correctly, these patches are changing the behavior
of lock_cpu_hotplug() from mutex-style locking to a kind of read-write
locking. I think that's fine, but the naming of the new API poorly
reflects its real behavior. Conversion of lock_cpu_hotplug() users
should be done with care. Most of them - those that need one of the
cpu maps to remain unchanged during a critical section - can be
considered readers. But a few (such as pseries_add_processor() and
pseries_remove_processor()) are writers, because they modify one of
the maps.

So, why not:

get_cpus_online -> cpumaps_read_lock
put_cpus_online -> cpumaps_read_unlock
cpu_hotplug_begin -> cpumaps_write_lock
cpu_hotplug_end -> cpumaps_write_unlock

Or something similar?

Hi Nathan,
I totally see your point and agree with it.

However, though the new implementation appears to have
a read-write-semaphore behaviour it differs in
following aspects:

a) This implementation allows natural recursion of reads, just
as in the case of preempt_count.
This is not possible in case of a normal read-write
lock because if you have something like the following in
a timeline,
R1 -- > R2--> W3 --> R1, where
Ri is a read by a thread i and
Wi is a write by a thread i,
then thread1 will deadlock as it will be blocked behind W3,
holding the semaphore.

b) Regular Reader Writer semaphores are fair. As in if a new reader
arrives when a writer is already present, the new reader waits until
the writer in front of it finishes. But, in the new implementation,
the new reader will proceed as long as the refcount is non-zero, and the
writer will get to run only when the number of readers = 0. However, the
readers which arrive when the writer is executing, will block until the
writer is done.

Because of these properties, the implementation is more similar to the
refcounting, except that it allows the new bunch of readers to
sleep during an ongoing write.

Like you pointed out, since the code of pseries_add_processor and
other places where the cpu_present_map is being changed during the
runtime requires write protection, how if the
cpu_hotplug_begin/cpu_hotplug_end support is made
available outside cpu.c and appropriately named ?

Personally I would propose the following names to reflect the behaviour,
but if the community is okay with the word "lock" in the API's I don't
have any problems renaming them to what you've suggested.

/*
* Api's which ensure that the cpu_present_map and the cpu_online_map
* do not change while operating in a critical section.
*/

get_cpu_maps();
put_cpu_maps();

/*
* Api's to serialize the updates to the cpu_present_map/cpu_online_map.
*/

cpu_map_update_begin();
cpu_map_update_done();

Thanks and Regards
gautham.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • [RFC PATCH] Fair low-latency rwlock v3
    ... same CPU. ... Using a writer subscription to the rwlock to make sure the readers stop taking ... periodical interrupt readers on 7/8 cpus. ...
    (Linux-Kernel)
  • [RT PATCH v2] seqlock: serialize against writers
    ... This fixes a nasty deadlock on my systems under ... Seqlocks have always advertised that readers do not "block", ... the readers to serialize with the writers under contention. ... and it will pi-boost the writer to prevent inversion. ...
    (Linux-Kernel)
  • Re: [RT PATCH v3] seqlock: serialize against writers
    ... Problem 1) If A spins in seqbegin due to writer contention retries, ... Problem 3) Lower priority tasks such as C may continually acquire the write ... a rwlock for the basic spinlock previously used, and forces the readers ... unsigned ret; ...
    (Linux-Kernel)
  • [RT PATCH v3] seqlock: serialize against writers
    ... Problem 1) If A spins in seqbegin due to writer contention retries, ... Problem 3) Lower priority tasks such as C may continually acquire the write ... a rwlock for the basic spinlock previously used, and forces the readers ... unsigned ret; ...
    (Linux-Kernel)
  • Re: [PATCH 3/3] ring-buffer: make cpu buffer entries counter atomic
    ... that's not really good as atomics can be rather expensive and ... The race is with on CPU not other CPUs, and on archs like x86 there ... A writer can subtract them if it overwrites a page before the producer ... Only the writers are pinned to a CPU, the readers happen on any CPU. ...
    (Linux-Kernel)