Re: [net-next rfc v7 2/3] virtio_net: multiqueue support

2012-12-03 Thread Michael S. Tsirkin
On Mon, Dec 03, 2012 at 02:05:27PM +0800, Jason Wang wrote:
 On Monday, December 03, 2012 12:34:08 PM Rusty Russell wrote:
  Jason Wang jasow...@redhat.com writes:
   +static const struct ethtool_ops virtnet_ethtool_ops;
   +
   +/*
   + * Converting between virtqueue no. and kernel tx/rx queue no.
   + * 0:rx0 1:tx0 2:cvq 3:rx1 4:tx1 ... 2N+1:rxN 2N+2:txN
   + */
   +static int vq2txq(struct virtqueue *vq)
   +{
   + int index = virtqueue_get_queue_index(vq);
   + return index == 1 ? 0 : (index - 2) / 2;
   +}
   +
   +static int txq2vq(int txq)
   +{
   + return txq ? 2 * txq + 2 : 1;
   +}
   +
   +static int vq2rxq(struct virtqueue *vq)
   +{
   + int index = virtqueue_get_queue_index(vq);
   + return index ? (index - 1) / 2 : 0;
   +}
   +
   +static int rxq2vq(int rxq)
   +{
   + return rxq ? 2 * rxq + 1 : 0;
   +}
   +
  
  I thought MST changed the proposed spec to make the control queue always
  the last one, so this logic becomes trivial.
 
 But it may break the support of legacy guest. If we boot a legacy single 
 queue 
 guest on a 2 queue virtio-net device. It may think vq 2 is cvq which is 
 indeed 
 rx1.

Legacy guyest support should be handled by host using feature
bits in the usual way: host should detect legacy guest
by checking the VIRTIO_NET_F_RFS feature.

If VIRTIO_NET_F_RFS is acked, cvq is vq max_virtqueue_pairs * 2.
If it's not acked, cvq is vq 2.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 2/3] virtio_net: multiqueue support

2012-12-03 Thread Jason Wang
On 12/03/2012 05:47 PM, Michael S. Tsirkin wrote:
 On Mon, Dec 03, 2012 at 02:05:27PM +0800, Jason Wang wrote:
 On Monday, December 03, 2012 12:34:08 PM Rusty Russell wrote:
 Jason Wang jasow...@redhat.com writes:
 +static const struct ethtool_ops virtnet_ethtool_ops;
 +
 +/*
 + * Converting between virtqueue no. and kernel tx/rx queue no.
 + * 0:rx0 1:tx0 2:cvq 3:rx1 4:tx1 ... 2N+1:rxN 2N+2:txN
 + */
 +static int vq2txq(struct virtqueue *vq)
 +{
 +  int index = virtqueue_get_queue_index(vq);
 +  return index == 1 ? 0 : (index - 2) / 2;
 +}
 +
 +static int txq2vq(int txq)
 +{
 +  return txq ? 2 * txq + 2 : 1;
 +}
 +
 +static int vq2rxq(struct virtqueue *vq)
 +{
 +  int index = virtqueue_get_queue_index(vq);
 +  return index ? (index - 1) / 2 : 0;
 +}
 +
 +static int rxq2vq(int rxq)
 +{
 +  return rxq ? 2 * rxq + 1 : 0;
 +}
 +
 I thought MST changed the proposed spec to make the control queue always
 the last one, so this logic becomes trivial.
 But it may break the support of legacy guest. If we boot a legacy single 
 queue 
 guest on a 2 queue virtio-net device. It may think vq 2 is cvq which is 
 indeed 
 rx1.
 Legacy guyest support should be handled by host using feature
 bits in the usual way: host should detect legacy guest
 by checking the VIRTIO_NET_F_RFS feature.

 If VIRTIO_NET_F_RFS is acked, cvq is vq max_virtqueue_pairs * 2.
 If it's not acked, cvq is vq 2.


We could, but we didn't gain much from this. Furthermore, we need also do the 
dynamic creation/destroying of virtqueues during feature negotiation which 
seems not supported in qemu now.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 2/3] virtio_net: multiqueue support

