Re: [PATCH net-next] net: micrel: Change to receive timestamp in the frame for lan8841

[Horatiu Vultur <horatiu.vultur@microchip.com>]

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