Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Kolacinski, Karol]

>On 12/7/2022 4:27 PM, Richard Cochran wrote:
>> On Wed, Dec 07, 2022 at 01:10:37PM -0800, Tony Nguyen wrote:
>>> From: Anatolii Gerasymenko anatolii.gerasymenko@intel.com
>>>
>>> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>>> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>>> sends messages to AQ and waits for responses. This causes
>>> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>>> causes problems with the reading of the incoming signal timestamps,
>>> which disrupts a 100 Hz signal.
>>>
>>> Create an additional kthread_worker pf.ptp.kworker_extts to service only
>>> ice_ptp_extts_work() as soon as possible.
>> 
>> Looks like this driver isn't using the do_aux_work callback.  That
>> would provide a kthread worker for free.  Why not use that?

According to do_aux_work description, it is used to do 'auxiliary (periodic)
operations'.
ice_ptp_extts_work is not exactly periodic as the work is queued only when the
interrupt comes.

Thanks,
Karol

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Richard Cochran]

On Mon, Dec 12, 2022 at 11:24:22PM +0000, Kolacinski, Karol wrote:

> According to do_aux_work description, it is used to do 'auxiliary (periodic)
> operations'.
> ice_ptp_extts_work is not exactly periodic as the work is queued only when the
> interrupt comes.

You can use it in any way that you like, periodic or not.  The return
value from the driver callback determines if and when the next
callback occurs.

Thanks,
Richard

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Tony Nguyen]

On 12/7/2022 4:27 PM, Richard Cochran wrote:
> On Wed, Dec 07, 2022 at 01:10:37PM -0800, Tony Nguyen wrote:
>> From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>>
>> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>> sends messages to AQ and waits for responses. This causes
>> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>> causes problems with the reading of the incoming signal timestamps,
>> which disrupts a 100 Hz signal.
>>
>> Create an additional kthread_worker pf.ptp.kworker_extts to service only
>> ice_ptp_extts_work() as soon as possible.
> 
> Looks like this driver isn't using the do_aux_work callback.  That
> would provide a kthread worker for free.  Why not use that?

The author is no longer with Intel anymore; we have another developer 
who is looking into this. Will make the change or get back to you.

Thanks,
Tony

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Richard Cochran]

On Wed, Dec 07, 2022 at 01:10:37PM -0800, Tony Nguyen wrote:
> From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
> 
> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
> sends messages to AQ and waits for responses. This causes
> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
> causes problems with the reading of the incoming signal timestamps,
> which disrupts a 100 Hz signal.
> 
> Create an additional kthread_worker pf.ptp.kworker_extts to service only
> ice_ptp_extts_work() as soon as possible.

Looks like this driver isn't using the do_aux_work callback.  That
would provide a kthread worker for free.  Why not use that?

Thanks,
Richard

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Jacob Keller]

On 12/7/2022 2:19 PM, Saeed Mahameed wrote:
> On 07 Dec 13:10, Tony Nguyen wrote:
>> From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>>
>> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>> sends messages to AQ and waits for responses. This causes
>> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>> causes problems with the reading of the incoming signal timestamps,
>> which disrupts a 100 Hz signal.
>>
> 
> Sounds like an optimization rather than a bug fix, unless you explain what
> the symptoms are and how critical this patch is.

I'm not the original author, but I think Anatolii is out right now. I'll 
try to explain for him.

The problem is that extts must execute to read the timestamp value from 
the incoming signal. It has to complete before the next level change. 
Otherwise I believe if we don't read it in time the next level change 
will be ignored.

For a 100Hz signal, this means it has to process within 10 milliseconds. 
Kworkers can only execute one task at a time. If the periodic work is 
already blocking and currently processing an AdminQ message it might go 
to sleep for a while until it can process the message. Even if the task 
goes to sleep, the other kthread item cannot execute on the same 
kworker. This can potentially result in a delay long enough to prevent 
the external timestamp work function from reading the external timestamp 
within the time limit. I believe this results in missed external 
timestamp events. I am not certain if this loss is permanent or only 
transient.

I would consider that a bug, because it results in loss of 
functionality, not just something like lower CPU usage.

