Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Tue, 20 Mar 2007, Oleg Nesterov wrote:
> On 03/20, Oleg Nesterov wrote:
> >
> > On 03/19, Davide Libenzi wrote:
> > >
> > > I'd need a get_task_struct in any case in order to safely call
> > > unlock_task_sighand(). At that point I'd prefer to just pass through the
> > > struct pid*. I'll be posting the new version for review as soon as I
> > > complete a few tests ...
> >
> > If signalfd_get_sighand()->lock_task_sighand() succeeds, it is safe to
> > dereference ctx->tsk. The task can't be freed and ctx->tsk can't be cleared
> > while we are holding siglock.
> >
> > However, I was wrong, we still need a re-check after lock_task_sighand().
> > We should check ctx->tsk != NULL.
>
> IOW, we can (afaics) do
>
> static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx
> *ctx,
> unsigned long *flags)
> {
> struct sighand_struct *sighand = NULL;
> struct tsak_struct *tsk;
>
> rcu_read_lock();
> tsk = rcu_dereference(ctx->tsk); // not needed, just a
> documentation
> if (tsk != NULL)
> sighand = lock_task_sighand(tsk, flags);
> rcu_read_unlock();
>
> if (sighand && !ctx->tsk)) {
> unlock_task_sighand(tsk, flags);
> sighand = NULL;
> }
>
> return sighand;
> }
>
> If signalfd_get_sighand() succeeds, ctx->tsk is pinned.
I did a similar thing, but I renamed the locking functions and its
parameters. After looking at what the pid thing was doing, I realized that
it was not really needed.
- Davide
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On 03/20, Oleg Nesterov wrote:
>
> On 03/19, Davide Libenzi wrote:
> >
> > I'd need a get_task_struct in any case in order to safely call
> > unlock_task_sighand(). At that point I'd prefer to just pass through the
> > struct pid*. I'll be posting the new version for review as soon as I
> > complete a few tests ...
>
> If signalfd_get_sighand()->lock_task_sighand() succeeds, it is safe to
> dereference ctx->tsk. The task can't be freed and ctx->tsk can't be cleared
> while we are holding siglock.
>
> However, I was wrong, we still need a re-check after lock_task_sighand().
> We should check ctx->tsk != NULL.
IOW, we can (afaics) do
static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx
*ctx,
unsigned long *flags)
{
struct sighand_struct *sighand = NULL;
struct tsak_struct *tsk;
rcu_read_lock();
tsk = rcu_dereference(ctx->tsk); // not needed, just a
documentation
if (tsk != NULL)
sighand = lock_task_sighand(tsk, flags);
rcu_read_unlock();
if (sighand && !ctx->tsk)) {
unlock_task_sighand(tsk, flags);
sighand = NULL;
}
return sighand;
}
If signalfd_get_sighand() succeeds, ctx->tsk is pinned.
Oleg.
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On 03/19, Davide Libenzi wrote:
>
> On Mon, 19 Mar 2007, Oleg Nesterov wrote:
>
> > On 03/19, Davide Libenzi wrote:
> > >
> > > On Mon, 19 Mar 2007, Eric W. Biederman wrote:
> > >
> > > > > +struct signalfd_ctx {
> > > > > + struct list_head lnk;
> > > > > + wait_queue_head_t wqh;
> > > > > + sigset_t sigmask;
> > > > > + struct task_struct *tsk;
> > > > > +};
> > > >
> > > > I think you want to use a struct pid *pid instead of a pointer to the
> > > > task struct here. It is slightly less efficient (one more
> > > > dereference) but it means that we won't pin the task struct in memory
> > > > indefinitely. Pinning the task_struct like this makes for a very
> > > > interesting way to get around the limits on the number of processes a
> > > > user can have.
> > >
> > > Hmm, when the task is detached from the sighand, we get a notify, so I
> > > could do a put from there. This would avoid the extra de-reference. I
> > > need
> > > to verify locking though ...
> >
> > In that case (if I understand you correctly) we don't need
> > {get,put}_task_struct()
> > at all.
> >
> > signalfd_deliver(-1) sets ctx->tsk = NULL, signalfd_get_sighand() reads
> > ->tsk
> > under rcu_read_lock(). The code becomes even simpler, we don't need to check
> > list_empty(>lnk).
>
> I'd need a get_task_struct in any case in order to safely call
> unlock_task_sighand(). At that point I'd prefer to just pass through the
> struct pid*. I'll be posting the new version for review as soon as I
> complete a few tests ...
No.
First, you don't need to use unlock_task_sighand(), you can just use
spin_unlock_irqrestore(sighand) directly. But this is, I agree, not good.
If signalfd_get_sighand()->lock_task_sighand() succeeds, it is safe to
dereference ctx->tsk. The task can't be freed and ctx->tsk can't be cleared
while we are holding siglock.
However, I was wrong, we still need a re-check after lock_task_sighand().
We should check ctx->tsk != NULL.
Oleg.
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Mon, 19 Mar 2007, Oleg Nesterov wrote:
> On 03/19, Davide Libenzi wrote:
> >
> > On Mon, 19 Mar 2007, Eric W. Biederman wrote:
> >
> > > > +struct signalfd_ctx {
> > > > + struct list_head lnk;
> > > > + wait_queue_head_t wqh;
> > > > + sigset_t sigmask;
> > > > + struct task_struct *tsk;
> > > > +};
> > >
> > > I think you want to use a struct pid *pid instead of a pointer to the
> > > task struct here. It is slightly less efficient (one more
> > > dereference) but it means that we won't pin the task struct in memory
> > > indefinitely. Pinning the task_struct like this makes for a very
> > > interesting way to get around the limits on the number of processes a
> > > user can have.
> >
> > Hmm, when the task is detached from the sighand, we get a notify, so I
> > could do a put from there. This would avoid the extra de-reference. I need
> > to verify locking though ...
>
> In that case (if I understand you correctly) we don't need
> {get,put}_task_struct()
> at all.
>
> signalfd_deliver(-1) sets ctx->tsk = NULL, signalfd_get_sighand() reads ->tsk
> under rcu_read_lock(). The code becomes even simpler, we don't need to check
> list_empty(>lnk).
I'd need a get_task_struct in any case in order to safely call
unlock_task_sighand(). At that point I'd prefer to just pass through the
struct pid*. I'll be posting the new version for review as soon as I
complete a few tests ...
- Davide
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On 03/19, Davide Libenzi wrote:
>
> On Mon, 19 Mar 2007, Eric W. Biederman wrote:
>
> > > +struct signalfd_ctx {
> > > + struct list_head lnk;
> > > + wait_queue_head_t wqh;
> > > + sigset_t sigmask;
> > > + struct task_struct *tsk;
> > > +};
> >
> > I think you want to use a struct pid *pid instead of a pointer to the
> > task struct here. It is slightly less efficient (one more
> > dereference) but it means that we won't pin the task struct in memory
> > indefinitely. Pinning the task_struct like this makes for a very
> > interesting way to get around the limits on the number of processes a
> > user can have.
>
> Hmm, when the task is detached from the sighand, we get a notify, so I
> could do a put from there. This would avoid the extra de-reference. I need
> to verify locking though ...
In that case (if I understand you correctly) we don't need
{get,put}_task_struct()
at all.
signalfd_deliver(-1) sets ctx->tsk = NULL, signalfd_get_sighand() reads ->tsk
under rcu_read_lock(). The code becomes even simpler, we don't need to check
list_empty(>lnk).
Oleg.
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
> Davide Libenzi writes:
>
> > On Mon, 19 Mar 2007, Eric W. Biederman wrote:
> >
> >> Davide Libenzi writes:
> >>
> >> > struct signalfd_siginfo {
> >> > __u32 signo;/* si_signo */
> >> > __s32 err; /* si_errno */
> >> > __s32 code; /* si_code */
> >> > __u32 pid; /* si_pid */
> >> > __u32 uid; /* si_uid */
> >> > __s32 fd; /* si_fd */
> >> > __u32 tid; /* si_fd */
> >> > __u32 band; /* si_band */
> >> > __u32 overrun; /* si_overrun */
> >> > __u32 trapno; /* si_trapno */
> >> > __s32 status; /* si_status */
> >> > __s32 svint;/* si_int */
> >> > __u64 svptr;/* si_ptr */
> >> > __u64 utime;/* si_utime */
> >> > __u64 stime;/* si_stime */
> >> > __u64 addr; /* si_addr */
> >> > };
> >>
> >> Shouldn't we pad this to 128 bytes like we do siginfo in case there are
> >> more fields we need to include, or we need to extend the size of some
> >> field?
> >
> > Yes, I guess we can.
>
> I'm just a little paranoid about ABI's. There is always something
> that crops up. And while we can probably cope by simply having another
> version of the signalfd or whatever your syscall is, but having to do
> that at the first sign of trouble sucks. Especially since we would have
> to maintain two versions indefinitely.
Ok, I added the padding to 128 bytes to the struct.
> >> I think you want to use a struct pid *pid instead of a pointer to the
> >> task struct here. It is slightly less efficient (one more
> >> dereference) but it means that we won't pin the task struct in memory
> >> indefinitely. Pinning the task_struct like this makes for a very
> >> interesting way to get around the limits on the number of processes a
> >> user can have.
> >
> > Hmm, when the task is detached from the sighand, we get a notify, so I
> > could do a put from there. This would avoid the extra de-reference. I need
> > to verify locking though ...
>
> Ok. That sounds more efficient than playing with struct pid pointers,
> if it works.
I'm looking into this right now ...
- Davide
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
Davide Libenzi writes:
> On Mon, 19 Mar 2007, Eric W. Biederman wrote:
>
>> Davide Libenzi writes:
>>
>> > struct signalfd_siginfo {
>> >__u32 signo;/* si_signo */
>> >__s32 err; /* si_errno */
>> >__s32 code; /* si_code */
>> >__u32 pid; /* si_pid */
>> >__u32 uid; /* si_uid */
>> >__s32 fd; /* si_fd */
>> >__u32 tid; /* si_fd */
>> >__u32 band; /* si_band */
>> >__u32 overrun; /* si_overrun */
>> >__u32 trapno; /* si_trapno */
>> >__s32 status; /* si_status */
>> >__s32 svint;/* si_int */
>> >__u64 svptr;/* si_ptr */
>> >__u64 utime;/* si_utime */
>> >__u64 stime;/* si_stime */
>> >__u64 addr; /* si_addr */
>> > };
>>
>> Shouldn't we pad this to 128 bytes like we do siginfo in case there are
>> more fields we need to include, or we need to extend the size of some
>> field?
>
> Yes, I guess we can.
I'm just a little paranoid about ABI's. There is always something
that crops up. And while we can probably cope by simply having another
version of the signalfd or whatever your syscall is, but having to do
that at the first sign of trouble sucks. Especially since we would have
to maintain two versions indefinitely.
>> > +
>> > +struct signalfd_ctx {
>> > + struct list_head lnk;
>> > + wait_queue_head_t wqh;
>> > + sigset_t sigmask;
>> > + struct task_struct *tsk;
>> > +};
>>
>> I think you want to use a struct pid *pid instead of a pointer to the
>> task struct here. It is slightly less efficient (one more
>> dereference) but it means that we won't pin the task struct in memory
>> indefinitely. Pinning the task_struct like this makes for a very
>> interesting way to get around the limits on the number of processes a
>> user can have.
>
> Hmm, when the task is detached from the sighand, we get a notify, so I
> could do a put from there. This would avoid the extra de-reference. I need
> to verify locking though ...
Ok. That sounds more efficient than playing with struct pid pointers,
if it works.
Eric
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
> Davide Libenzi writes:
>
> > struct signalfd_siginfo {
> > __u32 signo;/* si_signo */
> > __s32 err; /* si_errno */
> > __s32 code; /* si_code */
> > __u32 pid; /* si_pid */
> > __u32 uid; /* si_uid */
> > __s32 fd; /* si_fd */
> > __u32 tid; /* si_fd */
> > __u32 band; /* si_band */
> > __u32 overrun; /* si_overrun */
> > __u32 trapno; /* si_trapno */
> > __s32 status; /* si_status */
> > __s32 svint;/* si_int */
> > __u64 svptr;/* si_ptr */
> > __u64 utime;/* si_utime */
> > __u64 stime;/* si_stime */
> > __u64 addr; /* si_addr */
> > };
>
> Shouldn't we pad this to 128 bytes like we do siginfo in case there are
> more fields we need to include, or we need to extend the size of some
> field?
Yes, I guess we can.
> I'm tempted to suggest we have a per arch function that tests current
> to see if we are in a compat process or not so we can just use
> siginfo. But that is probably overkill.
>
> > +
> > +
> > +struct signalfd_ctx {
> > + struct list_head lnk;
> > + wait_queue_head_t wqh;
> > + sigset_t sigmask;
> > + struct task_struct *tsk;
> > +};
>
> I think you want to use a struct pid *pid instead of a pointer to the
> task struct here. It is slightly less efficient (one more
> dereference) but it means that we won't pin the task struct in memory
> indefinitely. Pinning the task_struct like this makes for a very
> interesting way to get around the limits on the number of processes a
> user can have.
Hmm, when the task is detached from the sighand, we get a notify, so I
could do a put from there. This would avoid the extra de-reference. I need
to verify locking though ...
- Davide
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
Davide Libenzi writes:
> struct signalfd_siginfo {
> __u32 signo;/* si_signo */
> __s32 err; /* si_errno */
> __s32 code; /* si_code */
> __u32 pid; /* si_pid */
> __u32 uid; /* si_uid */
> __s32 fd; /* si_fd */
> __u32 tid; /* si_fd */
> __u32 band; /* si_band */
> __u32 overrun; /* si_overrun */
> __u32 trapno; /* si_trapno */
> __s32 status; /* si_status */
> __s32 svint;/* si_int */
> __u64 svptr;/* si_ptr */
> __u64 utime;/* si_utime */
> __u64 stime;/* si_stime */
> __u64 addr; /* si_addr */
> };
Shouldn't we pad this to 128 bytes like we do siginfo in case there are
more fields we need to include, or we need to extend the size of some
field?
I'm tempted to suggest we have a per arch function that tests current
to see if we are in a compat process or not so we can just use
siginfo. But that is probably overkill.
> +
> +
> +struct signalfd_ctx {
> + struct list_head lnk;
> + wait_queue_head_t wqh;
> + sigset_t sigmask;
> + struct task_struct *tsk;
> +};
I think you want to use a struct pid *pid instead of a pointer to the
task struct here. It is slightly less efficient (one more
dereference) but it means that we won't pin the task struct in memory
indefinitely. Pinning the task_struct like this makes for a very
interesting way to get around the limits on the number of processes a
user can have.
Eric
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
Davide Libenzi davidel@xmailserver.org writes:
struct signalfd_siginfo {
__u32 signo;/* si_signo */
__s32 err; /* si_errno */
__s32 code; /* si_code */
__u32 pid; /* si_pid */
__u32 uid; /* si_uid */
__s32 fd; /* si_fd */
__u32 tid; /* si_fd */
__u32 band; /* si_band */
__u32 overrun; /* si_overrun */
__u32 trapno; /* si_trapno */
__s32 status; /* si_status */
__s32 svint;/* si_int */
__u64 svptr;/* si_ptr */
__u64 utime;/* si_utime */
__u64 stime;/* si_stime */
__u64 addr; /* si_addr */
};
Shouldn't we pad this to 128 bytes like we do siginfo in case there are
more fields we need to include, or we need to extend the size of some
field?
I'm tempted to suggest we have a per arch function that tests current
to see if we are in a compat process or not so we can just use
siginfo. But that is probably overkill.
+
+
+struct signalfd_ctx {
+ struct list_head lnk;
+ wait_queue_head_t wqh;
+ sigset_t sigmask;
+ struct task_struct *tsk;
+};
I think you want to use a struct pid *pid instead of a pointer to the
task struct here. It is slightly less efficient (one more
dereference) but it means that we won't pin the task struct in memory
indefinitely. Pinning the task_struct like this makes for a very
interesting way to get around the limits on the number of processes a
user can have.
Eric
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
Davide Libenzi davidel@xmailserver.org writes:
struct signalfd_siginfo {
__u32 signo;/* si_signo */
__s32 err; /* si_errno */
__s32 code; /* si_code */
__u32 pid; /* si_pid */
__u32 uid; /* si_uid */
__s32 fd; /* si_fd */
__u32 tid; /* si_fd */
__u32 band; /* si_band */
__u32 overrun; /* si_overrun */
__u32 trapno; /* si_trapno */
__s32 status; /* si_status */
__s32 svint;/* si_int */
__u64 svptr;/* si_ptr */
__u64 utime;/* si_utime */
__u64 stime;/* si_stime */
__u64 addr; /* si_addr */
};
Shouldn't we pad this to 128 bytes like we do siginfo in case there are
more fields we need to include, or we need to extend the size of some
field?
Yes, I guess we can.
I'm tempted to suggest we have a per arch function that tests current
to see if we are in a compat process or not so we can just use
siginfo. But that is probably overkill.
+
+
+struct signalfd_ctx {
+ struct list_head lnk;
+ wait_queue_head_t wqh;
+ sigset_t sigmask;
+ struct task_struct *tsk;
+};
I think you want to use a struct pid *pid instead of a pointer to the
task struct here. It is slightly less efficient (one more
dereference) but it means that we won't pin the task struct in memory
indefinitely. Pinning the task_struct like this makes for a very
interesting way to get around the limits on the number of processes a
user can have.
Hmm, when the task is detached from the sighand, we get a notify, so I
could do a put from there. This would avoid the extra de-reference. I need
to verify locking though ...
- Davide
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
Davide Libenzi davidel@xmailserver.org writes:
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
Davide Libenzi davidel@xmailserver.org writes:
struct signalfd_siginfo {
__u32 signo;/* si_signo */
__s32 err; /* si_errno */
__s32 code; /* si_code */
__u32 pid; /* si_pid */
__u32 uid; /* si_uid */
__s32 fd; /* si_fd */
__u32 tid; /* si_fd */
__u32 band; /* si_band */
__u32 overrun; /* si_overrun */
__u32 trapno; /* si_trapno */
__s32 status; /* si_status */
__s32 svint;/* si_int */
__u64 svptr;/* si_ptr */
__u64 utime;/* si_utime */
__u64 stime;/* si_stime */
__u64 addr; /* si_addr */
};
Shouldn't we pad this to 128 bytes like we do siginfo in case there are
more fields we need to include, or we need to extend the size of some
field?
Yes, I guess we can.
I'm just a little paranoid about ABI's. There is always something
that crops up. And while we can probably cope by simply having another
version of the signalfd or whatever your syscall is, but having to do
that at the first sign of trouble sucks. Especially since we would have
to maintain two versions indefinitely.
+
+struct signalfd_ctx {
+ struct list_head lnk;
+ wait_queue_head_t wqh;
+ sigset_t sigmask;
+ struct task_struct *tsk;
+};
I think you want to use a struct pid *pid instead of a pointer to the
task struct here. It is slightly less efficient (one more
dereference) but it means that we won't pin the task struct in memory
indefinitely. Pinning the task_struct like this makes for a very
interesting way to get around the limits on the number of processes a
user can have.
Hmm, when the task is detached from the sighand, we get a notify, so I
could do a put from there. This would avoid the extra de-reference. I need
to verify locking though ...
Ok. That sounds more efficient than playing with struct pid pointers,
if it works.
Eric
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
Davide Libenzi davidel@xmailserver.org writes:
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
Davide Libenzi davidel@xmailserver.org writes:
struct signalfd_siginfo {
__u32 signo;/* si_signo */
__s32 err; /* si_errno */
__s32 code; /* si_code */
__u32 pid; /* si_pid */
__u32 uid; /* si_uid */
__s32 fd; /* si_fd */
__u32 tid; /* si_fd */
__u32 band; /* si_band */
__u32 overrun; /* si_overrun */
__u32 trapno; /* si_trapno */
__s32 status; /* si_status */
__s32 svint;/* si_int */
__u64 svptr;/* si_ptr */
__u64 utime;/* si_utime */
__u64 stime;/* si_stime */
__u64 addr; /* si_addr */
};
Shouldn't we pad this to 128 bytes like we do siginfo in case there are
more fields we need to include, or we need to extend the size of some
field?
Yes, I guess we can.
I'm just a little paranoid about ABI's. There is always something
that crops up. And while we can probably cope by simply having another
version of the signalfd or whatever your syscall is, but having to do
that at the first sign of trouble sucks. Especially since we would have
to maintain two versions indefinitely.
Ok, I added the padding to 128 bytes to the struct.
I think you want to use a struct pid *pid instead of a pointer to the
task struct here. It is slightly less efficient (one more
dereference) but it means that we won't pin the task struct in memory
indefinitely. Pinning the task_struct like this makes for a very
interesting way to get around the limits on the number of processes a
user can have.
Hmm, when the task is detached from the sighand, we get a notify, so I
could do a put from there. This would avoid the extra de-reference. I need
to verify locking though ...
Ok. That sounds more efficient than playing with struct pid pointers,
if it works.
I'm looking into this right now ...
- Davide
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On 03/19, Davide Libenzi wrote:
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
+struct signalfd_ctx {
+ struct list_head lnk;
+ wait_queue_head_t wqh;
+ sigset_t sigmask;
+ struct task_struct *tsk;
+};
I think you want to use a struct pid *pid instead of a pointer to the
task struct here. It is slightly less efficient (one more
dereference) but it means that we won't pin the task struct in memory
indefinitely. Pinning the task_struct like this makes for a very
interesting way to get around the limits on the number of processes a
user can have.
Hmm, when the task is detached from the sighand, we get a notify, so I
could do a put from there. This would avoid the extra de-reference. I need
to verify locking though ...
In that case (if I understand you correctly) we don't need
{get,put}_task_struct()
at all.
signalfd_deliver(-1) sets ctx-tsk = NULL, signalfd_get_sighand() reads -tsk
under rcu_read_lock(). The code becomes even simpler, we don't need to check
list_empty(ctx-lnk).
Oleg.
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Mon, 19 Mar 2007, Oleg Nesterov wrote:
On 03/19, Davide Libenzi wrote:
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
+struct signalfd_ctx {
+ struct list_head lnk;
+ wait_queue_head_t wqh;
+ sigset_t sigmask;
+ struct task_struct *tsk;
+};
I think you want to use a struct pid *pid instead of a pointer to the
task struct here. It is slightly less efficient (one more
dereference) but it means that we won't pin the task struct in memory
indefinitely. Pinning the task_struct like this makes for a very
interesting way to get around the limits on the number of processes a
user can have.
Hmm, when the task is detached from the sighand, we get a notify, so I
could do a put from there. This would avoid the extra de-reference. I need
to verify locking though ...
In that case (if I understand you correctly) we don't need
{get,put}_task_struct()
at all.
signalfd_deliver(-1) sets ctx-tsk = NULL, signalfd_get_sighand() reads -tsk
under rcu_read_lock(). The code becomes even simpler, we don't need to check
list_empty(ctx-lnk).
I'd need a get_task_struct in any case in order to safely call
unlock_task_sighand(). At that point I'd prefer to just pass through the
struct pid*. I'll be posting the new version for review as soon as I
complete a few tests ...
- Davide
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On 03/19, Davide Libenzi wrote:
On Mon, 19 Mar 2007, Oleg Nesterov wrote:
On 03/19, Davide Libenzi wrote:
On Mon, 19 Mar 2007, Eric W. Biederman wrote:
+struct signalfd_ctx {
+ struct list_head lnk;
+ wait_queue_head_t wqh;
+ sigset_t sigmask;
+ struct task_struct *tsk;
+};
I think you want to use a struct pid *pid instead of a pointer to the
task struct here. It is slightly less efficient (one more
dereference) but it means that we won't pin the task struct in memory
indefinitely. Pinning the task_struct like this makes for a very
interesting way to get around the limits on the number of processes a
user can have.
Hmm, when the task is detached from the sighand, we get a notify, so I
could do a put from there. This would avoid the extra de-reference. I
need
to verify locking though ...
In that case (if I understand you correctly) we don't need
{get,put}_task_struct()
at all.
signalfd_deliver(-1) sets ctx-tsk = NULL, signalfd_get_sighand() reads
-tsk
under rcu_read_lock(). The code becomes even simpler, we don't need to check
list_empty(ctx-lnk).
I'd need a get_task_struct in any case in order to safely call
unlock_task_sighand(). At that point I'd prefer to just pass through the
struct pid*. I'll be posting the new version for review as soon as I
complete a few tests ...
No.
First, you don't need to use unlock_task_sighand(), you can just use
spin_unlock_irqrestore(sighand) directly. But this is, I agree, not good.
If signalfd_get_sighand()-lock_task_sighand() succeeds, it is safe to
dereference ctx-tsk. The task can't be freed and ctx-tsk can't be cleared
while we are holding siglock.
However, I was wrong, we still need a re-check after lock_task_sighand().
We should check ctx-tsk != NULL.
Oleg.
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On 03/20, Oleg Nesterov wrote:
On 03/19, Davide Libenzi wrote:
I'd need a get_task_struct in any case in order to safely call
unlock_task_sighand(). At that point I'd prefer to just pass through the
struct pid*. I'll be posting the new version for review as soon as I
complete a few tests ...
If signalfd_get_sighand()-lock_task_sighand() succeeds, it is safe to
dereference ctx-tsk. The task can't be freed and ctx-tsk can't be cleared
while we are holding siglock.
However, I was wrong, we still need a re-check after lock_task_sighand().
We should check ctx-tsk != NULL.
IOW, we can (afaics) do
static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx
*ctx,
unsigned long *flags)
{
struct sighand_struct *sighand = NULL;
struct tsak_struct *tsk;
rcu_read_lock();
tsk = rcu_dereference(ctx-tsk); // not needed, just a
documentation
if (tsk != NULL)
sighand = lock_task_sighand(tsk, flags);
rcu_read_unlock();
if (sighand !ctx-tsk)) {
unlock_task_sighand(tsk, flags);
sighand = NULL;
}
return sighand;
}
If signalfd_get_sighand() succeeds, ctx-tsk is pinned.
Oleg.
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Tue, 20 Mar 2007, Oleg Nesterov wrote:
On 03/20, Oleg Nesterov wrote:
On 03/19, Davide Libenzi wrote:
I'd need a get_task_struct in any case in order to safely call
unlock_task_sighand(). At that point I'd prefer to just pass through the
struct pid*. I'll be posting the new version for review as soon as I
complete a few tests ...
If signalfd_get_sighand()-lock_task_sighand() succeeds, it is safe to
dereference ctx-tsk. The task can't be freed and ctx-tsk can't be cleared
while we are holding siglock.
However, I was wrong, we still need a re-check after lock_task_sighand().
We should check ctx-tsk != NULL.
IOW, we can (afaics) do
static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx
*ctx,
unsigned long *flags)
{
struct sighand_struct *sighand = NULL;
struct tsak_struct *tsk;
rcu_read_lock();
tsk = rcu_dereference(ctx-tsk); // not needed, just a
documentation
if (tsk != NULL)
sighand = lock_task_sighand(tsk, flags);
rcu_read_unlock();
if (sighand !ctx-tsk)) {
unlock_task_sighand(tsk, flags);
sighand = NULL;
}
return sighand;
}
If signalfd_get_sighand() succeeds, ctx-tsk is pinned.
I did a similar thing, but I renamed the locking functions and its
parameters. After looking at what the pid thing was doing, I realized that
it was not really needed.
- Davide
-
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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Mon, 19 Mar 2007, Arnd Bergmann wrote: > On Sunday 18 March 2007, Davide Libenzi wrote: > > bah, __put_user is basically a move, so I don't think that efficency would > > be that different (assuming that it'd matter in this case). The only thing > > many __put_user do, is increase the exception table sizes. > > The cost of user access functions varies a lot depending on the > architectures. Those platforms with a 4G/4G split e.g. need to do more > than a simple move, and for s390 it may even come down to an indirect > function call, which incurs significant register pressure. Heh, I'd like ppl to agree on this, because I clearly remember in having an argoument with Andrew for the same thing, where I was doing stack setup plus copy_to_user() ;) - Davide - 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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Sunday 18 March 2007, Davide Libenzi wrote: > bah, __put_user is basically a move, so I don't think that efficency would > be that different (assuming that it'd matter in this case). The only thing > many __put_user do, is increase the exception table sizes. The cost of user access functions varies a lot depending on the architectures. Those platforms with a 4G/4G split e.g. need to do more than a simple move, and for s390 it may even come down to an indirect function call, which incurs significant register pressure. Arnd <>< - 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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Sat, 17 Mar 2007, Arnd Bergmann wrote:
> > +asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t
> > sizemask) +{
> > + int error;
> > + unsigned long flags;
> > + sigset_t sigmask;
> > + struct signalfd_ctx *ctx;
> > + struct sighand_struct *sighand;
> > + struct file *file;
> > + struct inode *inode;
> > +
> > + error = -EINVAL;
> > + if (sizemask != sizeof(sigset_t) ||
> > + copy_from_user(, user_mask, sizeof(sigmask)))
> > + goto err_exit;
>
> sizeof(sigset_t) may be different for native and 32-bit compat code.
> It would be good if you could handle sizemask==4 && sizeof(sigset_t)==8
> in this code, so that there is no need for an extra compat_sys_signalfd
> function.
As Stephen reported, we do need the compat in any case. Better keep all
the compat adjustments under CONFIG_COMPAT, so archs that don't need it
don't need to link to it.
> > + if ((sighand = signalfd_get_sighand(ctx, )) != NULL) {
> > + if (next_signal(>tsk->pending, >sigmask) > 0 ||
> > + next_signal(>tsk->signal->shared_pending,
> > + >sigmask) > 0)
> > + events |= POLLIN;
> > + signalfd_put_sighand(ctx, sighand, );
> > + } else
> > + events |= POLLIN;
> > +
> > + return events;
> > +}
>
> I never really understood the events mask, but other subsystems often
> use (POLLIN | POLLRDNORM) instead of just POLLIN. Is there a reason
> for not returning POLLRDNORM here?
I don't think those fds will have to deal with the concept of bands and
priorities. I believe POLLIN is fine here.
> > +static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> > +siginfo_t const *kinfo)
> > +{
> > + long err;
> > +
> > + err = __clear_user(uinfo, sizeof(*uinfo));
> > +
> > + /*
> > +* If you change siginfo_t structure, please be sure
> > +* this code is fixed accordingly.
> > +*/
> > + err |= __put_user(kinfo->si_signo, >signo);
> > + err |= __put_user(kinfo->si_errno, >err);
> > + err |= __put_user((short)kinfo->si_code, >code);
> > + switch (kinfo->si_code & __SI_MASK) {
> > + case __SI_KILL:
> > + err |= __put_user(kinfo->si_pid, >pid);
> > + err |= __put_user(kinfo->si_uid, >uid);
> > + break;
> > + case __SI_TIMER:
> > +err |= __put_user(kinfo->si_tid, >tid);
> > +err |= __put_user(kinfo->si_overrun, >overrun);
> > +err |= __put_user(kinfo->si_ptr, >svptr);
> > + break;
> > + case __SI_POLL:
> > + err |= __put_user(kinfo->si_band, >band);
> > + err |= __put_user(kinfo->si_fd, >fd);
> > + break;
> > + case __SI_FAULT:
> > + err |= __put_user(kinfo->si_addr, >addr);
> > +#ifdef __ARCH_SI_TRAPNO
> > + err |= __put_user(kinfo->si_trapno, >trapno);
> > +#endif
> > + break;
> > + case __SI_CHLD:
> > + err |= __put_user(kinfo->si_pid, >pid);
> > + err |= __put_user(kinfo->si_uid, >uid);
> > + err |= __put_user(kinfo->si_status, >status);
> > + err |= __put_user(kinfo->si_utime, >utime);
> > + err |= __put_user(kinfo->si_stime, >stime);
> > + break;
> > + case __SI_RT: /* This is not generated by the kernel as of now. */
> > + case __SI_MESGQ: /* But this is */
> > + err |= __put_user(kinfo->si_pid, >pid);
> > + err |= __put_user(kinfo->si_uid, >uid);
> > + err |= __put_user(kinfo->si_ptr, >svptr);
> > + break;
> > + default: /* this is just in case for now ... */
> > + err |= __put_user(kinfo->si_pid, >pid);
> > + err |= __put_user(kinfo->si_uid, >uid);
> > + break;
> > + }
> > +
> > + return err ? -EFAULT: sizeof(*uinfo);
> > +}
>
> Doing it this way looks rather inefficient to me. I think it's
> better to just prepare the signalfd_siginfo on the stack and
> do a single copy_to_user.
bah, __put_user is basically a move, so I don't think that efficency would
be that different (assuming that it'd matter in this case). The only thing
many __put_user do, is increase the exception table sizes.
> Also, what's the reasoning behind defining a new structure
> instead of just returning siginfo_t? Sure siginfo_t is ugly
> but it is a well-defined structure and users already deal
> with the problems it causes.
Compat on sys_read() would be insane ;)
>
> > +static void __exit signalfd_exit(void)
> > +{
> > + kmem_cache_destroy(signalfd_ctx_cachep);
> > +}
> > +
> > +module_init(signalfd_init);
> > +module_exit(signalfd_exit);
> > +
> > +MODULE_LICENSE("GPL");
>
> Since this file defines a syscall, it can't be a module, so why bother
> with this?
Agreed, remove exit function and using fs_initcall.
>
> > +
> > +struct signalfd_siginfo {
> > + __u32 signo;
> > + __s32 err;
> > + __s32 code;
> > + __u32 pid;
> > + __u32 uid;
> > + __s32 fd;
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Sat, 17 Mar 2007, Arnd Bergmann wrote:
+asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t
sizemask) +{
+ int error;
+ unsigned long flags;
+ sigset_t sigmask;
+ struct signalfd_ctx *ctx;
+ struct sighand_struct *sighand;
+ struct file *file;
+ struct inode *inode;
+
+ error = -EINVAL;
+ if (sizemask != sizeof(sigset_t) ||
+ copy_from_user(sigmask, user_mask, sizeof(sigmask)))
+ goto err_exit;
sizeof(sigset_t) may be different for native and 32-bit compat code.
It would be good if you could handle sizemask==4 sizeof(sigset_t)==8
in this code, so that there is no need for an extra compat_sys_signalfd
function.
As Stephen reported, we do need the compat in any case. Better keep all
the compat adjustments under CONFIG_COMPAT, so archs that don't need it
don't need to link to it.
+ if ((sighand = signalfd_get_sighand(ctx, flags)) != NULL) {
+ if (next_signal(ctx-tsk-pending, ctx-sigmask) 0 ||
+ next_signal(ctx-tsk-signal-shared_pending,
+ ctx-sigmask) 0)
+ events |= POLLIN;
+ signalfd_put_sighand(ctx, sighand, flags);
+ } else
+ events |= POLLIN;
+
+ return events;
+}
I never really understood the events mask, but other subsystems often
use (POLLIN | POLLRDNORM) instead of just POLLIN. Is there a reason
for not returning POLLRDNORM here?
I don't think those fds will have to deal with the concept of bands and
priorities. I believe POLLIN is fine here.
+static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
+siginfo_t const *kinfo)
+{
+ long err;
+
+ err = __clear_user(uinfo, sizeof(*uinfo));
+
+ /*
+* If you change siginfo_t structure, please be sure
+* this code is fixed accordingly.
+*/
+ err |= __put_user(kinfo-si_signo, uinfo-signo);
+ err |= __put_user(kinfo-si_errno, uinfo-err);
+ err |= __put_user((short)kinfo-si_code, uinfo-code);
+ switch (kinfo-si_code __SI_MASK) {
+ case __SI_KILL:
+ err |= __put_user(kinfo-si_pid, uinfo-pid);
+ err |= __put_user(kinfo-si_uid, uinfo-uid);
+ break;
+ case __SI_TIMER:
+err |= __put_user(kinfo-si_tid, uinfo-tid);
+err |= __put_user(kinfo-si_overrun, uinfo-overrun);
+err |= __put_user(kinfo-si_ptr, uinfo-svptr);
+ break;
+ case __SI_POLL:
+ err |= __put_user(kinfo-si_band, uinfo-band);
+ err |= __put_user(kinfo-si_fd, uinfo-fd);
+ break;
+ case __SI_FAULT:
+ err |= __put_user(kinfo-si_addr, uinfo-addr);
+#ifdef __ARCH_SI_TRAPNO
+ err |= __put_user(kinfo-si_trapno, uinfo-trapno);
+#endif
+ break;
+ case __SI_CHLD:
+ err |= __put_user(kinfo-si_pid, uinfo-pid);
+ err |= __put_user(kinfo-si_uid, uinfo-uid);
+ err |= __put_user(kinfo-si_status, uinfo-status);
+ err |= __put_user(kinfo-si_utime, uinfo-utime);
+ err |= __put_user(kinfo-si_stime, uinfo-stime);
+ break;
+ case __SI_RT: /* This is not generated by the kernel as of now. */
+ case __SI_MESGQ: /* But this is */
+ err |= __put_user(kinfo-si_pid, uinfo-pid);
+ err |= __put_user(kinfo-si_uid, uinfo-uid);
+ err |= __put_user(kinfo-si_ptr, uinfo-svptr);
+ break;
+ default: /* this is just in case for now ... */
+ err |= __put_user(kinfo-si_pid, uinfo-pid);
+ err |= __put_user(kinfo-si_uid, uinfo-uid);
+ break;
+ }
+
+ return err ? -EFAULT: sizeof(*uinfo);
+}
Doing it this way looks rather inefficient to me. I think it's
better to just prepare the signalfd_siginfo on the stack and
do a single copy_to_user.
bah, __put_user is basically a move, so I don't think that efficency would
be that different (assuming that it'd matter in this case). The only thing
many __put_user do, is increase the exception table sizes.
Also, what's the reasoning behind defining a new structure
instead of just returning siginfo_t? Sure siginfo_t is ugly
but it is a well-defined structure and users already deal
with the problems it causes.
Compat on sys_read() would be insane ;)
+static void __exit signalfd_exit(void)
+{
+ kmem_cache_destroy(signalfd_ctx_cachep);
+}
+
+module_init(signalfd_init);
+module_exit(signalfd_exit);
+
+MODULE_LICENSE(GPL);
Since this file defines a syscall, it can't be a module, so why bother
with this?
Agreed, remove exit function and using fs_initcall.
+
+struct signalfd_siginfo {
+ __u32 signo;
+ __s32 err;
+ __s32 code;
+ __u32 pid;
+ __u32 uid;
+ __s32 fd;
+ __u32 tid;
+ __u32 band;
+ __u32 overrun;
+ __u32 trapno;
+ __s32 status;
+ __s32
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Sunday 18 March 2007, Davide Libenzi wrote: bah, __put_user is basically a move, so I don't think that efficency would be that different (assuming that it'd matter in this case). The only thing many __put_user do, is increase the exception table sizes. The cost of user access functions varies a lot depending on the architectures. Those platforms with a 4G/4G split e.g. need to do more than a simple move, and for s390 it may even come down to an indirect function call, which incurs significant register pressure. Arnd - 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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Mon, 19 Mar 2007, Arnd Bergmann wrote: On Sunday 18 March 2007, Davide Libenzi wrote: bah, __put_user is basically a move, so I don't think that efficency would be that different (assuming that it'd matter in this case). The only thing many __put_user do, is increase the exception table sizes. The cost of user access functions varies a lot depending on the architectures. Those platforms with a 4G/4G split e.g. need to do more than a simple move, and for s390 it may even come down to an indirect function call, which incurs significant register pressure. Heh, I'd like ppl to agree on this, because I clearly remember in having an argoument with Andrew for the same thing, where I was doing stack setup plus copy_to_user() ;) - Davide - 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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Sat, 17 Mar 2007 22:35:08 +0100 Arnd Bergmann <[EMAIL PROTECTED]> wrote: > > sizeof(sigset_t) may be different for native and 32-bit compat code. > It would be good if you could handle sizemask==4 && sizeof(sigset_t)==8 > in this code, so that there is no need for an extra compat_sys_signalfd > function. We still need a compat function to get the bit layout and the wordlayout correct in the sigset. -- Cheers, Stephen Rothwell[EMAIL PROTECTED] http://www.canb.auug.org.au/~sfr/ pgpG55iZjZ636.pgp Description: PGP signature
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Saturday 17 March 2007 22:35:08 Arnd Bergmann wrote: > Also, what's the reasoning behind defining a new structure > instead of just returning siginfo_t? Sure siginfo_t is ugly > but it is a well-defined structure and users already deal > with the problems it causes. Ok, found the answer myself, fops->read() must not do the conversion to compat_siginfo_t on a 64 bit kernel, that would just be too ugly for words. Arnd <>< - 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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Friday 16 March 2007 01:22:15 Davide Libenzi wrote:
> +
> +static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx
> *ctx, + unsigned long
> *flags);
> +static void signalfd_put_sighand(struct signalfd_ctx *ctx,
> + struct sighand_struct *sighand,
> + unsigned long *flags);
> +static void signalfd_cleanup(struct signalfd_ctx *ctx);
> +static int signalfd_close(struct inode *inode, struct file *file);
> +static unsigned int signalfd_poll(struct file *file, poll_table *wait);
> +static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> + siginfo_t const *kinfo);
> +static ssize_t signalfd_read(struct file *file, char __user *buf, size_t
> count, + loff_t *ppos);
> +
see my comment about forward declarations in the previous mail
> +asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t
> sizemask) +{
> + int error;
> + unsigned long flags;
> + sigset_t sigmask;
> + struct signalfd_ctx *ctx;
> + struct sighand_struct *sighand;
> + struct file *file;
> + struct inode *inode;
> +
> + error = -EINVAL;
> + if (sizemask != sizeof(sigset_t) ||
> + copy_from_user(, user_mask, sizeof(sigmask)))
> + goto err_exit;
sizeof(sigset_t) may be different for native and 32-bit compat code.
It would be good if you could handle sizemask==4 && sizeof(sigset_t)==8
in this code, so that there is no need for an extra compat_sys_signalfd
function.
> + if ((sighand = signalfd_get_sighand(ctx, )) != NULL) {
> + if (next_signal(>tsk->pending, >sigmask) > 0 ||
> + next_signal(>tsk->signal->shared_pending,
> + >sigmask) > 0)
> + events |= POLLIN;
> + signalfd_put_sighand(ctx, sighand, );
> + } else
> + events |= POLLIN;
> +
> + return events;
> +}
I never really understood the events mask, but other subsystems often
use (POLLIN | POLLRDNORM) instead of just POLLIN. Is there a reason
for not returning POLLRDNORM here?
> +static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> + siginfo_t const *kinfo)
> +{
> + long err;
> +
> + err = __clear_user(uinfo, sizeof(*uinfo));
> +
> + /*
> + * If you change siginfo_t structure, please be sure
> + * this code is fixed accordingly.
> + */
> + err |= __put_user(kinfo->si_signo, >signo);
> + err |= __put_user(kinfo->si_errno, >err);
> + err |= __put_user((short)kinfo->si_code, >code);
> + switch (kinfo->si_code & __SI_MASK) {
> + case __SI_KILL:
> + err |= __put_user(kinfo->si_pid, >pid);
> + err |= __put_user(kinfo->si_uid, >uid);
> + break;
> + case __SI_TIMER:
> + err |= __put_user(kinfo->si_tid, >tid);
> + err |= __put_user(kinfo->si_overrun, >overrun);
> + err |= __put_user(kinfo->si_ptr, >svptr);
> + break;
> + case __SI_POLL:
> + err |= __put_user(kinfo->si_band, >band);
> + err |= __put_user(kinfo->si_fd, >fd);
> + break;
> + case __SI_FAULT:
> + err |= __put_user(kinfo->si_addr, >addr);
> +#ifdef __ARCH_SI_TRAPNO
> + err |= __put_user(kinfo->si_trapno, >trapno);
> +#endif
> + break;
> + case __SI_CHLD:
> + err |= __put_user(kinfo->si_pid, >pid);
> + err |= __put_user(kinfo->si_uid, >uid);
> + err |= __put_user(kinfo->si_status, >status);
> + err |= __put_user(kinfo->si_utime, >utime);
> + err |= __put_user(kinfo->si_stime, >stime);
> + break;
> + case __SI_RT: /* This is not generated by the kernel as of now. */
> + case __SI_MESGQ: /* But this is */
> + err |= __put_user(kinfo->si_pid, >pid);
> + err |= __put_user(kinfo->si_uid, >uid);
> + err |= __put_user(kinfo->si_ptr, >svptr);
> + break;
> + default: /* this is just in case for now ... */
> + err |= __put_user(kinfo->si_pid, >pid);
> + err |= __put_user(kinfo->si_uid, >uid);
> + break;
> + }
> +
> + return err ? -EFAULT: sizeof(*uinfo);
> +}
Doing it this way looks rather inefficient to me. I think it's
better to just prepare the signalfd_siginfo on the stack and
do a single copy_to_user.
Also, what's the reasoning behind defining a new structure
instead of just returning siginfo_t? Sure siginfo_t is ugly
but it is a well-defined structure and users already deal
with the problems it causes.
> +static void __exit signalfd_exit(void)
> +{
> + kmem_cache_destroy(signalfd_ctx_cachep);
> +}
> +
> +module_init(signalfd_init);
> +module_exit(signalfd_exit);
> +
> +MODULE_LICENSE("GPL");
Since this file defines a syscall, it can't
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Friday 16 March 2007 01:22:15 Davide Libenzi wrote:
+
+static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx
*ctx, + unsigned long
*flags);
+static void signalfd_put_sighand(struct signalfd_ctx *ctx,
+ struct sighand_struct *sighand,
+ unsigned long *flags);
+static void signalfd_cleanup(struct signalfd_ctx *ctx);
+static int signalfd_close(struct inode *inode, struct file *file);
+static unsigned int signalfd_poll(struct file *file, poll_table *wait);
+static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
+ siginfo_t const *kinfo);
+static ssize_t signalfd_read(struct file *file, char __user *buf, size_t
count, + loff_t *ppos);
+
see my comment about forward declarations in the previous mail
+asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t
sizemask) +{
+ int error;
+ unsigned long flags;
+ sigset_t sigmask;
+ struct signalfd_ctx *ctx;
+ struct sighand_struct *sighand;
+ struct file *file;
+ struct inode *inode;
+
+ error = -EINVAL;
+ if (sizemask != sizeof(sigset_t) ||
+ copy_from_user(sigmask, user_mask, sizeof(sigmask)))
+ goto err_exit;
sizeof(sigset_t) may be different for native and 32-bit compat code.
It would be good if you could handle sizemask==4 sizeof(sigset_t)==8
in this code, so that there is no need for an extra compat_sys_signalfd
function.
+ if ((sighand = signalfd_get_sighand(ctx, flags)) != NULL) {
+ if (next_signal(ctx-tsk-pending, ctx-sigmask) 0 ||
+ next_signal(ctx-tsk-signal-shared_pending,
+ ctx-sigmask) 0)
+ events |= POLLIN;
+ signalfd_put_sighand(ctx, sighand, flags);
+ } else
+ events |= POLLIN;
+
+ return events;
+}
I never really understood the events mask, but other subsystems often
use (POLLIN | POLLRDNORM) instead of just POLLIN. Is there a reason
for not returning POLLRDNORM here?
+static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
+ siginfo_t const *kinfo)
+{
+ long err;
+
+ err = __clear_user(uinfo, sizeof(*uinfo));
+
+ /*
+ * If you change siginfo_t structure, please be sure
+ * this code is fixed accordingly.
+ */
+ err |= __put_user(kinfo-si_signo, uinfo-signo);
+ err |= __put_user(kinfo-si_errno, uinfo-err);
+ err |= __put_user((short)kinfo-si_code, uinfo-code);
+ switch (kinfo-si_code __SI_MASK) {
+ case __SI_KILL:
+ err |= __put_user(kinfo-si_pid, uinfo-pid);
+ err |= __put_user(kinfo-si_uid, uinfo-uid);
+ break;
+ case __SI_TIMER:
+ err |= __put_user(kinfo-si_tid, uinfo-tid);
+ err |= __put_user(kinfo-si_overrun, uinfo-overrun);
+ err |= __put_user(kinfo-si_ptr, uinfo-svptr);
+ break;
+ case __SI_POLL:
+ err |= __put_user(kinfo-si_band, uinfo-band);
+ err |= __put_user(kinfo-si_fd, uinfo-fd);
+ break;
+ case __SI_FAULT:
+ err |= __put_user(kinfo-si_addr, uinfo-addr);
+#ifdef __ARCH_SI_TRAPNO
+ err |= __put_user(kinfo-si_trapno, uinfo-trapno);
+#endif
+ break;
+ case __SI_CHLD:
+ err |= __put_user(kinfo-si_pid, uinfo-pid);
+ err |= __put_user(kinfo-si_uid, uinfo-uid);
+ err |= __put_user(kinfo-si_status, uinfo-status);
+ err |= __put_user(kinfo-si_utime, uinfo-utime);
+ err |= __put_user(kinfo-si_stime, uinfo-stime);
+ break;
+ case __SI_RT: /* This is not generated by the kernel as of now. */
+ case __SI_MESGQ: /* But this is */
+ err |= __put_user(kinfo-si_pid, uinfo-pid);
+ err |= __put_user(kinfo-si_uid, uinfo-uid);
+ err |= __put_user(kinfo-si_ptr, uinfo-svptr);
+ break;
+ default: /* this is just in case for now ... */
+ err |= __put_user(kinfo-si_pid, uinfo-pid);
+ err |= __put_user(kinfo-si_uid, uinfo-uid);
+ break;
+ }
+
+ return err ? -EFAULT: sizeof(*uinfo);
+}
Doing it this way looks rather inefficient to me. I think it's
better to just prepare the signalfd_siginfo on the stack and
do a single copy_to_user.
Also, what's the reasoning behind defining a new structure
instead of just returning siginfo_t? Sure siginfo_t is ugly
but it is a well-defined structure and users already deal
with the problems it causes.
+static void __exit signalfd_exit(void)
+{
+ kmem_cache_destroy(signalfd_ctx_cachep);
+}
+
+module_init(signalfd_init);
+module_exit(signalfd_exit);
+
+MODULE_LICENSE(GPL);
Since this file defines a syscall, it can't
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Saturday 17 March 2007 22:35:08 Arnd Bergmann wrote: Also, what's the reasoning behind defining a new structure instead of just returning siginfo_t? Sure siginfo_t is ugly but it is a well-defined structure and users already deal with the problems it causes. Ok, found the answer myself, fops-read() must not do the conversion to compat_siginfo_t on a 64 bit kernel, that would just be too ugly for words. Arnd - 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/
Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Sat, 17 Mar 2007 22:35:08 +0100 Arnd Bergmann [EMAIL PROTECTED] wrote: sizeof(sigset_t) may be different for native and 32-bit compat code. It would be good if you could handle sizemask==4 sizeof(sigset_t)==8 in this code, so that there is no need for an extra compat_sys_signalfd function. We still need a compat function to get the bit layout and the wordlayout correct in the sigset. -- Cheers, Stephen Rothwell[EMAIL PROTECTED] http://www.canb.auug.org.au/~sfr/ pgpG55iZjZ636.pgp Description: PGP signature
[patch 2/13] signal/timer/event fds v6 - signalfd core ...
This patch series implements the new signalfd() system call.
I took part of the original Linus code (and you know how
badly it can be broken :), and I added even more breakage ;)
Signals are fetched from the same signal queue used by the process,
so signalfd will compete with standard kernel delivery in dequeue_signal().
If you want to reliably fetch signals on the signalfd file, you need to
block them with sigprocmask(SIG_BLOCK).
This seems to be working fine on my Dual Opteron machine. I made a quick
test program for it:
http://www.xmailserver.org/signafd-test.c
The signalfd() system call implements signal delivery into a file
descriptor receiver. The signalfd file descriptor if created with the
following API:
int signalfd(int ufd, const sigset_t *mask, size_t masksize);
The "ufd" parameter allows to change an existing signalfd sigmask, w/out
going to close/create cycle (Linus idea). Use "ufd" == -1 if you want a
brand new signalfd file.
The "mask" allows to specify the signal mask of signals that we are
interested in. The "masksize" parameter is the size of "mask".
The signalfd fd supports the poll(2) and read(2) system calls. The poll(2)
will return POLLIN when signals are available to be dequeued. As a direct
consequence of supporting the Linux poll subsystem, the signalfd fd can use
used together with epoll(2) too.
The read(2) system call will return a "struct signalfd_siginfo" structure
in the userspace supplied buffer. The return value is the number of bytes
copied in the supplied buffer, or -1 in case of error. The read(2) call
can also return 0, in case the sighand structure to which the signalfd
was attached, has been orphaned. The O_NONBLOCK flag is also supported, and
read(2) will return -EAGAIN in case no signal is available.
The format of the struct signalfd_siginfo is, and the valid fields depends
of the (->code & __SI_MASK) value, in the same way a struct siginfo would:
struct signalfd_siginfo {
__u32 signo;/* si_signo */
__s32 err; /* si_errno */
__s32 code; /* si_code */
__u32 pid; /* si_pid */
__u32 uid; /* si_uid */
__s32 fd; /* si_fd */
__u32 tid; /* si_fd */
__u32 band; /* si_band */
__u32 overrun; /* si_overrun */
__u32 trapno; /* si_trapno */
__s32 status; /* si_status */
__s32 svint;/* si_int */
__u64 svptr;/* si_ptr */
__u64 utime;/* si_utime */
__u64 stime;/* si_stime */
__u64 addr; /* si_addr */
};
Signed-off-by: Davide Libenzi
- Davide
Index: linux-2.6.21-rc3.quilt/fs/signalfd.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux-2.6.21-rc3.quilt/fs/signalfd.c2007-03-15 15:33:52.0
-0700
@@ -0,0 +1,381 @@
+/*
+ * fs/signalfd.c
+ *
+ * Copyright (C) 2003 Linus Torvalds
+ *
+ * Mon Mar 5, 2007: Davide Libenzi
+ * Changed ->read() to return a siginfo strcture instead of signal number.
+ * Fixed locking in ->poll().
+ * Added sighand-detach notification.
+ * Added fd re-use in sys_signalfd() syscall.
+ * Now using anonymous inode source.
+ * Thanks to Oleg Nesterov for useful code review and suggestions.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+
+
+struct signalfd_ctx {
+ struct list_head lnk;
+ wait_queue_head_t wqh;
+ sigset_t sigmask;
+ struct task_struct *tsk;
+};
+
+
+
+static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx *ctx,
+ unsigned long *flags);
+static void signalfd_put_sighand(struct signalfd_ctx *ctx,
+struct sighand_struct *sighand,
+unsigned long *flags);
+static void signalfd_cleanup(struct signalfd_ctx *ctx);
+static int signalfd_close(struct inode *inode, struct file *file);
+static unsigned int signalfd_poll(struct file *file, poll_table *wait);
+static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
+siginfo_t const *kinfo);
+static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
+loff_t *ppos);
+
+
+
+static const struct file_operations signalfd_fops = {
+ .release= signalfd_close,
+ .poll = signalfd_poll,
+ .read = signalfd_read,
+};
+static struct kmem_cache *signalfd_ctx_cachep;
+
+
+
+static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx *ctx,
+ unsigned long *flags)
+{
+ struct sighand_struct *sighand;
+
+ rcu_read_lock();
+ sighand = lock_task_sighand(ctx->tsk, flags);
+ rcu_read_unlock();
+
+ if (sighand &&
[patch 2/13] signal/timer/event fds v6 - signalfd core ...
This patch series implements the new signalfd() system call.
I took part of the original Linus code (and you know how
badly it can be broken :), and I added even more breakage ;)
Signals are fetched from the same signal queue used by the process,
so signalfd will compete with standard kernel delivery in dequeue_signal().
If you want to reliably fetch signals on the signalfd file, you need to
block them with sigprocmask(SIG_BLOCK).
This seems to be working fine on my Dual Opteron machine. I made a quick
test program for it:
http://www.xmailserver.org/signafd-test.c
The signalfd() system call implements signal delivery into a file
descriptor receiver. The signalfd file descriptor if created with the
following API:
int signalfd(int ufd, const sigset_t *mask, size_t masksize);
The ufd parameter allows to change an existing signalfd sigmask, w/out
going to close/create cycle (Linus idea). Use ufd == -1 if you want a
brand new signalfd file.
The mask allows to specify the signal mask of signals that we are
interested in. The masksize parameter is the size of mask.
The signalfd fd supports the poll(2) and read(2) system calls. The poll(2)
will return POLLIN when signals are available to be dequeued. As a direct
consequence of supporting the Linux poll subsystem, the signalfd fd can use
used together with epoll(2) too.
The read(2) system call will return a struct signalfd_siginfo structure
in the userspace supplied buffer. The return value is the number of bytes
copied in the supplied buffer, or -1 in case of error. The read(2) call
can also return 0, in case the sighand structure to which the signalfd
was attached, has been orphaned. The O_NONBLOCK flag is also supported, and
read(2) will return -EAGAIN in case no signal is available.
The format of the struct signalfd_siginfo is, and the valid fields depends
of the (-code __SI_MASK) value, in the same way a struct siginfo would:
struct signalfd_siginfo {
__u32 signo;/* si_signo */
__s32 err; /* si_errno */
__s32 code; /* si_code */
__u32 pid; /* si_pid */
__u32 uid; /* si_uid */
__s32 fd; /* si_fd */
__u32 tid; /* si_fd */
__u32 band; /* si_band */
__u32 overrun; /* si_overrun */
__u32 trapno; /* si_trapno */
__s32 status; /* si_status */
__s32 svint;/* si_int */
__u64 svptr;/* si_ptr */
__u64 utime;/* si_utime */
__u64 stime;/* si_stime */
__u64 addr; /* si_addr */
};
Signed-off-by: Davide Libenzi davidel@xmailserver.org
- Davide
Index: linux-2.6.21-rc3.quilt/fs/signalfd.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux-2.6.21-rc3.quilt/fs/signalfd.c2007-03-15 15:33:52.0
-0700
@@ -0,0 +1,381 @@
+/*
+ * fs/signalfd.c
+ *
+ * Copyright (C) 2003 Linus Torvalds
+ *
+ * Mon Mar 5, 2007: Davide Libenzi davidel@xmailserver.org
+ * Changed -read() to return a siginfo strcture instead of signal number.
+ * Fixed locking in -poll().
+ * Added sighand-detach notification.
+ * Added fd re-use in sys_signalfd() syscall.
+ * Now using anonymous inode source.
+ * Thanks to Oleg Nesterov for useful code review and suggestions.
+ */
+
+#include linux/file.h
+#include linux/poll.h
+#include linux/slab.h
+#include linux/init.h
+#include linux/fs.h
+#include linux/mount.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/signal.h
+#include linux/list.h
+#include linux/anon_inodes.h
+#include linux/signalfd.h
+
+#include asm/uaccess.h
+
+
+
+struct signalfd_ctx {
+ struct list_head lnk;
+ wait_queue_head_t wqh;
+ sigset_t sigmask;
+ struct task_struct *tsk;
+};
+
+
+
+static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx *ctx,
+ unsigned long *flags);
+static void signalfd_put_sighand(struct signalfd_ctx *ctx,
+struct sighand_struct *sighand,
+unsigned long *flags);
+static void signalfd_cleanup(struct signalfd_ctx *ctx);
+static int signalfd_close(struct inode *inode, struct file *file);
+static unsigned int signalfd_poll(struct file *file, poll_table *wait);
+static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
+siginfo_t const *kinfo);
+static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
+loff_t *ppos);
+
+
+
+static const struct file_operations signalfd_fops = {
+ .release= signalfd_close,
+ .poll = signalfd_poll,
+ .read = signalfd_read,
+};
+static struct kmem_cache *signalfd_ctx_cachep;
+
+
+
+static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx *ctx,
+ unsigned
