[PATCH net-next] virtio_net: ethtool tx napi configuration

[PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

From: Willem de Bruijn <willemb@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.

Link: https://patchwork.ozlabs.org/patch/948149/
Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Willem de Bruijn <willemb@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,
+		.tx_max_coalesced_frames = 1,
+	};
+	struct virtnet_info *vi = netdev_priv(dev);
+	int i, napi_weight = 0;
+
+	if (ec->tx_max_coalesced_frames & ethtool_coalesce_napi_mask) {
+		ec->tx_max_coalesced_frames &= ~ethtool_coalesce_napi_mask;
+		napi_weight = NAPI_POLL_WEIGHT;
+	}
+
+	/* disallow changes to fields not explicitly tested above */
+	if (memcmp(ec, &ec_default, sizeof(ec_default)))
+		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++)
+			vi->sq[i].napi.weight = napi_weight;
+	}
+
+	return 0;
+}
+
+static int virtnet_get_coalesce(struct net_device *dev,
+				struct ethtool_coalesce *ec)
+{
+	const struct ethtool_coalesce ec_default = {
+		.cmd = ETHTOOL_GCOALESCE,
+		.rx_max_coalesced_frames = 1,
+		.tx_max_coalesced_frames = 1,
+	};
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	memcpy(ec, &ec_default, sizeof(ec_default));
+
+	if (vi->sq[0].napi.weight)
+		ec->tx_max_coalesced_frames |= ethtool_coalesce_napi_mask;
+
+	return 0;
+}
+
 static void virtnet_init_settings(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -2219,6 +2269,8 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
 	.get_ts_info = ethtool_op_get_ts_info,
 	.get_link_ksettings = virtnet_get_link_ksettings,
 	.set_link_ksettings = virtnet_set_link_ksettings,
+	.set_coalesce = virtnet_set_coalesce,
+	.get_coalesce = virtnet_get_coalesce,
 };
 
 static void virtnet_freeze_down(struct virtio_device *vdev)
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月10日 06:44, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@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;

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.

>
> Link: https://patchwork.ozlabs.org/patch/948149/
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Willem de Bruijn <willemb@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.

Thanks

> +		.tx_max_coalesced_frames = 1,
> +	};
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	int i, napi_weight = 0;
> +
> +	if (ec->tx_max_coalesced_frames & ethtool_coalesce_napi_mask) {
> +		ec->tx_max_coalesced_frames &= ~ethtool_coalesce_napi_mask;
> +		napi_weight = NAPI_POLL_WEIGHT;
> +	}
> +
> +	/* disallow changes to fields not explicitly tested above */
> +	if (memcmp(ec, &ec_default, sizeof(ec_default)))
> +		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++)
> +			vi->sq[i].napi.weight = napi_weight;
> +	}
> +
> +	return 0;
> +}
> +
> +static int virtnet_get_coalesce(struct net_device *dev,
> +				struct ethtool_coalesce *ec)
> +{
> +	const struct ethtool_coalesce ec_default = {
> +		.cmd = ETHTOOL_GCOALESCE,
> +		.rx_max_coalesced_frames = 1,
> +		.tx_max_coalesced_frames = 1,
> +	};
> +	struct virtnet_info *vi = netdev_priv(dev);
> +
> +	memcpy(ec, &ec_default, sizeof(ec_default));
> +
> +	if (vi->sq[0].napi.weight)
> +		ec->tx_max_coalesced_frames |= ethtool_coalesce_napi_mask;
> +
> +	return 0;
> +}
> +
>   static void virtnet_init_settings(struct net_device *dev)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
> @@ -2219,6 +2269,8 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
>   	.get_ts_info = ethtool_op_get_ts_info,
>   	.get_link_ksettings = virtnet_get_link_ksettings,
>   	.set_link_ksettings = virtnet_set_link_ksettings,
> +	.set_coalesce = virtnet_set_coalesce,
> +	.get_coalesce = virtnet_get_coalesce,
>   };
>   
>   static void virtnet_freeze_down(struct virtio_device *vdev)

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Mon, Sep 10, 2018 at 2:01 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年09月10日 06:44, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@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.

> >
> > Link: https://patchwork.ozlabs.org/patch/948149/
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Willem de Bruijn <willemb@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.

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.

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月10日 21:35, Willem de Bruijn wrote:
> On Mon, Sep 10, 2018 at 2:01 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年09月10日 06:44, Willem de Bruijn wrote:
>>> From: Willem de Bruijn <willemb@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 <jasowang@redhat.com>
>>> Signed-off-by: Willem de Bruijn <willemb@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

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

