[PATCH net-next 0/2] mlx4_en: Add PTP support

[PATCH net-next 0/2] mlx4_en: Add PTP support

[Shawn Bohrer]

From: Shawn Bohrer <sbohrer@rgmadvisors.com>

This adds support to the mlx4_en driver to support running a PTP client
with hardware timestamp support.  It also allows synchronization of the
hardware timestamped packets with the system clock.  

Shawn Bohrer (2):
  mlx4_en: Add PTP hardware clock
  mlx4_en: Only cycle port if HW timestamp config changes

 drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  196 ++++++++++++++++++++++-
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
 drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
 4 files changed, 200 insertions(+), 8 deletions(-)

-- 
1.7.7.6

[PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

From: Shawn Bohrer <sbohrer@rgmadvisors.com>

This adds a PHC to the mlx4_en driver.  The code is largely based off of
the e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
very similar.

This driver has been tested with both Documentation/ptp/testptp and the
linuxptp project (http://linuxptp.sourceforge.net/) and appears to work
on a Mellanox ConnectX-3 card.

Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
 drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
 4 files changed, 196 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
index fd64410..9b0d515 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
@@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
 			    struct skb_shared_hwtstamps *hwts,
 			    u64 timestamp)
 {
+	unsigned long flags;
 	u64 nsec;
 
+	spin_lock_irqsave(&mdev->clock_lock, flags);
 	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
 
 	memset(hwts, 0, sizeof(struct skb_shared_hwtstamps));
 	hwts->hwtstamp = ns_to_ktime(nsec);
 }
 
+/**
+ * mlx4_en_remove_timestamp - disable PTP device
+ * @mdev: board private structure
+ *
+ * Stop the PTP support.
+ **/
+void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev)
+{
+	if (mdev->ptp_clock) {
+		ptp_clock_unregister(mdev->ptp_clock);
+		mdev->ptp_clock = NULL;
+		mlx4_info(mdev, "removed PHC\n");
+	}
+}
+
+void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
+{
+	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
+					      mdev->overflow_period);
+	unsigned long flags;
+
+	if (timeout) {
+		spin_lock_irqsave(&mdev->clock_lock, flags);
+		timecounter_read(&mdev->clock);
+		spin_unlock_irqrestore(&mdev->clock_lock, flags);
+		mdev->last_overflow_check = jiffies;
+	}
+}
+
+/**
+ * mlx4_en_phc_adjfreq - adjust the frequency of the hardware clock
+ * @ptp: ptp clock structure
+ * @delta: Desired frequency change in parts per billion
+ *
+ * Adjust the frequency of the PHC cycle counter by the indicated delta from
+ * the base frequency.
+ **/
+static int mlx4_en_phc_adjfreq(struct ptp_clock_info *ptp, s32 delta)
+{
+	u64 adj;
+	u32 diff, mult;
+	int neg_adj = 0;
+	unsigned long flags;
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+
+	if (delta < 0) {
+		neg_adj = 1;
+		delta = -delta;
+	}
+	mult = mdev->nominal_c_mult;
+	adj = mult;
+	adj *= delta;
+	diff = div_u64(adj, 1000000000ULL);
+
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	timecounter_read(&mdev->clock);
+	mdev->cycles.mult = neg_adj ? mult - diff : mult + diff;
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_adjtime - Shift the time of the hardware clock
+ * @ptp: ptp clock structure
+ * @delta: Desired change in nanoseconds
+ *
+ * Adjust the timer by resetting the timecounter structure.
+ **/
+static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	unsigned long flags;
+	s64 now;
+
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	now = timecounter_read(&mdev->clock);
+	now += delta;
+	timecounter_init(&mdev->clock, &mdev->cycles, now);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_gettime - Reads the current time from the hardware clock
+ * @ptp: ptp clock structure
+ * @ts: timespec structure to hold the current time value
+ *
+ * Read the timecounter and return the correct value in ns after converting
+ * it into a struct timespec.
+ **/
+static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	unsigned long flags;
+	u32 remainder;
+	u64 ns;
+
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	ns = timecounter_read(&mdev->clock);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+
+	ts->tv_sec = div_u64_rem(ns, NSEC_PER_SEC, &remainder);
+	ts->tv_nsec = remainder;
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_settime - Set the current time on the hardware clock
+ * @ptp: ptp clock structure
+ * @ts: timespec containing the new time for the cycle counter
+ *
+ * Reset the timecounter to use a new base value instead of the kernel
+ * wall timer value.
+ **/
+static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
+			       const struct timespec *ts)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	u64 ns = timespec_to_ns(ts);
+	unsigned long flags;
+
+	/* reset the timecounter */
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	timecounter_init(&mdev->clock, &mdev->cycles, ns);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_enable - enable or disable an ancillary feature
+ * @ptp: ptp clock structure
+ * @request: Desired resource to enable or disable
+ * @on: Caller passes one to enable or zero to disable
+ *
+ * Enable (or disable) ancillary features of the PHC subsystem.
+ * Currently, no ancillary features are supported.
+ **/
+static int mlx4_en_phc_enable(struct ptp_clock_info __always_unused *ptp,
+			      struct ptp_clock_request __always_unused *request,
+			      int __always_unused on)
+{
+	return -EOPNOTSUPP;
+}
+
+static const struct ptp_clock_info mlx4_en_ptp_clock_info = {
+	.owner		= THIS_MODULE,
+	.max_adj	= 100000000,
+	.n_alarm	= 0,
+	.n_ext_ts	= 0,
+	.n_per_out	= 0,
+	.pps		= 0,
+	.adjfreq	= mlx4_en_phc_adjfreq,
+	.adjtime	= mlx4_en_phc_adjtime,
+	.gettime	= mlx4_en_phc_gettime,
+	.settime	= mlx4_en_phc_settime,
+	.enable		= mlx4_en_phc_enable,
+};
+
 void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 {
 	struct mlx4_dev *dev = mdev->dev;
+	unsigned long flags;
 	u64 ns;
 
 	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
@@ -127,9 +297,12 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 	mdev->cycles.shift = 14;
 	mdev->cycles.mult =
 		clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
+	mdev->nominal_c_mult = mdev->cycles.mult;
 
+	spin_lock_irqsave(&mdev->clock_lock, flags);
 	timecounter_init(&mdev->clock, &mdev->cycles,
 			 ktime_to_ns(ktime_get_real()));
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
 
 	/* Calculate period in seconds to call the overflow watchdog - to make
 	 * sure counter is checked at least once every wrap around.
@@ -137,15 +310,18 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 	ns = cyclecounter_cyc2ns(&mdev->cycles, mdev->cycles.mask);
 	do_div(ns, NSEC_PER_SEC / 2 / HZ);
 	mdev->overflow_period = ns;
-}
 
-void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
-{
-	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
-					      mdev->overflow_period);
+	/* Configure the PHC */
+	mdev->ptp_clock_info = mlx4_en_ptp_clock_info;
+	snprintf(mdev->ptp_clock_info.name, 16, "mlx4 ptp");
 
-	if (timeout) {
-		timecounter_read(&mdev->clock);
-		mdev->last_overflow_check = jiffies;
+	mdev->ptp_clock = ptp_clock_register(&mdev->ptp_clock_info,
+					     &mdev->pdev->dev);
+	if (IS_ERR(mdev->ptp_clock)) {
+		mdev->ptp_clock = NULL;
+		mlx4_err(mdev, "ptp_clock_register failed\n");
+	} else {
+		mlx4_info(mdev, "registered PHC clock\n");
 	}
+
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 0596f9f..3e8d336 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1193,6 +1193,9 @@ static int mlx4_en_get_ts_info(struct net_device *dev,
 		info->rx_filters =
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_ALL);
+
+		if (mdev->ptp_clock)
+			info->phc_index = ptp_clock_index(mdev->ptp_clock);
 	}
 
 	return ret;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_main.c b/drivers/net/ethernet/mellanox/mlx4/en_main.c
index 0d087b0..6268057 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_main.c
@@ -196,6 +196,9 @@ static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
 		if (mdev->pndev[i])
 			mlx4_en_destroy_netdev(mdev->pndev[i]);
 
+	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
+		mlx4_en_remove_timestamp(mdev);
+
 	flush_workqueue(mdev->workqueue);
 	destroy_workqueue(mdev->workqueue);
 	(void) mlx4_mr_free(dev, &mdev->mr);
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index f3758de..2b4083f 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -45,6 +45,7 @@
 #include <linux/dcbnl.h>
 #endif
 #include <linux/cpu_rmap.h>
+#include <linux/ptp_clock_kernel.h>
 
 #include <linux/mlx4/device.h>
 #include <linux/mlx4/qp.h>
@@ -373,10 +374,14 @@ struct mlx4_en_dev {
 	u32                     priv_pdn;
 	spinlock_t              uar_lock;
 	u8			mac_removed[MLX4_MAX_PORTS + 1];
+	spinlock_t		clock_lock;
+	u32			nominal_c_mult;
 	struct cyclecounter	cycles;
 	struct timecounter	clock;
 	unsigned long		last_overflow_check;
 	unsigned long		overflow_period;
+	struct ptp_clock	*ptp_clock;
+	struct ptp_clock_info	ptp_clock_info;
 };
 
 
@@ -786,6 +791,7 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
 			    struct skb_shared_hwtstamps *hwts,
 			    u64 timestamp);
 void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev);
