[dpdk-dev] PATCH] mbuf: rte_pktmbuf_dump: don't add 0x when using %p in format strings
On 2016-06-21 15:11, Ferruh Yigit wrote: > On 6/20/2016 5:19 PM, Stephen Hemminger wrote: >> On Mon, 20 Jun 2016 12:44:35 +0200 >> Simon Kagstrom wrote: >> >>> I.e., avoid dump messages with double 0x0x, e.g., >>> >>> dump mbuf at 0x0x7fac7b17c800, phys=17b17c880, buf_len=2176 >>> pkt_len=2064, ol_flags=0, nb_segs=1, in_port=255 >>> segment at 0x0x7fac7b17c800, data=0x0x7fac7b17c8f0, data_len=2064 >>> >>> Signed-off-by: Simon Kagstrom >> >> Agreed. Many other places have same glitch. > > Are you planning to extend your patch to fix all, or I can send a patch > for kni and vmxnet3? I wasn't planning on that (this is something I just stumbled upon), so please go ahead! // Simon
[dpdk-dev] [PATCH / RFC] sched: Correct subport calcuation
Hi again! Any news about this patch? I'm off for parental leave starting next week (until january), so any comments (or simply dropping it!) would be good to have before that :-) // Simon On 2016-06-10 08:29, Simon Kagstrom wrote: > Signed-off-by: Simon Kagstrom > --- > I'm a total newbie to the rte_sched design and implementation, so I've > added the RFC. > > We get crashes (at other places in the scheduler) without this code. > > lib/librte_sched/rte_sched.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c > index 1609ea8..b46ecfb 100644 > --- a/lib/librte_sched/rte_sched.c > +++ b/lib/librte_sched/rte_sched.c > @@ -1869,7 +1869,7 @@ grinder_next_pipe(struct rte_sched_port *port, uint32_t > pos) > > /* Install new pipe in the grinder */ > grinder->pindex = pipe_qindex >> 4; > - grinder->subport = port->subport + (grinder->pindex / > port->n_pipes_per_subport); > + grinder->subport = port->subport + (grinder->pindex / > port->n_subports_per_port); > grinder->pipe = port->pipe + grinder->pindex; > grinder->pipe_params = NULL; /* to be set after the pipe structure is > prefetched */ > grinder->productive = 0; >
[dpdk-dev] [PATCH / RFC ] ethdev: Allow rte_eth_dev_configure with zero RX/TX queues
Ping? Any more comments on this? // Simon On 2016-05-16 15:16, Simon K?gstr?m wrote: > On 2016-05-16 14:43, Pattan, Reshma wrote: This was added to allow devices, at least with one direction (RX/TX) >>> supported. As, devices with both directions disabled doesn't make sense >>> right? >>> >>> Well, not for running them, no. But this is a part of the shutdown procedure >>> between tests (I should have been more clear I guess). >> >> Yes I understood this. But I am not sure if you can use >> rte_eth_dev_configure(port, 0, 0) to free the resources. >> Can you check if you can use rte_eth_dev_rx_queue_stop/ >> rte_eth_dev_tx_queue_stop to achieve the same, because they do take care of >> releasing mbufs, but doesn't free the queue's sw-ring and queue. > > But isn't that very strange behavior. Aren't the descriptor rings > allocated in rx_queue_setup()? If so, the sequence > > rx_queue_stop(); // Release buffers > rx_queue_start(); > > would leave the descriptor ring empty after start, i.e., not able to > receive data. > > // Simon >
[dpdk-dev] [PATCH] Add rte_mempool_free
Thanks for adding this, I've been missing this function! On 2016-05-16 21:56, Walker, Benjamin wrote: > On Mon, 2016-05-16 at 16:57 +, Wiles, Keith wrote: >> The big question is how do you know the mempool is not being used someplace? > > That's the user's responsibility. Use after free is certainly possible if the > user doesn't take > care, just like any alloc/free in C. This is the same situation as > rte_ring_free or > rte_memzone_free. To help prevent users from shooting themselves in the foot > I did add a check that > all of the elements have been freed back to the pool at the top of the > function. There are certainly > potential race conditions if the user is freeing this on one thread and using > it from another that I > haven't handled. I'm not sure these cases need to be handled though - they're > not handled by > rte_ring_free, for example. Also, the user can use rte_mempool_full() to see if there are entries still allocated from it. And perhaps rte_mempool_free() should at least check if the pool is full before releasing it and warn or panic. // Simon
[dpdk-dev] [PATCH / RFC ] ethdev: Allow rte_eth_dev_configure with zero RX/TX queues
On 2016-05-16 14:43, Pattan, Reshma wrote: >>> This was added to allow devices, at least with one direction (RX/TX) >> supported. As, devices with both directions disabled doesn't make sense >> right? >> >> Well, not for running them, no. But this is a part of the shutdown procedure >> between tests (I should have been more clear I guess). > > Yes I understood this. But I am not sure if you can use > rte_eth_dev_configure(port, 0, 0) to free the resources. > Can you check if you can use rte_eth_dev_rx_queue_stop/ > rte_eth_dev_tx_queue_stop to achieve the same, because they do take care of > releasing mbufs, but doesn't free the queue's sw-ring and queue. But isn't that very strange behavior. Aren't the descriptor rings allocated in rx_queue_setup()? If so, the sequence rx_queue_stop(); // Release buffers rx_queue_start(); would leave the descriptor ring empty after start, i.e., not able to receive data. // Simon
[dpdk-dev] [PATCH / RFC ] ethdev: Allow rte_eth_dev_configure with zero RX/TX queues
On 2016-05-16 12:24, Pattan, Reshma wrote:
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index
>> a31018e..5481d45 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -944,11 +944,6 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t
>> nb_rx_q, uint16_t nb_tx_q,
>> */
>> (*dev->dev_ops->dev_infos_get)(dev, _info);
>>
>> -if (nb_rx_q == 0 && nb_tx_q == 0) {
>> -RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx
>> queue cannot be 0\n", port_id);
>> -return -EINVAL;
>> -}
>
> This was added to allow devices, at least with one direction (RX/TX)
> supported. As, devices with both directions disabled doesn't make sense
> right?
Well, not for running them, no. But this is a part of the shutdown
procedure between tests (I should have been more clear I guess).
As far as I can see in the code, rte_eth_dev_configure() is the only
point which actually calls {rx,tx}_queue_release(), so without this
call, we can't get the memory pool full again.
So basically, our test suite looks like
rte_eth_dev_configure(port, 32, 32); // For example
rte_eth_dev_configure(port, 0, 0);
Check that the mempool is full again
rte_eth_dev_configure(port, 32, 32);
rte_eth_dev_configure(port, 0, 0);
Check that the mempool is full again
...
And without this fix, the mempool check fails since a few of the buffers
are tied up in the RX descriptor ring of the PMD.
// Simon
[dpdk-dev] librte_table build race with SYMLINK-FILE?
On 2016-04-11 20:53, Thomas Monjalon wrote: > 2016-04-11 11:15, Stephen Hemminger: >> On Mon, 11 Apr 2016 12:46:16 +0200 >> Simon K?gstr?m wrote: >>> In file included from [...]lib/librte_table/rte_table_lpm.c:43:0: >>> [...]/dpdk.build/include/rte_lpm.h:484:25: fatal error: rte_lpm_sse.h: >>> No such file or directory >>> #include "rte_lpm_sse.h" >> >> The issue is a missing dependency in the mk file for LPM. > > There is already this line: > DEPDIRS-$(CONFIG_RTE_LIBRTE_TABLE) += lib/librte_lpm Sorry, this whole issue might have been caused by my old configuration being carried over to 16.04. I'll let you know if I run into it again! // Simon
[dpdk-dev] librte_table build race with SYMLINK-FILE?
Hi!
I'm upgrading from DPDK 2.1 to 16.04-rc4, and have a new build issue
which I didn't see before. It's in the librte_table and happens from
time to time (unfrequently) in my out-of-tree build. It looks like a
race between comilation and SYMLINK-FILE:
[...]
== Build lib/librte_table
CC rte_table_lpm_ipv6.o
CC rte_table_lpm.o
CC rte_table_acl.o
CC rte_table_hash_key8.o
In file included from [...]lib/librte_table/rte_table_lpm.c:43:0:
[...]/dpdk.build/include/rte_lpm.h:484:25: fatal error: rte_lpm_sse.h:
No such file or directory
#include "rte_lpm_sse.h"
^
compilation terminated.
CC rte_table_hash_key16.o
[...]
In this case, rte_lpm_sse.h is optionally symlinked if we're not on ARM.
I've tried patching away the issue by unconditionally symlinking the
_{neon,sse}.h files, and while I don't see the problem after that, I
don't really see why it would improve the situation.
Does anyone else see this as well?
// Simon
[dpdk-dev] [PATCH 1/7] lib/librte_ether: Add 2/2.5/25/50Gbps link speeds
On 2016-03-03 10:28, Thomas Monjalon wrote: > 2016-03-03 08:53, Simon K?gstr?m: >> I realize this is a more general question, but is it really meaningful >> to have macros for all possible link speeds? We're working on a PMD >> driver with a channelized interface exposed as DPDK ports. Each channel >> can be configured with an arbitrary speed, so e.g., 1337 Mbps is also >> possible. > > What is the benefit? Why not negotiate the maximum capability of the peer? Communication is channelized over a backplane, and each channel has a specific (and configurable) capacity. >> To me, it would seem more natural to just have a number in mbits for the >> link speed. > > Please jump in the thread initiated by Marc Sune months ago. OK. I haven't been following the DPDK mailing list for a while, so I wasn't aware of this. // Simon
[dpdk-dev] [PATCH 1/7] lib/librte_ether: Add 2/2.5/25/50Gbps link speeds
Hi!
On 2016-03-03 05:08, Stephen Hurd wrote:
> Add additional ETH_LINK_SPEED_* macros for 2, 2.5, 25, and 50 Gbps links
>
> Signed-off-by: Stephen Hurd
> ---
> lib/librte_ether/rte_ethdev.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 16da821..cb40bbb 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -254,10 +254,14 @@ struct rte_eth_link {
> #define ETH_LINK_SPEED_10 10 /**< 10 megabits/second. */
> #define ETH_LINK_SPEED_100 100 /**< 100 megabits/second. */
> #define ETH_LINK_SPEED_1000 1000/**< 1 gigabits/second. */
> +#define ETH_LINK_SPEED_2000 2000/**< 2 gigabits/second. */
> +#define ETH_LINK_SPEED_2500 2500/**< 2.5 gigabits/second. */
> #define ETH_LINK_SPEED_11 /**< 10 gigabits/second. */
> #define ETH_LINK_SPEED_10G 1 /**< alias of 10 gigabits/second. */
> #define ETH_LINK_SPEED_20G 2 /**< 20 gigabits/second. */
> +#define ETH_LINK_SPEED_25G 25000/**< 25 gigabits/second. */
> #define ETH_LINK_SPEED_40G 4 /**< 40 gigabits/second. */
> +#define ETH_LINK_SPEED_50G 5 /**< 50 gigabits/second. */
I realize this is a more general question, but is it really meaningful
to have macros for all possible link speeds? We're working on a PMD
driver with a channelized interface exposed as DPDK ports. Each channel
can be configured with an arbitrary speed, so e.g., 1337 Mbps is also
possible.
To me, it would seem more natural to just have a number in mbits for the
link speed.
// Simon
[dpdk-dev] [PATCH] mk: fix warning spew when EXTRA_CFLAGS specifies warning flags
On 2015-12-07 13:56, Panu Matilainen wrote: > Starting with commit 9aa2053c6e81493b23346ff4e387903560de5c81 > EXTRA_CFLAGS is sometimes being passed to the compiler without > WERROR_FLAGS which can cause spurious warnings by the dozen, > for example with when compiling with EXTRA_CFLAGS="-Wformat-security": > > cc1: warning: -Wformat-security ignored without -Wformat [-Wformat-security] > > Passing WERROR_FLAGS to AUTO_CPU helper makes the warning flag usage > consistent throughout the codebase, silencing the warnings. > [...] > diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk > index c6bb8de..28f203b 100644 > --- a/mk/rte.cpuflags.mk > +++ b/mk/rte.cpuflags.mk > @@ -33,7 +33,7 @@ > # used to set the RTE_CPUFLAG_* environment variables giving details > # of what instruction sets the target cpu supports. > > -AUTO_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(EXTRA_CFLAGS) -dM -E - < > /dev/null) > +AUTO_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(WERROR_FLAGS) > $(EXTRA_CFLAGS) -dM -E - < /dev/null) Acked-by: Simon Kagstrom
[dpdk-dev] [PATCH v2] mk: pass EXTRA_CFLAGS to AUTO_CPUFLAGS to enable local modifications
On 2015-12-07 09:33, Panu Matilainen wrote: > On 12/04/2015 08:53 PM, Thomas Monjalon wrote: > We have encountered a CPU where the AES-NI instruction set is disabled > due to export restrictions. Since the build machine and target machine > is different, using -native configs doesn't work, and on this CPU, the > application refuses to run due to the AES CPU flags being amiss. > > The patch passes EXTRA_CFLAGS to the figure-out-cpu-flags helper, > which allows us to add -mno-aes to the compile flags and resolve this > problem. > > More specifically, when EXTRA_CFLAGS contains warning flag manipulation > this patch can cause mismatch between other options that are okay > elsewhere in dpdk make. A simple fix is to pass WERROR_FLAGS to > AUTO_CPUFLAGS too to counter this, ie > > diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk > index c6bb8de..28f203b 100644 > --- a/mk/rte.cpuflags.mk > +++ b/mk/rte.cpuflags.mk > @@ -33,7 +33,7 @@ > # used to set the RTE_CPUFLAG_* environment variables giving details > # of what instruction sets the target cpu supports. > > -AUTO_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(EXTRA_CFLAGS) -dM -E > - < /dev/null) > +AUTO_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(WERROR_FLAGS) > $(EXTRA_CFLAGS) -dM -E - < /dev/null) Looks fine to me at least. // Simon
[dpdk-dev] [PATCH v2] mk: pass EXTRA_CFLAGS to AUTO_CPUFLAGS to enable local modifications
Hi Olivier! Does this commit look OK now with the changes you proposed? Thanks, // Simon On 2015-11-24 08:50, Simon Kagstrom wrote: > We have encountered a CPU where the AES-NI instruction set is disabled > due to export restrictions. Since the build machine and target machine > is different, using -native configs doesn't work, and on this CPU, the > application refuses to run due to the AES CPU flags being amiss. > > The patch passes EXTRA_CFLAGS to the figure-out-cpu-flags helper, > which allows us to add -mno-aes to the compile flags and resolve this > problem. > > Signed-off-by: Simon Kagstrom > --- > ChangeLog: > > v2: > * Put EXTRA_CFLAGS after MACHINE_CFLAGS to enable overriding values > > mk/rte.cpuflags.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk > index bec7bdd..4b30c6b 100644 > --- a/mk/rte.cpuflags.mk > +++ b/mk/rte.cpuflags.mk > @@ -33,7 +33,7 @@ > # used to set the RTE_CPUFLAG_* environment variables giving details > # of what instruction sets the target cpu supports. > > -AUTO_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) -dM -E - < /dev/null) > +AUTO_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(EXTRA_CFLAGS) -dM -E - < > /dev/null) > > # adding flags to CPUFLAGS > >
[dpdk-dev] [PATCH] rte_sched: release enqueued mbufs on rte_sched_port_free()
On 2015-11-04 19:14, Stephen Hemminger wrote: > On Wed, 28 Oct 2015 10:56:33 +0100 > Simon Kagstrom wrote: > >> Otherwise mbufs will leak when the port is destroyed. The >> rte_sched_port_qbase() and rte_sched_port_qsize() functions are used >> in free now, so move them up. >> >> Signed-off-by: Simon Kagstrom > > Overall it looks good, and fixes a long standing bug. > Maybe good to expose it as a API function rte_sched_port_flush > to allow use from applications. I'm sorry, I missed this reply! I will fix the issues you point to and repost. // Simon
[dpdk-dev] [PATCH v2] kni: Use utsrelease.h to determine Ubuntu kernel version
On 2015-11-04 19:21, Stephen Hemminger wrote: > On Wed, 4 Nov 2015 12:29:01 +0100 > Simon K?gstr?m wrote: > >> On 2015-11-04 11:35, Thomas Monjalon wrote: >>> 2015-08-20 08:51, Simon Kagstrom: -ifeq ($(shell test -f /proc/version_signature && lsb_release -si 2>/dev/null),Ubuntu) +ifeq ($(shell lsb_release -si 2>/dev/null),Ubuntu) MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(shell lsb_release -sr | tr -d .) -UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature | \ -cut -d'~' -f1 | cut -d- -f1,2 | tr .- $(comma)) +UBUNTU_KERNEL_CODE := $(shell echo `grep UTS_RELEASE $(RTE_KERNELDIR)/include/generated/utsrelease.h \ + | cut -d '"' -f2 | cut -d- -f1,2 | tr .- $(comma)`,1) MODULE_CFLAGS += -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))" endif >>> >> So lsb_release will come from the chroot, as it should, but without the >> patch, the kernel version will not come from the installed kernel >> headers in the chroot, but the running kernel - which might even not be >> Ubuntu. > > The danger here is starting to assume the build machine is the same as the > running image. Using /proc to determine runtime environment is wrong. Exactly, and our build breaks because of this without the patch. So the patch removes the check in /proc and instead takes the kernel version from the kernel headers. // Simon
[dpdk-dev] [PATCH v2] kni: Use utsrelease.h to determine Ubuntu kernel version
On 2015-11-04 11:35, Thomas Monjalon wrote: > 2015-08-20 08:51, Simon Kagstrom: >> -ifeq ($(shell test -f /proc/version_signature && lsb_release -si >> 2>/dev/null),Ubuntu) >> +ifeq ($(shell lsb_release -si 2>/dev/null),Ubuntu) >> MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(shell lsb_release -sr | tr -d .) >> -UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature | \ >> -cut -d'~' -f1 | cut -d- -f1,2 | tr .- $(comma)) >> +UBUNTU_KERNEL_CODE := $(shell echo `grep UTS_RELEASE >> $(RTE_KERNELDIR)/include/generated/utsrelease.h \ >> + | cut -d '"' -f2 | cut -d- -f1,2 | tr .- $(comma)`,1) >> MODULE_CFLAGS += >> -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))" >> endif > > Yes we must check RTE_KERNELDIR instead of the running kernel. > But it is still checking lsb_release for the running system. > It seems not consistent. I don't think so: the case the patch addresses is where the running kernel and rootfs doesn't match, like in a chroot environment. So lsb_release will come from the chroot, as it should, but without the patch, the kernel version will not come from the installed kernel headers in the chroot, but the running kernel - which might even not be Ubuntu. // Simon
[dpdk-dev] [PATCH v2] kni: Use utsrelease.h to determine Ubuntu kernel version
Ping? (Also including Stephen, Patrice and Pawel which has had comments on an earlier iteration of this patch). // Simon On 2015-08-20 08:51, Simon Kagstrom wrote: > /proc/version_signature is the version for the host machine, but in > e.g., chroots, this does not necessarily match that DPDK is built > for. DPDK will then build for the wrong kernel version - that of the > server, and not that installed in the (build) chroot. > > The patch uses utsrelease.h from the kernel sources instead and fakes > the upload version. > > Tested on a server with Ubuntu 12.04, building in a chroot for Ubuntu > 14.04. > > Signed-off-by: Simon Kagstrom > Signed-off-by: Johan Faltstrom > --- > ChangeLog: > > v2: Improve description and motivation for the patch. > > lib/librte_eal/linuxapp/kni/Makefile | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/lib/librte_eal/linuxapp/kni/Makefile > b/lib/librte_eal/linuxapp/kni/Makefile > index fb673d9..ac99d3f 100644 > --- a/lib/librte_eal/linuxapp/kni/Makefile > +++ b/lib/librte_eal/linuxapp/kni/Makefile > @@ -44,10 +44,10 @@ MODULE_CFLAGS += -I$(RTE_OUTPUT)/include > -I$(SRCDIR)/ethtool/ixgbe -I$(SRCDIR)/e > MODULE_CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h > MODULE_CFLAGS += -Wall -Werror > > -ifeq ($(shell test -f /proc/version_signature && lsb_release -si > 2>/dev/null),Ubuntu) > +ifeq ($(shell lsb_release -si 2>/dev/null),Ubuntu) > MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(shell lsb_release -sr | tr -d .) > -UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature | \ > -cut -d'~' -f1 | cut -d- -f1,2 | tr .- $(comma)) > +UBUNTU_KERNEL_CODE := $(shell echo `grep UTS_RELEASE > $(RTE_KERNELDIR)/include/generated/utsrelease.h \ > + | cut -d '"' -f2 | cut -d- -f1,2 | tr .- $(comma)`,1) > MODULE_CFLAGS += > -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))" > endif > >
[dpdk-dev] [PATCH] rte_ether: clarify rte_eth_set_queue_rate_limit tx_rate parameter
Ping? // Simon On 2015-10-20 15:20, Simon Kagstrom wrote: > The tx_rate unit is Mbps. > > Gleaned from the ixgbe implementation, the 82599 datasheet and the use > in test-pmd. > > Signed-off-by: Simon Kagstrom > --- > lib/librte_ether/rte_ethdev.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > index 8a8c82b..ff9aab7 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -3021,7 +3021,7 @@ int rte_eth_mirror_rule_reset(uint8_t port_id, > * @param queue_idx > * The queue id. > * @param tx_rate > - * The tx rate allocated from the total link speed for this queue. > + * The tx rate in Mbps. Allocated from the total port link speed. > * @return > * - (0) if successful. > * - (-ENOTSUP) if hardware doesn't support this feature. >
[dpdk-dev] [PATCH] rte_sched: release enqueued mbufs on rte_sched_port_free()
Ping?
(CC:ing Stephen Hemminger as well)
// Simon
On 2015-10-28 10:56, Simon Kagstrom wrote:
> Otherwise mbufs will leak when the port is destroyed. The
> rte_sched_port_qbase() and rte_sched_port_qsize() functions are used
> in free now, so move them up.
>
> Signed-off-by: Simon Kagstrom
> ---
> lib/librte_sched/rte_sched.c | 44
> +++-
> 1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 9c9419d..81462cd 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -312,6 +312,23 @@ rte_sched_port_queues_per_port(struct rte_sched_port
> *port)
> return RTE_SCHED_QUEUES_PER_PIPE * port->n_pipes_per_subport *
> port->n_subports_per_port;
> }
>
> +static inline struct rte_mbuf **
> +rte_sched_port_qbase(struct rte_sched_port *port, uint32_t qindex)
> +{
> + uint32_t pindex = qindex >> 4;
> + uint32_t qpos = qindex & 0xF;
> +
> + return (port->queue_array + pindex * port->qsize_sum +
> port->qsize_add[qpos]);
> +}
> +
> +static inline uint16_t
> +rte_sched_port_qsize(struct rte_sched_port *port, uint32_t qindex)
> +{
> + uint32_t tc = (qindex >> 2) & 0x3;
> +
> + return port->qsize[tc];
> +}
> +
> static int
> rte_sched_port_check_params(struct rte_sched_port_params *params)
> {
> @@ -717,11 +734,21 @@ rte_sched_port_config(struct rte_sched_port_params
> *params)
> void
> rte_sched_port_free(struct rte_sched_port *port)
> {
> + unsigned int queue;
> /* Check user parameters */
> if (port == NULL){
> return;
> }
>
> + /* Free enqueued mbufs */
> + for (queue = 0; queue < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; queue++) {
> + unsigned int i;
> + struct rte_mbuf **mbufs = rte_sched_port_qbase(port, queue);
> +
> + for (i = 0; i < rte_sched_port_qsize(port, queue); i++)
> + rte_pktmbuf_free(mbufs[i]);
> + }
> +
> rte_bitmap_free(port->bmp);
> rte_free(port);
> }
> @@ -1032,23 +1059,6 @@ rte_sched_port_qindex(struct rte_sched_port *port,
> uint32_t subport, uint32_t pi
> return result;
> }
>
> -static inline struct rte_mbuf **
> -rte_sched_port_qbase(struct rte_sched_port *port, uint32_t qindex)
> -{
> - uint32_t pindex = qindex >> 4;
> - uint32_t qpos = qindex & 0xF;
> -
> - return (port->queue_array + pindex * port->qsize_sum +
> port->qsize_add[qpos]);
> -}
> -
> -static inline uint16_t
> -rte_sched_port_qsize(struct rte_sched_port *port, uint32_t qindex)
> -{
> - uint32_t tc = (qindex >> 2) & 0x3;
> -
> - return port->qsize[tc];
> -}
> -
> #if RTE_SCHED_DEBUG
>
> static inline int
>
[dpdk-dev] dpdk pre-patch testing introduction
Hi Liu! I think this is a really, really great addition! On 2015-10-29 16:19, Liu, Yong wrote: > Currently the patch testing only run unit test and basic function test on > this platform. > It can make sure that new patch doesn't break original code and functions. > Due to coverage limitation, it can't verify the functionality of new patch. > If you want to verify your patch's new function, you need to check with > tester and perform specific testing on it. Speaking of coverage, I do have a few suggestions: 1. Build and test both with and without optimizations (build with EXTRA_CFLAGS="-O0"). While uncommon, I've seen code break in the non-optimized case because the compiler simply removes some unused code which would otherwise cause link errors. 2. I think it would be very good to collect code coverage for patches as well. (Yes, this is pushing my own tool) I've written a code coverage tool called kcov [1], which only needs debugging information (-g) in the binary to collect code coverage. It outputs results in various formats: lcov-style HTML, cobertura XML and can post directly to coveralls.io. It's also simple to run: kcov --include-pattern=dpdk /tmp/kcov-output ./testpmd [args...] To the mail report, I think it would be good to add information about code coverage increase/decrease with the patch and ideally which lines of the patch which were covered Coverage: 69.2% (+5 lines with the patch) Coverage for the patch: 5/7 lines both of these should be fairly simple to parse out from the XML output. I'd be happy to help you if think it's a good idea! // Simon [1] https://github.com/SimonKagstrom/kcov
[dpdk-dev] Unit for tx_rate in rte_eth_set_queue_rate_limit?
Hi! What is the unit of the tx_rate parameter to the rte_eth_set_queue_rate_limit function? It's documented as /** * Set the rate limitation for a queue on an Ethernet device. * * @param port_id * The port identifier of the Ethernet device. * @param queue_idx * The queue id. * @param tx_rate * The tx rate allocated from the total link speed for this queue. * @return * - (0) if successful. * - (-ENOTSUP) if hardware doesn't support this feature. * - (-ENODEV) if *port_id* invalid. * - (-EINVAL) if bad parameter. */ int rte_eth_set_queue_rate_limit(uint8_t port_id, uint16_t queue_idx, uint16_t tx_rate); I parse this as meaning a percentage of total link speed, i.e., on a 10 Gbps link, 50 would mean 5Gbps, 10 means 1Gbps etc. Is this correct? // Simon
[dpdk-dev] DPDK User Space: Session onUseability and Ease of Use
On 2015-10-13 18:36, Mcnamara, John wrote: > > * More sample apps > > - Some more examples of using secondary processes. One thing I would appreciate is more basic samples for the more advanced functionality. For example, the qos_sched example is (in my opimion) quite hard to follow since it has both an integrated command line and a config file. I think it would be better to split the example to multiple smaller samples, which uses static setups to more clearly illustrate some particular functionality of the QoS framework. // Simon
[dpdk-dev] Unsafe array accesses in rte_sched.c
On 2015-10-16 15:39, Dumitrescu, Cristian wrote: >> port->qsize_add[12] = port->qsize_add[11] + port->qsize[2]; >> port->qsize_add[13] = port->qsize_add[12] + port->qsize[3]; >> port->qsize_add[14] = port->qsize_add[13] + port->qsize[3]; >> port->qsize_add[15] = port->qsize_add[14] + port->qsize[3]; >> >> port->qsize_sum = port->qsize_add[15] + port->qsize[3]; >> } >> >> but port->qsize is actually defined as >> >> uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE]; >> > > Not sure what you see "unsafe" here: qsize is an array of 4 elements, while > qsize_add is a different array of 16 elements? Please explain. Sorry, I should have been more explicit: What I mean that the code should loop over RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE instead of hard-coding the numbers. It certainly works with the current RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE, but it would be safer (and in my opinion more clear) if it would not assume RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE == 4. // Simon
[dpdk-dev] Unsafe array accesses in rte_sched.c
Hi!
I'm investigating DPDK support for pacing output streams and trying to
understand the QoS framework. However, I quickly found some instances of
unsafe array accesses. E.g., the rte_sched_port_config_qsize function
looks like this:
static void
rte_sched_port_config_qsize(struct rte_sched_port *port)
{
/* TC 0 */
port->qsize_add[0] = 0;
port->qsize_add[1] = port->qsize_add[0] + port->qsize[0];
port->qsize_add[2] = port->qsize_add[1] + port->qsize[0];
port->qsize_add[3] = port->qsize_add[2] + port->qsize[0];
[...]
/* TC 3 */
port->qsize_add[12] = port->qsize_add[11] + port->qsize[2];
port->qsize_add[13] = port->qsize_add[12] + port->qsize[3];
port->qsize_add[14] = port->qsize_add[13] + port->qsize[3];
port->qsize_add[15] = port->qsize_add[14] + port->qsize[3];
port->qsize_sum = port->qsize_add[15] + port->qsize[3];
}
but port->qsize is actually defined as
uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
There are similar problems in rte_sched_port_log_pipe_profile() and
probably other places.
I don't understand the code well enough to send patches for these,
although the fixes should be fairly trivial. Perhaps this is already
known as it should be fairly easy to trigger with static checkers?
// Simon
[dpdk-dev] [PATCH] mk: Quote $(KERNELCC) to allow ccache builds
On 2015-10-13 14:45, Thomas Monjalon wrote: > 2015-10-13 14:39, Simon K?gstr?m: >> Two of the patches (this one included) I have outstanding are build fixes >> for use in our build environment, so it would be nice to them upstreamed. > > Waiting for integration of your patches, maybe you have some free time to > help other developers by making reviews ;) Waiting for integration is not my only work-task :-) Anyway, I have too superficial knowledge about DPDK to chime in with relevant comments in most cases, but I'll comment if I feel I can contribute. // Simon
[dpdk-dev] [PATCH] mk: Quote $(KERNELCC) to allow ccache builds
On 2015-10-13 14:26, Olivier MATZ wrote: > Sorry for not having answered before. Thanks for looking at it now though! >> This is one of three outstanding DPDK patches I have which hasn't seen >> any activitiy in a while. Is there a list of pending applies somewhere >> to monitor activity? > > There is the patchwork tool: > http://dpdk.org/dev/patchwork/project/dpdk/list/ Thanks! I knew about it, but forgot to look there. It would be nice to have tags to signify e.g., for-v2.2, candidate-v2.2 etc. like you can have on github to easier see where patches are going, but perhaps patchwork doesn't work that way. Is the process to ping old patches like this on the mailing list? Two of the patches (this one included) I have outstanding are build fixes for use in our build environment, so it would be nice to them upstreamed. // Simon
[dpdk-dev] DPDK.org Community Call - Sept 24 - Discuss Growth, Improvements
On 2015-09-24 10:36, Thomas Monjalon wrote: > 2015-09-23 23:56, St Leger, Jim: >> This call is aimed to get more open dialogue in the community. > > I wonder how a call can "get more open dialogue"? > Because of being "live", a lot of people cannot attend at this time. > A call is also a place where only people having the strongest voice will be > heard. > Finally it will be really well understood only by english-native people > (depending on the accent of speakers). > > I know only one way to be really open and we are using it for this discussion. One good thing would be if someone could write a summary of the community call discussion for further discussion on the mailing list. // Simon
[dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code
On 2015-09-08 01:21, Ananyev, Konstantin wrote: >> >> Thanks. I got it wrong anyway, what I wanted was to be able to handle >> the day when nb_segs changes to a 16-bit number, but then it should >> really be >> >> ... >= 1 << (sizeof(head->nb_segs) * 8) >> >> anyway. I'll fix that and also add a warning that the implementation >> will do a linear search to find the tail entry. > > Probably just me, but I can't foresee the situation when we would need to > increase nb_segs to 16 bits. > Looks like an overkill to me. I don't think it will happen either, but with this solution, this particular piece of code will work regardless. The value is known at compile-time anyway, so it should not be a performance issue. // Simon
[dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code
On 2015-09-07 14:32, Ananyev, Konstantin wrote:
>> +static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf
>> *tail)
>> +{
>> +struct rte_mbuf *cur_tail;
>> +
>> +/* Check for number-of-segments-overflow */
>> +if (head->nb_segs + tail->nb_segs >= sizeof(head->nb_segs) << 8)
>> +return -EOVERFLOW;
>
> Would probably be better 'sizeof(head->nb_segs) << CHAR_BIT', or even just: '
> > UINT8_MAX'.
> Konstantin
Thanks. I got it wrong anyway, what I wanted was to be able to handle
the day when nb_segs changes to a 16-bit number, but then it should
really be
... >= 1 << (sizeof(head->nb_segs) * 8)
anyway. I'll fix that and also add a warning that the implementation
will do a linear search to find the tail entry.
// Simon
[dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code
On 2015-09-07 11:35, Olivier MATZ wrote: >> Wonder why do we need to do that? >> Probably head mbuf is out of space and want to expand it using >> pktmbuf_chain()? >> So in that case seems logical: >> 1) allocate new mbuf (it's pkt_len will be 0) >> b) call pktmbuf_chain() > > By experience, having empty segments in the middle of a mbuf > chain is problematic (functions getting ptr at offsets, some pmds > or hardware may behave badly), I wanted to avoid that risk. > > Now, the use-case you described is legitimate. Another option would > be to have another function pktmbuf_append_new(m) that returns a new > mbuf that is already chained to the other. I see with that method in that you have to remember to actually update pkt_len in the head buffer when chaining an empty mbuf. Anyway, to disallow this behavior should probably not be the responsibility of rte_pktmbuf_chain(), so I'm fine with leaving the check out. // Simon
[dpdk-dev] [PATCH] mem: Warn once if /proc/self/pagemap is unreadable
On 2015-06-23 12:39, Simon Kagstrom wrote: > Newer kernels make this unreadable for security reasons for non-roots. > Running the application will then fill the logs with > > rte_mem_virt2phy: cannot open /proc/self/pagemap > > messages. I found a bug with the patch, so please disregard it for a while. Sorry about that! // Simon
[dpdk-dev] [PATCH] mk: add support for gdb debug info generation
On 2015-06-23 09:47, Thomas Monjalon wrote: > 2015-06-23 08:39, Gonzalez Monroy, Sergio: >> I guess you could argue that, to always build with debug info then strip >> it down. >> You would need another flag to strip debug info for production, or leave >> it for debugging. >> >> In my opinion is not worth it, but it you feel strongly about it you can >> submit patches and >> let the community decide. > > I think stripping is a packaging responsibility. > It would be a good idea to always provide debugging symbols. Yes, I think this would be the best way too, and should be pretty much standard procedure. DPDK is anyhow just a library - stripping should be up to the application / packaging to do. // Simon
[dpdk-dev] [PATCH] mk: add support for gdb debug info generation
On 2015-06-22 09:44, Gonzalez Monroy, Sergio wrote: > On 19/06/2015 22:29, Cyril Chemparathy wrote: >> From: Cyril Chemparathy >> >> It is often useful to build with debug enabled, we add a config >> (CONFIG_RTE_TOOLCHAIN_DEBUG) to do so. >> >> +ifeq ($(CONFIG_RTE_TOOLCHAIN_DEBUG),y) >> +TOOLCHAIN_CFLAGS += -g -ggdb >> +TOOLCHAIN_LDFLAGS += -g -ggdb >> +endif > I don't think you need to modify the makefiles and introduce a new > compile time option for this. > The same result can be easily achieved by setting EXTRA_CFLAGS in the > command line. ie: > $ make install T=x86_64-native-linuxapp-gcc EXTRA_CFLAGS='-g -ggdb' Why isn't -g standard though? The binaries should/will anyhow be stripped when used for production - but debugging information should be useful when analysing crashes. // Simon
[dpdk-dev] [RFC PATCH] eal:Add new API for parsing args at rte_eal_init time
On 2015-06-04 23:27, Chilikin, Andrey wrote:
>> Its not a bad addition, I'm just not sure its worth having to take on the
>> additional API surface to include. I'd be more supportive if you could
>> enhance
>> the function to allow the previously mentioned before/after flexibiilty.
>> Then we
>> could just deprecate rte_eal_init as an API call entirely, and use this
>> instead.
>
> Before/after would be very useful, a lot of applications use only "-c" and
> "-n" EAL command line parameters and "-c" in many cases is redundant as
> application can calculate core mask from its own parameters, and "-n" just a
> required parameter which can be defaulted to a platform specific value. So in
> addition to rte_set_application_usage_hook() it would be nice to have some
> more general way of overwriting eal initialization parameters.
I've always found it a bit strange that DPDK forces argv handling this
way. The application will anyway have to setup system-specific stuff
(buffer count etc) for the ports to use, so special-casing memory and
core setup seems strange. I think it would be more logical to have EAL
configuration from a structure like for the ports:
struct dpdk_conf conf =
{
.core_mask = 0x7,
.huge_pages = 1,
[...]
};
rte_eal_init();
And make the current argv parser optional, i.e., something like
int main(argc, argv)
{
struct dpdk_conf conf;
ret = rte_eal_parse_argv();
rte_eal_init();
argc -= ret; ...
}
// Simon
[dpdk-dev] [PATCH / RFC] kni: Add set_rx_mode callback to handle multicast groups
On 2015-06-02 05:44, Zhang, Helin wrote: >> -Original Message- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Simon Kagstrom >> Sent: Thursday, May 7, 2015 9:18 PM >> To: dev at dpdk.org >> Subject: [dpdk-dev] [PATCH / RFC] kni: Add set_rx_mode callback to handle >> multicast groups >> >> This is needed to add / remove interfaces in multicast groups via the ip >> tool. > Could you help to explain with more details of why it is needed? We did some (very basic) tests with IGMP, which involves adding multicast addresses to ETH interfaces. This is done via the ip tool, an example can be found on e.g., http://superuser.com/questions/324824/linux-built-in-or-open-source-program-to-join-multicast-group and this will fail on KNI interfaces with the current code because of an unimplemented ioctl (as Stephen Hemminger said earlier). The patch simply adds an empty callback so that the ioctl succeeds, and this is the same thing as the Linux tap interface does (so I think it should be enough for KNI as well). If you want, I can update the patch with a bit more description (something like the above). // Simon
[dpdk-dev] [PATCH] kni: Use utsrelease.h to determine Ubuntu kernel version
On 2015-05-28 12:48, Wodkowski, PawelX wrote: >>> Please only check if UTS_RELEASE is available on all Ubuntu versions DPDK >> support. >> >> From some digging, it appears it entered the kernel tree in 2006 and >> moved to include/generated/ in 2009 so I guess that should be fine for >> DPDK builds? > > I also think that it is OK but I also think should check by building you (o > ask > someone to do it for you) on those systems not by theory :) Well, I think this is one of the main motivations from something like what Thomas F. Herbert proposed in another thread recently, DPDK: Proposal for a patch patch-test integration tree basically, a continuous-integration-type of system should test-build (and probably test) any prospective patch to see that it builds for various targets. In my view, this would be a perfect match for github+travis-ci. Anyway, I'll see if I can dig up an older Ubuntu to build on, unless someone else steps up and tests the patch. (My issue to start with was that the build fails on a 14.04 chroot on a 12.04 host, but I only have access to the chroot there). // Simon
[dpdk-dev] [PATCH] kni: Use utsrelease.h to determine Ubuntu kernel version
On 2015-05-28 12:05, Wodkowski, PawelX wrote: >>> >>> -UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature | \ >>> -cut -d'~' -f1 | cut -d- -f1,2 | tr .- $(comma)) >>> +UBUNTU_KERNEL_CODE := $(shell echo `grep UTS_RELEASE >>> $(RTE_KERNELDIR)/include/generated/utsrelease.h \ >>> +| cut -d '"' -f2 | cut -d- -f1,2 | tr .- $(comma)`,1) > > It is fine for me if it do the job and does not break build on other OS (also > other > Ubuntu versions especially 12.04 if we still support it). > Please only check if UTS_RELEASE is available on all Ubuntu versions DPDK > support. >From some digging, it appears it entered the kernel tree in 2006 and moved to include/generated/ in 2009 so I guess that should be fine for DPDK builds? // Simon
[dpdk-dev] [PATCH / RFC] kni: Add set_rx_mode callback to handle multicast groups
Stephen, Helin, perhaps you have some comment about this patch?
// Simon
On 2015-05-07 15:17, Simon Kagstrom wrote:
> This is needed to add / remove interfaces in multicast groups via the
> ip tool.
>
> The callback does nothing - the same as the kernel tun.c.
>
> Signed-off-by: Simon Kagstrom
> ---
> Marked RFC since I'm by no means an expert on this. We noticed this
> when playing with KNI and IGMP handling.
>
> lib/librte_eal/linuxapp/kni/kni_net.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c
> b/lib/librte_eal/linuxapp/kni/kni_net.c index dd95db5..cf93c4b 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_net.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_net.c
> @@ -495,6 +495,11 @@ kni_net_ioctl(struct net_device *dev, struct ifreq
> *rq, int cmd) return 0;
> }
>
> +static void
> +kni_net_set_rx_mode(struct net_device *dev)
> +{
> +}
> +
> static int
> kni_net_change_mtu(struct net_device *dev, int new_mtu)
> {
> @@ -645,6 +650,7 @@ static const struct net_device_ops
> kni_net_netdev_ops = { .ndo_start_xmit = kni_net_tx,
> .ndo_change_mtu = kni_net_change_mtu,
> .ndo_do_ioctl = kni_net_ioctl,
> + .ndo_set_rx_mode = kni_net_set_rx_mode,
> .ndo_get_stats = kni_net_stats,
> .ndo_tx_timeout = kni_net_tx_timeout,
> .ndo_set_mac_address = kni_net_set_mac,
>
[dpdk-dev] [PATCH] rte_reorder: Allow sequence numbers > 0 as starting point
Thanks for the review, Sergio!
On 2015-05-28 09:49, Gonzalez Monroy, Sergio wrote:
>> @@ -325,6 +327,12 @@ rte_reorder_insert(struct rte_reorder_buffer *b,
>> struct rte_mbuf *mbuf)
>> uint32_t offset, position;
>> struct cir_buffer *order_buf = >order_buf;
>> +if (!b->is_initialized) {
>> +b->min_seqn = mbuf->seqn;
>> +
>> +b->is_initialized = 1;
>> +}
>> +
>> /*
>>* calculate the offset from the head pointer we need to go.
>>* The subtraction takes care of the sequence number wrapping.
> So my first impression was, why do this in insert instead of init?
> I guess the goal was trying to avoid changing the API, but would it not
> be worth it? after all is a one time thing only.
We don't know the first sequence number until the first insert, so I
think it has to be there. Alternatively, there could be an API to set
the minimum sequence number, but I think that would instead make the
application uglier, and isn't that also just exposing library
implementation details in the API?
> About the implementation, packets being inserted could be out of order,
> so the first packet inserted may not be the first in your sequence. Now
> what happens with that packet would be app specific so probably is not a
> big deal but what about initializing min_seqn to something like
> (mbuf->seqn - b->size/2) ? That would give enough room for packets out
> of order.
I thought about that, but you will always miss some packets if you have
an active stream at start anyway, so in the end I removed that part.
But perhaps you are right about this issue, I'm not sure.
> You should also update the documentation regarding rte_reorder_insert.
Actually, the rte_reorder.h file says nothing about the (current)
limitation of the first seq number having to be 0, so I think this patch
actually improves the documentation without touching it :-)
// Simon
[dpdk-dev] DPDK: Proposal for a patch patch-test integration tree
I spot a can of worms to be opened here :-) On 2015-05-27 22:48, Thomas F Herbert wrote: > Work Flow and Process: > > All patches will be taken from from public submissions to dpdk-dev.org > scraped from dpdk patchwork. Patches will be applied to the patch-test > tree and tested against HEAD as they are received. The feedback from the > testing will be provided to the community. The patch-test tree will > periodically be git pull'ed from dpdk. > > Longer term goal: > > Initially, the patches will be applied along with some simple smoke > tests. The longer term goal is to automate this process, apply more > extensive tests and post the results in dpdk patchwork, > http://dpdk.org/dev/patchwork/project/dpdk/list/ which would have an > accompanying mailing list for distribution of a results summary of the > tests. Actually, github and services such as travis-ci and coveralls already provide this functionality (with very little setup). So when someone sends a pull request, the continuous integration service travis-ci will notice it, and start a build and (possibly) run a test suite on the code - with the patches applied. If code coverage is collected in the process [1], it's uploaded to the coveralls site. Both travis-ci and coveralls will add a note to the pull request saying something like "Build failed with this patch, be careful" or "Build OK, everything is fine" and "Coverage decreased with 5% with this patch" etc etc. Of course, github provides an API so it's entirely possible to add your own continuous integration support with the same functionality as travis-ci (customized for DPDK). So before venturing into implementing something like this, I think the DPDK project should at least consider the existing alternatives. And with that, I close the can of worms again. I hope no worms were hurt in the process! :-) // Simon [1] https://github.com/SimonKagstrom/kcov - yes my personal project
[dpdk-dev] [PATCH] eal_common_options: Allow combining -m and --no-huge
Hi John and David! On 2015-05-27 09:29, Mcnamara, John wrote:> > Minor comments. > > * Without mem_parsed the () aren't required and the conditional will fit on one line. > > * The section prefix on the first/subject line should be "eal:" and the commit message/justification could be clearer. It would be worth doing a "git log" on the file and following the previous conventions for commit message on that component/file. OK, I'll fix these issues. On 2015-05-27 09:43, David Marchand wrote: > Well, I asked some question last time : > http://dpdk.org/ml/archives/dev/2015-March/015867.html > > This patch looks good but since I don't use this --no-huge that often, I > would like to know how you tested this. I used the pcap PMD driver with file input, basically ./test-dpdk --no-huge -m 1024 -l 0,1 -n3 --vdev 'eth_pcap0,rx_pcap=/tmp/eth-out.pcap,tx_pcap=/tmp/eth0.pcap' this patch allows me to run this as myself (i.e., not root) > And please, when resending a patch, it should be marked as vX. Sorry about that, I stashed it and forgot about it for a while, and then just re-made the commit. (Not only that: I lost the reply in the flood of DPDK mails, so sort of what I was complaining about in the first place!) Anyway, I'll update the patch and resend. // Simon
[dpdk-dev] Small request for patch review in DPDK
Hi! As a well-meaning outsider submitting small patches to DPDK, I'd very much appreciate help from the community with review and comments. Since I'm a DPDK newbie, the patches might be incorrect or wrong in some other way, so I think it's even more important with reviews for people like me. But I can't help getting this nagging feeling that the patches are actually lost in the flood of mail to the DPDK mailing list, since I haven't gotten any reaction to any of the three patches I've sent the last month. The patches in question are kni: Add set_rx_mode callback to handle multicast groups rte_reorder: Allow sequence numbers > 0 as starting point eal_common_options: Allow combining -m and --no-huge so if someone feels like mining their DPDK mail box, please review if you have the time and knowledge about the area :-) And I can't help thinking that maybe github pull requests would be harder to miss (or at least easier to find after a few weeks of other traffic). // Simon
[dpdk-dev] [PATCH / RFC] kni: Add set_rx_mode callback to handle multicast groups
Ping?
// Simon
On 2015-05-07 15:17, Simon Kagstrom wrote:
> This is needed to add / remove interfaces in multicast groups via the
> ip tool.
>
> The callback does nothing - the same as the kernel tun.c.
>
> Signed-off-by: Simon Kagstrom
> ---
> Marked RFC since I'm by no means an expert on this. We noticed this
> when playing with KNI and IGMP handling.
>
> lib/librte_eal/linuxapp/kni/kni_net.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c
> b/lib/librte_eal/linuxapp/kni/kni_net.c index dd95db5..cf93c4b 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_net.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_net.c
> @@ -495,6 +495,11 @@ kni_net_ioctl(struct net_device *dev, struct ifreq
> *rq, int cmd) return 0;
> }
>
> +static void
> +kni_net_set_rx_mode(struct net_device *dev)
> +{
> +}
> +
> static int
> kni_net_change_mtu(struct net_device *dev, int new_mtu)
> {
> @@ -645,6 +650,7 @@ static const struct net_device_ops
> kni_net_netdev_ops = { .ndo_start_xmit = kni_net_tx,
> .ndo_change_mtu = kni_net_change_mtu,
> .ndo_do_ioctl = kni_net_ioctl,
> + .ndo_set_rx_mode = kni_net_set_rx_mode,
> .ndo_get_stats = kni_net_stats,
> .ndo_tx_timeout = kni_net_tx_timeout,
> .ndo_set_mac_address = kni_net_set_mac,
>
[dpdk-dev] GitHub sandbox for the DPDK community
On 2015-05-06 10:12, Panu Matilainen wrote:
> On 05/05/2015 07:43 PM, Wiles, Keith wrote:
>
>> GitHub offers a different set of processes and
>> tools, which we do not have to create. Moving to GitHub is a change
>> for the community and I feel a good change for the better.
>
> Like quite a few others in this thread, I dont care if the git repo
> moved to the end of internet as long as email continues to be a
> first-class means for patch submissions, reviews and other
> communication. It doesn't have to be the only way as clearly many people
> prefer otherwise.
Perhaps something like pull-request-mailer could be used to tend to both
camps? I.e., sending out github pull requests to the mailing list for
review:
https://github.com/google/pull-request-mailer
Anyway, for me personally (as a DPDK outsider), what I feel would be the
main improvement with using github would be that they have a very
well-integrated bug reporting system that keeps track of e.g., the
commit that fixes the bug etc.
I recently submitted a build issue to the mailing list, which Olivier
Matz promptly fixed with a patch (but which haven't been merged as far
as I can tell). In the gihub workflow, I'd submitted a bug report
("Issue #13" for example), Olivier would have fixed this through a
merge-request ("Issue #13: scripts: fix relpath.sh output when build dir
is a symlink") and I'd acked that fix in the bug report. When the merge
request was merged to the git repo, the bug report would be closed.
I'm also interested in the architecture discussions etc (or the github
debate!) on the list, but I really don't read patches sent to the list.
So if I had a vote (which I shouldn't have :-)), I'd vote for a gradual
move to github and a mailing list split.
// Simon
[dpdk-dev] [PATCH] scripts: fix relpath.sh output when build dir is a symlink
On 2015-05-05 11:00, Olivier MATZ wrote: > On 05/05/2015 11:00 AM, Olivier Matz wrote: >> The script relpath.sh return the relative path of the first directory >> from the second directory. It is used to generate relative symlinks, >> which can be useful if the build directory is embedded in the dpdk >> directory: the whole dpdk can be moved without breaking the links, >> which is helpful for an installation. >> >> In case the build directory is a symlink, the script was not generating >> the proper relative path. Fix this by calling "readlink -f" on the >> arguments. > > Can you have a try with this patch? Yes, this fixes my build issue, thanks! Verified-by: Simon Kagstrom // Simon
[dpdk-dev] build issue with out-of-tree builds and multiple mounts
On 2015-05-04 14:48, Olivier MATZ wrote: > Hi Simon, > > On 05/04/2015 02:42 PM, Simon K?gstr?m wrote: >> Hi! >> >> I'm trying to do a out-of-tree build of DPDK 2.0.0 (with make -C and >> O=), but failing with errors such as >> >> In file included from >> [...]/lib/librte_eal/common/include/rte_eal_memconfig.h:40:0, >> [...] >> rte_malloc_heap.h:39:26: fatal error: rte_spinlock.h: No such file or >> directory >> >> Looking in the include/ directory in my build directory, I see a lot of >> invalid symlinks to things like rte_spinlock.h: >> >> lrwxrwxrwx 1 ska users 99 May 4 14:33 rte_spinlock.h -> >> ../[...]/dpdk/lib/librte_eal/common/include/arch/x86/rte_spinlock.h > > Can you please send the full make command line that produces this > issue? make -C /home/ska/devel/dpdk EXTRA_CFLAGS="-g -O0" RTE_KERNELDIR=/tmp/ubuntu-14.04/usr/src/linux-headers-3.13.0-51-generic O=/home/ska/build/dpdk T=x86_64-default-linuxapp-gcc V=1 The config step works fine, but it fails on this step. /home/ska is mounted on NFS, and /home/ska/build is a symlink to /lhome/ska/build It appears to work if I build outside the symlink, i.e.., build with O=/lhome/ska/build/... instead. // Simon
[dpdk-dev] build issue with out-of-tree builds and multiple mounts
Hi! I'm trying to do a out-of-tree build of DPDK 2.0.0 (with make -C and O=), but failing with errors such as In file included from [...]/lib/librte_eal/common/include/rte_eal_memconfig.h:40:0, [...] rte_malloc_heap.h:39:26: fatal error: rte_spinlock.h: No such file or directory Looking in the include/ directory in my build directory, I see a lot of invalid symlinks to things like rte_spinlock.h: lrwxrwxrwx 1 ska users 99 May 4 14:33 rte_spinlock.h -> ../[...]/dpdk/lib/librte_eal/common/include/arch/x86/rte_spinlock.h and I believe that this is caused by my use of multiple mounts: I have my home directory and the source code on NFS, while the build directory is on local disk (under a build/-symlink from my home directory). So using ../ relative to the build directory will actually end up on the local disk, and not work. I guess it should be possible to workaround this by building somewhere else, but perhaps there is some patch to be made in mk/? Any pointer to where to look? // Simon
[dpdk-dev] tools brainstorming
On 2015-03-23 17:29, Thomas Monjalon wrote: > 2015-03-20 16:18, Simon K?gstr?m: >>> - make autotests easier and faster to run for smoke testing >>> - automated basic testpmd check >> >> Code coverage for automated tests can be useful as well. >> >> In a way I'm speaking in my own interests here since I've written a tool >> to do just this (and produce nice HTML etc output), kcov, that can be >> found at github (https://github.com/SimonKagstrom/kcov). > > Feel free to do some DPDK integration for your kcov tool. > It could definitely help to write good tests. Well, there's not that much integration needed for kcov. It needs debugging information in the binary, so something like the patch below is needed (and the same for clang, not sure about icc). Apart from that, I just ran it as kcov --exclude-path=/usr /tmp/kcov-dpdk ./basicfwd --no-huge -c 1 -n 1 to collect coverage info and generate a report. The report for this particular case can be seen here: http://www.c64-network.org/kcov-dpdk/index.html I'm not sure how your automatic test suite works, but just running it like you do today but through kcov like above should do the trick. // Simon diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk index 88f235c..d3cd9e5 100644 --- a/mk/toolchain/gcc/rte.vars.mk +++ b/mk/toolchain/gcc/rte.vars.mk @@ -60,7 +60,7 @@ endif HOSTAS= as TOOLCHAIN_ASFLAGS = -TOOLCHAIN_CFLAGS = +TOOLCHAIN_CFLAGS = -g TOOLCHAIN_LDFLAGS = ifeq ($(CONFIG_RTE_LIBRTE_GCOV),y)
[dpdk-dev] tools brainstorming
On 2015-03-20 15:51, Thomas Monjalon wrote: > As we are lazy developers, writing guidelines is not enough. It must be > coupled with the integration of some tools. Let's work on these ones: > - make autotests easier and faster to run for smoke testing > - automated basic testpmd check > - build check with various options combinations > - abi check (started with validate-abi.sh) > - static analyze (clang, free online coverity) > - comment check (doxygen, codespell, kerspell) > - format check (customized checkpatch) Code coverage for automated tests can be useful as well. In a way I'm speaking in my own interests here since I've written a tool to do just this (and produce nice HTML etc output), kcov, that can be found at github (https://github.com/SimonKagstrom/kcov). // Simon
[dpdk-dev] [PATCH] headers: typeof -> __typeof__ to unbreak C++11 code
On 2015-03-06 11:30, Ananyev, Konstantin wrote: > > Hi Simon, > >> -Original Message- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Simon K?gstr?m >> Sent: Friday, March 06, 2015 8:28 AM >> To: dev at dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] headers: typeof -> __typeof__ to unbreak >> C++11 code >> >> Ping? Konstantins simpler patch is fine for me, but at least one of >> these two would be very nice to have so that modern C++ code can use DPDK. >> > Can you do v2 as suggested, or do you want me to do it? You wrote that patch (which looks better than mine), so feel free to submit it! Thanks, // Simon
[dpdk-dev] [PATCH] headers: typeof -> __typeof__ to unbreak C++11 code
Ping? Konstantins simpler patch is fine for me, but at least one of
these two would be very nice to have so that modern C++ code can use DPDK.
// Simon
On 2015-03-02 08:55, Simon K?gstr?m wrote:
> On 2015-02-27 17:24, Ananyev, Konstantin wrote:
>> Actually, I wonder wouldn't something like the one below be sufficient?
>>
>> diff --git a/lib/librte_eal/common/include/rte_common.h
>> b/lib/librte_eal/common/
>> index 8ac940c..1867692 100644
>> --- a/lib/librte_eal/common/include/rte_common.h
>> +++ b/lib/librte_eal/common/include/rte_common.h
>> @@ -51,6 +51,15 @@ extern "C" {
>> #include
>> #include
>>
>> +#ifndef typeof
>> +#define typeof __typeof__
>> +#endif
>> +
>> +#ifndef asm
>> +#define asm __asm__
>> +#endif
>
> Yes, I've tested, and this works with g++ as well.
>
> // Simon
>
