| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <daf48f7c-e3e4-a23a-0422-a98c409ff0eb@intel.com>
Date: Tue, 26 Sep 2023 16:48:16 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vishvambar Panth S <vishvambarpanth.s@...rochip.com>,
<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>
CC: <bryan.whitehead@...rochip.com>, <unglinuxdriver@...rochip.com>,
<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <richardcochran@...il.com>
Subject: Re: [PATCH net-next] net: microchip: lan743x: improve throughput with
rx timestamp config
On 9/26/2023 8:56 AM, Vishvambar Panth S wrote:
> Currently all RX frames are timestamped which results in a performance
> penalty when timestamping is not needed. The default is now being
> changed to not timestamp any Rx frames (HWTSTAMP_FILTER_NONE), but
> support has been added to allow changing the desired RX timestamping
> mode (HWTSTAMP_FILTER_ALL - which was the previous setting,
> HWTSTAMP_FILTER_PTP_V2_EVENT, HWTSTAMP_FILTER_PTP_V2_SYNC and
> HWTSTAMP_FILTER_PTP_V2_DELAY_REQ are the supported options) using
> SIOCSHWTSTAMP. All settings were tested using the hwstamp_ctl application.
> It is also noted that ptp4l, when started, preconfigures the device to
> timestamp using HWTSTAMP_FILTER_PTP_V2_EVENT, so this driver continues
> to work properly "out of the box".
>
> Test setup: x64 PC with LAN7430 ---> x64 PC as partner
>
I don't think I would bother to support HWTSTAMP_FILTER_PTP_V2_DELAY_REQ
or HWTSTAMP_FILTER_PTP_V2_SYNC as these are pretty historic and only
useful for hardware which can't do HWTSTAMP_FILTER_PTP_V2_EVENT.
> iperf3 with - Timestamp all incoming packets:
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval Transfer Bitrate Retr
> [ 5] 0.00-5.05 sec 517 MBytes 859 Mbits/sec 0 sender
> [ 5] 0.00-5.00 sec 515 MBytes 864 Mbits/sec receiver
>
> iperf Done.
>
> iperf3 with - Timestamp only PTP packets:
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval Transfer Bitrate Retr
> [ 5] 0.00-5.04 sec 563 MBytes 937 Mbits/sec 0 sender
> [ 5] 0.00-5.00 sec 561 MBytes 941 Mbits/sec receiver
>
>
Pretty significant cost here for the timestamping all frames. Makes
sense to leave the default to NONE unless requested.
Please find the earlier conversation at the link below
> Link: https://lore.kernel.org/all/20230731125418.75140-1-vishvambarpanth.s@microchip.com/
>
> Signed-off-by: Vishvambar Panth S <vishvambarpanth.s@...rochip.com>
> ---
> .../net/ethernet/microchip/lan743x_ethtool.c | 5 +-
> drivers/net/ethernet/microchip/lan743x_main.c | 58 ++++++++++++++++++-
> drivers/net/ethernet/microchip/lan743x_main.h | 8 +++
> drivers/net/ethernet/microchip/lan743x_ptp.c | 9 +++
> 4 files changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> index 2db5949b4c7e..855844df5ea1 100644
> --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -1047,7 +1047,10 @@ static int lan743x_ethtool_get_ts_info(struct net_device *netdev,
> BIT(HWTSTAMP_TX_ON) |
> BIT(HWTSTAMP_TX_ONESTEP_SYNC);
> ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> - BIT(HWTSTAMP_FILTER_ALL);
> + BIT(HWTSTAMP_FILTER_ALL) |
> + BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |
> + BIT(HWTSTAMP_FILTER_PTP_V2_SYNC) |
> + BIT(HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
> return 0;
> }
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index f940895b14e8..0389bc7cf603 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -1870,6 +1870,63 @@ static int lan743x_tx_get_avail_desc(struct lan743x_tx *tx)
> return last_head - last_tail - 1;
> }
>
> +int lan743x_rx_set_tstamp_mode(struct lan743x_adapter *adapter,
> + int rx_filter)
> +{
> + int channel_number;
> + int index;
> + u32 data;
> +
> + switch (rx_filter) {
> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> + data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
> + data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
> + data |= PTP_RX_TS_CFG_SYNC_MSG_;
> + lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
> + break;
> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> + data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
> + data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
> + data |= PTP_RX_TS_CFG_DELAY_REQ_MSG_;
> + lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
> + break;
> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> + data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
> + data |= PTP_RX_TS_CFG_EVENT_MSGS_;
> + lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
> + break;
> + case HWTSTAMP_FILTER_NONE:
> + case HWTSTAMP_FILTER_ALL:
> + break;
At first this break was a bit confusing to me, since nothing is set here.
> + default:
> + netif_warn(adapter, drv, adapter->netdev,
> + "rx timestamp = %d is not supported\n",
> + rx_filter);
> + return -EINVAL;
> + }
> +
> + for (index = 0; index < LAN743X_USED_RX_CHANNELS; index++) {
> + channel_number = adapter->rx[index].channel_number;
> + data = lan743x_csr_read(adapter, RX_CFG_B(channel_number));
> + if (rx_filter == HWTSTAMP_FILTER_NONE) {
> + data &= ~(RX_CFG_B_TS_ALL_RX_ |
> + RX_CFG_B_TS_DESCR_EN_);
> + } else if (rx_filter == HWTSTAMP_FILTER_ALL) {
> + data |= RX_CFG_B_TS_ALL_RX_;
> + } else {
> + /* enable storing timestamping in extension descriptor
> + * instead of timestamping all the packets
> + */
> + data &= ~RX_CFG_B_TS_ALL_RX_;
> + data |= RX_CFG_B_TS_DESCR_EN_;
> + }
I might have made the decision of what to program in the switch case
above and then done the write here with "data &= ~MASK; data |= setting"
rather than having two separate decision points.
> + lan743x_csr_write(adapter, RX_CFG_B(channel_number),
> + data);
> + }
> +
> + return 0;
> +}
> +
Rest of the code seems fine, and the implementation looks ok.
I'd suggest dropping the not so useful timestamp filters for only sync
or only delay request, keeping only V2_EVENT, but either way:
Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
Powered by blists - more mailing lists