On 02/06/2016 06:55, "Ilya Maximets" <i.maxim...@samsung.com> wrote:
>Hi, Daniele.
>Thanks for review.
>
>On 02.06.2016 04:33, Daniele Di Proietto wrote:
>> Hi Ilya,
>>
>> apologies for the delay.
>>
>> I didn't take a extremely detailed look at this series, but I have
>> a few high level comments.
>>
>> Thanks for adding a command to configure the rxq affinity. Have
>> you thought about using the database instead? I think it will
>> be easier to use because it survives restarts, and one can batch
>> the affinity assignment for multiple ports without explicitly
>> calling pmd-reconfigure. I'm not sure what the best interface
>> would look like. Perhaps a string in Interface:other_config that
>> maps rxqs with core ids?
>>
>> I'd prefer to avoid exporting an explicit command like
>> dpif-netdev/pmd-reconfigure. If we use the database we don't have to,
>> right?
>
>I thought about solution with database. Actually, I can't see big
>difference between database and appctl in this case. For automatic
>usage both commands may be scripted, but for manual pinning this
>approaches equally uncomfortable.
>IMHO, if it will be database it shouldn't be a per 'Interface'
>string with mapping, because one map influences on other ports
>(core isolation). Also there is an issue with synchronization with
>'pmd-cpu-mask' that should be performed manually anyway.
>appctl command may be changed to receive string of all mappings and
>trigger reconfiguration. In this case there will be no need to have
>explicit 'dpif-netdev/pmd-reconfigure'.
Do we really need to implement core isolation? I'd prefer an interface where
if an interface has an affinity we enforce that (as far as we can with the
current pmd-cpu-mask), and for other interfaces we keep the current model.
Probably there are some limitation I'm not seeing with this model.
I'd prefer to keep the mapping in the database because it's more in line
with the rest of OVS configuration. The database survives crashes, restarts
and reboots.
>
>
>> I'm not sure what's the best way to introduce XPS in OVS. First of all,
>> for physical NICs I'd want to keep the current model where possible
>> (one queue per core, no locking). Maybe we can introduce some mapping
>> just for vhost-user ports in netdev-dpdk to implement packet steering?
>
>We can just set default values for 'n_txq' more accurately: 'n_cores() + 1'
>for phy ports and '1' for virtual. To avoid locking on TX to phy ports
>we may just check that 'n_txq' >= 'n_cores() + 1'. We can do that because
>reconfiguration required to change 'n_txq' and, in current implementation
>of XPS, one queue will not be used twice if we have unused queues.
Ok, if it can be done without any overhead for phy ports, I'm fine with that.
>
>> Also, have you considered packet reordering issues? If a 5-tuple is
>> processed by one core and we often change the tx queue, we end up
>> with the same 5-tuple on two different tx queues.
>
>To avoid reordering issues there is a timeout mechanism inside XPS.
>Current tx queue_id may be changed only if there was no packets to
>this queue for a significant amount of time (XPS_CYCLES).
Ok, no problem then.
>
>> Lastly I'm not 100% sure we need a "n-txq" parameter. I think that
>> for vhost-user ports, we know the value in new_device() (we should
>> do that for rxqs too now that we have netdev_reconfigure). physical
>> NICs txqs should match the cpu cores and avoid locking, where
>> possible.
>
>Actually, we don't need this if default values will be set as described
>above and reconfiguration request will be used inside 'new_device()'
>(I guess, this should be implemented separately).
>But it's harmless and, actually, almost free in implementation. May be
>it can be used for some specific cases or devices.
In general, we try to limit the knobs that a user can play with, unless they
provide some value.
In this case, if we need something for netdev-dummy, we can implement it
explicitly for it and not avoid exposing it otherwise.
>Best regards, Ilya Maximets.
>
>> On 24/05/2016 06:34, "Ilya Maximets" <i.maxim...@samsung.com> wrote:
>>
>>> Manual pinning of RX queues to PMD threads required for performance
>>> optimisation. This will give to user ability to achieve max. performance
>>> using less number of CPUs because currently only user may know which
>>> ports are heavy loaded and which are not.
>>>
>>> To give full controll on ports TX queue manipulation mechanisms also
>>> required. For example, to avoid issue described in 'dpif-netdev: XPS
>>> (Transmit Packet Steering) implementation.' which becomes worse with
>>> ability of manual pinning.
>>> ( http://openvswitch.org/pipermail/dev/2016-March/067152.html )
>>>
>>> First 3 patches: prerequisites to XPS implementation.
>>> Patch #4: XPS implementation.
>>> Patches #5 and #6: Manual pinning implementation.
>>>
>>> Version 2:
>>> * Rebased on current master.
>>> * Fixed initialization of newly allocated memory in
>>> 'port_reconfigure()'.
>>>
>>> Ilya Maximets (6):
>>> netdev-dpdk: Use instant sending instead of queueing of packets.
>>> dpif-netdev: Allow configuration of number of tx queues.
>>> netdev-dpdk: Mandatory locking of TX queues.
>>> dpif-netdev: XPS (Transmit Packet Steering) implementation.
>>> dpif-netdev: Add dpif-netdev/pmd-reconfigure appctl command.
>>> dpif-netdev: Add dpif-netdev/pmd-rxq-set appctl command.
>>>
>>> INSTALL.DPDK.md | 44 +++--
>>> NEWS | 4 +
>>> lib/dpif-netdev.c | 393
>>> ++++++++++++++++++++++++++++++++++-----------
>>> lib/netdev-bsd.c | 1 -
>>> lib/netdev-dpdk.c | 198 ++++++-----------------
>>> lib/netdev-dummy.c | 1 -
>>> lib/netdev-linux.c | 1 -
>>> lib/netdev-provider.h | 18 +--
>>> lib/netdev-vport.c | 1 -
>>> lib/netdev.c | 30 ----
>>> lib/netdev.h | 1 -
>>> vswitchd/ovs-vswitchd.8.in | 10 ++
>>> 12 files changed, 400 insertions(+), 302 deletions(-)
>>>
>>> --
>>> 2.5.0
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