> >> 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().

Yes, that precaution should resolve that edge case.

> Let me do more stress test on this.
>
> >
> >>> Link: https://patchwork.ozlabs.org/patch/948149/
> >>> Suggested-by: Jason Wang <jasowang@redhat.com>
> >>> Signed-off-by: Willem de Bruijn <willemb@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.

Come to think of it, max_frames 0 is no worse than max_frames
UINT_MAX. Sure, let's do that.

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月11日 09:14, Willem de Bruijn wrote:
>>>> 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().
> Yes, that precaution should resolve that edge case.
>

I've done a stress test and it passes. The test contains:

- vm with 2 queues
- a bash script to enable and disable tx napi
- two netperf UDP_STREAM sessions to send small packets

Thanks

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Tue, Sep 11, 2018 at 11:35 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年09月11日 09:14, Willem de Bruijn wrote:
> >>>> 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().
> > Yes, that precaution should resolve that edge case.
> >
>
> I've done a stress test and it passes. The test contains:
>
> - vm with 2 queues
> - a bash script to enable and disable tx napi
> - two netperf UDP_STREAM sessions to send small packets

Great. That matches my results. Do you want to send the v2?

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月12日 21:43, Willem de Bruijn wrote:
> On Tue, Sep 11, 2018 at 11:35 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年09月11日 09:14, Willem de Bruijn wrote:
>>>>>> 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().
>>> Yes, that precaution should resolve that edge case.
>>>
>> I've done a stress test and it passes. The test contains:
>>
>> - vm with 2 queues
>> - a bash script to enable and disable tx napi
>> - two netperf UDP_STREAM sessions to send small packets
> Great. That matches my results. Do you want to send the v2?

Some mails were blocked so I do not receive some replies in time. So I 
post a V2 (but as you've pointed out, it's buggy).

Thanks

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Florian Fainelli]

On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@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.

Humm, would not a private ethtool flag to switch TX NAPI on/off be more 
appropriate rather than use the coalescing configuration API here?

> 
> Link: https://patchwork.ozlabs.org/patch/948149/
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Willem de Bruijn <willemb@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,
> +		.tx_max_coalesced_frames = 1,
> +	};
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	int i, napi_weight = 0;
> +
> +	if (ec->tx_max_coalesced_frames & ethtool_coalesce_napi_mask) {
> +		ec->tx_max_coalesced_frames &= ~ethtool_coalesce_napi_mask;
> +		napi_weight = NAPI_POLL_WEIGHT;
> +	}
> +
> +	/* disallow changes to fields not explicitly tested above */
> +	if (memcmp(ec, &ec_default, sizeof(ec_default)))
> +		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++)
> +			vi->sq[i].napi.weight = napi_weight;
> +	}
> +
> +	return 0;
> +}
> +
> +static int virtnet_get_coalesce(struct net_device *dev,
> +				struct ethtool_coalesce *ec)
> +{
> +	const struct ethtool_coalesce ec_default = {
> +		.cmd = ETHTOOL_GCOALESCE,
> +		.rx_max_coalesced_frames = 1,
> +		.tx_max_coalesced_frames = 1,
> +	};
> +	struct virtnet_info *vi = netdev_priv(dev);
> +
> +	memcpy(ec, &ec_default, sizeof(ec_default));
> +
> +	if (vi->sq[0].napi.weight)
> +		ec->tx_max_coalesced_frames |= ethtool_coalesce_napi_mask;
> +
> +	return 0;
> +}
> +
>   static void virtnet_init_settings(struct net_device *dev)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
> @@ -2219,6 +2269,8 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
>   	.get_ts_info = ethtool_op_get_ts_info,
>   	.get_link_ksettings = virtnet_get_link_ksettings,
>   	.set_link_ksettings = virtnet_set_link_ksettings,
> +	.set_coalesce = virtnet_set_coalesce,
> +	.get_coalesce = virtnet_get_coalesce,
>   };
>   
>   static void virtnet_freeze_down(struct virtio_device *vdev)
> 

-- 
Florian

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@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.
>
> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
> appropriate rather than use the coalescing configuration API here?

What do you mean by private ethtool flag? A new field in ethtool
--features (-k)?

Configurable napi-tx is not a common feature across devices. We really
want virtio-net to also just convert to napi-tx as default, but need a
way to gradually convert with application opt-out if some workloads
see regressions. There is prior art in interpreting coalesce values as
more than a direct mapping to usec. The e1000 is one example.

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Florian Fainelli]

