[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Jason Wang]

On 11/17/2015 04:23 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 16, 2015 at 02:20:57PM -0800, Flavio Leitner wrote:
>> > On Wed, Oct 28, 2015 at 11:12:25PM +0200, Michael S. Tsirkin wrote:
>>> > > On Wed, Oct 28, 2015 at 06:30:41PM -0200, Flavio Leitner wrote:
 > > > On Sat, Oct 24, 2015 at 08:47:10PM +0300, Michael S. Tsirkin wrote:
> > > > > On Sat, Oct 24, 2015 at 12:34:08AM -0200, Flavio Leitner wrote:
>> > > > > > On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin 
>> > > > > > wrote:
>>> > > > > > > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
 > > > > > > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. 
 > > > > > > > Tsirkin wrote:
> > > > > > > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu 
> > > > > > > > > wrote:
>>> > > > > > > > > > > Please note that for virtio devices, guest is 
>>> > > > > > > > > > > supposed to
>>> > > > > > > > > > > control the placement of incoming packets in RX 
>>> > > > > > > > > > > queues.
>> > > > > > > > > > 
>> > > > > > > > > > I may not follow you.
>> > > > > > > > > > 
>> > > > > > > > > > Enqueuing packets to a RX queue is done at vhost 
>> > > > > > > > > > lib, outside the
>> > > > > > > > > > guest, how could the guest take the control here?
>> > > > > > > > > > 
>> > > > > > > > > >--yliu
> > > > > > > > > 
> > > > > > > > > vhost should do what guest told it to.
> > > > > > > > > 
> > > > > > > > > See virtio spec:
> > > > > > > > >   5.1.6.5.5 Automatic receive steering in 
> > > > > > > > > multiqueue mode
 > > > > > > > 
 > > > > > > > Spec says:
 > > > > > > > 
 > > > > > > > After the driver transmitted a packet of a flow on 
 > > > > > > > transmitqX,
 > > > > > > > the device SHOULD cause incoming packets for that 
 > > > > > > > flow to be
 > > > > > > > steered to receiveqX.
 > > > > > > > 
 > > > > > > > 
 > > > > > > > Michael, I still have no idea how vhost could know the 
 > > > > > > > flow even
 > > > > > > > after discussion with Huawei. Could you be more specific 
 > > > > > > > about
 > > > > > > > this? Say, how could guest know that? And how could 
 > > > > > > > guest tell
 > > > > > > > vhost which RX is gonna to use?
 > > > > > > > 
 > > > > > > > Thanks.
 > > > > > > > 
 > > > > > > >  --yliu
>>> > > > > > > 
>>> > > > > > > I don't really understand the question.
>>> > > > > > > 
>>> > > > > > > When guests transmits a packet, it makes a decision
>>> > > > > > > about the flow to use, and maps that to a tx/rx pair of 
>>> > > > > > > queues.
>>> > > > > > > 
>>> > > > > > > It sends packets out on the tx queue and expects device to
>>> > > > > > > return packets from the same flow on the rx queue.
>> > > > > > 
>> > > > > > Why? I can understand that there should be a mapping between
>> > > > > > flows and queues in a way that there is no re-ordering, but
>> > > > > > I can't see the relation of receiving a flow with a TX queue.
>> > > > > > 
>> > > > > > fbl
> > > > > 
> > > > > That's the way virtio chose to program the rx steering logic.
> > > > > 
> > > > > It's low overhead (no special commands), and
> > > > > works well for TCP when user is an endpoint since rx and tx
> > > > > for tcp are generally tied (because of ack handling).
>> > 
>> > It is low overhead for the control plane, but not for the data plane.
> Well, there's zero data plane overhead within the guest.
> You can't go lower :)
>
> > > > > We can discuss other ways, e.g. special commands for guests to
> > > > > program steering.
> > > > > We'd have to first see some data showing the current scheme
> > > > > is problematic somehow.
>> > 
>> > The issue is that the spec assumes the packets are coming in
>> > a serialized way and the distribution will be made by vhost-user
>> > but that isn't necessarily true.
>> > 
> Making the distribution guest controlled is obviously the right
> thing to do if guest is the endpoint: we need guest scheduler to
> make the decisions, it's the only entity that knows
> how are tasks distributed across VCPUs.
>
> It's possible that this is not the right thing for when guest
> is just doing bridging between two VNICs:
> are you saying packets should just go from RX queue N
> on eth0 to TX queue N on eth1, making host make all
> the queue selection decisions?

The problem looks like current automatic steering policy is not flexible
for all kinds of workload in guest. So we can implement the feature of
ntuple filters and export the interfaces to let guest/drivers to decide.

>
> This sounds reasonable. Since 

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Flavio Leitner]

On Tue, Nov 17, 2015 at 10:23:38AM +0200, Michael S. Tsirkin wrote:
> On Mon, Nov 16, 2015 at 02:20:57PM -0800, Flavio Leitner wrote:
> > On Wed, Oct 28, 2015 at 11:12:25PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Oct 28, 2015 at 06:30:41PM -0200, Flavio Leitner wrote:
> > > > On Sat, Oct 24, 2015 at 08:47:10PM +0300, Michael S. Tsirkin wrote:
> > > > > On Sat, Oct 24, 2015 at 12:34:08AM -0200, Flavio Leitner wrote:
> > > > > > On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > > > > > > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin 
> > > > > > > > wrote:
> > > > > > > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > > > > > > > Please note that for virtio devices, guest is supposed to
> > > > > > > > > > > control the placement of incoming packets in RX queues.
> > > > > > > > > > 
> > > > > > > > > > I may not follow you.
> > > > > > > > > > 
> > > > > > > > > > Enqueuing packets to a RX queue is done at vhost lib, 
> > > > > > > > > > outside the
> > > > > > > > > > guest, how could the guest take the control here?
> > > > > > > > > > 
> > > > > > > > > > --yliu
> > > > > > > > > 
> > > > > > > > > vhost should do what guest told it to.
> > > > > > > > > 
> > > > > > > > > See virtio spec:
> > > > > > > > >   5.1.6.5.5 Automatic receive steering in multiqueue mode
> > > > > > > > 
> > > > > > > > Spec says:
> > > > > > > > 
> > > > > > > > After the driver transmitted a packet of a flow on 
> > > > > > > > transmitqX,
> > > > > > > > the device SHOULD cause incoming packets for that flow to be
> > > > > > > > steered to receiveqX.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Michael, I still have no idea how vhost could know the flow even
> > > > > > > > after discussion with Huawei. Could you be more specific about
> > > > > > > > this? Say, how could guest know that? And how could guest tell
> > > > > > > > vhost which RX is gonna to use?
> > > > > > > > 
> > > > > > > > Thanks.
> > > > > > > > 
> > > > > > > > --yliu
> > > > > > > 
> > > > > > > I don't really understand the question.
> > > > > > > 
> > > > > > > When guests transmits a packet, it makes a decision
> > > > > > > about the flow to use, and maps that to a tx/rx pair of queues.
> > > > > > > 
> > > > > > > It sends packets out on the tx queue and expects device to
> > > > > > > return packets from the same flow on the rx queue.
> > > > > > 
> > > > > > Why? I can understand that there should be a mapping between
> > > > > > flows and queues in a way that there is no re-ordering, but
> > > > > > I can't see the relation of receiving a flow with a TX queue.
> > > > > > 
> > > > > > fbl
> > > > > 
> > > > > That's the way virtio chose to program the rx steering logic.
> > > > > 
> > > > > It's low overhead (no special commands), and
> > > > > works well for TCP when user is an endpoint since rx and tx
> > > > > for tcp are generally tied (because of ack handling).
> > 
> > It is low overhead for the control plane, but not for the data plane.
> 
> Well, there's zero data plane overhead within the guest.
> You can't go lower :)

I agree, but I am talking about vhost-user or whatever means we use to
provide packets to the virtio backend. That will have to distribute
the packets according to the guest's mapping which is not zero overhead.


> > > > > We can discuss other ways, e.g. special commands for guests to
> > > > > program steering.
> > > > > We'd have to first see some data showing the current scheme
> > > > > is problematic somehow.
> > 
> > The issue is that the spec assumes the packets are coming in
> > a serialized way and the distribution will be made by vhost-user
> > but that isn't necessarily true.
> > 
> 
> Making the distribution guest controlled is obviously the right
> thing to do if guest is the endpoint: we need guest scheduler to
> make the decisions, it's the only entity that knows
> how are tasks distributed across VCPUs.

Again, I agree.  My point is that it can also allows no mapping
or full freedom. I don't see that as an option now.

> It's possible that this is not the right thing for when guest
> is just doing bridging between two VNICs:
> are you saying packets should just go from RX queue N
> on eth0 to TX queue N on eth1, making host make all
> the queue selection decisions?

The idea is that the guest could TX on queue N and the host
would push packets from the same stream on RX queue Y. So,
guest is free to send packets on any queue and the host is
free to send packet on any queue as long as both keep a stable
mapping to avoid re-ordering.

