On Wed, 20 Dec 2006 19:31:01 -0500
Mathieu Desnoyers <[EMAIL PROTECTED]> wrote:
> Hi,
>
> Here is a patch, result of the combined work of Tom Zanussi and myself, to add
> CPU hotplug support to Relay.
>
> ...
>
> +
> + lock_cpu_hotplug();
> + for_each_online_cpu(i)
> + if (chan->buf[i])
> + __relay_reset(chan->buf[i], 0);
> + unlock_cpu_hotplug();
__relay_reset() runs flush_scheduled_work(). If one of the queued works
takes lock_cpu_hoplug() (and some do), thou art most deadest meat.
I think the core design problem you have here is that you are using
cpu_online_map to record the presence or absence of resources which belong
to the relay driver. Why do that - you don't own cpu_online_map (but you
do get some notifications when it wants to change, that's all).
Perhaps a better approach would be to teach the relay driver to maintain
its own resources (already there, in chan->buf[]). So relay.c has a record
of which per-cpu buffers are present and which are not. That information
gets changed under a lock which the relay driver owns and controls.
You already have such a lock: relay_channels_mutex. So some code in here
is using lock_cpu_hotplug() to protect relay's resources while other code
is using relay_channels_mutex. Which is it?
Your proposed change apparently chooses to not release per-cpu resources on
cpu-hotunplug. I think. That's the sort of thing which should be
communicated in the (presently non-existent) patch changelog.
The changelog should also tell us *why* this patch was written. Right now
it's in "why on earth should we merge this" territory.
Meanwhile, let's shrink 10% off of relay.o's .text, shall we?
--- a/kernel/relay.c~relay-remove-inlining
+++ a/kernel/relay.c
@@ -322,7 +322,7 @@ static void wakeup_readers(struct work_s
*
* See relay_reset for description of effect.
*/
-static inline void __relay_reset(struct rchan_buf *buf, unsigned int init)
+static void __relay_reset(struct rchan_buf *buf, unsigned int init)
{
size_t i;
@@ -418,7 +418,7 @@ static struct rchan_buf *relay_open_buf(
* The channel buffer and channel buffer data structure are then freed
* automatically when the last reference is given up.
*/
-static inline void relay_close_buf(struct rchan_buf *buf)
+static void relay_close_buf(struct rchan_buf *buf)
{
buf->finalized = 1;
cancel_delayed_work(&buf->wake_readers);
@@ -426,7 +426,7 @@ static inline void relay_close_buf(struc
kref_put(&buf->kref, relay_remove_buf);
}
-static inline void setup_callbacks(struct rchan *chan,
+static void setup_callbacks(struct rchan *chan,
struct rchan_callbacks *cb)
{
if (!cb) {
@@ -946,11 +946,10 @@ typedef int (*subbuf_actor_t) (size_t re
/*
* relay_file_read_subbufs - read count bytes, bridging subbuf boundaries
*/
-static inline ssize_t relay_file_read_subbufs(struct file *filp,
- loff_t *ppos,
- subbuf_actor_t subbuf_actor,
- read_actor_t actor,
- read_descriptor_t *desc)
+static ssize_t relay_file_read_subbufs(struct file *filp, loff_t *ppos,
+ subbuf_actor_t subbuf_actor,
+ read_actor_t actor,
+ read_descriptor_t *desc)
{
struct rchan_buf *buf = filp->private_data;
size_t read_start, avail;
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