On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
>>> From: Willem de Bruijn <willemb@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.
>>
>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
>> appropriate rather than use the coalescing configuration API here?
> 
> What do you mean by private ethtool flag? A new field in ethtool
> --features (-k)?

I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then 
ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that 
private flag. mlx5 has a number of privates flags for instance.

> 
> Configurable napi-tx is not a common feature across devices. We really
> want virtio-net to also just convert to napi-tx as default, but need a
> way to gradually convert with application opt-out if some workloads
> see regressions.

The rationale makes sense, no questions about it.

> There is prior art in interpreting coalesce values as
> more than a direct mapping to usec. The e1000 is one example.
>

Looked at both e1000 and e1000e and they both have a similar programming 
of the HW's interrupt target rate register, which is relevant to 
interrupt coalescing, what part of these drivers do you see as doing 
something not quite coalescing related?
-- 
Florian

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
> > On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >>
> >>
> >> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
> >>> From: Willem de Bruijn <willemb@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.
> >>
> >> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
> >> appropriate rather than use the coalescing configuration API here?
> >
> > What do you mean by private ethtool flag? A new field in ethtool
> > --features (-k)?
>
> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
> private flag. mlx5 has a number of privates flags for instance.

Interesting, thanks! I was not at all aware of those ethtool flags.
Am having a look. It definitely looks promising.

> > Configurable napi-tx is not a common feature across devices. We really
> > want virtio-net to also just convert to napi-tx as default, but need a
> > way to gradually convert with application opt-out if some workloads
> > see regressions.
>
> The rationale makes sense, no questions about it.
>
> > There is prior art in interpreting coalesce values as
> > more than a direct mapping to usec. The e1000 is one example.
> >
>
> Looked at both e1000 and e1000e and they both have a similar programming
> of the HW's interrupt target rate register, which is relevant to
> interrupt coalescing, what part of these drivers do you see as doing
> something not quite coalescing related?

It's all coalescing related, for sure. e1000_set_coalesce just does not
translate the tx-usecs values into microsecond latency directly.

It modifies both the interrupt throttle rate adapter->itr and interrupt mode
adapter->itr_setting, which are initially set in e1000_check_options from
module param InterruptThrottleRate.

Value 0 disables interrupt moderation. 1 and 3 program a dynamic mode.
2 is an illegal value as is 5..9. 10..10000 converts from usec to interrupt
rate/sec.

I took tx-napi to be a similar interrupt related option as, say, dynamic
conservative mode interrupt moderation.

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> >
> >
> > On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
> > > On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > >>
> > >>
> > >>
> > >> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
> > >>> From: Willem de Bruijn <willemb@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.
> > >>
> > >> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
> > >> appropriate rather than use the coalescing configuration API here?
> > >
> > > What do you mean by private ethtool flag? A new field in ethtool
> > > --features (-k)?
> >
> > I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
> > ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
> > private flag. mlx5 has a number of privates flags for instance.
>
> Interesting, thanks! I was not at all aware of those ethtool flags.
> Am having a look. It definitely looks promising.

Okay, I made that change. That is indeed much cleaner, thanks.
Let me send the patch, initially as RFC.

I've observed one issue where if we toggle the flag before bringing
up the device, it hits a kernel BUG at include/linux/netdevice.h:515

        BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月13日 07:27, Willem de Bruijn wrote:
> On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>
>>>
>>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
>>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>
>>>>>
>>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
>>>>>> From: Willem de Bruijn <willemb@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.
>>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
>>>>> appropriate rather than use the coalescing configuration API here?
>>>> What do you mean by private ethtool flag? A new field in ethtool
>>>> --features (-k)?
>>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
>>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
>>> private flag. mlx5 has a number of privates flags for instance.
>> Interesting, thanks! I was not at all aware of those ethtool flags.
>> Am having a look. It definitely looks promising.
> Okay, I made that change. That is indeed much cleaner, thanks.
> Let me send the patch, initially as RFC.
>
> I've observed one issue where if we toggle the flag before bringing
> up the device, it hits a kernel BUG at include/linux/netdevice.h:515
>
>          BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));

This reminds me that we need to check netif_running() before trying to 
enable and disable tx napi in ethtool_set_coalesce().