What if the guest is not trustable and the host has the requirement
to send priority packets to queue#0?  That is not possible if 
backend is forced to follow guest mapping.

> This sounds reasonable. Since there's a mix of local and
> bridged traffic normally, does this mean we need
> a 

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Michael S. Tsirkin]

On Mon, Nov 16, 2015 at 02:20:57PM -0800, Flavio Leitner wrote:
> On Wed, Oct 28, 2015 at 11:12:25PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Oct 28, 2015 at 06:30:41PM -0200, Flavio Leitner wrote:
> > > On Sat, Oct 24, 2015 at 08:47:10PM +0300, Michael S. Tsirkin wrote:
> > > > On Sat, Oct 24, 2015 at 12:34:08AM -0200, Flavio Leitner wrote:
> > > > > On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > > > > > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin 
> > > > > > > wrote:
> > > > > > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > > > > > > Please note that for virtio devices, guest is supposed to
> > > > > > > > > > control the placement of incoming packets in RX queues.
> > > > > > > > > 
> > > > > > > > > I may not follow you.
> > > > > > > > > 
> > > > > > > > > Enqueuing packets to a RX queue is done at vhost lib, outside 
> > > > > > > > > the
> > > > > > > > > guest, how could the guest take the control here?
> > > > > > > > > 
> > > > > > > > >   --yliu
> > > > > > > > 
> > > > > > > > vhost should do what guest told it to.
> > > > > > > > 
> > > > > > > > See virtio spec:
> > > > > > > > 5.1.6.5.5 Automatic receive steering in multiqueue mode
> > > > > > > 
> > > > > > > Spec says:
> > > > > > > 
> > > > > > > After the driver transmitted a packet of a flow on transmitqX,
> > > > > > > the device SHOULD cause incoming packets for that flow to be
> > > > > > > steered to receiveqX.
> > > > > > > 
> > > > > > > 
> > > > > > > Michael, I still have no idea how vhost could know the flow even
> > > > > > > after discussion with Huawei. Could you be more specific about
> > > > > > > this? Say, how could guest know that? And how could guest tell
> > > > > > > vhost which RX is gonna to use?
> > > > > > > 
> > > > > > > Thanks.
> > > > > > > 
> > > > > > >   --yliu
> > > > > > 
> > > > > > I don't really understand the question.
> > > > > > 
> > > > > > When guests transmits a packet, it makes a decision
> > > > > > about the flow to use, and maps that to a tx/rx pair of queues.
> > > > > > 
> > > > > > It sends packets out on the tx queue and expects device to
> > > > > > return packets from the same flow on the rx queue.
> > > > > 
> > > > > Why? I can understand that there should be a mapping between
> > > > > flows and queues in a way that there is no re-ordering, but
> > > > > I can't see the relation of receiving a flow with a TX queue.
> > > > > 
> > > > > fbl
> > > > 
> > > > That's the way virtio chose to program the rx steering logic.
> > > > 
> > > > It's low overhead (no special commands), and
> > > > works well for TCP when user is an endpoint since rx and tx
> > > > for tcp are generally tied (because of ack handling).
> 
> It is low overhead for the control plane, but not for the data plane.

Well, there's zero data plane overhead within the guest.
You can't go lower :)

> > > > We can discuss other ways, e.g. special commands for guests to
> > > > program steering.
> > > > We'd have to first see some data showing the current scheme
> > > > is problematic somehow.
> 
> The issue is that the spec assumes the packets are coming in
> a serialized way and the distribution will be made by vhost-user
> but that isn't necessarily true.
> 

Making the distribution guest controlled is obviously the right
thing to do if guest is the endpoint: we need guest scheduler to
make the decisions, it's the only entity that knows
how are tasks distributed across VCPUs.

It's possible that this is not the right thing for when guest
is just doing bridging between two VNICs:
are you saying packets should just go from RX queue N
on eth0 to TX queue N on eth1, making host make all
the queue selection decisions?

This sounds reasonable. Since there's a mix of local and
bridged traffic normally, does this mean we need
a per-packet flag that tells host to
ignore the packet for classification purposes?


> > > The issue is that the restriction imposes operations to be done in the
> > > data path.  For instance, Open vSwitch has N number of threads to manage
> > > X RX queues. We distribute them in round-robin fashion.  So, the thread
> > > polling one RX queue will do all the packet processing and push it to the
> > > TX queue of the other device (vhost-user or not) using the same 'id'.
> > > 
> > > Doing so we can avoid locking between threads and TX queues and any other
> > > extra computation while still keeping the packet ordering/distribution 
> > > fine.
> > > 
> > > However, if vhost-user has to send packets according with guest mapping,
> > > it will require locking between queues and additional operations to select
> > > the appropriate queue.  Those actions will cause performance issues.
> > 
> > You only need to send updates if guest moves a flow to another queue.
> > This is very rare since guest must avoid reordering.
> 
> OK, maybe I 

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Flavio Leitner]

On Wed, Oct 28, 2015 at 11:12:25PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 28, 2015 at 06:30:41PM -0200, Flavio Leitner wrote:
> > On Sat, Oct 24, 2015 at 08:47:10PM +0300, Michael S. Tsirkin wrote:
> > > On Sat, Oct 24, 2015 at 12:34:08AM -0200, Flavio Leitner wrote:
> > > > On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > > > > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > > > > > Please note that for virtio devices, guest is supposed to
> > > > > > > > > control the placement of incoming packets in RX queues.
> > > > > > > > 
> > > > > > > > I may not follow you.
> > > > > > > > 
> > > > > > > > Enqueuing packets to a RX queue is done at vhost lib, outside 
> > > > > > > > the
> > > > > > > > guest, how could the guest take the control here?
> > > > > > > > 
> > > > > > > > --yliu
> > > > > > > 
> > > > > > > vhost should do what guest told it to.
> > > > > > > 
> > > > > > > See virtio spec:
> > > > > > >   5.1.6.5.5 Automatic receive steering in multiqueue mode
> > > > > > 
> > > > > > Spec says:
> > > > > > 
> > > > > > After the driver transmitted a packet of a flow on transmitqX,
> > > > > > the device SHOULD cause incoming packets for that flow to be
> > > > > > steered to receiveqX.
> > > > > > 
> > > > > > 
> > > > > > Michael, I still have no idea how vhost could know the flow even
> > > > > > after discussion with Huawei. Could you be more specific about
> > > > > > this? Say, how could guest know that? And how could guest tell
> > > > > > vhost which RX is gonna to use?
> > > > > > 
> > > > > > Thanks.
> > > > > > 
> > > > > > --yliu
> > > > > 
> > > > > I don't really understand the question.
> > > > > 
> > > > > When guests transmits a packet, it makes a decision
> > > > > about the flow to use, and maps that to a tx/rx pair of queues.
> > > > > 
> > > > > It sends packets out on the tx queue and expects device to
> > > > > return packets from the same flow on the rx queue.
> > > > 
> > > > Why? I can understand that there should be a mapping between
> > > > flows and queues in a way that there is no re-ordering, but
> > > > I can't see the relation of receiving a flow with a TX queue.
> > > > 
> > > > fbl
> > > 
> > > That's the way virtio chose to program the rx steering logic.
> > > 
> > > It's low overhead (no special commands), and
> > > works well for TCP when user is an endpoint since rx and tx
> > > for tcp are generally tied (because of ack handling).

It is low overhead for the control plane, but not for the data plane.

> > > We can discuss other ways, e.g. special commands for guests to
> > > program steering.
> > > We'd have to first see some data showing the current scheme
> > > is problematic somehow.

The issue is that the spec assumes the packets are coming in
a serialized way and the distribution will be made by vhost-user
but that isn't necessarily true.


> > The issue is that the restriction imposes operations to be done in the
> > data path.  For instance, Open vSwitch has N number of threads to manage
> > X RX queues. We distribute them in round-robin fashion.  So, the thread
> > polling one RX queue will do all the packet processing and push it to the
> > TX queue of the other device (vhost-user or not) using the same 'id'.
> > 
> > Doing so we can avoid locking between threads and TX queues and any other
> > extra computation while still keeping the packet ordering/distribution fine.
> > 
> > However, if vhost-user has to send packets according with guest mapping,
> > it will require locking between queues and additional operations to select
> > the appropriate queue.  Those actions will cause performance issues.
> 
> You only need to send updates if guest moves a flow to another queue.
> This is very rare since guest must avoid reordering.

OK, maybe I missed something.  Could you point me to the spec talking
about the update?


> Oh and you don't have to have locking.  Just update the table and make
> the target pick up the new value at leasure, worst case a packet ends up
> in the wrong queue.

You do because packets are coming on different vswitch queues and they
could get mapped to the same virtio queue enforced by the guest, so some
sort of synchronization is needed.

That is one thing.  Another is that it will need some mapping between the
hash available in the vswitch (not necessary L2~L4) with the hash/queue
mapping provided by the guest.  That doesn't require locking, but it's a
costly operation.  Alternatively, vswitch could calculate full L2-L4 hash
which is also a costly operation.

