* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Xu, Qian Q @ 2016-10-28 6:05 UTC (permalink / raw)
To: Maxime Coquelin, Yuanhan Liu
Cc: dev@dpdk.org, vkaplans@redhat.com, mst@redhat.com
In-Reply-To: <ba08f796-8a0e-85c9-ad75-62a782e6249f@redhat.com>
In my BDW-EP platform(similar to HSW), I can also see the performance drop. So what's the next step now?
Intel CPU GEN:
SNB-->IVB--->HSW-->BDW-EP
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
Sent: Thursday, October 27, 2016 6:53 PM
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: mst@redhat.com; dev@dpdk.org; vkaplans@redhat.com
Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors support to the TX path
On 10/27/2016 12:33 PM, Yuanhan Liu wrote:
> On Thu, Oct 27, 2016 at 11:10:34AM +0200, Maxime Coquelin wrote:
>> Hi Zhihong,
>>
>> On 10/27/2016 11:00 AM, Wang, Zhihong wrote:
>>> Hi Maxime,
>>>
>>> Seems indirect desc feature is causing serious performance
>>> degradation on Haswell platform, about 20% drop for both mrg=on and
>>> mrg=off (--txqflags=0xf00, non-vector version), both iofwd and
>>> macfwd.
>> I tested PVP (with macswap on guest) and Txonly/Rxonly on an Ivy
>> Bridge platform, and didn't faced such a drop.
>
> I was actually wondering that may be the cause. I tested it with my
> IvyBridge server as well, I saw no drop.
Sorry, mine is a SandyBridge, not IvyBridge.
^ permalink raw reply
* Re: [PATCH] Revert "bonding: use existing enslaved device queues"
From: Ilya Maximets @ 2016-10-28 6:14 UTC (permalink / raw)
To: Jan Blunck; +Cc: Dyasly Sergey, stable, dev
In-Reply-To: <03e11be9-45b2-e05f-99f1-8be1bcc05f19@samsung.com>
On 25.10.2016 09:26, Ilya Maximets wrote:
> On 24.10.2016 17:54, Jan Blunck wrote:
>> On Wed, Oct 19, 2016 at 5:47 AM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>> On 18.10.2016 18:19, Jan Blunck wrote:
>>>> On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>> On 18.10.2016 15:28, Jan Blunck wrote:
>>>>>> If the application already configured queues the PMD should not
>>>>>> silently claim ownership and reset them.
>>>>>>
>>>>>> What exactly is the problem when changing MTU? This works fine from
>>>>>> what I can tell.
>>>>>
>>>>> Following scenario leads to APP PANIC:
>>>>>
>>>>> 1. mempool_1 = rte_mempool_create()
>>>>> 2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
>>>>> 3. rte_eth_dev_start(bond0);
>>>>> 4. mempool_2 = rte_mempool_create();
>>>>> 5. rte_eth_dev_stop(bond0);
>>>>> 6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
>>>>> 7. rte_eth_dev_start(bond0);
>>>>> * RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
>>>>> 8. rte_mempool_free(mempool_1);
>>>>> 9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
>>>>> PANIC in rte_mempool_get_ops():
>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>>
>>>>> You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
>>>>> Bug is easily reproducible.
>>>>>
>>>>
>>>> I see. I'm not 100% that this is expected to work without leaking the
>>>> driver's queues though. The driver is allowed to do allocations in
>>>> its rx_queue_setup() function that are being freed via
>>>> rx_queue_release() later. But rx_queue_release() is only called if you
>>>> reconfigure the
>>>> device with 0 queues.
>
> It's not true. Drivers usually calls 'rx_queue_release()' inside
> 'rx_queue_setup()' function while reallocating the already allocated
> queue. (See ixgbe driver for example). Also all HW queues are
> usually destroyed inside 'eth_dev_stop()' and reallocated in
> 'eth_dev_start()' or '{rx,tx}_queue_setup()'.
> So, there is no leaks at all.
>
>>>> From what I understand there is no other way to
>>>> reconfigure a device to use another mempool.
>>>>
>>>> But ... even that wouldn't work with the bonding driver right now: the
>>>> bonding master only configures the slaves during startup. I can put
>>>> that on my todo list.
>
> No, bonding master reconfigures new slaves in 'rte_eth_bond_slave_add()'
> if needed.
>
>>>> Coming back to your original problem: changing the MTU for the bond
>>>> does work through rte_eth_dev_set_mtu() for slaves supporting that. In
>>>> any other case you could (re-)configure rxmode.max_rx_pkt_len (and
>>>> jumbo_frame / enable_scatter accordingly). This does work without a
>>>> call to rte_eth_rx_queue_setup().
>>>
>>> Thanks for suggestion, but using of rte_eth_dev_set_mtu() without
>>> reconfiguration will require to have mempools with huge mbufs (9KB)
>>> for all ports from the start. This is unacceptable because leads to
>>> significant performance regressions because of fast cache exhausting.
>>> Also this will require big work to rewrite OVS reconfiguration code
>>> this way.
>>> Anyway, it isn't the MTU only problem. Number of rx/tx descriptors
>>> also can't be changed in runtime.
>>>
>>>
>>> I'm not fully understand what is the use case for this 'reusing' code.
>>> Could you, please, describe situation where this behaviour is necessary?
>>
>> The device that is added to the bond was used before and therefore
>> already has allocated queues. Therefore we reuse the existing queues
>> of the devices instead of borrowing the queues of the bond device. If
>> the slave is removed from the bond again there is no need to allocate
>> the queues again.
>>
>> Hope that clarifies the usecase
>
> So, As I see, this is just an optimization that leads to differently
> configured queues of same device and possible application crash if the
> old configuration doesn't valid any more.
>
> With revert applied in your usecase while adding the device to bond
> it's queues will be automatically reconfigured according to configuration
> of the bond device. If the slave is removed from the bond all its'
> queues will remain as configured by bond. You can reconfigure them if
> needed. I guess, that in your case configuration of slave devices,
> actually, matches configuration of bond device. In that case slave
> will remain in the same state after removing from bond as it was
> before adding.
So, Jan, Declan, what do you think about this?
Best regards, Ilya Maximets.
^ permalink raw reply
* [PATCH] net/qede: fix gcc compiler option checks
From: Rasesh Mody @ 2016-10-28 6:37 UTC (permalink / raw)
To: thomas.monjalon; +Cc: dev, Dept-EngDPDKDev
In-Reply-To: <4958892.sIoVxbTeGS@xps13>
From: Rasesh Mody <Rasesh.Mody@cavium.com>
Using GCC_VERSION to check gcc version and decide whether to include
that compiler option.
Fixes: ec94dbc57362 ("qede: add base driver")
Fixes: ecc7a5a27ffe ("net/qede/base: fix 32-bit build")
Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
---
drivers/net/qede/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/qede/Makefile b/drivers/net/qede/Makefile
index 39751e4..29b443d 100644
--- a/drivers/net/qede/Makefile
+++ b/drivers/net/qede/Makefile
@@ -46,11 +46,11 @@ endif
endif
ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
-ifeq ($(shell gcc -Wno-unused-but-set-variable -Werror -E - < /dev/null > /dev/null 2>&1; echo $$?),0)
+ifeq ($(shell test $(GCC_VERSION) -ge 44 && echo 1), 1)
CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable
endif
CFLAGS_BASE_DRIVER += -Wno-missing-declarations
-ifeq ($(shell gcc -Wno-maybe-uninitialized -Werror -E - < /dev/null > /dev/null 2>&1; echo $$?),0)
+ifeq ($(shell test $(GCC_VERSION) -ge 46 && echo 1), 1)
CFLAGS_BASE_DRIVER += -Wno-maybe-uninitialized
endif
CFLAGS_BASE_DRIVER += -Wno-strict-prototypes
--
1.8.3.1
^ permalink raw reply related
* [PATCH] net/qede: fix advertising link speed capability
From: Rasesh Mody @ 2016-10-28 6:42 UTC (permalink / raw)
To: thomas.monjalon; +Cc: dev, Dept-EngDPDKDev
In-Reply-To: <1772382.AymLqBrjhc@xps13>
From: Harish Patil <harish.patil@qlogic.com>
Fix to advertise device's link speed capability based on current
link speed rather than returning driver supported speeds.
Fixes: 95e67b479506 ("net/qede: add 100G link speed capability")
Signed-off-by: Harish Patil <harish.patil@qlogic.com>
---
drivers/net/qede/qede_ethdev.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index b91b478..4c4c669 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -646,6 +646,7 @@ qede_dev_info_get(struct rte_eth_dev *eth_dev,
{
struct qede_dev *qdev = eth_dev->data->dev_private;
struct ecore_dev *edev = &qdev->edev;
+ struct qed_link_output link;
PMD_INIT_FUNC_TRACE(edev);
@@ -678,8 +679,9 @@ qede_dev_info_get(struct rte_eth_dev *eth_dev,
DEV_TX_OFFLOAD_UDP_CKSUM |
DEV_TX_OFFLOAD_TCP_CKSUM);
- dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G |
- ETH_LINK_SPEED_100G;
+ memset(&link, 0, sizeof(struct qed_link_output));
+ qdev->ops->common->get_link(edev, &link);
+ dev_info->speed_capa = rte_eth_speed_bitflag(link.speed, 0);
}
/* return 0 means link status changed, -1 means not changed */
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] net/qede: fix advertising link speed capability
From: Thomas Monjalon @ 2016-10-28 7:26 UTC (permalink / raw)
To: Rasesh Mody; +Cc: dev, Dept-EngDPDKDev
In-Reply-To: <1477636922-18546-1-git-send-email-rasesh.mody@qlogic.com>
2016-10-27 23:42, Rasesh Mody:
> From: Harish Patil <harish.patil@qlogic.com>
>
> Fix to advertise device's link speed capability based on current
> link speed rather than returning driver supported speeds.
[...]
> - dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G |
> - ETH_LINK_SPEED_100G;
> + memset(&link, 0, sizeof(struct qed_link_output));
> + qdev->ops->common->get_link(edev, &link);
> + dev_info->speed_capa = rte_eth_speed_bitflag(link.speed, 0);
> }
No, that's wrong.
You must advertise a capability!
So what the device supports?
Are every qede devices support ETH_LINK_SPEED_100G?
^ permalink raw reply
* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Pierre Pfister (ppfister) @ 2016-10-28 7:32 UTC (permalink / raw)
To: Wang, Zhihong; +Cc: mst@redhat.com, dev@dpdk.org, vkaplans@redhat.com
In-Reply-To: <8F6C2BD409508844A0EFC19955BE09414E7D8C3B@SHSMSX103.ccr.corp.intel.com>
> Le 27 oct. 2016 à 12:19, Wang, Zhihong <zhihong.wang@intel.com> a écrit :
>
>
>
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Thursday, October 27, 2016 5:55 PM
>> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
>> <yuanhan.liu@linux.intel.com>; stephen@networkplumber.org; Pierre
>> Pfister (ppfister) <ppfister@cisco.com>
>> Cc: Xie, Huawei <huawei.xie@intel.com>; dev@dpdk.org;
>> vkaplans@redhat.com; mst@redhat.com
>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors support
>> to the TX path
>>
>>
>>
>> On 10/27/2016 11:10 AM, Maxime Coquelin wrote:
>>> Hi Zhihong,
>>>
>>> On 10/27/2016 11:00 AM, Wang, Zhihong wrote:
>>>> Hi Maxime,
>>>>
>>>> Seems indirect desc feature is causing serious performance
>>>> degradation on Haswell platform, about 20% drop for both
>>>> mrg=on and mrg=off (--txqflags=0xf00, non-vector version),
>>>> both iofwd and macfwd.
>>> I tested PVP (with macswap on guest) and Txonly/Rxonly on an Ivy Bridge
>>> platform, and didn't faced such a drop.
>>> Have you tried to pass indirect_desc=off to qemu cmdline to see if you
>>> recover the performance?
>>>
>>> Yuanhan, which platform did you use when you tested it with zero copy?
>>>
>>>>
>>>> I'm using RC2, and the CPU is Xeon E5-2699 v3 @ 2.30GHz.
>>>>
>>>> Could you please verify if this is true in your test?
>>> I'll try -rc1/-rc2 on my platform, and let you know.
>> As a first test, I tried again Txonly from the guest to the host (Rxonly),
>> where Tx indirect descriptors are used, on my E5-2665 @2.40GHz:
>> v16.11-rc1: 10.81Mpps
>> v16.11-rc2: 10.91Mpps
>>
>> -rc2 is even slightly better in my case.
>> Could you please run the same test on your platform?
>
> I mean to use rc2 as both host and guest, and compare the
> perf between indirect=0 and indirect=1.
>
> I use PVP traffic, tried both testpmd and OvS as the forwarding
> engine in host, and testpmd in guest.
>
> Thanks
> Zhihong
From my experience, and as Michael pointed out, the best mode for small packets is obviously
ANY_LAYOUT so it uses a single descriptor per packet.
So, disabling indirect descriptors may give you better pps for 64 bytes packets, but that doesn't mean you should not implement, or enable, it in your driver. That just means that the guest is not taking the right decision, and uses indirect while it should actually use any_layout.
Given virtio/vhost design (most decision comes from the guest), the host should be liberal in what it accepts, and not try to influence guest implementation by carefully picking the features it supports. Otherwise guests will never get a chance to make the right decisions either.
- Pierre
>
>>
>> And could you provide me more info on your fwd bench?
>> Do you use dpdk-pktgen on host, or you do fwd on howt with a real NIC
>> also?
>>
>> Thanks,
>> Maxime
>>> Thanks,
>>> Maxime
>>>
>>>>
>>>>
>>>> Thanks
>>>> Zhihong
>>>>
>>>>> -----Original Message-----
>>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>>> Sent: Monday, October 17, 2016 10:15 PM
>>>>> To: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>>> Cc: Wang, Zhihong <zhihong.wang@intel.com>; Xie, Huawei
>>>>> <huawei.xie@intel.com>; dev@dpdk.org; vkaplans@redhat.com;
>>>>> mst@redhat.com; stephen@networkplumber.org
>>>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
>>>>> support
>>>>> to the TX path
>>>>>
>>>>>
>>>>>
>>>>> On 10/17/2016 03:21 PM, Yuanhan Liu wrote:
>>>>>> On Mon, Oct 17, 2016 at 01:23:23PM +0200, Maxime Coquelin wrote:
>>>>>>>> On my side, I just setup 2 Windows 2016 VMs, and confirm the issue.
>>>>>>>> I'll continue the investigation early next week.
>>>>>>>
>>>>>>> The root cause is identified.
>>>>>>> When INDIRECT_DESC feature is negotiated, Windows guest uses
>> indirect
>>>>>>> for both Tx and Rx descriptors, whereas Linux guests (Virtio PMD &
>>>>>>> virtio-net kernel driver) use indirect only for Tx.
>>>>>>> I'll implement indirect support for the Rx path in vhost lib, but the
>>>>>>> change will be too big for -rc release.
>>>>>>> I propose in the mean time to disable INDIRECT_DESC feature in vhost
>>>>>>> lib, we can still enable it locally for testing.
>>>>>>>
>>>>>>> Yuanhan, is it ok for you?
>>>>>>
>>>>>> That's okay.
>>>>> I'll send a patch to disable it then.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> Has anyone already tested Windows guest with vhost-net, which
>> also
>>>>> has
>>>>>>>> indirect descs support?
>>>>>>>
>>>>>>> I tested and confirm it works with vhost-net.
>>>>>>
>>>>>> I'm a bit confused then. IIRC, vhost-net also doesn't support indirect
>>>>>> for Rx path, right?
>>>>>
>>>>> No, it does support it actually.
>>>>> I thought it didn't support too, I misread the Kernel implementation of
>>>>> vhost-net and virtio-net. Acutally, virtio-net makes use of indirect
>>>>> in Rx path when mergeable buffers is disabled.
>>>>>
>>>>> The confusion certainly comes from me, sorry about that.
>>>>>
>>>>> Maxime
^ permalink raw reply
* Re: [RFC PATCH v2 2/3] lib: add bitrate statistics library
From: Morten Brørup @ 2016-10-28 7:39 UTC (permalink / raw)
To: Remy Horton, dev
In-Reply-To: <1477616671-9547-3-git-send-email-remy.horton@intel.com>
Comments below.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> Sent: Friday, October 28, 2016 3:05 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC PATCH v2 2/3] lib: add bitrate statistics
> library
>
> This patch adds a library that calculates peak and average data-rate
> statistics. For ethernet devices. These statistics are reported using
> the metrics library.
>
> Signed-off-by: Remy Horton <remy.horton@intel.com>
> diff --git a/lib/librte_bitratestats/rte_bitrate.c
> b/lib/librte_bitratestats/rte_bitrate.c
> new file mode 100644
> index 0000000..fcdf401
> --- /dev/null
> +++ b/lib/librte_bitratestats/rte_bitrate.c
> @@ -0,0 +1,126 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright(c) 2016 Intel Corporation. All rights reserved.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or
> without
> + * modification, are permitted provided that the following
> conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above
> copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> copyright
> + * notice, this list of conditions and the following disclaimer
> in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Intel Corporation nor the names of its
> + * contributors may be used to endorse or promote products
> derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> + */
> +
> +#include <rte_common.h>
> +#include <rte_ethdev.h>
> +#include <rte_malloc.h>
> +#include <rte_metrics.h>
> +#include <rte_bitrate.h>
> +
> +
> +struct rte_stats_bitrate_s {
> + uint64_t last_ibytes;
> + uint64_t last_obytes;
> + uint64_t peak_ibits;
> + uint64_t peak_obits;
> + uint64_t ewma_ibits;
> + uint64_t ewma_obits;
> +};
> +
> +struct rte_stats_bitrates_s {
> + struct rte_stats_bitrate_s port_stats[RTE_MAX_ETHPORTS];
> + uint16_t id_stats_set;
> +};
> +
> +
> +struct rte_stats_bitrates_s *rte_stats_bitrate_create(void) {
> + return rte_zmalloc(NULL, sizeof(struct rte_stats_bitrates_s), 0);
> }
> +
> +
> +int
> +rte_stats_bitrate_reg(struct rte_stats_bitrates_s *bitrate_data)
> +{
> + const char *names[] = {
> + "mean_bits_in", "mean_bits_out",
> + "peak_bits_in", "peak_bits_out",
> + };
> + int return_value;
> +
> + bitrate_data = rte_stats_bitrate_create();
> + if (bitrate_data == NULL)
> + rte_exit(EXIT_FAILURE, "Could not allocate bitrate
> data.\n");
> + return_value = rte_metrics_reg_metrics(&names[0], 4);
> + if (return_value >= 0)
> + bitrate_data->id_stats_set = return_value;
> + return return_value;
> +}
> +
> +
> +int
> +rte_stats_bitrate_calc(struct rte_stats_bitrates_s *bitrate_data,
> + uint8_t port_id)
> +{
> + struct rte_stats_bitrate_s *port_data;
> + struct rte_eth_stats eth_stats;
> + int ret_code;
> + uint64_t cnt_bits;
> + int64_t delta;
> + const int64_t alpha_percent = 20;
> + uint64_t values[4];
> +
> + ret_code = rte_eth_stats_get(port_id, ð_stats);
> + if (ret_code != 0)
> + return ret_code;
> +
> + port_data = &bitrate_data->port_stats[port_id];
> +
> + /* Incoming */
> + cnt_bits = (eth_stats.ibytes - port_data->last_ibytes) << 3;
> + port_data->last_ibytes = eth_stats.ibytes;
> + if (cnt_bits > port_data->peak_ibits)
> + port_data->peak_ibits = cnt_bits;
> + delta = cnt_bits;
> + delta -= port_data->ewma_ibits;
> + delta = (delta * alpha_percent) / 100;
> + port_data->ewma_ibits += delta;
> +
> + /* Outgoing */
> + cnt_bits = (eth_stats.obytes - port_data->last_obytes) << 3;
> + port_data->last_obytes = eth_stats.obytes;
> + if (cnt_bits > port_data->peak_obits)
> + port_data->peak_obits = cnt_bits;
> + delta = cnt_bits;
> + delta -= port_data->ewma_obits;
> + delta = (delta * alpha_percent) / 100;
> + port_data->ewma_obits += delta;
> +
> + values[0] = port_data->ewma_ibits;
> + values[1] = port_data->ewma_obits;
> + values[2] = port_data->peak_ibits;
> + values[3] = port_data->peak_obits;
> + rte_metrics_update_metrics(port_id, bitrate_data->id_stats_set,
> + values, 4);
> + return 0;
> +}
When working with statistical calculations using integer arithmetic, you should round off the integer result by adding 0.5 to the result, which you do by adding half of the divisor to the dividend, like this:
delta = (delta * alpha_percent + 50) / 100;
The numbers in this particular case are probably very big, so not rounding off doesn't affect the result a lot; but then you should add a comment about why rounding down is acceptable.
Med venlig hilsen / kind regards
- Morten Brørup
^ permalink raw reply
* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Maxime Coquelin @ 2016-10-28 7:42 UTC (permalink / raw)
To: Wang, Zhihong, Yuanhan Liu
Cc: mst@redhat.com, dev@dpdk.org, vkaplans@redhat.com
In-Reply-To: <8F6C2BD409508844A0EFC19955BE09414E7D90C7@SHSMSX103.ccr.corp.intel.com>
On 10/28/2016 02:49 AM, Wang, Zhihong wrote:
>
>> > -----Original Message-----
>> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
>> > Sent: Thursday, October 27, 2016 6:46 PM
>> > To: Maxime Coquelin <maxime.coquelin@redhat.com>
>> > Cc: Wang, Zhihong <zhihong.wang@intel.com>;
>> > stephen@networkplumber.org; Pierre Pfister (ppfister)
>> > <ppfister@cisco.com>; Xie, Huawei <huawei.xie@intel.com>; dev@dpdk.org;
>> > vkaplans@redhat.com; mst@redhat.com
>> > Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors support
>> > to the TX path
>> >
>> > On Thu, Oct 27, 2016 at 12:35:11PM +0200, Maxime Coquelin wrote:
>>> > >
>>> > >
>>> > > On 10/27/2016 12:33 PM, Yuanhan Liu wrote:
>>>> > > >On Thu, Oct 27, 2016 at 11:10:34AM +0200, Maxime Coquelin wrote:
>>>>> > > >>Hi Zhihong,
>>>>> > > >>
>>>>> > > >>On 10/27/2016 11:00 AM, Wang, Zhihong wrote:
>>>>>> > > >>>Hi Maxime,
>>>>>> > > >>>
>>>>>> > > >>>Seems indirect desc feature is causing serious performance
>>>>>> > > >>>degradation on Haswell platform, about 20% drop for both
>>>>>> > > >>>mrg=on and mrg=off (--txqflags=0xf00, non-vector version),
>>>>>> > > >>>both iofwd and macfwd.
>>>>> > > >>I tested PVP (with macswap on guest) and Txonly/Rxonly on an Ivy
>> > Bridge
>>>>> > > >>platform, and didn't faced such a drop.
>>>> > > >
>>>> > > >I was actually wondering that may be the cause. I tested it with
>>>> > > >my IvyBridge server as well, I saw no drop.
>>>> > > >
>>>> > > >Maybe you should find a similar platform (Haswell) and have a try?
>>> > > Yes, that's why I asked Zhihong whether he could test Txonly in guest to
>>> > > see if issue is reproducible like this.
>> >
>> > I have no Haswell box, otherwise I could do a quick test for you. IIRC,
>> > he tried to disable the indirect_desc feature, then the performance
>> > recovered. So, it's likely the indirect_desc is the culprit here.
>> >
>>> > > I will be easier for me to find an Haswell machine if it has not to be
>>> > > connected back to back to and HW/SW packet generator.
> In fact simple loopback test will also do, without pktgen.
>
> Start testpmd in both host and guest, and do "start" in one
> and "start tx_first 32" in another.
>
> Perf drop is about 24% in my test.
>
Thanks, I never tried this test.
I managed to find an Haswell platform (Intel(R) Xeon(R) CPU E5-2699 v3
@ 2.30GHz), and can reproduce the problem with the loop test you
mention. I see a performance drop about 10% (8.94Mpps/8.08Mpps).
Out of curiosity, what are the numbers you get with your setup?
As I never tried this test, I run it again on my Sandy Bridge setup, and
I also see a performance regression, this time of 4%.
If I understand correctly the test, only 32 packets are allocated,
corresponding to a single burst, which is less than the queue size.
So it makes sense that the performance is lower with this test case.
Thanks,
Maxime
^ permalink raw reply
* Re: [RFC PATCH v2 2/3] lib: add bitrate statistics library
From: Remy Horton @ 2016-10-28 7:48 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
In-Reply-To: <20161027181220.752e90ea@xeon-e3>
On 28/10/2016 09:12, Stephen Hemminger wrote:
> On Fri, 28 Oct 2016 09:04:30 +0800
> Remy Horton <remy.horton@intel.com> wrote:
>
>> +
>> +struct rte_stats_bitrate_s {
>> + uint64_t last_ibytes;
>> + uint64_t last_obytes;
>> + uint64_t peak_ibits;
>> + uint64_t peak_obits;
>> + uint64_t ewma_ibits;
>> + uint64_t ewma_obits;
>> +};
>> +
>
> Reader/write access of 64 bit values is not safe on 32 bit platforms.
> I think you need to add a generation counter (see Linux kernel syncp)
> to handle 32 bit architecture. If done correctly, it would be a nop
> on 64 bit platforms.
I don't see a problem since this is private persistent data that is only
read/written from rte_stats_bitrate_calc(), and once calculated it
pushes them into the metrics library using rte_metrics_update_metrics().
The idea is that downstream consumers get the values using
rte_metrics_get_values() rather than reading rte_stats_bitrate_s directly.
Having said that, what you mention quite likley affects the metrics
library itself.. :)
^ permalink raw reply
* Re: [PATCH v4] vhost: Add indirect descriptors support to the TX path
From: Maxime Coquelin @ 2016-10-28 7:58 UTC (permalink / raw)
To: Pierre Pfister (ppfister), Wang, Zhihong
Cc: mst@redhat.com, dev@dpdk.org, vkaplans@redhat.com
In-Reply-To: <79DA8EFC-215C-4075-8D1A-FF81EC3CBB21@cisco.com>
On 10/28/2016 09:32 AM, Pierre Pfister (ppfister) wrote:
>
>> Le 27 oct. 2016 à 12:19, Wang, Zhihong <zhihong.wang@intel.com> a écrit :
>>
>>
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>> Sent: Thursday, October 27, 2016 5:55 PM
>>> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
>>> <yuanhan.liu@linux.intel.com>; stephen@networkplumber.org; Pierre
>>> Pfister (ppfister) <ppfister@cisco.com>
>>> Cc: Xie, Huawei <huawei.xie@intel.com>; dev@dpdk.org;
>>> vkaplans@redhat.com; mst@redhat.com
>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors support
>>> to the TX path
>>>
>>>
>>>
>>> On 10/27/2016 11:10 AM, Maxime Coquelin wrote:
>>>> Hi Zhihong,
>>>>
>>>> On 10/27/2016 11:00 AM, Wang, Zhihong wrote:
>>>>> Hi Maxime,
>>>>>
>>>>> Seems indirect desc feature is causing serious performance
>>>>> degradation on Haswell platform, about 20% drop for both
>>>>> mrg=on and mrg=off (--txqflags=0xf00, non-vector version),
>>>>> both iofwd and macfwd.
>>>> I tested PVP (with macswap on guest) and Txonly/Rxonly on an Ivy Bridge
>>>> platform, and didn't faced such a drop.
>>>> Have you tried to pass indirect_desc=off to qemu cmdline to see if you
>>>> recover the performance?
>>>>
>>>> Yuanhan, which platform did you use when you tested it with zero copy?
>>>>
>>>>>
>>>>> I'm using RC2, and the CPU is Xeon E5-2699 v3 @ 2.30GHz.
>>>>>
>>>>> Could you please verify if this is true in your test?
>>>> I'll try -rc1/-rc2 on my platform, and let you know.
>>> As a first test, I tried again Txonly from the guest to the host (Rxonly),
>>> where Tx indirect descriptors are used, on my E5-2665 @2.40GHz:
>>> v16.11-rc1: 10.81Mpps
>>> v16.11-rc2: 10.91Mpps
>>>
>>> -rc2 is even slightly better in my case.
>>> Could you please run the same test on your platform?
>>
>> I mean to use rc2 as both host and guest, and compare the
>> perf between indirect=0 and indirect=1.
>>
>> I use PVP traffic, tried both testpmd and OvS as the forwarding
>> engine in host, and testpmd in guest.
>>
>> Thanks
>> Zhihong
>
> From my experience, and as Michael pointed out, the best mode for small packets is obviously
> ANY_LAYOUT so it uses a single descriptor per packet.
Of course, having a single descriptor is in theory the best way.
But, in current Virtio PMD implementation, with no offload supported, we
never access the virtio header at transmit time, it is allocated and
zeroed at startup.
For ANY_LAYOUT case, the virtio header is prepended to the packet, and
need to be zeroed at packet transmit time. The performance impact is
quite important, as show the measurements I made one month ago (txonly):
- 2 descs per packet: 11.6Mpps
- 1 desc per packet: 9.6Mpps
As Michael suggested, I tried to replace the memset by direct
fields assignation, but it only recovers a small part of the drop.
What I suggested is to introduce a new feature, so that we can skip the
virtio header when no offload is negotiated.
Maybe you have other ideas?
> So, disabling indirect descriptors may give you better pps for 64 bytes packets, but that doesn't mean you should not implement, or enable, it in your driver. That just means that the guest is not taking the right decision, and uses indirect while it should actually use any_layout.
+1, it really depends on the use-case.
>
> Given virtio/vhost design (most decision comes from the guest), the host should be liberal in what it accepts, and not try to influence guest implementation by carefully picking the features it supports. Otherwise guests will never get a chance to make the right decisions either.
Agree, what we need is to be able to disable Virtio PMD features
without having to rebuild the PMD.
It will certainly require an new API change to add this option.
Thanks,
Maxime
>
> - Pierre
>
>>
>>>
>>> And could you provide me more info on your fwd bench?
>>> Do you use dpdk-pktgen on host, or you do fwd on howt with a real NIC
>>> also?
>>>
>>> Thanks,
>>> Maxime
>>>> Thanks,
>>>> Maxime
>>>>
>>>>>
>>>>>
>>>>> Thanks
>>>>> Zhihong
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>>>> Sent: Monday, October 17, 2016 10:15 PM
>>>>>> To: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>>>> Cc: Wang, Zhihong <zhihong.wang@intel.com>; Xie, Huawei
>>>>>> <huawei.xie@intel.com>; dev@dpdk.org; vkaplans@redhat.com;
>>>>>> mst@redhat.com; stephen@networkplumber.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: Add indirect descriptors
>>>>>> support
>>>>>> to the TX path
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 10/17/2016 03:21 PM, Yuanhan Liu wrote:
>>>>>>> On Mon, Oct 17, 2016 at 01:23:23PM +0200, Maxime Coquelin wrote:
>>>>>>>>> On my side, I just setup 2 Windows 2016 VMs, and confirm the issue.
>>>>>>>>> I'll continue the investigation early next week.
>>>>>>>>
>>>>>>>> The root cause is identified.
>>>>>>>> When INDIRECT_DESC feature is negotiated, Windows guest uses
>>> indirect
>>>>>>>> for both Tx and Rx descriptors, whereas Linux guests (Virtio PMD &
>>>>>>>> virtio-net kernel driver) use indirect only for Tx.
>>>>>>>> I'll implement indirect support for the Rx path in vhost lib, but the
>>>>>>>> change will be too big for -rc release.
>>>>>>>> I propose in the mean time to disable INDIRECT_DESC feature in vhost
>>>>>>>> lib, we can still enable it locally for testing.
>>>>>>>>
>>>>>>>> Yuanhan, is it ok for you?
>>>>>>>
>>>>>>> That's okay.
>>>>>> I'll send a patch to disable it then.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> Has anyone already tested Windows guest with vhost-net, which
>>> also
>>>>>> has
>>>>>>>>> indirect descs support?
>>>>>>>>
>>>>>>>> I tested and confirm it works with vhost-net.
>>>>>>>
>>>>>>> I'm a bit confused then. IIRC, vhost-net also doesn't support indirect
>>>>>>> for Rx path, right?
>>>>>>
>>>>>> No, it does support it actually.
>>>>>> I thought it didn't support too, I misread the Kernel implementation of
>>>>>> vhost-net and virtio-net. Acutally, virtio-net makes use of indirect
>>>>>> in Rx path when mergeable buffers is disabled.
>>>>>>
>>>>>> The confusion certainly comes from me, sorry about that.
>>>>>>
>>>>>> Maxime
>
^ permalink raw reply
* Re: [RFC] [PATCH v2] libeventdev: event driven programming model framework for DPDK
From: Bruce Richardson @ 2016-10-28 8:36 UTC (permalink / raw)
To: Jerin Jacob
Cc: Vangati, Narender, dev@dpdk.org, Eads, Gage,
thomas.monjalon@6wind.com
In-Reply-To: <20161028030140.GA2967@localhost.localdomain>
On Fri, Oct 28, 2016 at 08:31:41AM +0530, Jerin Jacob wrote:
> On Wed, Oct 26, 2016 at 01:54:14PM +0100, Bruce Richardson wrote:
> > On Wed, Oct 26, 2016 at 05:54:17PM +0530, Jerin Jacob wrote:
> > > On Wed, Oct 26, 2016 at 12:11:03PM +0000, Van Haaren, Harry wrote:
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jerin Jacob
> > Thanks. One other suggestion is that it might be useful to provide
> > support for having typed queues explicitly in the API. Right now, when
> > you create an queue, the queue_conf structure takes as parameters how
> > many atomic flows that are needed for the queue, or how many reorder
> > slots need to be reserved for it. This implicitly hints at the type of
> > traffic which will be sent to the queue, but I'm wondering if it's
> > better to make it explicit. There are certain optimisations that can be
> > looked at if we know that a queue only handles packets of a particular
> > type. [Not having to handle reordering when pulling events from a core
> > can be a big win for software!].
>
> If it helps in SW implementation, then I think we can add this in queue
> configuration.
>
> >
> > How about adding: "allowed_event_types" as a field to
> > rte_event_queue_conf, with possible values:
> > * atomic
> > * ordered
> > * parallel
> > * mixed - allowing all 3 types. I think allowing 2 of three types might
> > make things too complicated.
> >
> > An open question would then be how to behave when the queue type and
> > requested event type conflict. We can either throw an error, or just
> > ignore the event type and always treat enqueued events as being of the
> > queue type. I prefer the latter, because it's faster not having to
> > error-check, and it pushes the responsibility on the app to know what
> > it's doing.
>
> How about making default as "mixed" and let application configures what
> is not required?. That way application responsibility is clear.
> something similar to ETH_TXQ_FLAGS_NOMULTSEGS, ETH_TXQ_FLAGS_NOREFCOUNT
> with default.
>
I suppose it could work, but why bother doing that? If an app knows it's
only going to use one traffic type, why not let it just state what it
will do rather than try to specify what it won't do. If mixed is needed,
then it's easy enough to specify - and we can make it the zero/default
value too.
Our software implementation for now, only supports one type per queue -
which we suspect should meet a lot of use-cases. We'll have to see about
adding in mixed types in future.
/Bruce
^ permalink raw reply
* Re: [RFC] [PATCH v2] libeventdev: event driven programming model framework for DPDK
From: Jerin Jacob @ 2016-10-28 9:06 UTC (permalink / raw)
To: Bruce Richardson
Cc: Vangati, Narender, dev@dpdk.org, Eads, Gage,
thomas.monjalon@6wind.com
In-Reply-To: <20161028083646.GA82872@bricha3-MOBL3.ger.corp.intel.com>
On Fri, Oct 28, 2016 at 09:36:46AM +0100, Bruce Richardson wrote:
> On Fri, Oct 28, 2016 at 08:31:41AM +0530, Jerin Jacob wrote:
> > On Wed, Oct 26, 2016 at 01:54:14PM +0100, Bruce Richardson wrote:
> > > On Wed, Oct 26, 2016 at 05:54:17PM +0530, Jerin Jacob wrote:
> > > > On Wed, Oct 26, 2016 at 12:11:03PM +0000, Van Haaren, Harry wrote:
> > > > > > -----Original Message-----
> > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jerin Jacob
> > > Thanks. One other suggestion is that it might be useful to provide
> > > support for having typed queues explicitly in the API. Right now, when
> > > you create an queue, the queue_conf structure takes as parameters how
> > > many atomic flows that are needed for the queue, or how many reorder
> > > slots need to be reserved for it. This implicitly hints at the type of
> > > traffic which will be sent to the queue, but I'm wondering if it's
> > > better to make it explicit. There are certain optimisations that can be
> > > looked at if we know that a queue only handles packets of a particular
> > > type. [Not having to handle reordering when pulling events from a core
> > > can be a big win for software!].
> >
> > If it helps in SW implementation, then I think we can add this in queue
> > configuration.
> >
> > >
> > > How about adding: "allowed_event_types" as a field to
> > > rte_event_queue_conf, with possible values:
> > > * atomic
> > > * ordered
> > > * parallel
> > > * mixed - allowing all 3 types. I think allowing 2 of three types might
> > > make things too complicated.
> > >
> > > An open question would then be how to behave when the queue type and
> > > requested event type conflict. We can either throw an error, or just
> > > ignore the event type and always treat enqueued events as being of the
> > > queue type. I prefer the latter, because it's faster not having to
> > > error-check, and it pushes the responsibility on the app to know what
> > > it's doing.
> >
> > How about making default as "mixed" and let application configures what
> > is not required?. That way application responsibility is clear.
> > something similar to ETH_TXQ_FLAGS_NOMULTSEGS, ETH_TXQ_FLAGS_NOREFCOUNT
> > with default.
> >
> I suppose it could work, but why bother doing that? If an app knows it's
> only going to use one traffic type, why not let it just state what it
> will do rather than try to specify what it won't do. If mixed is needed,
My thought was more inline with ethdev spec, like, ref-count is default,
if application need exception then set ETH_TXQ_FLAGS_NOREFCOUNT. But it is OK, if
you need other way.
> then it's easy enough to specify - and we can make it the zero/default
> value too.
OK. Then we will make MIX as zero/default and add "allowed_event_types" in
event queue config.
/Jerin
>
> Our software implementation for now, only supports one type per queue -
> which we suspect should meet a lot of use-cases. We'll have to see about
> adding in mixed types in future.
>
> /Bruce
^ permalink raw reply
* Re: [PATCH v11 1/6] ethdev: add Tx preparation
From: Ananyev, Konstantin @ 2016-10-28 10:15 UTC (permalink / raw)
To: Kulasek, TomaszX, Thomas Monjalon; +Cc: dev@dpdk.org
In-Reply-To: <3042915272161B4EB253DA4D77EB373A14F45162@IRSMSX102.ger.corp.intel.com>
Hi Tomasz,
>
> Hi
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Thursday, October 27, 2016 18:24
> > To: Thomas Monjalon <thomas.monjalon@6wind.com>
> > Cc: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
> >
> >
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Thursday, October 27, 2016 5:02 PM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Cc: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
> > >
> > > 2016-10-27 15:52, Ananyev, Konstantin:
> > > >
> > > > >
> > > > > Hi Tomasz,
> > > > >
> > > > > This is a major new function in the API and I still have some
> > comments.
> > > > >
> > > > > 2016-10-26 14:56, Tomasz Kulasek:
> > > > > > --- a/config/common_base
> > > > > > +++ b/config/common_base
> > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y
> > > > >
> > > > > We cannot enable it until it is implemented in every drivers.
> > > >
> > > > Not sure why?
> > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as noop.
> > > > Right now it is not mandatory for the PMD to implement it.
> > >
> > > If it is not implemented, the application must do the preparation by
> > itself.
> > > From patch 6:
> > > "
> > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > application and used Tx preparation API for packet preparation and
> > > verification.
> > > "
> > > So how does it behave with other drivers?
> >
> > Hmm so it seems that we broke testpmd csumonly mode for non-intel
> > drivers..
> > My bad, missed that part completely.
> > Yes, then I suppose for now we'll need to support both (with and without)
> > code paths for testpmd.
> > Probably a new fwd mode or just extra parameter for the existing one?
> > Any other suggestions?
> >
>
> I had sent txprep engine in v2 (http://dpdk.org/dev/patchwork/patch/15775/), but I'm opened on the suggestions. If you like it I can resent
> it in place of csumonly modification.
I still not sure it is worth to have another version of csum...
Can we introduce a new global variable in testpmd and a new command:
testpmd> csum tx_prep
or so?
Looking at current testpmd patch, I suppose the changes will be minimal.
What do you think?
Konstantin
>
> Tomasz
>
> > >
> > > > > > struct rte_eth_dev {
> > > > > > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive
> > function. */
> > > > > > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit
> > > > > > function. */
> > > > > > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit
> > > > > > +prepare function. */
> > > > > > struct rte_eth_dev_data *data; /**< Pointer to device data */
> > > > > > const struct eth_driver *driver;/**< Driver for this device */
> > > > > > const struct eth_dev_ops *dev_ops; /**< Functions exported by
> > > > > > PMD */
> > > > >
> > > > > Could you confirm why tx_pkt_prep is not in dev_ops?
> > > > > I guess we want to have several implementations?
> > > >
> > > > Yes, it depends on configuration options, same as tx_pkt_burst.
> > > >
> > > > >
> > > > > Shouldn't we have a const struct control_dev_ops and a struct
> > datapath_dev_ops?
> > > >
> > > > That's probably a good idea, but I suppose it is out of scope for that
> > patch.
> > >
> > > No it's not out of scope.
> > > It answers to the question "why is it added in this structure and not
> > dev_ops".
> > > We won't do this change when nothing else is changed in the struct.
> >
> > Not sure I understood you here:
> > Are you saying datapath_dev_ops/controlpath_dev_ops have to be introduced
> > as part of that patch?
> > But that's a lot of changes all over rte_ethdev.[h,c].
> > It definitely worse a separate patch (might be some discussion) for me.
> > Konstantin
> >
> >
^ permalink raw reply
* Re: [PATCH v11 1/6] ethdev: add Tx preparation
From: Kulasek, TomaszX @ 2016-10-28 10:22 UTC (permalink / raw)
To: Ananyev, Konstantin, Thomas Monjalon; +Cc: dev@dpdk.org
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CEAEE@irsmsx105.ger.corp.intel.com>
Hi Konstantin,
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, October 28, 2016 12:16
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
>
> Hi Tomasz,
>
> >
> > Hi
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Thursday, October 27, 2016 18:24
> > > To: Thomas Monjalon <thomas.monjalon@6wind.com>
> > > Cc: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > Sent: Thursday, October 27, 2016 5:02 PM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > > Cc: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
> > > >
> > > > 2016-10-27 15:52, Ananyev, Konstantin:
> > > > >
> > > > > >
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > This is a major new function in the API and I still have some
> > > comments.
> > > > > >
> > > > > > 2016-10-26 14:56, Tomasz Kulasek:
> > > > > > > --- a/config/common_base
> > > > > > > +++ b/config/common_base
> > > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y
> > > > > >
> > > > > > We cannot enable it until it is implemented in every drivers.
> > > > >
> > > > > Not sure why?
> > > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as
> noop.
> > > > > Right now it is not mandatory for the PMD to implement it.
> > > >
> > > > If it is not implemented, the application must do the preparation
> > > > by
> > > itself.
> > > > From patch 6:
> > > > "
> > > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > > application and used Tx preparation API for packet preparation and
> > > > verification.
> > > > "
> > > > So how does it behave with other drivers?
> > >
> > > Hmm so it seems that we broke testpmd csumonly mode for non-intel
> > > drivers..
> > > My bad, missed that part completely.
> > > Yes, then I suppose for now we'll need to support both (with and
> > > without) code paths for testpmd.
> > > Probably a new fwd mode or just extra parameter for the existing one?
> > > Any other suggestions?
> > >
> >
> > I had sent txprep engine in v2
> > (http://dpdk.org/dev/patchwork/patch/15775/), but I'm opened on the
> suggestions. If you like it I can resent it in place of csumonly
> modification.
>
> I still not sure it is worth to have another version of csum...
> Can we introduce a new global variable in testpmd and a new command:
> testpmd> csum tx_prep
> or so?
> Looking at current testpmd patch, I suppose the changes will be minimal.
> What do you think?
> Konstantin
>
This is not a problem.
Tomasz
^ permalink raw reply
* Re: [PATCH v11 1/6] ethdev: add Tx preparation
From: Thomas Monjalon @ 2016-10-28 10:22 UTC (permalink / raw)
To: Ananyev, Konstantin, Kulasek, TomaszX; +Cc: dev
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CEAEE@irsmsx105.ger.corp.intel.com>
2016-10-28 10:15, Ananyev, Konstantin:
> > From: Ananyev, Konstantin
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2016-10-27 15:52, Ananyev, Konstantin:
> > > > > > 2016-10-26 14:56, Tomasz Kulasek:
> > > > > > > --- a/config/common_base
> > > > > > > +++ b/config/common_base
> > > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y
> > > > > >
> > > > > > We cannot enable it until it is implemented in every drivers.
> > > > >
> > > > > Not sure why?
> > > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as noop.
> > > > > Right now it is not mandatory for the PMD to implement it.
> > > >
> > > > If it is not implemented, the application must do the preparation by
> > > itself.
> > > > From patch 6:
> > > > "
> > > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > > application and used Tx preparation API for packet preparation and
> > > > verification.
> > > > "
> > > > So how does it behave with other drivers?
> > >
> > > Hmm so it seems that we broke testpmd csumonly mode for non-intel
> > > drivers..
> > > My bad, missed that part completely.
> > > Yes, then I suppose for now we'll need to support both (with and without)
> > > code paths for testpmd.
> > > Probably a new fwd mode or just extra parameter for the existing one?
> > > Any other suggestions?
> > >
> >
> > I had sent txprep engine in v2 (http://dpdk.org/dev/patchwork/patch/15775/), but I'm opened on the suggestions. If you like it I can resent
> > it in place of csumonly modification.
>
> I still not sure it is worth to have another version of csum...
> Can we introduce a new global variable in testpmd and a new command:
> testpmd> csum tx_prep
> or so?
> Looking at current testpmd patch, I suppose the changes will be minimal.
> What do you think?
No please no!
The problem is not in testpmd.
The problem is in every applications.
Should we prepare the checksums or let tx_prep do it?
The result will depend of the driver used.
^ permalink raw reply
* Re: [PATCH v11 1/6] ethdev: add Tx preparation
From: Ananyev, Konstantin @ 2016-10-28 10:28 UTC (permalink / raw)
To: Thomas Monjalon, Kulasek, TomaszX; +Cc: dev@dpdk.org
In-Reply-To: <18243372.Y4s77Td6b4@xps13>
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, October 28, 2016 11:22 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
>
> 2016-10-28 10:15, Ananyev, Konstantin:
> > > From: Ananyev, Konstantin
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2016-10-27 15:52, Ananyev, Konstantin:
> > > > > > > 2016-10-26 14:56, Tomasz Kulasek:
> > > > > > > > --- a/config/common_base
> > > > > > > > +++ b/config/common_base
> > > > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y
> > > > > > >
> > > > > > > We cannot enable it until it is implemented in every drivers.
> > > > > >
> > > > > > Not sure why?
> > > > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as noop.
> > > > > > Right now it is not mandatory for the PMD to implement it.
> > > > >
> > > > > If it is not implemented, the application must do the preparation by
> > > > itself.
> > > > > From patch 6:
> > > > > "
> > > > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > > > application and used Tx preparation API for packet preparation and
> > > > > verification.
> > > > > "
> > > > > So how does it behave with other drivers?
> > > >
> > > > Hmm so it seems that we broke testpmd csumonly mode for non-intel
> > > > drivers..
> > > > My bad, missed that part completely.
> > > > Yes, then I suppose for now we'll need to support both (with and without)
> > > > code paths for testpmd.
> > > > Probably a new fwd mode or just extra parameter for the existing one?
> > > > Any other suggestions?
> > > >
> > >
> > > I had sent txprep engine in v2 (http://dpdk.org/dev/patchwork/patch/15775/), but I'm opened on the suggestions. If you like it I can
> resent
> > > it in place of csumonly modification.
> >
> > I still not sure it is worth to have another version of csum...
> > Can we introduce a new global variable in testpmd and a new command:
> > testpmd> csum tx_prep
> > or so?
> > Looking at current testpmd patch, I suppose the changes will be minimal.
> > What do you think?
>
> No please no!
> The problem is not in testpmd.
> The problem is in every applications.
> Should we prepare the checksums or let tx_prep do it?
Not sure, I understood you...
Right now we don't' change other apps.
They would work as before.
If people would like to start to use tx_prep in their apps -
they are free to do that.
If they like to keep doing that manually - that's fine too.
>From other side we need an ability to test (and demonstrate) that new functionality.
So we do need changes in testpmd.
Konstantin
> The result will depend of the driver used.
^ permalink raw reply
* Re: Solarflare PMD submission question
From: Andrew Rybchenko @ 2016-10-28 10:50 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
In-Reply-To: <7987614.kmGMHz0qWb@xps13>
Thomas,
On 10/27/2016 01:37 PM, Thomas Monjalon wrote:
> First of all, welcome to DPDK!
Thanks!
> 2016-10-27 09:34, Andrew Rybchenko:
>> we would like to include Solarflare libefx-based PMD in the DPDK 17.02
>> and start the upstreaming process.
>> The driver supports Solarflare SFN7xxx and SFN8xxx families of 10/40
>> Gbps adapters.
>> The driver has base driver. It is just fresh version of the same code
>> which is used in the FreeBSD [1], illumos [2] and some other Solarflare
>> drivers.
> Unfortunately it is common to have some big base drivers in DPDK.
> Note that some PMD rely on their kernel counterpart for the control path.
> It is a way to avoid code duplication.
Linux kernel sfc driver has control path functionality, but technically
it is a different code.
> As far as I understand, it is easier to share queues with DPDK from kernel
> when the device supports an IOMMU.
>
>> The question is how to submit the base driver which is pretty big. Mail
>> size of the patch which imports it is about 2 Mb.
> First answer is a question:
> Have you thought about cooperating with the kernel driver for your PMD?
Yes, we considered it, but decided that we need pure userspace driver since
the approach has its advantages: no specific dependencies from kernel,
the same PMD for Linux and FreeBSD etc.
> If you really cannot use this approach, then we have to maintain this
> whole base driver in DPDK.
> It will be easier to read, understand and reference if it is a bit split.
> Could you try to send it as 10 to 20 patches explaining the role of each
> part and giving some design details?
First of all I'd like to double check that it is clear that we discuss
libefx
(base driver in terms of DPDK) import here. The PMD itself is already split
in 20+ patches.
The only thing which comes to my mind is to split libefx import on subsystem
basis (few files per subsystem). It is artificial and added files will
be abandoned
until the patch which adds them into build. It could be something like:
1. External interfaces definition
2. Internal interfaces definition
3. Registers definition (hardware interface)
4. Management CPU interface definition (it is one file, but still big
650K)
5. Management CPU interface implementation
and so on for NIC global controls, interrupts, event queue, transmit,
receive,
filtering etc.
> It would be also really appreciated to provide a design documentation
> in doc/guides/nics. Are the datasheets open? A link in the doc would help.
We have a documentation which grows together with supported features,
but it is rather for users. Important design decisions (not so many) are
documented nearby corresponding code. Unfortunately there is no open
datasheets. Management CPU interface definition has comments.
> Please be prepare to work on several iterations of the patch series.
We have already passed a number of iterations internally, so it will not
frighten.
> PS: the mailing list put emails exceeding 300KB into a moderation queue.
Nice to know that it is not completely rejected, since even if we split as
described above, we still have one candidate which will end-up in moderation
queue.
Thanks,
Andrew.
^ permalink raw reply
* Re: [PATCH v11 1/6] ethdev: add Tx preparation
From: Richardson, Bruce @ 2016-10-28 11:02 UTC (permalink / raw)
To: Ananyev, Konstantin, Thomas Monjalon, Kulasek, TomaszX; +Cc: dev@dpdk.org
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CEB79@irsmsx105.ger.corp.intel.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Friday, October 28, 2016 11:29 AM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>; Kulasek, TomaszX
> <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
>
>
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Friday, October 28, 2016 11:22 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Kulasek,
> > TomaszX <tomaszx.kulasek@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
> >
> > 2016-10-28 10:15, Ananyev, Konstantin:
> > > > From: Ananyev, Konstantin
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2016-10-27 15:52, Ananyev, Konstantin:
> > > > > > > > 2016-10-26 14:56, Tomasz Kulasek:
> > > > > > > > > --- a/config/common_base
> > > > > > > > > +++ b/config/common_base
> > > > > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y
> > > > > > > >
> > > > > > > > We cannot enable it until it is implemented in every
> drivers.
> > > > > > >
> > > > > > > Not sure why?
> > > > > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act
> as noop.
> > > > > > > Right now it is not mandatory for the PMD to implement it.
> > > > > >
> > > > > > If it is not implemented, the application must do the
> > > > > > preparation by
> > > > > itself.
> > > > > > From patch 6:
> > > > > > "
> > > > > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > > > > application and used Tx preparation API for packet preparation
> > > > > > and verification.
> > > > > > "
> > > > > > So how does it behave with other drivers?
> > > > >
> > > > > Hmm so it seems that we broke testpmd csumonly mode for
> > > > > non-intel drivers..
> > > > > My bad, missed that part completely.
> > > > > Yes, then I suppose for now we'll need to support both (with and
> > > > > without) code paths for testpmd.
> > > > > Probably a new fwd mode or just extra parameter for the existing
> one?
> > > > > Any other suggestions?
> > > > >
> > > >
> > > > I had sent txprep engine in v2
> > > > (http://dpdk.org/dev/patchwork/patch/15775/), but I'm opened on
> > > > the suggestions. If you like it I can
> > resent
> > > > it in place of csumonly modification.
> > >
> > > I still not sure it is worth to have another version of csum...
> > > Can we introduce a new global variable in testpmd and a new command:
> > > testpmd> csum tx_prep
> > > or so?
> > > Looking at current testpmd patch, I suppose the changes will be
> minimal.
> > > What do you think?
> >
> > No please no!
> > The problem is not in testpmd.
> > The problem is in every applications.
> > Should we prepare the checksums or let tx_prep do it?
>
> Not sure, I understood you...
> Right now we don't' change other apps.
> They would work as before.
> If people would like to start to use tx_prep in their apps - they are free
> to do that.
> If they like to keep doing that manually - that's fine too.
> From other side we need an ability to test (and demonstrate) that new
> functionality.
> So we do need changes in testpmd.
> Konstantin
>
Just my 2c on this:
* given this is new functionality, and no apps are currently using it, I'm not sure I see the harm in having the function available by default. We just need to be clear about the limits of the function and the fact that apps need to do work themselves if the driver doesn't provide the function.
* having it enabled will then allow any apps that want to use it do to so.
* however, for our sample apps, and by default in testpmd, we *shouldn't* use this functionality, in the absence of any fallback, so that is where I would look to have the enable/disable switch, not in the library.
* going forward, I think a SW fallback inside the ethdev API itself would be a good addition to make this fully generic.
Hope this helps, [and also that I haven't missed some subtlety in the discussion!]
/Bruce
^ permalink raw reply
* Re: [PATCH v3 2/2] net/i40e: fix VF bonded device link down
From: Ferruh Yigit @ 2016-10-28 11:05 UTC (permalink / raw)
To: Qiming Yang, dev
In-Reply-To: <1477628339-5816-2-git-send-email-qiming.yang@intel.com>
Hi Qiming,
On 10/28/2016 5:18 AM, Qiming Yang wrote:
> If VF device is used as slave of a bond device, it will be polled
> periodically through alarm. Interrupt is involved here. And then
> VF will send I40E_VIRTCHNL_OP_GET_LINK_STAT message to
> PF to query the status. The response is handled by interrupt
> callback. Interrupt is involved here again. That's why bond
> device cannot bring up.
>
> This patch removes I40E_VIRTCHNL_OP_GET_LINK_STAT
> message. Link status in VF driver will be updated when PF driver
> notify it, and VF stores this link status locally. VF driver just
> returns the local status when being required.
>
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> ---
<...>
> static void
> +i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev)
> +{
> + struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> + struct i40e_virtchnl_pf_event event;
> + int i;
> +
> + event.event = I40E_VIRTCHNL_EVENT_LINK_CHANGE;
> + event.event_data.link_event.link_status =
> + dev->data->dev_link.link_status;
> + event.event_data.link_event.link_speed =
> + dev->data->dev_link.link_speed;
This gives a compilation error with icc [1], casting to enum will be
enough to fix I guess.
[1]
.../drivers/net/i40e/i40e_ethdev.c(5453): error #188: enumerated type
mixed with another type
event.event_data.link_event.link_speed =
^
<...>
> diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
> index d5b2d45..350f6a0 100644
<...>
>
> +static void
> +i40e_notify_vf_link_status(struct rte_eth_dev *dev, struct i40e_pf_vf *vf)
> +{
> + struct i40e_virtchnl_pf_event event;
> +
> + event.event = I40E_VIRTCHNL_EVENT_LINK_CHANGE;
> + event.event_data.link_event.link_status =
> + dev->data->dev_link.link_status;
> + event.event_data.link_event.link_speed =
> + dev->data->dev_link.link_speed;
Same error here:
.../drivers/net/i40e/i40e_pf.c(908): error #188: enumerated type mixed
with another type
event.event_data.link_event.link_speed =
^
And while sending a new version of the patch, can you please:
1- CC the maintainers
2- Patchset already acked by Jingjing, keep the ack in the patches.
Thanks,
ferruh
^ permalink raw reply
* Re: [PATCH v11 1/6] ethdev: add Tx preparation
From: Jerin Jacob @ 2016-10-28 11:14 UTC (permalink / raw)
To: Ananyev, Konstantin; +Cc: dev@dpdk.org, Thomas Monjalon
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CEAEE@irsmsx105.ger.corp.intel.com>
On Fri, Oct 28, 2016 at 10:15:47AM +0000, Ananyev, Konstantin wrote:
> Hi Tomasz,
>
> > > > > Not sure why?
> > > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as noop.
> > > > > Right now it is not mandatory for the PMD to implement it.
> > > >
> > > > If it is not implemented, the application must do the preparation by
> > > itself.
> > > > From patch 6:
> > > > "
> > > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > > application and used Tx preparation API for packet preparation and
> > > > verification.
> > > > "
> > > > So how does it behave with other drivers?
> > >
> > > Hmm so it seems that we broke testpmd csumonly mode for non-intel
> > > drivers..
> > > My bad, missed that part completely.
> > > Yes, then I suppose for now we'll need to support both (with and without)
> > > code paths for testpmd.
> > > Probably a new fwd mode or just extra parameter for the existing one?
> > > Any other suggestions?
> > >
> >
> > I had sent txprep engine in v2 (http://dpdk.org/dev/patchwork/patch/15775/), but I'm opened on the suggestions. If you like it I can resent
> > it in place of csumonly modification.
>
> I still not sure it is worth to have another version of csum...
> Can we introduce a new global variable in testpmd and a new command:
> testpmd> csum tx_prep
Just my 2 cents, As "tx_prep" is a generic API and if PMD tries to
fix-up some other limitation(not csum) then in that case it is difficult for
the application to know in which PMD&application combination it needs be used.
> or so?
> Looking at current testpmd patch, I suppose the changes will be minimal.
> What do you think?
> Konstantin
>
> >
> > Tomasz
> >
> > > >
> > > > > > > struct rte_eth_dev {
> > > > > > > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive
> > > function. */
> > > > > > > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit
> > > > > > > function. */
> > > > > > > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit
> > > > > > > +prepare function. */
> > > > > > > struct rte_eth_dev_data *data; /**< Pointer to device data */
> > > > > > > const struct eth_driver *driver;/**< Driver for this device */
> > > > > > > const struct eth_dev_ops *dev_ops; /**< Functions exported by
> > > > > > > PMD */
> > > > > >
> > > > > > Could you confirm why tx_pkt_prep is not in dev_ops?
> > > > > > I guess we want to have several implementations?
> > > > >
> > > > > Yes, it depends on configuration options, same as tx_pkt_burst.
> > > > >
> > > > > >
> > > > > > Shouldn't we have a const struct control_dev_ops and a struct
> > > datapath_dev_ops?
> > > > >
> > > > > That's probably a good idea, but I suppose it is out of scope for that
> > > patch.
> > > >
> > > > No it's not out of scope.
> > > > It answers to the question "why is it added in this structure and not
> > > dev_ops".
> > > > We won't do this change when nothing else is changed in the struct.
> > >
> > > Not sure I understood you here:
> > > Are you saying datapath_dev_ops/controlpath_dev_ops have to be introduced
> > > as part of that patch?
> > > But that's a lot of changes all over rte_ethdev.[h,c].
> > > It definitely worse a separate patch (might be some discussion) for me.
> > > Konstantin
> > >
> > >
>
^ permalink raw reply
* Re: [PATCH v11 1/6] ethdev: add Tx preparation
From: Ananyev, Konstantin @ 2016-10-28 11:29 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev@dpdk.org
In-Reply-To: <2500924.jYNDaNt7Th@xps13>
Hi Thomasz,
>
> 2016-10-27 16:24, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-10-27 15:52, Ananyev, Konstantin:
> > > > > Hi Tomasz,
> > > > >
> > > > > This is a major new function in the API and I still have some comments.
> > > > >
> > > > > 2016-10-26 14:56, Tomasz Kulasek:
> > > > > > --- a/config/common_base
> > > > > > +++ b/config/common_base
> > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y
> > > > >
> > > > > We cannot enable it until it is implemented in every drivers.
> > > >
> > > > Not sure why?
> > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as noop.
> > > > Right now it is not mandatory for the PMD to implement it.
> > >
> > > If it is not implemented, the application must do the preparation by itself.
> > > From patch 6:
> > > "
> > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > application and used Tx preparation API for packet preparation and
> > > verification.
> > > "
> > > So how does it behave with other drivers?
> >
> > Hmm so it seems that we broke testpmd csumonly mode for non-intel drivers..
> > My bad, missed that part completely.
> > Yes, then I suppose for now we'll need to support both (with and without) code paths for testpmd.
> > Probably a new fwd mode or just extra parameter for the existing one?
> > Any other suggestions?
>
> Please think how we can use it in every applications.
> It is not ready.
> Either we introduce the API without enabling it, or we implement it
> in every drivers.
I understand your position here, but just like to point that:
1) It is a new functionality optional to use.
The app is free not to use that functionality and still do the preparation itself
(as it has to do it now).
All existing apps would keep working as expected without using that function.
Though if the app developer knows that for all HW models he plans to run on
tx_prep is implemented - he is free to use it.
2) It would be difficult for Tomasz (and other Intel guys) to implement tx_prep()
for all non-Intel HW that DPDK supports right now.
We just don't have all the actual HW in stock and probably adequate knowledge of it.
So we depend here on the good will of other PMD mainaners/developers to implement
tx_prep() for these devices.
From other side, if it will be disabled by default, then, I think,
PMD developers just wouldn't be motivated to implement it.
So it will be left untested and unused forever.
>
> > > > > > struct rte_eth_dev {
> > > > > > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
> > > > > > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
> > > > > > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit prepare function. */
> > > > > > struct rte_eth_dev_data *data; /**< Pointer to device data */
> > > > > > const struct eth_driver *driver;/**< Driver for this device */
> > > > > > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> > > > >
> > > > > Could you confirm why tx_pkt_prep is not in dev_ops?
> > > > > I guess we want to have several implementations?
> > > >
> > > > Yes, it depends on configuration options, same as tx_pkt_burst.
> > > >
> > > > >
> > > > > Shouldn't we have a const struct control_dev_ops and a struct datapath_dev_ops?
> > > >
> > > > That's probably a good idea, but I suppose it is out of scope for that patch.
> > >
> > > No it's not out of scope.
> > > It answers to the question "why is it added in this structure and not dev_ops".
> > > We won't do this change when nothing else is changed in the struct.
> >
> > Not sure I understood you here:
> > Are you saying datapath_dev_ops/controlpath_dev_ops have to be introduced as part of that patch?
> > But that's a lot of changes all over rte_ethdev.[h,c].
> > It definitely worse a separate patch (might be some discussion) for me.
>
> Yes it could be a separate patch in the same patchset.
Honestly, I think it is a good idea, but it is too late and too risky to do such change right now.
We are on RC2 right now, just few days before RC3...
Can't that wait till 17.02?
>From my understanding - it is pure code restructuring, without any functionality affected.
Konstantin
^ permalink raw reply
* Re: [PATCH v11 1/6] ethdev: add Tx preparation
From: Ananyev, Konstantin @ 2016-10-28 11:34 UTC (permalink / raw)
To: Ananyev, Konstantin, Thomas Monjalon; +Cc: dev@dpdk.org
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CEC55@irsmsx105.ger.corp.intel.com>
>
> Hi Thomasz,
>
> >
> > 2016-10-27 16:24, Ananyev, Konstantin:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2016-10-27 15:52, Ananyev, Konstantin:
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > This is a major new function in the API and I still have some comments.
> > > > > >
> > > > > > 2016-10-26 14:56, Tomasz Kulasek:
> > > > > > > --- a/config/common_base
> > > > > > > +++ b/config/common_base
> > > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y
> > > > > >
> > > > > > We cannot enable it until it is implemented in every drivers.
> > > > >
> > > > > Not sure why?
> > > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act as noop.
> > > > > Right now it is not mandatory for the PMD to implement it.
> > > >
> > > > If it is not implemented, the application must do the preparation by itself.
> > > > From patch 6:
> > > > "
> > > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > > application and used Tx preparation API for packet preparation and
> > > > verification.
> > > > "
> > > > So how does it behave with other drivers?
> > >
> > > Hmm so it seems that we broke testpmd csumonly mode for non-intel drivers..
> > > My bad, missed that part completely.
> > > Yes, then I suppose for now we'll need to support both (with and without) code paths for testpmd.
> > > Probably a new fwd mode or just extra parameter for the existing one?
> > > Any other suggestions?
> >
> > Please think how we can use it in every applications.
> > It is not ready.
> > Either we introduce the API without enabling it, or we implement it
> > in every drivers.
>
> I understand your position here, but just like to point that:
> 1) It is a new functionality optional to use.
> The app is free not to use that functionality and still do the preparation itself
> (as it has to do it now).
> All existing apps would keep working as expected without using that function.
> Though if the app developer knows that for all HW models he plans to run on
> tx_prep is implemented - he is free to use it.
> 2) It would be difficult for Tomasz (and other Intel guys) to implement tx_prep()
> for all non-Intel HW that DPDK supports right now.
> We just don't have all the actual HW in stock and probably adequate knowledge of it.
> So we depend here on the good will of other PMD mainaners/developers to implement
> tx_prep() for these devices.
> From other side, if it will be disabled by default, then, I think,
> PMD developers just wouldn't be motivated to implement it.
> So it will be left untested and unused forever.
Actually as another thought:
Can we have it enabled by default, but mark it as experimental or so?
If memory serves me right, we've done that for cryptodev in the past, no?
Konstantin
>
> >
> > > > > > > struct rte_eth_dev {
> > > > > > > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
> > > > > > > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
> > > > > > > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit prepare function. */
> > > > > > > struct rte_eth_dev_data *data; /**< Pointer to device data */
> > > > > > > const struct eth_driver *driver;/**< Driver for this device */
> > > > > > > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> > > > > >
> > > > > > Could you confirm why tx_pkt_prep is not in dev_ops?
> > > > > > I guess we want to have several implementations?
> > > > >
> > > > > Yes, it depends on configuration options, same as tx_pkt_burst.
> > > > >
> > > > > >
> > > > > > Shouldn't we have a const struct control_dev_ops and a struct datapath_dev_ops?
> > > > >
> > > > > That's probably a good idea, but I suppose it is out of scope for that patch.
> > > >
> > > > No it's not out of scope.
> > > > It answers to the question "why is it added in this structure and not dev_ops".
> > > > We won't do this change when nothing else is changed in the struct.
> > >
> > > Not sure I understood you here:
> > > Are you saying datapath_dev_ops/controlpath_dev_ops have to be introduced as part of that patch?
> > > But that's a lot of changes all over rte_ethdev.[h,c].
> > > It definitely worse a separate patch (might be some discussion) for me.
> >
> > Yes it could be a separate patch in the same patchset.
>
> Honestly, I think it is a good idea, but it is too late and too risky to do such change right now.
> We are on RC2 right now, just few days before RC3...
> Can't that wait till 17.02?
> From my understanding - it is pure code restructuring, without any functionality affected.
> Konstantin
>
^ permalink raw reply
* [PATCH] app/test: fix wrong pointer values in crypto perftest
From: Arek Kusztal @ 2016-10-28 11:37 UTC (permalink / raw)
To: dev
This commit fixes problem with device hanging because of
wrong pointer values in snow3g performance test
Fixes: 97fe6461c7cb ("app/test: add SNOW 3G performance test")
Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
---
app/test/test_cryptodev_perf.c | 101 ++++++++++++++++++++++++++++++++++++-----
1 file changed, 90 insertions(+), 11 deletions(-)
diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
index 53dd8f5..59a6891 100644
--- a/app/test/test_cryptodev_perf.c
+++ b/app/test/test_cryptodev_perf.c
@@ -2565,6 +2565,8 @@ test_perf_create_aes_sha_session(uint8_t dev_id, enum chain_mode chain,
}
}
+#define SNOW3G_CIPHER_IV_LENGTH 16
+
static struct rte_cryptodev_sym_session *
test_perf_create_snow3g_session(uint8_t dev_id, enum chain_mode chain,
enum rte_crypto_cipher_algorithm cipher_algo, unsigned cipher_key_len,
@@ -2587,6 +2589,7 @@ test_perf_create_snow3g_session(uint8_t dev_id, enum chain_mode chain,
auth_xform.auth.op = RTE_CRYPTO_AUTH_OP_GENERATE;
auth_xform.auth.algo = auth_algo;
+ auth_xform.auth.add_auth_data_length = SNOW3G_CIPHER_IV_LENGTH;
auth_xform.auth.key.data = snow3g_hash_key;
auth_xform.auth.key.length = get_auth_key_max_length(auth_algo);
auth_xform.auth.digest_length = get_auth_digest_length(auth_algo);
@@ -2686,8 +2689,6 @@ test_perf_create_openssl_session(uint8_t dev_id, enum chain_mode chain,
#define TRIPLE_DES_BLOCK_SIZE 8
#define TRIPLE_DES_CIPHER_IV_LENGTH 8
-#define SNOW3G_CIPHER_IV_LENGTH 16
-
static struct rte_mbuf *
test_perf_create_pktmbuf(struct rte_mempool *mpool, unsigned buf_sz)
{
@@ -2812,6 +2813,69 @@ test_perf_set_crypto_op_snow3g(struct rte_crypto_op *op, struct rte_mbuf *m,
}
static inline struct rte_crypto_op *
+test_perf_set_crypto_op_snow3g_cipher(struct rte_crypto_op *op,
+ struct rte_mbuf *m,
+ struct rte_cryptodev_sym_session *sess,
+ unsigned data_len)
+{
+ if (rte_crypto_op_attach_sym_session(op, sess) != 0) {
+ rte_crypto_op_free(op);
+ return NULL;
+ }
+
+ /* Cipher Parameters */
+ op->sym->cipher.iv.data = rte_pktmbuf_mtod(m, uint8_t *);
+ op->sym->cipher.iv.length = SNOW3G_CIPHER_IV_LENGTH;
+ rte_memcpy(op->sym->cipher.iv.data, snow3g_iv, SNOW3G_CIPHER_IV_LENGTH);
+ op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m);
+
+ op->sym->cipher.data.offset = SNOW3G_CIPHER_IV_LENGTH;
+ op->sym->cipher.data.length = data_len << 3;
+
+ op->sym->m_src = m;
+
+ return op;
+}
+
+
+static inline struct rte_crypto_op *
+test_perf_set_crypto_op_snow3g_hash(struct rte_crypto_op *op,
+ struct rte_mbuf *m,
+ struct rte_cryptodev_sym_session *sess,
+ unsigned data_len,
+ unsigned digest_len)
+{
+ if (rte_crypto_op_attach_sym_session(op, sess) != 0) {
+ rte_crypto_op_free(op);
+ return NULL;
+ }
+
+ /* Authentication Parameters */
+
+ op->sym->auth.digest.data =
+ (uint8_t *)rte_pktmbuf_mtod_offset(m, uint8_t *,
+ data_len);
+ op->sym->auth.digest.phys_addr =
+ rte_pktmbuf_mtophys_offset(m, data_len +
+ SNOW3G_CIPHER_IV_LENGTH);
+ op->sym->auth.digest.length = digest_len;
+ op->sym->auth.aad.data = rte_pktmbuf_mtod(m, uint8_t *);
+ op->sym->auth.aad.length = SNOW3G_CIPHER_IV_LENGTH;
+ rte_memcpy(op->sym->auth.aad.data, snow3g_iv,
+ SNOW3G_CIPHER_IV_LENGTH);
+ op->sym->auth.aad.phys_addr = rte_pktmbuf_mtophys(m);
+
+ /* Data lengths/offsets Parameters */
+ op->sym->auth.data.offset = SNOW3G_CIPHER_IV_LENGTH;
+ op->sym->auth.data.length = data_len << 3;
+
+ op->sym->m_src = m;
+
+ return op;
+}
+
+
+static inline struct rte_crypto_op *
test_perf_set_crypto_op_3des(struct rte_crypto_op *op, struct rte_mbuf *m,
struct rte_cryptodev_sym_session *sess, unsigned int data_len,
unsigned int digest_len)
@@ -3017,9 +3081,14 @@ test_perf_snow3g(uint8_t dev_id, uint16_t queue_id,
/* Generate a burst of crypto operations */
for (i = 0; i < (pparams->burst_size * NUM_MBUF_SETS); i++) {
+ /*
+ * Buffer size + iv/aad len is allocated, for perf tests they
+ * are equal + digest len.
+ */
mbufs[i] = test_perf_create_pktmbuf(
ts_params->mbuf_mp,
- pparams->buf_size);
+ pparams->buf_size + SNOW3G_CIPHER_IV_LENGTH +
+ digest_length);
if (mbufs[i] == NULL) {
printf("\nFailed to get mbuf - freeing the rest.\n");
@@ -3049,12 +3118,22 @@ test_perf_snow3g(uint8_t dev_id, uint16_t queue_id,
/*Don't exit, dequeue, more ops should become available*/
} else {
for (i = 0; i < ops_needed; i++) {
- ops[i+op_offset] =
- test_perf_set_crypto_op_snow3g(ops[i+op_offset],
- mbufs[i +
- (pparams->burst_size * (j % NUM_MBUF_SETS))],
- sess,
- pparams->buf_size, digest_length);
+ if (pparams->chain == HASH_ONLY)
+ ops[i+op_offset] =
+ test_perf_set_crypto_op_snow3g_hash(ops[i+op_offset],
+ mbufs[i +
+ (pparams->burst_size * (j % NUM_MBUF_SETS))],
+ sess,
+ pparams->buf_size, digest_length);
+ else if (pparams->chain == CIPHER_ONLY)
+ ops[i+op_offset] =
+ test_perf_set_crypto_op_snow3g_cipher(ops[i+op_offset],
+ mbufs[i +
+ (pparams->burst_size * (j % NUM_MBUF_SETS))],
+ sess,
+ pparams->buf_size);
+ else
+ return 1;
}
/* enqueue burst */
@@ -3372,8 +3451,8 @@ test_perf_snow3G_vary_pkt_size(void)
unsigned total_operations = 1000000;
uint8_t i, j;
unsigned k;
- uint16_t burst_sizes[] = {64};
- uint16_t buf_lengths[] = {40, 64, 80, 120, 240, 256, 400, 512, 600, 1024, 2048};
+ uint16_t burst_sizes[] = { 64 };
+ uint16_t buf_lengths[] = { 40, 64, 80, 120, 240, 256, 400, 512, 600, 1024, 2048 };
struct perf_test_params params_set[] = {
{
--
2.1.0
^ permalink raw reply related
* Re: [PATCH] net/mlx5: fix default set for multicast traffic
From: Adrien Mazarguil @ 2016-10-28 12:07 UTC (permalink / raw)
To: Raslan Darawsheh; +Cc: dev
In-Reply-To: <2c3d48cf2736dcc6824e39c911666521befdd918.1477295827.git.rasland@mellanox.com>
On Mon, Oct 24, 2016 at 10:59:14AM +0300, Raslan Darawsheh wrote:
> Remove non-IPv6 multicast traffic with destination MAC 33:33:* from the
> default set when not in promiscuous or allmulticast modes.
>
> Fixes: 0497ddaac511 ("mlx5: add special flows for broadcast and IPv6 multicast")
>
> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> ---
> drivers/net/mlx5/mlx5_rxmode.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/mlx5/mlx5_rxmode.c b/drivers/net/mlx5/mlx5_rxmode.c
> index 173e6e8..4ffe703 100644
> --- a/drivers/net/mlx5/mlx5_rxmode.c
> +++ b/drivers/net/mlx5/mlx5_rxmode.c
> @@ -104,7 +104,6 @@ static const struct special_flow_init special_flow_init[] = {
> .hash_types =
> 1 << HASH_RXQ_UDPV6 |
> 1 << HASH_RXQ_IPV6 |
> - 1 << HASH_RXQ_ETH |
> 0,
> .per_vlan = 1,
> },
> --
> 1.9.1
(NACK)
While technically correct, looks like this patch sometimes break IPv6
multicast traffic as well, let's drop it until we figure out the reason.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH] net/mlx5: fix handling of small mbuf sizes
From: Adrien Mazarguil @ 2016-10-28 12:19 UTC (permalink / raw)
To: Raslan Darawsheh; +Cc: dev
In-Reply-To: <937aac679e571c4fff8ab720f4dd910fe10a283e.1477296585.git.rasland@mellanox.com>
On Mon, Oct 24, 2016 at 11:10:59AM +0300, Raslan Darawsheh wrote:
> When mbufs are smaller than MRU, multi-segment support must be enabled to
> default set when not in promiscuous or allmulticast modes.
>
> Fixes: 9964b965ad69 ("net/mlx5: re-add Rx scatter support")
>
> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> ---
> drivers/net/mlx5/mlx5_rxq.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index 4dc5cc3..62253ed 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -946,6 +946,12 @@ rxq_ctrl_setup(struct rte_eth_dev *dev, struct rxq_ctrl *rxq_ctrl,
> (void)conf; /* Thresholds configuration (ignored). */
> /* Enable scattered packets support for this queue if necessary. */
> assert(mb_len >= RTE_PKTMBUF_HEADROOM);
> + /* If smaller than MRU, multi-segment support must be enabled. */
> + if (mb_len < (priv->mtu > dev->data->dev_conf.rxmode.max_rx_pkt_len ?
> + dev->data->dev_conf.rxmode.max_rx_pkt_len :
> + priv->mtu
> + ))
Let's move poor "))" to the end of the previous line.
> + dev->data->dev_conf.rxmode.jumbo_frame = 1;
> if ((dev->data->dev_conf.rxmode.jumbo_frame) &&
> (dev->data->dev_conf.rxmode.max_rx_pkt_len >
> (mb_len - RTE_PKTMBUF_HEADROOM))) {
> --
> 1.9.1
Besides the above comment:
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
--
Adrien Mazarguil
6WIND
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox