Messages in this thread | | | Date | Thu, 25 Jan 2007 19:21:41 +0300 | From | Oleg Nesterov <> | Subject | Re: + aio-completion-signal-notification.patch added to -mm tree |
| |
Sebastien Dugue wrote: > > +static long aio_setup_sigevent(struct aio_notify *notify, > + struct sigevent __user *user_event) > +{ > + sigevent_t event; > + struct task_struct *target; > + > + if (copy_from_user(&event, user_event, sizeof(event))) > + return -EFAULT; > + > + if (event.sigev_notify == SIGEV_NONE) > + return 0; > + > + notify->notify = event.sigev_notify; > + notify->signo = event.sigev_signo; > + notify->value = event.sigev_value; > + > + read_lock(&tasklist_lock);
We don't need tasklist_lock, we can use rcu_read_lock() instead.
> + target = good_sigevent(&event); > + > + if (unlikely(!target || (target->flags & PF_EXITING))) > + goto out_unlock;
PF_EXITING check is racy and unneded. In fact, it is wrong. If the main thread is already died, we can only use SIGEV_THREAD_ID signals, because otherwise good_sigevent() returns ->group_leader.
> @@ -994,6 +1077,15 @@ int fastcall aio_complete(struct kiocb * > kunmap_atomic(ring, KM_IRQ1); > > pr_debug("added to ring %p at [%lu]\n", iocb, tail); > + > + if (iocb->ki_notify.notify != SIGEV_NONE) { > + ret = aio_send_signal(&iocb->ki_notify); > + > + /* If signal generation failed, release the sigqueue */ > + if (ret) > + sigqueue_free(iocb->ki_notify.sigq);
We should not use sigqueue_free() here. It takes current->sighand->siglock to remove sigqueue from "struct sigpending". But current is just a "random" process here.
Yes, if I understand this patch correctly, it is not possible that this sigqueue is pending, but still this is bad imho.
> static void __sigqueue_free(struct sigqueue *q) > { > - if (q->flags & SIGQUEUE_PREALLOC) > + if (q->flags & SIGQUEUE_PREALLOC && q->info.si_code != SI_ASYNCIO) > return;
Oh, this is not nice. Could we change send_sigqueue/send_group_sigqueue instead ?
- BUG_ON(!(q->flags & SIGQUEUE_PREALLOC)); + BUG_ON(!(q->flags & SIGQUEUE_PREALLOC) && q->info.si_code != SI_ASYNCIO);
This way aio can use __sigqueue_alloc/__sigqueue_free directly and forget about SIGQUEUE_PREALLOC.
On the other hand, imho this patch takes a wrong direction.
The purpose of SIGQUEUE_PREALLOC + send_sigqueue() is to re-use the same sigqueue while sending a stream of signals. But in aio case we allocate sigqueue to send only 1 signal, then it freed after the delivery like the regular sigqueue. So what is the point?
I'd suggest to not use this interface. Just use group_send_sig_info() or specific_send_sig_info(). Yes, this way we will do GFP_ATOMIC allocation of sigqueue in interrupt context, but is this so bad in this case?
Oleg.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |