From: "Pucha, HimasekharX Reddy" <himasekharx.reddy.pucha@intel.com>
To: mschmidt <mschmidt@redhat.com>,
"intel-wired-lan@lists.osuosl.org"
<intel-wired-lan@lists.osuosl.org>
Cc: Jiri Pirko <jiri@resnulli.us>,
"Temerkhanov, Sergey" <sergey.temerkhanov@intel.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Richard Cochran <richardcochran@gmail.com>,
"Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>,
"Kolacinski, Karol" <karol.kolacinski@intel.com>,
Marcin Szycik <marcin.szycik@linux.intel.com>,
"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
"Kitszel, Przemyslaw" <przemyslaw.kitszel@intel.com>,
"Keller, Jacob E" <jacob.e.keller@intel.com>
Subject: RE: [Intel-wired-lan] [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path
Date: Fri, 29 Mar 2024 05:01:16 +0000 [thread overview]
Message-ID: <CYYPR11MB84291C3013A4C2E0D30BF84DBD3A2@CYYPR11MB8429.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20240325232039.76836-3-mschmidt@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Tuesday, March 26, 2024 4:51 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Jiri Pirko <jiri@resnulli.us>; Temerkhanov, Sergey <sergey.temerkhanov@intel.com>; netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Marcin Szycik <marcin.szycik@linux.intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path
>
> The PTP hardware semaphore (PFTSYN_SEM) is used to synchronize operations that program the PTP timers. The operations involve issuing commands to the sideband queue. The E810 does not have a hardware sideband queue, so the admin queue is used. The admin queue is slow.
> I have observed delays in hundreds of milliseconds waiting for ice_sq_done.
>
> When phc2sys reads the time from the ice PTP clock and PFTSYN_SEM is held by a task performing one of the slow operations, ice_ptp_lock can easily time out. phc2sys gets -EBUSY and the kernel prints:
> ice 0000:XX:YY.0: PTP failed to get time These messages appear once every few seconds, causing log spam.
>
> The E810 datasheet recommends an algorithm for reading the upper 64 bits of the GLTSYN_TIME register. It matches what's implemented in ice_ptp_read_src_clk_reg. It is robust against wrap-around, but not necessarily against the concurrent setting of the register (with GLTSYN_CMD_{INIT,ADJ}_TIME commands). Perhaps that's why
> ice_ptp_gettimex64 also takes PFTSYN_SEM.
>
> The race with time setters can be prevented without relying on the PTP hardware semaphore. Using the "ice_adapter" from the previous patch, we can have a common spinlock for the PFs that share the clock hardware.
> It will protect the reading and writing to the GLTSYN_TIME register.
> The writing is performed indirectly, by the hardware, as a result of the driver writing GLTSYN_CMD_SYNC in ice_ptp_exec_tmr_cmd. I wasn't sure if the ice_flush there is enough to make sure GLTSYN_TIME has been updated, but it works well in my testing.
>
> My test code can be seen here:
> https://gitlab.com/mschmidt2/linux/-/commits/ice-ptp-host-side-lock-10
> It consists of:
> - kernel threads reading the time in a busy loop and looking at the
> deltas between consecutive values, reporting new maxima.
> - a shell script that sets the time repeatedly;
> - a bpftrace probe to produce a histogram of the measured deltas.
> Without the spinlock ptp_gltsyn_time_lock, it is easy to see tearing.
> Deltas in the [2G, 4G) range appear in the histograms.
> With the spinlock added, there is no tearing and the biggest delta I saw was in the range [1M, 2M), that is under 2 ms.
>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_adapter.c | 2 ++ drivers/net/ethernet/intel/ice/ice_adapter.h | 6 ++++++
> drivers/net/ethernet/intel/ice/ice_ptp.c | 8 +-------
> drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 3 +++
> 4 files changed, 12 insertions(+), 7 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
WARNING: multiple messages have this Message-ID (diff)
From: "Pucha, HimasekharX Reddy" <himasekharx.reddy.pucha@intel.com>
To: mschmidt <mschmidt@redhat.com>,
"intel-wired-lan@lists.osuosl.org"
<intel-wired-lan@lists.osuosl.org>
Cc: Jiri Pirko <jiri@resnulli.us>,
"Temerkhanov, Sergey" <sergey.temerkhanov@intel.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Richard Cochran <richardcochran@gmail.com>,
"Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>,
"Kolacinski, Karol" <karol.kolacinski@intel.com>,
Marcin Szycik <marcin.szycik@linux.intel.com>,
"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
"Kitszel, Przemyslaw" <przemyslaw.kitszel@intel.com>,
"Keller, Jacob E" <jacob.e.keller@intel.com>
Subject: Re: [Intel-wired-lan] [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path
Date: Fri, 29 Mar 2024 05:01:16 +0000 [thread overview]
Message-ID: <CYYPR11MB84291C3013A4C2E0D30BF84DBD3A2@CYYPR11MB8429.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20240325232039.76836-3-mschmidt@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Tuesday, March 26, 2024 4:51 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Jiri Pirko <jiri@resnulli.us>; Temerkhanov, Sergey <sergey.temerkhanov@intel.com>; netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Marcin Szycik <marcin.szycik@linux.intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path
>
> The PTP hardware semaphore (PFTSYN_SEM) is used to synchronize operations that program the PTP timers. The operations involve issuing commands to the sideband queue. The E810 does not have a hardware sideband queue, so the admin queue is used. The admin queue is slow.
> I have observed delays in hundreds of milliseconds waiting for ice_sq_done.
>
> When phc2sys reads the time from the ice PTP clock and PFTSYN_SEM is held by a task performing one of the slow operations, ice_ptp_lock can easily time out. phc2sys gets -EBUSY and the kernel prints:
> ice 0000:XX:YY.0: PTP failed to get time These messages appear once every few seconds, causing log spam.
>
> The E810 datasheet recommends an algorithm for reading the upper 64 bits of the GLTSYN_TIME register. It matches what's implemented in ice_ptp_read_src_clk_reg. It is robust against wrap-around, but not necessarily against the concurrent setting of the register (with GLTSYN_CMD_{INIT,ADJ}_TIME commands). Perhaps that's why
> ice_ptp_gettimex64 also takes PFTSYN_SEM.
>
> The race with time setters can be prevented without relying on the PTP hardware semaphore. Using the "ice_adapter" from the previous patch, we can have a common spinlock for the PFs that share the clock hardware.
> It will protect the reading and writing to the GLTSYN_TIME register.
> The writing is performed indirectly, by the hardware, as a result of the driver writing GLTSYN_CMD_SYNC in ice_ptp_exec_tmr_cmd. I wasn't sure if the ice_flush there is enough to make sure GLTSYN_TIME has been updated, but it works well in my testing.
>
> My test code can be seen here:
> https://gitlab.com/mschmidt2/linux/-/commits/ice-ptp-host-side-lock-10
> It consists of:
> - kernel threads reading the time in a busy loop and looking at the
> deltas between consecutive values, reporting new maxima.
> - a shell script that sets the time repeatedly;
> - a bpftrace probe to produce a histogram of the measured deltas.
> Without the spinlock ptp_gltsyn_time_lock, it is easy to see tearing.
> Deltas in the [2G, 4G) range appear in the histograms.
> With the spinlock added, there is no tearing and the biggest delta I saw was in the range [1M, 2M), that is under 2 ms.
>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_adapter.c | 2 ++ drivers/net/ethernet/intel/ice/ice_adapter.h | 6 ++++++
> drivers/net/ethernet/intel/ice/ice_ptp.c | 8 +-------
> drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 3 +++
> 4 files changed, 12 insertions(+), 7 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
next prev parent reply other threads:[~2024-03-29 5:01 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-25 23:20 [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Michal Schmidt
2024-03-25 23:20 ` [Intel-wired-lan] " Michal Schmidt
2024-03-25 23:20 ` [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC Michal Schmidt
2024-03-25 23:20 ` [Intel-wired-lan] " Michal Schmidt
2024-03-29 4:57 ` Pucha, HimasekharX Reddy
2024-03-29 4:57 ` Pucha, HimasekharX Reddy
2024-03-25 23:20 ` [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path Michal Schmidt
2024-03-25 23:20 ` [Intel-wired-lan] " Michal Schmidt
2024-03-29 5:01 ` Pucha, HimasekharX Reddy [this message]
2024-03-29 5:01 ` Pucha, HimasekharX Reddy
2024-03-25 23:20 ` [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64 Michal Schmidt
2024-03-25 23:20 ` [Intel-wired-lan] " Michal Schmidt
2024-03-26 14:46 ` Sai Krishna Gajula
2024-03-26 14:46 ` [Intel-wired-lan] " Sai Krishna Gajula
2024-03-29 5:03 ` Pucha, HimasekharX Reddy
2024-03-29 5:03 ` Pucha, HimasekharX Reddy
2024-03-29 8:35 ` Kalesh Anakkur Purayil
2024-03-29 8:35 ` [Intel-wired-lan] " Kalesh Anakkur Purayil
2024-03-26 9:51 ` [Intel-wired-lan] [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Ivan Vecera
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CYYPR11MB84291C3013A4C2E0D30BF84DBD3A2@CYYPR11MB8429.namprd11.prod.outlook.com \
--to=himasekharx.reddy.pucha@intel.com \
--cc=anthony.l.nguyen@intel.com \
--cc=arkadiusz.kubalewski@intel.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jacob.e.keller@intel.com \
--cc=jiri@resnulli.us \
--cc=karol.kolacinski@intel.com \
--cc=marcin.szycik@linux.intel.com \
--cc=mschmidt@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=przemyslaw.kitszel@intel.com \
--cc=richardcochran@gmail.com \
--cc=sergey.temerkhanov@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.