2012-12-03 Thread Michael S. Tsirkin
On Tue, Nov 27, 2012 at 06:15:59PM +0800, Jason Wang wrote:
 - if (!try_fill_recv(vi-rq, GFP_KERNEL))
 - schedule_delayed_work(vi-rq.refill, 0);
 + for (i = 0; i  vi-max_queue_pairs; i++)
 + if (!try_fill_recv(vi-rq[i], GFP_KERNEL))
 + schedule_delayed_work(vi-rq[i].refill, 0);
  
   mutex_lock(vi-config_lock);
   vi-config_enable = true;
   mutex_unlock(vi-config_lock);
  
 + BUG_ON(virtnet_set_queues(vi));
 +
   return 0;
  }
  #endif

Also crashing on device nack of command is also not nice.
In this case it seems we can just switch to
single-queue mode which should always be safe.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 2/3] virtio_net: multiqueue support

2012-12-03 Thread Jason Wang
On 12/03/2012 06:14 PM, Michael S. Tsirkin wrote:
 On Tue, Nov 27, 2012 at 06:15:59PM +0800, Jason Wang wrote:
  -  if (!try_fill_recv(vi-rq, GFP_KERNEL))
  -  schedule_delayed_work(vi-rq.refill, 0);
  +  for (i = 0; i  vi-max_queue_pairs; i++)
  +  if (!try_fill_recv(vi-rq[i], GFP_KERNEL))
  +  schedule_delayed_work(vi-rq[i].refill, 0);
   
 mutex_lock(vi-config_lock);
 vi-config_enable = true;
 mutex_unlock(vi-config_lock);
   
  +  BUG_ON(virtnet_set_queues(vi));
  +
 return 0;
   }
   #endif
 Also crashing on device nack of command is also not nice.
 In this case it seems we can just switch to
 single-queue mode which should always be safe.

Not sure it's safe. It depends on the reason why this call fails. If we
left a state that the driver only use single queue but the device use
multi queues, we may still lost the network.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCHv5] virtio-spec: virtio network device RFS support

2012-12-03 Thread Michael S. Tsirkin
Add RFS support to virtio network device.
Add a new feature flag VIRTIO_NET_F_RFS for this feature, a new
configuration field max_virtqueue_pairs to detect supported number of
virtqueues as well as a new command VIRTIO_NET_CTRL_RFS to program
packet steering for unidirectional protocols.

---

Changes from v5:
- Address Rusty's comments.
  Changes are only in the text, not the ideas.
- Some minor formatting changes.

Changes from v4:
- address Jason's comments
- have configuration specify the number of VQ pairs and not pairs - 1

Changes from v3:
- rename multiqueue - rfs this is what we support
- Be more explicit about what driver should do.
- Simplify layout making VQs functionality depend on feature.
- Remove unused commands, only leave in programming # of queues

Changes from v2:
Address Jason's comments on v2:
- Changed STEERING_HOST to STEERING_RX_FOLLOWS_TX:
   this is both clearer and easier to support.
   It does not look like we need a separate steering command
   since host can just watch tx packets as they go.
- Moved RX and TX steering sections near each other.
- Add motivation for other changes in v2

Changes from Jason's rfc:
- reserved vq 3: this makes all rx vqs even and tx vqs odd, which
   looks nicer to me.
- documented packet steering, added a generalized steering programming
   command. Current modes are single queue and host driven multiqueue,
   but I envision support for guest driven multiqueue in the future.
- make default vqs unused when in mq mode - this wastes some memory
   but makes it more efficient to switch between modes as
   we can avoid this causing packet reordering.

Rusty, could you please take a look and comment soon?
If this looks OK to everyone, we can proceed with finalizing the
implementation. Would be nice to try and put it in 3.8.

 virtio-spec.lyx | 310 ++--
 1 file changed, 301 insertions(+), 9 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 83f2771..119925c 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -59,6 +59,7 @@
 \author -608949062 Rusty Russell,,, 
 \author -385801441 Cornelia Huck cornelia.h...@de.ibm.com
 \author 1531152142 Paolo Bonzini,,, 
+\author 1986246365 Michael S. Tsirkin 
 \end_header
 
 \begin_body
@@ -4170,9 +4171,46 @@ ID 1
 \end_layout
 
 \begin_layout Description