Packets ending in the wrong queue isn't that bad, but then we need to
enforce processing order because re-ordering is really bad.


> > I see no real benefit from enforcing the guest mapping outside to
> > justify all the computation cost, so 

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Michael S. Tsirkin]

On Wed, Oct 28, 2015 at 06:30:41PM -0200, Flavio Leitner wrote:
> On Sat, Oct 24, 2015 at 08:47:10PM +0300, Michael S. Tsirkin wrote:
> > On Sat, Oct 24, 2015 at 12:34:08AM -0200, Flavio Leitner wrote:
> > > On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > > > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > > > > Please note that for virtio devices, guest is supposed to
> > > > > > > > control the placement of incoming packets in RX queues.
> > > > > > > 
> > > > > > > I may not follow you.
> > > > > > > 
> > > > > > > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > > > > > > guest, how could the guest take the control here?
> > > > > > > 
> > > > > > >   --yliu
> > > > > > 
> > > > > > vhost should do what guest told it to.
> > > > > > 
> > > > > > See virtio spec:
> > > > > > 5.1.6.5.5 Automatic receive steering in multiqueue mode
> > > > > 
> > > > > Spec says:
> > > > > 
> > > > > After the driver transmitted a packet of a flow on transmitqX,
> > > > > the device SHOULD cause incoming packets for that flow to be
> > > > > steered to receiveqX.
> > > > > 
> > > > > 
> > > > > Michael, I still have no idea how vhost could know the flow even
> > > > > after discussion with Huawei. Could you be more specific about
> > > > > this? Say, how could guest know that? And how could guest tell
> > > > > vhost which RX is gonna to use?
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > >   --yliu
> > > > 
> > > > I don't really understand the question.
> > > > 
> > > > When guests transmits a packet, it makes a decision
> > > > about the flow to use, and maps that to a tx/rx pair of queues.
> > > > 
> > > > It sends packets out on the tx queue and expects device to
> > > > return packets from the same flow on the rx queue.
> > > 
> > > Why? I can understand that there should be a mapping between
> > > flows and queues in a way that there is no re-ordering, but
> > > I can't see the relation of receiving a flow with a TX queue.
> > > 
> > > fbl
> > 
> > That's the way virtio chose to program the rx steering logic.
> > 
> > It's low overhead (no special commands), and
> > works well for TCP when user is an endpoint since rx and tx
> > for tcp are generally tied (because of ack handling).
> > 
> > We can discuss other ways, e.g. special commands for guests to
> > program steering.
> > We'd have to first see some data showing the current scheme
> > is problematic somehow.
> 
> The issue is that the restriction imposes operations to be done in the
> data path.  For instance, Open vSwitch has N number of threads to manage
> X RX queues. We distribute them in round-robin fashion.  So, the thread
> polling one RX queue will do all the packet processing and push it to the
> TX queue of the other device (vhost-user or not) using the same 'id'.
> 
> Doing so we can avoid locking between threads and TX queues and any other
> extra computation while still keeping the packet ordering/distribution fine.
> 
> However, if vhost-user has to send packets according with guest mapping,
> it will require locking between queues and additional operations to select
> the appropriate queue.  Those actions will cause performance issues.


You only need to send updates if guest moves a flow to another queue.
This is very rare since guest must avoid reordering.

Oh and you don't have to have locking.  Just update the table and make
the target pick up the new value at leasure, worst case a packet ends up
in the wrong queue.


> I see no real benefit from enforcing the guest mapping outside to
> justify all the computation cost, so my suggestion is to change the
> spec to suggest that behavior, but not to require that to be compliant.
> 
> Does that make sense?
> 
> Thanks,
> fbl

It's not a question of what the spec says, it's a question of the
quality of implementation: guest needs to be able to balance load
between CPUs serving the queues, this means it needs a way to control
steering.

IMO having dpdk control it makes no sense in the scenario.

This is different from dpdk sending packets to real NIC
queues which all operate in parallel.

-- 
MST

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Flavio Leitner]

On Sat, Oct 24, 2015 at 08:47:10PM +0300, Michael S. Tsirkin wrote:
> On Sat, Oct 24, 2015 at 12:34:08AM -0200, Flavio Leitner wrote:
> > On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > > > Please note that for virtio devices, guest is supposed to
> > > > > > > control the placement of incoming packets in RX queues.
> > > > > > 
> > > > > > I may not follow you.
> > > > > > 
> > > > > > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > > > > > guest, how could the guest take the control here?
> > > > > > 
> > > > > > --yliu
> > > > > 
> > > > > vhost should do what guest told it to.
> > > > > 
> > > > > See virtio spec:
> > > > >   5.1.6.5.5 Automatic receive steering in multiqueue mode
> > > > 
> > > > Spec says:
> > > > 
> > > > After the driver transmitted a packet of a flow on transmitqX,
> > > > the device SHOULD cause incoming packets for that flow to be
> > > > steered to receiveqX.
> > > > 
> > > > 
> > > > Michael, I still have no idea how vhost could know the flow even
> > > > after discussion with Huawei. Could you be more specific about
> > > > this? Say, how could guest know that? And how could guest tell
> > > > vhost which RX is gonna to use?
> > > > 
> > > > Thanks.
> > > > 
> > > > --yliu
> > > 
> > > I don't really understand the question.
> > > 
> > > When guests transmits a packet, it makes a decision
> > > about the flow to use, and maps that to a tx/rx pair of queues.
> > > 
> > > It sends packets out on the tx queue and expects device to
> > > return packets from the same flow on the rx queue.
> > 
> > Why? I can understand that there should be a mapping between
> > flows and queues in a way that there is no re-ordering, but
> > I can't see the relation of receiving a flow with a TX queue.
> > 
> > fbl
> 
> That's the way virtio chose to program the rx steering logic.
> 
> It's low overhead (no special commands), and
> works well for TCP when user is an endpoint since rx and tx
> for tcp are generally tied (because of ack handling).
> 
> We can discuss other ways, e.g. special commands for guests to
> program steering.
> We'd have to first see some data showing the current scheme
> is problematic somehow.

The issue is that the restriction imposes operations to be done in the
data path.  For instance, Open vSwitch has N number of threads to manage
X RX queues. We distribute them in round-robin fashion.  So, the thread
polling one RX queue will do all the packet processing and push it to the
TX queue of the other device (vhost-user or not) using the same 'id'.

Doing so we can avoid locking between threads and TX queues and any other
extra computation while still keeping the packet ordering/distribution fine.

However, if vhost-user has to send packets according with guest mapping,
it will require locking between queues and additional operations to select
the appropriate queue.  Those actions will cause performance issues.

I see no real benefit from enforcing the guest mapping outside to
justify all the computation cost, so my suggestion is to change the
spec to suggest that behavior, but not to require that to be compliant.

Does that make sense?

Thanks,
fbl

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Michael S. Tsirkin]

On Sat, Oct 24, 2015 at 12:34:08AM -0200, Flavio Leitner wrote:
> On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > > Please note that for virtio devices, guest is supposed to
> > > > > > control the placement of incoming packets in RX queues.
> > > > > 
> > > > > I may not follow you.
> > > > > 
> > > > > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > > > > guest, how could the guest take the control here?
> > > > > 
> > > > >   --yliu
> > > > 
> > > > vhost should do what guest told it to.
> > > > 
> > > > See virtio spec:
> > > > 5.1.6.5.5 Automatic receive steering in multiqueue mode
> > > 
> > > Spec says:
> > > 
> > > After the driver transmitted a packet of a flow on transmitqX,
> > > the device SHOULD cause incoming packets for that flow to be
> > > steered to receiveqX.
> > > 
> > > 
> > > Michael, I still have no idea how vhost could know the flow even
> > > after discussion with Huawei. Could you be more specific about
> > > this? Say, how could guest know that? And how could guest tell
> > > vhost which RX is gonna to use?
> > > 
> > > Thanks.
> > > 
> > >   --yliu
> > 
> > I don't really understand the question.
> > 
> > When guests transmits a packet, it makes a decision
> > about the flow to use, and maps that to a tx/rx pair of queues.
> > 
> > It sends packets out on the tx queue and expects device to
> > return packets from the same flow on the rx queue.
> 
> Why? I can understand that there should be a mapping between
> flows and queues in a way that there is no re-ordering, but
> I can't see the relation of receiving a flow with a TX queue.
> 
> fbl

That's the way virtio chose to program the rx steering logic.

It's low overhead (no special commands), and
works well for TCP when user is an endpoint since rx and tx
for tcp are generally tied (because of ack handling).

We can discuss other ways, e.g. special commands for guests to
program steering.
We'd have to first see some data showing the current scheme
is problematic somehow.


> > During transmit, device needs to figure out the flow
> > of packets as they are received from guest, and track
> > which flows go on which tx queue.
> > When it selects the rx queue, it has to use the same table.
> > 
> > There is currently no provision for controlling
> > steering for uni-directional
> > flows which are possible e.g. with UDP.
> > 
> > We might solve this in a future spec - for example, set a flag notifying
> > guest that steering information is missing for a given flow, for example
> > by setting a flag in a packet, or using the command queue, and have
> > guest send a dummy empty packet to set steering rule for this flow.
> > 
> > 
> > -- 
> > MST
> > 

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Flavio Leitner]

