Re: [PATCH] vt: Make SAK run in process context.

From: Andrew Morton
Date: Mon Dec 11 2006 - 15:57:25 EST


On Mon, 11 Dec 2006 06:07:03 -0700
ebiederm@xxxxxxxxxxxx (Eric W. Biederman) wrote:

>
> This defers SAK so we can use the normal console semaphore to order
> operations.
>
> This removes the xchg operations that I used to attempt to attmically
> update struct pid, because of the strange locking used for SAK. With
> SAK using the normal console semaphore nothing special is needed.
>

This is all a bit smelly.

>
> diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
> index 7a6c1c0..bd6912d 100644
> --- a/drivers/char/keyboard.c
> +++ b/drivers/char/keyboard.c
> @@ -595,15 +595,10 @@ static void fn_spawn_con(struct vc_data *vc)
>
> static void fn_SAK(struct vc_data *vc)
> {
> - struct tty_struct *tty = vc->vc_tty;
> + static DECLARE_WORK(SAK_work, deferred_SAK, NULL);
> + SAK_work.data = &vc_cons[fg_console];

static storage...

> static void sysrq_handle_SAK(int key, struct tty_struct *tty)
> {
> - if (tty)
> - do_SAK(tty);
> - reset_vc(vc_cons[fg_console].d);
> + static DECLARE_WORK(SAK_work, deferred_SAK, NULL);

More.

>
> +void deferred_SAK(void *data)
> +{
> + struct vc *vc_con = data;
> + struct vc_data *vc;
> + struct tty_struct *tty;
> +
> + acquire_console_sem();
> + vc = vc_con->d;
> + if (vc) {
> + tty = vc->vc_tty;
> + /*
> + * SAK should also work in all raw modes and reset
> + * them properly.
> + */
> + if (tty)
> + do_SAK(tty);
> + reset_vc(vc);
> + }
> + release_console_sem();
> +}

And a workqueue callback which calls a function which immediately does
another schedule_work().

I suspect you can fix all of this by passing a function pointer into
do_SAK(): to either __do_SAK or to some new function which does the vc
lookup then calls __do_SAK().

It probably means that you'll need to pass some payload into the workqueue
callback, and dhowells just went and broke that on us. That can be fixed
by adding a new `void *tty_struct.SAK_work_data'.


hmm, do_SAK() is being a bit bad, overwriting the ->SAK_work on a
work_struct which might presently be scheduled. To do this safely we need
a new variant on queue_work():

int queue_work_with_data(struct workqueue_struct *wq,
struct work_struct *work, void **datap, void *data
{
int ret = 0, cpu = get_cpu();

if (!test_and_set_bit(WORK_STRUCT_PENDING, &work->management)) {
if (datap)
*datap = data;
if (unlikely(is_single_threaded(wq)))
cpu = singlethread_cpu;
BUG_ON(!list_empty(&work->entry));
__queue_work(per_cpu_ptr(wq->cpu_wq, cpu), work);
ret = 1;
}
put_cpu();
return ret;
}

then, of course,

int queue_work(struct workqueue_struct *wq, struct work_struct *work)
{
return queue_work_with_data(wq, work, NULL, NULL);
}

iirc, other places in the kernel need queue_work_with_data(), for removal
of the *_WORK_NAR() stuff.
-
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/