-Virtqueues 0:receiveq.
- 1:transmitq.
- 2:controlq
+Virtqueues 0:receiveq
+\change_inserted 1986246365 1352742829
+0
+\change_unchanged
+.
+ 1:transmitq
+\change_inserted 1986246365 1352742832
+0
+\change_deleted 1986246365 1352742947
+.
+ 
+\change_inserted 1986246365 1352742952
+.
+ 
+ 2N
+\begin_inset Foot
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1354531595
+N=0 if VIRTIO_NET_F_RFS is not negotiated, otherwise N is derived from 
+\emph on
+max_virtqueue_pairs
+\emph default
+ control
+\emph on
+ 
+\emph default
+field.
+ 
+\end_layout
+
+\end_inset
+
+: receivqN.
+ 2N+1: transmitqN.
+ 2N+
+\change_unchanged
+2:controlq
 \begin_inset Foot
 status open
 
@@ -4343,6 +4381,16 @@ VIRTIO_NET_F_CTRL_VLAN
 
 \begin_layout Description
 VIRTIO_NET_F_GUEST_ANNOUNCE(21) Guest can send gratuitous packets.
+\change_inserted 1986246365 1352742767
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 1986246365 1352742808
+VIRTIO_NET_F_RFS(22) Device supports Receive Flow Steering.
+\change_unchanged
+
 \end_layout
 
 \end_deeper
@@ -4355,11 +4403,45 @@ configuration
 \begin_inset space ~
 \end_inset
 
-layout Two configuration fields are currently defined.
+layout 
+\change_deleted 1986246365 1352743300
+Two
+\change_inserted 1986246365 1354531413
+Three
+\change_unchanged
+ configuration fields are currently defined.
  The mac address field always exists (though is only valid if VIRTIO_NET_F_MAC
  is set), and the status field only exists if VIRTIO_NET_F_STATUS is set.
  Two read-only bits are currently defined for the status field: 
VIRTIO_NET_S_LIN
 K_UP and VIRTIO_NET_S_ANNOUNCE.
+
+\change_inserted 1986246365 1354531470
+ The following read-only field, 
+\emph on
+max_virtqueue_pairs
+\emph default
+ only exists if VIRTIO_NET_F_RFS is set.
+ This field specifies the maximum number of each of transmit and receive
+ virtqueues (receiveq0..receiveq
+\emph on
+N
+\emph default
+ and transmitq0..transmitq
+\emph on
+N
+\emph default
+ respectively; 
+\emph on
+N
+\emph default
+=
+\emph on
+max_virtqueue_pairs - 1
+\emph default
+) that can be configured once VIRTIO_NET_F_RFS is negotiated.
+ Legal values for this field are 1 to 8000h.
+
+\change_unchanged
  
 \begin_inset listings
 inline false
@@ -4392,6 +4474,17 @@ struct virtio_net_config {
 \begin_layout Plain Layout
 
 u16 status;
+\change_inserted 1986246365 1354531427
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1354531437
+
+u16 max_virtqueue_pairs;
+\change_unchanged
+
 \end_layout
 
 \begin_layout Plain Layout
@@ -4410,7 +4503,24 @@ Device Initialization
 
 

Re: [net-next rfc v7 2/3] virtio_net: multiqueue support

2012-12-03 Thread Michael S. Tsirkin
On Mon, Dec 03, 2012 at 06:30:49PM +0800, Jason Wang wrote:
 On 12/03/2012 06:14 PM, Michael S. Tsirkin wrote:
  On Tue, Nov 27, 2012 at 06:15:59PM +0800, Jason Wang wrote:
   -if (!try_fill_recv(vi-rq, GFP_KERNEL))
   -schedule_delayed_work(vi-rq.refill, 0);
   +for (i = 0; i  vi-max_queue_pairs; i++)
   +if (!try_fill_recv(vi-rq[i], GFP_KERNEL))
   +schedule_delayed_work(vi-rq[i].refill, 0);

mutex_lock(vi-config_lock);
vi-config_enable = true;
mutex_unlock(vi-config_lock);

   +BUG_ON(virtnet_set_queues(vi));
   +
return 0;
}
#endif
  Also crashing on device nack of command is also not nice.
  In this case it seems we can just switch to
  single-queue mode which should always be safe.
 
 Not sure it's safe. It depends on the reason why this call fails. If we
 left a state that the driver only use single queue but the device use
 multi queues, we may still lost the network.

Not the way driver is currently written - you'll happily
process incoming packets from all queues so no problem?

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 2/3] virtio_net: multiqueue support

