Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <daf48f7c-e3e4-a23a-0422-a98c409ff0eb@intel.com>
Date:   Tue, 26 Sep 2023 16:48:16 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH net-next] net: microchip: lan743x: improve throughput with
 rx timestamp config
Content-Language: en-US
To:     Vishvambar Panth S <vishvambarpanth.s@microchip.com>,
        <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>
CC:     <bryan.whitehead@microchip.com>, <unglinuxdriver@microchip.com>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <richardcochran@gmail.com>
References: <20230926155658.159184-1-vishvambarpanth.s@microchip.com>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20230926155658.159184-1-vishvambarpanth.s@microchip.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UTBLYnZoRFRLbGoxQ2Fpc3Jzb1FoSVFQWFRoUm51R0RYUy9mY051Q2dCbUVL?=
 =?utf-8?B?YXFvT244Rm9uVmlacUEzMTlkajNkRHo3VUZVTzU2YUVrTUVPN0RVRDN0NW5a?=
 =?utf-8?B?VjRhcmFKd1Zwb0ttQVBqVDVGUDlxNk14TWlmSEdja3JDK0g2YVZWcWpKVlNW?=
 =?utf-8?B?d0JISmNoRThFMzlUNWs0bTBxZ0szNzdld256RVpodWROMnpDL3ZqQTliMFRx?=
 =?utf-8?B?WTVBWmFZWnBuM2lkME1UYTZCOFBCV2swUlNnTzJ5TWFvL0szZ2hjOU9LNUNj?=
 =?utf-8?B?ZlVsTVZCb1J0V3NHMGE3cnp2b0FLNUY1V29DSFpYdzNoY3RDQTVDRkRkbmhG?=
 =?utf-8?B?MWpSZjk4TWxjSnpHMndqUjkzUzFZNXJrZXp1ajZKVWUwNFk0NU1POHlyLy90?=
 =?utf-8?B?RzRadXZTcmNnZDZRMUhSN0liUStSNHJuVWNOZXpPdVVlRU00OG9Ja09DVTNV?=
 =?utf-8?B?cDR3R1RxVGdPanhlK3FncXNHNzBub3NHWUp5NHhPMzVOdlJLQnpqTHFzZWFx?=
 =?utf-8?B?bExtNGZtQTlrTjFLeThldG1YY2l5Q2U4T2N4Y2xWUnl2SnFYVnMzMHg2QXFE?=
 =?utf-8?B?L0hhSFZMWGtqNms0YnB1L0s2d3R4SzNPMmJsV05uQzBSRklCbkphYTljdWl2?=
 =?utf-8?B?akdLMkZ1R1poUG16U2Z2ZG1TUUYrOTBTbUR4blZONnJuMGpYbmRmemNzSGgv?=
 =?utf-8?B?L0M5RUQ5T3N1ZU1JbUVqRDNqZDB5aFNMYlFHdVkraWpCWDMrbUtpcDNRWEpY?=
 =?utf-8?B?d2tKZDVya0xXdUZMcUVLYmQxdXFpU3M2M09sbmhvKzJ6dGR5QWlIZVBvWXFk?=
 =?utf-8?B?S09UbVhmSmkyWXJWNmNjbVlad2RNWWFkUFBoVXJ1eGxLZmo3eXZCcU9nb1lj?=
 =?utf-8?B?UzdZcFVjcER2eUFHSVJxcVliY2RBTlMwa3p0cS8xcVhzV2RVdzFIa2FIRXVN?=
 =?utf-8?B?NUVSQmNsVU8rdWZnOXgxT2FPZTdpWUQyVXdHeHR6WGVFUlkrQndtM1hnV0xC?=
 =?utf-8?B?ODJ6dDd6cFpMSUtBWlFFN3dmU0FpaUgrTzFENmZZK08yVnlORmo0bEp2bENj?=
 =?utf-8?B?ZW40QXdzRzBrTU0ybWkybkhManVYdTIwOVBvN3V3VWNnRThBbFBNZW1SZnZX?=
 =?utf-8?B?TkR2MC9LdjJEa1lQZkdiblhnbzloeFRZUVd0R3c4UE9TY2FaQWI2L1YxdGdW?=
 =?utf-8?B?eFQzVHVWYlBBaEIzT0ZzVW1Wbk9OaDF0OTZkT0RqbnQrMkY1NXpCUEs4OGhs?=
 =?utf-8?B?RGcwZGdzTkJpbkQrSmQ4aFhuZm9GYU5RNUI4SWZGamFnUVUxMFl6OGhlN2Nr?=
 =?utf-8?B?T3BtNCthRWp6dHhGQ2JpNTVhZStnUld5UysxeHh3bGd1bEJVa05WcExOTnJ2?=
 =?utf-8?B?RTJ0L0JkUHM0c3h6cTQ0NWJxQmZYekdJZllZWHBGZFIxd3BOQTZBYW83WDJB?=
 =?utf-8?B?MkVEeU8ybG91NTExaXNxcVBQZzBIVElGSFJRRk54aG12QUtYNEtMQi9Eb3dO?=
 =?utf-8?B?MkZESHlFckVLVDd5cng4cHIyRWF6eXczd3FTRG02VUQwU2tJc2hrL1FqalQy?=
 =?utf-8?B?WjUwYXk3OUl2and2RjdrSUJvMkw0QzBVWjllL3NNNDUydUo2ODRmeUZyYlV2?=
 =?utf-8?B?SVNYbHZkOUNNeGVhbkEyZXhVYk9vMkFtQWhsZXB0emxjQXVSaDdtSVA5eFZz?=
 =?utf-8?B?dlowQjNHTjNBRmZFS0FidTlETXFqczRxOVZLYkpRSjFJeWhBa2wwdW1BTVUy?=
 =?utf-8?B?ZWpQK3FxelpVMktobG1XeVFzLzNyNDBrWE1DbWdlNmliUFNJeXpYNW5QVGNo?=
 =?utf-8?B?M1doaWFZYktieUlRdFpzSlRSalQ3YnBlU3BzMlQ4S1pCeGF6RjVPZGFzdEQr?=
 =?utf-8?B?Q05Ra3BwSEdDa0NBZkF1SHlibmsrY1JjblhyRzlzK3ZHOTE2YldkNmg0cXc1?=
 =?utf-8?B?RnJ5c2w2NitGU2pvSm4wUmhVSmpPenhaVnY0ajNKTjhZUnBGNDZnd1l4Z1JV?=
 =?utf-8?B?QW0xaW5aNGdJQWxzMFdXcFVTT2NZMXJTU1A4RjU5Z1pUKzdQTm5hbGlJaEZU?=
 =?utf-8?B?endLdmVpajFoOEhqM2RhS1lwUXNEaFh5R3VDc3o2SHBMbzNCMXRkaTRuRGdy?=
 =?utf-8?B?ZjN1U3AvUDZGdG8yOHVRaEt2WHNGUGh2Mk9KUTJCSTl5OEJudklzV290ZTR0?=
 =?utf-8?B?TUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 62ba2979-1f91-4aef-e267-08dbbeeb0f65
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2023 23:48:18.9541
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4hXtkBT8yiX1JZjCRTpq8UMVQnoxNuP62xHXFi01U+7yxRASgeAbXdCGI4s3/JosentpmVM1CEnmXP01CKyeR8DsqhymcUcsB+/FuZWZY7s=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7274
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Tue, 26 Sep 2023 17:40:14 -0700 (PDT)


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@microchip.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@intel.com>