On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > Please note that for virtio devices, guest is supposed to
> > > > > control the placement of incoming packets in RX queues.
> > > > 
> > > > I may not follow you.
> > > > 
> > > > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > > > guest, how could the guest take the control here?
> > > > 
> > > > --yliu
> > > 
> > > vhost should do what guest told it to.
> > > 
> > > See virtio spec:
> > >   5.1.6.5.5 Automatic receive steering in multiqueue mode
> > 
> > Spec says:
> > 
> > After the driver transmitted a packet of a flow on transmitqX,
> > the device SHOULD cause incoming packets for that flow to be
> > steered to receiveqX.
> > 
> > 
> > Michael, I still have no idea how vhost could know the flow even
> > after discussion with Huawei. Could you be more specific about
> > this? Say, how could guest know that? And how could guest tell
> > vhost which RX is gonna to use?
> > 
> > Thanks.
> > 
> > --yliu
> 
> I don't really understand the question.
> 
> When guests transmits a packet, it makes a decision
> about the flow to use, and maps that to a tx/rx pair of queues.
> 
> It sends packets out on the tx queue and expects device to
> return packets from the same flow on the rx queue.

Why? I can understand that there should be a mapping between
flows and queues in a way that there is no re-ordering, but
I can't see the relation of receiving a flow with a TX queue.

fbl

> During transmit, device needs to figure out the flow
> of packets as they are received from guest, and track
> which flows go on which tx queue.
> When it selects the rx queue, it has to use the same table.
> 
> There is currently no provision for controlling
> steering for uni-directional
> flows which are possible e.g. with UDP.
> 
> We might solve this in a future spec - for example, set a flag notifying
> guest that steering information is missing for a given flow, for example
> by setting a flag in a packet, or using the command queue, and have
> guest send a dummy empty packet to set steering rule for this flow.
> 
> 
> -- 
> MST
> 

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Yuanhan Liu]

On Thu, Oct 22, 2015 at 05:19:01PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 22, 2015 at 10:07:10PM +0800, Yuanhan Liu wrote:
> > On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > > > Please note that for virtio devices, guest is supposed to
> > > > > > > control the placement of incoming packets in RX queues.
> > > > > > 
> > > > > > I may not follow you.
> > > > > > 
> > > > > > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > > > > > guest, how could the guest take the control here?
> > > > > > 
> > > > > > --yliu
> > > > > 
> > > > > vhost should do what guest told it to.
> > > > > 
> > > > > See virtio spec:
> > > > >   5.1.6.5.5 Automatic receive steering in multiqueue mode
> > > > 
> > > > Spec says:
> > > > 
> > > > After the driver transmitted a packet of a flow on transmitqX,
> > > > the device SHOULD cause incoming packets for that flow to be
> > > > steered to receiveqX.
> > > > 
> > > > 
> > > > Michael, I still have no idea how vhost could know the flow even
> > > > after discussion with Huawei. Could you be more specific about
> > > > this? Say, how could guest know that? And how could guest tell
> > > > vhost which RX is gonna to use?
> > > > 
> > > > Thanks.
> > > > 
> > > > --yliu
> > > 
> > > I don't really understand the question.
> > > 
> > > When guests transmits a packet, it makes a decision
> > > about the flow to use, and maps that to a tx/rx pair of queues.
> > > 
> > > It sends packets out on the tx queue and expects device to
> > > return packets from the same flow on the rx queue.
> > > 
> > > During transmit, device needs to figure out the flow
> > > of packets as they are received from guest, and track
> > > which flows go on which tx queue.
> > > When it selects the rx queue, it has to use the same table.
> > 
> > Thanks for the length explanation, Michael!
> > 
> > I guess the key is are we able to get the table inside vhost-user
> > lib? And, are you looking for something like following?
> > 
> > static int rte_vhost_enqueue_burst(pkts)
> > {
> > for_each_pkts(pkt) {
> > int rxq = get_rxq_from_table(pkt);
> > 
> > queue_to_rxq(pkt, rxq);
> > }
> > }
> > 
> > BTW, there should be such implementation at some where, right?
> > If so, would you please point it to me?
> 
> See tun_flow_update in drivers/net/tun.c in Linux.

Thanks.

We had a discussion today, and we need implement that. However, the
v2.2 merge window is pretty near the end so far. So it's unlikely we
could make it in this release. We may add it in v2.3.


--yliu
> 
> 
> > In the meanwhile, I will read more doc/code to try to understand
> > it.
> > 
> > --yliu
> > 
> > > 
> > > There is currently no provision for controlling
> > > steering for uni-directional
> > > flows which are possible e.g. with UDP.
> > > 
> > > We might solve this in a future spec - for example, set a flag notifying
> > > guest that steering information is missing for a given flow, for example
> > > by setting a flag in a packet, or using the command queue, and have
> > > guest send a dummy empty packet to set steering rule for this flow.
> > > 
> > > 
> > > -- 
> > > MST

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Yuanhan Liu]

On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > Please note that for virtio devices, guest is supposed to
> > > > > control the placement of incoming packets in RX queues.
> > > > 
> > > > I may not follow you.
> > > > 
> > > > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > > > guest, how could the guest take the control here?
> > > > 
> > > > --yliu
> > > 
> > > vhost should do what guest told it to.
> > > 
> > > See virtio spec:
> > >   5.1.6.5.5 Automatic receive steering in multiqueue mode
> > 
> > Spec says:
> > 
> > After the driver transmitted a packet of a flow on transmitqX,
> > the device SHOULD cause incoming packets for that flow to be
> > steered to receiveqX.
> > 
> > 
> > Michael, I still have no idea how vhost could know the flow even
> > after discussion with Huawei. Could you be more specific about
> > this? Say, how could guest know that? And how could guest tell
> > vhost which RX is gonna to use?
> > 
> > Thanks.
> > 
> > --yliu
> 
> I don't really understand the question.
> 
> When guests transmits a packet, it makes a decision
> about the flow to use, and maps that to a tx/rx pair of queues.
> 
> It sends packets out on the tx queue and expects device to
> return packets from the same flow on the rx queue.
> 
> During transmit, device needs to figure out the flow
> of packets as they are received from guest, and track
> which flows go on which tx queue.
> When it selects the rx queue, it has to use the same table.

Thanks for the length explanation, Michael!

I guess the key is are we able to get the table inside vhost-user
lib? And, are you looking for something like following?

static int rte_vhost_enqueue_burst(pkts)
{
for_each_pkts(pkt) {
int rxq = get_rxq_from_table(pkt);

queue_to_rxq(pkt, rxq);
}
}

BTW, there should be such implementation at some where, right?
If so, would you please point it to me?

In the meanwhile, I will read more doc/code to try to understand
it.

--yliu

> 
> There is currently no provision for controlling
> steering for uni-directional
> flows which are possible e.g. with UDP.
> 
> We might solve this in a future spec - for example, set a flag notifying
> guest that steering information is missing for a given flow, for example
> by setting a flag in a packet, or using the command queue, and have
> guest send a dummy empty packet to set steering rule for this flow.
> 
> 
> -- 
> MST

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Yuanhan Liu]

On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > Please note that for virtio devices, guest is supposed to
> > > control the placement of incoming packets in RX queues.
> > 
> > I may not follow you.
> > 
> > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > guest, how could the guest take the control here?
> > 
> > --yliu
> 
> vhost should do what guest told it to.
> 
> See virtio spec:
>   5.1.6.5.5 Automatic receive steering in multiqueue mode

Spec says:

After the driver transmitted a packet of a flow on transmitqX,
the device SHOULD cause incoming packets for that flow to be
steered to receiveqX.


Michael, I still have no idea how vhost could know the flow even
after discussion with Huawei. Could you be more specific about
this? Say, how could guest know that? And how could guest tell
vhost which RX is gonna to use?

Thanks.

--yliu

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Michael S. Tsirkin]