2012-12-03 Thread Michael S. Tsirkin
On Mon, Dec 03, 2012 at 06:01:58PM +0800, Jason Wang wrote:
 On 12/03/2012 05:47 PM, Michael S. Tsirkin wrote:
  On Mon, Dec 03, 2012 at 02:05:27PM +0800, Jason Wang wrote:
  On Monday, December 03, 2012 12:34:08 PM Rusty Russell wrote:
  Jason Wang jasow...@redhat.com writes:
  +static const struct ethtool_ops virtnet_ethtool_ops;
  +
  +/*
  + * Converting between virtqueue no. and kernel tx/rx queue no.
  + * 0:rx0 1:tx0 2:cvq 3:rx1 4:tx1 ... 2N+1:rxN 2N+2:txN
  + */
  +static int vq2txq(struct virtqueue *vq)
  +{
  +int index = virtqueue_get_queue_index(vq);
  +return index == 1 ? 0 : (index - 2) / 2;
  +}
  +
  +static int txq2vq(int txq)
  +{
  +return txq ? 2 * txq + 2 : 1;
  +}
  +
  +static int vq2rxq(struct virtqueue *vq)
  +{
  +int index = virtqueue_get_queue_index(vq);
  +return index ? (index - 1) / 2 : 0;
  +}
  +
  +static int rxq2vq(int rxq)
  +{
  +return rxq ? 2 * rxq + 1 : 0;
  +}
  +
  I thought MST changed the proposed spec to make the control queue always
  the last one, so this logic becomes trivial.
  But it may break the support of legacy guest. If we boot a legacy single 
  queue 
  guest on a 2 queue virtio-net device. It may think vq 2 is cvq which is 
  indeed 
  rx1.
  Legacy guyest support should be handled by host using feature
  bits in the usual way: host should detect legacy guest
  by checking the VIRTIO_NET_F_RFS feature.
 
  If VIRTIO_NET_F_RFS is acked, cvq is vq max_virtqueue_pairs * 2.
  If it's not acked, cvq is vq 2.
 
 
 We could, but we didn't gain much from this.

It just seems cleaner and easier to understand.

 Furthermore, we need also
 do the dynamic creation/destroying of virtqueues during feature
 negotiation which seems not supported in qemu now.

It's not *done* in qemu now, but it seems easy: just call
virtio_add_queue for vq2 and on from virtio_net_set_features.
As features can be modified multiple times, we
should add virtio_del_queue and call that beforehand
to get to the known state (two vqs).

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 1/3] virtio-net: separate fields of sending/receiving queue from virtnet_info

2012-12-03 Thread Michael S. Tsirkin
On Mon, Dec 03, 2012 at 01:15:01PM +0800, Jason Wang wrote:
   +
 
   + /* Work struct for refilling if we run low on memory. */
 
   + struct delayed_work refill;
 
 
 
  I can't really see the justificaiton for a refill per queue. Just have
 
  one work iterate all the queues if it happens, unless it happens often
 
  (in which case, we need to look harder at this anyway).
 
  
 
 But during this kind of iteration, we may need enable/disable the napi
 regardless of whether the receive queue has lots to be refilled. This may add
 extra latency.

We are running from the timer, so latency is not a concern I think.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 3/3] virtio-net: change the number of queues through ethtool

2012-12-03 Thread Michael S. Tsirkin
On Mon, Dec 03, 2012 at 02:09:28PM +0800, Jason Wang wrote:
 On Sunday, December 02, 2012 06:09:06 PM Michael S. Tsirkin wrote:
  On Tue, Nov 27, 2012 at 06:16:00PM +0800, Jason Wang wrote:
   This patch implement the {set|get}_channels method of ethool to allow user
   to change the number of queues dymaically when the device is running.
   This would let the user to configure it on demand.
   
   Signed-off-by: Jason Wang jasow...@redhat.com
   ---
   
