Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 11:05:41 +0200
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Richard Cochran <richardcochran@gmail.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <andrew@lunn.ch>, <hkallweit1@gmail.com>, <linux@armlinux.org.uk>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>
Subject: Re: [PATCH net-next] net: micrel: Change to receive timestamp in the
 frame for lan8841
Message-ID: <20230608090541.23ldnmzr5r56bgf7@soft-dev3-1>
References: <20230607070948.1746768-1-horatiu.vultur@microchip.com>
 <ZIDCpPbCFCxKBV2k@hoboy.vegasvil.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <ZIDCpPbCFCxKBV2k@hoboy.vegasvil.org>
Precedence: bulk

Hi Richard,

The 06/07/2023 10:47, Richard Cochran wrote:
> 
> On Wed, Jun 07, 2023 at 09:09:48AM +0200, Horatiu Vultur wrote:
> 
> > Doing these changes to start to get the received timestamp in the
> > reserved field of the header, will give a great CPU usage performance.
> > Running ptp4l with logSyncInterval of -9 will give a ~50% CPU
> > improvment.
> 
> Really?

I have run a simple top on the PC while running ptp4l.

This is the output without this patch:
  PID  PPID USER     STAT   VSZ %VSZ %CPU COMMAND
  136     2 root     DW       0   0%  58% [irq/33-e2004118]
  141   133 root     R     2232   0%  23% ptp4l -f /tmp/linux.cfg

And this is the output with the patch:
  142   134 root     S     2232   0%  15% ptp4l -f /tmp/linux.cfg
   36     2 root     DW       0   0%  15% [ptp0]
  137     2 root     SW       0   0%   0% [irq/33-e2004118]

If you think I should do better measurements, I am open to suggestions.

> 
> > -static struct lan8814_ptp_rx_ts *lan8841_ptp_get_rx_ts(struct kszphy_ptp_priv *ptp_priv)
> > -{
> > -     struct phy_device *phydev = ptp_priv->phydev;
> > -     struct lan8814_ptp_rx_ts *rx_ts;
> > -     u32 sec, nsec;
> > -     u16 seq;
> > -
> > -     nsec = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_NS_HI);
> > -     if (!(nsec & LAN8841_PTP_RX_INGRESS_NSEC_HI_VALID))
> > -             return NULL;
> > -
> > -     nsec = ((nsec & 0x3fff) << 16);
> > -     nsec = nsec | phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_NS_LO);
> > -
> > -     sec = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_SEC_HI);
> > -     sec = sec << 16;
> > -     sec = sec | phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_SEC_LO);
> > -
> > -     seq = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_MSG_HEADER2);
> 
> Before: 5x phy_read_mmd() per frame ...
> 
> > -     rx_ts = kzalloc(sizeof(*rx_ts), GFP_KERNEL);
> > -     if (!rx_ts)
> > -             return NULL;
> > -
> > -     rx_ts->seconds = sec;
> > -     rx_ts->nsec = nsec;
> > -     rx_ts->seq_id = seq;
> > -
> > -     return rx_ts;
> > -}
> 
> > +static void lan8841_ptp_getseconds(struct ptp_clock_info *ptp,
> > +                                struct timespec64 *ts)
> > +{
> > +     struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> > +                                                     ptp_clock_info);
> > +     struct phy_device *phydev = ptp_priv->phydev;
> > +     time64_t s;
> > +
> > +     mutex_lock(&ptp_priv->ptp_lock);
> > +     /* Issue the command to read the LTC */
> > +     phy_write_mmd(phydev, 2, LAN8841_PTP_CMD_CTL,
> > +                   LAN8841_PTP_CMD_CTL_PTP_LTC_READ);
> > +
> > +     /* Read the LTC */
> > +     s = phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_HI);
> > +     s <<= 16;
> > +     s |= phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_MID);
> > +     s <<= 16;
> > +     s |= phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_LO);
> 
> After: 4x phy_read_mmd() per frame.  How does that save 50% cpu?
> 
> > +     mutex_unlock(&ptp_priv->ptp_lock);
> > +
> > +     set_normalized_timespec64(ts, s, 0);
> > +}
> 
> 
> > +static long lan8841_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > +{
> > +     struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> > +                                                     ptp_clock_info);
> > +     struct skb_shared_hwtstamps *shhwtstamps;
> > +     struct timespec64 ts;
> > +     struct sk_buff *skb;
> > +     u32 ts_header;
> > +
> > +     while ((skb = skb_dequeue(&ptp_priv->rx_queue)) != NULL) {
> > +             lan8841_ptp_getseconds(ptp, &ts);
> 
> No need to call this once per frame.  It would be sufficent to call it
> once every 2 seconds and cache the result.
> 
> > +             ts_header = __be32_to_cpu(LAN8841_SKB_CB(skb)->header->reserved2);
> > +
> > +             shhwtstamps = skb_hwtstamps(skb);
> > +             memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> > +
> > +             /* Check for any wrap arounds for the second part */
> > +             if ((ts.tv_sec & GENMASK(1, 0)) < ts_header >> 30)
> > +                     ts.tv_sec -= GENMASK(1, 0) + 1;
> > +
> > +             shhwtstamps->hwtstamp =
> > +                     ktime_set((ts.tv_sec & ~(GENMASK(1, 0))) | ts_header >> 30,
> > +                               ts_header & GENMASK(29, 0));
> > +             LAN8841_SKB_CB(skb)->header->reserved2 = 0;
> > +
> > +             netif_rx(skb);
> > +     }
> > +
> > +     return -1;
> > +}
> 
> Thanks,
> Richard

-- 
/Horatiu