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 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. We this
requirement the only solution I see is using spin_trylock/unlock() in
interrupt context.

Another requirement is __not__ going to sleep while holding a spinlock
(as Rusty says) and again the only solution I see is using mutex.

Looking at my previous patch I found that it should be written like
this:

@@ -199,6 +200,7 @@ asmlinkage long sys_time_pps_setparams(int source,
const struct pps_kparams __user *params) {
int ret;
+ struct pps_kparams temp;

pr_debug(``%s: source %d\n'', __FUNCTION__, source);

@@ -228,13 +230,16 @@ asmlinkage long sys_time_pps_setparams(int source,
}

/* Save the new parameters */
- ret = copy_from_user(&pps_source[source].params, params,
- sizeof(struct pps_kparams));
+ ret = copy_from_user(&temp, params, sizeof(struct pps_kparams));
if (ret) {
ret = -EFAULT;
goto sys_time_pps_setparams_exit;
}

+ spin_lock(&pps_lock);
+
+ pps_source[source].params = temp;
+
/* Restore the read only parameters */
if ((params->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
/* section 3.3 of RFC 2783 interpreted */
@@ -245,6 +250,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);

In this manner I can going to sleep in copy_from_user() whitout
holding any locks.

On Mon, Jul 30, 2007 at 02:50:56PM +0530, Satyam Sharma wrote:

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 ...

Ok, I see, but how you can get your PPS source data struct starting
from a file descriptor? :-o

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.

With your suggestion code should be changed as follow:

fd_gps = open("/dev/ttyS0", ...);

fd_pps = open("/dev/pps0", ...);

pps_time_create(fd_pps, &handler);

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 didn't find a good solution for it.

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? 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?

I studied the problem trying to find a good solution for both NTPD
code (tring to change it as less as possible) and PPS sources
connected with CPU's GPIOs, but currently I find nothing better that
this.

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 :-)

Ok, if you don't think so try looking at NTPD code (written by RFC's
gurus) and try to resolve the problem where both GPS antenna and PPS
source are connected with a serial port, and the problem where only
the GPS antenna is connected with the serial port but the PPS source
is connected with a CPU's GPIO.

If you solve both them all PPS users will thank you a lot forever! :)

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!

Ok, in your sceraio no problem, but continue to read RFC:

All of the other functions in the PPS API operate on PPS handles
(type: pps_handle_t). The time_pps_create() is used to convert an
already-open UNIX file descriptor, for an appropriate special file,
into a PPS handle.

The definition of what special files are appropriate for use with the
PPS API is outside the scope of this specification, and may vary
based on both operating system implementation, and local system
configuration. One typical case is a serial line, whose DCD pin is
connected to a source of PPS events.

This shows that RFC creators though only at PPS sources connected with
an _already_ opened device (as serial ports or parallel one), if not
why don't define the function time_pps_create() as:

int time_pps_create(char *ppsdev, pps_handle_t *handle);

???

No, they require an _already_ opened file descriptor because they
supposed you can use serial line file descriptor! So, if this is not
your case (because your PPS source is connected with a CPU's GPIO) you
_must_ use a second open() (not considere at all into NTPD code) or,
as I did, use a special function like pps_findsource/path().

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 ...

I read it. Ok, according your solution and not considering the two
open()s to do we should do:

fd_pps = open("/dev/pps0", ...);

time_pps_create(fd_pps, &handle);

ok? Two qeustions:

1) If the RFC wan't broken why it doesn't simply say that you can use
fd_pps and pps handler?

2) How can you get kernel PPS source data just getting as input
fd_pps value?

Again, RFC was sane if it said that you can get a PPS hadler by simply
doing:

time_pps_create("/dev/pps0", &handle);

this could be easily implemented with just an open()...

They didn't like this because they __never__ considered the case where
the PPS source is not connected at all with any serial/parallel
decices.

On Mon, Jul 30, 2007 at 03:01:28PM +0530, Satyam Sharma wrote:

What does the section on locking between hard irq contexts (or between
user process context and hard irq context) say?

That I should use spin_lock_irqsave() but the document also:

1) says that I cannot hold spinlocks while sleeping and

2) doesn't say that using spin_lock/unlock() in user context and
spin_trylock/unlock() in interrupt context is wrong.

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 :-)

Great! Thanks a lot, so we can discuss on them and maybe we can prive
Linux of this PPS support! :)

Thanks a lot for your time, this discussion was most important for me
in better understanding locks problems (and also to improve my poor
english, eheheheh :).

Ciao,

Rodolfo

--

GNU/Linux Solutions e-mail: giometti@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxxx
Embedded Systems giometti@xxxxxxxx
UNIX programming phone: +39 349 2432127
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: LinuxPPS & spinlocks
    ... but the goal is to avoid races. ... I meant that if you have to lock between user context and interrupt ... antenna and PPS source share the same serial port? ...
    (Linux-Kernel)
  • Re: GRU driver feedback
    ... "atomic pte lookup" which looks up the pte atomically using similar ... Existence of the mm is guaranteed thru an indirect path. ... struct cannot go away until the GRU context that caused the interrupt ... When the GRU hardware sends an interrupt, ...
    (Linux-Kernel)
  • Re: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier interface
    ... interrupt from eth to host wakes the cpu out of idle, ... softirq-net-rx in interrupt context to NAPI-poll the ethernet ... The interface is just ugly IMO. ... that must support being invoked from atomic-context. ...
    (Linux-Kernel)
  • Re: LinuxPPS & spinlocks
    ... does nothing (if we are already in an interrupt handler). ... context, but doing like this I will disable interrupts ... protect syscalls from each others and from pps_register/unregister, ... Note that *data* is protected by a lock, ...
    (Linux-Kernel)
  • Re: GRU driver feedback
    ... Existence of the mm is guaranteed thru an indirect path. ... struct cannot go away until the GRU context that caused the interrupt ... When the GRU hardware sends an interrupt, ...
    (Linux-Kernel)