Thanks

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Thu, Sep 13, 2018 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年09月13日 07:27, Willem de Bruijn wrote:
> > On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> >> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>
> >>>
> >>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
> >>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>>
> >>>>>
> >>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
> >>>>>> From: Willem de Bruijn <willemb@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.
> >>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
> >>>>> appropriate rather than use the coalescing configuration API here?
> >>>> What do you mean by private ethtool flag? A new field in ethtool
> >>>> --features (-k)?
> >>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
> >>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
> >>> private flag. mlx5 has a number of privates flags for instance.
> >> Interesting, thanks! I was not at all aware of those ethtool flags.
> >> Am having a look. It definitely looks promising.
> > Okay, I made that change. That is indeed much cleaner, thanks.
> > Let me send the patch, initially as RFC.
> >
> > I've observed one issue where if we toggle the flag before bringing
> > up the device, it hits a kernel BUG at include/linux/netdevice.h:515
> >
> >          BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
>
> This reminds me that we need to check netif_running() before trying to
> enable and disable tx napi in ethtool_set_coalesce().

The first iteration of my patch checked IFF_UP and effectively
only allowed the change when not running. What do you mean
by need to check?

And to respond to the other follow-up notes at once:

> Consider we may have interrupt moderation in the future, I tend to use
> set_coalesce. Otherwise we may need two steps to enable moderation:
>
> - tx-napi on
> - set_coalesce

FWIW, I don't care strongly whether we do this through coalesce or priv_flags.

>> +                     if (!napi_weight)
>> +                             virtqueue_enable_cb(vi->sq[i].vq);
>
> I don't get why we need to disable enable cb here.

To avoid entering no-napi mode with too few descriptors to
make progress and no way to get out of that state. This is a
pretty crude attempt at handling that, admittedly.

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月13日 22:58, Willem de Bruijn wrote:
> On Thu, Sep 13, 2018 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年09月13日 07:27, Willem de Bruijn wrote:
>>> On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
>>> <willemdebruijn.kernel@gmail.com> wrote:
>>>> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>
>>>>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
>>>>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>>
>>>>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
>>>>>>>> From: Willem de Bruijn <willemb@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.
>>>>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
>>>>>>> appropriate rather than use the coalescing configuration API here?
>>>>>> What do you mean by private ethtool flag? A new field in ethtool
>>>>>> --features (-k)?
>>>>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
>>>>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
>>>>> private flag. mlx5 has a number of privates flags for instance.
>>>> Interesting, thanks! I was not at all aware of those ethtool flags.
>>>> Am having a look. It definitely looks promising.
>>> Okay, I made that change. That is indeed much cleaner, thanks.
>>> Let me send the patch, initially as RFC.
>>>
>>> I've observed one issue where if we toggle the flag before bringing
>>> up the device, it hits a kernel BUG at include/linux/netdevice.h:515
>>>
>>>           BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
>> This reminds me that we need to check netif_running() before trying to
>> enable and disable tx napi in ethtool_set_coalesce().
> The first iteration of my patch checked IFF_UP and effectively
> only allowed the change when not running. What do you mean
> by need to check?

I mean if device is not up, there's no need to toggle napi state and tx 
lock.

>
> And to respond to the other follow-up notes at once:
>
>> Consider we may have interrupt moderation in the future, I tend to use
>> set_coalesce. Otherwise we may need two steps to enable moderation:
>>
>> - tx-napi on
>> - set_coalesce
> FWIW, I don't care strongly whether we do this through coalesce or priv_flags.

Ok.

>>> +                     if (!napi_weight)
>>> +                             virtqueue_enable_cb(vi->sq[i].vq);
>> I don't get why we need to disable enable cb here.
> To avoid entering no-napi mode with too few descriptors to
> make progress and no way to get out of that state. This is a
> pretty crude attempt at handling that, admittedly.

But in this case, we will call enable_cb_delayed() and we will finally 
get a interrupt?

