On 2018年09月10日 21:35, Willem de Bruijn wrote:
On Mon, Sep 10, 2018 at 2:01 AM Jason Wang <jasow...@redhat.com> wrote:
On 2018年09月10日 06:44, Willem de Bruijn wrote:
From: Willem de Bruijn <will...@google.com>
Implement ethtool .set_coalesce (-C) and .get_coalesce (-c) handlers.
Interrupt moderation is currently not supported, so these accept and
display the default settings of 0 usec and 1 frame.
Toggle tx napi through a bit in tx-frames. So as to not interfere
with possible future interrupt moderation, use bit 10, well outside
the reasonable range of real interrupt moderation values.
Changes are not atomic. The tx IRQ, napi BH and transmit path must
be quiesced when switching modes. Only allow changing this setting
when the device is down.
I cook a fixup, and it looks works in my setup:
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b320b6b14749..9181c3f2f832 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2204,10 +2204,17 @@ static int virtnet_set_coalesce(struct
net_device *dev,
return -EINVAL;
if (napi_weight ^ vi->sq[0].napi.weight) {
- if (dev->flags & IFF_UP)
- return -EBUSY;
- for (i = 0; i < vi->max_queue_pairs; i++)
+ for (i = 0; i < vi->max_queue_pairs; i++) {
+ struct netdev_queue *txq =
+ netdev_get_tx_queue(vi->dev, i);
+
+ virtnet_napi_tx_disable(&vi->sq[i].napi);
+ __netif_tx_lock_bh(txq);
vi->sq[i].napi.weight = napi_weight;
+ __netif_tx_unlock_bh(txq);
+ virtnet_napi_tx_enable(vi, vi->sq[i].vq,
+ &vi->sq[i].napi);
+ }
}
return 0;
Thanks! It passes my simple stress test, too. Which consists of two
concurrent loops, one toggling the ethtool option, another running
TCP_RR.
The only left case is the speculative tx polling in RX NAPI. I think we
don't need to care in this case since it was not a must for correctness.
As long as the txq lock is held that will be a noop, anyway. The other
concurrent action is skb_xmit_done. It looks correct to me, but need
to think about it a bit. The tricky transition is coming out of napi without
having >= 2 + MAX_SKB_FRAGS clean descriptors. If the queue is
stopped it may deadlock transmission in no-napi mode.
Yes, maybe we can enable tx queue when napi weight is zero in
virtnet_poll_tx(). Let me do more stress test on this.
Link: https://patchwork.ozlabs.org/patch/948149/
Suggested-by: Jason Wang <jasow...@redhat.com>
Signed-off-by: Willem de Bruijn <will...@google.com>
---
drivers/net/virtio_net.c | 52 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 765920905226..b320b6b14749 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -66,6 +66,8 @@ DECLARE_EWMA(pkt_len, 0, 64)
#define VIRTNET_DRIVER_VERSION "1.0.0"
+static const u32 ethtool_coalesce_napi_mask = (1UL << 10);
+
static const unsigned long guest_offloads[] = {
VIRTIO_NET_F_GUEST_TSO4,
VIRTIO_NET_F_GUEST_TSO6,
@@ -2181,6 +2183,54 @@ static int virtnet_get_link_ksettings(struct net_device
*dev,
return 0;
}
+static int virtnet_set_coalesce(struct net_device *dev,
+ struct ethtool_coalesce *ec)
+{
+ const struct ethtool_coalesce ec_default = {
+ .cmd = ETHTOOL_SCOALESCE,
+ .rx_max_coalesced_frames = 1,
I think rx part is no necessary.
The definition of ethtool_coalesce has:
"* It is illegal to set both usecs and max_frames to zero as this
* would cause interrupts to never be generated. To disable
* coalescing, set usecs = 0 and max_frames = 1."
I'd rather not diverge from this prescribed behavior unless there's
a strong reason.
I get this.
On the related point in the other thread:
Rethink about this, how about something like:
- UINT_MAX: no tx interrupt
- other value: tx interrupt with possible interrupt moderation
Okay, that will be simpler to configure.
I wonder maybe we can use ethtool_coalesce definition. E.g usecs = 0 &&
max_frame = 0 means no interrupt? Just a thought, I'm ok with either.
Thanks