> 
> code LGTM, although i find it wasteful to create a kthread per device event
> type, but i can't think of a better way.
> 

Right. I can't think of another good way either. I think we can't 
process this directly in the interrupt which is why we had processed it 
in a kthread item to begin with.

Thanks,
Jake

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Saeed Mahameed]

On 07 Dec 13:10, Tony Nguyen wrote:
>From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>
>ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>sends messages to AQ and waits for responses. This causes
>ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>causes problems with the reading of the incoming signal timestamps,
>which disrupts a 100 Hz signal.
>

Sounds like an optimization rather than a bug fix, unless you explain what
the symptoms are and how critical this patch is.

code LGTM, although i find it wasteful to create a kthread per device event
type, but i can't think of a better way.

>Create an additional kthread_worker pf.ptp.kworker_extts to service only
>ice_ptp_extts_work() as soon as possible.
>
>Fixes: 77a781155a65 ("ice: enable receive hardware timestamping")
>Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
>Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>---
> drivers/net/ethernet/intel/ice/ice_main.c |  5 ++++-
> drivers/net/ethernet/intel/ice/ice_ptp.c  | 15 ++++++++++++++-
> drivers/net/ethernet/intel/ice/ice_ptp.h  |  2 ++
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>index ca2898467dcb..d0f14e73e8da 100644
>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>@@ -3106,7 +3106,10 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data)
> 						     GLTSYN_STAT_EVENT1_M |
> 						     GLTSYN_STAT_EVENT2_M);
> 		ena_mask &= ~PFINT_OICR_TSYN_EVNT_M;
>-		kthread_queue_work(pf->ptp.kworker, &pf->ptp.extts_work);
>+
>+		if (pf->ptp.kworker_extts)
>+			kthread_queue_work(pf->ptp.kworker_extts,
>+					   &pf->ptp.extts_work);
> 	}
>
> #define ICE_AUX_CRIT_ERR (PFINT_OICR_PE_CRITERR_M | PFINT_OICR_HMC_ERR_M | PFINT_OICR_PE_PUSH_M)
>diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>index 0f668468d141..f9e20622ad9f 100644
>--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>@@ -2604,7 +2604,7 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
>  */
> static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
> {
>-	struct kthread_worker *kworker;
>+	struct kthread_worker *kworker, *kworker_extts;
>
> 	/* Initialize work functions */
> 	kthread_init_delayed_work(&ptp->work, ice_ptp_periodic_work);
>@@ -2620,6 +2620,13 @@ static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
>
> 	ptp->kworker = kworker;
>
>+	kworker_extts = kthread_create_worker(0, "ice-ptp-extts-%s",
>+					      dev_name(ice_pf_to_dev(pf)));
>+	if (IS_ERR(kworker_extts))
>+		return PTR_ERR(kworker_extts);
>+
>+	ptp->kworker_extts = kworker_extts;
>+
> 	/* Start periodic work going */
> 	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
>
>@@ -2719,11 +2726,17 @@ void ice_ptp_release(struct ice_pf *pf)
>
> 	ice_ptp_port_phy_stop(&pf->ptp.port);
> 	mutex_destroy(&pf->ptp.port.ps_lock);
>+
> 	if (pf->ptp.kworker) {
> 		kthread_destroy_worker(pf->ptp.kworker);
> 		pf->ptp.kworker = NULL;
> 	}
>
>+	if (pf->ptp.kworker_extts) {
>+		kthread_destroy_worker(pf->ptp.kworker_extts);
>+		pf->ptp.kworker_extts = NULL;
>+	}
>+
> 	if (!pf->ptp.clock)
> 		return;
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
>index 028349295b71..c63ad2c9af4c 100644
>--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
>+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
>@@ -165,6 +165,7 @@ struct ice_ptp_port {
>  * @ext_ts_chan: the external timestamp channel in use
>  * @ext_ts_irq: the external timestamp IRQ in use
>  * @kworker: kwork thread for handling periodic work
>+ * @kworker_extts: kworker thread for handling extts work
>  * @perout_channels: periodic output data
>  * @info: structure defining PTP hardware capabilities
>  * @clock: pointer to registered PTP clock device
>@@ -186,6 +187,7 @@ struct ice_ptp {
> 	u8 ext_ts_chan;
> 	u8 ext_ts_irq;
> 	struct kthread_worker *kworker;
>+	struct kthread_worker *kworker_extts;
> 	struct ice_perout_channel perout_channels[GLTSYN_TGT_H_IDX_MAX];
> 	struct ptp_clock_info info;
> 	struct ptp_clock *clock;
>-- 
>2.35.1
>

[PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Tony Nguyen]

From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>

ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
sends messages to AQ and waits for responses. This causes
ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
causes problems with the reading of the incoming signal timestamps,
which disrupts a 100 Hz signal.

Create an additional kthread_worker pf.ptp.kworker_extts to service only
ice_ptp_extts_work() as soon as possible.

Fixes: 77a781155a65 ("ice: enable receive hardware timestamping")
Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c |  5 ++++-
 drivers/net/ethernet/intel/ice/ice_ptp.c  | 15 ++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_ptp.h  |  2 ++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index ca2898467dcb..d0f14e73e8da 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3106,7 +3106,10 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data)
 						     GLTSYN_STAT_EVENT1_M |
 						     GLTSYN_STAT_EVENT2_M);
 		ena_mask &= ~PFINT_OICR_TSYN_EVNT_M;
-		kthread_queue_work(pf->ptp.kworker, &pf->ptp.extts_work);
+
+		if (pf->ptp.kworker_extts)
+			kthread_queue_work(pf->ptp.kworker_extts,
+					   &pf->ptp.extts_work);
 	}
 
 #define ICE_AUX_CRIT_ERR (PFINT_OICR_PE_CRITERR_M | PFINT_OICR_HMC_ERR_M | PFINT_OICR_PE_PUSH_M)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 0f668468d141..f9e20622ad9f 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2604,7 +2604,7 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
  */
 static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
 {
-	struct kthread_worker *kworker;
+	struct kthread_worker *kworker, *kworker_extts;
 
 	/* Initialize work functions */
 	kthread_init_delayed_work(&ptp->work, ice_ptp_periodic_work);
@@ -2620,6 +2620,13 @@ static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
 
 	ptp->kworker = kworker;
 
+	kworker_extts = kthread_create_worker(0, "ice-ptp-extts-%s",
+					      dev_name(ice_pf_to_dev(pf)));
+	if (IS_ERR(kworker_extts))
+		return PTR_ERR(kworker_extts);
+
+	ptp->kworker_extts = kworker_extts;
+
 	/* Start periodic work going */
 	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
 
@@ -2719,11 +2726,17 @@ void ice_ptp_release(struct ice_pf *pf)
 
 	ice_ptp_port_phy_stop(&pf->ptp.port);
 	mutex_destroy(&pf->ptp.port.ps_lock);
+
 	if (pf->ptp.kworker) {
 		kthread_destroy_worker(pf->ptp.kworker);
 		pf->ptp.kworker = NULL;
 	}
 
+	if (pf->ptp.kworker_extts) {
+		kthread_destroy_worker(pf->ptp.kworker_extts);
+		pf->ptp.kworker_extts = NULL;
+	}
+
 	if (!pf->ptp.clock)
 		return;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 028349295b71..c63ad2c9af4c 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -165,6 +165,7 @@ struct ice_ptp_port {
  * @ext_ts_chan: the external timestamp channel in use
  * @ext_ts_irq: the external timestamp IRQ in use
  * @kworker: kwork thread for handling periodic work
+ * @kworker_extts: kworker thread for handling extts work
  * @perout_channels: periodic output data
  * @info: structure defining PTP hardware capabilities
  * @clock: pointer to registered PTP clock device
@@ -186,6 +187,7 @@ struct ice_ptp {
 	u8 ext_ts_chan;
 	u8 ext_ts_irq;
 	struct kthread_worker *kworker;
+	struct kthread_worker *kworker_extts;
 	struct ice_perout_channel perout_channels[GLTSYN_TGT_H_IDX_MAX];
 	struct ptp_clock_info info;
 	struct ptp_clock *clock;
-- 
2.35.1