Thanks

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Thu, Sep 13, 2018 at 11:27 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年09月13日 22:58, Willem de Bruijn wrote:
> > On Thu, Sep 13, 2018 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >>
> >> On 2018年09月13日 07:27, Willem de Bruijn wrote:
> >>> On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
> >>> <willemdebruijn.kernel@gmail.com> wrote:
> >>>> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>>
> >>>>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
> >>>>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>>>>
> >>>>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
> >>>>>>>> From: Willem de Bruijn <willemb@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.
> >>>>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
> >>>>>>> appropriate rather than use the coalescing configuration API here?
> >>>>>> What do you mean by private ethtool flag? A new field in ethtool
> >>>>>> --features (-k)?
> >>>>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
> >>>>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
> >>>>> private flag. mlx5 has a number of privates flags for instance.
> >>>> Interesting, thanks! I was not at all aware of those ethtool flags.
> >>>> Am having a look. It definitely looks promising.
> >>> Okay, I made that change. That is indeed much cleaner, thanks.
> >>> Let me send the patch, initially as RFC.
> >>>
> >>> I've observed one issue where if we toggle the flag before bringing
> >>> up the device, it hits a kernel BUG at include/linux/netdevice.h:515
> >>>
> >>>           BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
> >> This reminds me that we need to check netif_running() before trying to
> >> enable and disable tx napi in ethtool_set_coalesce().
> > The first iteration of my patch checked IFF_UP and effectively
> > only allowed the change when not running. What do you mean
> > by need to check?
>
> I mean if device is not up, there's no need to toggle napi state and tx
> lock.
>
> >
> > And to respond to the other follow-up notes at once:
> >
> >> Consider we may have interrupt moderation in the future, I tend to use
> >> set_coalesce. Otherwise we may need two steps to enable moderation:
> >>
> >> - tx-napi on
> >> - set_coalesce
> > FWIW, I don't care strongly whether we do this through coalesce or priv_flags.
>
> Ok.

Since you prefer coalesce, let's go with that (and a revision of your
latest patch).

>
> >>> +                     if (!napi_weight)
> >>> +                             virtqueue_enable_cb(vi->sq[i].vq);
> >> I don't get why we need to disable enable cb here.
> > To avoid entering no-napi mode with too few descriptors to
> > make progress and no way to get out of that state. This is a
> > pretty crude attempt at handling that, admittedly.
>
> But in this case, we will call enable_cb_delayed() and we will finally
> get a interrupt?

Right. It's a bit of a roundabout way to ensure that
netif_tx_wake_queue and thus eventually free_old_xmit_skbs are called.
It might make more sense to just wake the device without going through
an interrupt.

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月14日 11:40, Willem de Bruijn wrote:
> On Thu, Sep 13, 2018 at 11:27 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年09月13日 22:58, Willem de Bruijn wrote:
>>> On Thu, Sep 13, 2018 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>> On 2018年09月13日 07:27, Willem de Bruijn wrote:
>>>>> On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
>>>>> <willemdebruijn.kernel@gmail.com> wrote:
>>>>>> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
>>>>>>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
>>>>>>>>>> From: Willem de Bruijn <willemb@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.
>>>>>>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
>>>>>>>>> appropriate rather than use the coalescing configuration API here?
>>>>>>>> What do you mean by private ethtool flag? A new field in ethtool
>>>>>>>> --features (-k)?
>>>>>>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
>>>>>>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
>>>>>>> private flag. mlx5 has a number of privates flags for instance.
>>>>>> Interesting, thanks! I was not at all aware of those ethtool flags.
>>>>>> Am having a look. It definitely looks promising.
>>>>> Okay, I made that change. That is indeed much cleaner, thanks.
>>>>> Let me send the patch, initially as RFC.
>>>>>
>>>>> I've observed one issue where if we toggle the flag before bringing
>>>>> up the device, it hits a kernel BUG at include/linux/netdevice.h:515
>>>>>
>>>>>            BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
>>>> This reminds me that we need to check netif_running() before trying to
>>>> enable and disable tx napi in ethtool_set_coalesce().
>>> The first iteration of my patch checked IFF_UP and effectively
>>> only allowed the change when not running. What do you mean
>>> by need to check?
>> I mean if device is not up, there's no need to toggle napi state and tx
>> lock.
>>
>>> And to respond to the other follow-up notes at once:
>>>
>>>> Consider we may have interrupt moderation in the future, I tend to use
>>>> set_coalesce. Otherwise we may need two steps to enable moderation:
>>>>
>>>> - tx-napi on
>>>> - set_coalesce
>>> FWIW, I don't care strongly whether we do this through coalesce or priv_flags.
>> Ok.
> Since you prefer coalesce, let's go with that (and a revision of your
> latest patch).

Good to know this.

>>>>> +                     if (!napi_weight)
>>>>> +                             virtqueue_enable_cb(vi->sq[i].vq);
>>>> I don't get why we need to disable enable cb here.
>>> To avoid entering no-napi mode with too few descriptors to
>>> make progress and no way to get out of that state. This is a
>>> pretty crude attempt at handling that, admittedly.
>> But in this case, we will call enable_cb_delayed() and we will finally
>> get a interrupt?
> Right. It's a bit of a roundabout way to ensure that
> netif_tx_wake_queue and thus eventually free_old_xmit_skbs are called.
> It might make more sense to just wake the device without going through
> an interrupt.