+void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev);
 int mlx4_en_timestamp_config(struct net_device *dev,
 			     int tx_type,
 			     int rx_filter);
-- 
1.7.7.6

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Or Gerlitz]

On Tue, Dec 17, 2013 at 10:32 PM, Shawn Bohrer <shawn.bohrer@gmail.com> wrote:
> From: Shawn Bohrer <sbohrer@rgmadvisors.com>
>
> This adds a PHC to the mlx4_en driver.  The code is largely based off of
> the e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> very similar.
>
> This driver has been tested with both Documentation/ptp/testptp and the
> linuxptp project (http://linuxptp.sourceforge.net/) and appears to work
> on a Mellanox ConnectX-3 card.

so this is V3 -- and what are the V2 --> V3 changes?

>
> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
>  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
>  drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
>  drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
>  4 files changed, 196 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> index fd64410..9b0d515 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> @@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
>                             struct skb_shared_hwtstamps *hwts,
>                             u64 timestamp)
>  {
> +       unsigned long flags;
>         u64 nsec;
>
> +       spin_lock_irqsave(&mdev->clock_lock, flags);
>         nsec = timecounter_cyc2time(&mdev->clock, timestamp);
> +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
>
>         memset(hwts, 0, sizeof(struct skb_shared_hwtstamps));
>         hwts->hwtstamp = ns_to_ktime(nsec);
>  }
>
> +/**
> + * mlx4_en_remove_timestamp - disable PTP device
> + * @mdev: board private structure
> + *
> + * Stop the PTP support.
> + **/
> +void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev)
> +{
> +       if (mdev->ptp_clock) {
> +               ptp_clock_unregister(mdev->ptp_clock);
> +               mdev->ptp_clock = NULL;
> +               mlx4_info(mdev, "removed PHC\n");
> +       }
> +}
> +
> +void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> +{
> +       bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> +                                             mdev->overflow_period);
> +       unsigned long flags;
> +
> +       if (timeout) {
> +               spin_lock_irqsave(&mdev->clock_lock, flags);
> +               timecounter_read(&mdev->clock);
> +               spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +               mdev->last_overflow_check = jiffies;
> +       }
> +}
> +
> +/**
> + * mlx4_en_phc_adjfreq - adjust the frequency of the hardware clock
> + * @ptp: ptp clock structure
> + * @delta: Desired frequency change in parts per billion
> + *
> + * Adjust the frequency of the PHC cycle counter by the indicated delta from
> + * the base frequency.
> + **/
> +static int mlx4_en_phc_adjfreq(struct ptp_clock_info *ptp, s32 delta)
> +{
> +       u64 adj;
> +       u32 diff, mult;
> +       int neg_adj = 0;
> +       unsigned long flags;
> +       struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +                                               ptp_clock_info);
> +
> +       if (delta < 0) {
> +               neg_adj = 1;
> +               delta = -delta;
> +       }
> +       mult = mdev->nominal_c_mult;
> +       adj = mult;
> +       adj *= delta;
> +       diff = div_u64(adj, 1000000000ULL);
> +
> +       spin_lock_irqsave(&mdev->clock_lock, flags);
> +       timecounter_read(&mdev->clock);
> +       mdev->cycles.mult = neg_adj ? mult - diff : mult + diff;
> +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +       return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_adjtime - Shift the time of the hardware clock
> + * @ptp: ptp clock structure
> + * @delta: Desired change in nanoseconds
> + *
> + * Adjust the timer by resetting the timecounter structure.
> + **/
> +static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +       struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +                                               ptp_clock_info);
> +       unsigned long flags;
> +       s64 now;
> +
> +       spin_lock_irqsave(&mdev->clock_lock, flags);
> +       now = timecounter_read(&mdev->clock);
> +       now += delta;
> +       timecounter_init(&mdev->clock, &mdev->cycles, now);
> +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +       return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_gettime - Reads the current time from the hardware clock
> + * @ptp: ptp clock structure
> + * @ts: timespec structure to hold the current time value
> + *
> + * Read the timecounter and return the correct value in ns after converting
> + * it into a struct timespec.
> + **/
> +static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
> +{
> +       struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +                                               ptp_clock_info);
> +       unsigned long flags;
> +       u32 remainder;
> +       u64 ns;
> +
> +       spin_lock_irqsave(&mdev->clock_lock, flags);
> +       ns = timecounter_read(&mdev->clock);
> +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +       ts->tv_sec = div_u64_rem(ns, NSEC_PER_SEC, &remainder);
> +       ts->tv_nsec = remainder;
> +
> +       return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_settime - Set the current time on the hardware clock
> + * @ptp: ptp clock structure
> + * @ts: timespec containing the new time for the cycle counter
> + *
> + * Reset the timecounter to use a new base value instead of the kernel
> + * wall timer value.
> + **/
> +static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
> +                              const struct timespec *ts)
> +{
> +       struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +                                               ptp_clock_info);
> +       u64 ns = timespec_to_ns(ts);
> +       unsigned long flags;
> +
> +       /* reset the timecounter */
> +       spin_lock_irqsave(&mdev->clock_lock, flags);
> +       timecounter_init(&mdev->clock, &mdev->cycles, ns);
> +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +       return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_enable - enable or disable an ancillary feature
> + * @ptp: ptp clock structure
> + * @request: Desired resource to enable or disable
> + * @on: Caller passes one to enable or zero to disable
> + *
> + * Enable (or disable) ancillary features of the PHC subsystem.
> + * Currently, no ancillary features are supported.
> + **/
> +static int mlx4_en_phc_enable(struct ptp_clock_info __always_unused *ptp,
> +                             struct ptp_clock_request __always_unused *request,
> +                             int __always_unused on)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static const struct ptp_clock_info mlx4_en_ptp_clock_info = {
> +       .owner          = THIS_MODULE,
> +       .max_adj        = 100000000,
> +       .n_alarm        = 0,
> +       .n_ext_ts       = 0,
> +       .n_per_out      = 0,
> +       .pps            = 0,
> +       .adjfreq        = mlx4_en_phc_adjfreq,
> +       .adjtime        = mlx4_en_phc_adjtime,
> +       .gettime        = mlx4_en_phc_gettime,
> +       .settime        = mlx4_en_phc_settime,
> +       .enable         = mlx4_en_phc_enable,
> +};
> +
>  void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>  {
>         struct mlx4_dev *dev = mdev->dev;
> +       unsigned long flags;
>         u64 ns;
>
>         memset(&mdev->cycles, 0, sizeof(mdev->cycles));
> @@ -127,9 +297,12 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>         mdev->cycles.shift = 14;
>         mdev->cycles.mult =
>                 clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
> +       mdev->nominal_c_mult = mdev->cycles.mult;
>
> +       spin_lock_irqsave(&mdev->clock_lock, flags);
>         timecounter_init(&mdev->clock, &mdev->cycles,
>                          ktime_to_ns(ktime_get_real()));
> +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
>
>         /* Calculate period in seconds to call the overflow watchdog - to make
>          * sure counter is checked at least once every wrap around.
> @@ -137,15 +310,18 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>         ns = cyclecounter_cyc2ns(&mdev->cycles, mdev->cycles.mask);
>         do_div(ns, NSEC_PER_SEC / 2 / HZ);
>         mdev->overflow_period = ns;
> -}
>
> -void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> -{
> -       bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> -                                             mdev->overflow_period);
> +       /* Configure the PHC */
> +       mdev->ptp_clock_info = mlx4_en_ptp_clock_info;
> +       snprintf(mdev->ptp_clock_info.name, 16, "mlx4 ptp");
>
> -       if (timeout) {
> -               timecounter_read(&mdev->clock);
> -               mdev->last_overflow_check = jiffies;
> +       mdev->ptp_clock = ptp_clock_register(&mdev->ptp_clock_info,
> +                                            &mdev->pdev->dev);
> +       if (IS_ERR(mdev->ptp_clock)) {
> +               mdev->ptp_clock = NULL;
> +               mlx4_err(mdev, "ptp_clock_register failed\n");
> +       } else {
> +               mlx4_info(mdev, "registered PHC clock\n");
>         }
> +
>  }
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index 0596f9f..3e8d336 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -1193,6 +1193,9 @@ static int mlx4_en_get_ts_info(struct net_device *dev,
>                 info->rx_filters =
>                         (1 << HWTSTAMP_FILTER_NONE) |
>                         (1 << HWTSTAMP_FILTER_ALL);
> +
> +               if (mdev->ptp_clock)
> +                       info->phc_index = ptp_clock_index(mdev->ptp_clock);
>         }
>
>         return ret;
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_main.c b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> index 0d087b0..6268057 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> @@ -196,6 +196,9 @@ static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
>                 if (mdev->pndev[i])
>                         mlx4_en_destroy_netdev(mdev->pndev[i]);
>
> +       if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
> +               mlx4_en_remove_timestamp(mdev);
> +
>         flush_workqueue(mdev->workqueue);
>         destroy_workqueue(mdev->workqueue);
>         (void) mlx4_mr_free(dev, &mdev->mr);
> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> index f3758de..2b4083f 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> @@ -45,6 +45,7 @@
>  #include <linux/dcbnl.h>
>  #endif
>  #include <linux/cpu_rmap.h>
> +#include <linux/ptp_clock_kernel.h>
>
>  #include <linux/mlx4/device.h>
>  #include <linux/mlx4/qp.h>
> @@ -373,10 +374,14 @@ struct mlx4_en_dev {
>         u32                     priv_pdn;
>         spinlock_t              uar_lock;
>         u8                      mac_removed[MLX4_MAX_PORTS + 1];
> +       spinlock_t              clock_lock;
> +       u32                     nominal_c_mult;
>         struct cyclecounter     cycles;
>         struct timecounter      clock;
>         unsigned long           last_overflow_check;
>         unsigned long           overflow_period;
> +       struct ptp_clock        *ptp_clock;
> +       struct ptp_clock_info   ptp_clock_info;
>  };
>
>
> @@ -786,6 +791,7 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
>                             struct skb_shared_hwtstamps *hwts,
>                             u64 timestamp);
>  void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev);
> +void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev);
>  int mlx4_en_timestamp_config(struct net_device *dev,
>                              int tx_type,
>                              int rx_filter);
> --
> 1.7.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

