From: Sowmini Varadhan <sowmini.varadhan@oracle•com>
To: David Miller <davem@davemloft•net>
Cc: david.stevens@oracle•com, netdev@vger•kernel.org
Subject: Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data descriptor after short udelay()
Date: Fri, 5 Sep 2014 09:47:58 -0400 [thread overview]
Message-ID: <20140905134758.GB1256@oracle.com> (raw)
In-Reply-To: <20140904.223648.559995846094457545.davem@davemloft.net>
On (09/04/14 22:36), David Miller wrote:
>
> The memory barrier exists in order to make sure the cookies et al. are
> globally visible before the VIO_DESC_READY. We don't want stores to
> be reordered such that the VIO_DESC_READY is seen too early.
Ok, though David (dls) was just pointing out that a rmb() might
be missing in vnet_walk_rx_one() before checking for READY descriptor
> I'm having a hard time imagining that putting the wmb() afterwards
> would help, except by adding a new delay in a different place.
correct.
> I say that because the TX trigger is going to make a hypervisor call,
> and I bet that hypervisor trap syncs the store buffer of invoking cpu
> thread.
>
> Thinking further, another issue to consider is that the wmb() might no
> be necessary considering all of the above, because the remote entity
> should not look at this descriptor until the tx trigger occurs.
So, today the consumer already reads a series of descriptors based on
a single LDC start trigger already in vnet_walk_rx() - it doesnt
actually wait for an LDC "start" per descriptor.
Which is as it should be- the first "start" trigger of a burst
is just an async signal from producer to consumer that data is ready
for consumption.
A wmb() (and maybe a missing rmb()) should itself synchronize the critical
state more efficiently than an ldc exchange. The last bit
about VIO_DESC_READY may get flushed at some later point, and
the delay in patch 2/2 slightly helps work around that fudge factor,
but moving the wmb() before/after the VIO_DESC_READY (in my testing)
doesn't really make a difference to correctness - the consumer first
checks for VIO_DESC_READY, and by that time, the rest of the cookie
info should have been correctly sync'ed by the memory barrier.
If we get the memory-barriers right,
the trigger-per-vnet_start_xmit is superfluous, and only
serves to flood the ldc_channel (and make the env ripe for
!tx_has_space_for()).
(And a side-effect of this is that avoids severl other udelay() loops
that we'd have otherwise undergone as part of __vnet_tx_trigger(),
which is also a healthy thing to avoid)
> Anyways, if the hypervisor trap to signal the tx trigger does not
> flush the local cpu thread's store buffer, then yes moving the memory
> barrier might make sense.
As such, it's expensive and unnecessary to cripple ourselves down to
one LDC exchange per descriptor - the wmb() itself should ensure this
correctly in a cheaper way.
BTW, even if you set the READY state before (not after) the wmb()
there's still a small iperf-boost to be obtained by lingering
around in vnet_walk_rx() (as in patch 2/2)- that boost is from avoiding
what would otherwise be a ldc stop/start exchange between consumer
and producer.
But I'm not too attached to this boost (aka patch 2/2)-
benchmark-special code is always ugly.
--Sowmini
next prev parent reply other threads:[~2014-09-05 13:48 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-02 16:20 [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data descriptor after short udelay() Sowmini Varadhan
2014-09-02 16:27 ` David L Stevens
2014-09-02 16:32 ` Sowmini Varadhan
2014-09-05 5:36 ` David Miller
2014-09-05 13:47 ` Sowmini Varadhan [this message]
2014-09-06 21:02 ` Sowmini Varadhan
[not found] ` <20140907181510.GA23753@oracle.com>
2014-09-07 19:36 ` Raghuram Kothakota
2014-09-07 23:19 ` David Miller
2014-09-08 13:45 ` David L Stevens
2014-09-08 14:03 ` Sowmini Varadhan
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=20140905134758.GB1256@oracle.com \
--to=sowmini.varadhan@oracle$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=david.stevens@oracle$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/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