drivers/net/virtio_net.c |   41 +
1 files changed, 41 insertions(+), 0 deletions(-)
   
   diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
   index bcaa6e5..f08ec2a 100644
   --- a/drivers/net/virtio_net.c
   +++ b/drivers/net/virtio_net.c
   @@ -1578,10 +1578,51 @@ static struct virtio_driver virtio_net_driver = {
   
#endif
};
   
   +/* TODO: Eliminate OOO packets during switching */
   +static int virtnet_set_channels(struct net_device *dev,
   + struct ethtool_channels *channels)
   +{
   + struct virtnet_info *vi = netdev_priv(dev);
   + u16 queue_pairs = channels-combined_count;

by the way shouldn't this be combined_count / 2?

And below channels-max_combined = vi-max_queue_pairs * 2; ?

   +
   + /* We don't support separate rx/tx channels.
   +  * We don't allow setting 'other' channels.
   +  */
   + if (channels-rx_count || channels-tx_count || channels-other_count)
   + return -EINVAL;
   +
   + /* Only two modes were support currently */
   + if (queue_pairs != vi-max_queue_pairs  queue_pairs != 1)
   + return -EINVAL;
   +
  
  Why the limitation?
 
 Not sure the value bettwen 1 and max_queue_pairs is useful. But anyway, I can 
 remove this limitation.

I guess a reasonable value would be number of active CPUs,
which can change with CPU hotplug.

  Also how does userspace discover what the legal values are?
 
 Userspace only check whether the value is greater than max_queue_pairs.

Exactly. One can query max combined but the fact that configuring
any less is impossible might be surprising.

  
   + vi-curr_queue_pairs = queue_pairs;
   + BUG_ON(virtnet_set_queues(vi));
   +
   + netif_set_real_num_tx_queues(dev, vi-curr_queue_pairs);
   + netif_set_real_num_rx_queues(dev, vi-curr_queue_pairs);
   +
   + return 0;
   +}
   +
   +static void virtnet_get_channels(struct net_device *dev,
   +  struct ethtool_channels *channels)
   +{
   + struct virtnet_info *vi = netdev_priv(dev);
   +
   + channels-combined_count = vi-curr_queue_pairs;
   + channels-max_combined = vi-max_queue_pairs;
   + channels-max_other = 0;
   + channels-rx_count = 0;
   + channels-tx_count = 0;
   + channels-other_count = 0;
   +}
   +
   
static const struct ethtool_ops virtnet_ethtool_ops = {

 .get_drvinfo = virtnet_get_drvinfo,
 .get_link = ethtool_op_get_link,
 .get_ringparam = virtnet_get_ringparam,
   
   + .set_channels = virtnet_set_channels,
   + .get_channels = virtnet_get_channels,
   
};

static int __init init(void)
  
  --
  To unsubscribe from this list: send the line unsubscribe kvm in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next RFC] pktgen: don't wait for the device who doesn't free skb immediately after sent

2012-12-03 Thread Stephen Hemminger
On Mon, 03 Dec 2012 14:45:46 +0800
Jason Wang jasow...@redhat.com wrote:

 On Tuesday, November 27, 2012 08:49:19 AM Stephen Hemminger wrote:
  On Tue, 27 Nov 2012 14:45:13 +0800
  
  Jason Wang jasow...@redhat.com wrote:
   On 11/27/2012 01:37 AM, Stephen Hemminger wrote:
On Mon, 26 Nov 2012 15:56:52 +0800

Jason Wang jasow...@redhat.com wrote:
Some deivces do not free the old tx skbs immediately after it has been
sent
(usually in tx interrupt). One such example is virtio-net which
optimizes for virt and only free the possible old tx skbs during the
next packet sending. This would lead the pktgen to wait forever in the
refcount of the skb if no other pakcet will be sent afterwards.

Solving this issue by introducing a new flag IFF_TX_SKB_FREE_DELAY
which could notify the pktgen that the device does not free skb
immediately after it has been sent and let it not to wait for the
refcount to be one.

Signed-off-by: Jason Wang jasow...@redhat.com

Another alternative would be using skb_orphan() and skb-destructor.
There are other cases where skb's are not freed right away.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
   
   Hi Stephen:
   
   Do you mean registering a skb-destructor for pktgen then set and check
   bits in skb-tx_flag?
  
  Yes. Register a destructor that does something like update a counter (number
  of packets pending), then just spin while number of packets pending is over
  threshold.
 
 Have some experiments on this, looks like it does not work weel when 
 clone_skb 
 is used. For driver that call skb_orphan() in ndo_start_xmit, the destructor 
 is only called when the first packet were sent, but what we need to know is 
 when the last were sent. Any thoughts on this or we can just introduce 
 another 
 flag (anyway we have something like IFF_TX_SKB_SHARING) ?
 

The SKB_SHARING flag looks like the best solution then.
Surprisingly, transmit buffer completion is a major bottleneck for 10G
devices, and I suspect more changes will come.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PATCH] vhost-net: initialize zcopy packet counters