On Tue, Dec 17, 2013 at 11:04:48PM +0200, Or Gerlitz wrote:
> On Tue, Dec 17, 2013 at 10:32 PM, Shawn Bohrer <shawn.bohrer@gmail.com> wrote:
> > From: Shawn Bohrer <sbohrer@rgmadvisors.com>
> >
> > This adds a PHC to the mlx4_en driver.  The code is largely based off of
> > the e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> > very similar.
> >
> > This driver has been tested with both Documentation/ptp/testptp and the
> > linuxptp project (http://linuxptp.sourceforge.net/) and appears to work
> > on a Mellanox ConnectX-3 card.
> 
> so this is V3 -- and what are the V2 --> V3 changes?

Sorry, here is the evolution of this patch:

v2:
* Implemented mlx4_en_phc_adjfreq() in software.
* Protected timecounter with a spinlock
* Set the phc_index in mlx4_en_get_ts_info()
* Use a hard-coded ptp_clock_info.name

v3: is code identical to v2.  I simply finished testing it with
linuxptp, cleaned up the commit description a little and sent to davem.

> >
> > Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> > ---
> >  drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
> >  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
> >  drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
> >  drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
> >  4 files changed, 196 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > index fd64410..9b0d515 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > @@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
> >                             struct skb_shared_hwtstamps *hwts,
> >                             u64 timestamp)
> >  {
> > +       unsigned long flags;
> >         u64 nsec;
> >
> > +       spin_lock_irqsave(&mdev->clock_lock, flags);
> >         nsec = timecounter_cyc2time(&mdev->clock, timestamp);
> > +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> >
> >         memset(hwts, 0, sizeof(struct skb_shared_hwtstamps));
> >         hwts->hwtstamp = ns_to_ktime(nsec);
> >  }
> >
> > +/**
> > + * mlx4_en_remove_timestamp - disable PTP device
> > + * @mdev: board private structure
> > + *
> > + * Stop the PTP support.
> > + **/
> > +void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev)
> > +{
> > +       if (mdev->ptp_clock) {
> > +               ptp_clock_unregister(mdev->ptp_clock);
> > +               mdev->ptp_clock = NULL;
> > +               mlx4_info(mdev, "removed PHC\n");
> > +       }
> > +}
> > +
> > +void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> > +{
> > +       bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> > +                                             mdev->overflow_period);
> > +       unsigned long flags;
> > +
> > +       if (timeout) {
> > +               spin_lock_irqsave(&mdev->clock_lock, flags);
> > +               timecounter_read(&mdev->clock);
> > +               spin_unlock_irqrestore(&mdev->clock_lock, flags);
> > +               mdev->last_overflow_check = jiffies;
> > +       }
> > +}
> > +
> > +/**
> > + * mlx4_en_phc_adjfreq - adjust the frequency of the hardware clock
> > + * @ptp: ptp clock structure
> > + * @delta: Desired frequency change in parts per billion
> > + *
> > + * Adjust the frequency of the PHC cycle counter by the indicated delta from
> > + * the base frequency.
> > + **/
> > +static int mlx4_en_phc_adjfreq(struct ptp_clock_info *ptp, s32 delta)
> > +{
> > +       u64 adj;
> > +       u32 diff, mult;
> > +       int neg_adj = 0;
> > +       unsigned long flags;
> > +       struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> > +                                               ptp_clock_info);
> > +
> > +       if (delta < 0) {
> > +               neg_adj = 1;
> > +               delta = -delta;
> > +       }
> > +       mult = mdev->nominal_c_mult;
> > +       adj = mult;
> > +       adj *= delta;
> > +       diff = div_u64(adj, 1000000000ULL);
> > +
> > +       spin_lock_irqsave(&mdev->clock_lock, flags);
> > +       timecounter_read(&mdev->clock);
> > +       mdev->cycles.mult = neg_adj ? mult - diff : mult + diff;
> > +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * mlx4_en_phc_adjtime - Shift the time of the hardware clock
> > + * @ptp: ptp clock structure
> > + * @delta: Desired change in nanoseconds
> > + *
> > + * Adjust the timer by resetting the timecounter structure.
> > + **/
> > +static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
> > +{
> > +       struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> > +                                               ptp_clock_info);
> > +       unsigned long flags;
> > +       s64 now;
> > +
> > +       spin_lock_irqsave(&mdev->clock_lock, flags);
> > +       now = timecounter_read(&mdev->clock);
> > +       now += delta;
> > +       timecounter_init(&mdev->clock, &mdev->cycles, now);
> > +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * mlx4_en_phc_gettime - Reads the current time from the hardware clock
> > + * @ptp: ptp clock structure
> > + * @ts: timespec structure to hold the current time value
> > + *
> > + * Read the timecounter and return the correct value in ns after converting
> > + * it into a struct timespec.
> > + **/
> > +static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
> > +{
> > +       struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> > +                                               ptp_clock_info);
> > +       unsigned long flags;
> > +       u32 remainder;
> > +       u64 ns;
> > +
> > +       spin_lock_irqsave(&mdev->clock_lock, flags);
> > +       ns = timecounter_read(&mdev->clock);
> > +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> > +
> > +       ts->tv_sec = div_u64_rem(ns, NSEC_PER_SEC, &remainder);
> > +       ts->tv_nsec = remainder;
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * mlx4_en_phc_settime - Set the current time on the hardware clock
> > + * @ptp: ptp clock structure
> > + * @ts: timespec containing the new time for the cycle counter
> > + *
> > + * Reset the timecounter to use a new base value instead of the kernel
> > + * wall timer value.
> > + **/
> > +static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
> > +                              const struct timespec *ts)
> > +{
> > +       struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> > +                                               ptp_clock_info);
> > +       u64 ns = timespec_to_ns(ts);
> > +       unsigned long flags;
> > +
> > +       /* reset the timecounter */
> > +       spin_lock_irqsave(&mdev->clock_lock, flags);
> > +       timecounter_init(&mdev->clock, &mdev->cycles, ns);
> > +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * mlx4_en_phc_enable - enable or disable an ancillary feature
> > + * @ptp: ptp clock structure
> > + * @request: Desired resource to enable or disable
> > + * @on: Caller passes one to enable or zero to disable
> > + *
> > + * Enable (or disable) ancillary features of the PHC subsystem.
> > + * Currently, no ancillary features are supported.
> > + **/
> > +static int mlx4_en_phc_enable(struct ptp_clock_info __always_unused *ptp,
> > +                             struct ptp_clock_request __always_unused *request,
> > +                             int __always_unused on)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +static const struct ptp_clock_info mlx4_en_ptp_clock_info = {
> > +       .owner          = THIS_MODULE,
> > +       .max_adj        = 100000000,
> > +       .n_alarm        = 0,
> > +       .n_ext_ts       = 0,
> > +       .n_per_out      = 0,
> > +       .pps            = 0,
> > +       .adjfreq        = mlx4_en_phc_adjfreq,
> > +       .adjtime        = mlx4_en_phc_adjtime,
> > +       .gettime        = mlx4_en_phc_gettime,
> > +       .settime        = mlx4_en_phc_settime,
> > +       .enable         = mlx4_en_phc_enable,
> > +};
> > +
> >  void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
> >  {
> >         struct mlx4_dev *dev = mdev->dev;
> > +       unsigned long flags;
> >         u64 ns;
> >
> >         memset(&mdev->cycles, 0, sizeof(mdev->cycles));
> > @@ -127,9 +297,12 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
> >         mdev->cycles.shift = 14;
> >         mdev->cycles.mult =
> >                 clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
> > +       mdev->nominal_c_mult = mdev->cycles.mult;
> >
> > +       spin_lock_irqsave(&mdev->clock_lock, flags);
> >         timecounter_init(&mdev->clock, &mdev->cycles,
> >                          ktime_to_ns(ktime_get_real()));
> > +       spin_unlock_irqrestore(&mdev->clock_lock, flags);
> >
> >         /* Calculate period in seconds to call the overflow watchdog - to make
> >          * sure counter is checked at least once every wrap around.
> > @@ -137,15 +310,18 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
> >         ns = cyclecounter_cyc2ns(&mdev->cycles, mdev->cycles.mask);
> >         do_div(ns, NSEC_PER_SEC / 2 / HZ);
> >         mdev->overflow_period = ns;
> > -}
> >
> > -void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> > -{
> > -       bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> > -                                             mdev->overflow_period);
> > +       /* Configure the PHC */
> > +       mdev->ptp_clock_info = mlx4_en_ptp_clock_info;
> > +       snprintf(mdev->ptp_clock_info.name, 16, "mlx4 ptp");
> >
> > -       if (timeout) {
> > -               timecounter_read(&mdev->clock);
> > -               mdev->last_overflow_check = jiffies;
> > +       mdev->ptp_clock = ptp_clock_register(&mdev->ptp_clock_info,
> > +                                            &mdev->pdev->dev);
> > +       if (IS_ERR(mdev->ptp_clock)) {
> > +               mdev->ptp_clock = NULL;
> > +               mlx4_err(mdev, "ptp_clock_register failed\n");
> > +       } else {
> > +               mlx4_info(mdev, "registered PHC clock\n");
> >         }
> > +
> >  }
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> > index 0596f9f..3e8d336 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> > @@ -1193,6 +1193,9 @@ static int mlx4_en_get_ts_info(struct net_device *dev,
> >                 info->rx_filters =
> >                         (1 << HWTSTAMP_FILTER_NONE) |
> >                         (1 << HWTSTAMP_FILTER_ALL);
> > +
> > +               if (mdev->ptp_clock)
> > +                       info->phc_index = ptp_clock_index(mdev->ptp_clock);
> >         }
> >
> >         return ret;
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_main.c b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> > index 0d087b0..6268057 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> > @@ -196,6 +196,9 @@ static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
> >                 if (mdev->pndev[i])
> >                         mlx4_en_destroy_netdev(mdev->pndev[i]);
> >
> > +       if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
> > +               mlx4_en_remove_timestamp(mdev);
> > +
> >         flush_workqueue(mdev->workqueue);
> >         destroy_workqueue(mdev->workqueue);
> >         (void) mlx4_mr_free(dev, &mdev->mr);
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> > index f3758de..2b4083f 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> > +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> > @@ -45,6 +45,7 @@
> >  #include <linux/dcbnl.h>
> >  #endif
> >  #include <linux/cpu_rmap.h>
> > +#include <linux/ptp_clock_kernel.h>
> >
> >  #include <linux/mlx4/device.h>
> >  #include <linux/mlx4/qp.h>
> > @@ -373,10 +374,14 @@ struct mlx4_en_dev {
> >         u32                     priv_pdn;
> >         spinlock_t              uar_lock;
> >         u8                      mac_removed[MLX4_MAX_PORTS + 1];
> > +       spinlock_t              clock_lock;
> > +       u32                     nominal_c_mult;
> >         struct cyclecounter     cycles;
> >         struct timecounter      clock;
> >         unsigned long           last_overflow_check;
> >         unsigned long           overflow_period;
> > +       struct ptp_clock        *ptp_clock;
> > +       struct ptp_clock_info   ptp_clock_info;
> >  };
> >
> >
> > @@ -786,6 +791,7 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
> >                             struct skb_shared_hwtstamps *hwts,
> >                             u64 timestamp);
> >  void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev);
> > +void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev);
> >  int mlx4_en_timestamp_config(struct net_device *dev,
> >                              int tx_type,
> >                              int rx_filter);
> > --
> > 1.7.7.6
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Or Gerlitz]

