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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131223165934.GB6810@lintop.rgmadvisors.com>
Date:	Mon, 23 Dec 2013 10:59:34 -0600
From:	Shawn Bohrer <shawn.bohrer@...il.com>
To:	Hadar Hen Zion <hadarh@...lanox.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Amir Vadai <amirv@...lanox.com>,
	Richard Cochran <richardcochran@...il.com>,
	netdev@...r.kernel.org, tomk@...advisors.com,
	Shawn Bohrer <sbohrer@...advisors.com>
Subject: Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock

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@...advisors.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@...advisors.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html