2012-12-03 Thread Michael S. Tsirkin
These packet counters are used to drive the zercopy
selection heuristic so nothing too bad happens if they are off a bit -
and they are also reset once in a while.
But it's cleaner to clear them when backend is set so that
we start in a known state.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/vhost/net.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 67898fa..ff6c9199 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -823,6 +823,9 @@ static long vhost_net_set_backend(struct vhost_net *n, 
unsigned index, int fd)
r = vhost_init_used(vq);
if (r)
goto err_vq;
+
+   n-tx_packets = 0;
+   n-tx_zcopy_err = 0;
}
 
mutex_unlock(vq-mutex);
-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH] vhost-net: initialize zcopy packet counters

2012-12-03 Thread David Miller
From: Michael S. Tsirkin m...@redhat.com
Date: Mon, 3 Dec 2012 19:31:51 +0200

 These packet counters are used to drive the zercopy
 selection heuristic so nothing too bad happens if they are off a bit -
 and they are also reset once in a while.
 But it's cleaner to clear them when backend is set so that
 we start in a known state.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com

Applied to net-next, thanks.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


re: xen/blkback: Persistent grant maps for xen blk drivers

2012-12-03 Thread Dan Carpenter
Hello Roger Pau Monne,

The patch 0a8704a51f38: xen/blkback: Persistent grant maps for xen 
blk drivers from Oct 24, 2012, leads to the following warning:
drivers/block/xen-blkfront.c:807 blkif_free()
 warn: 'persistent_gnt' was already freed.

   807  llist_for_each_entry(persistent_gnt, all_gnts, node) {
   808  gnttab_end_foreign_access(persistent_gnt-gref, 
0, 0UL);
   809  __free_page(pfn_to_page(persistent_gnt-pfn));
   810  kfree(persistent_gnt);
  ^^
We dereference this to find the next element in the list.  It will work
if you don't have poisoning enabled or if the memory is not used
immediately by another process.  In other words, there will be rare
bugs where this causes a hard to debug crash or if you have poisoning
enabled it will have an easy to debug crash.

   811  }

regards,
dan carpenter

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


re: xen-blkback: move free persistent grants code

2012-12-03 Thread Dan Carpenter
Hello Roger Pau Monne,

The patch 4d4f270f1880: xen-blkback: move free persistent grants
code from Nov 16, 2012, leads to the following warning:
drivers/block/xen-blkback/blkback.c:238 free_persistent_gnts()
 warn: 'persistent_gnt' was already freed.

drivers/block/xen-blkback/blkback.c
   232  pages[segs_to_unmap] = persistent_gnt-page;
   233  rb_erase(persistent_gnt-node, root);
   234  kfree(persistent_gnt);

kfree();

   235  num--;
   236  
   237  if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST ||
   238  !rb_next(persistent_gnt-node)) {
 ^
Dereferenced inside the call to rb_next().

   239  ret = gnttab_unmap_refs(unmap, NULL, pages,
   240  segs_to_unmap);

regards,
dan carpenter

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 3/3] virtio-net: change the number of queues through ethtool

2012-12-03 Thread Ben Hutchings
On Mon, 2012-12-03 at 13:25 +0200, Michael S. Tsirkin wrote:
 On Mon, Dec 03, 2012 at 02:09:28PM +0800, Jason Wang wrote:
  On Sunday, December 02, 2012 06:09:06 PM Michael S. Tsirkin wrote:
   On Tue, Nov 27, 2012 at 06:16:00PM +0800, Jason Wang wrote:
This patch implement the {set|get}_channels method of ethool to allow 
user
to change the number of queues dymaically when the device is running.
This would let the user to configure it on demand.

Signed-off-by: Jason Wang jasow...@redhat.com
---

 drivers/net/virtio_net.c |   41 
+
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bcaa6e5..f08ec2a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1578,10 +1578,51 @@ static struct virtio_driver virtio_net_driver = 
{

 #endif
 };

+/* TODO: Eliminate OOO packets during switching */
+static int virtnet_set_channels(struct net_device *dev,
+   struct ethtool_channels *channels)
+{
+   struct virtnet_info *vi = netdev_priv(dev);
+   u16 queue_pairs = channels-combined_count;
 
 by the way shouldn't this be combined_count / 2?
 
 And below channels-max_combined = vi-max_queue_pairs * 2; ?
[...]

In this ethtool API, 'channel' means an IRQ and set of queues that
trigger it.  So each ethtool-channel will correspond to one queue-pair
and not one virtio channel.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 1/3] virtio-net: separate fields of sending/receiving queue from virtnet_info

2012-12-03 Thread Rusty Russell
Jason Wang jasow...@redhat.com writes:
 On Monday, December 03, 2012 12:25:42 PM Rusty Russell wrote:
  +
  +  /* Work struct for refilling if we run low on memory. */
  +  struct delayed_work refill;
 
 I can't really see the justificaiton for a refill per queue.  Just have
 one work iterate all the queues if it happens, unless it happens often
 (in which case, we need to look harder at this anyway).

 But during this kind of iteration, we may need enable/disable the napi 
 regardless of whether the receive queue has lots to be refilled. This may add 
 extra latency. 

Sure, but does it actually happen?  We only use the work when we run out
of memory.  If this happens in normal behaviour we need to change
something else...

Thanks,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH] virtio-spec: serial: english tweak

2012-12-03 Thread Amos Kong
- Original Message -
 A number of virtqueues are created seems clearer
 than the number of virtqueues: it's
 virtqueues that are created not the number.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com

Acked-by: Amos Kong ak...@redhat.com


Two opposite problem exists:

--- virtio-0.9.5.lyx.orig   2012-12-04 09:29:03.249676649 +0800
+++ virtio-0.9.5.lyx2012-12-04 09:28:26.272678220 +0800
@@ -8685,7 +8685,7 @@ s on QueueNum, QueueAlign and QueuePFN a
 \begin_inset Newline newline
 \end_inset
 
- Queue size is a number of elements in the queue, therefore size of the
+ Queue size is the number of elements in the queue, therefore size of the
  descriptor table and both available and used rings.
 \begin_inset Newline newline
 \end_inset
@@ -8850,7 +8850,7 @@ reference sub:Device-Initialization-Seq
 \end_layout
 
 \begin_layout Standard
-Virtual queue size is a number of elements in the queue, therefore size
+Virtual queue size is the number of elements in the queue, therefore size
  of the descriptor table and both available and used rings.
 \end_layout
 


 ---
 
 I'm not a native english speaker but the below
 seems correct to me. Rusty?
 
  virtio-spec.lyx | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/virtio-spec.lyx b/virtio-spec.lyx
 index d9626a9..a29315c 100644
 --- a/virtio-spec.lyx
 +++ b/virtio-spec.lyx
 @@ -6406,7 +6406,7 @@ If the VIRTIO_CONSOLE_F_MULTIPORT feature is
 negotiated, the driver can
   spawn multiple ports, not all of which may be attached to a
   console.
   Some could be generic ports.
   In this case, the control virtqueues are enabled and according to
   the max_nr_po
 -rts configuration-space value, the appropriate number of virtqueues
 are
 +rts configuration-space value, an appropriate number of virtqueues
 are
   created.
   A control message indicating the driver is ready is sent to the
   host.
   The host can then send control messages for adding new ports to the
   device.
 --
 MST
 ___
 Virtualization mailing list
 Virtualization@lists.linux-foundation.org
 https://lists.linuxfoundation.org/mailman/listinfo/virtualization
 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [net-next rfc v7 2/3] virtio_net: multiqueue support

2012-12-03 Thread Michael S. Tsirkin
On Mon, Dec 03, 2012 at 06:30:49PM +0800, Jason Wang wrote:
 On 12/03/2012 06:14 PM, Michael S. Tsirkin wrote:
  On Tue, Nov 27, 2012 at 06:15:59PM +0800, Jason Wang wrote:
   -if (!try_fill_recv(vi-rq, GFP_KERNEL))
   -schedule_delayed_work(vi-rq.refill, 0);
   +for (i = 0; i  vi-max_queue_pairs; i++)
   +if (!try_fill_recv(vi-rq[i], GFP_KERNEL))
   +schedule_delayed_work(vi-rq[i].refill, 0);

mutex_lock(vi-config_lock);
vi-config_enable = true;
mutex_unlock(vi-config_lock);

   +BUG_ON(virtnet_set_queues(vi));
   +
return 0;
}
#endif
  Also crashing on device nack of command is also not nice.
  In this case it seems we can just switch to
  single-queue mode which should always be safe.
 
 Not sure it's safe. It depends on the reason why this call fails. If we
 left a state that the driver only use single queue but the device use
 multi queues, we may still lost the network.

Looks like we won't: napi will stay enabled on all queues
so we will process incoming packets.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization