Re: LinuxPPS & spinlocks
Hi,
On Wed, 1 Aug 2007, Christopher Hoover wrote:
> Satyam Sharma infradead.org> writes:
> > On Mon, 30 Jul 2007, Rodolfo Giometti wrote:
> >
> > > On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote:
> > > Currently the RFC says to you that you should open the serial port:
> > >
> > > fd = open("/dev/ttyS0", ...);
> >
> > No, it does *NOT*. All it says is:
> >
> > The time_pps_create() is used to convert an already-open UNIX file
> > descriptor, for an appropriate special file, into a PPS handle.
> >
> > See? What I said is precisely the implementation the RFC envisages
> > (and the only sane way to implement it too).
>
> If we were totally rigurous about representing each device as a device node,
> your solution would be fine. But we don't.
Of course.
> The clocksource model (/sys/devices/system/clocksource) is a better way to
> go. One sysfs file is used to enumerate the possible sources and another is
> used to read or set the current source. No new system calls; no new ioctls.
Oh, not introducing any syscalls _at all_ would be fine, too. But the
RFC does /require/ an implementation to have them. I was only mentioning
the kind of implementation the RFC had in mind. But there are other ways
to achieve the same end goal, and yes, probably it's better to avoid
introducing syscalls in the first place and think of other mechanisms.
[ It's not that we're talking of IPsec or IPv6 or something here --
so RFC-compliance isn't overly important. But the final result
needs to be good, secure and well-designed, still. ]
Satyam
-
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: LinuxPPS & spinlocks
Satyam Sharma infradead.org> writes:
> On Mon, 30 Jul 2007, Rodolfo Giometti wrote:
>
> > On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote:
> > Currently the RFC says to you that you should open the serial port:
> >
> > fd = open("/dev/ttyS0", ...);
>
> No, it does *NOT*. All it says is:
>
> The time_pps_create() is used to convert an already-open UNIX file
> descriptor, for an appropriate special file, into a PPS handle.
>
> See? What I said is precisely the implementation the RFC envisages
> (and the only sane way to implement it too).
If we were totally rigurous about representing each device as a device node,
your solution would be fine. But we don't.
The clocksource model (/sys/devices/system/clocksource) is a better way to
go. One sysfs file is used to enumerate the possible sources and another is
used to read or set the current source. No new system calls; no new ioctls.
-ch
ch (at) murgatroid (dot) com
ch (at) hpl (dot) hp (dot) com
-
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: LinuxPPS spinlocks
Satyam Sharma satyam at infradead.org writes: On Mon, 30 Jul 2007, Rodolfo Giometti wrote: On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote: Currently the RFC says to you that you should open the serial port: fd = open(/dev/ttyS0, ...); No, it does *NOT*. All it says is: The time_pps_create() is used to convert an already-open UNIX file descriptor, for an appropriate special file, into a PPS handle. See? What I said is precisely the implementation the RFC envisages (and the only sane way to implement it too). If we were totally rigurous about representing each device as a device node, your solution would be fine. But we don't. The clocksource model (/sys/devices/system/clocksource) is a better way to go. One sysfs file is used to enumerate the possible sources and another is used to read or set the current source. No new system calls; no new ioctls. -ch ch (at) murgatroid (dot) com ch (at) hpl (dot) hp (dot) com - 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: LinuxPPS spinlocks
Hi, On Wed, 1 Aug 2007, Christopher Hoover wrote: Satyam Sharma satyam at infradead.org writes: On Mon, 30 Jul 2007, Rodolfo Giometti wrote: On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote: Currently the RFC says to you that you should open the serial port: fd = open(/dev/ttyS0, ...); No, it does *NOT*. All it says is: The time_pps_create() is used to convert an already-open UNIX file descriptor, for an appropriate special file, into a PPS handle. See? What I said is precisely the implementation the RFC envisages (and the only sane way to implement it too). If we were totally rigurous about representing each device as a device node, your solution would be fine. But we don't. Of course. The clocksource model (/sys/devices/system/clocksource) is a better way to go. One sysfs file is used to enumerate the possible sources and another is used to read or set the current source. No new system calls; no new ioctls. Oh, not introducing any syscalls _at all_ would be fine, too. But the RFC does /require/ an implementation to have them. I was only mentioning the kind of implementation the RFC had in mind. But there are other ways to achieve the same end goal, and yes, probably it's better to avoid introducing syscalls in the first place and think of other mechanisms. [ It's not that we're talking of IPsec or IPv6 or something here -- so RFC-compliance isn't overly important. But the final result needs to be good, secure and well-designed, still. ] Satyam - 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: LinuxPPS & spinlocks
Hi, On Tue, 31 Jul 2007, Rodolfo Giometti wrote: > Sorry for wasting your time. :'( Maybe you can provide your solution > for PPS support and get it included into kernel tree so we can use it > and live happy! :) Please stop embarrassing me (and yourself). Sorry, I did lose my patience (others you mention did not) but then that's precisely because of my own _less_ experience, if anything. No, it's not like I "want to provide my solution for PPS support and get it included into kernel tree" like you wrote above -- I simply identified 4 problem areas in your implementation and wanted them to be addressed. [If you can do so yourself, good enough, otherwise I'm happy to send patch based on your earlier one too -- the modifications I have in mind are few and simple.] I have no personal stake in this PPS stuff anyway, this is just a normal peer review of submissions that should always happen. Thanks, Satyam - 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: LinuxPPS & spinlocks
On Wed, Aug 01, 2007 at 12:19:48AM +0530, Satyam Sharma wrote: > That's just absolute bullshit. ... > I'm sorry to say this, Rodolfo, but _all_ your arguments above are > *totally* nonsensical and factually incorrect -- and I have had enough of > trying to talk sense to you, it's been ~15 mails in this thread already, > and I've been WASTING my time trying to teach / explain to you, but it's > just *shocking* that you prefer to stick to a wholly incorrect (which I > suspect you've already understood by now anyway) position rather than > just accepting that the present patch is wrong and go about correcting > it instead. > [snip] Sorry for wasting your time. :'( Maybe you can provide your solution for PPS support and get it included into kernel tree so we can use it and live happy! :) Several people (Andrew, David, et all) wrote to me a lot of letters without loosing their patience. I'm a poor programmer, not a «guru» like you, and I need more time to understand things. I'm trying to do my best. :) However I'm very proud to be a "poor programmer" (slow in understanding things) but a gentleman rather than a "kernel-guru" like you but totally ill-mannered. Thanks anyway for your (precious) time, Rodolfo P.S. A gentle programmer just wrote to me a letter where, proposing code/patches, showed to me possible good solutions, so I think I'm going to modify again the locking code soon. -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS & spinlocks
On Tue, 31 Jul 2007, Rodolfo Giometti wrote:
> On Tue, Jul 31, 2007 at 03:31:22AM +0530, Satyam Sharma wrote:
> > Yup, this would avoid races, but then we will lose events. Why is that
> > acceptable, when better alternative (above) exists?
>
> Because is better lossign events then recording them delayed. In the
> past we (LinuxPPS users) noticed that just postponing of one
> instruction the timestamp recording degrade the timesetting of about
> 50%!
> > Amazing. On the one hand, you want to use spin_trylock() in the hard irq
> > handler and spin_lock() (not irq safe) in the process context, because
> > you "don't care about losing some events". And now you want to avoid
> > "disabling irqs in user context to minimize possibility to delay events
> > recording"?
>
> Just for not delaying the IRQ handler. As already said, I prefere
> loosing events that delaying their timestamps recording.
> > Anyway, any such requirement you're talking about is just bogus, IMHO.
> > You're just disabling interrupts to access a data structure, for Gods'
> > sakes, how many nanoseconds do you imagine would you be "delaying"?
>
> As already said we noticed that just delaying of one instruction the
> timestamp recording the time setting degrades of about 50%.
>
> We have to take care of this point. It's _very_ important that _each_
> event had its timestamp recorded with delay as small as possible.
That's just absolute bullshit.
I'm sorry to say this, Rodolfo, but _all_ your arguments above are
*totally* nonsensical and factually incorrect -- and I have had enough of
trying to talk sense to you, it's been ~15 mails in this thread already,
and I've been WASTING my time trying to teach / explain to you, but it's
just *shocking* that you prefer to stick to a wholly incorrect (which I
suspect you've already understood by now anyway) position rather than
just accepting that the present patch is wrong and go about correcting
it instead.
> > The proper way to do this is to use a kernel buffer to do all kernel-side
> > work (you acquire/release lock during that work) and then copy_to_user()
> > later, at the end. [ And something opposite for the set_xxx syscall, i.e.
> > first off copy_from_user() to a kernel buffer up front, before doing
> > anything else itself, and then do all the work in the kernel on that. ]
>
> Mmm... I think this is not easy at all for sys_time_pps_fetch(). I
> suppose you have to complicate its code a lot!
>
> I don't undestand why we must complicate, and made unreadable, a code
> in order to follow the rule use-only-one-lock-mechanism. If by using
> mutex and spinlocks the code is more readable, where is the fault?
More nonsense. Utter nonsense. I really don't want to reply anymore ...
> > BTW your syscall implementations totally lack any kind of security checks
> > whatsoever ...
>
> Ok, we can correct them. :)
>
> > > Please, take alook at NTPD code. Common usage is:
> > >
> > >fd = open("/dev/ttyS0", ...);
> > >
> > >pps_time_create(fd, );
> > >
> > > since RFC supposes that at filedes "fd" is mapped both GPS data _and_
> > > PPS source.
> >
> > Umm, I don't think the RFC supposes/assumes this anywhere.
>
> I think so. If not, why they pretend an _already opened_ filedes then
> just a filename to be used as parameter for the open(2) syscall? :)
Try reading the RFC again, please. And *think*.
Anyway, considering:
1. broken/nonsensical locking,
2. wrong (completely RFC non-compliant) implementation,
3. a syscall (time_pps_cmd) that has no semantics defined anywhere,
not even mandated by the RFC, is (as you yourself admitted)
a pseudo-ioctl for all practical purposes,
4. abject lack of security in the syscall implementations,
But MOST importantly:
5. your _sticking_ to the broken implementation and being so
(shockingly!) unwilling to correct these,
I really cannot see how I can support this stuff in getting merged
at all, sorry.
Satyam
-
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: LinuxPPS & spinlocks
On Tue, Jul 31, 2007 at 03:31:22AM +0530, Satyam Sharma wrote:
> Hi Rodolfo,
Hi :)
> Yup, this would avoid races, but then we will lose events. Why is that
> acceptable, when better alternative (above) exists?
Because is better lossign events then recording them delayed. In the
past we (LinuxPPS users) noticed that just postponing of one
instruction the timestamp recording degrade the timesetting of about
50%!
> Seriously, your aversion to implement good, safe locking is amazing! :-)
I averse not by default. :) But I just wish understand _why_ a
solution is better then other.
I'm sure my solution is not the best at all, but before changing it I
wish understand if the new proposal is better.
> The proper way to do this is to use a kernel buffer to do all kernel-side
> work (you acquire/release lock during that work) and then copy_to_user()
> later, at the end. [ And something opposite for the set_xxx syscall, i.e.
> first off copy_from_user() to a kernel buffer up front, before doing
> anything else itself, and then do all the work in the kernel on that. ]
Mmm... I think this is not easy at all for sys_time_pps_fetch(). I
suppose you have to complicate its code a lot!
I don't undestand why we must complicate, and made unreadable, a code
in order to follow the rule use-only-one-lock-mechanism. If by using
mutex and spinlocks the code is more readable, where is the fault?
> BTW your syscall implementations totally lack any kind of security checks
> whatsoever ...
Ok, we can correct them. :)
> Amazing. On the one hand, you want to use spin_trylock() in the hard irq
> handler and spin_lock() (not irq safe) in the process context, because
> you "don't care about losing some events". And now you want to avoid
> "disabling irqs in user context to minimize possibility to delay events
> recording"?
Just for not delaying the IRQ handler. As already said, I prefere
loosing events that delaying their timestamps recording.
> Anyway, any such requirement you're talking about is just bogus, IMHO.
> You're just disabling interrupts to access a data structure, for Gods'
> sakes, how many nanoseconds do you imagine would you be "delaying"?
As already said we noticed that just delaying of one instruction the
timestamp recording the time setting degrades of about 50%.
We have to take care of this point. It's _very_ important that _each_
event had its timestamp recorded with delay as small as possible.
> > Please, take alook at NTPD code. Common usage is:
> >
> >fd = open("/dev/ttyS0", ...);
> >
> >pps_time_create(fd, );
> >
> > since RFC supposes that at filedes "fd" is mapped both GPS data _and_
> > PPS source.
>
> Umm, I don't think the RFC supposes/assumes this anywhere.
I think so. If not, why they pretend an _already opened_ filedes then
just a filename to be used as parameter for the open(2) syscall? :)
Again, it was simpler, and more logic, define the time_pps_create as
follow:
time_pps_create(char *ppsdev, pps_handle_t *handle);
this definition resolves very well all possibile cases.
> Of course. BTW time_pps_findpath/findsource do not have to be kernel
> implemented syscalls in the first place. The best, simplest and most
> straightforward place to implement them is in userspace -- the RFC
> mentions this as well.
Ok, I'll wait for your modification to see how you can do it.
> A solution I can think of is to create a mapping at the time of
> pps_register_source() between the PPS source (physically) and the special
> char device. Userspace open(2)'s the special char device corresponding to
> the PPS source, and then issues the time_pps_create() syscall. Here, we
> lookup the mapping previously created and return handle to the PPS source
> based on the special device's fd that's passed to us in time_pps_create().
Ok, just what pps_findpath does... however if you now how to
implement it I'll be very to see the code. :)
But, as you can see, this is due the bogus RFC specification. It was
more easier define the time_pps_create() as suggested above and you
didn't need no mapping at all.
> Ok, I'll take a look, thanks :-)
>
> [ what's the URL for these sources? ]
Oh, yes... sorry, here the NTP main site:
http://www.ntp.org
> open(PPSfilename, O_RDWR) doesn't sound like that at all. If the device
> for the serial port / parport was intended, it wouldn't be called
> "PPSfilename", would it?
No, I meant that if they require an already opened file descriptor is
beacuse they simply don't suppose that a PPS source may be totally
uncollerated to any device. If not they had two possibilities:
1) just using an open(2) to access a PPS source and using the returned
filedes as PPS handler,
2) or, just to be more general, define the time_pps_create() as above,
with the "char *ppsdevice" as parameter (then on UNIX systems we
implement it with open(2)).
> The physical port/device through which a PPS source is connected is
> immaterial. Anyway, I'll take a look at the
Re: LinuxPPS spinlocks
On Tue, Jul 31, 2007 at 03:31:22AM +0530, Satyam Sharma wrote: Hi Rodolfo, Hi :) Yup, this would avoid races, but then we will lose events. Why is that acceptable, when better alternative (above) exists? Because is better lossign events then recording them delayed. In the past we (LinuxPPS users) noticed that just postponing of one instruction the timestamp recording degrade the timesetting of about 50%! Seriously, your aversion to implement good, safe locking is amazing! :-) I averse not by default. :) But I just wish understand _why_ a solution is better then other. I'm sure my solution is not the best at all, but before changing it I wish understand if the new proposal is better. The proper way to do this is to use a kernel buffer to do all kernel-side work (you acquire/release lock during that work) and then copy_to_user() later, at the end. [ And something opposite for the set_xxx syscall, i.e. first off copy_from_user() to a kernel buffer up front, before doing anything else itself, and then do all the work in the kernel on that. ] Mmm... I think this is not easy at all for sys_time_pps_fetch(). I suppose you have to complicate its code a lot! I don't undestand why we must complicate, and made unreadable, a code in order to follow the rule use-only-one-lock-mechanism. If by using mutex and spinlocks the code is more readable, where is the fault? BTW your syscall implementations totally lack any kind of security checks whatsoever ... Ok, we can correct them. :) Amazing. On the one hand, you want to use spin_trylock() in the hard irq handler and spin_lock() (not irq safe) in the process context, because you don't care about losing some events. And now you want to avoid disabling irqs in user context to minimize possibility to delay events recording? Just for not delaying the IRQ handler. As already said, I prefere loosing events that delaying their timestamps recording. Anyway, any such requirement you're talking about is just bogus, IMHO. You're just disabling interrupts to access a data structure, for Gods' sakes, how many nanoseconds do you imagine would you be delaying? As already said we noticed that just delaying of one instruction the timestamp recording the time setting degrades of about 50%. We have to take care of this point. It's _very_ important that _each_ event had its timestamp recorded with delay as small as possible. Please, take alook at NTPD code. Common usage is: fd = open(/dev/ttyS0, ...); pps_time_create(fd, handler); since RFC supposes that at filedes fd is mapped both GPS data _and_ PPS source. Umm, I don't think the RFC supposes/assumes this anywhere. I think so. If not, why they pretend an _already opened_ filedes then just a filename to be used as parameter for the open(2) syscall? :) Again, it was simpler, and more logic, define the time_pps_create as follow: time_pps_create(char *ppsdev, pps_handle_t *handle); this definition resolves very well all possibile cases. Of course. BTW time_pps_findpath/findsource do not have to be kernel implemented syscalls in the first place. The best, simplest and most straightforward place to implement them is in userspace -- the RFC mentions this as well. Ok, I'll wait for your modification to see how you can do it. A solution I can think of is to create a mapping at the time of pps_register_source() between the PPS source (physically) and the special char device. Userspace open(2)'s the special char device corresponding to the PPS source, and then issues the time_pps_create() syscall. Here, we lookup the mapping previously created and return handle to the PPS source based on the special device's fd that's passed to us in time_pps_create(). Ok, just what pps_findpathC. does... however if you now how to implement it I'll be very to see the code. :) But, as you can see, this is due the bogus RFC specification. It was more easier define the time_pps_create() as suggested above and you didn't need no mapping at all. Ok, I'll take a look, thanks :-) [ what's the URL for these sources? ] Oh, yes... sorry, here the NTP main site: http://www.ntp.org open(PPSfilename, O_RDWR) doesn't sound like that at all. If the device for the serial port / parport was intended, it wouldn't be called PPSfilename, would it? No, I meant that if they require an already opened file descriptor is beacuse they simply don't suppose that a PPS source may be totally uncollerated to any device. If not they had two possibilities: 1) just using an open(2) to access a PPS source and using the returned filedes as PPS handler, 2) or, just to be more general, define the time_pps_create() as above, with the char *ppsdevice as parameter (then on UNIX systems we implement it with open(2)). The physical port/device through which a PPS source is connected is immaterial. Anyway, I'll take a look at the NTPD/userspace code you are mentioning as well. [ again, could you
Re: LinuxPPS spinlocks
On Tue, 31 Jul 2007, Rodolfo Giometti wrote: On Tue, Jul 31, 2007 at 03:31:22AM +0530, Satyam Sharma wrote: Yup, this would avoid races, but then we will lose events. Why is that acceptable, when better alternative (above) exists? Because is better lossign events then recording them delayed. In the past we (LinuxPPS users) noticed that just postponing of one instruction the timestamp recording degrade the timesetting of about 50%! Amazing. On the one hand, you want to use spin_trylock() in the hard irq handler and spin_lock() (not irq safe) in the process context, because you don't care about losing some events. And now you want to avoid disabling irqs in user context to minimize possibility to delay events recording? Just for not delaying the IRQ handler. As already said, I prefere loosing events that delaying their timestamps recording. Anyway, any such requirement you're talking about is just bogus, IMHO. You're just disabling interrupts to access a data structure, for Gods' sakes, how many nanoseconds do you imagine would you be delaying? As already said we noticed that just delaying of one instruction the timestamp recording the time setting degrades of about 50%. We have to take care of this point. It's _very_ important that _each_ event had its timestamp recorded with delay as small as possible. That's just absolute bullshit. I'm sorry to say this, Rodolfo, but _all_ your arguments above are *totally* nonsensical and factually incorrect -- and I have had enough of trying to talk sense to you, it's been ~15 mails in this thread already, and I've been WASTING my time trying to teach / explain to you, but it's just *shocking* that you prefer to stick to a wholly incorrect (which I suspect you've already understood by now anyway) position rather than just accepting that the present patch is wrong and go about correcting it instead. The proper way to do this is to use a kernel buffer to do all kernel-side work (you acquire/release lock during that work) and then copy_to_user() later, at the end. [ And something opposite for the set_xxx syscall, i.e. first off copy_from_user() to a kernel buffer up front, before doing anything else itself, and then do all the work in the kernel on that. ] Mmm... I think this is not easy at all for sys_time_pps_fetch(). I suppose you have to complicate its code a lot! I don't undestand why we must complicate, and made unreadable, a code in order to follow the rule use-only-one-lock-mechanism. If by using mutex and spinlocks the code is more readable, where is the fault? More nonsense. Utter nonsense. I really don't want to reply anymore ... BTW your syscall implementations totally lack any kind of security checks whatsoever ... Ok, we can correct them. :) Please, take alook at NTPD code. Common usage is: fd = open(/dev/ttyS0, ...); pps_time_create(fd, handler); since RFC supposes that at filedes fd is mapped both GPS data _and_ PPS source. Umm, I don't think the RFC supposes/assumes this anywhere. I think so. If not, why they pretend an _already opened_ filedes then just a filename to be used as parameter for the open(2) syscall? :) Try reading the RFC again, please. And *think*. Anyway, considering: 1. broken/nonsensical locking, 2. wrong (completely RFC non-compliant) implementation, 3. a syscall (time_pps_cmd) that has no semantics defined anywhere, not even mandated by the RFC, is (as you yourself admitted) a pseudo-ioctl for all practical purposes, 4. abject lack of security in the syscall implementations, But MOST importantly: 5. your _sticking_ to the broken implementation and being so (shockingly!) unwilling to correct these, I really cannot see how I can support this stuff in getting merged at all, sorry. Satyam - 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: LinuxPPS spinlocks
On Wed, Aug 01, 2007 at 12:19:48AM +0530, Satyam Sharma wrote: That's just absolute bullshit. ... I'm sorry to say this, Rodolfo, but _all_ your arguments above are *totally* nonsensical and factually incorrect -- and I have had enough of trying to talk sense to you, it's been ~15 mails in this thread already, and I've been WASTING my time trying to teach / explain to you, but it's just *shocking* that you prefer to stick to a wholly incorrect (which I suspect you've already understood by now anyway) position rather than just accepting that the present patch is wrong and go about correcting it instead. [snip] Sorry for wasting your time. :'( Maybe you can provide your solution for PPS support and get it included into kernel tree so we can use it and live happy! :) Several people (Andrew, David, et all) wrote to me a lot of letters without loosing their patience. I'm a poor programmer, not a «guru» like you, and I need more time to understand things. I'm trying to do my best. :) However I'm very proud to be a poor programmer (slow in understanding things) but a gentleman rather than a kernel-guru like you but totally ill-mannered. Thanks anyway for your (precious) time, Rodolfo P.S. A gentle programmer just wrote to me a letter where, proposing code/patches, showed to me possible good solutions, so I think I'm going to modify again the locking code soon. -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
Hi, On Tue, 31 Jul 2007, Rodolfo Giometti wrote: Sorry for wasting your time. :'( Maybe you can provide your solution for PPS support and get it included into kernel tree so we can use it and live happy! :) Please stop embarrassing me (and yourself). Sorry, I did lose my patience (others you mention did not) but then that's precisely because of my own _less_ experience, if anything. No, it's not like I want to provide my solution for PPS support and get it included into kernel tree like you wrote above -- I simply identified 4 problem areas in your implementation and wanted them to be addressed. [If you can do so yourself, good enough, otherwise I'm happy to send patch based on your earlier one too -- the modifications I have in mind are few and simple.] I have no personal stake in this PPS stuff anyway, this is just a normal peer review of submissions that should always happen. Thanks, Satyam - 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: LinuxPPS & spinlocks
Hi Rodolfo,
On Mon, 30 Jul 2007, Rodolfo Giometti wrote:
> On Mon, Jul 30, 2007 at 02:37:26PM +0530, Satyam Sharma wrote:
>
> > > Maybe you meant I should using spin_lock_irqsave/restore() in user
> > > context, but doing like this I will disable interrupts
> >
> > Yup, but the goal is to avoid races. Otherwise why bother doing any
> > locking at all?
>
> I meant that if you have to lock between user context and interrupt
> context you have two choises:
>
> 1) using spin_lock_irqsave/restore() in user context and spin_lock/unlock()
> in the interrupt context (as Rusty says),
Yes, but we need to use spin_lock_irqsave() from interrupt context to
synchronize between two hard irq contexts themselves.
> 2) or using spin_lock/unlock() in user context and
> spin_trylock/unlock() in the interrupt context in order to avoid dead
> locks.
Yup, this would avoid races, but then we will lose events. Why is that
acceptable, when better alternative (above) exists?
Seriously, your aversion to implement good, safe locking is amazing! :-)
> > > Of course, I meant "protecting data". In fact to protect pps_source[]
> > > I need spin_lock() to protect user context from interrupt context and
> > > mutex to protect user context from itself.
> >
> > But that's nonsensical! That's not how you implement locking!
> >
> > First, spin_lock() is *not* enough to protect access from process context
> > from access from interrupt context.
> >
> > Second, if you *already* have a lock to protect any data, introducing
> > *another* lock to protect the same data is ... utterly crazy!
>
> I see what you mean. But my question is about using spin_locks where
> we can sleep. Let me explain, consider sys_time_pps_getparams():
> [...]
> The copy_to_user() may sleep and if I change
> mutex_lock_interruptible() with a spin_lock I may hold it and then
> going to sleep... using mutex we can use the CPU for other
> computations.
The proper way to do this is to use a kernel buffer to do all kernel-side
work (you acquire/release lock during that work) and then copy_to_user()
later, at the end. [ And something opposite for the set_xxx syscall, i.e.
first off copy_from_user() to a kernel buffer up front, before doing
anything else itself, and then do all the work in the kernel on that. ]
BTW your syscall implementations totally lack any kind of security checks
whatsoever ...
> > > Why you wish using one lock per sources? Just one lock for the
> > > list/array is not enought? :-o
> >
> > No, I am *not* wishing / advocating that at all. Just that you appear so
> > _reluctant_ to use spinlocks and are unnecessarily worrying about
> > contention, disabling interrupts, etc etc.
> >
> > Just use the spin_lock_irqsave/restore() variants, and you'll be fine.
>
> What I wish is just to avoid disabling IRQs in user context in order
> to minimize the possibility to delay events recording.
Amazing. On the one hand, you want to use spin_trylock() in the hard irq
handler and spin_lock() (not irq safe) in the process context, because
you "don't care about losing some events". And now you want to avoid
"disabling irqs in user context to minimize possibility to delay events
recording"?
Anyway, any such requirement you're talking about is just bogus, IMHO.
You're just disabling interrupts to access a data structure, for Gods'
sakes, how many nanoseconds do you imagine would you be "delaying"?
> > > As you propose you need _two_ open() and not just one...
> >
> > No, why?
>
> Please, take alook at NTPD code. Common usage is:
>
>fd = open("/dev/ttyS0", ...);
>
>pps_time_create(fd, );
>
> since RFC supposes that at filedes "fd" is mapped both GPS data _and_
> PPS source.
Umm, I don't think the RFC supposes/assumes this anywhere.
> Furthermore with my pps_findpath() I can do:
>
>fd = open("/dev/ttyS0", ...);
>
>handler = pps_findpath("/dev/ttyS0", ...);
>
> which in most cases its more easy to manage for both user and kernel
> land.
>
> Can do the same with char devices?
Of course. BTW time_pps_findpath/findsource do not have to be kernel
implemented syscalls in the first place. The best, simplest and most
straightforward place to implement them is in userspace -- the RFC
mentions this as well.
> Maybe you can easily create
> /dev/pps0 but how can you relate it with the /dev/ttyS0 if your GPS
> antenna and PPS source share the same serial port?
A solution I can think of is to create a mapping at the time of
pps_register_source() between the PPS source (physically) and the special
char device. Userspace open(2)'s the special char device corresponding to
the PPS source, and then issues the time_pps_create() syscall. Here, we
lookup the mapping previously created and return handle to the PPS source
based on the special device's fd that's passed to us in time_pps_create().
> > > I quite sure that RFC is broken since it doesn't take in account that
> > > a PPS source maybe not connected with any cahr device at all. I
Re: LinuxPPS & spinlocks
On Mon, Jul 30, 2007 at 02:37:26PM +0530, Satyam Sharma wrote:
> On Mon, 30 Jul 2007, Rodolfo Giometti wrote:
> >
> > In pps_event() is not useful using spin_lock_irqsave/restore() since
> > the only difference between spin_lock_irqsave() and spin_lock() is
> > that the former will turn off interrupts if they are on, otherwise
> > does nothing (if we are already in an interrupt handler).
>
> Yup. But two pps_event()'s on different CPU's could still race.
??? :-o
Maybe one CPU spins 'till the other holds the lock but any races
should happen...
> > Maybe you meant I should using spin_lock_irqsave/restore() in user
> > context, but doing like this I will disable interrupts
>
> Yup, but the goal is to avoid races. Otherwise why bother doing any
> locking at all?
I meant that if you have to lock between user context and interrupt
context you have two choises:
1) using spin_lock_irqsave/restore() in user context and spin_lock/unlock()
in the interrupt context (as Rusty says),
2) or using spin_lock/unlock() in user context and
spin_trylock/unlock() in the interrupt context in order to avoid dead
locks.
Is that correct?
> > Of course, I meant "protecting data". In fact to protect pps_source[]
> > I need spin_lock() to protect user context from interrupt context and
> > mutex to protect user context from itself.
>
> But that's nonsensical! That's not how you implement locking!
>
> First, spin_lock() is *not* enough to protect access from process context
> from access from interrupt context.
>
> Second, if you *already* have a lock to protect any data, introducing
> *another* lock to protect the same data is ... utterly crazy!
I see what you mean. But my question is about using spin_locks where
we can sleep. Let me explain, consider sys_time_pps_getparams():
asmlinkage long sys_time_pps_getparams(int source,
struct pps_kparams __user *params)
{
int ret = 0;
pr_debug("%s: source %d\n", __FUNCTION__, source);
/* Sanity checks */
if (!params)
return -EINVAL;
if (mutex_lock_interruptible(_mutex))
return -EINTR;
ret = pps_check_source(source);
if (ret < 0) {
ret = -ENODEV;
goto sys_time_pps_getparams_exit;
}
/* Return current parameters */
ret = copy_to_user(params, _source[source].params,
sizeof(struct pps_kparams));
if (ret)
ret = -EFAULT;
sys_time_pps_getparams_exit:
mutex_unlock(_mutex);
return ret;
}
The copy_to_user() may sleep and if I change
mutex_lock_interruptible() with a spin_lock I may hold it and then
going to sleep... using mutex we can use the CPU for other
computations.
> > I see. But consider pps_register_source(). This function should
> > provide protection of pps_source against both interrupt context
> > (pps_event()) and user context (maybe pps_unregister_source() or one
> > syscalls). Using only mutex is not possible, since we cannot use mutex
> > in interrupt context, and using only spin_locks is not possible since
> > in UP() they became void.
>
> Yup, but that's okay. On UP, spin_lock_irqsave() becomes local_irq_save()
> which is what you want anyway on UP.
But doing like this may happen that I first execute local_irq_save()
and then go to sleep due copy_to_user()? :-o
> The simplest, most straightforward, and safest, most correct, way would
> be to just use spin_lock_irqsave/restore() to around all access to the
> shared/global data, from _any_ context.
Even if I may sleep while holding a spinlock? Rusty says here
(http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c557.html)
that:
Many functions in the kernel sleep (ie. call schedule()) directly
or indirectly: you can never call them while __holding a
spinlock__, or with preemption disabled. This also means you need
to be in user context: calling them from an interrupt is illegal.
> Anyway, I'll try and see if I find some time this week to implement
> what I was mentioning ...
Thanks a lot, so we can discuss on code. :)
> > That's the point. I don't wish using _irqsave/restore() since they may
> > delay interrupt handler execution. As above, I prefere loosing the
> > event then registering it at wrong time.
>
> Ok, think of it this way -- you don't have an option. You just *have*
> to use them. As I said, please read Rusty Russell's introduction to
> locking in the kernel.
I see but I don't see why using spin_lock/unlock() in user contex and
spin_trylock/unlock() in interrupt context is wrong. :)
> > Why you wish using one lock per sources? Just one lock for the
> > list/array is not enought? :-o
>
> No, I am *not* wishing / advocating that at all. Just that you appear so
> _reluctant_ to use spinlocks and are unnecessarily worrying about
>
Re: LinuxPPS & spinlocks
Hi Rodolfo, On Mon, 30 Jul 2007, Rodolfo Giometti wrote: > On Mon, Jul 30, 2007 at 10:39:38AM +0530, Satyam Sharma wrote: > > > > Nopes, this isn't quite correct/safe. I suggest you should read: > > > > http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/ > > I read it but still I don't see why my solution isn't correct/safe. :) What does the section on locking between hard irq contexts (or between user process context and hard irq context) say? > Can you please propose to me your solution? As I said, you could just use the spin_lock_irqsave/restore() variants ... If you want, I can try and implement the other bits that I had suggested for the other things as well :-) Ciao, Satyam - 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: LinuxPPS & spinlocks
On Mon, 30 Jul 2007, Rodolfo Giometti wrote:
> On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote:
> >
> > Fair enough, but I think the code could become a trifle simpler/easier
> > after the conversion, so probably greater chances of getting merged :-)
>
> I see. I'll start thinging about it.
>
> > But that's alright -- see, as I said, you're confusing between the
> > "special device" that represents the *PPS source* itself, with the port
> > or device that it uses to *physically* connect to the PC.
> >
> > In the RFC, when they say that the userspace app must open(2) the PPS
> > source (as they have illustrated in the example too), they mean that
> > it open(2)'s the special device/file associated with the PPS source,
> > and *not* the /dev/lpXXX or /dev/ttySXXX that it might be connected
> > through physically.
> >
> > So they mean something like /dev/pps0, /dev/pps1 etc instead. Of course,
> > no such special device exists on a Linux box already, but that's fine
> > and obvious! *You* are supposed to create / instantiate that when a
> > pps_register_source() is done from some in-kernel subsystem.
>
> So your are proposing to create a char device interface then using
> syscalls one? In this case how do you manage the case where your GPS
> antenna and PPS source are both connected with the serial port
> (i.e. /dev/ttyS0)?
That's *precisely* what I just explained above!
You create that special device at the time of pps_register_source()!
> Currently the RFC says to you that you should open the serial port:
>
> fd = open("/dev/ttyS0", ...);
No, it does *NOT*. All it says is:
The time_pps_create() is used to convert an already-open UNIX file
descriptor, for an appropriate special file, into a PPS handle.
See? What I said is precisely the implementation the RFC envisages
(and the only sane way to implement it too).
And later, where it gives an example, it shows:
fd = open(PPSfilename, O_RDWR, 0);
What I'm saying is that the "PPSfilename", as is obvious from the name
itself, is *not* a port such as lpXXX or ttySXXX, but an "appropriate
special file" corresponding to a ... PPS source! Really, the RFC is
quite clear and easy to read, I have no idea how to explain that more
clearly ...
> and the passing its filedes to pps_time_create() in order to get the
> corresponding PPS source handler:
>
> pps_time_create(fd, );
Yes.
> As you propose you need _two_ open() and not just one...
No, why?
> and even if
> you decide to open the /dev/ppsX inside the pps_time_create(), how do
> you recognise _which_ /dev/ppsX is connected with filedse "fd"?
That's trivial to implement in the kernel code for the time_pps_create()
syscall.
> I quite sure that RFC is broken since it doesn't take in account that
> a PPS source maybe not connected with any cahr device at all. I tried
> to explain this problem to RFC's gurus but they never answered to me,
> so I decided to resolve the problem by myself. ;)
Nopes, the RFC is not broken at all. All this physical-connection-port
device vs PPS-source-device confusion is just in your mind :-)
> > As I said, it's not the char device for the physical interface itself
> > being discussed there. That could be parport, uart, some arbit GPIO pin
> > whatever. But whenever the corresponding kernel subsystem does a
> > register_source(), you could create the /dev/ppsXXX device ...
>
> Ok, but in this case you still are _not_ RFC compliant (as showed
> above). You need that users give to you _two_ devices (the serial line
> and the PPS source), meanwhile, for the RFC, you just need one. So no
> differences from my solution from this point of view.
Yeah, so how am I not RFC compliant? Userspace will *only* open(2) the
special char device of the *PPS source*, and have *nothing* to do with
the device corresponding to the physical device/port it is connected
through!
> > Hmm, but that's a non-standard, not-mandated-by-RFC syscall. I don't see
> > how you can get this merged, really :-)
>
> They are not-mandated-by-RFC since simply RFC _is broken_! :)
It is not ...
> I need them (or just one of them) in order to find a PPS source into
> the system. Just as you need the second device name in your solution
> with char devices.
No, I don't need any "second device". I *only* need the "appropriate
special file" as mentioned in the RFC. I don't give a *damn* for
what *physical device/port* the source is actually connected through.
I suggest you should read the RFC again ...
Satyam
-
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: LinuxPPS & spinlocks
Hi, On Mon, 30 Jul 2007, Rodolfo Giometti wrote: > On Mon, Jul 30, 2007 at 09:49:20AM +0530, Satyam Sharma wrote: > > > > Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore() > > in pps_event() around the access to pps_source. > > In pps_event() is not useful using spin_lock_irqsave/restore() since > the only difference between spin_lock_irqsave() and spin_lock() is > that the former will turn off interrupts if they are on, otherwise > does nothing (if we are already in an interrupt handler). Yup. But two pps_event()'s on different CPU's could still race. > Maybe you meant I should using spin_lock_irqsave/restore() in user > context, but doing like this I will disable interrupts Yup, but the goal is to avoid races. Otherwise why bother doing any locking at all? > and I don't > wish doing it since, in this manner, the interrupt handler will be > delayed and the (probably) PPS event recording will be wrong. I > prefere loosing the event that registering it at delayed time. What you're risking is not "losing an event" (which, btw, you should not be, either), but a *deadlock*. > > > About using both mutex and spinlock I did it since (I think) I should > > > protect syscalls from each others and from pps_register/unregister(), > > > and pps_event() against pps_register/unregister(). > > > > Nopes, it's not about protecting code from each other, you're needlessly > > complicating things. Locking is pretty simple, really -- any shared data, > > that can be concurrently accessed by multiple threads (or from interrupts) > > must be protected with a lock. Note that *data* is protected by a lock, > > and not "code" that handles it (well, this is the kind of behaviour most > > cases need, at least, including yours). > > Of course, I meant "protecting data". In fact to protect pps_source[] > I need spin_lock() to protect user context from interrupt context and > mutex to protect user context from itself. But that's nonsensical! That's not how you implement locking! First, spin_lock() is *not* enough to protect access from process context from access from interrupt context. Second, if you *already* have a lock to protect any data, introducing *another* lock to protect the same data is ... utterly crazy! > > So here we're introducing the lock to protect *pps_source*, and not keep > > *threads* of execution from stepping over each other. So, simply, just > > ensure you grab the lock whenever you want to start accessing the shared > > data, and release it when you're done. > > I see. But consider pps_register_source(). This function should > provide protection of pps_source against both interrupt context > (pps_event()) and user context (maybe pps_unregister_source() or one > syscalls). Using only mutex is not possible, since we cannot use mutex > in interrupt context, and using only spin_locks is not possible since > in UP() they became void. Yup, but that's okay. On UP, spin_lock_irqsave() becomes local_irq_save() which is what you want anyway on UP. > Can you please show me how I could write pps_register_source() in > order to be correct from your point of view? The simplest, most straightforward, and safest, most correct, way would be to just use spin_lock_irqsave/restore() to around all access to the shared/global data, from _any_ context. Anyway, I'll try and see if I find some time this week to implement what I was mentioning ... > > The _irqsave/restore() variants are required because (say) one of the > > syscalls executing in process context grabs the spinlock. Then, before it > > has released it, it gets interrupted and pps_event() begins executing. > > Now pps_event() also wants to grab the lock, but the syscall already > > has it, so will continue spinning and deadlock! > > That's the point. I don't wish using _irqsave/restore() since they may > delay interrupt handler execution. As above, I prefere loosing the > event then registering it at wrong time. Ok, think of it this way -- you don't have an option. You just *have* to use them. As I said, please read Rusty Russell's introduction to locking in the kernel. > > I think you're unnecessarily worrying about contention here -- you can > > have multiple locks (one for the list, and separate ones for your sources) > > if you're really worrying about contention -- or probably rwlocks. But > > really, rwlocks would end up being *slower* than spinlocks, unless the > > contention is really heavy and it helps to keep multiple readers in the > > critical section. But frankly, with at max a few (I'd expect generally > > one) PPS sources ever to be connected / registered with teh system, and > > just one-pulse-per-second, I don't see why any contention is ever gonna > > happen. > > Why you wish using one lock per sources? Just one lock for the > list/array is not enought? :-o No, I am *not* wishing / advocating that at all. Just that you appear so _reluctant_ to use spinlocks and are unnecessarily worrying about
Re: LinuxPPS & spinlocks
On Mon, Jul 30, 2007 at 10:39:38AM +0530, Satyam Sharma wrote: > > Nopes, this isn't quite correct/safe. I suggest you should read: > > http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/ I read it but still I don't see why my solution isn't correct/safe. :) Can you please propose to me your solution? Thanks a lot for you time! Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS & spinlocks
On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote:
>
> Fair enough, but I think the code could become a trifle simpler/easier
> after the conversion, so probably greater chances of getting merged :-)
I see. I'll start thinging about it.
> But that's alright -- see, as I said, you're confusing between the
> "special device" that represents the *PPS source* itself, with the port
> or device that it uses to *physically* connect to the PC.
>
> In the RFC, when they say that the userspace app must open(2) the PPS
> source (as they have illustrated in the example too), they mean that
> it open(2)'s the special device/file associated with the PPS source,
> and *not* the /dev/lpXXX or /dev/ttySXXX that it might be connected
> through physically.
>
> So they mean something like /dev/pps0, /dev/pps1 etc instead. Of course,
> no such special device exists on a Linux box already, but that's fine
> and obvious! *You* are supposed to create / instantiate that when a
> pps_register_source() is done from some in-kernel subsystem.
So your are proposing to create a char device interface then using
syscalls one? In this case how do you manage the case where your GPS
antenna and PPS source are both connected with the serial port
(i.e. /dev/ttyS0)?
Currently the RFC says to you that you should open the serial port:
fd = open("/dev/ttyS0", ...);
and the passing its filedes to pps_time_create() in order to get the
corresponding PPS source handler:
pps_time_create(fd, );
As you propose you need _two_ open() and not just one... and even if
you decide to open the /dev/ppsX inside the pps_time_create(), how do
you recognise _which_ /dev/ppsX is connected with filedse "fd"?
I quite sure that RFC is broken since it doesn't take in account that
a PPS source maybe not connected with any cahr device at all. I tried
to explain this problem to RFC's gurus but they never answered to me,
so I decided to resolve the problem by myself. ;)
> As I said, it's not the char device for the physical interface itself
> being discussed there. That could be parport, uart, some arbit GPIO pin
> whatever. But whenever the corresponding kernel subsystem does a
> register_source(), you could create the /dev/ppsXXX device ...
Ok, but in this case you still are _not_ RFC compliant (as showed
above). You need that users give to you _two_ devices (the serial line
and the PPS source), meanwhile, for the RFC, you just need one. So no
differences from my solution from this point of view.
> Hmm, but that's a non-standard, not-mandated-by-RFC syscall. I don't see
> how you can get this merged, really :-)
They are not-mandated-by-RFC since simply RFC _is broken_! :)
I need them (or just one of them) in order to find a PPS source into
the system. Just as you need the second device name in your solution
with char devices.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail:[EMAIL PROTECTED]
Linux Device Driver [EMAIL PROTECTED]
Embedded Systems[EMAIL PROTECTED]
UNIX programming phone: +39 349 2432127
-
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: LinuxPPS & spinlocks
On Mon, Jul 30, 2007 at 09:49:20AM +0530, Satyam Sharma wrote: > > Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore() > in pps_event() around the access to pps_source. In pps_event() is not useful using spin_lock_irqsave/restore() since the only difference between spin_lock_irqsave() and spin_lock() is that the former will turn off interrupts if they are on, otherwise does nothing (if we are already in an interrupt handler). Maybe you meant I should using spin_lock_irqsave/restore() in user context, but doing like this I will disable interrupts and I don't wish doing it since, in this manner, the interrupt handler will be delayed and the (probably) PPS event recording will be wrong. I prefere loosing the event that registering it at delayed time. > > About using both mutex and spinlock I did it since (I think) I should > > protect syscalls from each others and from pps_register/unregister(), > > and pps_event() against pps_register/unregister(). > > Nopes, it's not about protecting code from each other, you're needlessly > complicating things. Locking is pretty simple, really -- any shared data, > that can be concurrently accessed by multiple threads (or from interrupts) > must be protected with a lock. Note that *data* is protected by a lock, > and not "code" that handles it (well, this is the kind of behaviour most > cases need, at least, including yours). Of course, I meant "protecting data". In fact to protect pps_source[] I need spin_lock() to protect user context from interrupt context and mutex to protect user context from itself. > So here we're introducing the lock to protect *pps_source*, and not keep > *threads* of execution from stepping over each other. So, simply, just > ensure you grab the lock whenever you want to start accessing the shared > data, and release it when you're done. I see. But consider pps_register_source(). This function should provide protection of pps_source against both interrupt context (pps_event()) and user context (maybe pps_unregister_source() or one syscalls). Using only mutex is not possible, since we cannot use mutex in interrupt context, and using only spin_locks is not possible since in UP() they became void. Can you please show me how I could write pps_register_source() in order to be correct from your point of view? > The _irqsave/restore() variants are required because (say) one of the > syscalls executing in process context grabs the spinlock. Then, before it > has released it, it gets interrupted and pps_event() begins executing. > Now pps_event() also wants to grab the lock, but the syscall already > has it, so will continue spinning and deadlock! That's the point. I don't wish using _irqsave/restore() since they may delay interrupt handler execution. As above, I prefere loosing the event then registering it at wrong time. > I think you're unnecessarily worrying about contention here -- you can > have multiple locks (one for the list, and separate ones for your sources) > if you're really worrying about contention -- or probably rwlocks. But > really, rwlocks would end up being *slower* than spinlocks, unless the > contention is really heavy and it helps to keep multiple readers in the > critical section. But frankly, with at max a few (I'd expect generally > one) PPS sources ever to be connected / registered with teh system, and > just one-pulse-per-second, I don't see why any contention is ever gonna > happen. Why you wish using one lock per sources? Just one lock for the list/array is not enought? :-o Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
On Mon, Jul 30, 2007 at 09:49:20AM +0530, Satyam Sharma wrote: Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore() in pps_event() around the access to pps_source. In pps_event() is not useful using spin_lock_irqsave/restore() since the only difference between spin_lock_irqsave() and spin_lock() is that the former will turn off interrupts if they are on, otherwise does nothing (if we are already in an interrupt handler). Maybe you meant I should using spin_lock_irqsave/restore() in user context, but doing like this I will disable interrupts and I don't wish doing it since, in this manner, the interrupt handler will be delayed and the (probably) PPS event recording will be wrong. I prefere loosing the event that registering it at delayed time. About using both mutex and spinlock I did it since (I think) I should protect syscalls from each others and from pps_register/unregister(), and pps_event() against pps_register/unregister(). Nopes, it's not about protecting code from each other, you're needlessly complicating things. Locking is pretty simple, really -- any shared data, that can be concurrently accessed by multiple threads (or from interrupts) must be protected with a lock. Note that *data* is protected by a lock, and not code that handles it (well, this is the kind of behaviour most cases need, at least, including yours). Of course, I meant protecting data. In fact to protect pps_source[] I need spin_lock() to protect user context from interrupt context and mutex to protect user context from itself. So here we're introducing the lock to protect *pps_source*, and not keep *threads* of execution from stepping over each other. So, simply, just ensure you grab the lock whenever you want to start accessing the shared data, and release it when you're done. I see. But consider pps_register_source(). This function should provide protection of pps_source against both interrupt context (pps_event()) and user context (maybe pps_unregister_source() or one syscalls). Using only mutex is not possible, since we cannot use mutex in interrupt context, and using only spin_locks is not possible since in UP() they became void. Can you please show me how I could write pps_register_source() in order to be correct from your point of view? The _irqsave/restore() variants are required because (say) one of the syscalls executing in process context grabs the spinlock. Then, before it has released it, it gets interrupted and pps_event() begins executing. Now pps_event() also wants to grab the lock, but the syscall already has it, so will continue spinning and deadlock! That's the point. I don't wish using _irqsave/restore() since they may delay interrupt handler execution. As above, I prefere loosing the event then registering it at wrong time. I think you're unnecessarily worrying about contention here -- you can have multiple locks (one for the list, and separate ones for your sources) if you're really worrying about contention -- or probably rwlocks. But really, rwlocks would end up being *slower* than spinlocks, unless the contention is really heavy and it helps to keep multiple readers in the critical section. But frankly, with at max a few (I'd expect generally one) PPS sources ever to be connected / registered with teh system, and just one-pulse-per-second, I don't see why any contention is ever gonna happen. Why you wish using one lock per sources? Just one lock for the list/array is not enought? :-o Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote: Fair enough, but I think the code could become a trifle simpler/easier after the conversion, so probably greater chances of getting merged :-) I see. I'll start thinging about it. But that's alright -- see, as I said, you're confusing between the special device that represents the *PPS source* itself, with the port or device that it uses to *physically* connect to the PC. In the RFC, when they say that the userspace app must open(2) the PPS source (as they have illustrated in the example too), they mean that it open(2)'s the special device/file associated with the PPS source, and *not* the /dev/lpXXX or /dev/ttySXXX that it might be connected through physically. So they mean something like /dev/pps0, /dev/pps1 etc instead. Of course, no such special device exists on a Linux box already, but that's fine and obvious! *You* are supposed to create / instantiate that when a pps_register_source() is done from some in-kernel subsystem. So your are proposing to create a char device interface then using syscalls one? In this case how do you manage the case where your GPS antenna and PPS source are both connected with the serial port (i.e. /dev/ttyS0)? Currently the RFC says to you that you should open the serial port: fd = open(/dev/ttyS0, ...); and the passing its filedes to pps_time_create() in order to get the corresponding PPS source handler: pps_time_create(fd, handler); As you propose you need _two_ open() and not just one... and even if you decide to open the /dev/ppsX inside the pps_time_create(), how do you recognise _which_ /dev/ppsX is connected with filedse fd? I quite sure that RFC is broken since it doesn't take in account that a PPS source maybe not connected with any cahr device at all. I tried to explain this problem to RFC's gurus but they never answered to me, so I decided to resolve the problem by myself. ;) As I said, it's not the char device for the physical interface itself being discussed there. That could be parport, uart, some arbit GPIO pin whatever. But whenever the corresponding kernel subsystem does a register_source(), you could create the /dev/ppsXXX device ... Ok, but in this case you still are _not_ RFC compliant (as showed above). You need that users give to you _two_ devices (the serial line and the PPS source), meanwhile, for the RFC, you just need one. So no differences from my solution from this point of view. Hmm, but that's a non-standard, not-mandated-by-RFC syscall. I don't see how you can get this merged, really :-) They are not-mandated-by-RFC since simply RFC _is broken_! :) I need them (or just one of them) in order to find a PPS source into the system. Just as you need the second device name in your solution with char devices. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
On Mon, Jul 30, 2007 at 10:39:38AM +0530, Satyam Sharma wrote: Nopes, this isn't quite correct/safe. I suggest you should read: http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/ I read it but still I don't see why my solution isn't correct/safe. :) Can you please propose to me your solution? Thanks a lot for you time! Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
Hi, On Mon, 30 Jul 2007, Rodolfo Giometti wrote: On Mon, Jul 30, 2007 at 09:49:20AM +0530, Satyam Sharma wrote: Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore() in pps_event() around the access to pps_source. In pps_event() is not useful using spin_lock_irqsave/restore() since the only difference between spin_lock_irqsave() and spin_lock() is that the former will turn off interrupts if they are on, otherwise does nothing (if we are already in an interrupt handler). Yup. But two pps_event()'s on different CPU's could still race. Maybe you meant I should using spin_lock_irqsave/restore() in user context, but doing like this I will disable interrupts Yup, but the goal is to avoid races. Otherwise why bother doing any locking at all? and I don't wish doing it since, in this manner, the interrupt handler will be delayed and the (probably) PPS event recording will be wrong. I prefere loosing the event that registering it at delayed time. What you're risking is not losing an event (which, btw, you should not be, either), but a *deadlock*. About using both mutex and spinlock I did it since (I think) I should protect syscalls from each others and from pps_register/unregister(), and pps_event() against pps_register/unregister(). Nopes, it's not about protecting code from each other, you're needlessly complicating things. Locking is pretty simple, really -- any shared data, that can be concurrently accessed by multiple threads (or from interrupts) must be protected with a lock. Note that *data* is protected by a lock, and not code that handles it (well, this is the kind of behaviour most cases need, at least, including yours). Of course, I meant protecting data. In fact to protect pps_source[] I need spin_lock() to protect user context from interrupt context and mutex to protect user context from itself. But that's nonsensical! That's not how you implement locking! First, spin_lock() is *not* enough to protect access from process context from access from interrupt context. Second, if you *already* have a lock to protect any data, introducing *another* lock to protect the same data is ... utterly crazy! So here we're introducing the lock to protect *pps_source*, and not keep *threads* of execution from stepping over each other. So, simply, just ensure you grab the lock whenever you want to start accessing the shared data, and release it when you're done. I see. But consider pps_register_source(). This function should provide protection of pps_source against both interrupt context (pps_event()) and user context (maybe pps_unregister_source() or one syscalls). Using only mutex is not possible, since we cannot use mutex in interrupt context, and using only spin_locks is not possible since in UP() they became void. Yup, but that's okay. On UP, spin_lock_irqsave() becomes local_irq_save() which is what you want anyway on UP. Can you please show me how I could write pps_register_source() in order to be correct from your point of view? The simplest, most straightforward, and safest, most correct, way would be to just use spin_lock_irqsave/restore() to around all access to the shared/global data, from _any_ context. Anyway, I'll try and see if I find some time this week to implement what I was mentioning ... The _irqsave/restore() variants are required because (say) one of the syscalls executing in process context grabs the spinlock. Then, before it has released it, it gets interrupted and pps_event() begins executing. Now pps_event() also wants to grab the lock, but the syscall already has it, so will continue spinning and deadlock! That's the point. I don't wish using _irqsave/restore() since they may delay interrupt handler execution. As above, I prefere loosing the event then registering it at wrong time. Ok, think of it this way -- you don't have an option. You just *have* to use them. As I said, please read Rusty Russell's introduction to locking in the kernel. I think you're unnecessarily worrying about contention here -- you can have multiple locks (one for the list, and separate ones for your sources) if you're really worrying about contention -- or probably rwlocks. But really, rwlocks would end up being *slower* than spinlocks, unless the contention is really heavy and it helps to keep multiple readers in the critical section. But frankly, with at max a few (I'd expect generally one) PPS sources ever to be connected / registered with teh system, and just one-pulse-per-second, I don't see why any contention is ever gonna happen. Why you wish using one lock per sources? Just one lock for the list/array is not enought? :-o No, I am *not* wishing / advocating that at all. Just that you appear so _reluctant_ to use spinlocks and are unnecessarily worrying about contention, disabling interrupts, etc etc. Just use the spin_lock_irqsave/restore() variants, and
Re: LinuxPPS spinlocks
On Mon, 30 Jul 2007, Rodolfo Giometti wrote: On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote: Fair enough, but I think the code could become a trifle simpler/easier after the conversion, so probably greater chances of getting merged :-) I see. I'll start thinging about it. But that's alright -- see, as I said, you're confusing between the special device that represents the *PPS source* itself, with the port or device that it uses to *physically* connect to the PC. In the RFC, when they say that the userspace app must open(2) the PPS source (as they have illustrated in the example too), they mean that it open(2)'s the special device/file associated with the PPS source, and *not* the /dev/lpXXX or /dev/ttySXXX that it might be connected through physically. So they mean something like /dev/pps0, /dev/pps1 etc instead. Of course, no such special device exists on a Linux box already, but that's fine and obvious! *You* are supposed to create / instantiate that when a pps_register_source() is done from some in-kernel subsystem. So your are proposing to create a char device interface then using syscalls one? In this case how do you manage the case where your GPS antenna and PPS source are both connected with the serial port (i.e. /dev/ttyS0)? That's *precisely* what I just explained above! You create that special device at the time of pps_register_source()! Currently the RFC says to you that you should open the serial port: fd = open(/dev/ttyS0, ...); No, it does *NOT*. All it says is: The time_pps_create() is used to convert an already-open UNIX file descriptor, for an appropriate special file, into a PPS handle. See? What I said is precisely the implementation the RFC envisages (and the only sane way to implement it too). And later, where it gives an example, it shows: fd = open(PPSfilename, O_RDWR, 0); What I'm saying is that the PPSfilename, as is obvious from the name itself, is *not* a port such as lpXXX or ttySXXX, but an appropriate special file corresponding to a ... PPS source! Really, the RFC is quite clear and easy to read, I have no idea how to explain that more clearly ... and the passing its filedes to pps_time_create() in order to get the corresponding PPS source handler: pps_time_create(fd, handler); Yes. As you propose you need _two_ open() and not just one... No, why? and even if you decide to open the /dev/ppsX inside the pps_time_create(), how do you recognise _which_ /dev/ppsX is connected with filedse fd? That's trivial to implement in the kernel code for the time_pps_create() syscall. I quite sure that RFC is broken since it doesn't take in account that a PPS source maybe not connected with any cahr device at all. I tried to explain this problem to RFC's gurus but they never answered to me, so I decided to resolve the problem by myself. ;) Nopes, the RFC is not broken at all. All this physical-connection-port device vs PPS-source-device confusion is just in your mind :-) As I said, it's not the char device for the physical interface itself being discussed there. That could be parport, uart, some arbit GPIO pin whatever. But whenever the corresponding kernel subsystem does a register_source(), you could create the /dev/ppsXXX device ... Ok, but in this case you still are _not_ RFC compliant (as showed above). You need that users give to you _two_ devices (the serial line and the PPS source), meanwhile, for the RFC, you just need one. So no differences from my solution from this point of view. Yeah, so how am I not RFC compliant? Userspace will *only* open(2) the special char device of the *PPS source*, and have *nothing* to do with the device corresponding to the physical device/port it is connected through! Hmm, but that's a non-standard, not-mandated-by-RFC syscall. I don't see how you can get this merged, really :-) They are not-mandated-by-RFC since simply RFC _is broken_! :) It is not ... I need them (or just one of them) in order to find a PPS source into the system. Just as you need the second device name in your solution with char devices. No, I don't need any second device. I *only* need the appropriate special file as mentioned in the RFC. I don't give a *damn* for what *physical device/port* the source is actually connected through. I suggest you should read the RFC again ... Satyam - 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: LinuxPPS spinlocks
Hi Rodolfo, On Mon, 30 Jul 2007, Rodolfo Giometti wrote: On Mon, Jul 30, 2007 at 10:39:38AM +0530, Satyam Sharma wrote: Nopes, this isn't quite correct/safe. I suggest you should read: http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/ I read it but still I don't see why my solution isn't correct/safe. :) What does the section on locking between hard irq contexts (or between user process context and hard irq context) say? Can you please propose to me your solution? As I said, you could just use the spin_lock_irqsave/restore() variants ... If you want, I can try and implement the other bits that I had suggested for the other things as well :-) Ciao, Satyam - 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: LinuxPPS spinlocks
On Mon, Jul 30, 2007 at 02:37:26PM +0530, Satyam Sharma wrote:
On Mon, 30 Jul 2007, Rodolfo Giometti wrote:
In pps_event() is not useful using spin_lock_irqsave/restore() since
the only difference between spin_lock_irqsave() and spin_lock() is
that the former will turn off interrupts if they are on, otherwise
does nothing (if we are already in an interrupt handler).
Yup. But two pps_event()'s on different CPU's could still race.
??? :-o
Maybe one CPU spins 'till the other holds the lock but any races
should happen...
Maybe you meant I should using spin_lock_irqsave/restore() in user
context, but doing like this I will disable interrupts
Yup, but the goal is to avoid races. Otherwise why bother doing any
locking at all?
I meant that if you have to lock between user context and interrupt
context you have two choises:
1) using spin_lock_irqsave/restore() in user context and spin_lock/unlock()
in the interrupt context (as Rusty says),
2) or using spin_lock/unlock() in user context and
spin_trylock/unlock() in the interrupt context in order to avoid dead
locks.
Is that correct?
Of course, I meant protecting data. In fact to protect pps_source[]
I need spin_lock() to protect user context from interrupt context and
mutex to protect user context from itself.
But that's nonsensical! That's not how you implement locking!
First, spin_lock() is *not* enough to protect access from process context
from access from interrupt context.
Second, if you *already* have a lock to protect any data, introducing
*another* lock to protect the same data is ... utterly crazy!
I see what you mean. But my question is about using spin_locks where
we can sleep. Let me explain, consider sys_time_pps_getparams():
asmlinkage long sys_time_pps_getparams(int source,
struct pps_kparams __user *params)
{
int ret = 0;
pr_debug(%s: source %d\n, __FUNCTION__, source);
/* Sanity checks */
if (!params)
return -EINVAL;
if (mutex_lock_interruptible(pps_mutex))
return -EINTR;
ret = pps_check_source(source);
if (ret 0) {
ret = -ENODEV;
goto sys_time_pps_getparams_exit;
}
/* Return current parameters */
ret = copy_to_user(params, pps_source[source].params,
sizeof(struct pps_kparams));
if (ret)
ret = -EFAULT;
sys_time_pps_getparams_exit:
mutex_unlock(pps_mutex);
return ret;
}
The copy_to_user() may sleep and if I change
mutex_lock_interruptible() with a spin_lock I may hold it and then
going to sleep... using mutex we can use the CPU for other
computations.
I see. But consider pps_register_source(). This function should
provide protection of pps_source against both interrupt context
(pps_event()) and user context (maybe pps_unregister_source() or one
syscalls). Using only mutex is not possible, since we cannot use mutex
in interrupt context, and using only spin_locks is not possible since
in UP() they became void.
Yup, but that's okay. On UP, spin_lock_irqsave() becomes local_irq_save()
which is what you want anyway on UP.
But doing like this may happen that I first execute local_irq_save()
and then go to sleep due copy_to_user()? :-o
The simplest, most straightforward, and safest, most correct, way would
be to just use spin_lock_irqsave/restore() to around all access to the
shared/global data, from _any_ context.
Even if I may sleep while holding a spinlock? Rusty says here
(http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c557.html)
that:
Many functions in the kernel sleep (ie. call schedule()) directly
or indirectly: you can never call them while __holding a
spinlock__, or with preemption disabled. This also means you need
to be in user context: calling them from an interrupt is illegal.
Anyway, I'll try and see if I find some time this week to implement
what I was mentioning ...
Thanks a lot, so we can discuss on code. :)
That's the point. I don't wish using _irqsave/restore() since they may
delay interrupt handler execution. As above, I prefere loosing the
event then registering it at wrong time.
Ok, think of it this way -- you don't have an option. You just *have*
to use them. As I said, please read Rusty Russell's introduction to
locking in the kernel.
I see but I don't see why using spin_lock/unlock() in user contex and
spin_trylock/unlock() in interrupt context is wrong. :)
Why you wish using one lock per sources? Just one lock for the
list/array is not enought? :-o
No, I am *not* wishing / advocating that at all. Just that you appear so
_reluctant_ to use spinlocks and are unnecessarily worrying about
contention, disabling interrupts, etc etc.
Just use the
Re: LinuxPPS spinlocks
Hi Rodolfo, On Mon, 30 Jul 2007, Rodolfo Giometti wrote: On Mon, Jul 30, 2007 at 02:37:26PM +0530, Satyam Sharma wrote: Maybe you meant I should using spin_lock_irqsave/restore() in user context, but doing like this I will disable interrupts Yup, but the goal is to avoid races. Otherwise why bother doing any locking at all? I meant that if you have to lock between user context and interrupt context you have two choises: 1) using spin_lock_irqsave/restore() in user context and spin_lock/unlock() in the interrupt context (as Rusty says), Yes, but we need to use spin_lock_irqsave() from interrupt context to synchronize between two hard irq contexts themselves. 2) or using spin_lock/unlock() in user context and spin_trylock/unlock() in the interrupt context in order to avoid dead locks. Yup, this would avoid races, but then we will lose events. Why is that acceptable, when better alternative (above) exists? Seriously, your aversion to implement good, safe locking is amazing! :-) Of course, I meant protecting data. In fact to protect pps_source[] I need spin_lock() to protect user context from interrupt context and mutex to protect user context from itself. But that's nonsensical! That's not how you implement locking! First, spin_lock() is *not* enough to protect access from process context from access from interrupt context. Second, if you *already* have a lock to protect any data, introducing *another* lock to protect the same data is ... utterly crazy! I see what you mean. But my question is about using spin_locks where we can sleep. Let me explain, consider sys_time_pps_getparams(): [...] The copy_to_user() may sleep and if I change mutex_lock_interruptible() with a spin_lock I may hold it and then going to sleep... using mutex we can use the CPU for other computations. The proper way to do this is to use a kernel buffer to do all kernel-side work (you acquire/release lock during that work) and then copy_to_user() later, at the end. [ And something opposite for the set_xxx syscall, i.e. first off copy_from_user() to a kernel buffer up front, before doing anything else itself, and then do all the work in the kernel on that. ] BTW your syscall implementations totally lack any kind of security checks whatsoever ... Why you wish using one lock per sources? Just one lock for the list/array is not enought? :-o No, I am *not* wishing / advocating that at all. Just that you appear so _reluctant_ to use spinlocks and are unnecessarily worrying about contention, disabling interrupts, etc etc. Just use the spin_lock_irqsave/restore() variants, and you'll be fine. What I wish is just to avoid disabling IRQs in user context in order to minimize the possibility to delay events recording. Amazing. On the one hand, you want to use spin_trylock() in the hard irq handler and spin_lock() (not irq safe) in the process context, because you don't care about losing some events. And now you want to avoid disabling irqs in user context to minimize possibility to delay events recording? Anyway, any such requirement you're talking about is just bogus, IMHO. You're just disabling interrupts to access a data structure, for Gods' sakes, how many nanoseconds do you imagine would you be delaying? As you propose you need _two_ open() and not just one... No, why? Please, take alook at NTPD code. Common usage is: fd = open(/dev/ttyS0, ...); pps_time_create(fd, handler); since RFC supposes that at filedes fd is mapped both GPS data _and_ PPS source. Umm, I don't think the RFC supposes/assumes this anywhere. Furthermore with my pps_findpath() I can do: fd = open(/dev/ttyS0, ...); handler = pps_findpath(/dev/ttyS0, ...); which in most cases its more easy to manage for both user and kernel land. Can do the same with char devices? Of course. BTW time_pps_findpath/findsource do not have to be kernel implemented syscalls in the first place. The best, simplest and most straightforward place to implement them is in userspace -- the RFC mentions this as well. Maybe you can easily create /dev/pps0 but how can you relate it with the /dev/ttyS0 if your GPS antenna and PPS source share the same serial port? A solution I can think of is to create a mapping at the time of pps_register_source() between the PPS source (physically) and the special char device. Userspace open(2)'s the special char device corresponding to the PPS source, and then issues the time_pps_create() syscall. Here, we lookup the mapping previously created and return handle to the PPS source based on the special device's fd that's passed to us in time_pps_create(). I quite sure that RFC is broken since it doesn't take in account that a PPS source maybe not connected with any cahr device at all. I tried to explain this problem to RFC's gurus but they never answered to me, so I decided to resolve the problem by
Re: LinuxPPS & spinlocks
Hi Rodolfo, On Sun, 29 Jul 2007, Rodolfo Giometti wrote: > On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote: > > > Take the race between the time_pps_setparams() syscall and a concurrent > > pps_event() from an interrupt for instance. From sys_time_pps_setparams, > > the parameters for an existing source are not modified / set atomically, > > which means a pps_event() called on the same source in between will see > > invalid parameters ... and bad things will happen. > > I think this should be a good solution... :) > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c > index 08de71d..f0c42ec 100644 > --- a/drivers/pps/kapi.c > +++ b/drivers/pps/kapi.c > @@ -29,12 +29,6 @@ > #include > > /* > - * Local variables > - */ > - > -static spinlock_t pps_lock = SPIN_LOCK_UNLOCKED; > - > -/* > * Local functions > */ > > diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c > index 9176c01..91b7e4d 100644 > --- a/drivers/pps/pps.c > +++ b/drivers/pps/pps.c > @@ -35,6 +35,7 @@ > > struct pps_s pps_source[PPS_MAX_SOURCES]; > DEFINE_MUTEX(pps_mutex); > +spinlock_t pps_lock = SPIN_LOCK_UNLOCKED; > > /* > * Misc functions > @@ -227,6 +228,8 @@ asmlinkage long sys_time_pps_setparams(int source, > goto sys_time_pps_setparams_exit; > } > > + spin_lock(_lock); > + > /* Save the new parameters */ > ret = copy_from_user(_source[source].params, params, > sizeof(struct pps_kparams)); > @@ -245,6 +248,8 @@ asmlinkage long sys_time_pps_setparams(int source, > pps_source[source].params.mode |= PPS_CANWAIT; > pps_source[source].params.api_version = PPS_API_VERS; > > + spin_unlock(_lock); > + > sys_time_pps_setparams_exit: > mutex_unlock(_mutex); Nopes, this isn't quite correct/safe. I suggest you should read: http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/ Satyam - 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: LinuxPPS & spinlocks
Hi, On Sun, 29 Jul 2007, Rodolfo Giometti wrote: > On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote: > > > > [ Also, have you considered making pps_source a list and not an array? > > > It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc > > > kind of gymnastics in there, and you _can_ return a pointer to the > > > corresponding pps source struct from the register() function to the > > > in-kernel > > > users, so that way you get to retain the O(1) access to the corresponding > > > source when a client calls into pps_event(), similar to how you're using > > > the > > > array index presently. ] > > > > I think the above would be sane and safe -- your driver has pretty simple > > lifetime rules, and "sources" are only created / destroyed from within > > kernel, > > as and when clients call pps_register_source() and pps_unregister_source(). > > So pps_event() can be called on a given source only between the > > corresponding register() and unregister() -- which means register() can > > return us a reference/pointer on the source after allocating / adding it to > > the list (instead of the fixed array index as it presently is), which > > remains > > valid for the entire duration of the source, till unregister() is called, > > after > > which we can't be calling pps_event() on the same source anyway. > > Ok. I see. I'll study the problem but I think this is can be done > later, now I think is better having a working code. :) Fair enough, but I think the code could become a trifle simpler/easier after the conversion, so probably greater chances of getting merged :-) > > Ok, I've looked through (most of) the RFC and code now, and am only > > commenting on a design-level for now. Anyway, I didn't like the way > > you've significantly drifted from the RFC in several ways: > > > > 1. The RFC mandates no such userspace interface / syscall as the > > time_pps_cmd() that you've implemented -- it looks, smells, and feels > > like an ioctl, in fact that's what it is for practical purposes. I'm > > confused > > as to why didn't you just go ahead and implement the special-file-and- > > file-descriptor based approach as advocated / mandated there. > > This is because is not always true that a PPS source is connected with > a char (or other) device. But that's alright -- see, as I said, you're confusing between the "special device" that represents the *PPS source* itself, with the port or device that it uses to *physically* connect to the PC. In the RFC, when they say that the userspace app must open(2) the PPS source (as they have illustrated in the example too), they mean that it open(2)'s the special device/file associated with the PPS source, and *not* the /dev/lpXXX or /dev/ttySXXX that it might be connected through physically. So they mean something like /dev/pps0, /dev/pps1 etc instead. Of course, no such special device exists on a Linux box already, but that's fine and obvious! *You* are supposed to create / instantiate that when a pps_register_source() is done from some in-kernel subsystem. > People whose designed RFC didn't think about > systems where the PPS signal is connected with a CPU's GPIO and the > O.S. doesn't provide any char device to refere with! As I said, it's not the char device for the physical interface itself being discussed there. That could be parport, uart, some arbit GPIO pin whatever. But whenever the corresponding kernel subsystem does a register_source(), you could create the /dev/ppsXXX device ... > In the common desktop PCs the GPS antenna is connected with the serial > line and the PPS source is attached to the serial CD, but in the > embedded systems this is _not_ true. GPS antennae may still be > connected with serial line but the PPS signal is usually connected > with a GPIO pin. > > In this scenario you cannot use the serial file descriptor to manage > the PPS signal since it cannot goes to the serial port. See above. > > [ You've implemented the (optional, as per RFC) time_pps_findsource > > operation in the kernel using the above "pseudo-ioctl", but that wasn't > > necessary -- as the RFC itself illustrates, it's something that can easily > > be done (in fact should be done) completely in userspace itself. ] > > I used pseudo-ioctl interface since it allows me to easily extend PPS > support with special, and uncommon, commands. Hmm, but that's a non-standard, not-mandated-by-RFC syscall. I don't see how you can get this merged, really :-) > > * At the time of pps_register_source() -- called by an in-kernel client > > subsystem that creates a PPS source -- allocate a pps source, generate > > an identifier for it, instantiate a special file -- the RFC does not mention > > whether a char or block device, but char device (I noticed an example > > in the RFC where they've used /dev/ppsXX as a possible path) would be > > proper for this. Finally add it to the list of sources. This returns a > > reference/pointer on that
Re: LinuxPPS & spinlocks
Hi Rodolfo, On Sun, 29 Jul 2007, Rodolfo Giometti wrote: > On Sat, Jul 28, 2007 at 02:17:24AM +0530, Satyam Sharma wrote: > > > > I only glanced through the code, so could be wrong, but I noticed that > > the only global / shared data you have in there is a global "pps_source" > > array of pps_s structs. That's accessed / modified from the various > > syscalls introduced in the API exported to userspace, as well as the > > register/unregister/pps_event API exported to in-kernel client subsystems, > > yes? So it looks like you need to introduce proper locking for it, simply > > type-qualifying it as "volatile" is not enough. > > > > However, I think you've introduced two locks for it. The syscalls (that > > run in process context, obviously) seem to use a pps_mutex and > > pps_event() seems to be using the pps_lock spinlock (because that > > gets executed from interrupt context) -- and from the looks of it, the > > register/unregister functions are using /both/ the mutex and spinlock (!) > > This is right. > > > This isn't quite right, (in fact there's nothing to protect pps_event from > > racing against a syscall), so you should use *only* the spinlock for > > synchronization -- the spin_lock_irqsave/restore() variants, in fact. > > We can't use the spin_lock_irqsave/restore() variants since PPS > sources cannot manage IRQ enable/disable. For instance, the serial > source doesn't manage IRQs directly but just uses it to record PPS > events. The serial driver manages the IRQ enable/disable, not the PPS > source which only uses the IRQ handler to records events. Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore() in pps_event() around the access to pps_source. > About using both mutex and spinlock I did it since (I think) I should > protect syscalls from each others and from pps_register/unregister(), > and pps_event() against pps_register/unregister(). Nopes, it's not about protecting code from each other, you're needlessly complicating things. Locking is pretty simple, really -- any shared data, that can be concurrently accessed by multiple threads (or from interrupts) must be protected with a lock. Note that *data* is protected by a lock, and not "code" that handles it (well, this is the kind of behaviour most cases need, at least, including yours). So here we're introducing the lock to protect *pps_source*, and not keep *threads* of execution from stepping over each other. So, simply, just ensure you grab the lock whenever you want to start accessing the shared data, and release it when you're done. The _irqsave/restore() variants are required because (say) one of the syscalls executing in process context grabs the spinlock. Then, before it has released it, it gets interrupted and pps_event() begins executing. Now pps_event() also wants to grab the lock, but the syscall already has it, so will continue spinning and deadlock! > > [ Also, have you considered making pps_source a list and not an array? > > It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc > > kind of gymnastics in there, and you _can_ return a pointer to the > > corresponding pps source struct from the register() function to the > > in-kernel > > users, so that way you get to retain the O(1) access to the corresponding > > source when a client calls into pps_event(), similar to how you're using the > > array index presently. ] > > > > I also noticed code like (from pps_event): > > > > + /* Try to grab the lock, if not we prefere loose the event... */ > > + if (!spin_trylock(_lock)) > > + return; > > > > which looks worrisome and unnecessary. That spinlock looks to be of > > fine enough granularity to me, do you think there'd be any contention > > on it? I /think/ you can simply make that a spin_lock(). > > This is due the fact I cannot manage IRQ enable/disable. What I meant is that you could make it a proper spin_lock() -- or spin_lock_irqsave(), actually -- instead of the _trylock_ variant that it currently is. I think you're unnecessarily worrying about contention here -- you can have multiple locks (one for the list, and separate ones for your sources) if you're really worrying about contention -- or probably rwlocks. But really, rwlocks would end up being *slower* than spinlocks, unless the contention is really heavy and it helps to keep multiple readers in the critical section. But frankly, with at max a few (I'd expect generally one) PPS sources ever to be connected / registered with teh system, and just one-pulse-per-second, I don't see why any contention is ever gonna happen. Satyam - 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: LinuxPPS & spinlocks
On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote:
> Take the race between the time_pps_setparams() syscall and a concurrent
> pps_event() from an interrupt for instance. From sys_time_pps_setparams,
> the parameters for an existing source are not modified / set atomically,
> which means a pps_event() called on the same source in between will see
> invalid parameters ... and bad things will happen.
I think this should be a good solution... :)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 08de71d..f0c42ec 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -29,12 +29,6 @@
#include
/*
- * Local variables
- */
-
-static spinlock_t pps_lock = SPIN_LOCK_UNLOCKED;
-
-/*
* Local functions
*/
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 9176c01..91b7e4d 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -35,6 +35,7 @@
struct pps_s pps_source[PPS_MAX_SOURCES];
DEFINE_MUTEX(pps_mutex);
+spinlock_t pps_lock = SPIN_LOCK_UNLOCKED;
/*
* Misc functions
@@ -227,6 +228,8 @@ asmlinkage long sys_time_pps_setparams(int source,
goto sys_time_pps_setparams_exit;
}
+ spin_lock(_lock);
+
/* Save the new parameters */
ret = copy_from_user(_source[source].params, params,
sizeof(struct pps_kparams));
@@ -245,6 +248,8 @@ asmlinkage long sys_time_pps_setparams(int source,
pps_source[source].params.mode |= PPS_CANWAIT;
pps_source[source].params.api_version = PPS_API_VERS;
+ spin_unlock(_lock);
+
sys_time_pps_setparams_exit:
mutex_unlock(_mutex);
diff --git a/include/linux/pps.h b/include/linux/pps.h
index 6eca3ea..93e7384 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -174,6 +174,7 @@ struct pps_s {
extern struct pps_s pps_source[PPS_MAX_SOURCES];
extern struct mutex pps_mutex;
+extern spinlock_t pps_lock;
/*
* Global functions
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail:[EMAIL PROTECTED]
Linux Device Driver [EMAIL PROTECTED]
Embedded Systems[EMAIL PROTECTED]
UNIX programming phone: +39 349 2432127
-
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: LinuxPPS & spinlocks
On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote: > > Ok, I've looked through (most of) the RFC and code now, and am only > commenting on a design-level for now. Anyway, I didn't like the way > you've significantly drifted from the RFC in several ways: Please, read documentation file Documentation/pps/pps.txt where I exaplain why I did such RFC changes in my implementation. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS & spinlocks
On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote: > > Take the race between the time_pps_setparams() syscall and a concurrent > pps_event() from an interrupt for instance. From sys_time_pps_setparams, > the parameters for an existing source are not modified / set atomically, > which means a pps_event() called on the same source in between will see > invalid parameters ... and bad things will happen. I are right. I'll add spinlocks. :) > > [ Also, have you considered making pps_source a list and not an array? > > It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc > > kind of gymnastics in there, and you _can_ return a pointer to the > > corresponding pps source struct from the register() function to the > > in-kernel > > users, so that way you get to retain the O(1) access to the corresponding > > source when a client calls into pps_event(), similar to how you're using the > > array index presently. ] > > I think the above would be sane and safe -- your driver has pretty simple > lifetime rules, and "sources" are only created / destroyed from within kernel, > as and when clients call pps_register_source() and pps_unregister_source(). > So pps_event() can be called on a given source only between the > corresponding register() and unregister() -- which means register() can > return us a reference/pointer on the source after allocating / adding it to > the list (instead of the fixed array index as it presently is), which remains > valid for the entire duration of the source, till unregister() is called, > after > which we can't be calling pps_event() on the same source anyway. Ok. I see. I'll study the problem but I think this is can be done later, now I think is better having a working code. :) > Ok, I've looked through (most of) the RFC and code now, and am only > commenting on a design-level for now. Anyway, I didn't like the way > you've significantly drifted from the RFC in several ways: > > 1. The RFC mandates no such userspace interface / syscall as the > time_pps_cmd() that you've implemented -- it looks, smells, and feels > like an ioctl, in fact that's what it is for practical purposes. I'm confused > as to why didn't you just go ahead and implement the special-file-and- > file-descriptor based approach as advocated / mandated there. This is because is not always true that a PPS source is connected with a char (or other) device. People whose designed RFC didn't think about systems where the PPS signal is connected with a CPU's GPIO and the O.S. doesn't provide any char device to refere with! In the common desktop PCs the GPS antenna is connected with the serial line and the PPS source is attached to the serial CD, but in the embedded systems this is _not_ true. GPS antennae may still be connected with serial line but the PPS signal is usually connected with a GPIO pin. In this scenario you cannot use the serial file descriptor to manage the PPS signal since it cannot goes to the serial port. > [ You've implemented the (optional, as per RFC) time_pps_findsource > operation in the kernel using the above "pseudo-ioctl", but that wasn't > necessary -- as the RFC itself illustrates, it's something that can easily > be done (in fact should be done) completely in userspace itself. ] I used pseudo-ioctl interface since it allows me to easily extend PPS support with special, and uncommon, commands. > 2. If you fix the above two issues, you'll notice that you don't need to > short-circuit the (RFC-mandated) time_pps_create/destroy(handle) > syscalls in the userspace header/library anymore, as you presently are. This is just the reason why I added those functions. :) > Here's how I'd go about desiging/implementing this: > > * At the time of pps_register_source() -- called by an in-kernel client > subsystem that creates a PPS source -- allocate a pps source, generate > an identifier for it, instantiate a special file -- the RFC does not mention > whether a char or block device, but char device (I noticed an example > in the RFC where they've used /dev/ppsXX as a possible path) would be > proper for this. Finally add it to the list of sources. This returns a > reference/pointer on that source back to the in-kernel client, which then > passes *that* to pps_event(), similar to how you're presently using the > array index. If your GPS antenna is connected with a CPU's GPIO you have _no_ in-kernel client subsystem that creates a PPS source. > [ The way you've passed the path of the parport/uart device itself > (/dev/lpXX or [/dev/%s%d, drv->name, port->line]) to register_source() > in pps_info.path doesn't quite look right to me. Note that the userspace is > expected to open(2) the special file corresponding to the *PPS* source, > as instantiated from the above code, and not the /dev/xyz special file of > the *physical* port through which a pulse-generating device may be > connected to the PC. ] As above, if your GPS antenna is connected with a CPU's GPIO you
Re: LinuxPPS & spinlocks
On Sat, Jul 28, 2007 at 02:17:24AM +0530, Satyam Sharma wrote: > > I only glanced through the code, so could be wrong, but I noticed that > the only global / shared data you have in there is a global "pps_source" > array of pps_s structs. That's accessed / modified from the various > syscalls introduced in the API exported to userspace, as well as the > register/unregister/pps_event API exported to in-kernel client subsystems, > yes? So it looks like you need to introduce proper locking for it, simply > type-qualifying it as "volatile" is not enough. > > However, I think you've introduced two locks for it. The syscalls (that > run in process context, obviously) seem to use a pps_mutex and > pps_event() seems to be using the pps_lock spinlock (because that > gets executed from interrupt context) -- and from the looks of it, the > register/unregister functions are using /both/ the mutex and spinlock (!) This is right. > This isn't quite right, (in fact there's nothing to protect pps_event from > racing against a syscall), so you should use *only* the spinlock for > synchronization -- the spin_lock_irqsave/restore() variants, in fact. We can't use the spin_lock_irqsave/restore() variants since PPS sources cannot manage IRQ enable/disable. For instance, the serial source doesn't manage IRQs directly but just uses it to record PPS events. The serial driver manages the IRQ enable/disable, not the PPS source which only uses the IRQ handler to records events. About using both mutex and spinlock I did it since (I think) I should protect syscalls from each others and from pps_register/unregister(), and pps_event() against pps_register/unregister(). > [ Also, have you considered making pps_source a list and not an array? > It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc > kind of gymnastics in there, and you _can_ return a pointer to the > corresponding pps source struct from the register() function to the in-kernel > users, so that way you get to retain the O(1) access to the corresponding > source when a client calls into pps_event(), similar to how you're using the > array index presently. ] > > I also noticed code like (from pps_event): > > + /* Try to grab the lock, if not we prefere loose the event... */ > + if (!spin_trylock(_lock)) > + return; > > which looks worrisome and unnecessary. That spinlock looks to be of > fine enough granularity to me, do you think there'd be any contention > on it? I /think/ you can simply make that a spin_lock(). This is due the fact I cannot manage IRQ enable/disable. > Overall the code looks simple / straightforward enough to me (except for > the parport / uart stuff that I have no clue about), and I'll also read up on > the relevant RFC for this and would hopefully try and give you a more > meaningful review over the weekend. Thanks a lot for your help! Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
On Sat, Jul 28, 2007 at 02:17:24AM +0530, Satyam Sharma wrote: I only glanced through the code, so could be wrong, but I noticed that the only global / shared data you have in there is a global pps_source array of pps_s structs. That's accessed / modified from the various syscalls introduced in the API exported to userspace, as well as the register/unregister/pps_event API exported to in-kernel client subsystems, yes? So it looks like you need to introduce proper locking for it, simply type-qualifying it as volatile is not enough. However, I think you've introduced two locks for it. The syscalls (that run in process context, obviously) seem to use a pps_mutex and pps_event() seems to be using the pps_lock spinlock (because that gets executed from interrupt context) -- and from the looks of it, the register/unregister functions are using /both/ the mutex and spinlock (!) This is right. This isn't quite right, (in fact there's nothing to protect pps_event from racing against a syscall), so you should use *only* the spinlock for synchronization -- the spin_lock_irqsave/restore() variants, in fact. We can't use the spin_lock_irqsave/restore() variants since PPS sources cannot manage IRQ enable/disable. For instance, the serial source doesn't manage IRQs directly but just uses it to record PPS events. The serial driver manages the IRQ enable/disable, not the PPS source which only uses the IRQ handler to records events. About using both mutex and spinlock I did it since (I think) I should protect syscalls from each others and from pps_register/unregister(), and pps_event() against pps_register/unregister(). [ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ] I also noticed code like (from pps_event): + /* Try to grab the lock, if not we prefere loose the event... */ + if (!spin_trylock(pps_lock)) + return; which looks worrisome and unnecessary. That spinlock looks to be of fine enough granularity to me, do you think there'd be any contention on it? I /think/ you can simply make that a spin_lock(). This is due the fact I cannot manage IRQ enable/disable. Overall the code looks simple / straightforward enough to me (except for the parport / uart stuff that I have no clue about), and I'll also read up on the relevant RFC for this and would hopefully try and give you a more meaningful review over the weekend. Thanks a lot for your help! Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote: Take the race between the time_pps_setparams() syscall and a concurrent pps_event() from an interrupt for instance. From sys_time_pps_setparams, the parameters for an existing source are not modified / set atomically, which means a pps_event() called on the same source in between will see invalid parameters ... and bad things will happen. I are right. I'll add spinlocks. :) [ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ] I think the above would be sane and safe -- your driver has pretty simple lifetime rules, and sources are only created / destroyed from within kernel, as and when clients call pps_register_source() and pps_unregister_source(). So pps_event() can be called on a given source only between the corresponding register() and unregister() -- which means register() can return us a reference/pointer on the source after allocating / adding it to the list (instead of the fixed array index as it presently is), which remains valid for the entire duration of the source, till unregister() is called, after which we can't be calling pps_event() on the same source anyway. Ok. I see. I'll study the problem but I think this is can be done later, now I think is better having a working code. :) Ok, I've looked through (most of) the RFC and code now, and am only commenting on a design-level for now. Anyway, I didn't like the way you've significantly drifted from the RFC in several ways: 1. The RFC mandates no such userspace interface / syscall as the time_pps_cmd() that you've implemented -- it looks, smells, and feels like an ioctl, in fact that's what it is for practical purposes. I'm confused as to why didn't you just go ahead and implement the special-file-and- file-descriptor based approach as advocated / mandated there. This is because is not always true that a PPS source is connected with a char (or other) device. People whose designed RFC didn't think about systems where the PPS signal is connected with a CPU's GPIO and the O.S. doesn't provide any char device to refere with! In the common desktop PCs the GPS antenna is connected with the serial line and the PPS source is attached to the serial CD, but in the embedded systems this is _not_ true. GPS antennae may still be connected with serial line but the PPS signal is usually connected with a GPIO pin. In this scenario you cannot use the serial file descriptor to manage the PPS signal since it cannot goes to the serial port. [ You've implemented the (optional, as per RFC) time_pps_findsource operation in the kernel using the above pseudo-ioctl, but that wasn't necessary -- as the RFC itself illustrates, it's something that can easily be done (in fact should be done) completely in userspace itself. ] I used pseudo-ioctl interface since it allows me to easily extend PPS support with special, and uncommon, commands. 2. If you fix the above two issues, you'll notice that you don't need to short-circuit the (RFC-mandated) time_pps_create/destroy(handle) syscalls in the userspace header/library anymore, as you presently are. This is just the reason why I added those functions. :) Here's how I'd go about desiging/implementing this: * At the time of pps_register_source() -- called by an in-kernel client subsystem that creates a PPS source -- allocate a pps source, generate an identifier for it, instantiate a special file -- the RFC does not mention whether a char or block device, but char device (I noticed an example in the RFC where they've used /dev/ppsXX as a possible path) would be proper for this. Finally add it to the list of sources. This returns a reference/pointer on that source back to the in-kernel client, which then passes *that* to pps_event(), similar to how you're presently using the array index. If your GPS antenna is connected with a CPU's GPIO you have _no_ in-kernel client subsystem that creates a PPS source. [ The way you've passed the path of the parport/uart device itself (/dev/lpXX or [/dev/%s%d, drv-name, port-line]) to register_source() in pps_info.path doesn't quite look right to me. Note that the userspace is expected to open(2) the special file corresponding to the *PPS* source, as instantiated from the above code, and not the /dev/xyz special file of the *physical* port through which a pulse-generating device may be connected to the PC. ] As above, if your GPS antenna is connected with a CPU's GPIO you have _no_ device to open at all, that's why you need at least a function
Re: LinuxPPS spinlocks
On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote: Ok, I've looked through (most of) the RFC and code now, and am only commenting on a design-level for now. Anyway, I didn't like the way you've significantly drifted from the RFC in several ways: Please, read documentation file Documentation/pps/pps.txt where I exaplain why I did such RFC changes in my implementation. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote:
Take the race between the time_pps_setparams() syscall and a concurrent
pps_event() from an interrupt for instance. From sys_time_pps_setparams,
the parameters for an existing source are not modified / set atomically,
which means a pps_event() called on the same source in between will see
invalid parameters ... and bad things will happen.
I think this should be a good solution... :)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 08de71d..f0c42ec 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -29,12 +29,6 @@
#include linux/pps.h
/*
- * Local variables
- */
-
-static spinlock_t pps_lock = SPIN_LOCK_UNLOCKED;
-
-/*
* Local functions
*/
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 9176c01..91b7e4d 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -35,6 +35,7 @@
struct pps_s pps_source[PPS_MAX_SOURCES];
DEFINE_MUTEX(pps_mutex);
+spinlock_t pps_lock = SPIN_LOCK_UNLOCKED;
/*
* Misc functions
@@ -227,6 +228,8 @@ asmlinkage long sys_time_pps_setparams(int source,
goto sys_time_pps_setparams_exit;
}
+ spin_lock(pps_lock);
+
/* Save the new parameters */
ret = copy_from_user(pps_source[source].params, params,
sizeof(struct pps_kparams));
@@ -245,6 +248,8 @@ asmlinkage long sys_time_pps_setparams(int source,
pps_source[source].params.mode |= PPS_CANWAIT;
pps_source[source].params.api_version = PPS_API_VERS;
+ spin_unlock(pps_lock);
+
sys_time_pps_setparams_exit:
mutex_unlock(pps_mutex);
diff --git a/include/linux/pps.h b/include/linux/pps.h
index 6eca3ea..93e7384 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -174,6 +174,7 @@ struct pps_s {
extern struct pps_s pps_source[PPS_MAX_SOURCES];
extern struct mutex pps_mutex;
+extern spinlock_t pps_lock;
/*
* Global functions
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail:[EMAIL PROTECTED]
Linux Device Driver [EMAIL PROTECTED]
Embedded Systems[EMAIL PROTECTED]
UNIX programming phone: +39 349 2432127
-
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: LinuxPPS spinlocks
Hi Rodolfo, On Sun, 29 Jul 2007, Rodolfo Giometti wrote: On Sat, Jul 28, 2007 at 02:17:24AM +0530, Satyam Sharma wrote: I only glanced through the code, so could be wrong, but I noticed that the only global / shared data you have in there is a global pps_source array of pps_s structs. That's accessed / modified from the various syscalls introduced in the API exported to userspace, as well as the register/unregister/pps_event API exported to in-kernel client subsystems, yes? So it looks like you need to introduce proper locking for it, simply type-qualifying it as volatile is not enough. However, I think you've introduced two locks for it. The syscalls (that run in process context, obviously) seem to use a pps_mutex and pps_event() seems to be using the pps_lock spinlock (because that gets executed from interrupt context) -- and from the looks of it, the register/unregister functions are using /both/ the mutex and spinlock (!) This is right. This isn't quite right, (in fact there's nothing to protect pps_event from racing against a syscall), so you should use *only* the spinlock for synchronization -- the spin_lock_irqsave/restore() variants, in fact. We can't use the spin_lock_irqsave/restore() variants since PPS sources cannot manage IRQ enable/disable. For instance, the serial source doesn't manage IRQs directly but just uses it to record PPS events. The serial driver manages the IRQ enable/disable, not the PPS source which only uses the IRQ handler to records events. Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore() in pps_event() around the access to pps_source. About using both mutex and spinlock I did it since (I think) I should protect syscalls from each others and from pps_register/unregister(), and pps_event() against pps_register/unregister(). Nopes, it's not about protecting code from each other, you're needlessly complicating things. Locking is pretty simple, really -- any shared data, that can be concurrently accessed by multiple threads (or from interrupts) must be protected with a lock. Note that *data* is protected by a lock, and not code that handles it (well, this is the kind of behaviour most cases need, at least, including yours). So here we're introducing the lock to protect *pps_source*, and not keep *threads* of execution from stepping over each other. So, simply, just ensure you grab the lock whenever you want to start accessing the shared data, and release it when you're done. The _irqsave/restore() variants are required because (say) one of the syscalls executing in process context grabs the spinlock. Then, before it has released it, it gets interrupted and pps_event() begins executing. Now pps_event() also wants to grab the lock, but the syscall already has it, so will continue spinning and deadlock! [ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ] I also noticed code like (from pps_event): + /* Try to grab the lock, if not we prefere loose the event... */ + if (!spin_trylock(pps_lock)) + return; which looks worrisome and unnecessary. That spinlock looks to be of fine enough granularity to me, do you think there'd be any contention on it? I /think/ you can simply make that a spin_lock(). This is due the fact I cannot manage IRQ enable/disable. What I meant is that you could make it a proper spin_lock() -- or spin_lock_irqsave(), actually -- instead of the _trylock_ variant that it currently is. I think you're unnecessarily worrying about contention here -- you can have multiple locks (one for the list, and separate ones for your sources) if you're really worrying about contention -- or probably rwlocks. But really, rwlocks would end up being *slower* than spinlocks, unless the contention is really heavy and it helps to keep multiple readers in the critical section. But frankly, with at max a few (I'd expect generally one) PPS sources ever to be connected / registered with teh system, and just one-pulse-per-second, I don't see why any contention is ever gonna happen. Satyam - 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: LinuxPPS spinlocks
Hi, On Sun, 29 Jul 2007, Rodolfo Giometti wrote: On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote: [ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ] I think the above would be sane and safe -- your driver has pretty simple lifetime rules, and sources are only created / destroyed from within kernel, as and when clients call pps_register_source() and pps_unregister_source(). So pps_event() can be called on a given source only between the corresponding register() and unregister() -- which means register() can return us a reference/pointer on the source after allocating / adding it to the list (instead of the fixed array index as it presently is), which remains valid for the entire duration of the source, till unregister() is called, after which we can't be calling pps_event() on the same source anyway. Ok. I see. I'll study the problem but I think this is can be done later, now I think is better having a working code. :) Fair enough, but I think the code could become a trifle simpler/easier after the conversion, so probably greater chances of getting merged :-) Ok, I've looked through (most of) the RFC and code now, and am only commenting on a design-level for now. Anyway, I didn't like the way you've significantly drifted from the RFC in several ways: 1. The RFC mandates no such userspace interface / syscall as the time_pps_cmd() that you've implemented -- it looks, smells, and feels like an ioctl, in fact that's what it is for practical purposes. I'm confused as to why didn't you just go ahead and implement the special-file-and- file-descriptor based approach as advocated / mandated there. This is because is not always true that a PPS source is connected with a char (or other) device. But that's alright -- see, as I said, you're confusing between the special device that represents the *PPS source* itself, with the port or device that it uses to *physically* connect to the PC. In the RFC, when they say that the userspace app must open(2) the PPS source (as they have illustrated in the example too), they mean that it open(2)'s the special device/file associated with the PPS source, and *not* the /dev/lpXXX or /dev/ttySXXX that it might be connected through physically. So they mean something like /dev/pps0, /dev/pps1 etc instead. Of course, no such special device exists on a Linux box already, but that's fine and obvious! *You* are supposed to create / instantiate that when a pps_register_source() is done from some in-kernel subsystem. People whose designed RFC didn't think about systems where the PPS signal is connected with a CPU's GPIO and the O.S. doesn't provide any char device to refere with! As I said, it's not the char device for the physical interface itself being discussed there. That could be parport, uart, some arbit GPIO pin whatever. But whenever the corresponding kernel subsystem does a register_source(), you could create the /dev/ppsXXX device ... In the common desktop PCs the GPS antenna is connected with the serial line and the PPS source is attached to the serial CD, but in the embedded systems this is _not_ true. GPS antennae may still be connected with serial line but the PPS signal is usually connected with a GPIO pin. In this scenario you cannot use the serial file descriptor to manage the PPS signal since it cannot goes to the serial port. See above. [ You've implemented the (optional, as per RFC) time_pps_findsource operation in the kernel using the above pseudo-ioctl, but that wasn't necessary -- as the RFC itself illustrates, it's something that can easily be done (in fact should be done) completely in userspace itself. ] I used pseudo-ioctl interface since it allows me to easily extend PPS support with special, and uncommon, commands. Hmm, but that's a non-standard, not-mandated-by-RFC syscall. I don't see how you can get this merged, really :-) * At the time of pps_register_source() -- called by an in-kernel client subsystem that creates a PPS source -- allocate a pps source, generate an identifier for it, instantiate a special file -- the RFC does not mention whether a char or block device, but char device (I noticed an example in the RFC where they've used /dev/ppsXX as a possible path) would be proper for this. Finally add it to the list of sources. This returns a reference/pointer on that source back to the in-kernel client, which then passes *that* to pps_event(), similar to how you're presently using the
Re: LinuxPPS spinlocks
Hi Rodolfo, On Sun, 29 Jul 2007, Rodolfo Giometti wrote: On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote: Take the race between the time_pps_setparams() syscall and a concurrent pps_event() from an interrupt for instance. From sys_time_pps_setparams, the parameters for an existing source are not modified / set atomically, which means a pps_event() called on the same source in between will see invalid parameters ... and bad things will happen. I think this should be a good solution... :) diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c index 08de71d..f0c42ec 100644 --- a/drivers/pps/kapi.c +++ b/drivers/pps/kapi.c @@ -29,12 +29,6 @@ #include linux/pps.h /* - * Local variables - */ - -static spinlock_t pps_lock = SPIN_LOCK_UNLOCKED; - -/* * Local functions */ diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c index 9176c01..91b7e4d 100644 --- a/drivers/pps/pps.c +++ b/drivers/pps/pps.c @@ -35,6 +35,7 @@ struct pps_s pps_source[PPS_MAX_SOURCES]; DEFINE_MUTEX(pps_mutex); +spinlock_t pps_lock = SPIN_LOCK_UNLOCKED; /* * Misc functions @@ -227,6 +228,8 @@ asmlinkage long sys_time_pps_setparams(int source, goto sys_time_pps_setparams_exit; } + spin_lock(pps_lock); + /* Save the new parameters */ ret = copy_from_user(pps_source[source].params, params, sizeof(struct pps_kparams)); @@ -245,6 +248,8 @@ asmlinkage long sys_time_pps_setparams(int source, pps_source[source].params.mode |= PPS_CANWAIT; pps_source[source].params.api_version = PPS_API_VERS; + spin_unlock(pps_lock); + sys_time_pps_setparams_exit: mutex_unlock(pps_mutex); Nopes, this isn't quite correct/safe. I suggest you should read: http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/ Satyam - 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: LinuxPPS & spinlocks
Hi, On 7/28/07, Satyam Sharma <[EMAIL PROTECTED]> wrote: > Hi Rodolfo, > > On 7/28/07, Rodolfo Giometti <[EMAIL PROTECTED]> wrote: > > On Fri, Jul 27, 2007 at 01:40:14PM -0600, Chris Friesen wrote: > > > > > > My point is that the lock should be used to protect specific data. Thus, > > > it > > > would be more correct to say, "spinlock foo is taken because > > > pps_register_source() accesses variable bar". > > > > > > That way, if someone else wants to access "bar", they know that they need > > > to take lock "foo". > > > > Ah, ok! I see. :) > > I only glanced through the code, so could be wrong, but I noticed that > the only global / shared data you have in there is a global "pps_source" > array of pps_s structs. That's accessed / modified from the various > syscalls introduced in the API exported to userspace, as well as the > register/unregister/pps_event API exported to in-kernel client subsystems, > yes? So it looks like you need to introduce proper locking for it, simply > type-qualifying it as "volatile" is not enough. > > However, I think you've introduced two locks for it. The syscalls (that > run in process context, obviously) seem to use a pps_mutex and > pps_event() seems to be using the pps_lock spinlock (because that > gets executed from interrupt context) -- and from the looks of it, the > register/unregister functions are using /both/ the mutex and spinlock (!) > > This isn't quite right, (in fact there's nothing to protect pps_event from > racing against a syscall), so you should use *only* the spinlock for > synchronization -- the spin_lock_irqsave/restore() variants, in fact. Take the race between the time_pps_setparams() syscall and a concurrent pps_event() from an interrupt for instance. From sys_time_pps_setparams, the parameters for an existing source are not modified / set atomically, which means a pps_event() called on the same source in between will see invalid parameters ... and bad things will happen. > [ Also, have you considered making pps_source a list and not an array? > It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc > kind of gymnastics in there, and you _can_ return a pointer to the > corresponding pps source struct from the register() function to the in-kernel > users, so that way you get to retain the O(1) access to the corresponding > source when a client calls into pps_event(), similar to how you're using the > array index presently. ] I think the above would be sane and safe -- your driver has pretty simple lifetime rules, and "sources" are only created / destroyed from within kernel, as and when clients call pps_register_source() and pps_unregister_source(). So pps_event() can be called on a given source only between the corresponding register() and unregister() -- which means register() can return us a reference/pointer on the source after allocating / adding it to the list (instead of the fixed array index as it presently is), which remains valid for the entire duration of the source, till unregister() is called, after which we can't be calling pps_event() on the same source anyway. > I also noticed code like (from pps_event): > > + /* Try to grab the lock, if not we prefere loose the event... */ > + if (!spin_trylock(_lock)) > + return; > > which looks worrisome and unnecessary. That spinlock looks to be of > fine enough granularity to me, do you think there'd be any contention > on it? I /think/ you can simply make that a spin_lock(). > > Overall the code looks simple / straightforward enough to me (except for > the parport / uart stuff that I have no clue about), and I'll also read up on > the relevant RFC for this and would hopefully try and give you a more > meaningful review over the weekend. Ok, I've looked through (most of) the RFC and code now, and am only commenting on a design-level for now. Anyway, I didn't like the way you've significantly drifted from the RFC in several ways: 1. The RFC mandates no such userspace interface / syscall as the time_pps_cmd() that you've implemented -- it looks, smells, and feels like an ioctl, in fact that's what it is for practical purposes. I'm confused as to why didn't you just go ahead and implement the special-file-and- file-descriptor based approach as advocated / mandated there. [ You've implemented the (optional, as per RFC) time_pps_findsource operation in the kernel using the above "pseudo-ioctl", but that wasn't necessary -- as the RFC itself illustrates, it's something that can easily be done (in fact should be done) completely in userspace itself. ] 2. If you fix the above two issues, you'll notice that you don't need to short-circuit the (RFC-mandated) time_pps_create/destroy(handle) syscalls in the userspace header/library anymore, as you presently are. Here's how I'd go about desiging/implementing this: * At the time of pps_register_source() -- called by an in-kernel client subsystem that creates a PPS source -- allocate a pps source, generate
Re: LinuxPPS & spinlocks
Hi Rodolfo, On 7/28/07, Rodolfo Giometti <[EMAIL PROTECTED]> wrote: > On Fri, Jul 27, 2007 at 01:40:14PM -0600, Chris Friesen wrote: > > > > My point is that the lock should be used to protect specific data. Thus, it > > would be more correct to say, "spinlock foo is taken because > > pps_register_source() accesses variable bar". > > > > That way, if someone else wants to access "bar", they know that they need > > to take lock "foo". > > Ah, ok! I see. :) I only glanced through the code, so could be wrong, but I noticed that the only global / shared data you have in there is a global "pps_source" array of pps_s structs. That's accessed / modified from the various syscalls introduced in the API exported to userspace, as well as the register/unregister/pps_event API exported to in-kernel client subsystems, yes? So it looks like you need to introduce proper locking for it, simply type-qualifying it as "volatile" is not enough. However, I think you've introduced two locks for it. The syscalls (that run in process context, obviously) seem to use a pps_mutex and pps_event() seems to be using the pps_lock spinlock (because that gets executed from interrupt context) -- and from the looks of it, the register/unregister functions are using /both/ the mutex and spinlock (!) This isn't quite right, (in fact there's nothing to protect pps_event from racing against a syscall), so you should use *only* the spinlock for synchronization -- the spin_lock_irqsave/restore() variants, in fact. [ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ] I also noticed code like (from pps_event): + /* Try to grab the lock, if not we prefere loose the event... */ + if (!spin_trylock(_lock)) + return; which looks worrisome and unnecessary. That spinlock looks to be of fine enough granularity to me, do you think there'd be any contention on it? I /think/ you can simply make that a spin_lock(). Overall the code looks simple / straightforward enough to me (except for the parport / uart stuff that I have no clue about), and I'll also read up on the relevant RFC for this and would hopefully try and give you a more meaningful review over the weekend. Thanks, Satyam - 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: LinuxPPS & spinlocks
On Fri, Jul 27, 2007 at 01:40:14PM -0600, Chris Friesen wrote: > > My point is that the lock should be used to protect specific data. Thus, it > would be more correct to say, "spinlock foo is taken because > pps_register_source() accesses variable bar". > > That way, if someone else wants to access "bar", they know that they need > to take lock "foo". Ah, ok! I see. :) Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS & spinlocks
Rodolfo Giometti wrote: What do you mean? Did you find an error into my patch? :-o Functions pps_event() and pps_register_source()/pps_unregister_source() take accesso to shared data, that's why I used spinlocks. My point is that the lock should be used to protect specific data. Thus, it would be more correct to say, "spinlock foo is taken because pps_register_source() accesses variable bar". That way, if someone else wants to access "bar", they know that they need to take lock "foo". Chris - 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: LinuxPPS & spinlocks
On Fri, Jul 27, 2007 at 01:08:58PM -0600, Chris Friesen wrote: > Rodolfo Giometti wrote: > >> The pps_event() is now protected by a spinlock against >> pps_register_source() and pps_unregister_source()... > > Locks protect data, not code. It may make more sense to identify the > specific data being protected by the spinlock. What do you mean? Did you find an error into my patch? :-o Functions pps_event() and pps_register_source()/pps_unregister_source() take accesso to shared data, that's why I used spinlocks. Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS & spinlocks
Rodolfo Giometti wrote: The pps_event() is now protected by a spinlock against pps_register_source() and pps_unregister_source()... Locks protect data, not code. It may make more sense to identify the specific data being protected by the spinlock. Chris - 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: LinuxPPS spinlocks
Rodolfo Giometti wrote: The pps_event() is now protected by a spinlock against pps_register_source() and pps_unregister_source()... Locks protect data, not code. It may make more sense to identify the specific data being protected by the spinlock. Chris - 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: LinuxPPS spinlocks
Rodolfo Giometti wrote: What do you mean? Did you find an error into my patch? :-o Functions pps_event() and pps_register_source()/pps_unregister_source() take accesso to shared data, that's why I used spinlocks. My point is that the lock should be used to protect specific data. Thus, it would be more correct to say, spinlock foo is taken because pps_register_source() accesses variable bar. That way, if someone else wants to access bar, they know that they need to take lock foo. Chris - 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: LinuxPPS spinlocks
On Fri, Jul 27, 2007 at 01:40:14PM -0600, Chris Friesen wrote: My point is that the lock should be used to protect specific data. Thus, it would be more correct to say, spinlock foo is taken because pps_register_source() accesses variable bar. That way, if someone else wants to access bar, they know that they need to take lock foo. Ah, ok! I see. :) Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
On Fri, Jul 27, 2007 at 01:08:58PM -0600, Chris Friesen wrote: Rodolfo Giometti wrote: The pps_event() is now protected by a spinlock against pps_register_source() and pps_unregister_source()... Locks protect data, not code. It may make more sense to identify the specific data being protected by the spinlock. What do you mean? Did you find an error into my patch? :-o Functions pps_event() and pps_register_source()/pps_unregister_source() take accesso to shared data, that's why I used spinlocks. Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: LinuxPPS spinlocks
Hi Rodolfo, On 7/28/07, Rodolfo Giometti [EMAIL PROTECTED] wrote: On Fri, Jul 27, 2007 at 01:40:14PM -0600, Chris Friesen wrote: My point is that the lock should be used to protect specific data. Thus, it would be more correct to say, spinlock foo is taken because pps_register_source() accesses variable bar. That way, if someone else wants to access bar, they know that they need to take lock foo. Ah, ok! I see. :) I only glanced through the code, so could be wrong, but I noticed that the only global / shared data you have in there is a global pps_source array of pps_s structs. That's accessed / modified from the various syscalls introduced in the API exported to userspace, as well as the register/unregister/pps_event API exported to in-kernel client subsystems, yes? So it looks like you need to introduce proper locking for it, simply type-qualifying it as volatile is not enough. However, I think you've introduced two locks for it. The syscalls (that run in process context, obviously) seem to use a pps_mutex and pps_event() seems to be using the pps_lock spinlock (because that gets executed from interrupt context) -- and from the looks of it, the register/unregister functions are using /both/ the mutex and spinlock (!) This isn't quite right, (in fact there's nothing to protect pps_event from racing against a syscall), so you should use *only* the spinlock for synchronization -- the spin_lock_irqsave/restore() variants, in fact. [ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ] I also noticed code like (from pps_event): + /* Try to grab the lock, if not we prefere loose the event... */ + if (!spin_trylock(pps_lock)) + return; which looks worrisome and unnecessary. That spinlock looks to be of fine enough granularity to me, do you think there'd be any contention on it? I /think/ you can simply make that a spin_lock(). Overall the code looks simple / straightforward enough to me (except for the parport / uart stuff that I have no clue about), and I'll also read up on the relevant RFC for this and would hopefully try and give you a more meaningful review over the weekend. Thanks, Satyam - 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: LinuxPPS spinlocks
Hi, On 7/28/07, Satyam Sharma [EMAIL PROTECTED] wrote: Hi Rodolfo, On 7/28/07, Rodolfo Giometti [EMAIL PROTECTED] wrote: On Fri, Jul 27, 2007 at 01:40:14PM -0600, Chris Friesen wrote: My point is that the lock should be used to protect specific data. Thus, it would be more correct to say, spinlock foo is taken because pps_register_source() accesses variable bar. That way, if someone else wants to access bar, they know that they need to take lock foo. Ah, ok! I see. :) I only glanced through the code, so could be wrong, but I noticed that the only global / shared data you have in there is a global pps_source array of pps_s structs. That's accessed / modified from the various syscalls introduced in the API exported to userspace, as well as the register/unregister/pps_event API exported to in-kernel client subsystems, yes? So it looks like you need to introduce proper locking for it, simply type-qualifying it as volatile is not enough. However, I think you've introduced two locks for it. The syscalls (that run in process context, obviously) seem to use a pps_mutex and pps_event() seems to be using the pps_lock spinlock (because that gets executed from interrupt context) -- and from the looks of it, the register/unregister functions are using /both/ the mutex and spinlock (!) This isn't quite right, (in fact there's nothing to protect pps_event from racing against a syscall), so you should use *only* the spinlock for synchronization -- the spin_lock_irqsave/restore() variants, in fact. Take the race between the time_pps_setparams() syscall and a concurrent pps_event() from an interrupt for instance. From sys_time_pps_setparams, the parameters for an existing source are not modified / set atomically, which means a pps_event() called on the same source in between will see invalid parameters ... and bad things will happen. [ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ] I think the above would be sane and safe -- your driver has pretty simple lifetime rules, and sources are only created / destroyed from within kernel, as and when clients call pps_register_source() and pps_unregister_source(). So pps_event() can be called on a given source only between the corresponding register() and unregister() -- which means register() can return us a reference/pointer on the source after allocating / adding it to the list (instead of the fixed array index as it presently is), which remains valid for the entire duration of the source, till unregister() is called, after which we can't be calling pps_event() on the same source anyway. I also noticed code like (from pps_event): + /* Try to grab the lock, if not we prefere loose the event... */ + if (!spin_trylock(pps_lock)) + return; which looks worrisome and unnecessary. That spinlock looks to be of fine enough granularity to me, do you think there'd be any contention on it? I /think/ you can simply make that a spin_lock(). Overall the code looks simple / straightforward enough to me (except for the parport / uart stuff that I have no clue about), and I'll also read up on the relevant RFC for this and would hopefully try and give you a more meaningful review over the weekend. Ok, I've looked through (most of) the RFC and code now, and am only commenting on a design-level for now. Anyway, I didn't like the way you've significantly drifted from the RFC in several ways: 1. The RFC mandates no such userspace interface / syscall as the time_pps_cmd() that you've implemented -- it looks, smells, and feels like an ioctl, in fact that's what it is for practical purposes. I'm confused as to why didn't you just go ahead and implement the special-file-and- file-descriptor based approach as advocated / mandated there. [ You've implemented the (optional, as per RFC) time_pps_findsource operation in the kernel using the above pseudo-ioctl, but that wasn't necessary -- as the RFC itself illustrates, it's something that can easily be done (in fact should be done) completely in userspace itself. ] 2. If you fix the above two issues, you'll notice that you don't need to short-circuit the (RFC-mandated) time_pps_create/destroy(handle) syscalls in the userspace header/library anymore, as you presently are. Here's how I'd go about desiging/implementing this: * At the time of pps_register_source() -- called by an in-kernel client subsystem that creates a PPS source -- allocate a pps source, generate an identifier for it, instantiate a special file -- the RFC does not mention whether a
