From: Hadar Hen Zion <hadarh@mellanox•com>
To: Shawn Bohrer <shawn.bohrer@gmail•com>
Cc: "David S. Miller" <davem@davemloft•net>,
Or Gerlitz <ogerlitz@mellanox•com>,
Amir Vadai <amirv@mellanox•com>,
Richard Cochran <richardcochran@gmail•com>,
<netdev@vger•kernel.org>, <tomk@rgmadvisors•com>,
Shawn Bohrer <sbohrer@rgmadvisors•com>
Subject: Re: [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock
Date: Tue, 24 Dec 2013 17:38:35 +0200 [thread overview]
Message-ID: <52B9AA7B.8090401@mellanox.com> (raw)
In-Reply-To: <20131223165934.GB6810@lintop.rgmadvisors.com>
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
next prev parent reply other threads:[~2013-12-24 15:38 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-17 20:32 [PATCH net-next 0/2] mlx4_en: Add PTP support Shawn Bohrer
2013-12-17 20:32 ` [PATCH net-next 1/2] mlx4_en: Add PTP hardware clock Shawn Bohrer
2013-12-17 21:04 ` Or Gerlitz
2013-12-17 21:46 ` Shawn Bohrer
2013-12-18 7:59 ` Or Gerlitz
2013-12-18 15:03 ` Shawn Bohrer
2013-12-22 13:13 ` Hadar Hen Zion
2013-12-23 16:29 ` Shawn Bohrer
2013-12-23 16:59 ` Shawn Bohrer
2013-12-24 15:38 ` Hadar Hen Zion [this message]
2013-12-25 11:49 ` Richard Cochran
2013-12-23 18:48 ` Richard Cochran
2013-12-24 13:58 ` Hadar Hen Zion
2013-12-25 11:53 ` Richard Cochran
[not found] ` <CAKBbMu33zXkG4+S1kP6zB+4iKMNNoy=XVBtoqZZEkqurRsgvQw@mail.gmail.com>
2013-12-25 17:02 ` Richard Cochran
2013-12-26 8:26 ` Hadar Hen Zion
2013-12-26 8:33 ` Hadar Hen Zion
2013-12-26 9:17 ` Richard Cochran
2013-12-26 14:41 ` Hadar Hen-Zion
2013-12-26 14:49 ` Richard Cochran
2013-12-17 20:32 ` [PATCH net-next 2/2] mlx4_en: Only cycle port if HW timestamp config changes Shawn Bohrer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52B9AA7B.8090401@mellanox.com \
--to=hadarh@mellanox$(echo .)com \
--cc=amirv@mellanox$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=ogerlitz@mellanox$(echo .)com \
--cc=richardcochran@gmail$(echo .)com \
--cc=sbohrer@rgmadvisors$(echo .)com \
--cc=shawn.bohrer@gmail$(echo .)com \
--cc=tomk@rgmadvisors$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox