Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
> On 13 Sep 2016, at 20:00, Richard Cochran wrote: > > On Tue, Sep 13, 2016 at 09:32:39AM +0100, Kieran Tyrrell wrote: >> Should the ptp capabilities (in the igb case) state the number of >> alarms as 2? (even though only one can be enabled at a time, and in >> fact you can create hundreds of posix timers off a single hardware >> alarm? > > Looking back, it is unfortunate that the ptp_clock_caps.n_alarm field > is named like that. We only need one bit to tell the user that a > driver supports timers. > > In addition to that, there should some indication of the number of > interrupt sources (i210 has 2) and their connection to the timer, > periodic output, and pps features, if any. Something like > PTP_PIN_GETFUNC/_SETFUNC. > >> Just want to get it straight with you before I start coding, thanks! > > I haven't given it much thought, and I am open to creative > suggestions. My only requirement is that the user will be able to > query the capabilities and dial what he wants. For the other features > we advertise the number of feature channels and the number of pins. > The user decides which channel to use on each pin. If there are fewer > pins than channels, then it is clear that he can't have everything at > once. > > For now, if you would rather concentrate on the actual timer code, > just set i210's caps to n_alarm=1 and n_per_out=1 and leave the rest > as a todo. I wouldn’t be comfortable removing one of the periodic outputs from igb in case anyone is using both. I’ve added an ioctl to enable/disable the timer functionality on either of the two channels (TT0/TT1). Of course you can only set it on one at a time. Personally I preferred the approach of posix timer_create() allocating the hardware required when the first timer was created, but the ioctl fulfils the requirement of putting the user in control. We now have the situation though that timer_create() will succeed, but timer_settime() can fail, as the PTP subsystem isn’t allocating the hardware timer any more, the user is. Also the user could now disable the hardware timer while posix timers are created and active, leaving them hanging. I’ll post a patch containing my latest code (sorry ptp and igb will be mixed together in this patch) and if it looks ok I’ll separate out the ptp and igb parts for a proper patch. Thanks, Kieran. -- ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
On Thu, Sep 15, 2016 at 12:46:14PM +0100, Kieran Tyrrell wrote: > I wouldn’t be comfortable removing one of the periodic outputs from > igb in case anyone is using both. I only meant doing this for testing purposes. > I’ve added an ioctl to enable/disable the timer functionality on > either of the two channels (TT0/TT1). Of course you can only set it > on one at a time. For testing this is ok, but in the end we will need a proper interface, like the PTP_PIN_SETFUNC ioctl. > We now have the situation though that timer_create() will > succeed, but timer_settime() can fail, as the PTP subsystem isn’t > allocating the hardware timer any more, the user is. I don't see why timer_create() cannot simply check whether an interrupt channel is available, and if not, return an error. > Also the user could now disable the hardware timer while posix > timers are created and active, leaving them hanging. Let the timer_create() call lock the interrupt channel, preventing re-assignment until all timers have gone away. Thanks, Richard -- ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
> On 15 Sep 2016, at 12:33, Richard Cochran wrote: > > On Thu, Sep 15, 2016 at 09:56:24AM +0100, Kieran Tyrrell wrote: > >> My signal delivery problem was a bug in the igb interrupt handler, >> where I was assuming I could set a new trigger time for TT0 before >> acknowledging the interrupt. Turns out I have to disable TT0, >> acknowledge the interrupt, program the new trigger time, and then >> re-enable TT0 for it to work. All timer signals are now delivered >> synchronously and reliably. > > So does this bug affect signal generation using TSINTR_TT0/1? > > If so, that should be patched right away, before adding any new > features. I’m not sure, in the timer case it may only have been a problem when setting a new timer trigger time which has (by the time the registers were written and the interrupt acknowledged) already elapsed. I don’t have physical pins on my i210 card to attach a probe to, but I suppose this could be verified by adding debug to the interrupt handler. I’ll see if I can test this... -- ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
On Thu, Sep 15, 2016 at 09:56:24AM +0100, Kieran Tyrrell wrote: > My signal delivery problem was a bug in the igb interrupt handler, > where I was assuming I could set a new trigger time for TT0 before > acknowledging the interrupt. Turns out I have to disable TT0, > acknowledge the interrupt, program the new trigger time, and then > re-enable TT0 for it to work. All timer signals are now delivered > synchronously and reliably. So does this bug affect signal generation using TSINTR_TT0/1? If so, that should be patched right away, before adding any new features. Thanks, Richard -- ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
> On 12 Sep 2016, at 18:14, Richard Cochran wrote: > > On Mon, Sep 12, 2016 at 02:52:03PM +0100, Kieran Tyrrell wrote: > >> I agree. Previously I was finding that sending the signals directly >> from ptp_clock_event unreliable (around 1 in 1,000,000 signals >> seemed to be getting lost) while from a work queue they all arrived >> safely. After further debugging I’m not sure if the problem was with >> the PTP module, the igb module, my test code, or even my >> PC. (running the same posix timer tests on the MONOTONIC clock >> occasionally produces kernel log messages hrtimer:interrupt took >> 3193ns). > > Something is fishy, yes, but we should get to the bottom of it. I can > help test this once you drop the work queue. > > I have seen the interrupts on the i210 get stuck when stressing the > input time stamp function with a 1 kHz input. The interrupts simply > stop triggering. But your issue sounds different. My signal delivery problem was a bug in the igb interrupt handler, where I was assuming I could set a new trigger time for TT0 before acknowledging the interrupt. Turns out I have to disable TT0, acknowledge the interrupt, program the new trigger time, and then re-enable TT0 for it to work. All timer signals are now delivered synchronously and reliably. -- ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
On Tue, Sep 13, 2016 at 09:32:39AM +0100, Kieran Tyrrell wrote: > Should the ptp capabilities (in the igb case) state the number of > alarms as 2? (even though only one can be enabled at a time, and in > fact you can create hundreds of posix timers off a single hardware > alarm? Looking back, it is unfortunate that the ptp_clock_caps.n_alarm field is named like that. We only need one bit to tell the user that a driver supports timers. In addition to that, there should some indication of the number of interrupt sources (i210 has 2) and their connection to the timer, periodic output, and pps features, if any. Something like PTP_PIN_GETFUNC/_SETFUNC. > Just want to get it straight with you before I start coding, thanks! I haven't given it much thought, and I am open to creative suggestions. My only requirement is that the user will be able to query the capabilities and dial what he wants. For the other features we advertise the number of feature channels and the number of pins. The user decides which channel to use on each pin. If there are fewer pins than channels, then it is clear that he can't have everything at once. For now, if you would rather concentrate on the actual timer code, just set i210's caps to n_alarm=1 and n_per_out=1 and leave the rest as a todo. Thanks, Richard -- ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
> On 12 Sep 2016, at 10:09, Richard Cochran wrote: > > On Fri, Sep 09, 2016 at 10:39:09PM +0100, Kieran Tyrrell wrote: >> +int (*timerenable)(struct ptp_clock_info *ptp, bool enable); >> +int (*timersettime)(struct ptp_clock_info *ptp, struct timespec64 *ts); > > These need kerneldoc comments describing the usage above. I find the > name 'timerenable' confusing, because we already have 'enable', and > that method immediately activates the requested feature. The new > timerenable is actually performing a kind of resource reservation, like > the 'verify' method. > > I understood what you were trying to do with 'timerenable', but we > should consider integrating the timers with the other interrupt based > functionality. For the user it makes little sense if the capabilities > advertise timers and periodic signals, for example, only to receive > EBUSY later on at run time. > > In the case of the i210, there is an either/or choice regarding the > TSINTR_TT1/0 interrupts. We need to put the user in charge somehow, > like we do for mapping of auxiliary functions to HW pins. I am about to rework this, to add an ioctl to allow enabling the timer on a specific channel (TT0/TT1 in the case of igb). As it doesn’t make sense to enable both (hardware) timers at the same time (as the posix timers will only use one), should this be a simple: PTP_ENABLE_ALARM with an int parameter to set the channel/alarm number to use? (and -1 to disable)? Should the ptp capabilities (in the igb case) state the number of alarms as 2? (even though only one can be enabled at a time, and in fact you can create hundreds of posix timers off a single hardware alarm? Just want to get it straight with you before I start coding, thanks! Kieran. -- ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
On Mon, Sep 12, 2016 at 02:52:03PM +0100, Kieran Tyrrell wrote: > > The clock_gettime and timer_settime etc prototypes are declared in > struct posix_clock_operations in posix-clock.h, and used by > everything that implements a POSIX clock. The new interfaces that I > have defined between the PTP module and the ethernet driver use > timespec64. Ok. > likewise here, I am reusing the existing clock_gettime function to > get the current time. I assume you are not suggesting I update the > whole POSIX clock implementation to use timespec64? No, not unless you really want to. That effort is headed by Arnd Bergmann, and I thought this part was finished already. > I agree. Previously I was finding that sending the signals directly > from ptp_clock_event unreliable (around 1 in 1,000,000 signals > seemed to be getting lost) while from a work queue they all arrived > safely. After further debugging I’m not sure if the problem was with > the PTP module, the igb module, my test code, or even my > PC. (running the same posix timer tests on the MONOTONIC clock > occasionally produces kernel log messages hrtimer:interrupt took > 3193ns). Something is fishy, yes, but we should get to the bottom of it. I can help test this once you drop the work queue. I have seen the interrupts on the i210 get stuck when stressing the input time stamp function with a 1 kHz input. The interrupts simply stop triggering. But your issue sounds different. Thanks, Richard -- What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohodev2dev ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
> On 12 Sep 2016, at 10:09, Richard Cochran wrote:
>
> On Fri, Sep 09, 2016 at 10:39:09PM +0100, Kieran Tyrrell wrote:
>> a PTP hardware clock supporting timers (alarms) can now be used via the
>> POSIX timer (timer_create, timer_settime etc) interface
>>
>> Signed-off-by: Kieran Tyrrell
>> ---
>> drivers/ptp/ptp_clock.c | 233
>> +++
>> drivers/ptp/ptp_private.h| 4 +
>> include/linux/ptp_clock_kernel.h | 2 +
>> kernel/signal.c | 1 +
>> 4 files changed, 240 insertions(+)
>>
>> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
>> index 2e481b9..067e41c 100644
>> --- a/drivers/ptp/ptp_clock.c
>> +++ b/drivers/ptp/ptp_clock.c
>> @@ -36,6 +36,8 @@
>> #define PTP_PPS_EVENT PPS_CAPTUREASSERT
>> #define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
>>
>> +#define PTP_TIMER_MINIMUM_INTERVAL_NS 10
>> +
>> /* private globals */
>>
>> static dev_t ptp_devt;
>> @@ -43,6 +45,108 @@ static struct class *ptp_class;
>>
>> static DEFINE_IDA(ptp_clocks_map);
>>
>> +static int ptp_clock_gettime(struct posix_clock *pc, struct timespec *tp);
>
> Please don't introduce new code with 'struct timespec'.
> Use timespec64 instead.
The clock_gettime and timer_settime etc prototypes are declared in struct
posix_clock_operations in posix-clock.h, and used by everything that implements
a POSIX clock.
The new interfaces that I have defined between the PTP module and the ethernet
driver use timespec64.
>
>> +
>> +static int set_device_timer_earliest(struct ptp_clock *ptp)
>> +{
>> +struct timerqueue_node *next;
>> +int err;
>> +unsigned long tq_lock_flags;
>> +struct timespec64 ts;
>> +
>> +spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
>> +
>> +next = timerqueue_getnext(&ptp->timerqueue);
>> +
>> +/* Skip over expired or not set timers */
>> +while (next) {
>> +if (next->expires.tv64 != 0)
>> +break;
>> +next = timerqueue_iterate_next(next);
>> +}
>> +
>> +spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
>> +
>> +if (next) {
>> +ts = ktime_to_timespec64(next->expires);
>> +err = ptp->info->timersettime(ptp->info, &ts);
>> +if(err)
>> +return err;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static void ptp_alarm_work(struct work_struct *work)
>> +{
>> +struct ptp_clock *ptp = container_of(work, struct ptp_clock,
>> alarm_work);
>> +struct task_struct *task;
>> +struct siginfo info;
>> +struct timerqueue_node *next;
>> +struct k_itimer *kit;
>> +struct timespec time_now;
>
> timespec64.
likewise here, I am reusing the existing clock_gettime function to get the
current time. I assume you are not suggesting I update the whole POSIX clock
implementation to use timespec64?
>
>> +s64 ns_now;
>> +int shared;
>> +bool signal_failed_to_send;
>> +unsigned long tq_lock_flags;
>> +
>> +if(0 != ptp_clock_gettime(&ptp->clock, &time_now))
>> +return;
>
> The coding style requires a space after the 'if' keyword. Please run
> your patches through scripts/checkpatch.pl to catch this and other
> simple mistakes.
Will do, sorry about that.
>
>> +ns_now = timespec_to_ns(&time_now);
>> +
>> +spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
>> +
>> +next = timerqueue_getnext(&ptp->timerqueue);
>> +
>> +/* Skip over expired or not set timers */
>
> Why keep expired timers in the queue at all?
it was because I used the queue to know how many timers had been created, and
to simplify the code. I’ve reworked it now so that only active timers are in
the queue.
>
>> +while (next) {
>> +if (next->expires.tv64 != 0)
>> +break;
>> +next = timerqueue_iterate_next(next);
>> +}
>> +
>> +while (next) {
>> +if (next->expires.tv64 > ns_now)
>> +break;
>> +
>> +rcu_read_lock();
>> +
>> +kit = container_of(next, struct k_itimer, it.real.timer.node);
>
> This doesn't need to be under the RCU lock.
>
>> +task = pid_task(kit->it_pid, PIDTYPE_PID);
>> +if (task)
>> +{
>> +memset(&info, 0, sizeof(info));
>> +info.si_signo = SIGALRM;
>> +info.si_code = SI_TIMER;
>> +info._sifields._timer._tid = kit->it_id;
>> +kit->sigq->info.si_sys_private = 0;
>> +shared = !(kit->it_sigev_notify & SIGEV_THREAD_ID);
>
> Nor does any of this.
>
> Wait a minute... You have 'info' on the stack, initialize it, and then?
> It isn't used at all.
yes sorry, inactive debugging code…
>
>> +signal_failed_to_send = send_sigqueue(kit->sigq, task,
>> shared) > 0;
>
> Is there a reason not to re-use posix_timer_event() here?
Thanks! I was looking for a w
Re: [Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface
On Fri, Sep 09, 2016 at 10:39:09PM +0100, Kieran Tyrrell wrote:
> a PTP hardware clock supporting timers (alarms) can now be used via the POSIX
> timer (timer_create, timer_settime etc) interface
>
> Signed-off-by: Kieran Tyrrell
> ---
> drivers/ptp/ptp_clock.c | 233
> +++
> drivers/ptp/ptp_private.h| 4 +
> include/linux/ptp_clock_kernel.h | 2 +
> kernel/signal.c | 1 +
> 4 files changed, 240 insertions(+)
>
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 2e481b9..067e41c 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -36,6 +36,8 @@
> #define PTP_PPS_EVENT PPS_CAPTUREASSERT
> #define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
>
> +#define PTP_TIMER_MINIMUM_INTERVAL_NS 10
> +
> /* private globals */
>
> static dev_t ptp_devt;
> @@ -43,6 +45,108 @@ static struct class *ptp_class;
>
> static DEFINE_IDA(ptp_clocks_map);
>
> +static int ptp_clock_gettime(struct posix_clock *pc, struct timespec *tp);
Please don't introduce new code with 'struct timespec'.
Use timespec64 instead.
> +
> +static int set_device_timer_earliest(struct ptp_clock *ptp)
> +{
> + struct timerqueue_node *next;
> + int err;
> + unsigned long tq_lock_flags;
> + struct timespec64 ts;
> +
> + spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
> +
> + next = timerqueue_getnext(&ptp->timerqueue);
> +
> + /* Skip over expired or not set timers */
> + while (next) {
> + if (next->expires.tv64 != 0)
> + break;
> + next = timerqueue_iterate_next(next);
> + }
> +
> + spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
> +
> + if (next) {
> + ts = ktime_to_timespec64(next->expires);
> + err = ptp->info->timersettime(ptp->info, &ts);
> + if(err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static void ptp_alarm_work(struct work_struct *work)
> +{
> + struct ptp_clock *ptp = container_of(work, struct ptp_clock,
> alarm_work);
> + struct task_struct *task;
> + struct siginfo info;
> + struct timerqueue_node *next;
> + struct k_itimer *kit;
> + struct timespec time_now;
timespec64.
> + s64 ns_now;
> + int shared;
> + bool signal_failed_to_send;
> + unsigned long tq_lock_flags;
> +
> + if(0 != ptp_clock_gettime(&ptp->clock, &time_now))
> + return;
The coding style requires a space after the 'if' keyword. Please run
your patches through scripts/checkpatch.pl to catch this and other
simple mistakes.
> + ns_now = timespec_to_ns(&time_now);
> +
> + spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
> +
> + next = timerqueue_getnext(&ptp->timerqueue);
> +
> + /* Skip over expired or not set timers */
Why keep expired timers in the queue at all?
> + while (next) {
> + if (next->expires.tv64 != 0)
> + break;
> + next = timerqueue_iterate_next(next);
> + }
> +
> + while (next) {
> + if (next->expires.tv64 > ns_now)
> + break;
> +
> + rcu_read_lock();
> +
> + kit = container_of(next, struct k_itimer, it.real.timer.node);
This doesn't need to be under the RCU lock.
> + task = pid_task(kit->it_pid, PIDTYPE_PID);
> + if (task)
> + {
> + memset(&info, 0, sizeof(info));
> + info.si_signo = SIGALRM;
> + info.si_code = SI_TIMER;
> + info._sifields._timer._tid = kit->it_id;
> + kit->sigq->info.si_sys_private = 0;
> + shared = !(kit->it_sigev_notify & SIGEV_THREAD_ID);
Nor does any of this.
Wait a minute... You have 'info' on the stack, initialize it, and then?
It isn't used at all.
> + signal_failed_to_send = send_sigqueue(kit->sigq, task,
> shared) > 0;
Is there a reason not to re-use posix_timer_event() here?
> + }
> + rcu_read_unlock();
> +
> + next = timerqueue_iterate_next(next);
> +
> + /* update and reinsert the last one that has fired */
> + timerqueue_del(&ptp->timerqueue, &kit->it.real.timer.node);
> + if ( (0 == ktime_to_ns(kit->it.real.interval)) ||
> signal_failed_to_send) {
> + /* this is not a periodic timer (or the signal failed
> to send), so stop it */
> + kit->it.real.timer.node.expires = ns_to_ktime(0);
> + }
> + else {
> + /* this IS a periodic timer, so set the next fire time
> */
> + kit->it.real.timer.node.expires =
> ktime_add(kit->it.real.timer.node.expires, kit->it.real.interval);
> + }
> + timerqueue_add(&ptp->timerqueue, &kit->it.real.timer.node)
