[dpdk-dev] DPDK techboard minutes of July 05
Hi all, Here is the meeting notes for the last DPDK technical board meeting held on 2017-07-05. Member attendees: - Hemant Agrawal - Jan Blunck - Jerin Jacob - Olivier Matz - Stephen Hemminger - Thomas Monjalon - Yuanhan Liu 0. Release blocking issues No major issues, but as always, we lack of reviews. 1. Check action points from minutes of last meeting - The repo for the new build system is created. - Dom0 support will be marked as deprecated this release and be removed in the next release (v17.11). 2. Release backups Thomas proposed to have some release backups [0], so that there is always someone to take this role in case of accident or long vacations. And the board approves to let Ferruh Yigit and Yuanhan Liu be the release backups. Note it's not for sharing Thomas load; it's just backups. [0]: http://dpdk.org/ml/archives/dev/2017-June/069229.html 3. Next meeting Hemant will chair.
Re: [dpdk-dev] [PATCH 2/2] testpmd: give more hint on invalid RETA size
On Mon, Jul 10, 2017 at 01:19:06AM +, Wu, Jingjing wrote:
> > -Original Message-
> > From: Yuanhan Liu [mailto:y...@fridaylinux.org]
> > Subject: [PATCH 2/2] testpmd: give more hint on invalid RETA size
> >
> > Print the valid RTE size range so that user knows what goes wrong.
> >
> > Signed-off-by: Yuanhan Liu
> > ---
> > app/test-pmd/cmdline.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> > index c8faef9..d32a1df 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -2140,12 +2140,14 @@ cmd_showport_reta_parsed(void *parsed_result,
> > struct cmd_showport_reta *res = parsed_result;
> > struct rte_eth_rss_reta_entry64 reta_conf[8];
> > struct rte_eth_dev_info dev_info;
> > + uint16_t max_reta_size;
> >
> > memset(&dev_info, 0, sizeof(dev_info));
> > rte_eth_dev_info_get(res->port_id, &dev_info);
> > - if (dev_info.reta_size == 0 || res->size > dev_info.reta_size ||
> > - res->size > ETH_RSS_RETA_SIZE_512) {
> > - printf("Invalid redirection table size: %u\n", res->size);
> > + max_reta_size = RTE_MIN(dev_info.reta_size, ETH_RSS_RETA_SIZE_512);
> > + if (res->size == 0 || res->size > max_reta_size) {
> > + printf("Invalid redirection table size: %u (1-%u)\n",
> > + res->size, max_reta_size);
> > return;
> > }
> >
> Why not merge this with previous one?
Because they are two differnt things, even though each of them is really tiny.
--yliu
Re: [dpdk-dev] [PATCH v10 20/20] ethdev: add control interface support
On Tue, Jul 04, 2017 at 05:13:37PM +0100, Ferruh Yigit wrote:
> @@ -157,8 +164,12 @@ rte_eth_dev_pci_generic_probe(struct rte_pci_device
> *pci_dev,
>
> RTE_FUNC_PTR_OR_ERR_RET(*dev_init, -EINVAL);
> ret = dev_init(eth_dev);
> - if (ret)
> + if (ret) {
> rte_eth_dev_pci_release(eth_dev);
> + return ret;
> + }
> +
> + rte_eth_control_interface_create(eth_dev->data->port_id);
Hi,
So you are creating a virtual kernel interface for each PCI port. What
about the VDEVs? If you plan to create one for each port, why not create
it at the stage while allocating the eth device, or at the stage while
starting the port if the former is too earlier?
Another thing comes to my mind is have you tried it with multi-process
model? Looks like it will create the control interface twice? Or it will
just be failed since the interface already exists?
I also have few questions regarding the whole design. So seems that the
ctrl_if only exports two APIs and they all will be only used in the EAL
layer. Thus, one question is did you plan to let APP use them? Judging
EAL already calls them automatically, I don't think it makes sense to
let the APP call it again. That being said, what's the point of the making
it be an lib? Why not just put it under EAL or somewhere else, and let
EAL invoke it as normal helper functions (instead of by public APIs)?
I will avoid adding a new lib if possible. Otherwise, it increases the
chance of ABI/API breakage is needed in future for extensions.
The same question goes to the ethtool lib. Since your solution can work
well with the well-known ethtool, which is also way more widely available
than the DPDK ethtool app, what's the point of keeping the ethtool app
then? Like above, I also don't think those APIs are meant for APPs (or
are they?). Thus, with the ethtool app removed, we then could again avoid
introducing a new lib.
--yliu
Re: [dpdk-dev] [PATCH 0/2] virtio fix false offload claims
On Fri, Jul 07, 2017 at 12:52:48PM -0700, Stephen Hemminger wrote: > While doing code for Hyper-V, noticed that the virtio driver was > confused about receive versus transmit offloads. The virtio > checksum offload is L4 (TCP/UDP) only, not IPv4. Also, TSO > and LRO are not the same. > > This may break some program that was assuming it was getting offloads > that it wasn't. Applied to dpdk-next-virtio. And I think they should be backported to stable releases, thus, Cc: sta...@dpdk.org Thanks. --yliu > > Stephen Hemminger (2): > virtio: don't falsely claim to do IP checksum > virtio: don't claim to support LRO > > drivers/net/virtio/virtio_ethdev.c | 30 +- > 1 file changed, 5 insertions(+), 25 deletions(-) > > -- > 2.11.0
Re: [dpdk-dev] [PATCH v3] examples/vhost: introduce a new vhost-user-scsi sample application
On Fri, Jul 07, 2017 at 02:21:28PM +0200, Thomas Monjalon wrote: > 08/07/2017 07:21, Changpeng Liu: > > MAINTAINERS | 2 + > > doc/guides/sample_app_ug/vhost_scsi.rst | 110 +++ > > examples/Makefile | 2 +- > > examples/vhost_scsi/Makefile| 59 > > examples/vhost_scsi/scsi.c | 507 > > > > examples/vhost_scsi/scsi_spec.h | 488 > > ++ > > examples/vhost_scsi/vhost_scsi.c| 472 + > > examples/vhost_scsi/vhost_scsi.h| 250 > > 8 files changed, 1889 insertions(+), 1 deletion(-) > > I think you are missing to update the doc index. Oops, right, and fixed. Thanks! --yliu
Re: [dpdk-dev] [PATCH v3] examples/vhost: introduce a new vhost-user-scsi sample application
On Fri, Jul 07, 2017 at 02:10:57PM +0530, Hemant Agrawal wrote: > On 7/7/2017 10:37 AM, Yuanhan Liu wrote: > >On Sat, Jul 08, 2017 at 01:21:37PM +0800, Changpeng Liu wrote: > >>vhost-user protocol is common to many virtio devices, such as > >>virtio_net/virtio_scsi/virtio_blk. Since DPDK vhost library > >>removed the NET specific data structures, the vhost library > >>is common to other virtio devices, such as virtio-scsi. > >> > >>Here we introduce a simple memory based block device that > >>can be presented to Guest VM through vhost-user-scsi-pci > >>controller. Similar with vhost-net, the sample application > >>will process the I/Os sent via virt rings. > >> > >>Signed-off-by: Changpeng Liu > > > >Applied to dpdk-next-virtio. > > > > Though my comment is late. > I am just wondering if it is not better to refactor the existing vhost > applications and add this as a mode instead of adding one more sample > application? Ideally, yes. However, I don't find a straightforward way to do that for this scsi example, for following reasons: - Current vhost switch example was not well designed. Worse, it has has few NIC features bond, say VMDq and VLAN. - Vhost switch is for networking, while this one is for storage. To let them co-exist in one app, we have to firstly refactor current vhost-user example, which should be a big rework that I don't think it's worthwhile. Or, maybe we could put vhost_switch and vhost_scsi into one dir (say "vhost") under examples and let them look like one example (with 2 standalone apps), like what we did for multi_process examples? And we probably could do more, for example, the l2fwd, l3fwd and their variants? However, IMO, it just make the examples dir more cleanly, nothing else. Except someone will spend more time to refactor them, such as removing the duplicate code and introduce them as helper functions. Again, It may not be that worthwhile, judging that they are just examples. --yliu
[dpdk-dev] [PATCH 2/2] testpmd: give more hint on invalid RETA size
Print the valid RTE size range so that user knows what goes wrong.
Signed-off-by: Yuanhan Liu
---
app/test-pmd/cmdline.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index c8faef9..d32a1df 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2140,12 +2140,14 @@ cmd_showport_reta_parsed(void *parsed_result,
struct cmd_showport_reta *res = parsed_result;
struct rte_eth_rss_reta_entry64 reta_conf[8];
struct rte_eth_dev_info dev_info;
+ uint16_t max_reta_size;
memset(&dev_info, 0, sizeof(dev_info));
rte_eth_dev_info_get(res->port_id, &dev_info);
- if (dev_info.reta_size == 0 || res->size > dev_info.reta_size ||
- res->size > ETH_RSS_RETA_SIZE_512) {
- printf("Invalid redirection table size: %u\n", res->size);
+ max_reta_size = RTE_MIN(dev_info.reta_size, ETH_RSS_RETA_SIZE_512);
+ if (res->size == 0 || res->size > max_reta_size) {
+ printf("Invalid redirection table size: %u (1-%u)\n",
+ res->size, max_reta_size);
return;
}
--
2.7.4
[dpdk-dev] [PATCH 1/2] testpmd: allow to query any RETA size
Currently, testpmd just allows to query the RETA info only when the
required size equals to configured RETA size.
This patch allows to query any RETA size <= the configured size. This
helps when the RETA size is big (say 512) and when I just want to peak
few RETA entries.
Signed-off-by: Yuanhan Liu
---
app/test-pmd/cmdline.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 0afac68..c8faef9 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2143,7 +2143,7 @@ cmd_showport_reta_parsed(void *parsed_result,
memset(&dev_info, 0, sizeof(dev_info));
rte_eth_dev_info_get(res->port_id, &dev_info);
- if (dev_info.reta_size == 0 || res->size != dev_info.reta_size ||
+ if (dev_info.reta_size == 0 || res->size > dev_info.reta_size ||
res->size > ETH_RSS_RETA_SIZE_512) {
printf("Invalid redirection table size: %u\n", res->size);
return;
--
2.7.4
Re: [dpdk-dev] [PATCH v3] examples/vhost: introduce a new vhost-user-scsi sample application
On Sat, Jul 08, 2017 at 01:21:37PM +0800, Changpeng Liu wrote: > vhost-user protocol is common to many virtio devices, such as > virtio_net/virtio_scsi/virtio_blk. Since DPDK vhost library > removed the NET specific data structures, the vhost library > is common to other virtio devices, such as virtio-scsi. > > Here we introduce a simple memory based block device that > can be presented to Guest VM through vhost-user-scsi-pci > controller. Similar with vhost-net, the sample application > will process the I/Os sent via virt rings. > > Signed-off-by: Changpeng Liu Applied to dpdk-next-virtio. Thanks. --yliu
Re: [dpdk-dev] [PATCH v2] examples/vhost: introduce a new vhost-user-scsi sample application
On Sat, Jul 08, 2017 at 12:14:51PM +0800, Changpeng Liu wrote:
> +.. note::
> +You must check whether your Qemu can support "vhost-user-scsi" or not,
> +Qemu v2.9.50 or newer version is required.
QEMU v2.9.50 looks like a stable version, and I don't think they will
backport new features on it (or, do they?).
So, it should be "v2.10" here?
> + switch (pc) {
> + case SPC_VPD_SUPPORTED_VPD_PAGES:
> + hlen = 4;
> + vpage->params[0] = SPC_VPD_SUPPORTED_VPD_PAGES;
> + vpage->params[1] = SPC_VPD_UNIT_SERIAL_NUMBER;
> + vpage->params[2] = SPC_VPD_DEVICE_IDENTIFICATION;
> + len = 3;
> + /* PAGE LENGTH */
> + to_be16(vpage->alloc_len, len);
> + break;
You didn't resolve my previous comment regarding the "break"
indentation issue.
Otherwise, this patch looks good to me.
--yliu
Re: [dpdk-dev] [PATCH] examples/vhost: introduce a new vhost-user-scsi sample application
On Fri, Jul 07, 2017 at 02:00:38AM +, Liu, Changpeng wrote: > > > > -Original Message- > > From: Yuanhan Liu [mailto:y...@fridaylinux.org] > > Sent: Friday, July 7, 2017 8:51 AM > > To: Liu, Changpeng > > Cc: dev@dpdk.org > > Subject: Re: [PATCH] examples/vhost: introduce a new vhost-user-scsi sample > > application > > > > On Thu, Jun 29, 2017 at 05:28:32PM +0800, Changpeng Liu wrote: > > > +.. note:: > > > +You must check whether you Qemu can support "vhost-user-scsi" or not, > > > +the latest Qemu code is recommended. > > > > You should specify the least QEMU version that supports vhost-user scsi > > here. > > Okay, will add Qemu version and commit ID. Just the QEMU version has is enough. > > They are common pitfalls of vhost-user examples. You don't have to repeat > > it again here. Instead, you could add a link to the vhost-example doc and > > let this section only cover issues belong to vhost-user scsi. Make sure you will handle this comment in the next version. > > > + resp_code = 0x70; /* Current + Fixed format */ > > > > Could you add macros for those magic numbers? I saw quite a lot of them. > All the numbers are defined by SCSI specifications, I think this is okay, if > you looked into kernel scsi module or > kernel LIO scsi target, they also used the specification numbers directly. Okay. > > > > BTW, you might want to build it on a 32bit system. I think this patch > > won't build successfully on it. > I didn't build it with 32bit system before, I don't know the result. Nah, the build should be fine. I found it passes the build test: http://dpdk.org/ml/archives/test-report/2017-June/023091.html --yliu
Re: [dpdk-dev] [PATCH] examples/vhost: introduce a new vhost-user-scsi sample application
On Thu, Jun 29, 2017 at 05:28:32PM +0800, Changpeng Liu wrote:
> +.. note::
> +You must check whether you Qemu can support "vhost-user-scsi" or not,
> +the latest Qemu code is recommended.
You should specify the least QEMU version that supports vhost-user scsi here.
> +
> +Common Issues
> +-
> +
> +* QEMU fails to allocate memory on hugetlbfs, with an error like the
> + following::
> +
> + file_ram_alloc: can't mmap RAM pages: Cannot allocate memory
> +
> + When running QEMU the above error indicates that it has failed to allocate
> + memory for the Virtual Machine on the hugetlbfs. This is typically due to
> + insufficient hugepages being free to support the allocation request. The
> + number of free hugepages can be checked as follows:
> +
> + .. code-block:: console
> +
> + cat /sys/kernel/mm/hugepages/hugepages-/nr_hugepages
> +
> + The command above indicates how many hugepages are free to support QEMU's
> + allocation request.
> +
> +* vhost-user will not work with a QEMU version without shared memory mapping:
> +
> + Make sure ``share=on`` QEMU option is given.
They are common pitfalls of vhost-user examples. You don't have to repeat
it again here. Instead, you could add a link to the vhost-example doc and
let this section only cover issues belong to vhost-user scsi.
> +* vhost_scsi can not start with block size 512 Bytes:
> +
> + Currently DPDK vhost library was designed for NET device(althrough the APIs
> + are generic now), for 512 Bytes block device, Qemu BIOS(x86 BIOS Enhanced
> + Disk Device) will enumerate all block device and do some IO to those block
> + devices with 512 Bytes sector size. DPDK vhost library can not process such
> + scenarios(both BIOS and OS will enumerate the block device), so as a
> + workaround, the vhost_scsi example application hardcoded the block size
> + with 512 Bytes.
A bit confused here. You said we can not use 512 bytes while you hardcode it
with 512 bytes?
> +static void
> +scsi_task_build_sense_data(struct vhost_scsi_task *task, int sk,
> +int asc, int ascq)
> +{
> + uint8_t *cp;
> + int resp_code;
> +
> + resp_code = 0x70; /* Current + Fixed format */
Could you add macros for those magic numbers? I saw quite a lot of them.
BTW, you might want to build it on a 32bit system. I think this patch
won't build successfully on it.
--yliu
Re: [dpdk-dev] [PATCH] vhost: print the reason on numa node obtaining failure
On Thu, Jul 06, 2017 at 04:30:43PM +0300, Ilya Maximets wrote:
> syscall always returns '-1' on failure and there is no point
> in printing that value. 'errno' is much more informative.
>
> Fixes: 586e39001317 ("vhost: export numa node")
>
> Signed-off-by: Ilya Maximets
Applied to dpdk-next-virtio.
Thanks.
--yliu
Re: [dpdk-dev] [PATCH] net/virtio: fix the wrong comment
On Fri, Jun 23, 2017 at 12:14:45AM -0400, Yong Wang wrote: > Since "rte_eal_dev_init()" has been removed, the comment referred to > it should be modified simultaneously. > > Signed-off-by: Yong Wang Applied to dpdk-next-virtio. Thanks. --yliu
Re: [dpdk-dev] [PATCH] net/virtio-user: fix crash when detaching device
On Fri, Jun 23, 2017 at 08:41:37AM -0400, Allain Legacy wrote:
> The rte_eth_dev.data pointer is set to a reference to a static table.
> Attempting to rte_free() it leads to a panic. For example, the
> following commands result in a panic if run in testpmd
>
> testpmd> port attach virtio_user0,path=/dev/vhost-net,iface=test0
> testpmd> port stop 2
> testpmd> port close 2
> testpmd> port detach 2
>
> Fixes: ce2eabdd43ec ("net/virtio-user: add virtual device")
Note that it's prefered (and asked) to add stable tag inside the commit log.
Cc: sta...@dpdk.org
Applied to dpdk-next-virtio, with above tag added.
Thanks.
--yliu
Re: [dpdk-dev] [PATCH 2/3] vhost: check return values of pthread_* calls
On Tue, Jul 04, 2017 at 10:50:42AM +0200, Jens Freimann wrote:
> +out_mutex:
> + if (pthread_mutex_destroy(&vsocket->conn_mutex))
> + RTE_LOG(ERR, VHOST_CONFIG,
> + "error: failed to destroy connection mutex\n");
One minor comment though: {} should be used for one signle statement
spawning more than one lines. See section "1.6.2. Control Statements
and Loops":
http://dpdk.org/doc/guides/contributing/coding_style.html
Anyway, I fixed while apply.
--yliu
Re: [dpdk-dev] [PATCH 0/3] vhost: small cleanups
On Tue, Jul 04, 2017 at 10:50:40AM +0200, Jens Freimann wrote: > This series is a bunch of small cleanups regarding checking return values etc. Series applied to dpdk-next-virtio. You probably need update your script (if you are using one) about my email address. Thanks. --yliu
Re: [dpdk-dev] [PATCH v10 2/3] lib/gro: add TCP/IPv4 GRO support
Again, just some quick comments after a glimpse.
On Sat, Jul 01, 2017 at 07:08:42PM +0800, Jiayu Hu wrote:
> + for (i = 0; i < nb_pkts; i++) {
> + if (RTE_ETH_IS_IPV4_HDR(pkts[i]->packet_type) &&
> + (pkts[i]->packet_type & RTE_PTYPE_L4_TCP)) {
> + ret = gro_tcp4_reassemble(pkts[i],
> + &tcp_tbl,
> + param->max_packet_size,
> + current_time);
> + if (ret > 0)
> + /* merge successfully */
> + nb_after_gro--;
> + else if (ret < 0)
> + unprocess_pkts[unprocess_num++] =
> + pkts[i];
Even it's just one statement, if the statement is spawned more than
one line, including the comment, the {} should be used.
Section 1.6.2. Control Statements and Loops of:
http://dpdk.org/doc/guides/contributing/coding_style.html
> + } else
> + unprocess_pkts[unprocess_num++] =
> + pkts[i];
Besides, why breaking it to two lines, judging that it can be fit into
one line smaller than 80 chars.
> + }
> +
> + /* re-arrange GROed packets */
> + if (nb_after_gro < nb_pkts) {
> + i = gro_tcp4_tbl_timeout_flush(&tcp_tbl, 0,
> + pkts, nb_pkts);
> + if (unprocess_num > 0)
> + memcpy(&pkts[i], unprocess_pkts,
> + sizeof(struct rte_mbuf *) *
> + unprocess_num);
Ditto.
> +void *gro_tcp4_tbl_create(uint16_t socket_id,
> + uint16_t max_flow_num,
> + uint16_t max_item_per_flow)
> +{
> + size_t size;
> + uint32_t entries_num;
> + struct gro_tcp4_tbl *tbl;
> +
> + entries_num = max_flow_num * max_item_per_flow;
> + entries_num = entries_num > GRO_TCP4_TBL_MAX_ITEM_NUM ?
> + GRO_TCP4_TBL_MAX_ITEM_NUM : entries_num;
> +
> + if (entries_num == 0)
> + return NULL;
> +
> + tbl = (struct gro_tcp4_tbl *)rte_zmalloc_socket(
> + __func__,
> + sizeof(struct gro_tcp4_tbl),
> + RTE_CACHE_LINE_SIZE,
> + socket_id);
Again, the cast (from void *) is unnessary and should be dropped.
> + memcpy(&(tbl->keys[key_idx].key),
> + &key, sizeof(struct tcp4_key));
Again, I believe they two can be fit into one single line.
--yliu
Re: [dpdk-dev] [PATCH v10 1/3] lib: add Generic Receive Offload API framework
Haven't looked at the details yet, and below are some quick comments
after a glimpse.
On Sat, Jul 01, 2017 at 07:08:41PM +0800, Jiayu Hu wrote:
...
> +void *rte_gro_tbl_create(const
> + const struct rte_gro_param *param)
The DPDK style is:
void *
rte_gro_tbl_destroy(...)
Also you should revisit all other functions, as I have seen quite many
coding style issues like this.
> +{
> + gro_tbl_create_fn create_tbl_fn;
> + gro_tbl_destroy_fn destroy_tbl_fn;
> + struct gro_tbl *gro_tbl;
> + uint64_t gro_type_flag = 0;
> + uint8_t i, j;
> +
> + gro_tbl = rte_zmalloc_socket(__func__,
> + sizeof(struct gro_tbl),
> + RTE_CACHE_LINE_SIZE,
> + param->socket_id);
> + if (gro_tbl == NULL)
> + return NULL;
> + gro_tbl->max_packet_size = param->max_packet_size;
> + gro_tbl->max_timeout_cycles = param->max_timeout_cycles;
> + gro_tbl->desired_gro_types = param->desired_gro_types;
> +
> + for (i = 0; i < RTE_GRO_TYPE_MAX_NUM; i++) {
> + gro_type_flag = 1 << i;
> +
> + if ((param->desired_gro_types & gro_type_flag) == 0)
> + continue;
> + create_tbl_fn = tbl_create_functions[i];
> + if (create_tbl_fn == NULL)
> + continue;
> +
> + gro_tbl->tbls[i] = create_tbl_fn(
> + param->socket_id,
> + param->max_flow_num,
> + param->max_item_per_flow);
> + if (gro_tbl->tbls[i] == NULL) {
> + /* destroy all allocated tables */
> + for (j = 0; j < i; j++) {
> + gro_type_flag = 1 << j;
> + if ((param->desired_gro_types & gro_type_flag)
> == 0)
> + continue;
> + destroy_tbl_fn = tbl_destroy_functions[j];
> + if (destroy_tbl_fn)
> + destroy_tbl_fn(gro_tbl->tbls[j]);
> + }
> + rte_free(gro_tbl);
> + return NULL;
The typical way to handle this is to re-use rte_gro_tbl_destroy() as
much as possible. This saves duplicate code.
> + }
> + }
> + return gro_tbl;
> +}
> +
> +void rte_gro_tbl_destroy(void *tbl)
> +{
> + gro_tbl_destroy_fn destroy_tbl_fn;
> + struct gro_tbl *gro_tbl = (struct gro_tbl *)tbl;
The cast (from void *) is unnecessary and can be dropped.
...
> +/**
> + * the max packets number that rte_gro_reassemble_burst can
> + * process in each invocation.
> + */
> +#define RTE_GRO_MAX_BURST_ITEM_NUM 128UL
> +
> +/* max number of supported GRO types */
> +#define RTE_GRO_TYPE_MAX_NUM 64
> +#define RTE_GRO_TYPE_SUPPORT_NUM 0 /**< current supported GRO num */
The reason we need use comment style of "/**< ... */" is because this
is what the doc generator (doxygen) recognizes. If not doing this, your
comment won't be displayed at the generated doc page (for example,
http://dpdk.org/doc/api/rte__ethdev_8h.html#ade7de72f6c0f8102d01a0b3438856900).
The format, as far as I known, could be:
/**< here is a comment */
#define A_MACRO x
Or the one you did for RTE_GRO_TYPE_SUPPORT_NUM: put it at the end
of the line.
That being said, the comments for RTE_GRO_MAX_BURST_ITEM_NUM and
RTE_GRO_TYPE_MAX_NUM should be changed. Again, you should revisit
other places.
> +
> +
> +struct rte_gro_param {
> + uint64_t desired_gro_types; /**< desired GRO types */
> + uint32_t max_packet_size; /**< max length of merged packets */
> + uint16_t max_flow_num; /**< max flow number */
> + uint16_t max_item_per_flow; /**< max packet number per flow */
> +
> + /* socket index where the Ethernet port connects to */
Ditto.
...
> +++ b/lib/librte_gro/rte_gro_version.map
> @@ -0,0 +1,12 @@
> +DPDK_17.08 {
> + global:
> +
> + rte_gro_tbl_create;
> + rte_gro_tbl_destroy;
> + rte_gro_reassemble_burst;
> + rte_gro_reassemble;
> + rte_gro_timeout_flush;
> + rte_gro_tbl_item_num;
The undocumented habit is to list them in alpha order.
--yliu
Re: [dpdk-dev] [PATCH v10 1/3] lib: add Generic Receive Offload API framework
On Mon, Jul 03, 2017 at 05:56:20AM +, Hu, Jiayu wrote:
> > > +/**
> > > + * GRO table, which is used to merge packets. It keeps many reassembly
> > > + * tables of desired GRO types. Applications need to create GRO tables
> > > + * before using rte_gro_reassemble to perform GRO.
> > > + */
> > > +struct gro_tbl {
> > > + uint64_t desired_gro_types; /**< GRO types to perform */
> > > + /* max TTL measured in nanosecond */
> > > + uint64_t max_timeout_cycles;
> > > + /* max length of merged packet measured in byte */
> > > + uint32_t max_packet_size;
> > > + /* reassebly tables of desired GRO types */
> > > + void *tbls[RTE_GRO_TYPE_MAX_NUM];
> > > +};
> > > +
> > > +void *rte_gro_tbl_create(const
> > > + const struct rte_gro_param *param)
> >
> > The name of this API and the definition of struct gro_tbl involve some
> > confusion. A gro table contains gro tables? I suppose a better name is
> > needed, for example, struct gro_ctl.
>
> Actually, a GRO table includes N reassembly tables. But gro_tbl is not a good
> name. I will change the name. Thanks.
Haven't looked at the details yet, but, probably, gro_ctx (context) is a better
and more typical name?
--yliu
[dpdk-dev] next techboard meeting (5th, July)
Hi all, The next meeting of the techboard will happen on IRC #dpdk-board, at 3pm UTC, this Wednesday 5th of July. The agenda is updated here: https://annuel.framapad.org/p/r.0c3cc4d1e011214183872a98f6b5c7db For your convenience, I copied it here. 0. Release blocking issues? 1. Check action points from minutes of last meeting 2. Release backups http://dpdk.org/ml/archives/dev/2017-June/069229.html --yliu
Re: [dpdk-dev] [PATCH] vhost: fix MTU device feature check
On Thu, Jun 29, 2017 at 08:01:45AM +, Tan, Jianfeng wrote:
>
>
> > -Original Message-
> > From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
> > Sent: Thursday, June 29, 2017 3:58 PM
> > To: dev@dpdk.org; y...@fridaylinux.org
> > Cc: i.dyu...@samsung.com; Tan, Jianfeng; sta...@dpdk.org; Maxime
> > Coquelin
> > Subject: [PATCH] vhost: fix MTU device feature check
> >
> > The MTU feature support check has to be done against MTU
> > feature bit mask, and not bit position.
> >
> > Cc: sta...@dpdk.org
> > Fixes: 72e8543093df ("vhost: add API to get MTU value")
> >
> > Signed-off-by: Maxime Coquelin
>
> Reviewed-by: Jianfeng Tan
Applied to dpdk-next-virtio.
Thanks.
--yliu
Re: [dpdk-dev] [PATCH] vhost: fix checking of device features
On Wed, Jun 28, 2017 at 03:40:31PM +0300, Ivan Dyukov wrote:
> To compare enabled features in current device we must use bit
> mask instead of bit position.
>
> CC: sta...@dpdk.org
> Fixes: c843af3aa13e ("vhost: access header only")
>
> Signed-off-by: Ivan Dyukov
Applied to dpdk-next-virtio.
Thanks.
--yliu
Re: [dpdk-dev] [PATCH v2] doc: update virtio vring size and virtio_header size
On Fri, Jun 30, 2017 at 02:43:47PM +, Mcnamara, John wrote: > > > > -Original Message- > > From: Yang, Zhiyong > > Sent: Thursday, May 11, 2017 3:17 AM > > To: dev@dpdk.org > > Cc: Mcnamara, John ; yuanhan@linux.intel.com; > > maxime.coque...@redhat.com; Yang, Zhiyong > > Subject: [PATCH v2] doc: update virtio vring size and virtio_header size > > > > Add more explanations about vring size changes and different virtio_header > > size. > > > > Signed-off-by: Zhiyong Yang > > Acked-by: John McNamara Applied to dpdk-next-virtio. Thanks. --yliu
Re: [dpdk-dev] [PATCH 1/2] vhost: fix TCP csum not set
On Wed, Jun 07, 2017 at 09:25:38PM +0200, Maxime Coquelin wrote:
>
>
> On 06/07/2017 08:41 AM, Jianfeng Tan wrote:
> >As PKT_TX_TCP_SEG flag in mbuf->ol_flags implies PKT_TX_TCP_CKSUM,
> >applications, e.g., testpmd, don't set PKT_TX_TCP_CKSUM when TSO
> >is set.
> >
> >This leads to that packets get dropped in VM tcp stack layer because
> >of bad TCP csum.
> >
> >To fix this, we make sure TCP NEEDS_CSUM info is set into virtio net
> >header when PKT_TX_TCP_SEG is set, so that VM tcp stack will not
> >check the TCP csum.
> >
> >Fixes: 859b480d5afd ("vhost: add guest offload setting")
> >Cc: sta...@dpdk.org
> >
> >Cc: Yuanhan Liu
> >Cc: Maxime Coquelin
> >Cc: Jiayu Hu
> >Signed-off-by: Jianfeng Tan
>
> Reviewed-by: Maxime Coquelin
Series applied to dpdk-next-virtio.
Thanks!
--yliu
Re: [dpdk-dev] [PATCH] vhost: clean up per-socket mutex
On Wed, Jun 14, 2017 at 10:19:53AM +0200, Jens Freimann wrote:
> On Mon, Jun 12, 2017 at 02:29:04PM -0700, Daniel Verkamp wrote:
> > vsocket->conn_mutex was allocated with pthread_mutex_init() but never
> > freed with pthread_mutex_destroy(). This is a potential memory leak,
> > depending on how pthread_mutex_t is implemented.
> >
> > Signed-off-by: Daniel Verkamp
> > ---
> > lib/librte_vhost/socket.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> > index c7f99b0..9720773 100644
> > --- a/lib/librte_vhost/socket.c
> > +++ b/lib/librte_vhost/socket.c
> > @@ -636,6 +636,7 @@ rte_vhost_driver_register(const char *path, uint64_t
> > flags)
> > vsocket->reconnect = !(flags & RTE_VHOST_USER_NO_RECONNECT);
> > if (vsocket->reconnect && reconn_tid == 0) {
> > if (vhost_user_reconnect_init() < 0) {
> > + pthread_mutex_destroy(&vsocket->conn_mutex);
> > free(vsocket->path);
> > free(vsocket);
> > goto out;
> > @@ -646,6 +647,7 @@ rte_vhost_driver_register(const char *path, uint64_t
> > flags)
> > }
> > ret = create_unix_socket(vsocket);
> > if (ret < 0) {
> > + pthread_mutex_destroy(&vsocket->conn_mutex);
> > free(vsocket->path);
> > free(vsocket);
> > goto out;
> > @@ -724,6 +726,7 @@ rte_vhost_driver_unregister(const char *path)
> > }
> > pthread_mutex_unlock(&vsocket->conn_mutex);
> >
> > + pthread_mutex_destroy(&vsocket->conn_mutex);
>
> Seems like we never do it, but shouldn't we check the return value
> here?
I think so. There are more: pthread_mutex_init/lock, etc. We probably
should make another patch to fix it. For this patch, I'd like to apply
as it is.
So applied to dpdk-next-virtio.
Thanks.
--yliu
[dpdk-dev] [dpdk-announce] DPDK 17.05.1 released
ecked Lee Roberts (1): kni: fix build on RHEL 7.4 Markus Theil (1): net/igb: fix add/delete of flex filters Nélio Laranjeiro (1): net/mlx5: fix flow application order on stop/start Pascal Mazon (1): net/tap: fix some flow collision Qi Zhang (3): net/ixgbe: fix fdir mask not be reset net/i40e: exclude internal packet's byte count net/i40e: fix VF statistics Rahul Lakkireddy (2): net/cxgbe: fix port statistics net/cxgbe: fix rxq default params for ports under same PF Shahaf Shuler (1): net/mlx5: fix completion buffer size Shijith Thotton (1): net/liquidio: fix MTU calculation from port configuration Tiwei Bie (1): net/virtio: zero the whole memory zone Wei Dai (1): lpm: fix index of tbl8 Wei Zhao (1): net/igb: fix checksum valid flags Wenzhuo Lu (1): net/i40e/base: fix Tx error stats on VF Yongseok Koh (2): net/mlx5: fix exception handling net/mlx5: fix redundant free of Tx buffer Yuanhan Liu (2): vhost: fix crash on NUMA version: 17.05.1
[dpdk-dev] 17.05.1 patches review and test
Hi all, I'm doing an unexpected stable release, which is expected to be released in about two months. The reason I'm doing it is there is an import bug fix commit that is needed for OVS 2.8 release. The fix is a vhost-user commit comes from me. Well, unfortunately, I'm also the author of the original bug :/ So I'm doing an urgent release. That also means we will have another 17.05 release (17.05.2) shortly after v17.08 is out. And here is a list of patches targeted for this release. Please help review and test. The planned date for the final release is 30, June. Before that, please shout if anyone has objections. These patches are located at branch 17.05 of dpdk-stable repo: http://dpdk.org/browse/dpdk-stable/ Thanks. --yliu --- Ajit Khaparde (1): net/bnxt: fix reporting of link status Alejandro Lucero (1): vfio: fix array bounds check Andrew Rybchenko (1): net/sfc: add Tx queue flush failed flag for sanity Andy Moreton (2): net/sfc/base: fix error code usage in common code net/sfc/base: let caller know that queue is already flushed Beilei Xing (1): app/testpmd: fix creating E-Tag and NVGRE flow rules Chas Williams (3): net/af_packet: handle possible null pointer net/af_packet: fix packet bytes counting net/ring: fix adding MAC addresses Dariusz Stojaczyk (2): vhost: fix malloc size too small vhost: fix guest pages memory leak David Marchand (1): drivers/net: fix vfio kmod dependency Ferruh Yigit (3): kni: fix build with gcc 7.1 net/enic: fix build with gcc 7.1 net/mlx5: fix build with gcc 7.1 Harish Patil (1): net/qede: fix VXLAN tunnel Tx offload flag setting Jerin Jacob (1): examples/vhost: fix uninitialized descriptor indexes John Miller (4): net/ark: fix buffer not null terminated net/ark: fix return code not checked net/ark: fix null pointer dereference net/ark: fix return value of null not checked Lee Roberts (1): kni: fix build on RHEL 7.4 Markus Theil (1): net/igb: fix add/delete of flex filters Nélio Laranjeiro (1): net/mlx5: fix flow application order on stop/start Pascal Mazon (1): net/tap: fix some flow collision Qi Zhang (3): net/ixgbe: fix fdir mask not be reset net/i40e: exclude internal packet's byte count net/i40e: fix VF statistics Rahul Lakkireddy (2): net/cxgbe: fix port statistics net/cxgbe: fix rxq default params for ports under same PF Shahaf Shuler (1): net/mlx5: fix completion buffer size Shijith Thotton (1): net/liquidio: fix MTU calculation from port configuration Tiwei Bie (1): net/virtio: zero the whole memory zone Wei Dai (1): lpm: fix index of tbl8 Wei Zhao (1): net/igb: fix checksum valid flags Wenzhuo Lu (1): net/i40e/base: fix Tx error stats on VF Yongseok Koh (2): net/mlx5: fix exception handling net/mlx5: fix redundant free of Tx buffer Yuanhan Liu (1): vhost: fix crash on NUMA
Re: [dpdk-dev] [PATCH v2] test/test_mbuf: remove mempool global var
On Tue, Jun 20, 2017 at 09:35:07AM +0200, Thomas Monjalon wrote: > 20/06/2017 06:14, santosh: > > On Tuesday 20 June 2017 02:07 AM, Thomas Monjalon wrote: > > > > > 08/06/2017 16:28, Santosh Shukla: > > >> Let test_mbuf alloc and free mempool. > > >> > > >> Cc: sta...@dpdk.org > > >> Signed-off-by: Santosh Shukla > > > Why Cc stable? > > > Is it fixing something? > > > > > w/o this fix, application can't run more than once. > > Reason: Static allocation of resources and exiting w/o freeing so leak. > > > > Patch makes app resource handling dynamic and Now user could run > > test more than once and app exits gracefully. thats why Cc: stable a need > > (IHMO). > > Thanks. > > OK > So we need a Fixes: tag in order to be able to guess which > release it should be backported to. Also, I would suggest you to include above info (the real issue) in the commit log. --yliu
[dpdk-dev] [git pull] virtio changes for 17.08-rc1
Hi Thomas, Please consider pulling following virtio changes for 17.08-rc1 at git://dpdk.org/next/dpdk-next-virtiomaster Thanks. --yliu --- Daniel Verkamp (1): vhost: access VhostUsrMsg via packed struct Dariusz Stojaczyk (3): vhost: fix malloc size too small vhost: fix memory leak vhost: added error log for badly negotiated features Jens Freimann (1): vhost: check malloc return value when allocating guest pages Tiwei Bie (1): net/virtio: zero the whole memory zone Yuanhan Liu (1): vhost: fix crash on NUMA Zhihong Wang (1): vhost: support rx_queue_count --- drivers/net/vhost/rte_eth_vhost.c | 13 drivers/net/virtio/virtio_ethdev.c | 2 +- lib/librte_vhost/rte_vhost.h | 12 +++ lib/librte_vhost/rte_vhost_version.map | 7 + lib/librte_vhost/vhost.c | 28 - lib/librte_vhost/vhost_user.c | 81 +--- 6 files changed, 109 insertions(+), 34 deletions(-)
Re: [dpdk-dev] [PATCH] vhost: added error log in vhost_user_set_features
On Fri, Jun 16, 2017 at 04:32:05PM +0200, Dariusz Stojaczyk wrote: > Since vhost_user_set_features failure is not handled in any way, a > single error log has been added to at least to let the user know that > something has gone wrong. Indeed. And applied to dpdk-next-virtio. Thanks. --yliu
Re: [dpdk-dev] [PATCH] virtio: fix PCI config err handling
On Mon, Jun 05, 2017 at 04:44:33PM +0100, Brian Russell wrote:
> In virtio_read_caps, rte_pci_read_config returns the number of bytes
> read from PCI config or <0 on error.
> If less than the expected number of bytes are read then log the
> failure and return rather than carrying on with garbage.
>
> Signed-off-by: Brian Russell
> ---
> drivers/net/virtio/virtio_pci.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> index b7b3d61..90c098e 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c
> @@ -592,16 +592,18 @@ virtio_read_caps(struct rte_pci_device *dev, struct
> virtio_hw *hw)
> }
>
> ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST);
> - if (ret < 0) {
> - PMD_INIT_LOG(DEBUG, "failed to read pci capability list");
> + if (ret != 1) {
> + PMD_INIT_LOG(DEBUG,
> + "failed to read pci capability list, ret %d", ret);
> return -1;
> }
There are two things worth mentioning:
- rte_pci.h states nothing about the return value
- The BSD implementation actually returns 0 on success, while the Linux
implementation indeed returns the bytes read.
--yliu
Re: [dpdk-dev] [PATCH] net/virtio: zero the whole memory zone
On Mon, Jun 12, 2017 at 12:34:30PM +0800, Tiwei Bie wrote:
> Zero the whole memory zone instead of the first few bytes.
>
> Fixes: c1f86306a026 ("virtio: add new driver")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Tiwei Bie
> ---
> drivers/net/virtio/virtio_ethdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 45f9bca..88118f1 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -424,7 +424,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t
> vtpci_queue_idx)
> }
> }
>
> - memset(mz->addr, 0, sizeof(mz->len));
> + memset(mz->addr, 0, mz->len);
Nice catch! Applied to dpdk-next-virtio.
Thanks.
--yliu
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: optimize vxlan parsing function
On Thu, Jun 01, 2017 at 02:56:30PM +0800, Beilei Xing wrote: > This commit optimizes vxlan parsing function. How? --yliu > Signed-off-by: Beilei Xing > --- > drivers/net/i40e/i40e_flow.c | 176 > ++- > 1 file changed, 55 insertions(+), 121 deletions(-)
Re: [dpdk-dev] [dpdk-stable] [PATCH] vhost: fix crash on NUMA
On Fri, Jun 02, 2017 at 10:20:38AM +0200, Jens Freimann wrote:
> On Fri, Jun 02, 2017 at 08:14:46AM +0800, Yuanhan Liu wrote:
> > The queue allocation was changed, from allocating one queue-pair at a
> > time to one queue at a time. Most of the changes have been done, but
> > just with one being missed: the size of coping the old queue is still
>
> s/coping/copying/ ?
right, thanks.
>
> > based on queue-pair at numa_realloc(), which leads to overwritten issue.
> > As a result, crash may happen.
> >
> > Fix it by specifying the right copy size. Also, the net queue macros
> > are not used any more. Remove them.
> >
> > Fixes: ab4d7b9f1afc ("vhost: turn queue pair to vring")
> >
> > Cc: sta...@dpdk.org
> > Reported-by: Ciara Loftus
> > Signed-off-by: Yuanhan Liu
> > ---
> > lib/librte_vhost/vhost_user.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
>
> Reviewed-by: Jens Freimann
Applied to dpdk-next-virtio.
--yliu
[dpdk-dev] [PATCH] maintainers: update email address
Signed-off-by: Yuanhan Liu --- - I left Intel today. --- MAINTAINERS | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index afb4cab..f6095ef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -424,7 +424,7 @@ F: doc/guides/nics/vmxnet3.rst F: doc/guides/nics/features/vmxnet3.ini Vhost-user -M: Yuanhan Liu +M: Yuanhan Liu M: Maxime Coquelin T: git://dpdk.org/next/dpdk-next-virtio F: lib/librte_vhost/ @@ -434,14 +434,14 @@ F: doc/guides/sample_app_ug/vhost.rst Vhost PMD M: Tetsuya Mukawa -M: Yuanhan Liu +M: Yuanhan Liu M: Maxime Coquelin T: git://dpdk.org/next/dpdk-next-virtio F: drivers/net/vhost/ F: doc/guides/nics/features/vhost.ini Virtio PMD -M: Yuanhan Liu +M: Yuanhan Liu M: Maxime Coquelin T: git://dpdk.org/next/dpdk-next-virtio F: drivers/net/virtio/ -- 2.7.4
[dpdk-dev] [dpdk-announce] DPDK 17.02.1 released
acket type net/ixgbe: fix VF Rx mode for allmulticast disabled Wei Wang (1): net/bonding: fix updating slave link status Wei Zhao (5): net/ixgbe: fix generic filter return net/ixgbe: remove tpid check for flow director net/ixgbe: remove useless item type check net/ixgbe: fix type check for flow type net/ixgbe: fix ntuple filter for SCTP Wenfeng Liu (2): net/virtio-user: fix tapfds close net/virtio-user: fix overflow Wenzhuo Lu (10): net/i40e: fix TC bitmap of VEB net/ixgbe/base: fix build error net/ixgbe: fix Rx queue blocking issue net/ixgbe: fix all queues drop setting of DCB net/i40e: fix broadcast promiscuous mode setting net/i40e: fix VLAN filter net/ixgbe: fix TC bandwidth setting net/i40e: fix VLAN promisc setting cmdline: fix parsing net/ixgbe: fix default MAC setting Xiao Wang (3): net/fm10k: fix pointer cast net/fm10k: fix secondary process crash net/virtio: fix queue notify Yong Wang (3): doc: fix a typo in howto guide net/e1000/base: fix multicast setting in VF net/ena: fix return of hash control flushing Yongseok Koh (3): net/mlx5: fix reusing Rx/Tx queues net/mlx5: fix index handling for Tx ring net/mlx5: fix rollback when starting device Yuanhan Liu (5): vhost: fix multiple queue not enabled for old kernels vhost: fix max queues vhost: fix fd leaks for vhost-user server mode vhost: fix dequeue zero copy version: 17.02.1 Zhiyong Yang (1): examples/multi_process: fix timer update
Re: [dpdk-dev] [RFC] proposal of allowing personal/project repos on DPDK.org
On Thu, Jun 01, 2017 at 09:36:39AM +, Dumitrescu, Cristian wrote: > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Tiwei Bie > > Hello everyone, > > > > We'd like to make a proposal of making DPDK.org allow hosting > > some personal/project repos, which could be very useful when > > someone wants to try some experimental projects in DPDK. Many > > formal/mature opensource communities allow this. Such as: > > > > FreeBSD: > > > > The main branch is here: > > > > https://svnweb.freebsd.org/base/head/ > > > > And developers can try or start experimental projects here: > > > > https://svnweb.freebsd.org/base/user/ > > https://svnweb.freebsd.org/base/projects/ > > https://svnweb.freebsd.org/base/vendor/ > > ... > > > > We can also find similar things on kernel.org and freedesktop.org: > > > > https://git.kernel.org/ > > https://cgit.freedesktop.org/ > > > > But currently on DPDK.org, for the repos of DPDK, besides the main > > repo, there are just one repo for stable release, few old repos > > which are obsoleted, and some "-next" repos which are mainly used > > as the preparation of pull requests for different subsystems of DPDK. > > Some “-next” repos may be developed individually for a short time > > when they are created, but will always be merged to the main repo > > after few releases. We think they are too formal/limited to try > > new ideas. > > > > What we want to proposal is to make DPDK.org allow hosting some > > DPDK based repos which may be very experimental, which even not > > be planned to be merged back to the main repo directly, and may > > be deleted directly if it proves that no one really cares about > > it. Just like what other opensource communities did, allow some > > core developers/vendors create their own repos and try ideas on > > DPDK.org without too many restrictions. We think it can provide > > people a very convenient way to try ideas in DPDK community and > > eventually help DPDK grow. > > > > Allowing it won't have any negative impact on the existing repos > > of DPDK, instead, it can help to keep them tidy and clean when > > someone wants/needs to try some very experimental and big ideas > > publicly in DPDK as he/she can start it in an experimental repo. > > An experimental repo can be merged back to the main repo when it > > proves to be mature and useful, or could just be deleted when no > > one really cares about it any more. > > > > Please share your thoughts on this. Many thanks! :-) > > > > Best regards, > > Tiwei Bie > > +1 > > Fd.io has the sandbox area, why not having something similar on dpdk.org? +1. Also, cc'ed Stephen, who is going to chair next tech-board meeting in next week. Let's have a brief discuss there? --yliu
[dpdk-dev] [PATCH] vhost: fix crash on NUMA
The queue allocation was changed, from allocating one queue-pair at a
time to one queue at a time. Most of the changes have been done, but
just with one being missed: the size of coping the old queue is still
based on queue-pair at numa_realloc(), which leads to overwritten issue.
As a result, crash may happen.
Fix it by specifying the right copy size. Also, the net queue macros
are not used any more. Remove them.
Fixes: ab4d7b9f1afc ("vhost: turn queue pair to vring")
Cc: sta...@dpdk.org
Reported-by: Ciara Loftus
Signed-off-by: Yuanhan Liu
---
lib/librte_vhost/vhost_user.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 5c8058b..e486b78 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -238,8 +238,6 @@ numa_realloc(struct virtio_net *dev, int index)
struct vhost_virtqueue *old_vq, *vq;
int ret;
- enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
-
old_dev = dev;
vq = old_vq = dev->virtqueue[index];
@@ -261,7 +259,7 @@ numa_realloc(struct virtio_net *dev, int index)
if (!vq)
return dev;
- memcpy(vq, old_vq, sizeof(*vq) * VIRTIO_QNUM);
+ memcpy(vq, old_vq, sizeof(*vq));
rte_free(old_vq);
}
--
2.8.1
[dpdk-dev] [dpdk-announce] DPDK 16.11.2 (LTS) released
): net/mlx4: update link status upon probing with LSC net/mlx4: fix returned values upon failed probing net/mlx5: fix returned values upon failed probing Gage Eads (1): crypto/qat: fix dequeue statistics Harish Patil (3): net/qede: fix missing UDP protocol in RSS offload types net/qede: fix default MAC address handling net/qede: fix fastpath rings reset phase Henry Cai (2): net/cxgbe: fix possible null pointer dereference net/i40e: fix allocation check Huanle Han (1): net/virtio: fix crash when closing twice Ido Barnea (1): net/ixgbevf: set xstats id values Ilya Maximets (2): vhost: change log levels in client mode net/bonding: allow configuring jumbo frames without slaves Jeff Guo (4): net/i40e: fix hash input set on X722 app: enable HW CRC strip by default test: enable HW CRC strip by default examples: enable HW CRC strip by default Jerin Jacob (5): eal/linux: fix build with glibc 2.25 net/i40e: fix incorrect packet index reference net/thunderx: fix 32-bit build net/thunderx: fix build on FreeBSD net/thunderx: fix deadlock in Rx path Jia Yu (1): net/ixgbe: fix setting MTU on stopped device Jianfeng Tan (4): vfio: fix secondary process start net/virtio-user: fix address on 32-bit system net/virtio: fix MSI-X for modern devices net/virtio: fix link status always down Jiayu Hu (1): app/testpmd: fix exit without freeing resources Jingjing Wu (5): net/ixgbe: fix multi-queue mode check in SRIOV mode app/testpmd: fix init config for multi-queue mode app/testpmd: fix TC mapping in DCB init config net/i40e/base: fix potential out of bound array access examples/l3fwd-power: fix handling no Rx queue Johan Samuelsson (1): net/pcap: fix using mbuf after freeing it Kevin Traynor (1): vhost: fix false sharing Marcin Wilk (1): net/thunderx: fix stats access out of bounds Matt Peters (2): net/virtio: disable LSC interrupt if MSIX not enabled net/i40e: fix mbuf alloc failed counter Michal Krawczyk (3): net/ena: cleanup if refilling of Rx descriptors fails net/ena: fix Rx descriptors allocation net/ena: fix delayed cleanup of Rx descriptors Michał Mirosław (1): net: fix stripped VLAN flag for offload emulation Nelio Laranjeiro (1): net/mlx5: fix an uninitialized variable Nikhil Rao (1): vfio: fix disabling INTx Nélio Laranjeiro (2): net/mlx5: fix supported packets types net/mlx5: fix Tx when first segment size is too short Olivier Matz (4): mk: fix shell errors when building with clang mk: fix lib filtering when linking app app/testpmd: fix crash at mbuf pool creation app/testpmd: fix number of mbufs in pool Pablo de Lara (5): examples/l2fwd-crypto: fix AEAD tests when AAD is zero examples/l2fwd-crypto: fix padding calculation crypto/openssl: fix AES-GCM capability examples/l2fwd-crypto: fix packets array index examples/l3fwd-power: fix Rx descriptor size Pascal Mazon (1): mk: fix quoting for ARM mtune argument Qi Zhang (7): net/i40e: fix compile error net/i40e: fix VF link speed net/i40e: add missing 25G link speed net/ixgbe: fix memory overflow in 32-bit SSE Rx net/i40e: fix memory overflow in 32-bit SSE Rx net/fm10k: fix memory overflow in 32-bit SSE Rx net/i40e: fix VF link status update Qiming Yang (2): net/igb: fix VF MAC address setting net/igb: fix VF MAC address setting Rasesh Mody (3): net/qede/base: fix find zero bit macro net/qede: fix FW version string for VF doc: explain zlib dependency for bnx2x Shahaf Shuler (1): net/mlx5: fix VLAN stripping indication Shreyansh Jain (1): test/mempool: free mempool on exit Wei Dai (2): examples/ip_fragmentation: fix check of packet type net/ixgbe: fix VF Rx mode for allmulticast disabled Wei Wang (1): net/bonding: fix updating slave link status Wenfeng Liu (1): net/virtio-user: fix overflow Wenzhuo Lu (5): net/i40e: fix TC bitmap of VEB net/ixgbe/base: fix build error net/ixgbe: fix Rx queue blocking issue net/ixgbe: fix all queues drop setting of DCB net/ixgbe: fix TC bandwidth setting Xiao Wang (2): net/fm10k: fix pointer cast net/virtio: fix queue notify Yong Wang (2): net/e1000/base: fix multicast setting in VF net/ena: fix return of hash control flushing Yongseok Koh (1): net/mlx5: fix reusing Rx/Tx queues Yuanhan Liu (7): vhost: fix multiple queue not enabled for old kernels vhost: fix max queues vhost: fix fd leaks for vhost-user server mode vhost: fix use after free vhost: fix dequeue zero copy net/virtio: fix link status always being up version: 16.11.2 Zhiyong Yang (1): examples
[dpdk-dev] stable release 17.02.1 patches review and test
): vhost: try to shrink pfdset when fdset_add fails Michal Krawczyk (3): net/ena: fix Rx descriptors allocation net/ena: fix delayed cleanup of Rx descriptors net/ena: cleanup if refilling of Rx descriptors fails Michał Mirosław (1): net: fix stripped VLAN flag for offload emulation Nelio Laranjeiro (1): net/mlx5: fix an uninitialized variable Nikhil Rao (1): vfio: fix disabling INTx Nélio Laranjeiro (6): net/mlx5: fix startup when flow cannot be applied net/mlx5: fix supported packets types net/mlx5: fix flow mark action handling net/mlx5: fix drop queue creation error net/mlx5: fix resources free in the right function net/mlx5: fix Tx when first segment size is too short Olivier Matz (3): mk: fix shell errors when building with clang mk: fix lib filtering when linking app app/testpmd: fix crash at mbuf pool creation Pablo de Lara (7): app/crypto-perf: fix AES CBC 128 test vectors app/crypto-perf: fix AEAD tests when AAD is zero examples/l2fwd-crypto: fix AEAD tests when AAD is zero examples/l2fwd-crypto: fix padding calculation crypto/openssl: fix AES-GCM capability examples/l2fwd-crypto: fix packets array index examples/l3fwd-power: fix Rx descriptor size Pascal Mazon (1): mk: fix quoting for ARM mtune argument Qi Zhang (8): net/i40e: fix compile error net/i40e: fix VF link speed net/i40e: add missing 25G link speed net/i40e: fix VF link status update net/ixgbe: fix memory overflow in 32-bit SSE Rx net/i40e: fix memory overflow in 32-bit SSE Rx net/fm10k: fix memory overflow in 32-bit SSE Rx net/ixgbe: fix LSC interrupt Qiming Yang (2): net/igb: fix VF MAC address setting net/igb: fix VF MAC address setting Rami Rosen (1): net/i40e: fix a typo in flow Rasesh Mody (2): net/qede: fix FW version string for VF doc: explain zlib dependency for bnx2x Shahaf Shuler (1): net/mlx5: fix VLAN stripping indication Shreyansh Jain (2): mempool: fix crash when handler not found test/mempool: free mempool on exit Tiwei Bie (2): eal/bsd: fix ioport write operation eal/bsd: fix read on PCI configuration space Wei Dai (2): examples/ip_fragmentation: fix check of packet type net/ixgbe: fix VF Rx mode for allmulticast disabled Wei Wang (1): net/bonding: fix updating slave link status Wei Zhao (5): net/ixgbe: fix generic filter return net/ixgbe: remove tpid check for flow director net/ixgbe: remove useless item type check net/ixgbe: fix type check for flow type net/ixgbe: fix ntuple filter for SCTP Wenfeng Liu (2): net/virtio-user: fix tapfds close net/virtio-user: fix overflow Wenzhuo Lu (10): net/i40e: fix TC bitmap of VEB net/ixgbe/base: fix build error net/ixgbe: fix Rx queue blocking issue net/ixgbe: fix all queues drop setting of DCB net/i40e: fix broadcast promiscuous mode setting net/i40e: fix VLAN filter net/ixgbe: fix TC bandwidth setting net/i40e: fix VLAN promisc setting cmdline: fix parsing net/ixgbe: fix default MAC setting Xiao Wang (3): net/fm10k: fix pointer cast net/fm10k: fix secondary process crash net/virtio: fix queue notify Yong Wang (3): doc: fix a typo in howto guide net/e1000/base: fix multicast setting in VF net/ena: fix return of hash control flushing Yongseok Koh (3): net/mlx5: fix reusing Rx/Tx queues net/mlx5: fix index handling for Tx ring net/mlx5: fix rollback when starting device Yuanhan Liu (4): vhost: fix multiple queue not enabled for old kernels vhost: fix max queues vhost: fix fd leaks for vhost-user server mode vhost: fix dequeue zero copy Zhiyong Yang (1): examples/multi_process: fix timer update
Re: [dpdk-dev] [PATCH v4 0/3] vhost: undefined behavior fixes
On Fri, May 26, 2017 at 01:59:12PM +0200, Dariusz Stojaczyk wrote: > Fixes for memory-related undefined behavior issues in rte_vhost. > > Daniel Verkamp (1): > vhost: access VhostUsrMsg via packed struct > > Dariusz Stojaczyk (2): > vhost: fix malloc in rte_vhost_get_mem_table() > vhost: free guest_pages in vhost_backend_cleanup() Applied to dpdk-next-virtio. Thanks! --yliu > > lib/librte_vhost/vhost.c | 2 +- > lib/librte_vhost/vhost_user.c | 64 > --- > 2 files changed, 37 insertions(+), 29 deletions(-) > > -- > 2.7.4
Re: [dpdk-dev] [PATCH v3 3/3] vhost: access VhostUsrMsg via packed struct
On Wed, May 24, 2017 at 01:12:07PM +, Stojaczyk, DariuszX wrote: > > This is for fixing compile warnings with new clang 4.0? > > > > http://dpdk.org/ml/archives/dev/2017-April/064089.html > > > > If so, please show the exact warning in the commit log. > > > > Everything compiles, but is undefined behavior. Would you be a bit more informative about the "undefined behavior"? This patch set (including this one) looks good to me. I just want the commit log be more informative. Something like "Fixes unaligned access to fields" is a bit too vague. Thanks. --yliu > Accessing packed struct's fields through pointers would have to be done as > following: > e.g vhost_user_set_vring_addr(struct virtio_net *dev, struct vhost_vring_addr > *addr __attribute__((aligned(1))) > Since the code above is unacceptable, this patch makes all functions take > pointer to the parent struct (VhostUserMsg) > > > > > > > Signed-off-by: Daniel Verkamp > > > Signed-off-by: Dariusz Stojaczyk > > > --- > > > Fixed checkpatch warnings > > > > It's likely it will be easily missed while review. We normally do that: > > > > --- > > > > v3: fix checkpatch warnings > > > > v2: remove gerrit id > > > > --yliu > > Thanks, I'll stick with it from now on
Re: [dpdk-dev] [PATCH v3] vhost: support rx_queue_count
On Fri, May 26, 2017 at 01:18:02PM -0400, Zhihong Wang wrote: > This patch implements the ops rx_queue_count for vhost PMD by adding > a helper function rte_vhost_rx_queue_count in vhost lib. > > The ops rx_queue_count gets vhost RX queue avail count and helps to > understand the queue fill level. > > Signed-off-by: Zhihong Wang > Acked-by: Ciara Loftus Applied to dpdk-next-virtio. Thanks. --yliu
Re: [dpdk-dev] [dpdk-stable] 16.11.2 (LTS) patches review and test
On Wed, May 24, 2017 at 12:10:57PM +0100, Kevin Traynor wrote: > On 05/22/2017 06:20 AM, Yuanhan Liu wrote: > > Hi all, > > > > Here is a list of patches targeted for 16.11.2 (LTS) release. Please > > help review and test. The planned date for the final release is 31th, > > May. Before that, please shout if anyone has objections with these > > patches being applied. > > > > Hi Yuanhan, I did a few basic PVP tests with OVS-DPDK using dpdk-stable > and seems to be working fine. Kevin, Great. Thanks for testing! --yliu
Re: [dpdk-dev] [PATCH] vhost: support rx_queue_count
On Wed, May 24, 2017 at 10:36:01AM +0200, Jens Freimann wrote: > On Wed, May 24, 2017 at 04:14:19PM +0800, Yuanhan Liu wrote: > > On Tue, May 23, 2017 at 12:51:56PM +, Wang, Zhihong wrote: > > > > > > > > > > -Original Message- > > > > From: Jens Freimann [mailto:jfrei...@redhat.com] > > > > Sent: Tuesday, May 23, 2017 7:54 PM > > > > To: Wang, Zhihong > > > > Cc: dev@dpdk.org; yuanhan@linux.intel.com > > > > Subject: Re: [dpdk-dev] [PATCH] vhost: support rx_queue_count > > > > > > > > On Mon, May 22, 2017 at 04:01:08PM -0400, Zhihong Wang wrote: > > > > > This patch implements the ops rx_queue_count for vhost PMD by adding > > > > > a helper function rte_vhost_rx_queue_count in vhost lib. > > > > > > > > > > The ops ops rx_queue_count gets vhost RX queue avail count and helps > > > > > > > > s/ops ops/ops/ ? > > > > > > Thanks a lot! > > > > Seems you overlooked other comments, such as: > > > > > > > + vq = dev->virtqueue[qid]; > > > > > > > > check for vq == NULL? > > > > > > > > regards, > > > > Jens > > > > Besides, Jens, I'm not a big fan of "dev == NULL" over "!dev". I accept > > both :) > > Personally I'm with you on this, but it says different in > http://dpdk.org/doc/guides/contributing/coding_style.html > Anyway, up to you :) Yes, I know that. To make every body happy, I would suggset you to follow the coding style. But if you have already choosen another way, I won't bother to ask you do the change. Unless, it becomes a must in future. --yliu
Re: [dpdk-dev] [PATCH] vhost: support rx_queue_count
On Tue, May 23, 2017 at 12:51:56PM +, Wang, Zhihong wrote: > > > > -Original Message- > > From: Jens Freimann [mailto:jfrei...@redhat.com] > > Sent: Tuesday, May 23, 2017 7:54 PM > > To: Wang, Zhihong > > Cc: dev@dpdk.org; yuanhan@linux.intel.com > > Subject: Re: [dpdk-dev] [PATCH] vhost: support rx_queue_count > > > > On Mon, May 22, 2017 at 04:01:08PM -0400, Zhihong Wang wrote: > > > This patch implements the ops rx_queue_count for vhost PMD by adding > > > a helper function rte_vhost_rx_queue_count in vhost lib. > > > > > > The ops ops rx_queue_count gets vhost RX queue avail count and helps > > > > s/ops ops/ops/ ? > > Thanks a lot! Seems you overlooked other comments, such as: > > > + vq = dev->virtqueue[qid]; > > > > check for vq == NULL? > > > > regards, > > Jens Besides, Jens, I'm not a big fan of "dev == NULL" over "!dev". I accept both :) --yliu
Re: [dpdk-dev] Next Stable Release?
On Tue, May 23, 2017 at 09:05:38AM +, Hemant Agrawal wrote: > Which is the next DPDK LTS release? There is no answer yet. It could be v18.11. There was also a request for proposing v17.11 to be the next LTS. > Is 17.05 a DPDK LTS release? Nope, it's not in the plan. > Do we need to send a copy to sta...@dpdk.org for any fixes on 17.05? What you just need to do is to put following tag in the commit log. Cc: sta...@dpdk.org --yliu
Re: [dpdk-dev] [RFC PATCH 00/11] net/virtio: packed ring layout
On Thu, May 18, 2017 at 10:24:07PM +0800, Yuanhan Liu wrote: > On Wed, May 17, 2017 at 01:30:21PM +0200, Jens Freimann wrote: > > Hi Yuanhan, > > > > On Mon, May 08, 2017 at 01:02:57PM +0800, Yuanhan Liu wrote: > > > On Fri, May 05, 2017 at 09:57:11AM -0400, Jens Freimann wrote: > > > > I'm implementing the receive path now to eventually get some benchmark > > > > results for that as well (Patches not included yet) > > > > > > Good to know. Any progress? I'm asking because that was also my plan for > > > this week: make Rx work. I haven't started it though. > > > > just curious if you already had a chance to work on this? > > Yes, I made it work this Tue. I will try to share it tomorrow. I was on > vacation though. I have pushed it to the "for-testing" branch at dpdk-next-virtio tree. --yliu
Re: [dpdk-dev] [PATCH v3 3/3] vhost: access VhostUsrMsg via packed struct
On Thu, May 11, 2017 at 04:33:12PM +0200, Dariusz Stojaczyk wrote: > From: Daniel Verkamp > > Fixes unaligned access to fields. This is for fixing compile warnings with new clang 4.0? http://dpdk.org/ml/archives/dev/2017-April/064089.html If so, please show the exact warning in the commit log. > > Signed-off-by: Daniel Verkamp > Signed-off-by: Dariusz Stojaczyk > --- > Fixed checkpatch warnings It's likely it will be easily missed while review. We normally do that: --- v3: fix checkpatch warnings v2: remove gerrit id --yliu
Re: [dpdk-dev] [PATCH v1] doc: change doc line length limit in contributors guide
On Tue, May 16, 2017 at 02:20:58PM +, Mcnamara, John wrote: > > > > -Original Message- > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > Sent: Friday, May 12, 2017 10:24 AM > > ,,, > > > > > The current DPDK "single sentence per line plus wrap at ~120 characters" > > > guideline is unusual, not supported by editors and, with rare > > > exceptions, not followed by anyone. > > > > > > As such I think the guidelines should reflect how people actually > > > write docs and submit patches, which is wrapping at 80 characters. > > > > I am OK with 80 characters. > > However, I think we should keep trying to explain that it is better to > > wrap at the end of a sentence. > > > > Example: > > This long sentence with a lot of words which does not mean anything will > > wrap at 80 characters and continue on the second line. Then a new sentence > > starts and ends on the third line. > > > > It would be better like that: > > This long sentence with a lot of words which does not mean anything will > > wrap at 80 characters and continue on the second line. > > Then a new sentence starts and ends on the third line. > > This is essentially the same problem as the current guideline: that this > is an artificial way of writing text, it isn't supported by editors, > and is unlikely to be followed in practice. > > The first example is the way people write text and the way text is submitted > in patches so the guidelines should reflect this. +1 for the first one :) And, Reviewed-by: Yuanhan Liu --yliu
Re: [dpdk-dev] [PATCH] vhost: check malloc return value when allocating guest pages
On Thu, May 18, 2017 at 10:14:26AM +0200, Maxime Coquelin wrote:
>
>
> On 05/11/2017 05:25 PM, Jens Freimann wrote:
> >When we try to allocate guest pages we need to check the return value of
> >malloc(). Print an error message and return when it fails.
> >
> >Signed-off-by: Jens Freimann
> >---
> > lib/librte_vhost/vhost_user.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> >diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> >index 5c8058b..437e41f 100644
> >--- a/lib/librte_vhost/vhost_user.c
> >+++ b/lib/librte_vhost/vhost_user.c
> >@@ -515,6 +515,13 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct
> >VhostUserMsg *pmsg)
> > dev->max_guest_pages = 8;
> > dev->guest_pages = malloc(dev->max_guest_pages *
> > sizeof(struct guest_page));
> >+if (dev->guest_pages == NULL) {
> >+RTE_LOG(ERR, VHOST_CONFIG,
> >+"(%d) failed to allocate memory "
> >+"for dev->guest_pages\n",
> >+dev->vid);
> >+return -1;
> >+}
> > }
> > dev->mem = rte_zmalloc("vhost-mem-table", sizeof(struct
> > rte_vhost_memory) +
> >
>
> Reviewed-by: Maxime Coquelin
Applied to dpdk-next-virtio.
Thanks.
--yliu
[dpdk-dev] 16.11.2 (LTS) patches review and test
examples/l3fwd-power: fix Rx descriptor size Pascal Mazon (1): mk: fix quoting for ARM mtune argument Qi Zhang (7): net/i40e: fix compile error net/i40e: fix VF link speed net/i40e: add missing 25G link speed net/ixgbe: fix memory overflow in 32-bit SSE Rx net/i40e: fix memory overflow in 32-bit SSE Rx net/fm10k: fix memory overflow in 32-bit SSE Rx net/i40e: fix VF link status update Qiming Yang (2): net/igb: fix VF MAC address setting net/igb: fix VF MAC address setting Rasesh Mody (3): net/qede/base: fix find zero bit macro net/qede: fix FW version string for VF doc: explain zlib dependency for bnx2x Shahaf Shuler (1): net/mlx5: fix VLAN stripping indication Shreyansh Jain (1): test/mempool: free mempool on exit Wei Dai (2): examples/ip_fragmentation: fix check of packet type net/ixgbe: fix VF Rx mode for allmulticast disabled Wei Wang (1): net/bonding: fix updating slave link status Wenfeng Liu (1): net/virtio-user: fix overflow Wenzhuo Lu (5): net/i40e: fix TC bitmap of VEB net/ixgbe/base: fix build error net/ixgbe: fix Rx queue blocking issue net/ixgbe: fix all queues drop setting of DCB net/ixgbe: fix TC bandwidth setting Xiao Wang (2): net/fm10k: fix pointer cast net/virtio: fix queue notify Yong Wang (2): net/e1000/base: fix multicast setting in VF net/ena: fix return of hash control flushing Yongseok Koh (1): net/mlx5: fix reusing Rx/Tx queues Yuanhan Liu (6): vhost: fix multiple queue not enabled for old kernels vhost: fix max queues vhost: fix fd leaks for vhost-user server mode vhost: fix use after free vhost: fix dequeue zero copy net/virtio: fix link status always being up Zhiyong Yang (1): examples/multi_process: fix timer update
Re: [dpdk-dev] [RFC PATCH 00/11] net/virtio: packed ring layout
On Wed, May 17, 2017 at 01:30:21PM +0200, Jens Freimann wrote: > Hi Yuanhan, > > On Mon, May 08, 2017 at 01:02:57PM +0800, Yuanhan Liu wrote: > > On Fri, May 05, 2017 at 09:57:11AM -0400, Jens Freimann wrote: > > > I'm implementing the receive path now to eventually get some benchmark > > > results for that as well (Patches not included yet) > > > > Good to know. Any progress? I'm asking because that was also my plan for > > this week: make Rx work. I haven't started it though. > > just curious if you already had a chance to work on this? Yes, I made it work this Tue. I will try to share it tomorrow. I was on vacation though. --yliu
Re: [dpdk-dev] [PATCH] driver/net: remove unnecessary macro for unused variables
On Mon, May 15, 2017 at 10:26:00AM +0100, Ferruh Yigit wrote: > On 5/15/2017 10:19 AM, Yuanhan Liu wrote: > > On Mon, May 15, 2017 at 10:17:43AM +0100, Ferruh Yigit wrote: > >> On 5/12/2017 11:33 AM, Ferruh Yigit wrote: > >>> remove __rte_unused instances that are not required. > >>> > >> > >> Hi Yuanhan, > >> > >> Does this kind of refactoring patches, specially the ones covering > >> multiple drivers, cause trouble (more conflicts) to you while getting > >> patches for stable trees? > > > > Yes, it's likely. > > > >> If so, I can postpone them through the end of integration deadline. > > > > I don't think it's necessary though. If a conflict happens, I will ask > > the author to do backport :) > > OK, thanks for clarifying ... > > So, result is, refactoring patches won't be pushed through end of the > release. Yes, I see no strong need for that. --yliu
Re: [dpdk-dev] [PATCH] driver/net: remove unnecessary macro for unused variables
On Mon, May 15, 2017 at 10:17:43AM +0100, Ferruh Yigit wrote: > On 5/12/2017 11:33 AM, Ferruh Yigit wrote: > > remove __rte_unused instances that are not required. > > > > Hi Yuanhan, > > Does this kind of refactoring patches, specially the ones covering > multiple drivers, cause trouble (more conflicts) to you while getting > patches for stable trees? Yes, it's likely. > If so, I can postpone them through the end of integration deadline. I don't think it's necessary though. If a conflict happens, I will ask the author to do backport :) --yliu > But postponing them most probably will mean rebasing these patches > later, so if this is not causing any problem for you, please let me know > so that I can get them earlier. > > Thanks, > ferruh
Re: [dpdk-dev] [PATCH] driver/net: remove unnecessary macro for unused variables
On Fri, May 12, 2017 at 11:33:03AM +0100, Ferruh Yigit wrote: > remove __rte_unused instances that are not required. I'm wondering this is done by some scripts? --yliu
Re: [dpdk-dev] [RFC PATCH 00/11] net/virtio: packed ring layout
On Fri, May 05, 2017 at 09:57:11AM -0400, Jens Freimann wrote: > Hi Yuanhan, > > I rebased your patches on next-virtio/for-testing to current master, > made sure every patch compiles and still works. Thanks for that. > I'm implementing the receive path now to eventually get some benchmark > results for that as well (Patches not included yet) Good to know. Any progress? I'm asking because that was also my plan for this week: make Rx work. I haven't started it though. --yliu > Any comments to the existing patches are welcome, I will change them > accordingly. > > regards, > Jens > > > > Yuanhan Liu (11): > net/virtio: vring init for 1.1 > net/virtio: implement 1.1 guest Tx > net/virtio-user: add option to enable 1.1 > vhost: enable 1.1 for testing > vhost: set desc addr for 1.1 > vhost: implement virtio 1.1 dequeue path > vhost: mark desc being used > xxx: batch the desc_hw update? > xxx: virtio: remove overheads > vhost: prefetch desc > add virtio 1.1 test guide > > README-virtio-1.1| 50 ++ > drivers/net/virtio/Makefile | 1 + > drivers/net/virtio/virtio-1.1.h | 19 +++ > drivers/net/virtio/virtio_ethdev.c | 44 +++-- > drivers/net/virtio/virtio_ethdev.h | 3 + > drivers/net/virtio/virtio_pci.h | 7 + > drivers/net/virtio/virtio_ring.h | 15 +- > drivers/net/virtio/virtio_rxtx.c | 191 ++--- > drivers/net/virtio/virtio_rxtx_1.1.c | 161 ++ > drivers/net/virtio/virtio_user/virtio_user_dev.c | 9 +- > drivers/net/virtio/virtio_user/virtio_user_dev.h | 3 +- > drivers/net/virtio/virtio_user_ethdev.c | 14 +- > drivers/net/virtio/virtqueue.h | 10 ++ > lib/librte_vhost/vhost.h | 5 + > lib/librte_vhost/vhost_user.c| 1 + > lib/librte_vhost/virtio-1.1.h| 23 +++ > lib/librte_vhost/virtio_net.c| 208 > +++ > 17 files changed, 567 insertions(+), 197 deletions(-) > create mode 100644 README-virtio-1.1 > create mode 100644 drivers/net/virtio/virtio-1.1.h > create mode 100644 drivers/net/virtio/virtio_rxtx_1.1.c > create mode 100644 lib/librte_vhost/virtio-1.1.h > > -- > 1.8.3.1
Re: [dpdk-dev] [PATCH v7 1/3] ethdev: fix adding invalid MAC addr
On Fri, May 05, 2017 at 04:21:53PM +0200, Thomas Monjalon wrote: > 05/05/2017 02:40, Wei Dai: > > Some customers find adding MAC addr to VF sometimes can fail, > > but it is still stored in dev->data->mac_addrs[ ]. So this > > can lead to some errors that assumes the non-zero entry in > > dev->data->mac_addrs[ ] is valid. > > Following acknowledgements are from specific NIC PMD > > maintainer for their managing part. > > > > This patch changes the ethdev API, it should not be > > backported to a stable/LTS release so far. > > It changes only the internal function pointer used by drivers, > not the API. Oh, right. I was thinking the eth API rte_eth_dev_mac_addr also has to be changed. It turned out that it's already been designed with returning "int". Sorry for the noise. --yliu
Re: [dpdk-dev] [dpdk-stable] [PATCH 11/11] net/qede: fix to limit CFLAGS to base files
On Thu, May 04, 2017 at 12:14:30AM +, Mody, Rasesh wrote:
>
>
> > From: Yuanhan Liu [mailto:yuanhan@linux.intel.com]
> > Sent: Monday, May 01, 2017 11:15 PM
> >
> > On Tue, Apr 25, 2017 at 12:28:46AM -0700, Rasesh Mody wrote:
> > > From: Rasesh Mody
> > >
> > > Changes included in this fix
> > > - limit CFLAGS to base files
> > > - fix to remove/mark unused members
> > > - add checks for debug config option
> > > - make qede_set_mtu() and qede_udp_dst_port_del() static and others
> > >non-static as appropriate
> > > - move local APIs qede_vlan_offload_set() and
> > > qede_rx_cqe_to_pkt_type()
> > > - initialize variables as required
> >
> > When there are so many items in one single patch, it basically means it's
> > done wrongly. Generally, we should make one patch for each item.
> >
> > > Fixes: ec94dbc57362 ("qede: add base driver")
> > > Cc: sta...@dpdk.org
> >
> > It's also not a good idea to put "Cc: stable" tag in a huge fix patch.
> > It's very likely it won't apply cleanly to a stable/LTS release. For
> > instance, I
> > failed to apply it to 16.11.2 (LTS).
> >
> > >
> > > Signed-off-by: Rasesh Mody
> > > ---
> > > drivers/net/qede/Makefile | 32 -
> > > drivers/net/qede/base/ecore.h |4 +-
> > > drivers/net/qede/base/ecore_int_api.h |4 +-
> > > drivers/net/qede/qede_ethdev.c| 120 ++
> > ---
> > > drivers/net/qede/qede_ethdev.h| 32 -
> > > drivers/net/qede/qede_fdir.c | 13 +---
> > > drivers/net/qede/qede_if.h|4 ++
> > > drivers/net/qede/qede_main.c |8 +--
> > > drivers/net/qede/qede_rxtx.c | 118
> > > +-
> > --
> > > 9 files changed, 171 insertions(+), 164 deletions(-)
> >
> > It's also a clear sign of bad patch: too many changes for a single bug fix
> > patch.
> >
> > Most of them look like minor fixes to me. So my question is are there any
> > important items really should be picked for stable and LTS release?
> > More specifically, do they really fix any (fatal) issues? If no, I will
> > drop it. If
> > yes, please send a (or some) patch with the real fixes backported only to
> > stable ML, so that I could pick them.
>
> The patch is a Makefile change to restrict the CFLAG only to the base files.
> Once Makefile was changed it exposed few issues with PMD.
In such case, you could make patches to fix those issues first, one
patch for one issue, and then put the CFLAG change to the last.
> Hence, we thought of putting all the changes in single patch since they were
> relevant changes.
>
> As you stated most of them are minor fixes. We'll evaluate the patch if
> anything specifically need to go into the stable release and get back.
Thanks. The answer seems to be "NO" to me though.
--yliu
Re: [dpdk-dev] [PATCH v6 1/3] ethdev: fix adding invalid MAC addr
On Tue, May 02, 2017 at 08:44:23PM +0800, Wei Dai wrote:
> Some customers find adding MAC addr to VF sometimes can fail,
> but it is still stored in dev->data->mac_addrs[ ]. So this
> can lead to some errors that assumes the non-zero entry in
> dev->data->mac_addrs[ ] is valid.
> Following acknowledgements are from specific NIC PMD
> maintainer for their managing part.
>
> Fixes: af75078fece3 ("first public release")
> Cc: sta...@dpdk.org
Just a note, this patch changes API. It should not be backported to a
stable/LTS release, even though it fixes something.
--yliu
Re: [dpdk-dev] [dpdk-stable] [PATCH 11/11] net/qede: fix to limit CFLAGS to base files
On Tue, Apr 25, 2017 at 12:28:46AM -0700, Rasesh Mody wrote:
> From: Rasesh Mody
>
> Changes included in this fix
> - limit CFLAGS to base files
> - fix to remove/mark unused members
> - add checks for debug config option
> - make qede_set_mtu() and qede_udp_dst_port_del() static and others
>non-static as appropriate
> - move local APIs qede_vlan_offload_set() and qede_rx_cqe_to_pkt_type()
> - initialize variables as required
When there are so many items in one single patch, it basically means
it's done wrongly. Generally, we should make one patch for each item.
> Fixes: ec94dbc57362 ("qede: add base driver")
> Cc: sta...@dpdk.org
It's also not a good idea to put "Cc: stable" tag in a huge fix patch.
It's very likely it won't apply cleanly to a stable/LTS release. For
instance, I failed to apply it to 16.11.2 (LTS).
>
> Signed-off-by: Rasesh Mody
> ---
> drivers/net/qede/Makefile | 32 -
> drivers/net/qede/base/ecore.h |4 +-
> drivers/net/qede/base/ecore_int_api.h |4 +-
> drivers/net/qede/qede_ethdev.c| 120
> ++---
> drivers/net/qede/qede_ethdev.h| 32 -
> drivers/net/qede/qede_fdir.c | 13 +---
> drivers/net/qede/qede_if.h|4 ++
> drivers/net/qede/qede_main.c |8 +--
> drivers/net/qede/qede_rxtx.c | 118 +---
> 9 files changed, 171 insertions(+), 164 deletions(-)
It's also a clear sign of bad patch: too many changes for a single bug
fix patch.
Most of them look like minor fixes to me. So my question is are there
any important items really should be picked for stable and LTS release?
More specifically, do they really fix any (fatal) issues? If no, I will
drop it. If yes, please send a (or some) patch with the real fixes
backported only to stable ML, so that I could pick them.
Thanks.
--yliu
Re: [dpdk-dev] [PATCH] devtools: list stable commits do not have fixline
On Fri, Apr 28, 2017 at 11:00:07AM +0200, Thomas Monjalon wrote: > 28/04/2017 10:27, Yuanhan Liu: > > On Fri, Apr 28, 2017 at 10:15:46AM +0200, Thomas Monjalon wrote: > > > 28/04/2017 09:21, Yuanhan Liu: > > > > Some commits for stable releases (with Cc stable tag) may not have the > > > > fixline. For example: > > > > http://dpdk.org/dev/patchwork/patch/23955/ > > > > > > > > It disables a feature we have implemented in last release. The feature > > > > is done right. It's the QEMU implementaton being buggy, that we have to > > > > disable it to workaround those buggy QEMU releases (v2.7 - v2.9). > > > > Without > > > > such workaround, QEMU won't start when queue number >= 2. > > > > > > > > That said, we also have to backport it to stable releases, though there > > > > is no fixline (there was no DPDK bug to fix after all). > > > > > > How do we know where should it be backported? > > > > Good question. As a stable maintainer, I may not know. But the developer > > should know. For such case, he may add something like: > > > > Cc: sta...@dpdk.org # for v17.02+ > > It breaks backport semi-automation. But it should be (easily) fixed. > > It's a trick used widely in kernel and QEMU community. > > > > > It is fixing a bug with a correct implementation because of > > > a buggy dependency. But it is still a bug. > > > So I think we should put a Fixes: line. > > > > I don't have strong objection to this. It just doesn't make too much > > sense to me: there is no bug in the DPDK implementation after all. > > > > But if you insist, I'm okay with it. > > Yes I insist :) > It is fixing code to work with some dependencies. Okay. Besides, okay to merge this patch? As you stated, it does no harm. --yliu
Re: [dpdk-dev] [PATCH] devtools: list stable commits do not have fixline
On Fri, Apr 28, 2017 at 10:15:46AM +0200, Thomas Monjalon wrote: > 28/04/2017 09:21, Yuanhan Liu: > > Some commits for stable releases (with Cc stable tag) may not have the > > fixline. For example: > > http://dpdk.org/dev/patchwork/patch/23955/ > > > > It disables a feature we have implemented in last release. The feature > > is done right. It's the QEMU implementaton being buggy, that we have to > > disable it to workaround those buggy QEMU releases (v2.7 - v2.9). Without > > such workaround, QEMU won't start when queue number >= 2. > > > > That said, we also have to backport it to stable releases, though there > > is no fixline (there was no DPDK bug to fix after all). > > How do we know where should it be backported? Good question. As a stable maintainer, I may not know. But the developer should know. For such case, he may add something like: Cc: sta...@dpdk.org # for v17.02+ It's a trick used widely in kernel and QEMU community. > It is fixing a bug with a correct implementation because of > a buggy dependency. But it is still a bug. > So I think we should put a Fixes: line. I don't have strong objection to this. It just doesn't make too much sense to me: there is no bug in the DPDK implementation after all. But if you insist, I'm okay with it. > > > > There should be similar cases like this. Thus, this patch makes > > git-log-fixes.sh script also list those stable commits do not have > > fixline. > > I am against putting Cc: stable without Fixes: line. General, yes. And luckily, that should be rare. > However, this patch is harmless. Yes. Moreover, we should not miss some important fixes with it. --yliu
Re: [dpdk-dev] [PATCH] vhost: fix MQ fails to startup
On Fri, Apr 28, 2017 at 09:57:20AM +0200, Maxime Coquelin wrote: > >>>Maybe we could introduce a version message? With that, we could tell > >>>whether the frontend has fixed the known bug or not. > >> > >>That's a possibility, but this is not really the role of a protocol > >>version. As in this case, the protocol does not change, just an > >>implementation. > > > >Maybe. Well, you might could think this way: we do increase the version > >when we make a new release (with bugs being fixed). > > > >Or, we could also make the version two parts: major and minor. We increase > >major for major updates (say, new features, etc). We increase minor for > >bug fixes. > > > >The only thing that doesn't make too much sense is the bug is actually > >from the QEMU implementation but not from the vhost-user spec. > > Yes, I was maybe not clear, but that's what I meant when saying that was > not the role of the protocol version. Yes, I realized it later: I overlooked it. Sorry. > >Talking > >about that, it may make more sense to introduce a new message to carry > >the frontend version, something like a string "QEMU v2.8". > > I don't think this is a good idea as it would create more problems that it > would solve. Indeed, you would need also the distro version, as for > example, Red Hat could backport the fix in its QEMU v2.6 package, Ubuntu > in its v2.7, etc... I have thought of stable release, say "QEMU v2.8.1". But you are right, it got way more complex when distro backport is considered :( --yliu
Re: [dpdk-dev] [PATCH] vhost: fix MQ fails to startup
On Fri, Apr 28, 2017 at 03:35:53PM +0800, Yuanhan Liu wrote: > > >Maybe we could introduce a version message? With that, we could tell > > >whether the frontend has fixed the known bug or not. > > > > That's a possibility, but this is not really the role of a protocol > > version. As in this case, the protocol does not change, just an > > implementation. > > Maybe. Well, you might could think this way: we do increase the version > when we make a new release (with bugs being fixed). > > Or, we could also make the version two parts: major and minor. We increase > major for major updates (say, new features, etc). We increase minor for > bug fixes. Nah, just forgot above two paragraphs, I overlooked it. You just need care the below one. --yliu > The only thing that doesn't make too much sense is the bug is actually > from the QEMU implementation but not from the vhost-user spec. Talking > about that, it may make more sense to introduce a new message to carry > the frontend version, something like a string "QEMU v2.8".
Re: [dpdk-dev] [PATCH] vhost: fix MQ fails to startup
On Fri, Apr 28, 2017 at 09:23:54AM +0200, Maxime Coquelin wrote: > > > On 04/28/2017 04:25 AM, Yuanhan Liu wrote: > >On Thu, Apr 27, 2017 at 10:52:20AM +0200, Maxime Coquelin wrote: > >> > >> > >>On 04/27/2017 10:20 AM, Yuanhan Liu wrote: > >>>On Thu, Apr 27, 2017 at 09:56:47AM +0200, Maxime Coquelin wrote: > >>>>Hi Zhiyong, > >>>> > >>>>+Marc-André > >>>> > >>>>On 04/27/2017 08:34 AM, Zhiyong Yang wrote: > >>>>>vhost since dpdk17.02 + qemu2.7 and above will cause failures of > >>>>>new connection when negotiating to set MQ. (one queue pair works > >>>>>well).Because there exist some bugs in qemu code when introducing > >>>>>VHOST_USER_PROTOCOL_F_REPLY_ACK to qemu. when dealing with the vhost > >>>>>message VHOST_USER_SET_MEM_TABLE for the second time, qemu indeed > >>>>>doesn't send the messge (The message needs to be sent only once)but > >>>>>still will be waiting for dpdk's reply ack, then, qemu is always > >>>>>freezing. DPDK code works in the right way. > >>>> > >>>>I'm looking at Qemu's vhost_user_set_mem_table() function, but fail to > >>>>see how it could wait for the reply-ack if it didn't send the > >>>>VHOST_USER_SET_MEM_TABLE request before. > >>>> > >>>>>But the feature > >>>>>VHOST_USER_PROTOCOL_F_REPLY_ACK has to be disabled by default at the > >>>>>dpdk side in order to avoid the feature support of DPDK + qemu at > >>>>>the same time. if doing like that, MQ can works well. Once Qemu bugs > >>>>>have been fixed and upstreamed, we can enable it. > >>>> > >>>>The problem is for DPDK to detect whether bug is fixed in Qemu. > >>>>Maybe only way would be to have a new protocol feature flag, which is > >>>>not really its role. > >>> > >>>Wouldn't that be an overkill, judging that REPLY_ACK is not a must > >>>feature? > >> > >>Yes, maybe. But it was introduced to fix (possible) race conditions: > >>https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06173.html > > > >But AFAIK, that commit has been reverted: > > > > commit 94c9cb31c04737f86be29afefbff401cd23bc24d > > Author: Michael S. Tsirkin > > Date: Mon Aug 15 16:35:24 2016 +0300 > > Revert "vhost-user: Attempt to fix a race with set_mem_table." > > This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23. > > I still think it's the right thing to do, but > > tests have been failing sporadically. > > Revert for now, and hope to fix it before the release. > > No, what has been reverted is a workaround when REPLY_ACK protocol > feature has not been negotiated. Good to know. > > Instead of waiting for the backend to send the ack, the workaround > consisted in sending a GET_FEATURES request after having sent the > SET_MEM_TABLE request, in order to ensure SET_MEM_TABLE request handling > was done before. > > The problem is that it sometimes created a deadlock when when running > QEMU's vhost-user-test in TCG mode. > > >> > >>Note that I planned to use this feature for the device IOTLB > >>implementation to let the backend decide whether it wants the IOTLB > >>misses synchronous or asynchronous. But I can still change the protocol > >>spec to make this behavior specific to this request. > > > >Maybe we could introduce a version message? With that, we could tell > >whether the frontend has fixed the known bug or not. > > That's a possibility, but this is not really the role of a protocol > version. As in this case, the protocol does not change, just an > implementation. Maybe. Well, you might could think this way: we do increase the version when we make a new release (with bugs being fixed). Or, we could also make the version two parts: major and minor. We increase major for major updates (say, new features, etc). We increase minor for bug fixes. The only thing that doesn't make too much sense is the bug is actually from the QEMU implementation but not from the vhost-user spec. Talking about that, it may make more sense to introduce a new message to carry the frontend version, something like a string "QEMU v2.8". --yliu > > >Note that we already has the "version" info in current vhost-user spec. > >It's just 2 bits in the message "flag" field though, which is not quite > >enough. > > Indeed, it does not let room for lots of bugs :) > > Thanks, > Maxime > > --yliu > >
[dpdk-dev] [PATCH] devtools: list stable commits do not have fixline
Some commits for stable releases (with Cc stable tag) may not have the fixline. For example: http://dpdk.org/dev/patchwork/patch/23955/ It disables a feature we have implemented in last release. The feature is done right. It's the QEMU implementaton being buggy, that we have to disable it to workaround those buggy QEMU releases (v2.7 - v2.9). Without such workaround, QEMU won't start when queue number >= 2. That said, we also have to backport it to stable releases, though there is no fixline (there was no DPDK bug to fix after all). There should be similar cases like this. Thus, this patch makes git-log-fixes.sh script also list those stable commits do not have fixline. Signed-off-by: Yuanhan Liu --- devtools/git-log-fixes.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/devtools/git-log-fixes.sh b/devtools/git-log-fixes.sh index 3bf412e..7404946 100755 --- a/devtools/git-log-fixes.sh +++ b/devtools/git-log-fixes.sh @@ -116,7 +116,8 @@ stable_tag () # git log --oneline --reverse $range | while read id headline ; do origins=$(origin_filter $id) - [ -n "$origins" ] || echo "$headline" | grep -q fix || continue + stable=$(stable_tag $id) + [ "$stable" = "S" ] || [ -n "$origins" ] || echo "$headline" | grep -q fix || continue version=$(commit_version $id) if [ -n "$origins" ] ; then origver="$(origin_version $origins)" @@ -126,6 +127,5 @@ while read id headline ; do else origver='N/A' fi - stable=$(stable_tag $id) printf '%s %7s %s %s (%s)\n' $version $id $stable "$headline" "$origver" done -- 1.9.0
Re: [dpdk-dev] [PATCH v2] config: make AVX and AVX512 configurable
On Thu, Apr 27, 2017 at 07:00:14PM -0400, Zhihong Wang wrote: > Making AVX and AVX512 configurable is useful for performance and power > testing. > > The similar kernel patch at https://patchwork.kernel.org/patch/9618883/. > > AVX512 support like in rte_memcpy has been in DPDK since 16.04, but it's > still unproven in rich use cases in hardware. Therefore it's marked as > experimental for now, will enable it after enough field test and possible > optimization. > > Signed-off-by: Zhihong Wang Reviewed-by: Yuanhan Liu --yliu
Re: [dpdk-dev] [PATCH 1/3] net/virtio: fix dev detachable flag
On Wed, Feb 22, 2017 at 10:34:23AM +0800, Yuanhan Liu wrote:
> On Mon, Feb 20, 2017 at 10:04:45PM +0800, hanxue...@126.com wrote:
> > From: Huanle Han
> >
> > The dev detachable flag was removed by
> > commit f229eb4 ("net/virtio: fix rewriting LSC flag").
> >
> > Signed-off-by: Huanle Han
> > ---
> > drivers/net/virtio/virtio_ethdev.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > b/drivers/net/virtio/virtio_ethdev.c
> > index 4dc03b9..8465e1a 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1336,6 +1336,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev,
> > uint64_t req_features)
> > if (eth_dev->device) {
> > pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
> > rte_eth_copy_pci_info(eth_dev, pci_dev);
> > + eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
>
> This is a partial fix. The major issue here is rte_eth_copy_pci_info has
> an undocumented side effect: it resets the dev_flags unconditionally.
>
> Removing such reset should be able to fix it: it also looks like the right
> fix to me. Thomas, Ferruh?
>
> If not, we could at least call rte_eth_copy_pci_info() at
> eth_virtio_dev_init(),
> before we set any dev_flags bits.
This issue has been fixed by http://dpdk.org/dev/patchwork/patch/23949/.
--yliu
Re: [dpdk-dev] [PATCH 2/3] net/virtio: fix crash when close virtio dev twice
On Wed, Feb 22, 2017 at 10:24:13AM +0800, Yuanhan Liu wrote:
> On Mon, Feb 20, 2017 at 10:04:46PM +0800, hanxue...@126.com wrote:
> > From: Huanle Han
> >
> > This commit fixs segment fault when rte_eth_dev_close()
> > is called on a virtio dev more than once.
> > Assigning zero after free to avoids freed memory to
> > be accessed again.
>
> Thanks for the fix! And here are few more minor nits you might want be
> awre of:
>
> - a fix patch needs a fixline (check http://dpdk.org/dev for howto just
> in case you don't know)
>
> - if it fixes a fatal bug (like this one), it should also be picked (or
> backported) to a specific stable release. In this case, you should add
> Cc: sta...@dpdk.org
>
> just before your SoB (Signed-off-by).
Applied to dpdk-next-virtio with:
Fixes: 69c80d4ef89b ("net/virtio: allocate queue at init stage")
Cc: sta...@dpdk.org
Thanks.
--yliu
Re: [dpdk-dev] [PATCH] net/virtio-user: fix recognize physical devices
On Fri, Apr 21, 2017 at 02:28:09AM +, Jianfeng Tan wrote:
> Segfault happens when using virtio-user after commit 7f0a669e7b04
> ("ethdev: add allocation helper for virtual drivers").
>
> It's due to we use ethdev->device to recognize physical devices,
> but after above commit, this field is also filled for virtual
> devices. Then we obtain the wrong pci_dev pointer and accessing
> its field when copying pci info results in segfault.
>
> To fix it, we use hw->virtio_user_dev to differentiate physical
> devices from virtual devices.
>
> Fixes: 6a7c0dfcdf40 ("net/virtio: do not depend on PCI device of ethdev")
>
> Signed-off-by: Jianfeng Tan
Applied to dpdk-next-virtio.
Thanks.
--yliu
Re: [dpdk-dev] [PATCH v2 2/4] net/virtio: clean up LSC setting
On Thu, Apr 27, 2017 at 07:35:37AM +, Jianfeng Tan wrote: > LSC flag is set in several places, but only the last one takes effect; > so we remove the redundant ones and just keep the last one. Note that I modified this commit log a bit. It actually fixed a bug: the dev_flags is being overwritten by rte_eth_copy_pci_info(), which resets it to 0 unconditionally. Thus, "Cc: sta...@dpdk.org" is also added. Thanks. --yliu
Re: [dpdk-dev] [PATCH v2 0/4] fixes and cleanup on virtio LSC
On Thu, Apr 27, 2017 at 07:35:35AM +, Jianfeng Tan wrote: > v2: > - Split 2nd patch into two patches. > > Patch 1: fix wrong MSI-X for modern devices so that LSC is always not > available. > Patch 2: clean up LSC setting. > Patch 3: remove redundant MSI-X detection > Patch 4: fix link status always being down. > > Signed-off-by: Jianfeng Tan FYI, no need to add SoB in cover letter. Series applied to dpdk-next-virtio. Thanks. --yliu > > Jianfeng Tan (4): > net/virtio: fix wrong MSI-X for modern devices > net/virtio: clean up LSC setting > net/virtio: remove redundant MSI-X detection > net/virtio: fix link status always being down > > drivers/net/virtio/virtio_ethdev.c | 14 +- > drivers/net/virtio/virtio_pci.c| 52 > +- > drivers/net/virtio/virtio_pci.h| 3 +-- > 3 files changed, 13 insertions(+), 56 deletions(-) > > -- > 2.7.4
Re: [dpdk-dev] [PATCH v2] vhost: workaround MQ fails to startup
On Thu, Apr 27, 2017 at 12:00:52PM +0200, Maxime Coquelin wrote: > > > On 04/27/2017 11:41 AM, Zhiyong Yang wrote: > > vhost since dpdk17.02 + qemu2.7 and above will cause failures of > >new connection when negotiating to set MQ. (one queue pair works > >well). > >Because there exist some bugs in qemu code when introducing > >VHOST_USER_PROTOCOL_F_REPLY_ACK to qemu. when dealing with the vhost > >message VHOST_USER_SET_MEM_TABLE for the second time, qemu indeed > >doesn't send the messge (The message needs to be sent only once)but > >still will be waiting for dpdk's reply ack, then, qemu is always > >freezing. DPDK code indeed works in the right way. > >The feature VHOST_USER_PROTOCOL_F_REPLY_ACK has to be disabled > >by default at the dpdk side in order to avoid the feature support of > >DPDK + qemu at the same time. if doing like that, MQ can works well. > > > >Cc: sta...@dpdk.org > > > >Reported-by: Ciara Loftus > >Signed-off-by: Zhiyong Yang > >Tested-by: Ciara Loftus > >--- > > > >changes in V2 > >1. modify "workaround" instead of "fix" in the title. > >2. add a simple comment suggested by yuanhan > > > > lib/librte_vhost/vhost_user.h | 6 +- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > Reviewed-by: Maxime Coquelin Applied to dpdk-next-virtio. Thanks. --yliu
Re: [dpdk-dev] [PATCH] vhost: fix MQ fails to startup
On Thu, Apr 27, 2017 at 10:52:20AM +0200, Maxime Coquelin wrote: > > > On 04/27/2017 10:20 AM, Yuanhan Liu wrote: > >On Thu, Apr 27, 2017 at 09:56:47AM +0200, Maxime Coquelin wrote: > >>Hi Zhiyong, > >> > >>+Marc-André > >> > >>On 04/27/2017 08:34 AM, Zhiyong Yang wrote: > >>>vhost since dpdk17.02 + qemu2.7 and above will cause failures of > >>>new connection when negotiating to set MQ. (one queue pair works > >>>well).Because there exist some bugs in qemu code when introducing > >>>VHOST_USER_PROTOCOL_F_REPLY_ACK to qemu. when dealing with the vhost > >>>message VHOST_USER_SET_MEM_TABLE for the second time, qemu indeed > >>>doesn't send the messge (The message needs to be sent only once)but > >>>still will be waiting for dpdk's reply ack, then, qemu is always > >>>freezing. DPDK code works in the right way. > >> > >>I'm looking at Qemu's vhost_user_set_mem_table() function, but fail to > >>see how it could wait for the reply-ack if it didn't send the > >>VHOST_USER_SET_MEM_TABLE request before. > >> > >>>But the feature > >>>VHOST_USER_PROTOCOL_F_REPLY_ACK has to be disabled by default at the > >>>dpdk side in order to avoid the feature support of DPDK + qemu at > >>>the same time. if doing like that, MQ can works well. Once Qemu bugs > >>>have been fixed and upstreamed, we can enable it. > >> > >>The problem is for DPDK to detect whether bug is fixed in Qemu. > >>Maybe only way would be to have a new protocol feature flag, which is > >>not really its role. > > > >Wouldn't that be an overkill, judging that REPLY_ACK is not a must > >feature? > > Yes, maybe. But it was introduced to fix (possible) race conditions: > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06173.html But AFAIK, that commit has been reverted: commit 94c9cb31c04737f86be29afefbff401cd23bc24d Author: Michael S. Tsirkin Date: Mon Aug 15 16:35:24 2016 +0300 Revert "vhost-user: Attempt to fix a race with set_mem_table." This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23. I still think it's the right thing to do, but tests have been failing sporadically. Revert for now, and hope to fix it before the release. > > Note that I planned to use this feature for the device IOTLB > implementation to let the backend decide whether it wants the IOTLB > misses synchronous or asynchronous. But I can still change the protocol > spec to make this behavior specific to this request. Maybe we could introduce a version message? With that, we could tell whether the frontend has fixed the known bug or not. Note that we already has the "version" info in current vhost-user spec. It's just 2 bits in the message "flag" field though, which is not quite enough. --yliu
Re: [dpdk-dev] [PATCH] vhost: fix MQ fails to startup
On Thu, Apr 27, 2017 at 09:56:47AM +0200, Maxime Coquelin wrote: > Hi Zhiyong, > > +Marc-André > > On 04/27/2017 08:34 AM, Zhiyong Yang wrote: > >vhost since dpdk17.02 + qemu2.7 and above will cause failures of > >new connection when negotiating to set MQ. (one queue pair works > >well).Because there exist some bugs in qemu code when introducing > >VHOST_USER_PROTOCOL_F_REPLY_ACK to qemu. when dealing with the vhost > >message VHOST_USER_SET_MEM_TABLE for the second time, qemu indeed > >doesn't send the messge (The message needs to be sent only once)but > >still will be waiting for dpdk's reply ack, then, qemu is always > >freezing. DPDK code works in the right way. > > I'm looking at Qemu's vhost_user_set_mem_table() function, but fail to > see how it could wait for the reply-ack if it didn't send the > VHOST_USER_SET_MEM_TABLE request before. > > >But the feature > >VHOST_USER_PROTOCOL_F_REPLY_ACK has to be disabled by default at the > >dpdk side in order to avoid the feature support of DPDK + qemu at > >the same time. if doing like that, MQ can works well. Once Qemu bugs > >have been fixed and upstreamed, we can enable it. > > The problem is for DPDK to detect whether bug is fixed in Qemu. > Maybe only way would be to have a new protocol feature flag, which is > not really its role. Wouldn't that be an overkill, judging that REPLY_ACK is not a must feature? --yliu
Re: [dpdk-dev] [PATCH] vhost: fix MQ fails to startup
On Thu, Apr 27, 2017 at 02:34:53PM +0800, Zhiyong Yang wrote:
> vhost since dpdk17.02 + qemu2.7 and above will cause failures of
> new connection when negotiating to set MQ. (one queue pair works
> well).Because there exist some bugs in qemu code when introducing
> VHOST_USER_PROTOCOL_F_REPLY_ACK to qemu. when dealing with the vhost
> message VHOST_USER_SET_MEM_TABLE for the second time, qemu indeed
> doesn't send the messge (The message needs to be sent only once)but
> still will be waiting for dpdk's reply ack, then, qemu is always
> freezing. DPDK code works in the right way. But the feature
> VHOST_USER_PROTOCOL_F_REPLY_ACK has to be disabled by default at the
> dpdk side in order to avoid the feature support of DPDK + qemu at
> the same time. if doing like that, MQ can works well.
...
> Once Qemu bugs
> have been fixed and upstreamed, we can enable it.
As I have said, we should not enable it again, because there are already
few buggy QEMU releases out. We should make sure DPDK also works well with
them.
> Fixes: 73c8f9f69c6c("vhost: introduce reply ack feature")
That commit does nothing wrong. It's QEMU being buggy. That said, I will
not add such fixline. I will also use "workaround" instead of "fix" in
the title.
Also, this patch should be backported to stable release. So, you should
add:
Cc: sta...@dpdk.org
Besides, please reformat you commit log a bit. For example, add space
after punctuation, use paragraph as possible, etc.
>
> Reported-by: Loftus, Ciara
No "," is allowed.
> Signed-off-by: Zhiyong Yang
> ---
> lib/librte_vhost/vhost_user.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> index 2ba22db..a3d2900 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -52,7 +52,7 @@
> #define VHOST_USER_PROTOCOL_FEATURES ((1ULL << VHOST_USER_PROTOCOL_F_MQ) | \
>(1ULL <<
> VHOST_USER_PROTOCOL_F_LOG_SHMFD) |\
>(1ULL << VHOST_USER_PROTOCOL_F_RARP) |
> \
> - (1ULL <<
> VHOST_USER_PROTOCOL_F_REPLY_ACK) | \
> + (0ULL <<
> VHOST_USER_PROTOCOL_F_REPLY_ACK) | \
>(1ULL <<
> VHOST_USER_PROTOCOL_F_NET_MTU))
I think you might want to add a simple comment here, something like
/*
* disable REPLY_ACK feature to workaround the buggy QEMU implementation.
* Proved buggy QEMU includes v2.7 - v2.9.
*/
So, please send v2?
--yliu
Re: [dpdk-dev] [PATCH v2 2/4] net/virtio: clean up LSC setting
On Thu, Apr 27, 2017 at 07:35:37AM +, Jianfeng Tan wrote: > LSC flag is set in several places, but only the last one takes effect; > so we remove the redundant ones and just keep the last one. I think this patch would also fix the issue reported by: http://dpdk.org/dev/patchwork/patch/20552/ You patch is better, I will take yours. --yliu
Re: [dpdk-dev] [PATCH v2] net/virtio: support to turn on/off traffic flow
On Wed, Apr 26, 2017 at 09:45:42AM +0200, Maxime Coquelin wrote: > As said, only cosmetic comments, it looks otherwise valid to me. > If you fix these, feel free to add my: > Acked-by: Maxime Coquelin Oops, I forgot to reply this thread, that I have already applied it :/ --yliu
Re: [dpdk-dev] [PATCH 2/3] net/virtio: clean up LSC setting
On Wed, Apr 26, 2017 at 05:44:05AM +, Tan, Jianfeng wrote: > > > > -Original Message- > > From: Yuanhan Liu [mailto:yuanhan@linux.intel.com] > > Sent: Wednesday, April 26, 2017 1:33 PM > > To: Tan, Jianfeng > > Cc: dev@dpdk.org; maxime.coque...@redhat.com; tho...@monjalon.net > > Subject: Re: [PATCH 2/3] net/virtio: clean up LSC setting > > > > On Wed, Apr 26, 2017 at 04:52:50AM +, Jianfeng Tan wrote: > > > Clean up LSC setting: > > > > Firstly, this patch does two things. There should be two patches. > > I just merged them into one to adapt to the name of "clean up" :-) I then could merge all bug fix patches into one and name it "fix buges"? No, please don't do that. It hurts review. > > > > > - LSC flag is set in several places, but only the last one takes > > > effect; so we just keep the last one. > > > - As we already change to use capability list to detect MSI-X, remove > > > the redundant MSI-X detection in legacy devices. > > > > However, there is no capability list for legacy device. > > When I try legacy device on QEMU (disable-modern=true), I did detect MSI-X > capability. So does virtio spec mean legacy devices do not have > vendor-specific capability list? Oh, right. I mixed that up. --yliu
Re: [dpdk-dev] [PATCH 2/3] net/virtio: clean up LSC setting
On Wed, Apr 26, 2017 at 04:52:50AM +, Jianfeng Tan wrote: > Clean up LSC setting: Firstly, this patch does two things. There should be two patches. > - LSC flag is set in several places, but only the last one takes > effect; so we just keep the last one. > - As we already change to use capability list to detect MSI-X, remove > the redundant MSI-X detection in legacy devices. However, there is no capability list for legacy device. --yliu
Re: [dpdk-dev] [PATCH] net/virtio-user: fix recognize physical devices
On Fri, Apr 21, 2017 at 02:28:09AM +, Jianfeng Tan wrote:
> Segfault happens when using virtio-user after commit 7f0a669e7b04
> ("ethdev: add allocation helper for virtual drivers").
>
> It's due to we use ethdev->device to recognize physical devices,
> but after above commit, this field is also filled for virtual
> devices. Then we obtain the wrong pci_dev pointer and accessing
> its field when copying pci info results in segfault.
>
> To fix it, we use hw->virtio_user_dev to differentiate physical
> devices from virtual devices.
>
> Fixes: 6a7c0dfcdf40 ("net/virtio: do not depend on PCI device of ethdev")
>
> Signed-off-by: Jianfeng Tan
Acked-by: Yuanhan Liu
--yliu
Re: [dpdk-dev] [PATCH 05/13] vhost: fix errors with strict compilation flags
On Mon, Apr 24, 2017 at 05:52:59PM +0200, Adrien Mazarguil wrote:
> Exported headers must allow compilation with the strictest flags. This
> commit addresses the following errors:
>
> In file included from /tmp/check-includes.sh.20132.c:1:0:
> build/include/rte_vhost.h:73:30: error: ISO C forbids zero-size array
> 'regions' [-Werror=pedantic]
> [...]
>
> Also:
>
> - Add C++ awareness to rte_vhost.h for consistency with rte_eth_vhost.h.
> - Move Linux includes into C++ block to prevent linking issues with
> exported symbols.
> - Update check-includes.sh following the removal of rte_virtio_net.h.
>
> Finally, update check-includes.sh to ignore rte_vhost.h and rte_eth_vhost.h
> from now on since the Linux headers they depend on are not clean enough:
>
> In file included from /usr/include/linux/vhost.h:17:0,
> from build/include/rte_vhost.h:43,
> from build/include/rte_eth_vhost.h:44,
> from /tmp/check-includes.sh.20132.c:1:
> /usr/include/linux/virtio_ring.h: In function 'vring_init':
> /usr/include/linux/virtio_ring.h:146:16: error: pointer of type 'void *'
> used in arithmetic [-Werror=pointer-arith]
> [...]
> In file included from build/include/rte_vhost.h:43:0,
> from build/include/rte_eth_vhost.h:44,
> from /tmp/check-includes.sh.20132.c:1:
> /usr/include/linux/vhost.h: At top level:
> /usr/include/linux/vhost.h:73:3: error: ISO C99 doesn't support unnamed
> structs/unions [-Werror=pedantic]
> [...]
>
> Fixes: eb32247457fe ("vhost: export guest memory regions")
> Fixes: a798beb47c8e ("vhost: rename header file")
>
> Cc: Yuanhan Liu
> Cc: Maxime Coquelin
> Signed-off-by: Adrien Mazarguil
Acked-by: Yuanhan Liu
--yliu
Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port reset
On Fri, Apr 21, 2017 at 11:28:37AM +0200, Thomas Monjalon wrote: > > Maybe I should add some words in doc\guides\nics\i40e.rst to Record which > > configurations are saved and restored by the PMD driver in reset > > function. Which not list in that are recognized as not saved and restored > > by default. OTHER NIC for this feature can add similar record in their > > xxx.rst. > > No, when defining a generic API in ethdev, we must define the same > behaviour for every drivers. Agreed. That was my point. --yliu > Please check how to make the behaviour consistent and documented > in ethdev. We may need to document your new function and start/stop also.
Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port reset
On Thu, Apr 20, 2017 at 09:17:24AM +, Zhao1, Wei wrote:
> > > > Please explain exactly the responsibility of this function, and how
> > > > it is different from calling stop/configure/start.
> > >
> > > In this reset feature, reset function can do the calling
> > > stop/configure/start process, but also It can also do some restore
> > > work for the port, for example, it can restore the added parameters of
> > vlan, mac_addrs, promisc_unicast_enabled falg and
> > promisc_multicast_enabled flag.
> > > Maybe , I should add this explanation in the patch comments or function
> > comments?
> >
> > I'm curious why we have to do save & restore for a reset operation.
> > Why some configures have to be saved and restored? Doesn't "reset"
> > literally means reset everything?
> >
>
> Users maybe do not want to do a second configuration operation to waste time
> after reset which lost all previous configuration.
> But he still want these configuration valid after reset.
> So, save & restore can help them to save this process time and effort.
>
> > Even though, how do you tell what kind of configures need be restored and
> > what should not? Again, even though, will all PMDs supports restoring the
> > same set of configurations?
> >
>
> Yes, this is hard to say what may be need and what may be not for user.
> Now, the kinds of supported is list in patch set comment. And only i40e NIC
> support this feature.
Why it's the configurations listed in patch 2? Because they are requested
by customers?
Is that all could be saved? If not, are you going to save & restore all
possible configurations?
Assuming the configurations saved & restored may differ from different
PMD drivers, how could you keep the consistency then? And judging that the
application has no idea about the difference (yet it knows nothing about
what kind of configurations will be saved), how could the application know
that some configurations are not saved & restored by the driver that it
has to do re-configuration by itself?
> > While looking at your reset implementation for i40e, it looks more complex
> > than necessary: just thinking we have to call "xxx_queue_setup"
> > for all PMDs.
> >
> > I'm thinking a simple hardware reset might be enough?
> >
> > /* literally reset the hardware: reset everything */
> > rte_eth_reset(port)
> > {
> > eth_dev->ops->reset();
> > }
> >
>
> You mean just do a reset and do not restore any configuration?
> That may not meet the need for this feature from customer?
Right, I'm just aware of the configuration might be done by PF (but not
only by the application), that the VF port may be not aware of those
configurations. So the save & restore is needed. I don't quite like
how it is done in this patch set though. I also don't think the API is
well named: as said, reset should literally reset everything.
We may need think how to do it properly.
Thomas, Konstantin, what do you guys think of it?
--yliu
Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port reset
On Thu, Apr 06, 2017 at 02:57:29AM +, Zhao1, Wei wrote:
> > > + * Reset an ethernet device when it's not working. One scenario is,
> > > + after PF
> > > + * port is down and up, the related VF port should be reset.
> > > + * The API will stop the port, clear the rx/tx queues, re-setup the
> > > + rx/tx
> > > + * queues, restart the port.
> >
> > s/The API/This function/
> >
> > Please explain exactly the responsibility of this function, and how it is
> > different from calling stop/configure/start.
>
> In this reset feature, reset function can do the calling stop/configure/start
> process, but also
> It can also do some restore work for the port, for example, it can restore
> the added parameters
> of vlan, mac_addrs, promisc_unicast_enabled falg and
> promisc_multicast_enabled flag.
> Maybe , I should add this explanation in the patch comments or function
> comments?
I'm curious why we have to do save & restore for a reset operation.
Why some configures have to be saved and restored? Doesn't "reset"
literally means reset everything?
Even though, how do you tell what kind of configures need be restored
and what should not? Again, even though, will all PMDs supports
restoring the same set of configurations?
While looking at your reset implementation for i40e, it looks more
complex than necessary: just thinking we have to call "xxx_queue_setup"
for all PMDs.
I'm thinking a simple hardware reset might be enough?
/* literally reset the hardware: reset everything */
rte_eth_reset(port)
{
eth_dev->ops->reset();
}
Assume the application already has a function (say, port_init()) to
initiate a specific port, it then just needs do something like following
to handle the case you described in the commit log:
rte_eth_reset(port);
port_init(port);
Makes sense? Sorry it's completely wrong; I've limited knowledge on NIC
pmd drivers after all :/
--yliu
Re: [dpdk-dev] [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
On Thu, Apr 13, 2017 at 04:21:04PM +0800, Wei Dai wrote:
> some customers find adding mac addr to VF sometimes can fail,
> but it is still stored in dev->data->mac_addrs[ ]. So this
> can lead to some errors that assumes the non-zero entry in
> dev->data->mac_addrs[ ] is valid.
>
> Fixes: af75078fece3 ("first public release")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Wei Dai
> ---
> drivers/net/bnx2x/bnx2x_ethdev.c | 7 +--
> drivers/net/bnxt/bnxt_ethdev.c | 12 ++--
> drivers/net/e1000/base/e1000_api.c | 2 +-
> drivers/net/e1000/em_ethdev.c | 6 +++---
> drivers/net/e1000/igb_ethdev.c | 5 +++--
> drivers/net/enic/enic.h| 2 +-
> drivers/net/enic/enic_ethdev.c | 4 ++--
> drivers/net/enic/enic_main.c | 6 +++---
> drivers/net/fm10k/fm10k_ethdev.c | 3 ++-
> drivers/net/i40e/i40e_ethdev.c | 11 ++-
> drivers/net/i40e/i40e_ethdev_vf.c | 8
> drivers/net/ixgbe/ixgbe_ethdev.c | 27 ++-
> drivers/net/mlx4/mlx4.c| 18 +++---
> drivers/net/mlx5/mlx5.h| 4 ++--
> drivers/net/mlx5/mlx5_mac.c| 16 ++--
> drivers/net/qede/qede_ethdev.c | 6 --
> drivers/net/ring/rte_eth_ring.c| 3 ++-
> drivers/net/virtio/virtio_ethdev.c | 13 +++--
For virtio changes,
Acked-by: Yuanhan Liu
--yliu
Re: [dpdk-dev] [PATCH] vhost: fix dequeue zero copy
On Wed, Apr 19, 2017 at 01:26:01PM +0800, Yuanhan Liu wrote:
> For zero copy mode, we need pin the mbuf to not let the underlaying PMD
> driver (or the app) free the mbuf. Currently, only the heading mbuf is
> pinned. However, the mbuf free function would try to free all mbufs
> in the mbuf chain (-1 to the refcnt). This may lead the head mbuf being
> still pinned, while the other subsequent mbufs are actually freed. Which
> is wrong.
>
> It becomes more fatal after the mbuf refactor, more specificly, after
> the commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool"). The
> refcnt resets to 1 after the last real reference. OTOH, it leads to a
> situtation that we never know one mbuf is actually freed or not. This
> would result the mbuf __just__ after the heading mbuf being freed twice:
> it's firstly freed (and put back to mempool) when the underlaying PMD
> finishes the DMA. Later, it will then be freed again when vhost unpins
> it. Meaning, one mbuf may be returned to the mempool twice, while in
> turn, being allocated twice later. Something uncertain may happen then.
> For example, the VM2VM case becomes broken.
>
> Fixes: b0a985d1f340 ("vhost: add dequeue zero copy")
>
> Cc: sta...@dpdk.org
> Signed-off-by: Yuanhan Liu
Applied to dpdk-next-virtio.
--yliu
Re: [dpdk-dev] [PATCH v2] net/virtio-user: fix not working on 32-bit system
On Wed, Apr 19, 2017 at 06:21:59AM +, Tan, Jianfeng wrote: > > > > I've expected it to be plain english, something like following: > > > > /* > > * Return the physical address (or virtual address in case of > > * virtio-user) of mbuf data buffer. > > * > > * The address is firstly casted to the word size (sizeof(uintptr_t)) > > * before casting it to uint64_t. This is to make it work with different > > * combination of word size (64 bit and 32 bit) and virtio device > > * (virtio-pci and virtio-user). > > */ > > > > Okay to you? > > Yep, sounds better. Thanks! Good. Applied to dpdk-next-virtio, with above change. Thanks. --yliu
Re: [dpdk-dev] [PATCH] vhost: avoid memory write on net header when necessary
On Fri, Apr 14, 2017 at 03:53:18PM +0800, Yuanhan Liu wrote:
> Like what we did for virtio PMD driver [0][1], we could also apply such
> trick to vhost, to avoid the memory write on net header when necessary.
>
> [0]: c9ea670c1dc7 ("net/virtio: fix performance regression due to TSO")
> [1]: 16994abee215 ("net/virtio: optimize header reset on any layout")
>
> With this, the cache issue of the mergeable path is again greatly reduced:
> even the write of "num_buffers" could be avoided. A quick PVP test shows
> the gap between the mergeable Rx and non-mergable Rx is pretty small now:
> they are basically the same in my test.
>
> Signed-off-by: Yuanhan Liu
Applied to dpdk-next-virtio.
--yliu
Re: [dpdk-dev] [PATCH v2] net/virtio-user: fix not working on 32-bit system
On Wed, Apr 19, 2017 at 02:30:33AM +, Jianfeng Tan wrote:
> virtio-user cannot work on 32-bit system as higher 32-bit of the
> addr field (64-bit) in the desc is filled with non-zero value
> which should not happen for a 32-bit system.
>
> In case of virtio-user, we use buf_addr of mbuf to fill the
> virtqueue desc addr. This is a regression bug. For 32-bit system,
> the first 4 bytes of mbuf is buf_addr, with following 8 bytes for
> buf_phyaddr. With below wrong definition, both buf_addr and lower
> 4 bytes buf_phyaddr are obtained to fill the virtqueue desc.
> #define VIRTIO_MBUF_ADDR(mb, vq) \
> (*(uint64_t *)((uintptr_t)(mb) + (vq)->offset))
>
> Fixes: 25f80d108780 ("net/virtio: fix packet corruption")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Jianfeng Tan
> ---
> drivers/net/virtio/virtqueue.h | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index f9e3736..2e67460 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -69,10 +69,16 @@ struct rte_mbuf;
>
> #ifdef RTE_VIRTIO_USER
> /**
> - * Return the physical address (or virtual address in case of
> - * virtio-user) of mbuf data buffer.
> + *
> + * Return the physical address of mbuf data buffer for virtio pci:
> + * on 32-bit system, offset equals 4, return the second four bytes of mbuf;
> + * on 64-bit system, offset equals 8, return the second eight bytes of mbuf.
> + * Return the virtual address of mbuf data buffer for virtio-user.
> + * on 32-bit system, offset equals 0, return the first four bytes of mbuf;
> + * on 64-bit system, offset equals 0, return the first eight bytes of mbuf;
I've expected it to be plain english, something like following:
/*
* Return the physical address (or virtual address in case of
* virtio-user) of mbuf data buffer.
*
* The address is firstly casted to the word size (sizeof(uintptr_t))
* before casting it to uint64_t. This is to make it work with different
* combination of word size (64 bit and 32 bit) and virtio device
* (virtio-pci and virtio-user).
*/
Okay to you?
--yliu
[dpdk-dev] [PATCH] vhost: fix dequeue zero copy
For zero copy mode, we need pin the mbuf to not let the underlaying PMD
driver (or the app) free the mbuf. Currently, only the heading mbuf is
pinned. However, the mbuf free function would try to free all mbufs
in the mbuf chain (-1 to the refcnt). This may lead the head mbuf being
still pinned, while the other subsequent mbufs are actually freed. Which
is wrong.
It becomes more fatal after the mbuf refactor, more specificly, after
the commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool"). The
refcnt resets to 1 after the last real reference. OTOH, it leads to a
situtation that we never know one mbuf is actually freed or not. This
would result the mbuf __just__ after the heading mbuf being freed twice:
it's firstly freed (and put back to mempool) when the underlaying PMD
finishes the DMA. Later, it will then be freed again when vhost unpins
it. Meaning, one mbuf may be returned to the mempool twice, while in
turn, being allocated twice later. Something uncertain may happen then.
For example, the VM2VM case becomes broken.
Fixes: b0a985d1f340 ("vhost: add dequeue zero copy")
Cc: sta...@dpdk.org
Signed-off-by: Yuanhan Liu
---
lib/librte_vhost/virtio_net.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index b9f2168..d3d4424 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -871,6 +871,8 @@ static inline int __attribute__((always_inline))
"allocate memory for mbuf.\n");
return -1;
}
+ if (unlikely(dev->dequeue_zero_copy))
+ rte_mbuf_refcnt_update(cur, 1);
prev->next = cur;
prev->data_len = mbuf_offset;
--
1.9.0
Re: [dpdk-dev] [PATCH 0/2] net/virtio: support to turn on/off the traffic flow
On Wed, Apr 19, 2017 at 02:31:58AM +, Yang, Zhiyong wrote: > Haha, you are right. > I will rework it and send V2. Note that the "started" field is added in one of my recent patch, to addres the link status always being UP. So you should pull the latest virtio code and make a patch there: you don't need to introduce it any more. --yliu
Re: [dpdk-dev] [PATCH] net/virtio-user: fix LSC not working
On Fri, Apr 14, 2017 at 06:10:30AM +, Jianfeng Tan wrote:
> Previously, we miss to set intr_handle->fd which will be used as
> target file for epoll to check LSC.
>
> As a result, stdin (0) is used and intr thread keeps busy whenever
> data comes from stdin.
>
> To fix this, we use vhostfd as the target file for epoll to check
> the link status change events. And we move intr_handle initialization
> after vhost backend settup to make sure vhostfd is initialized.
>
> Fixes: 35c4f8554833 ("net/virtio-user: support to report net status")
>
> Signed-off-by: Jianfeng Tan
Applied to dpdk-next-virtio.
Thanks.
--yliu
Re: [dpdk-dev] [PATCH] net/virtio: fix link status always being up
On Fri, Apr 14, 2017 at 02:36:45PM +0800, Yuanhan Liu wrote:
> The virtio port link status will always be UP, even the port is stopped:
>
> testpmd> port stop 0
> Stopping ports...
> Checking link statuses...
> Port 0 Link Up - speed 1 Mbps - full-duplex
> Done
>
> The link status is queried by link_update callback when LSC is disabled.
> Which in turn queries the "status" field. However, the "status" is
> read-only. I couldn't think of some proper ways to change the status
> without doing device reset.
>
> Instead of doing (the heavy) reset at stop, this patch introduced a flag,
> which is set to 1 and 0 on start and stop, respectively. When it's set to
> 0, the link status is set to DOWN unconditionally.
>
> Fixes: a85786dc816f ("virtio: fix states handling during initialization")
>
> Cc: sta...@dpdk.org
> Signed-off-by: Yuanhan Liu
Applied to dpdk-next-virtio.
--yliu
Re: [dpdk-dev] [PATCH 0/2] net/virtio: support to turn on/off the traffic flow
On Mon, Apr 17, 2017 at 08:50:52AM +, Yang, Zhiyong wrote: > Hi, yuanhan: > Sorry for the delay reply due to my annual leave. > > > -Original Message- > > From: Yuanhan Liu [mailto:yuanhan@linux.intel.com] > > Sent: Thursday, April 6, 2017 12:00 PM > > To: Yang, Zhiyong > > Cc: dev@dpdk.org; maxime.coque...@redhat.com > > Subject: Re: [PATCH 0/2] net/virtio: support to turn on/off the traffic flow > > > > On Fri, Mar 31, 2017 at 07:40:17PM +0800, Zhiyong Yang wrote: > > > Current dpdk code virtio_dev_stop only disables interrupt and marks > > > link down, When it is invoked, tx/rx traffic flows still work. This is a > > > strange > > behavior. > > > The patchset supports the switch of flow by calling virtio_dev_start/stop. > > > > > > The implementation refers to vhost pmd. > > > > That's a difference story. Vhost pmd uses 2 vars to track the status, > > whereas you > > are using only one here. So why not setting/clearing "started" at > > dev_start/stop, > > respectively? > > Then we can check "started" at Rx/Tx functions. > > Yes, I use only one var since I think vhost pmd using two is too complex and > it is unnecessary. No, it's needed. For vhost-user pmd, we can only do Rx when both below items are met: - port is started - new_device() is invoked, aka, the device is connected For that reason, two vars is used to track it. > I'm setting/clearing started at virtio_dev_start/stop, update_queuing_status > is added to avoid > duplicate code. It's not about duplicate code. While we could make the var per-device, you make it per-queue. That's complex and unnecessary. > I don't understand your question. > > > > > BTW, why does it have to be atomic? > > > > Consider again. It is not necessary to use atomic here. But It seems that it > doesn't have an negative effect. Hmm... that's a good reason to keep it, just because it has no negative effect? Talking about the negative effect, badly, it really has. The atomic is more expensive. --yliu
Re: [dpdk-dev] [PATCH] vhost: fix use after free
On Tue, Apr 18, 2017 at 10:20:41AM +0200, Maxime Coquelin wrote:
>
>
> On 04/17/2017 09:27 AM, Yuanhan Liu wrote:
> >A "return" is missing on error, which could lead to a "use after free"
> >issue (about var "conn").
> >
> >Fixes: 65388b43f592 ("vhost: fix fd leaks for vhost-user server mode")
> >Coverity issue: 143476
> >
> >Reported-by: John McNamara
> >Signed-off-by: Yuanhan Liu
> >---
> > lib/librte_vhost/socket.c | 1 +
> > 1 file changed, 1 insertion(+)
>
> Reviewed-by: Maxime Coquelin
Applied to dpdk-next-virtio.
--yliu
Re: [dpdk-dev] clang compilation errors with clang 4.0
On Fri, Apr 14, 2017 at 04:55:53PM +0100, Bruce Richardson wrote:
> Hi all,
>
> just a heads-up that there are compilation errors showing up with
> compiling DPDK with clang 4.0 release. Here are some of the errors I am
> seeing. Volunteers to do patches for some of them welcome.
>
> Regards,
> /Bruce
>
> /home/bruce/dpdk.org/lib/librte_eal/common/eal_common_tailqs.c:92:24:
> warning: taking address of packed member 'qlock' of class or structure
> 'rte_mem_config' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> rte_rwlock_read_lock(&mcfg->qlock);
> ^~~
>
>
> /home/bruce/dpdk.org/lib/librte_eventdev/rte_eventdev.c:371:6: warning:
> logical not is only applied to the left hand side of this bitwise operator
> [-Wlogical-not-parentheses]
> if (!dev_conf->event_dev_cfg & RTE_EVENT_DEV_CFG_PER_DEQUEUE_TIMEOUT)
> {
> ^~
>
> /home/bruce/dpdk.org/lib/librte_ip_frag/rte_ipv4_reassembly.c:139:31:
> warning: taking address of packed member 'src_addr' of class or structure
> 'ipv4_hdr' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> psd = (unaligned_uint64_t *)&ip_hdr->src_addr;
> ^~~~
>
> /home/bruce/dpdk.org/lib/librte_vhost/vhost_user.c:1037:34: warning: taking
> address of packed member 'payload' of class or structure 'VhostUserMsg' may
> result in an unaligned pointer value [-Waddress-of-packed-member]
> vhost_user_set_vring_num(dev, &msg.payload.state);
>^
Besides the 2nd warnign, all others are the same. Though I'm not quite
sure others, for this vhost-user one, I think it's a false-positive
warning.
--yliu
[dpdk-dev] [PATCH] vhost: fix use after free
A "return" is missing on error, which could lead to a "use after free"
issue (about var "conn").
Fixes: 65388b43f592 ("vhost: fix fd leaks for vhost-user server mode")
Coverity issue: 143476
Reported-by: John McNamara
Signed-off-by: Yuanhan Liu
---
lib/librte_vhost/socket.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 66fd335..c7f99b0 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -242,6 +242,7 @@ struct vhost_user {
RTE_LOG(ERR, VHOST_CONFIG,
"failed to add fd %d into vhost server fdset\n",
fd);
+ return;
}
pthread_mutex_lock(&vsocket->conn_mutex);
--
1.9.0