On Thu, Oct 22, 2015 at 10:07:10PM +0800, Yuanhan Liu wrote:
> On Thu, Oct 22, 2015 at 02:32:31PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> > > On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > > > Please note that for virtio devices, guest is supposed to
> > > > > > control the placement of incoming packets in RX queues.
> > > > > 
> > > > > I may not follow you.
> > > > > 
> > > > > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > > > > guest, how could the guest take the control here?
> > > > > 
> > > > >   --yliu
> > > > 
> > > > vhost should do what guest told it to.
> > > > 
> > > > See virtio spec:
> > > > 5.1.6.5.5 Automatic receive steering in multiqueue mode
> > > 
> > > Spec says:
> > > 
> > > After the driver transmitted a packet of a flow on transmitqX,
> > > the device SHOULD cause incoming packets for that flow to be
> > > steered to receiveqX.
> > > 
> > > 
> > > Michael, I still have no idea how vhost could know the flow even
> > > after discussion with Huawei. Could you be more specific about
> > > this? Say, how could guest know that? And how could guest tell
> > > vhost which RX is gonna to use?
> > > 
> > > Thanks.
> > > 
> > >   --yliu
> > 
> > I don't really understand the question.
> > 
> > When guests transmits a packet, it makes a decision
> > about the flow to use, and maps that to a tx/rx pair of queues.
> > 
> > It sends packets out on the tx queue and expects device to
> > return packets from the same flow on the rx queue.
> > 
> > During transmit, device needs to figure out the flow
> > of packets as they are received from guest, and track
> > which flows go on which tx queue.
> > When it selects the rx queue, it has to use the same table.
> 
> Thanks for the length explanation, Michael!
> 
> I guess the key is are we able to get the table inside vhost-user
> lib? And, are you looking for something like following?
> 
>   static int rte_vhost_enqueue_burst(pkts)
>   {
>   for_each_pkts(pkt) {
>   int rxq = get_rxq_from_table(pkt);
>   
>   queue_to_rxq(pkt, rxq);
>   }
>   }
> 
> BTW, there should be such implementation at some where, right?
> If so, would you please point it to me?

See tun_flow_update in drivers/net/tun.c in Linux.


> In the meanwhile, I will read more doc/code to try to understand
> it.
> 
>   --yliu
> 
> > 
> > There is currently no provision for controlling
> > steering for uni-directional
> > flows which are possible e.g. with UDP.
> > 
> > We might solve this in a future spec - for example, set a flag notifying
> > guest that steering information is missing for a given flow, for example
> > by setting a flag in a packet, or using the command queue, and have
> > guest send a dummy empty packet to set steering rule for this flow.
> > 
> > 
> > -- 
> > MST

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Michael S. Tsirkin]

On Thu, Oct 22, 2015 at 05:49:55PM +0800, Yuanhan Liu wrote:
> On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > > Please note that for virtio devices, guest is supposed to
> > > > control the placement of incoming packets in RX queues.
> > > 
> > > I may not follow you.
> > > 
> > > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > > guest, how could the guest take the control here?
> > > 
> > >   --yliu
> > 
> > vhost should do what guest told it to.
> > 
> > See virtio spec:
> > 5.1.6.5.5 Automatic receive steering in multiqueue mode
> 
> Spec says:
> 
> After the driver transmitted a packet of a flow on transmitqX,
> the device SHOULD cause incoming packets for that flow to be
> steered to receiveqX.
> 
> 
> Michael, I still have no idea how vhost could know the flow even
> after discussion with Huawei. Could you be more specific about
> this? Say, how could guest know that? And how could guest tell
> vhost which RX is gonna to use?
> 
> Thanks.
> 
>   --yliu

I don't really understand the question.

When guests transmits a packet, it makes a decision
about the flow to use, and maps that to a tx/rx pair of queues.

It sends packets out on the tx queue and expects device to
return packets from the same flow on the rx queue.

During transmit, device needs to figure out the flow
of packets as they are received from guest, and track
which flows go on which tx queue.
When it selects the rx queue, it has to use the same table.

There is currently no provision for controlling
steering for uni-directional
flows which are possible e.g. with UDP.

We might solve this in a future spec - for example, set a flag notifying
guest that steering information is missing for a given flow, for example
by setting a flag in a packet, or using the command queue, and have
guest send a dummy empty packet to set steering rule for this flow.


-- 
MST

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Xie, Huawei]

On 10/21/2015 11:48 AM, Yuanhan Liu wrote:

[...]
>  
>  #define MAX_PKT_BURST 32
>  
> +static inline int __attribute__((always_inline))
> +is_valid_virt_queue_idx(uint32_t virtq_idx, int is_tx, uint32_t max_qp_idx)
> +{
> + if ((is_tx ^ (virtq_idx & 0x1)) ||
> + (virtq_idx >= max_qp_idx * VIRTIO_QNUM))
> + return 0;
> +
> + return 1;
> +}
> +
>  /**
>   * This function adds buffers to the virtio devices RX virtqueue. Buffers can
>   * be received from the physical port or from another virtio device. A packet
> @@ -68,12 +78,14 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>   uint8_t success = 0;
>  
>   LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_rx()\n", dev->device_fh);
> - if (unlikely(queue_id != VIRTIO_RXQ)) {
> - LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
> + if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
> + RTE_LOG(ERR, VHOST_DATA,
> + "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
> + __func__, dev->device_fh, queue_id);
>   return 0;
>   }
>  
> - vq = dev->virtqueue[VIRTIO_RXQ];
> + vq = dev->virtqueue[queue_id];
>   count = (count > MAX_PKT_BURST) ? MAX_PKT_BURST : count;
>  
>   /*
>
Besides the always_inline issue, i think we should remove the queue_id
check here in the "data" path. Caller should guarantee that they pass us
the correct queue idx.
We could add VHOST_DEBUG macro for the sanity check for debug purpose only.

On the other hand, currently we lack of enough check for the guest
because there could be malicious guests. Plan to fix this in next release.

[...]

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Yuanhan Liu]

On Wed, Oct 21, 2015 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 21, 2015 at 08:48:15PM +0800, Yuanhan Liu wrote:
> > > Please note that for virtio devices, guest is supposed to
> > > control the placement of incoming packets in RX queues.
> > 
> > I may not follow you.
> > 
> > Enqueuing packets to a RX queue is done at vhost lib, outside the
> > guest, how could the guest take the control here?
> > 
> > --yliu
> 
> vhost should do what guest told it to.
> 
> See virtio spec:
>   5.1.6.5.5 Automatic receive steering in multiqueue mode


Thanks for the info. I'll have a look tomorrow.

--yliu

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Yuanhan Liu]

On Wed, Oct 21, 2015 at 01:31:55PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 21, 2015 at 11:48:10AM +0800, Yuanhan Liu wrote:
> > From: Changchun Ouyang 
> > 
> > Do not use VIRTIO_RXQ or VIRTIO_TXQ anymore; use the queue_id
> > instead, which will be set to a proper value for a specific queue
> > when we have multiple queue support enabled.
> > 
> > For now, queue_id is still set with VIRTIO_RXQ or VIRTIO_TXQ,
> > so it should not break anything.
> > 
> > Signed-off-by: Changchun Ouyang 
> > Signed-off-by: Yuanhan Liu 
> > Acked-by: Flavio Leitner 
> 
> I tried to figure out how is queue_id set and I couldn't.

queue_id is set outside the DPDK library, it's up to application
to select a queue. There was a demo (examples/vhost/vhost-switch)
before, and it was removed. (check the cover letter for the reason).

> Please note that for virtio devices, guest is supposed to
> control the placement of incoming packets in RX queues.

I may not follow you.

Enqueuing packets to a RX queue is done at vhost lib, outside the
guest, how could the guest take the control here?

--yliu

> > ---
> > 
> > v7: commit title fix
> > ---
> >  lib/librte_vhost/vhost_rxtx.c | 46 
> > ++-
> >  1 file changed, 32 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> > index 7026bfa..14e00ef 100644
> > --- a/lib/librte_vhost/vhost_rxtx.c
> > +++ b/lib/librte_vhost/vhost_rxtx.c
> > @@ -42,6 +42,16 @@
> >  
> >  #define MAX_PKT_BURST 32
> >  
> > +static inline int __attribute__((always_inline))
> > +is_valid_virt_queue_idx(uint32_t virtq_idx, int is_tx, uint32_t max_qp_idx)
> > +{
> > +   if ((is_tx ^ (virtq_idx & 0x1)) ||
> > +   (virtq_idx >= max_qp_idx * VIRTIO_QNUM))
> > +   return 0;
> > +
> > +   return 1;
> > +}
> > +
> >  /**
> >   * This function adds buffers to the virtio devices RX virtqueue. Buffers 
> > can
> >   * be received from the physical port or from another virtio device. A 
> > packet
> > @@ -68,12 +78,14 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> > uint8_t success = 0;
> >  
> > LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_rx()\n", dev->device_fh);
> > -   if (unlikely(queue_id != VIRTIO_RXQ)) {
> > -   LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
> > +   if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
> > +   RTE_LOG(ERR, VHOST_DATA,
> > +   "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
> > +   __func__, dev->device_fh, queue_id);
> > return 0;
> > }
> >  
> > -   vq = dev->virtqueue[VIRTIO_RXQ];
> > +   vq = dev->virtqueue[queue_id];
> > count = (count > MAX_PKT_BURST) ? MAX_PKT_BURST : count;
> >  
> > /*
> > @@ -235,8 +247,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> >  }
> >  
> >  static inline uint32_t __attribute__((always_inline))
> > -copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t res_base_idx,
> > -   uint16_t res_end_idx, struct rte_mbuf *pkt)
> > +copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
> > +   uint16_t res_base_idx, uint16_t res_end_idx,
> > +   struct rte_mbuf *pkt)
> >  {
> > uint32_t vec_idx = 0;
> > uint32_t entry_success = 0;
> > @@ -264,7 +277,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, 
> > uint16_t res_base_idx,
> >  * Convert from gpa to vva
> >  * (guest physical addr -> vhost virtual addr)
> >  */
> > -   vq = dev->virtqueue[VIRTIO_RXQ];
> > +   vq = dev->virtqueue[queue_id];
> > vb_addr = gpa_to_vva(dev, vq->buf_vec[vec_idx].buf_addr);
> > vb_hdr_addr = vb_addr;
> >  
> > @@ -464,11 +477,14 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t 
> > queue_id,
> >  
> > LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_merge_rx()\n",
> > dev->device_fh);
> > -   if (unlikely(queue_id != VIRTIO_RXQ)) {
> > -   LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
> > +   if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
> > +   RTE_LOG(ERR, VHOST_DATA,
> > +   "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
> > +   __func__, dev->device_fh, queue_id);
> > +   return 0;
> > }
> >  
> > -   vq = dev->virtqueue[VIRTIO_RXQ];
> > +   vq = dev->virtqueue[queue_id];
> > count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
> >  
> > if (count == 0)
> > @@ -509,8 +525,8 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t 
> > queue_id,
> > res_cur_idx);
> > } while (success == 0);
> >  
> > -   entry_success = copy_from_mbuf_to_vring(dev, res_base_idx,
> > -   res_cur_idx, pkts[pkt_idx]);
> > +   entry_success = copy_from_mbuf_to_vring(dev, queue_id,
> > +   res_base_idx, res_cur_idx, 

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Thomas Monjalon]