I'm not sure I get this. If we don't enable tx napi, we tend to delay TX 
interrupt if we found the ring is about to full to avoid interrupt 
storm, so we're probably ok in this case.

Thanks

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Thu, Sep 13, 2018 at 11:53 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年09月14日 11:40, Willem de Bruijn wrote:
> > On Thu, Sep 13, 2018 at 11:27 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >>
> >> On 2018年09月13日 22:58, Willem de Bruijn wrote:
> >>> On Thu, Sep 13, 2018 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>>
> >>>> On 2018年09月13日 07:27, Willem de Bruijn wrote:
> >>>>> On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
> >>>>> <willemdebruijn.kernel@gmail.com> wrote:
> >>>>>> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>>>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
> >>>>>>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>>>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
> >>>>>>>>>> From: Willem de Bruijn <willemb@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.
> >>>>>>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
> >>>>>>>>> appropriate rather than use the coalescing configuration API here?
> >>>>>>>> What do you mean by private ethtool flag? A new field in ethtool
> >>>>>>>> --features (-k)?
> >>>>>>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
> >>>>>>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
> >>>>>>> private flag. mlx5 has a number of privates flags for instance.
> >>>>>> Interesting, thanks! I was not at all aware of those ethtool flags.
> >>>>>> Am having a look. It definitely looks promising.
> >>>>> Okay, I made that change. That is indeed much cleaner, thanks.
> >>>>> Let me send the patch, initially as RFC.
> >>>>>
> >>>>> I've observed one issue where if we toggle the flag before bringing
> >>>>> up the device, it hits a kernel BUG at include/linux/netdevice.h:515
> >>>>>
> >>>>>            BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
> >>>> This reminds me that we need to check netif_running() before trying to
> >>>> enable and disable tx napi in ethtool_set_coalesce().
> >>> The first iteration of my patch checked IFF_UP and effectively
> >>> only allowed the change when not running. What do you mean
> >>> by need to check?
> >> I mean if device is not up, there's no need to toggle napi state and tx
> >> lock.
> >>
> >>> And to respond to the other follow-up notes at once:
> >>>
> >>>> Consider we may have interrupt moderation in the future, I tend to use
> >>>> set_coalesce. Otherwise we may need two steps to enable moderation:
> >>>>
> >>>> - tx-napi on
> >>>> - set_coalesce
> >>> FWIW, I don't care strongly whether we do this through coalesce or priv_flags.
> >> Ok.
> > Since you prefer coalesce, let's go with that (and a revision of your
> > latest patch).
>
> Good to know this.
>
> >>>>> +                     if (!napi_weight)
> >>>>> +                             virtqueue_enable_cb(vi->sq[i].vq);
> >>>> I don't get why we need to disable enable cb here.
> >>> To avoid entering no-napi mode with too few descriptors to
> >>> make progress and no way to get out of that state. This is a
> >>> pretty crude attempt at handling that, admittedly.
> >> But in this case, we will call enable_cb_delayed() and we will finally
> >> get a interrupt?
> > Right. It's a bit of a roundabout way to ensure that
> > netif_tx_wake_queue and thus eventually free_old_xmit_skbs are called.
> > It might make more sense to just wake the device without going through
> > an interrupt.
>
> I'm not sure I get this. If we don't enable tx napi, we tend to delay TX
> interrupt if we found the ring is about to full to avoid interrupt
> storm, so we're probably ok in this case.

I'm only concerned about the transition state when converting from
napi to no-napi when the queue is stopped and tx interrupt disabled.

With napi mode the interrupt is only disabled if napi is scheduled,
in which case it will eventually reenable the interrupt. But when
switching to no-napi mode in this state no progress will be made.

