From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8B3CE433A8; Thu, 23 Nov 2023 12:56:17 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 788FA42F2D; Thu, 23 Nov 2023 12:56:17 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id C675F42F2B for ; Thu, 23 Nov 2023 12:56:15 +0100 (CET) Received: from kwepemm000004.china.huawei.com (unknown [172.30.72.53]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Sbc504QKlzvR0t; Thu, 23 Nov 2023 19:55:48 +0800 (CST) Received: from [10.67.121.59] (10.67.121.59) by kwepemm000004.china.huawei.com (7.193.23.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Thu, 23 Nov 2023 19:56:13 +0800 Message-ID: <665b0b6e-1ad9-a692-39cb-9e45e6b78b08@huawei.com> Date: Thu, 23 Nov 2023 19:56:12 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v3 0/2] ethdev: add the check for PTP capability To: Ferruh Yigit , CC: , , , Gagandeep Singh , Hemant Agrawal , Simei Su , Qi Zhang , Qiming Yang , Junfeng Guo References: <20220628133959.21381-1-liudongdong3@huawei.com> <20230817084226.55327-1-lihuisong@huawei.com> <1834a6a9-ef92-4a67-a987-490151cf5380@amd.com> <242e8583-969e-d8ca-2dd4-80e8cf73a662@huawei.com> <0d7f429c-8862-4f16-b7e5-46d69581f54f@amd.com> <3a11b30d-346f-446f-903a-5a56cbae3853@amd.com> From: "lihuisong (C)" In-Reply-To: <3a11b30d-346f-446f-903a-5a56cbae3853@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemm000004.china.huawei.com (7.193.23.18) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 在 2023/11/2 7:39, Ferruh Yigit 写道: > timesync_read_rx_timestamp > On 9/21/2023 12:59 PM, lihuisong (C) wrote: >> add ice & igc maintainers >> >> 在 2023/9/21 19:06, Ferruh Yigit 写道: >>> On 9/21/2023 11:02 AM, lihuisong (C) wrote: >>>> Hi Ferruh, >>>> >>>> Sorry for my delay reply because of taking a look at all PMDs >>>> implementation. >>>> >>>> >>>> 在 2023/9/16 1:46, Ferruh Yigit 写道: >>>>> On 8/17/2023 9:42 AM, Huisong Li wrote: >>>>>>   From the first version of ptpclient, it seems that this example >>>>>> assume that >>>>>> the PMDs support the PTP feature and enable PTP by default. Please see >>>>>> commit ab129e9065a5 ("examples/ptpclient: add minimal PTP client") >>>>>> which are introduced in 2015. >>>>>> >>>>>> And two years later, Rx HW timestamp offload was introduced to >>>>>> enable or >>>>>> disable PTP feature in HW via rte_eth_rxmode. Please see >>>>>> commit 42ffc45aa340 ("ethdev: add Rx HW timestamp capability"). >>>>>> >>>>> Hi Huisong, >>>>> >>>>> As far as I know this offload is not for PTP. >>>>> PTP and TIMESTAMP are different. >>>> If TIMESTAMP offload cannot stand for PTP, we may need to add one new >>>> offlaod for PTP. >>>> >>> Can you please detail what is "PTP offload"? >>> >> It indicates whether the device supports PTP or enable  PTP feature. >> > We have 'rte_eth_timesync_enable()' and 'rte_eth_timesync_disable()' > APIs to control PTP support. No, this is just to control it. we still need to like a device capablity to report application if the port support to call this API, right? > > But when mention from "offload", it is something device itself does. > > PTP is a protocol (IEEE 1588), and used to synchronize clocks. > What I get is protocol can be parsed by networking stack and it can be > used by application to synchronize clock. > > When you are refer to "PTP offload", does it mean device (NIC) > understands the protocol and parse it to synchronize device clock with > other devices? Good point. PTP offload is unreasonable. But the capablity is required indeed. What do you think of introducing a RTE_ETH_DEV_PTP in dev->data->dev_flags for PTP feature? > > > We have 'rte_eth_timesync_*()' APIs, my understanding is application > parses the PTP protocol, and it may use this information to configure > NIC to synchronize its clock, but it may also use PTP provided > information to sync any other clock. Is this understanding correct? > > >> If TIMESTAMP offload is not for PTP, I don't know what the point of this >> offload independent existence is. >> > TIMESTAMP offload request device to add timestamp to mbuf in ingress, > and use mbuf timestamp to schedule packet for egress. Agree. > > Technically this time-stamping can be done by driver, but if offload > set, HW timestamp is used for it. > > Rx timestamp can be used for various reasons, like debugging and > performance/latency analyses, etc.. > > >>>>> PTP is a protocol for time sync. >>>>> Rx TIMESTAMP offload is to ask HW to add timestamp to mbuf. >>>> Yes. >>>> But a lot of PMDs actually depand on HW to report Rx timestamp releated >>>> information >>>> because of reading Rx timestamp of PTP SYNC packet in read_rx_timestamp >>>> API. >>>> >>> HW support may be required for PTP but this doesn't mean timestamp >>> offload is used. >> understand. >>>>>> And then about four years later, ptpclient enable Rx timestamp offload >>>>>> because some PMDs require this offload to enable. Please see >>>>>> commit 7a04a4f67dca ("examples/ptpclient: enable Rx timestamp >>>>>> offload"). >>>>>> >>>>> dpaa2 seems using TIMESTAMP offload and PTP together, hence they >>>>> updated >>>>> ptpclient sample to set TIMESTAMP offload. >>>> There are many PMDs doing like this, such as ice, igc, cnxk, dpaa2, hns3 >>>> and so on. >>>> >>> Can you please point the ice & igc code, cc'ing their maintainers, we >>> can look together? >> *-->igc code:* >> >> Having following codes in igc_recv_scattered_pkts(): >> >>         if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) { >>             uint32_t *ts = rte_pktmbuf_mtod_offset(first_seg, >>                     uint32_t *, -IGC_TS_HDR_LEN); >>             rxq->rx_timestamp = (uint64_t)ts[3] * NSEC_PER_SEC + >>                     ts[2]; >>             rxm->timesync = rxq->queue_id; >>         } >> Note：this rxm->timesync will be used in timesync_read_rx_timestamp() >> > Above code requires TIMESTAMP offload to set timesync, but this > shouldn't be a requirement. Usage seems mixed. > >> *-->ice code:* >> >> #ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC >>         if (ice_timestamp_dynflag > 0 && >>             (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)) { >>             rxq->time_high = >>                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high); >>             if (unlikely(is_tsinit)) { >>                 ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1, >> rxq->time_high); >>                 rxq->hw_time_low = (uint32_t)ts_ns; >>                 rxq->hw_time_high = (uint32_t)(ts_ns >> 32); >>                 is_tsinit = false; >>             } else { >>                 if (rxq->time_high < rxq->hw_time_low) >>                     rxq->hw_time_high += 1; >>                 ts_ns = (uint64_t)rxq->hw_time_high << 32 | rxq->time_high; >>                 rxq->hw_time_low = rxq->time_high; >>             } >>             rxq->hw_time_update = rte_get_timer_cycles() / >>                          (rte_get_timer_hz() / 1000); >>             *RTE_MBUF_DYNFIELD(rxm, >>                        (ice_timestamp_dynfield_offset), >>                        rte_mbuf_timestamp_t *) = ts_ns; >>             pkt_flags |= ice_timestamp_dynflag; >>         } >> >>         if (ad->ptp_ena && ((rxm->packet_type & RTE_PTYPE_L2_MASK) == >>             RTE_PTYPE_L2_ETHER_TIMESYNC)) { >>             rxq->time_high = >>                rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high); >>             rxm->timesync = rxq->queue_id; >>             pkt_flags |= RTE_MBUF_F_RX_IEEE1588_PTP; >>         } >> #endif >> >> Note: rxm->timesync and rxq->time_high will be used in >> timesync_read_rx_timestamp() >> > This usage looks good, if TIMESTAMP offload enabled mbuf dynamic field > and flag set accordingly. hns3 also implemented PTP as ice did. > > And if PTP enabled, and PTP packet type detected relevant flag set in > mbuf, and timesyc value set to use later for 'timesync_read_rx_timestamp()'. Yes. > > > Although above usage looks correct, I can see in 'ice_timesync_enable()' > TIMESTAMP offload is used requirement to enable timesync. > TIMESTAMP offload seems used as way to enable HW timestamp, as Hemant > mentioned what is done is dpaa2. > > >>> >>>>> We need to clarify dpaa2 usage. >>>>> >>>>>> By all the records, this is more like a process of perfecting PTP >>>>>> feature. >>>>>> Not all network adaptors support PTP feature. So adding the check for >>>>>> PTP >>>>>> capability in ethdev layer is necessary. >>>>>> >>>>> Nope, as PTP (IEEE1588/802.1AS) implemented as dev_ops, and ops already >>>>> checked, so no additional check is needed. >>>> But only having dev_ops about PTP doesn't satisfy the use of this >>>> feature. >>>> For example, >>>> there are serveal network ports belonged to a driver on one OS, and only >>>> one port support PTP function. >>>> So driver needs one *PTP* offload. >>>>> We just need to clarify TIMESTAMP offload and PTP usage and find out >>>>> what is causing confusion. >>>> Yes it is a little bit confusion. >>>> There are two kinds of implementation: >>>> A： ixgbe and txgbe (it seems that their HW is similar) don't need >>>> TIMESTAMP offload，and only use dev_ops to finish PTP feature. >>>> B:  saving "Rx timestamp related information" from Rx description when >>>> receive PTP SYNC packet and >>>>      report it in read_rx_timestamp API. >>>> For case B, most of driver use TIMESTAMP offload to decide if driver >>>> save "Rx timestamp related information. >>>> What do you think about this, Ferruh? >>>>> I would be great if you can help on clarification, and update >>>>> documentation or API comments, or what ever required, for this. >>>> ok >>>>>> --- >>>>>> v3: >>>>>>    - patch [2/3] for hns3 has been applied and so remove it. >>>>>>    - ops pointer check is closer to usage. >>>>>> >>>>>> Huisong Li (2): >>>>>>     examples/ptpclient: add the check for PTP capability >>>>>>     ethdev: add the check for the valitity of timestamp offload >>>>>> >>>>>>    examples/ptpclient/ptpclient.c |  5 +++ >>>>>>    lib/ethdev/rte_ethdev.c        | 57 >>>>>> +++++++++++++++++++++++++++++++++- >>>>>>    2 files changed, 61 insertions(+), 1 deletion(-) >>>>>> >>>>> . >>> . > .