2015-10-21 08:47, Stephen Hemminger:
> On Wed, 21 Oct 2015 09:38:37 +
> "Ananyev, Konstantin"  wrote:
> > I also don't understand what's wrong with using 'always_inline' here.
> > As I understand the author wants compiler to *always inline* that function.
> > So seems perfectly ok to use it here.
> > As I remember just 'inline' is sort of recommendation that compiler is free 
> > to ignore.
> > Konstantin 
> 
> I follow Linux/Linus advice and resist the urge to add strong inlining.
> The compiler does a good job of deciding to inline, and many times
> the reason it chooses for not inlining are quite good like:
>   - the code is on an unlikely branch
>   - register pressure means inlining would mean the code would be worse
> 
> Therefore my rules are:
>   * only use inline for small functions. Let compiler decide on larger static 
> funcs
>   * write code where most functions are static (localized scope) where 
> compiler
> can decide
>   * reserve always inline for things that access hardware and would break if 
> not inlined.

It would be interesting to do some benchmarks with/without "always" keyword
and add these rules in the coding style guide.

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Bruce Richardson]

On 21/10/2015 16:47, Stephen Hemminger wrote:
> On Wed, 21 Oct 2015 09:38:37 +
> "Ananyev, Konstantin"  wrote:
>
 minor nits:
   * this doesn't need to be marked as always inline,
  that is as they say in English "shooting a fly with a bazooka"
>>> Stephen:
>>> always_inline "forces" the compiler to inline this function, like a macro.
>>> When should it be used or is it not preferred at all?
>> I also don't understand what's wrong with using 'always_inline' here.
>> As I understand the author wants compiler to *always inline* that function.
>> So seems perfectly ok to use it here.
>> As I remember just 'inline' is sort of recommendation that compiler is free 
>> to ignore.
>> Konstantin
> I follow Linux/Linus advice and resist the urge to add strong inlining.
> The compiler does a good job of deciding to inline, and many times
> the reason it chooses for not inlining are quite good like:
>- the code is on an unlikely branch
>- register pressure means inlining would mean the code would be worse
>
> Therefore my rules are:
>* only use inline for small functions. Let compiler decide on larger 
> static funcs
>* write code where most functions are static (localized scope) where 
> compiler
>  can decide
>* reserve always inline for things that access hardware and would break if 
> not inlined.
>
On the other hand, there are cases where we know the compiler will 
likely inline, but we also know that not inlining could have a high 
performance penalty, and in that case marking as "always inline" would 
be appropriate - even though it is likely unnecessary for most 
compilers. In such a case, I would expect the verification check to be: 
explicitly mark the function as *not* to be inlined, and see what the 
perf drop is. If it's a noticable drop, marking as always-inline is an 
ok precaution against future compiler changes.

Also, we need to remember that compilers cannot know whether a function 
is data path or not, and also whether a function will be called 
per-packet or per-burst. That's only something the programmer will know, 
and functions called per-packet on the datapath generally need to be 
inlined for performance.

/Bruce

/Bruce

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Ananyev, Konstantin]

> -Original Message-
> From: Richardson, Bruce
> Sent: Wednesday, October 21, 2015 4:56 PM
> To: Stephen Hemminger; Ananyev, Konstantin
> Cc: Michael S. Tsirkin; dev at dpdk.org; marcel at redhat.com; Changchun 
> Ouyang
> Subject: Re: [dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of 
> constant ring index
> 
> 
> 
> On 21/10/2015 16:47, Stephen Hemminger wrote:
> > On Wed, 21 Oct 2015 09:38:37 +
> > "Ananyev, Konstantin"  wrote:
> >
> >>>> minor nits:
> >>>>   * this doesn't need to be marked as always inline,
> >>>>  that is as they say in English "shooting a fly with a bazooka"
> >>> Stephen:
> >>> always_inline "forces" the compiler to inline this function, like a macro.
> >>> When should it be used or is it not preferred at all?
> >> I also don't understand what's wrong with using 'always_inline' here.
> >> As I understand the author wants compiler to *always inline* that function.
> >> So seems perfectly ok to use it here.
> >> As I remember just 'inline' is sort of recommendation that compiler is 
> >> free to ignore.
> >> Konstantin
> > I follow Linux/Linus advice and resist the urge to add strong inlining.
> > The compiler does a good job of deciding to inline, and many times
> > the reason it chooses for not inlining are quite good like:
> >- the code is on an unlikely branch
> >- register pressure means inlining would mean the code would be worse

Yep, that's all true, but as I remember 'inline' keyword itself doesn't force 
compiler to
always inline that function. It is more like a recommendation to the compiler.
Looking at any dpdk binary, there are plenty of places where function is 
declared as 'inline',
but compiler decided not to and followed standard function call convention for 
it.
Again, from C spec:
"6. A function declared with an inline function specifier is an inline 
function. Making a
function an inline function suggests that calls to the function be as fast as 
possible.138)
7. The extent to which such suggestions are effective is 
implementation-defined.139) 
...
139) For example, an implementation might never perform inline substitution, or 
might only perform inline
substitutions to calls in the scope of an inline declaration."

> >
> > Therefore my rules are:
> >* only use inline for small functions. Let compiler decide on larger 
> > static funcs

As I remember function we are talking about is really small.

> >* write code where most functions are static (localized scope) where 
> > compiler
> >  can decide
> >* reserve always inline for things that access hardware and would break 
> > if not inlined.

Sorry, but the latest rule looks too restrictive to me.
Don't see any reason why we all have to follow it.
BTW, as I can see there are plenty of always_inline functions inside linux 
kernel
(memory allocator, scheduler, etc).  

> >
> On the other hand, there are cases where we know the compiler will
> likely inline, but we also know that not inlining could have a high
> performance penalty, and in that case marking as "always inline" would
> be appropriate - even though it is likely unnecessary for most
> compilers.

Yep, totally agree here.
If memory serves me right - in the past we observed few noticeable performance 
drops
because of that when switching from one compiler version to another. 

Konstantin


 In such a case, I would expect the verification check to be:
> explicitly mark the function as *not* to be inlined, and see what the
> perf drop is. If it's a noticable drop, marking as always-inline is an
> ok precaution against future compiler changes.
> 
> Also, we need to remember that compilers cannot know whether a function
> is data path or not, and also whether a function will be called
> per-packet or per-burst. That's only something the programmer will know,
> and functions called per-packet on the datapath generally need to be
> inlined for performance.
> 
> /Bruce
> 
> /Bruce

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Yuanhan Liu]

On Tue, Oct 20, 2015 at 09:43:54PM -0700, Stephen Hemminger wrote:
> On Wed, 21 Oct 2015 11:48:10 +0800
> Yuanhan Liu  wrote:
> 
> >  
> > +static inline int __attribute__((always_inline))
> > +is_valid_virt_queue_idx(uint32_t virtq_idx, int is_tx, uint32_t max_qp_idx)
> > +{
> > +   if ((is_tx ^ (virtq_idx & 0x1)) ||
> > +   (virtq_idx >= max_qp_idx * VIRTIO_QNUM))
> > +   return 0;
> > +
> > +   return 1;
> > +}
> 
> minor nits:
>  * this doesn't need to be marked as always inline, 
> that is as they say in English "shooting a fly with a bazooka"
>  * prefer to just return logical result rather than have conditional:
>  * for booleans prefer the  type boolean.
> 
> static bool
> is_valid_virt_queue_idx(uint32_t virtq_idx, bool is_tx, uint32_t max_qp_idx)
> {
>   return (is_tx ^ (virtq_idx & 1)) || 
>   virtq_idx >= max_qp_idx * VIRTIO_QNUM;
> }