But it seems this cannot happen. When converting to no-napi
mode, set_coalesce waits for napi to complete in napi_disable.
So the interrupt should always start enabled when transitioning
into no-napi mode.

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月14日 12:46, Willem de Bruijn wrote:
> On Thu, Sep 13, 2018 at 11:53 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年09月14日 11:40, Willem de Bruijn wrote:
>>> On Thu, Sep 13, 2018 at 11:27 PM Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>> On 2018年09月13日 22:58, Willem de Bruijn wrote:
>>>>> On Thu, Sep 13, 2018 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2018年09月13日 07:27, Willem de Bruijn wrote:
>>>>>>> On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
>>>>>>> <willemdebruijn.kernel@gmail.com> wrote:
>>>>>>>> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>>>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
>>>>>>>>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>>>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
>>>>>>>>>>>> From: Willem de Bruijn <willemb@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.
>>>>>>>>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
>>>>>>>>>>> appropriate rather than use the coalescing configuration API here?
>>>>>>>>>> What do you mean by private ethtool flag? A new field in ethtool
>>>>>>>>>> --features (-k)?
>>>>>>>>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
>>>>>>>>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
>>>>>>>>> private flag. mlx5 has a number of privates flags for instance.
>>>>>>>> Interesting, thanks! I was not at all aware of those ethtool flags.
>>>>>>>> Am having a look. It definitely looks promising.
>>>>>>> Okay, I made that change. That is indeed much cleaner, thanks.
>>>>>>> Let me send the patch, initially as RFC.
>>>>>>>
>>>>>>> I've observed one issue where if we toggle the flag before bringing
>>>>>>> up the device, it hits a kernel BUG at include/linux/netdevice.h:515
>>>>>>>
>>>>>>>             BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
>>>>>> This reminds me that we need to check netif_running() before trying to
>>>>>> enable and disable tx napi in ethtool_set_coalesce().
>>>>> The first iteration of my patch checked IFF_UP and effectively
>>>>> only allowed the change when not running. What do you mean
>>>>> by need to check?
>>>> I mean if device is not up, there's no need to toggle napi state and tx
>>>> lock.
>>>>
>>>>> And to respond to the other follow-up notes at once:
>>>>>
>>>>>> Consider we may have interrupt moderation in the future, I tend to use
>>>>>> set_coalesce. Otherwise we may need two steps to enable moderation:
>>>>>>
>>>>>> - tx-napi on
>>>>>> - set_coalesce
>>>>> FWIW, I don't care strongly whether we do this through coalesce or priv_flags.
>>>> Ok.
>>> Since you prefer coalesce, let's go with that (and a revision of your
>>> latest patch).
>> Good to know this.
>>
>>>>>>> +                     if (!napi_weight)
>>>>>>> +                             virtqueue_enable_cb(vi->sq[i].vq);
>>>>>> I don't get why we need to disable enable cb here.
>>>>> To avoid entering no-napi mode with too few descriptors to
>>>>> make progress and no way to get out of that state. This is a
>>>>> pretty crude attempt at handling that, admittedly.
>>>> But in this case, we will call enable_cb_delayed() and we will finally
>>>> get a interrupt?
>>> Right. It's a bit of a roundabout way to ensure that
>>> netif_tx_wake_queue and thus eventually free_old_xmit_skbs are called.
>>> It might make more sense to just wake the device without going through
>>> an interrupt.
>> I'm not sure I get this. If we don't enable tx napi, we tend to delay TX
>> interrupt if we found the ring is about to full to avoid interrupt
>> storm, so we're probably ok in this case.
> I'm only concerned about the transition state when converting from
> napi to no-napi when the queue is stopped and tx interrupt disabled.
>
> With napi mode the interrupt is only disabled if napi is scheduled,
> in which case it will eventually reenable the interrupt. But when
> switching to no-napi mode in this state no progress will be made.
>
> But it seems this cannot happen. When converting to no-napi
> mode, set_coalesce waits for napi to complete in napi_disable.
> So the interrupt should always start enabled when transitioning
> into no-napi mode.

Yes, I see.

Thanks

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月14日 12:46, Willem de Bruijn wrote:
>> I'm not sure I get this. If we don't enable tx napi, we tend to delay TX
>> interrupt if we found the ring is about to full to avoid interrupt
>> storm, so we're probably ok in this case.
> I'm only concerned about the transition state when converting from
> napi to no-napi when the queue is stopped and tx interrupt disabled.
>
> With napi mode the interrupt is only disabled if napi is scheduled,
> in which case it will eventually reenable the interrupt. But when
> switching to no-napi mode in this state no progress will be made.
>
> But it seems this cannot happen. When converting to no-napi
> mode, set_coalesce waits for napi to complete in napi_disable.
> So the interrupt should always start enabled when transitioning
> into no-napi mode.

An update, I meet a hang in napi_disalbe(). But it's hard to be 
reproduced. I tend to choose a easy way like V1 that only allow the 
switching when device is down.

I will post the patch after a vacation. (or you can post if it was 
urgent for you).

Thanks

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Willem de Bruijn]

