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 A50D14388D; Thu, 11 Jan 2024 07:26:05 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7185140266; Thu, 11 Jan 2024 07:26:05 +0100 (CET) Received: from szxga07-in.huawei.com (szxga07-in.huawei.com [45.249.212.35]) by mails.dpdk.org (Postfix) with ESMTP id B138040042 for ; Thu, 11 Jan 2024 07:26:03 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.88.163]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4T9ZQ3153vz1Q8CW; Thu, 11 Jan 2024 14:24:27 +0800 (CST) Received: from kwepemm600004.china.huawei.com (unknown [7.193.23.242]) by mail.maildlp.com (Postfix) with ESMTPS id 288B51800B8; Thu, 11 Jan 2024 14:26:00 +0800 (CST) Received: from [10.67.121.59] (10.67.121.59) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Thu, 11 Jan 2024 14:25:59 +0800 Message-ID: <7ecc6f3b-78f8-6421-307d-2c6c484c6109@huawei.com> Date: Thu, 11 Jan 2024 14:25:58 +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 From: "lihuisong (C)" 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> <665b0b6e-1ad9-a692-39cb-9e45e6b78b08@huawei.com> In-Reply-To: <665b0b6e-1ad9-a692-39cb-9e45e6b78b08@huawei.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemm600004.china.huawei.com (7.193.23.242) 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 Hi Ferruh, 在 2023/11/23 19:56, lihuisong (C) 写道: > > 在 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? Can you take a look at this discussion line again? Let's introduce a  RTE_ETH_DEV_PTP in dev->data->dev_flags to reveal if the device support 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(-) >>>>>>> >>>>>> . >>>> . >> . > > .