Thanks for review, and here you go:


-- >8 --
>From 0be45f37c63c86e93b9caf751ceaeec0a4e66fa5 Mon Sep 17 00:00:00 2001
From: Changchun Ouyang 
Date: Wed, 16 Sep 2015 15:40:32 +0800
Subject: [PATCH] vhost: rxtx: use queue id instead of constant ring index

Do not use VIRTIO_RXQ or VIRTIO_TXQ anymore; use the queue_id
instead, which will be set to a proper value for a specific queue
when we have multiple queue support enabled.

For now, queue_id is still set with VIRTIO_RXQ or VIRTIO_TXQ,
so it should not break anything.

Signed-off-by: Changchun Ouyang 
Signed-off-by: Yuanhan Liu 
Acked-by: Flavio Leitner 

---

v8: simplify is_valid_vrit_queue_idx()

v7: commit title fix
---
 lib/librte_vhost/vhost_rxtx.c | 43 +--
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 7026bfa..1ec8850 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -32,6 +32,7 @@
  */

 #include 
+#include 
 #include 

 #include 
@@ -42,6 +43,12 @@

 #define MAX_PKT_BURST 32

+static bool
+is_valid_virt_queue_idx(uint32_t idx, int is_tx, uint32_t qp_nb)
+{
+   return (is_tx ^ (idx & 1)) == 0 && idx < qp_nb * VIRTIO_QNUM;
+}
+
 /**
  * This function adds buffers to the virtio devices RX virtqueue. Buffers can
  * be received from the physical port or from another virtio device. A packet
@@ -68,12 +75,14 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
uint8_t success = 0;

LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_rx()\n", dev->device_fh);
-   if (unlikely(queue_id != VIRTIO_RXQ)) {
-   LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
+   if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
+   RTE_LOG(ERR, VHOST_DATA,
+   "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
+   __func__, dev->device_fh, queue_id);
return 0;
}

-   vq = dev->virtqueue[VIRTIO_RXQ];
+   vq = dev->virtqueue[queue_id];
count = (count > MAX_PKT_BURST) ? MAX_PKT_BURST : count;

/*
@@ -235,8 +244,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 }

 static inline uint32_t __attribute__((always_inline))
-copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t res_base_idx,
-   uint16_t res_end_idx, struct rte_mbuf *pkt)
+copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
+   uint16_t res_base_idx, uint16_t res_end_idx,
+   struct rte_mbuf *pkt)
 {
uint32_t vec_idx = 0;
uint32_t entry_success = 0;
@@ -264,7 +274,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t 
res_base_idx,
 * Convert from gpa to vva
 * (guest physical addr -> vhost virtual addr)
 */
-   vq = dev->virtqueue[VIRTIO_RXQ];
+   vq = dev->virtqueue[queue_id];
vb_addr = gpa_to_vva(dev, vq->buf_vec[vec_idx].buf_addr);
vb_hdr_addr = vb_addr;

@@ -464,11 +474,14 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t 
queue_id,

LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_merge_rx()\n",
dev->device_fh);
-   if (unlikely(queue_id != VIRTIO_RXQ)) {
-   LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
+   if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
+   RTE_LOG(ERR, VHOST_DATA,
+   "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
+   __func__, dev->device_fh, queue_id);
+   return 0;
}

-   vq = dev->virtqueue[VIRTIO_RXQ];
+   vq = dev->virtqueue[queue_id];
count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);

if (count == 0)
@@ -509,8 +522,8 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t 
queue_id,
res_cur_idx);
} while (success == 0);

-   entry_success = 

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Michael S. Tsirkin]

On Wed, Oct 21, 2015 at 11:48:10AM +0800, Yuanhan Liu wrote:
> From: Changchun Ouyang 
> 
> Do not use VIRTIO_RXQ or VIRTIO_TXQ anymore; use the queue_id
> instead, which will be set to a proper value for a specific queue
> when we have multiple queue support enabled.
> 
> For now, queue_id is still set with VIRTIO_RXQ or VIRTIO_TXQ,
> so it should not break anything.
> 
> Signed-off-by: Changchun Ouyang 
> Signed-off-by: Yuanhan Liu 
> Acked-by: Flavio Leitner 

I tried to figure out how is queue_id set and I couldn't.
Please note that for virtio devices, guest is supposed to
control the placement of incoming packets in RX queues.


> ---
> 
> v7: commit title fix
> ---
>  lib/librte_vhost/vhost_rxtx.c | 46 
> ++-
>  1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> index 7026bfa..14e00ef 100644
> --- a/lib/librte_vhost/vhost_rxtx.c
> +++ b/lib/librte_vhost/vhost_rxtx.c
> @@ -42,6 +42,16 @@
>  
>  #define MAX_PKT_BURST 32
>  
> +static inline int __attribute__((always_inline))
> +is_valid_virt_queue_idx(uint32_t virtq_idx, int is_tx, uint32_t max_qp_idx)
> +{
> + if ((is_tx ^ (virtq_idx & 0x1)) ||
> + (virtq_idx >= max_qp_idx * VIRTIO_QNUM))
> + return 0;
> +
> + return 1;
> +}
> +
>  /**
>   * This function adds buffers to the virtio devices RX virtqueue. Buffers can
>   * be received from the physical port or from another virtio device. A packet
> @@ -68,12 +78,14 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>   uint8_t success = 0;
>  
>   LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_rx()\n", dev->device_fh);
> - if (unlikely(queue_id != VIRTIO_RXQ)) {
> - LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
> + if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
> + RTE_LOG(ERR, VHOST_DATA,
> + "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
> + __func__, dev->device_fh, queue_id);
>   return 0;
>   }
>  
> - vq = dev->virtqueue[VIRTIO_RXQ];
> + vq = dev->virtqueue[queue_id];
>   count = (count > MAX_PKT_BURST) ? MAX_PKT_BURST : count;
>  
>   /*
> @@ -235,8 +247,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>  }
>  
>  static inline uint32_t __attribute__((always_inline))
> -copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t res_base_idx,
> - uint16_t res_end_idx, struct rte_mbuf *pkt)
> +copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
> + uint16_t res_base_idx, uint16_t res_end_idx,
> + struct rte_mbuf *pkt)
>  {
>   uint32_t vec_idx = 0;
>   uint32_t entry_success = 0;
> @@ -264,7 +277,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t 
> res_base_idx,
>* Convert from gpa to vva
>* (guest physical addr -> vhost virtual addr)
>*/
> - vq = dev->virtqueue[VIRTIO_RXQ];
> + vq = dev->virtqueue[queue_id];
>   vb_addr = gpa_to_vva(dev, vq->buf_vec[vec_idx].buf_addr);
>   vb_hdr_addr = vb_addr;
>  
> @@ -464,11 +477,14 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t 
> queue_id,
>  
>   LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_merge_rx()\n",
>   dev->device_fh);
> - if (unlikely(queue_id != VIRTIO_RXQ)) {
> - LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
> + if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
> + RTE_LOG(ERR, VHOST_DATA,
> + "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
> + __func__, dev->device_fh, queue_id);
> + return 0;
>   }
>  
> - vq = dev->virtqueue[VIRTIO_RXQ];
> + vq = dev->virtqueue[queue_id];
>   count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
>  
>   if (count == 0)
> @@ -509,8 +525,8 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t 
> queue_id,
>   res_cur_idx);
>   } while (success == 0);
>  
> - entry_success = copy_from_mbuf_to_vring(dev, res_base_idx,
> - res_cur_idx, pkts[pkt_idx]);
> + entry_success = copy_from_mbuf_to_vring(dev, queue_id,
> + res_base_idx, res_cur_idx, pkts[pkt_idx]);
>  
>   rte_compiler_barrier();
>  
> @@ -562,12 +578,14 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, 
> uint16_t queue_id,
>   uint16_t free_entries, entry_success = 0;
>   uint16_t avail_idx;
>  
> - if (unlikely(queue_id != VIRTIO_TXQ)) {
> - LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
> + if (unlikely(!is_valid_virt_queue_idx(queue_id, 1, dev->virt_qp_nb))) {
> + RTE_LOG(ERR, VHOST_DATA,
> + "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
> +  

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Yuanhan Liu]

From: Changchun Ouyang 

Do not use VIRTIO_RXQ or VIRTIO_TXQ anymore; use the queue_id
instead, which will be set to a proper value for a specific queue
when we have multiple queue support enabled.

For now, queue_id is still set with VIRTIO_RXQ or VIRTIO_TXQ,
so it should not break anything.

Signed-off-by: Changchun Ouyang 
Signed-off-by: Yuanhan Liu 
Acked-by: Flavio Leitner 

---

v7: commit title fix
---
 lib/librte_vhost/vhost_rxtx.c | 46 ++-
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 7026bfa..14e00ef 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -42,6 +42,16 @@

 #define MAX_PKT_BURST 32

+static inline int __attribute__((always_inline))
+is_valid_virt_queue_idx(uint32_t virtq_idx, int is_tx, uint32_t max_qp_idx)
+{
+   if ((is_tx ^ (virtq_idx & 0x1)) ||
+   (virtq_idx >= max_qp_idx * VIRTIO_QNUM))
+   return 0;
+
+   return 1;
+}
+
 /**
  * This function adds buffers to the virtio devices RX virtqueue. Buffers can
  * be received from the physical port or from another virtio device. A packet
@@ -68,12 +78,14 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
uint8_t success = 0;

LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_rx()\n", dev->device_fh);
-   if (unlikely(queue_id != VIRTIO_RXQ)) {
-   LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
+   if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
+   RTE_LOG(ERR, VHOST_DATA,
+   "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
+   __func__, dev->device_fh, queue_id);
return 0;
}

-   vq = dev->virtqueue[VIRTIO_RXQ];
+   vq = dev->virtqueue[queue_id];
count = (count > MAX_PKT_BURST) ? MAX_PKT_BURST : count;

/*
@@ -235,8 +247,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 }

 static inline uint32_t __attribute__((always_inline))
-copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t res_base_idx,
-   uint16_t res_end_idx, struct rte_mbuf *pkt)
+copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
+   uint16_t res_base_idx, uint16_t res_end_idx,
+   struct rte_mbuf *pkt)
 {
uint32_t vec_idx = 0;
uint32_t entry_success = 0;
@@ -264,7 +277,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t 
res_base_idx,
 * Convert from gpa to vva
 * (guest physical addr -> vhost virtual addr)
 */
-   vq = dev->virtqueue[VIRTIO_RXQ];
+   vq = dev->virtqueue[queue_id];
vb_addr = gpa_to_vva(dev, vq->buf_vec[vec_idx].buf_addr);
vb_hdr_addr = vb_addr;

@@ -464,11 +477,14 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t 
queue_id,

LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_merge_rx()\n",
dev->device_fh);
-   if (unlikely(queue_id != VIRTIO_RXQ)) {
-   LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
+   if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->virt_qp_nb))) {
+   RTE_LOG(ERR, VHOST_DATA,
+   "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
+   __func__, dev->device_fh, queue_id);
+   return 0;
}