On 17/12/2013 23:46, Shawn Bohrer wrote:
> v3: is code identical to v2.  I simply finished testing it with
> linuxptp, cleaned up the commit description a little and sent to davem.
so the problem you saw with V2 is understood && gone with V3?

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

On Wed, Dec 18, 2013 at 09:59:51AM +0200, Or Gerlitz wrote:
> On 17/12/2013 23:46, Shawn Bohrer wrote:
> >v3: is code identical to v2.  I simply finished testing it with
> >linuxptp, cleaned up the commit description a little and sent to davem.
> so the problem you saw with V2 is understood && gone with V3?

The main problem I saw with linuxptp is fixed with patch 2/2:

mlx4_en: Only cycle port if HW timestamp config changes

So yes, again v3 is code identical to v2, and from my testing appears
to work as designed.

--
Shawn

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Hadar Hen Zion]

On 12/17/2013 10:32 PM, Shawn Bohrer wrote:
> From: Shawn Bohrer <sbohrer@rgmadvisors.com>
>
> This adds a PHC to the mlx4_en driver.  The code is largely based off of
> the e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> very similar.
>
> This driver has been tested with both Documentation/ptp/testptp and the
> linuxptp project (http://linuxptp.sourceforge.net/) and appears to work
> on a Mellanox ConnectX-3 card.
>
> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
>   drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
>   drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
>   drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
>   4 files changed, 196 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> index fd64410..9b0d515 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> @@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
>   			    struct skb_shared_hwtstamps *hwts,
>   			    u64 timestamp)
>   {
> +	unsigned long flags;
>   	u64 nsec;
>
> +	spin_lock_irqsave(&mdev->clock_lock, flags);

1. Missing initialization for clock_lock
2. Adding spin lock in the data path reduce performance by 15% when HW 
timestamping is enabled. I did some testing and replacing 
spin_lock_irqsave with read/write_lock_irqsave prevents the  performance 
decrease.

Thanks,
Hadar

>   	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
>
>   	memset(hwts, 0, sizeof(struct skb_shared_hwtstamps));
>   	hwts->hwtstamp = ns_to_ktime(nsec);
>   }
>
> +/**
> + * mlx4_en_remove_timestamp - disable PTP device
> + * @mdev: board private structure
> + *
> + * Stop the PTP support.
> + **/
> +void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev)
> +{
> +	if (mdev->ptp_clock) {
> +		ptp_clock_unregister(mdev->ptp_clock);
> +		mdev->ptp_clock = NULL;
> +		mlx4_info(mdev, "removed PHC\n");
> +	}
> +}
> +
> +void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> +{
> +	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> +					      mdev->overflow_period);
> +	unsigned long flags;
> +
> +	if (timeout) {
> +		spin_lock_irqsave(&mdev->clock_lock, flags);
> +		timecounter_read(&mdev->clock);
> +		spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +		mdev->last_overflow_check = jiffies;
> +	}
> +}
> +
> +/**
> + * mlx4_en_phc_adjfreq - adjust the frequency of the hardware clock
> + * @ptp: ptp clock structure
> + * @delta: Desired frequency change in parts per billion
> + *
> + * Adjust the frequency of the PHC cycle counter by the indicated delta from
> + * the base frequency.
> + **/
> +static int mlx4_en_phc_adjfreq(struct ptp_clock_info *ptp, s32 delta)
> +{
> +	u64 adj;
> +	u32 diff, mult;
> +	int neg_adj = 0;
> +	unsigned long flags;
> +	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +						ptp_clock_info);
> +
> +	if (delta < 0) {
> +		neg_adj = 1;
> +		delta = -delta;
> +	}
> +	mult = mdev->nominal_c_mult;
> +	adj = mult;
> +	adj *= delta;
> +	diff = div_u64(adj, 1000000000ULL);
> +
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	timecounter_read(&mdev->clock);
> +	mdev->cycles.mult = neg_adj ? mult - diff : mult + diff;
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +	return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_adjtime - Shift the time of the hardware clock
> + * @ptp: ptp clock structure
> + * @delta: Desired change in nanoseconds
> + *
> + * Adjust the timer by resetting the timecounter structure.
> + **/
> +static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +						ptp_clock_info);
> +	unsigned long flags;
> +	s64 now;
> +
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	now = timecounter_read(&mdev->clock);
> +	now += delta;
> +	timecounter_init(&mdev->clock, &mdev->cycles, now);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +	return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_gettime - Reads the current time from the hardware clock
> + * @ptp: ptp clock structure
> + * @ts: timespec structure to hold the current time value
> + *
> + * Read the timecounter and return the correct value in ns after converting
> + * it into a struct timespec.
> + **/
> +static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
> +{
> +	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +						ptp_clock_info);
> +	unsigned long flags;
> +	u32 remainder;
> +	u64 ns;
> +
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	ns = timecounter_read(&mdev->clock);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +	ts->tv_sec = div_u64_rem(ns, NSEC_PER_SEC, &remainder);
> +	ts->tv_nsec = remainder;
> +
> +	return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_settime - Set the current time on the hardware clock
> + * @ptp: ptp clock structure
> + * @ts: timespec containing the new time for the cycle counter
> + *
> + * Reset the timecounter to use a new base value instead of the kernel
> + * wall timer value.
> + **/
> +static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
> +			       const struct timespec *ts)
> +{
> +	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +						ptp_clock_info);
> +	u64 ns = timespec_to_ns(ts);
> +	unsigned long flags;
> +
> +	/* reset the timecounter */
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	timecounter_init(&mdev->clock, &mdev->cycles, ns);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +	return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_enable - enable or disable an ancillary feature
> + * @ptp: ptp clock structure
> + * @request: Desired resource to enable or disable
> + * @on: Caller passes one to enable or zero to disable
> + *
> + * Enable (or disable) ancillary features of the PHC subsystem.
> + * Currently, no ancillary features are supported.
> + **/
> +static int mlx4_en_phc_enable(struct ptp_clock_info __always_unused *ptp,
> +			      struct ptp_clock_request __always_unused *request,
> +			      int __always_unused on)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct ptp_clock_info mlx4_en_ptp_clock_info = {
> +	.owner		= THIS_MODULE,
> +	.max_adj	= 100000000,
> +	.n_alarm	= 0,
> +	.n_ext_ts	= 0,
> +	.n_per_out	= 0,
> +	.pps		= 0,
> +	.adjfreq	= mlx4_en_phc_adjfreq,
> +	.adjtime	= mlx4_en_phc_adjtime,
> +	.gettime	= mlx4_en_phc_gettime,
> +	.settime	= mlx4_en_phc_settime,
> +	.enable		= mlx4_en_phc_enable,
> +};
> +
>   void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>   {
>   	struct mlx4_dev *dev = mdev->dev;
> +	unsigned long flags;
>   	u64 ns;
>
>   	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
> @@ -127,9 +297,12 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>   	mdev->cycles.shift = 14;
>   	mdev->cycles.mult =
>   		clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
> +	mdev->nominal_c_mult = mdev->cycles.mult;
>
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
>   	timecounter_init(&mdev->clock, &mdev->cycles,
>   			 ktime_to_ns(ktime_get_real()));
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
>
>   	/* Calculate period in seconds to call the overflow watchdog - to make
>   	 * sure counter is checked at least once every wrap around.
> @@ -137,15 +310,18 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>   	ns = cyclecounter_cyc2ns(&mdev->cycles, mdev->cycles.mask);
>   	do_div(ns, NSEC_PER_SEC / 2 / HZ);
>   	mdev->overflow_period = ns;
> -}
>
> -void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> -{
> -	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> -					      mdev->overflow_period);
> +	/* Configure the PHC */
> +	mdev->ptp_clock_info = mlx4_en_ptp_clock_info;
> +	snprintf(mdev->ptp_clock_info.name, 16, "mlx4 ptp");
>
> -	if (timeout) {
> -		timecounter_read(&mdev->clock);
> -		mdev->last_overflow_check = jiffies;
> +	mdev->ptp_clock = ptp_clock_register(&mdev->ptp_clock_info,
> +					     &mdev->pdev->dev);
> +	if (IS_ERR(mdev->ptp_clock)) {
> +		mdev->ptp_clock = NULL;
> +		mlx4_err(mdev, "ptp_clock_register failed\n");
> +	} else {
> +		mlx4_info(mdev, "registered PHC clock\n");
>   	}
> +
>   }
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index 0596f9f..3e8d336 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -1193,6 +1193,9 @@ static int mlx4_en_get_ts_info(struct net_device *dev,
>   		info->rx_filters =
>   			(1 << HWTSTAMP_FILTER_NONE) |
>   			(1 << HWTSTAMP_FILTER_ALL);
> +
> +		if (mdev->ptp_clock)
> +			info->phc_index = ptp_clock_index(mdev->ptp_clock);
>   	}
>
>   	return ret;
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_main.c b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> index 0d087b0..6268057 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> @@ -196,6 +196,9 @@ static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
>   		if (mdev->pndev[i])
>   			mlx4_en_destroy_netdev(mdev->pndev[i]);
>
> +	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
> +		mlx4_en_remove_timestamp(mdev);
> +
>   	flush_workqueue(mdev->workqueue);
>   	destroy_workqueue(mdev->workqueue);
>   	(void) mlx4_mr_free(dev, &mdev->mr);
> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> index f3758de..2b4083f 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> @@ -45,6 +45,7 @@
>   #include <linux/dcbnl.h>
>   #endif
>   #include <linux/cpu_rmap.h>
> +#include <linux/ptp_clock_kernel.h>
>
>   #include <linux/mlx4/device.h>
>   #include <linux/mlx4/qp.h>
> @@ -373,10 +374,14 @@ struct mlx4_en_dev {
>   	u32                     priv_pdn;
>   	spinlock_t              uar_lock;
>   	u8			mac_removed[MLX4_MAX_PORTS + 1];
> +	spinlock_t		clock_lock;
> +	u32			nominal_c_mult;
>   	struct cyclecounter	cycles;
>   	struct timecounter	clock;
>   	unsigned long		last_overflow_check;
>   	unsigned long		overflow_period;
> +	struct ptp_clock	*ptp_clock;
> +	struct ptp_clock_info	ptp_clock_info;
>   };
>
>
> @@ -786,6 +791,7 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
>   			    struct skb_shared_hwtstamps *hwts,
>   			    u64 timestamp);
>   void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev);
> +void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev);
>   int mlx4_en_timestamp_config(struct net_device *dev,
>   			     int tx_type,
>   			     int rx_filter);
>

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