On Thu, Sep 27, 2018 at 4:51 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年09月14日 12:46, Willem de Bruijn wrote:
> >> I'm not sure I get this. If we don't enable tx napi, we tend to delay TX
> >> interrupt if we found the ring is about to full to avoid interrupt
> >> storm, so we're probably ok in this case.
> > I'm only concerned about the transition state when converting from
> > napi to no-napi when the queue is stopped and tx interrupt disabled.
> >
> > With napi mode the interrupt is only disabled if napi is scheduled,
> > in which case it will eventually reenable the interrupt. But when
> > switching to no-napi mode in this state no progress will be made.
> >
> > But it seems this cannot happen. When converting to no-napi
> > mode, set_coalesce waits for napi to complete in napi_disable.
> > So the interrupt should always start enabled when transitioning
> > into no-napi mode.
>
> An update, I meet a hang in napi_disalbe(). But it's hard to be
> reproduced. I tend to choose a easy way like V1 that only allow the
> switching when device is down.

I agree.

> I will post the patch after a vacation. (or you can post if it was
> urgent for you).

If you have time to review and add your signed-off-by, I can post it.
It's a pretty small diff at this point.

But no rush, we can also wait until after your vacation.

I also need to look at a patch to toggle LRO using ethtool, btw.

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月27日 21:53, Willem de Bruijn wrote:
> On Thu, Sep 27, 2018 at 4:51 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年09月14日 12:46, Willem de Bruijn wrote:
>>>> I'm not sure I get this. If we don't enable tx napi, we tend to delay TX
>>>> interrupt if we found the ring is about to full to avoid interrupt
>>>> storm, so we're probably ok in this case.
>>> I'm only concerned about the transition state when converting from
>>> napi to no-napi when the queue is stopped and tx interrupt disabled.
>>>
>>> With napi mode the interrupt is only disabled if napi is scheduled,
>>> in which case it will eventually reenable the interrupt. But when
>>> switching to no-napi mode in this state no progress will be made.
>>>
>>> But it seems this cannot happen. When converting to no-napi
>>> mode, set_coalesce waits for napi to complete in napi_disable.
>>> So the interrupt should always start enabled when transitioning
>>> into no-napi mode.
>> An update, I meet a hang in napi_disalbe(). But it's hard to be
>> reproduced. I tend to choose a easy way like V1 that only allow the
>> switching when device is down.
> I agree.
>
>> I will post the patch after a vacation. (or you can post if it was
>> urgent for you).
> If you have time to review and add your signed-off-by, I can post it.
> It's a pretty small diff at this point.
>
> But no rush, we can also wait until after your vacation.

Then let me post it after the vacation.

>
> I also need to look at a patch to toggle LRO using ethtool, btw.

Interesting, we've already did something similar during XDP. The 
GUEST_TSO_XXX part may need some private flags I believe.

Thanks

Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang]

On 2018年09月13日 03:11, Willem de Bruijn wrote:
> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>
>>>>
>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
>>>>> From: Willem de Bruijn <willemb@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.
>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
>>>> appropriate rather than use the coalescing configuration API here?
>>> What do you mean by private ethtool flag? A new field in ethtool
>>> --features (-k)?
>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
>> private flag. mlx5 has a number of privates flags for instance.
> Interesting, thanks! I was not at all aware of those ethtool flags.
> Am having a look. It definitely looks promising.
>
>>> Configurable napi-tx is not a common feature across devices. We really
>>> want virtio-net to also just convert to napi-tx as default, but need a
>>> way to gradually convert with application opt-out if some workloads
>>> see regressions.
>> The rationale makes sense, no questions about it.
>>
>>> There is prior art in interpreting coalesce values as
>>> more than a direct mapping to usec. The e1000 is one example.
>>>
>> Looked at both e1000 and e1000e and they both have a similar programming
>> of the HW's interrupt target rate register, which is relevant to
>> interrupt coalescing, what part of these drivers do you see as doing
>> something not quite coalescing related?
> It's all coalescing related, for sure. e1000_set_coalesce just does not
> translate the tx-usecs values into microsecond latency directly.
>
> It modifies both the interrupt throttle rate adapter->itr and interrupt mode
> adapter->itr_setting, which are initially set in e1000_check_options from
> module param InterruptThrottleRate.
>
> Value 0 disables interrupt moderation. 1 and 3 program a dynamic mode.
> 2 is an illegal value as is 5..9. 10..10000 converts from usec to interrupt
> rate/sec.
>
> I took tx-napi to be a similar interrupt related option as, say, dynamic
> conservative mode interrupt moderation.

Consider we may have interrupt moderation in the future, I tend to use 
set_coalesce. Otherwise we may need two steps to enable moderation:

- tx-napi on
- set_coalesce

Thanks