-   vq = dev->virtqueue[VIRTIO_RXQ];
+   vq = dev->virtqueue[queue_id];
count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);

if (count == 0)
@@ -509,8 +525,8 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t 
queue_id,
res_cur_idx);
} while (success == 0);

-   entry_success = copy_from_mbuf_to_vring(dev, res_base_idx,
-   res_cur_idx, pkts[pkt_idx]);
+   entry_success = copy_from_mbuf_to_vring(dev, queue_id,
+   res_base_idx, res_cur_idx, pkts[pkt_idx]);

rte_compiler_barrier();

@@ -562,12 +578,14 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t 
queue_id,
uint16_t free_entries, entry_success = 0;
uint16_t avail_idx;

-   if (unlikely(queue_id != VIRTIO_TXQ)) {
-   LOG_DEBUG(VHOST_DATA, "mq isn't supported in this version.\n");
+   if (unlikely(!is_valid_virt_queue_idx(queue_id, 1, dev->virt_qp_nb))) {
+   RTE_LOG(ERR, VHOST_DATA,
+   "%s (%"PRIu64"): virtqueue idx:%d invalid.\n",
+   __func__, dev->device_fh, queue_id);
return 0;
}

-   vq = dev->virtqueue[VIRTIO_TXQ];
+   vq = dev->virtqueue[queue_id];
avail_idx =  *((volatile uint16_t *)>avail->idx);

/* If there are no available buffers then return. */
-- 
1.9.0

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Ananyev, Konstantin]

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Xie, Huawei
> Sent: Wednesday, October 21, 2015 8:16 AM
> To: Stephen Hemminger; Yuanhan Liu
> Cc: dev at dpdk.org; marcel at redhat.com; Michael S. Tsirkin; Changchun 
> Ouyang
> Subject: Re: [dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of 
> constant ring index
> 
> On 10/21/2015 12:44 PM, Stephen Hemminger wrote:
> > On Wed, 21 Oct 2015 11:48:10 +0800
> > Yuanhan Liu  wrote:
> >
> >>
> >> +static inline int __attribute__((always_inline))
> >> +is_valid_virt_queue_idx(uint32_t virtq_idx, int is_tx, uint32_t 
> >> max_qp_idx)
> >> +{
> >> +  if ((is_tx ^ (virtq_idx & 0x1)) ||
> >> +  (virtq_idx >= max_qp_idx * VIRTIO_QNUM))
> >> +  return 0;
> >> +
> >> +  return 1;
> >> +}
> > minor nits:
> >  * this doesn't need to be marked as always inline,
> > that is as they say in English "shooting a fly with a bazooka"
> Stephen:
> always_inline "forces" the compiler to inline this function, like a macro.
> When should it be used or is it not preferred at all?

I also don't understand what's wrong with using 'always_inline' here.
As I understand the author wants compiler to *always inline* that function.
So seems perfectly ok to use it here.
As I remember just 'inline' is sort of recommendation that compiler is free to 
ignore.
Konstantin 

> 
> >  * prefer to just return logical result rather than have conditional:
> >  * for booleans prefer the  type boolean.
> >
> > static bool
> > is_valid_virt_queue_idx(uint32_t virtq_idx, bool is_tx, uint32_t max_qp_idx)
> > {
> > return (is_tx ^ (virtq_idx & 1)) ||
> > virtq_idx >= max_qp_idx * VIRTIO_QNUM;
> > }
> >

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Stephen Hemminger]

On Wed, 21 Oct 2015 09:38:37 +
"Ananyev, Konstantin"  wrote:

> > > minor nits:
> > >  * this doesn't need to be marked as always inline,
> > > that is as they say in English "shooting a fly with a bazooka"  
> > Stephen:
> > always_inline "forces" the compiler to inline this function, like a macro.
> > When should it be used or is it not preferred at all?  
> 
> I also don't understand what's wrong with using 'always_inline' here.
> As I understand the author wants compiler to *always inline* that function.
> So seems perfectly ok to use it here.
> As I remember just 'inline' is sort of recommendation that compiler is free 
> to ignore.
> Konstantin 

I follow Linux/Linus advice and resist the urge to add strong inlining.
The compiler does a good job of deciding to inline, and many times
the reason it chooses for not inlining are quite good like:
  - the code is on an unlikely branch
  - register pressure means inlining would mean the code would be worse

Therefore my rules are:
  * only use inline for small functions. Let compiler decide on larger static 
funcs
  * write code where most functions are static (localized scope) where compiler
can decide
  * reserve always inline for things that access hardware and would break if 
not inlined.

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Xie, Huawei]

On 10/21/2015 12:44 PM, Stephen Hemminger wrote:
> On Wed, 21 Oct 2015 11:48:10 +0800
> Yuanhan Liu  wrote:
>
>>  
>> +static inline int __attribute__((always_inline))
>> +is_valid_virt_queue_idx(uint32_t virtq_idx, int is_tx, uint32_t max_qp_idx)
>> +{
>> +if ((is_tx ^ (virtq_idx & 0x1)) ||
>> +(virtq_idx >= max_qp_idx * VIRTIO_QNUM))
>> +return 0;
>> +
>> +return 1;
>> +}
> minor nits:
>  * this doesn't need to be marked as always inline, 
> that is as they say in English "shooting a fly with a bazooka"
Stephen:
always_inline "forces" the compiler to inline this function, like a macro.
When should it be used or is it not preferred at all?

>  * prefer to just return logical result rather than have conditional:
>  * for booleans prefer the  type boolean.
>
> static bool
> is_valid_virt_queue_idx(uint32_t virtq_idx, bool is_tx, uint32_t max_qp_idx)
> {
>   return (is_tx ^ (virtq_idx & 1)) || 
>   virtq_idx >= max_qp_idx * VIRTIO_QNUM;
> }
>

[dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index

[Stephen Hemminger]

On Wed, 21 Oct 2015 11:48:10 +0800
Yuanhan Liu  wrote:

>  
> +static inline int __attribute__((always_inline))
> +is_valid_virt_queue_idx(uint32_t virtq_idx, int is_tx, uint32_t max_qp_idx)
> +{
> + if ((is_tx ^ (virtq_idx & 0x1)) ||
> + (virtq_idx >= max_qp_idx * VIRTIO_QNUM))
> + return 0;
> +
> + return 1;
> +}

minor nits:
 * this doesn't need to be marked as always inline, 
that is as they say in English "shooting a fly with a bazooka"
 * prefer to just return logical result rather than have conditional:
 * for booleans prefer the  type boolean.

static bool
is_valid_virt_queue_idx(uint32_t virtq_idx, bool is_tx, uint32_t max_qp_idx)
{
return (is_tx ^ (virtq_idx & 1)) || 
virtq_idx >= max_qp_idx * VIRTIO_QNUM;
}