On Sun, Dec 22, 2013 at 03:13:12PM +0200, Hadar Hen Zion wrote:
> On 12/17/2013 10:32 PM, Shawn Bohrer wrote:
> >From: Shawn Bohrer <sbohrer@rgmadvisors.com>
> >
> >This adds a PHC to the mlx4_en driver.  The code is largely based off of
> >the e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> >very similar.
> >
> >This driver has been tested with both Documentation/ptp/testptp and the
> >linuxptp project (http://linuxptp.sourceforge.net/) and appears to work
> >on a Mellanox ConnectX-3 card.
> >
> >Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> >---
> >  drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
> >  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
> >  drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
> >  drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
> >  4 files changed, 196 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> >index fd64410..9b0d515 100644
> >--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> >+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> >@@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
> >  			    struct skb_shared_hwtstamps *hwts,
> >  			    u64 timestamp)
> >  {
> >+	unsigned long flags;
> >  	u64 nsec;
> >
> >+	spin_lock_irqsave(&mdev->clock_lock, flags);
> 
> 1. Missing initialization for clock_lock
> 2. Adding spin lock in the data path reduce performance by 15% when
> HW timestamping is enabled. I did some testing and replacing
> spin_lock_irqsave with read/write_lock_irqsave prevents the
> performance decrease.

Thanks Hadar,

I'm testing this change now, and will resend when I'm done.  However,
I noticed the following in Documentation/spinlocks.txt

   NOTE! We are working hard to remove reader-writer spinlocks in most
   cases, so please don't add a new one without consensus.  (Instead, see
   Documentation/RCU/rcu.txt for complete information.)

So is there consensus for a rwlock here?

Thanks,
Shawn

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

On Mon, Dec 23, 2013 at 10:29:58AM -0600, Shawn Bohrer wrote:
> On Sun, Dec 22, 2013 at 03:13:12PM +0200, Hadar Hen Zion wrote:
> > On 12/17/2013 10:32 PM, Shawn Bohrer wrote:
> > >From: Shawn Bohrer <sbohrer@rgmadvisors.com>
> > >
> > >This adds a PHC to the mlx4_en driver.  The code is largely based off of
> > >the e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> > >very similar.
> > >
> > >This driver has been tested with both Documentation/ptp/testptp and the
> > >linuxptp project (http://linuxptp.sourceforge.net/) and appears to work
> > >on a Mellanox ConnectX-3 card.
> > >
> > >Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> > >---
> > >  drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
> > >  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
> > >  drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
> > >  drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
> > >  4 files changed, 196 insertions(+), 8 deletions(-)
> > >
> > >diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > >index fd64410..9b0d515 100644
> > >--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > >+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > >@@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
> > >  			    struct skb_shared_hwtstamps *hwts,
> > >  			    u64 timestamp)
> > >  {
> > >+	unsigned long flags;
> > >  	u64 nsec;
> > >
> > >+	spin_lock_irqsave(&mdev->clock_lock, flags);
> > 
> > 1. Missing initialization for clock_lock
> > 2. Adding spin lock in the data path reduce performance by 15% when
> > HW timestamping is enabled. I did some testing and replacing
> > spin_lock_irqsave with read/write_lock_irqsave prevents the
> > performance decrease.
> 
> Thanks Hadar,
> 
> I'm testing this change now, and will resend when I'm done.  However,
> I noticed the following in Documentation/spinlocks.txt
> 
>    NOTE! We are working hard to remove reader-writer spinlocks in most
>    cases, so please don't add a new one without consensus.  (Instead, see
>    Documentation/RCU/rcu.txt for complete information.)
> 
> So is there consensus for a rwlock here?

Also just to make sure I'm testing the correct thing.  These all need
to be write_lock_irqsave() except for the one in
mlx4_en_fill_hwtstamps() protecting timecounter_cyc2time() which can
be a read_lock_irqsave().  All of the other timecounter* calls write
to the timecounter including timecounter_read().  I'm assuming that is
what you tested and that should still eliminate the performance loss
since mlx4_en_fill_hwtstamps() should be the bottleneck.

Thanks,
Shawn

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Hadar Hen Zion]

On 12/23/2013 6:59 PM, Shawn Bohrer wrote:
> On Mon, Dec 23, 2013 at 10:29:58AM -0600, Shawn Bohrer wrote:
>> On Sun, Dec 22, 2013 at 03:13:12PM +0200, Hadar Hen Zion wrote:
>>> On 12/17/2013 10:32 PM, Shawn Bohrer wrote:
>>>> From: Shawn Bohrer <sbohrer@rgmadvisors.com>
>>>>
>>>> This adds a PHC to the mlx4_en driver.  The code is largely based off of
>>>> the e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
>>>> very similar.
>>>>
>>>> This driver has been tested with both Documentation/ptp/testptp and the
>>>> linuxptp project (http://linuxptp.sourceforge.net/) and appears to work
>>>> on a Mellanox ConnectX-3 card.
>>>>
>>>> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
>>>> ---
>>>>   drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
>>>>   drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
>>>>   drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
>>>>   drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
>>>>   4 files changed, 196 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
>>>> index fd64410..9b0d515 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
>>>> @@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
>>>>   			    struct skb_shared_hwtstamps *hwts,
>>>>   			    u64 timestamp)
>>>>   {
>>>> +	unsigned long flags;
>>>>   	u64 nsec;
>>>>
>>>> +	spin_lock_irqsave(&mdev->clock_lock, flags);
>>>
>>> 1. Missing initialization for clock_lock
>>> 2. Adding spin lock in the data path reduce performance by 15% when
>>> HW timestamping is enabled. I did some testing and replacing
>>> spin_lock_irqsave with read/write_lock_irqsave prevents the
>>> performance decrease.
>>
>> Thanks Hadar,
>>
>> I'm testing this change now, and will resend when I'm done.  However,
>> I noticed the following in Documentation/spinlocks.txt
>>
>>     NOTE! We are working hard to remove reader-writer spinlocks in most
>>     cases, so please don't add a new one without consensus.  (Instead, see
>>     Documentation/RCU/rcu.txt for complete information.)
>>
>> So is there consensus for a rwlock here?
>
> Also just to make sure I'm testing the correct thing.  These all need
> to be write_lock_irqsave() except for the one in
> mlx4_en_fill_hwtstamps() protecting timecounter_cyc2time() which can
> be a read_lock_irqsave().  All of the other timecounter* calls write
> to the timecounter including timecounter_read().  I'm assuming that is
> what you tested and that should still eliminate the performance loss
> since mlx4_en_fill_hwtstamps() should be the bottleneck.
>
> Thanks,
> Shawn
>
Yes, you were testing the correct thing.

But, after another check, I'm not sure we need any lock in 
mlx4_en_fill_hwtstamps() data path function.
Adding lock to mlx4_en_fill_hwtstamps() protects timecounter_cyc2time() 
which doesn't read hardware registers.
As you explained in your RFC mail:

 > In e1000e driver they protect the timecounter code with a spinlock
 > because the hardware reports the time in two 32bit registers.  The
 > Mellanox code looks similar.

The spin lock is needed when reading hardware registers.
My suggestion is to stay with spin locks in all the places protecting 
timecounter_read()/timecounter_init() and just remove the spin lock from 
timecounter_cyc2time()

Thanks,
Hadar

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Richard Cochran]

On Tue, Dec 24, 2013 at 05:38:35PM +0200, Hadar Hen Zion wrote:
> 
> The spin lock is needed when reading hardware registers.

Or when using a structure of fields that act together as one object.

> My suggestion is to stay with spin locks in all the places
> protecting timecounter_read()/timecounter_init() and just remove the
> spin lock from timecounter_cyc2time()

If you look at the implementation of timecounter_cyc2time, you will
see that locks are, in fact, needed.

See also the comments in clocksource.h.

Thanks,
Richard

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Richard Cochran]

On Sun, Dec 22, 2013 at 03:13:12PM +0200, Hadar Hen Zion wrote:

> 2. Adding spin lock in the data path reduce performance by 15% when
> HW timestamping is enabled. I did some testing and replacing
> spin_lock_irqsave with read/write_lock_irqsave prevents the
> performance decrease.

Why do the spin locks cause such a bottleneck?

Is there really that much lock contention in your test?

Your figure of 15% seems awfully high. How did you arrive at that
figure?

Thanks,
Richard

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Hadar Hen Zion]

On 12/23/2013 8:48 PM, Richard Cochran wrote:
> On Sun, Dec 22, 2013 at 03:13:12PM +0200, Hadar Hen Zion wrote:
>
>> 2. Adding spin lock in the data path reduce performance by 15% when
>> HW timestamping is enabled. I did some testing and replacing
>> spin_lock_irqsave with read/write_lock_irqsave prevents the
>> performance decrease.
>
> Why do the spin locks cause such a bottleneck?
>
> Is there really that much lock contention in your test?
>
> Your figure of 15% seems awfully high. How did you arrive at that
> figure?
>
> Thanks,
> Richard
>

The spin locks case such a bottleneck since I'm using multiple streams 
in my performance test. RSS mechanism scattered the streams between 
multiple RX rings while each RX ring is bound to a different cup.
The describe scenario cause lock contention between the different RX rings.

Performance drops from 37.8 Gbits/sec to 32.1 Gbits/sec when spin locks 
are added and goes back to 37.8 Gbits/sec when using read/write locks.

Thanks,
Hadar

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Richard Cochran]

On Tue, Dec 24, 2013 at 03:58:06PM +0200, Hadar Hen Zion wrote:
> 
> The spin locks case such a bottleneck since I'm using multiple
> streams in my performance test. RSS mechanism scattered the streams
> between multiple RX rings while each RX ring is bound to a different
> cup.
> The describe scenario cause lock contention between the different RX rings.
> 
> Performance drops from 37.8 Gbits/sec to 32.1 Gbits/sec when spin
> locks are added and goes back to 37.8 Gbits/sec when using
> read/write locks.

And you are time stamping every received packet?

Thanks,
Richard

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Richard Cochran]

On Wed, Dec 25, 2013 at 08:11:00AM -0600, Shawn Bohrer wrote:
> 
> The read/write lock appears to meet these needs but also appears to be
> frowned upon and RCU seems to be preferred.  I'm a bit ignorant when it
> comes to RCU, but can it be used in this case?

No, don't use RCU. It would not make sense here. Just use
reader/writer locks.

Thanks,
Richard

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Hadar Hen Zion]

On 12/25/2013 1:53 PM, Richard Cochran wrote:
> On Tue, Dec 24, 2013 at 03:58:06PM +0200, Hadar Hen Zion wrote:
>>
>> The spin locks case such a bottleneck since I'm using multiple
>> streams in my performance test. RSS mechanism scattered the streams
>> between multiple RX rings while each RX ring is bound to a different
>> cup.
>> The describe scenario cause lock contention between the different RX rings.
>>
>> Performance drops from 37.8 Gbits/sec to 32.1 Gbits/sec when spin
>> locks are added and goes back to 37.8 Gbits/sec when using
>> read/write locks.
>
> And you are time stamping every received packet?
>
> Thanks,
> Richard
>

Yes, current implementation time stamping every received packet.
I do think time stamping only packets from the relevant socket will make 
more sense but SIOCSHWTSTAMP ioctl provides only the device without any 
socket attributes.

Thanks,
Hadar

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Hadar Hen Zion]

On 12/26/2013 10:26 AM, Hadar Hen Zion wrote:
> On 12/25/2013 1:53 PM, Richard Cochran wrote:
>> On Tue, Dec 24, 2013 at 03:58:06PM +0200, Hadar Hen Zion wrote:
>>>
>>> The spin locks case such a bottleneck since I'm using multiple
>>> streams in my performance test. RSS mechanism scattered the streams
>>> between multiple RX rings while each RX ring is bound to a different
>>> cup.
>>> The describe scenario cause lock contention between the different RX
>>> rings.
>>>
>>> Performance drops from 37.8 Gbits/sec to 32.1 Gbits/sec when spin
>>> locks are added and goes back to 37.8 Gbits/sec when using
>>> read/write locks.
>>
>> And you are time stamping every received packet?
>>
>> Thanks,
>> Richard
>>
>
> Yes, current implementation time stamping every received packet.
> I do think time stamping only packets from the relevant socket will make
> more sense but SIOCSHWTSTAMP ioctl provides only the device without any
> socket attributes.
>
> Thanks,
> Hadar
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Is there a way to time stamping packets per socket?

Thanks,
Hadar

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Richard Cochran]

On Thu, Dec 26, 2013 at 10:33:26AM +0200, Hadar Hen Zion wrote:
> 
> Is there a way to time stamping packets per socket?

No, it is at the device level. Some devices support time stamping
different types of packets, like PTP events.

I looked again at your driver, and now it makes sense to me that the
spin lock is so costly in your tests. Every received packet calls
timecounter_cyc2time.  So you will need locking, and I think
reader/writer is perfectly fine for you to use.

Thanks,
Richard

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Hadar Hen-Zion]

On Thu, Dec 26, 2013 at 10:17:41AM +0100, Richard Cochran wrote:
> On Thu, Dec 26, 2013 at 10:33:26AM +0200, Hadar Hen Zion wrote:
> > 
> > Is there a way to time stamping packets per socket?
> 
> No, it is at the device level. Some devices support time stamping
> different types of packets, like PTP events.
> 
> I looked again at your driver, and now it makes sense to me that the
> spin lock is so costly in your tests. Every received packet calls
> timecounter_cyc2time.  So you will need locking, and I think
> reader/writer is perfectly fine for you to use.
> 
> Thanks,
> Richard
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


The below patch is my suggestion for using RCU.
According to my test there is no performace loss with RCU.
If reader/writer is deprecated, why not using RCU? 

Thanks,
Hadar

---
 drivers/net/ethernet/mellanox/mlx4/en_clock.c |   82 +++++++++++++++---------
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h  |    2 +-
 2 files changed, 52 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
index cc6a546..2fbe9aa 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
@@ -107,17 +107,54 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
 			    struct skb_shared_hwtstamps *hwts,
 			    u64 timestamp)
 {
-	unsigned long flags;
 	u64 nsec;
 
-	spin_lock_irqsave(&mdev->clock_lock, flags);
-	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
-	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+	rcu_read_lock();
+	nsec = timecounter_cyc2time(rcu_dereference(mdev->clock), timestamp);
+	rcu_read_unlock();
 
 	memset(hwts, 0, sizeof(struct skb_shared_hwtstamps));
 	hwts->hwtstamp = ns_to_ktime(nsec);
 }
 
+void mlx4_en_timecounter_init(struct mlx4_en_dev *mdev,
+			      u64 start_tstamp)
+{
+	struct timecounter *new_tc, *old_tc;
+	unsigned long flags;
+
+	new_tc = kmalloc(sizeof(*new_tc), GFP_KERNEL);
+
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	old_tc = mdev->clock;
+	*new_tc = *old_tc;
+	timecounter_init(new_tc, &mdev->cycles, start_tstamp);
+	rcu_assign_pointer(mdev->clock, new_tc);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+	synchronize_rcu();
+	kfree(old_tc);
+}
+
+u64 mlx4_en_timecounter_read(struct mlx4_en_dev *mdev)
+{
+	struct timecounter *new_tc, *old_tc;
+	unsigned long flags;
+	s64 now;
+
+	new_tc = kmalloc(sizeof(*new_tc), GFP_KERNEL);
+
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	old_tc = mdev->clock;
+	*new_tc = *old_tc;
+	now = timecounter_read(new_tc);
+	rcu_assign_pointer(mdev->clock, new_tc);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+	synchronize_rcu();
+	kfree(old_tc);
+
+	return now;
+}
+
 /**
  * mlx4_en_remove_timestamp - disable PTP device
  * @mdev: board private structure
@@ -129,6 +166,7 @@ void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev)
 	if (mdev->ptp_clock) {
 		ptp_clock_unregister(mdev->ptp_clock);
 		mdev->ptp_clock = NULL;
+		kfree(mdev->clock);
 		mlx4_info(mdev, "removed PHC\n");
 	}
 }
@@ -137,12 +175,9 @@ void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
 {
 	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
 					      mdev->overflow_period);
-	unsigned long flags;
 
 	if (timeout) {
-		spin_lock_irqsave(&mdev->clock_lock, flags);
-		timecounter_read(&mdev->clock);
-		spin_unlock_irqrestore(&mdev->clock_lock, flags);
+		mlx4_en_timecounter_read(mdev);
 		mdev->last_overflow_check = jiffies;
 	}
 }
@@ -160,7 +195,6 @@ static int mlx4_en_phc_adjfreq(struct ptp_clock_info *ptp, s32 delta)
 	u64 adj;
 	u32 diff, mult;
 	int neg_adj = 0;
-	unsigned long flags;
 	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
 						ptp_clock_info);
 
@@ -173,10 +207,8 @@ static int mlx4_en_phc_adjfreq(struct ptp_clock_info *ptp, s32 delta)
 	adj *= delta;
 	diff = div_u64(adj, 1000000000ULL);
 
-	spin_lock_irqsave(&mdev->clock_lock, flags);
-	timecounter_read(&mdev->clock);
+	mlx4_en_timecounter_read(mdev);
 	mdev->cycles.mult = neg_adj ? mult - diff : mult + diff;
-	spin_unlock_irqrestore(&mdev->clock_lock, flags);
 
 	return 0;
 }
@@ -192,14 +224,11 @@ static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
 {
 	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
 						ptp_clock_info);
-	unsigned long flags;
 	s64 now;
 
-	spin_lock_irqsave(&mdev->clock_lock, flags);
-	now = timecounter_read(&mdev->clock);
+	now = mlx4_en_timecounter_read(mdev);
 	now += delta;
-	timecounter_init(&mdev->clock, &mdev->cycles, now);
-	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+	mlx4_en_timecounter_init(mdev, now);
 
 	return 0;
 }
@@ -216,14 +245,10 @@ static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
 {
 	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
 						ptp_clock_info);
-	unsigned long flags;
 	u32 remainder;
 	u64 ns;
 
-	spin_lock_irqsave(&mdev->clock_lock, flags);
-	ns = timecounter_read(&mdev->clock);
-	spin_unlock_irqrestore(&mdev->clock_lock, flags);
-
+	ns = mlx4_en_timecounter_read(mdev);
 	ts->tv_sec = div_u64_rem(ns, NSEC_PER_SEC, &remainder);
 	ts->tv_nsec = remainder;
 
@@ -244,12 +269,9 @@ static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
 	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
 						ptp_clock_info);
 	u64 ns = timespec_to_ns(ts);
-	unsigned long flags;
 
 	/* reset the timecounter */
-	spin_lock_irqsave(&mdev->clock_lock, flags);
-	timecounter_init(&mdev->clock, &mdev->cycles, ns);
-	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+	mlx4_en_timecounter_init(mdev, ns);
 
 	return 0;
 }
@@ -287,9 +309,9 @@ static const struct ptp_clock_info mlx4_en_ptp_clock_info = {
 void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 {
 	struct mlx4_dev *dev = mdev->dev;
-	unsigned long flags;
 	u64 ns;
 
+	mdev->clock = kmalloc(sizeof(struct timecounter), GFP_KERNEL);
 	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
 	mdev->cycles.read = mlx4_en_read_clock;
 	mdev->cycles.mask = CLOCKSOURCE_MASK(48);
@@ -303,10 +325,7 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 		clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
 	mdev->nominal_c_mult = mdev->cycles.mult;
 
-	spin_lock_irqsave(&mdev->clock_lock, flags);
-	timecounter_init(&mdev->clock, &mdev->cycles,
-			 ktime_to_ns(ktime_get_real()));
-	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+	mlx4_en_timecounter_init(mdev, ktime_to_ns(ktime_get_real()));
 
 	/* Calculate period in seconds to call the overflow watchdog - to make
 	 * sure counter is checked at least once every wrap around.
@@ -323,6 +342,7 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 					     &mdev->pdev->dev);
 	if (IS_ERR(mdev->ptp_clock)) {
 		mdev->ptp_clock = NULL;
+		kfree(mdev->clock);
 		mlx4_err(mdev, "ptp_clock_register failed\n");
 	} else {
 		mlx4_info(mdev, "registered PHC clock\n");
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index e7733c1..7d1d460 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -379,7 +379,7 @@ struct mlx4_en_dev {
 	spinlock_t		clock_lock;
 	u32			nominal_c_mult;
 	struct cyclecounter	cycles;
-	struct timecounter	clock;
+	struct timecounter	*clock;
 	unsigned long		last_overflow_check;
 	unsigned long		overflow_period;
 	struct ptp_clock	*ptp_clock;
-- 
1.7.8.2

Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

[Richard Cochran]

On Thu, Dec 26, 2013 at 04:41:11PM +0200, Hadar Hen-Zion wrote:

> +u64 mlx4_en_timecounter_read(struct mlx4_en_dev *mdev)
> +{
> +	struct timecounter *new_tc, *old_tc;
> +	unsigned long flags;
> +	s64 now;
> +
> +	new_tc = kmalloc(sizeof(*new_tc), GFP_KERNEL);

So you want to call malloc/free on every clock read?
That is crazy.

> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	old_tc = mdev->clock;
> +	*new_tc = *old_tc;
> +	now = timecounter_read(new_tc);
> +	rcu_assign_pointer(mdev->clock, new_tc);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +	synchronize_rcu();
> +	kfree(old_tc);
> +
> +	return now;
> +}

Thanks,
Richard

[PATCH net-next 2/2] mlx4_en: Only cycle port if HW timestamp config changes

[Shawn Bohrer]

From: Shawn Bohrer <sbohrer@rgmadvisors.com>

If the hwtstamp_config matches what is currently set for the device then
simply return.  Without this change any program that tries to enable
hardware timestamps will cause the link to cycle even if hardware
timstamps were already enabled.

Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_clock.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
index 9b0d515..cc6a546 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
@@ -42,6 +42,10 @@ int mlx4_en_timestamp_config(struct net_device *dev, int tx_type, int rx_filter)
 	int port_up = 0;
 	int err = 0;
 
+	if (priv->hwtstamp_config.tx_type == tx_type &&
+	    priv->hwtstamp_config.rx_filter == rx_filter)
+		return 0;
+
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
 		port_up = 1;
-- 
1.7.7.6

[PATCH net-next 2/2] mlx4_en: Only cycle port if HW timestamp config changes

[Shawn Bohrer]

From: Shawn Bohrer <sbohrer@rgmadvisors.com>

If the hwtstamp_config matches what is currently set for the device then
simply return.  Without this change any program that tries to enable
hardware timestamps will cause the link to cycle even if hardware
timstamps were already enabled.

Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_clock.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
index 9b0d515..cc6a546 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
@@ -42,6 +42,10 @@ int mlx4_en_timestamp_config(struct net_device *dev, int tx_type, int rx_filter)
 	int port_up = 0;
 	int err = 0;
 
+	if (priv->hwtstamp_config.tx_type == tx_type &&
+	    priv->hwtstamp_config.rx_filter == rx_filter)
+		return 0;
+
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
 		port_up = 1;
-- 
1.7.7.6

Re: [PATCH net-next 2/2] mlx4_en: Only cycle port if HW timestamp config changes

[Shawn Bohrer]

On Tue, Dec 17, 2013 at 02:22:25PM -0600, Shawn Bohrer wrote:
> From: Shawn Bohrer <sbohrer@rgmadvisors.com>
> 
> If the hwtstamp_config matches what is currently set for the device then
> simply return.  Without this change any program that tries to enable
> hardware timestamps will cause the link to cycle even if hardware
> timstamps were already enabled.
> 
> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_clock.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> index 9b0d515..cc6a546 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> @@ -42,6 +42,10 @@ int mlx4_en_timestamp_config(struct net_device *dev, int tx_type, int rx_filter)
>  	int port_up = 0;
>  	int err = 0;
>  
> +	if (priv->hwtstamp_config.tx_type == tx_type &&
> +	    priv->hwtstamp_config.rx_filter == rx_filter)
> +		return 0;
> +
>  	mutex_lock(&mdev->state_lock);
>  	if (priv->port_up) {
>  		port_up = 1;
> -- 
> 1.7.7.6

Sorry had an email malfunction I'll resend this series again including
a cover and patch 1.

--
Shawn