* Re: [PATCH 1/1] eal: Don't fail secondary if primary is missing tailqs
From: Jean Tourrilhes @ 2016-10-05 16:49 UTC (permalink / raw)
To: David Marchand; +Cc: Sergio Gonzalez Monroy, dev@dpdk.org
In-Reply-To: <CALwxeUtkPsQop3zr4RnS7Y8bwKzar6DaRxCPWCX_trgc4kMm_w@mail.gmail.com>
On Wed, Oct 05, 2016 at 09:58:01AM +0200, David Marchand wrote:
> Hello,
Hi there,
> I thought you had unaligned binaries.
> You are compiling only one binary ?
Primary is compiled using the DPDK build process.
Secondary is build using the Snort build process.
Both are pointing to the exact same libdpdk.a.
> I am not sure Sergio is talking about the constructor approach.
But, this is exactly the cause of the problem.
> Anyway, the constructors invocation order should not matter.
For tailq, I agree. For mempool constructors, order do matter.
> Primary and secondary processes build their local tailq entries list
> in constructors (so far, I can't see how this is wrong).
> "Later", each process updates this list with the actual pointer to the
> lists by looking at the shared memory in rte_eal_init (calling
> rte_eal_tailqs_init).
>
> What matters is that secondary tailqs are a subset of the primary tailqs.
Which is not the case for me, I have secondary including all
tailqs, and primary only having a subset.
Check here :
http://dpdk.org/ml/archives/dev/2016-September/047329.html
> I still have some trouble understanding what you are trying to do.
Having things work ;-)
> As Sergio asked, can you come up with a simplified example/use case ?
Not trivial. I'll see what I can do.
> Thanks.
>
>
> --
> David Marchand
Regards,
Jean
^ permalink raw reply
* Re: [PATCH v2 1/3] mem: fix hugepage mapping error messages
From: Jean Tourrilhes @ 2016-10-05 16:41 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Sergio Gonzalez Monroy, dev
In-Reply-To: <5999446.HPKiItx4sF@xps13>
On Wed, Oct 05, 2016 at 11:51:48AM +0200, Thomas Monjalon wrote:
>
> Applied, thanks
> A rebase was necessary because of this patch: http://dpdk.org/commit/c00ae961
> Please check everything is OK.
Tested today's master. Working as expected.
Thanks !
Jean
^ permalink raw reply
* Re: [PATCH v2 04/22] qede/base: update base driver
From: Thomas Monjalon @ 2016-10-05 16:29 UTC (permalink / raw)
To: Mody, Rasesh
Cc: Ferruh Yigit, Rasesh Mody, dev, Dept-EngDPDKDev@qlogic.com,
Bruce Richardson
In-Reply-To: <BLUPR0701MB157207202A7D8EB817D33DA59FC10@BLUPR0701MB1572.namprd07.prod.outlook.com>
2016-09-30 18:40, Mody, Rasesh:
> > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> > Thank you for the update, base driver patch update now reduced from
> > "14653 insertions(+), 8536 deletions(-)" to
> > "10857 insertions(+), 4853 deletions(-)"
> >
> > But this is still to big for reviewing, specially there are some low hanging fruits
> > for cleanup, like big chunk of comment updates or whitespace updates or
> > non base driver codes in the patch.
> >
> > If the expectation is that somebody non maintainer review the code,
> > understand it and highlight any possible defects, I believe this patch is too big
> > and needs to be split more into logical pieces, but since this is a driver code
> > and a little special, and it may not be possible to completely understand the
> > code without knowing underlying hardware, I am not sure how to proceed
> > and adding Bruce and Thomas to cc for guidance.
As you said it makes contribution really hard.
So it is missing the point of sharing its source code in Open Source.
> This 8.10.x.x base driver is a common code shared by multiple drivers. It has gone through extensive testing.
> We have split the base driver patch into smaller logical patches. It would be difficult to split this patch further.
Please check how Intel updates its base drivers and try harder.
We all know it requires some time, but it provides a valuable knowledge base.
Thanks
^ permalink raw reply
* Re: [PATCH v4 1/2] librte_ether: add internal callback functions
From: Thomas Monjalon @ 2016-10-05 16:10 UTC (permalink / raw)
To: Bernard Iremonger; +Cc: dev, rahul.r.shah, wenzhuo.lu, az5157, jerin.jacob
In-Reply-To: <1475592734-22355-2-git-send-email-bernard.iremonger@intel.com>
2016-10-04 15:52, Bernard Iremonger:
> add _rte_eth_dev_callback_process_vf function.
> add _rte_eth_dev_callback_process_generic function
>
> Adding a callback to the user application on VF to PF mailbox message,
> allows passing information to the application controlling the PF
> when a VF mailbox event message is received, such as VF reset.
I have some difficulties to parse this explanation.
Please could you reword it and precise the direction of the message
and the use case context?
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2510,6 +2510,20 @@ void
> _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> enum rte_eth_event_type event)
> {
> + return _rte_eth_dev_callback_process_generic(dev, event, NULL);
> +}
> +
> +void
> +_rte_eth_dev_callback_process_vf(struct rte_eth_dev *dev,
> + enum rte_eth_event_type event, void *param)
> +{
> + return _rte_eth_dev_callback_process_generic(dev, event, param);
> +}
This function is just adding a parameter, compared to the legacy
_rte_eth_dev_callback_process.
Why calling it process_vf?
And by the way, why not just replacing the legacy function?
As it is a driver interface, there is no ABI restriction.
> +
> +void
> +_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev,
> + enum rte_eth_event_type event, void *param)
> +{
[...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -3026,6 +3026,7 @@ enum rte_eth_event_type {
> /**< queue state event (enabled/disabled) */
> RTE_ETH_EVENT_INTR_RESET,
> /**< reset interrupt event, sent to VF on PF reset */
> + RTE_ETH_EVENT_VF_MBOX, /**< PF mailbox processing callback */
> RTE_ETH_EVENT_MAX /**< max value of this enum */
> };
Either we choose to have a "generic" VF event well documented,
or it is just a specific event with a tip on where to find the doc.
Here we need at least to know how to handle the argument.
> +/**
> + * @internal Executes all the user application registered callbacks. Used by:
> + * _rte_eth_dev_callback_process and _rte_eth_dev_callback_process_vf
> + * It is for DPDK internal user only. User application should not call it
> + * directly.
> + *
> + * @param dev
> + * Pointer to struct rte_eth_dev.
> + * @param event
> + * Eth device interrupt event type.
> + *
> + * @param param
> + * parameters to pass back to user application.
> + *
> + * @return
> + * void
> + */
> +void
> +_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev,
> + enum rte_eth_event_type event, void *param);
This is really an internal function and should not be exported at all.
^ permalink raw reply
* Re: [PATCH v2 1/8] mbuf: add function to dump ol flag list
From: Olivier Matz @ 2016-10-05 14:19 UTC (permalink / raw)
To: De Lara Guarch, Pablo, dev@dpdk.org
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D8973CA04761@IRSMSX108.ger.corp.intel.com>
Hi Pablo,
On 10/05/2016 08:45 AM, De Lara Guarch, Pablo wrote:
> Hi Olivier,
>
>> -----Original Message-----
>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>> index fb2b962..56f37e6 100644
>> --- a/lib/librte_mbuf/rte_mbuf.c
>> +++ b/lib/librte_mbuf/rte_mbuf.c
>> @@ -319,6 +319,53 @@ const char *rte_get_rx_ol_flag_name(uint64_t mask)
>> }
>> }
>>
>> +struct flag_mask {
>> + uint64_t flag;
>> + uint64_t mask;
>> + const char *default_name;
>> +};
>> +
>> +/* write the list of rx ol flags in buffer buf */
>> +int rte_get_rx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
>
> Return type should go in a separate line (same case in the other function).
I'll fix that too, thanks for reviewing.
Olivier
^ permalink raw reply
* Re: [PATCH v2 4/8] app/testpmd: add option to enable lro
From: Olivier Matz @ 2016-10-05 14:18 UTC (permalink / raw)
To: De Lara Guarch, Pablo, dev@dpdk.org
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D8973CA04720@IRSMSX108.ger.corp.intel.com>
Hi Pablo,
On 10/05/2016 08:26 AM, De Lara Guarch, Pablo wrote:
> Hi Olivier,
>
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Friday, September 09, 2016 12:56 AM
>> To: dev@dpdk.org; De Lara Guarch, Pablo
>> Subject: [PATCH v2 4/8] app/testpmd: add option to enable lro
>>
>> Introduce a new argument '--enable-lro' to ask testpmd to enable the LRO
>> feature on enabled ports, like it's done for '--enable-rx-cksum' for
>> instance.
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>> app/test-pmd/parameters.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>> index 8792c2c..612ad37 100644
>> --- a/app/test-pmd/parameters.c
>> +++ b/app/test-pmd/parameters.c
>> @@ -150,6 +150,7 @@ usage(char* progname)
>> "If the drop-queue doesn't exist, the packet is dropped. "
>> "By default drop-queue=127.\n");
>> printf(" --crc-strip: enable CRC stripping by hardware.\n");
>> + printf(" --enable-lro: enable large receive offload.\n");
>> printf(" --enable-rx-cksum: enable rx hardware checksum
>> offload.\n");
>> printf(" --disable-hw-vlan: disable hardware vlan.\n");
>> printf(" --disable-hw-vlan-filter: disable hardware vlan filter.\n");
>> @@ -525,6 +526,7 @@ launch_args_parse(int argc, char** argv)
>> { "pkt-filter-size", 1, 0, 0 },
>> { "pkt-filter-drop-queue", 1, 0, 0 },
>> { "crc-strip", 0, 0, 0 },
>> + { "enable-lro", 0, 0, 0 },
>> { "enable-rx-cksum", 0, 0, 0 },
>> { "enable-scatter", 0, 0, 0 },
>> { "disable-hw-vlan", 0, 0, 0 },
>> @@ -765,6 +767,8 @@ launch_args_parse(int argc, char** argv)
>> }
>> if (!strcmp(lgopts[opt_idx].name, "crc-strip"))
>> rx_mode.hw_strip_crc = 1;
>> + if (!strcmp(lgopts[opt_idx].name, "enable-lro"))
>> + rx_mode.enable_lro = 1;
>> if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
>> rx_mode.enable_scatter = 1;
>> if (!strcmp(lgopts[opt_idx].name, "enable-rx-cksum"))
>> --
>> 2.8.1
>
> Could you add this new parameter in the testpmd documentation?
Yes, I'll do it.
Thanks,
Olivier
^ permalink raw reply
* Re: [PATCH v7] net/virtio: add set_mtu in virtio
From: Dey, Souvik @ 2016-10-05 14:05 UTC (permalink / raw)
To: Kavanagh, Mark B, yuanhan.liu@linux.intel.com,
stephen@networkplumber.org
Cc: dev@dpdk.org
In-Reply-To: <DC5AD7FA266D86499789B1BCAEC715F85C722FC4@irsmsx105.ger.corp.intel.com>
Yes Mark, I have modified the patch with the below comments.
drivers/net/virtio/virtio_ethdev.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 423c597..1dbfea6 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -653,12 +653,20 @@ virtio_dev_allmulticast_disable(struct rte_eth_dev *dev)
PMD_INIT_LOG(ERR, "Failed to disable allmulticast");
}
+#define VLAN_TAG_LEN 4 /* 802.3ac tag (not DMA'd) */
+
+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
+{
+ struct virtio_hw *hw = dev->data->dev_private;
+ uint32_t ether_hdr_len = ETHER_HDR_LEN + VLAN_TAG_LEN +
+ hw->vtnet_hdr_size;
+ uint32_t frame_size = mtu + ether_hdr_len;
+
+ if (mtu < ETHER_MIN_MTU || frame_size > VIRTIO_MAX_RX_PKTLEN) {
+ PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n",
+ ETHER_MIN_MTU, (VIRTIO_MAX_RX_PKTLEN - ether_hdr_len));
+ return -EINVAL;
+ }
+ return 0;
+}
Let mem know if this looks good or we have few more comments.
--
Regards,
Souvik
-----Original Message-----
From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com]
Sent: Wednesday, October 5, 2016 4:16 AM
To: Dey, Souvik <sodey@sonusnet.com>; yuanhan.liu@linux.intel.com; stephen@networkplumber.org
Cc: dev@dpdk.org
Subject: RE: [PATCH v7] net/virtio: add set_mtu in virtio
>Hi All,
> Is there any further comments or modifications required for this
>patch, or what next steps do you guys suggest here ?
Hi Souvik,
Some minor comments inline.
Thanks,
Mark
>
>--
>Regards,
>Souvik
>
>-----Original Message-----
>From: Dey, Souvik
>Sent: Saturday, October 1, 2016 10:09 AM
>To: mark.b.kavanagh@intel.com; yuanhan.liu@linux.intel.com;
>stephen@networkplumber.org; dev@dpdk.org
>Subject: RE: [PATCH v7] net/virtio: add set_mtu in virtio
>
>Hi Liu/Stephen/Mark,
>
> I have submitted Version 7 of this patch. Do let me know if this looks proper.
>
>--
>Regards,
>Souvik
>
>-----Original Message-----
>From: Dey, Souvik
>Sent: Thursday, September 29, 2016 4:32 PM
>To: mark.b.kavanagh@intel.com; yuanhan.liu@linux.intel.com;
>stephen@networkplumber.org; dev@dpdk.org
>Cc: Dey, Souvik <sodey@sonusnet.com>
>Subject: [PATCH v7] net/virtio: add set_mtu in virtio
>
>
>Virtio interfaces do not currently allow the user to specify a
>particular Maximum Transmission Unit (MTU).Consequently, the MTU of
>Virtio interfaces is typically set to the Ethernet default value of 1500.
>This is problematic in the case of cloud deployments, in which a
>specific (and potentially non-standard) MTU needs to be set by a DHCP
>server, which needs to be honored by all interfaces across the traffic
>path.To achieve this Virtio interfaces should support setting of MTU.
>In case when GRE/VXLAN tunneling is used for internal communication,
>there will be an overhead added by the infrastructure in the packet
>over and above the ETHER MTU of 1518. So to take care of this overhead
>in these cases the DHCP server corrects the L3 MTU to 1454. But since
>virtio interfaces was not having the MTU set functionality that MTU
>sent by the DHCP server was ignored and the instance will still send
>packets with 1500 MTU which after encapsulation will become more than
>1518 and eventually gets dropped in the infrastructure.
>By adding an additional 'set_mtu' function to the Virtio driver, we can
>honor the MTU sent by the DHCP server. The dhcp server/controller can
>then leverage this 'set_mtu' functionality to resolve the above
>mentioned issue of packets getting dropped due to incorrect size.
>
>
>Signed-off-by: Souvik Dey <sodey@sonusnet.com>
>
>---
>Changes in v7:
>- Replaced the CRC_LEN with the merge rx buf header length.
>- Changed the frame_len max validation to VIRTIO_MAX_RX_PKTLEN.
>Changes in v6:
>- Description of change corrected
>- Corrected the identations
>- Corrected the subject line too
>- The From line was also not correct
>- Re-submitting as the below patch was not proper Changes in v5:
>- Fix log message for out-of-bounds MTU parameter in virtio_mtu_set
>- Calculate frame size, based on 'mtu' parameter
>- Corrected the upper bound and lower bound checks in virtio_mtu_set
>Changes in v4: Incorporated review comments.
>Changes in v3: Corrected few style errors as reported by sys-stv.
>Changes in v2: Incorporated review comments.
>
> drivers/net/virtio/virtio_ethdev.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
>diff --git a/drivers/net/virtio/virtio_ethdev.c
>b/drivers/net/virtio/virtio_ethdev.c
>index 423c597..1dbfea6 100644
>--- a/drivers/net/virtio/virtio_ethdev.c
>+++ b/drivers/net/virtio/virtio_ethdev.c
>@@ -653,12 +653,20 @@ virtio_dev_allmulticast_disable(struct rte_eth_dev *dev)
> PMD_INIT_LOG(ERR, "Failed to disable allmulticast"); }
>
>+#define VLAN_TAG_LEN 4 /* 802.3ac tag (not DMA'd) */
There should be a blank line between the #define and the function prototype beneath.
>+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
>+ struct virtio_hw *hw = dev->data->dev_private;
>+ uint32_t ether_hdr_len = ETHER_HDR_LEN + VLAN_TAG_LEN +
>+ hw->vtnet_hdr_size;
I'll rely on Stephen and Yuanhan's judgment for this.
>+ uint32_t frame_size = mtu + ether_hdr_len;
>+
>+ if (mtu < ETHER_MIN_MTU || frame_size > VIRTIO_MAX_RX_PKTLEN) {
>+ PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n",
>+ ETHER_MIN_MTU, VIRTIO_MAX_RX_PKTLEN);
Shouldn't last format print parameter should be (VIRTIO_MAX_RX_PKTLEN - ether_hdr_len)?
i.e PMD_INIT_LOG(ERR, "MTU should........%d\n",
ETHER_MIN_MTU, (VIRTIO_MAX_RX_PKTLEN - ether_hdr_len));
>+ return -EINVAL;
>+ }
>+ return 0;
>+}
>
> /*
> * dev_ops for virtio, bare necessities for basic operation
> */
>@@ -677,7 +685,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops = {
> .allmulticast_enable = virtio_dev_allmulticast_enable,
> .allmulticast_disable = virtio_dev_allmulticast_disable,
>+ .mtu_set = virtio_mtu_set,
> .dev_infos_get = virtio_dev_info_get,
> .stats_get = virtio_dev_stats_get,
> .xstats_get = virtio_dev_xstats_get,
>--
>2.9.3.windows.1
^ permalink raw reply related
* Re: Proposal: enable redirection of DPDK logs from the user app
From: Olivier Matz @ 2016-10-05 14:03 UTC (permalink / raw)
To: Montorsi, Francesco, dev@dpdk.org
In-Reply-To: <348c536dca594a3fbd38137e2a5a29aa@bilemail1.empirix.com>
Hi Francesco,
On 10/05/2016 03:26 PM, Montorsi, Francesco wrote:
> Hi Olivier,
>
>> On 10/04/2016 02:28 PM, Montorsi, Francesco wrote:
>>> Yes, but to be honest, that seems a troublesome solution for something
>>> as easy as logging a string; e.g. by using fopencookie() approach, you
>>> don't have the concept of "log message", you just provide a function
>>> that must write a block of bytes somewhere.
>>> Typically instead, you need to know where a log message starts and
>>> ends, to e.g., add prefixes/postfixes to it.
>>
>> I'm not sure that true if you call setbuf(log_stream, NULL).
>>
>> In that case, it looks easy to prefix / postfix messages with a fopencookie
>> callback like:
>>
>> /* example on stdout */
>> ssize_t
>> simple_write(void *c, const char *buf, size_t size) {
>> ssize_t ret1, ret2, ret3;
>>
>> ret1 = fwrite("<", 1, 1, stdout);
>> if (ret1 == 0)
>> return 0;
>> ret2 = fwrite(buf, size, 1, stdout);
>> if (ret2 == 0)
>> return 0;
>> ret3 = fwrite(">", 1, 1, stdout);
>> if (ret3 == 0)
>> return 0;
>> return ret1 + ret2 + ret3;
>> }
>>
> I didn't know about setbuf()... but are we sure that in this way the simple_write() function will always receive a full string? I mean: in the manpage for setbuf() it says:
>
> "... When the first I/O operation occurs on a file, malloc(3) is called, and a buffer is obtained. .... If the argument buf is NULL, only the mode is affected; a new buffer will be allocated on the next read or write operation."
>
> But: is it true that 1 write operation corresponds to 1 vfprintf() call? Maybe if you have a "long" a single vfprintf() call may translate to several simple_write() calls... I don't know honestly.
I did a quick test with a fixed version of simple_write():
ssize_t
simple_write(void *c, const char *buf, size_t size)
{
ssize_t ret1 = -42, ret3 = -42;
ssize_t ret = 0;
ret1 = fwrite("<", 1, 1, stdout);
if (ret1 == 0)
goto ret;
ret = fwrite(buf, 1, size, stdout);
if (ret != size)
goto ret;
ret3 = fwrite(">", 1, 1, stdout);
if (ret3 == 0)
goto ret;
ret:
/* printf("ret=%d ret1=%d ret3=%d\n", (int)ret, (int)ret1, (int)ret3); */
return ret;
}
It looks like transmitting a string bigger than BUFSIZ (8192) induces
several calls to simple_write(). For smaller calls, it seems there is no
split (1 printf = 1 simple_write).
Of course, this is a just test and not a proof :)
I think we would have a similar issue with the other approach.
For me, the current API looks ok, however let's see the opinion of the
maintainer that could be different of mine. In any case, thank you for
proposing enhancements.
Regards,
Olivier
^ permalink raw reply
* Re: [PATCH] ip_frag: fix missing dependency on librte_hash
From: Thomas Monjalon @ 2016-10-05 13:46 UTC (permalink / raw)
To: Panu Matilainen; +Cc: dev, konstantin.ananyev
In-Reply-To: <932d0051cc30a749f810b70e6465cf77679e3c95.1475669648.git.pmatilai@redhat.com>
2016-10-05 15:14, Panu Matilainen:
> Not sure what exactly changed and where, but I've started getting
> build failures on Fedora rawhide i386:
> lib/librte_ip_frag/ip_frag_internal.c:36:23: fatal error:
> rte_jhash.h: No such file or directory
> #include <rte_jhash.h>
> ^
> Looking at librte_ip_frag, it clearly depends on librte_hash so
> its probably more a question of something commonly masking the issue.
>
> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH 2/2] kni: remove unnecessary ethtool files
From: Thomas Monjalon @ 2016-10-05 13:45 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, Remy Horton
In-Reply-To: <28679b7e-7005-6d3e-4c1c-0f962f1eacbf@intel.com>
2016-10-05 09:58, Remy Horton:
>
> On 30/09/2016 11:10, Ferruh Yigit wrote:
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> > lib/librte_eal/linuxapp/kni/Makefile | 3 -
> > .../linuxapp/kni/ethtool/igb/igb_debugfs.c | 28 --
> > .../linuxapp/kni/ethtool/igb/igb_hwmon.c | 260 ---------------
> > .../linuxapp/kni/ethtool/igb/igb_procfs.c | 363 ---------------------
> > 4 files changed, 654 deletions(-)
> > delete mode 100644 lib/librte_eal/linuxapp/kni/ethtool/igb/igb_debugfs.c
> > delete mode 100644 lib/librte_eal/linuxapp/kni/ethtool/igb/igb_hwmon.c
> > delete mode 100644 lib/librte_eal/linuxapp/kni/ethtool/igb/igb_procfs.c
>
> Acked-by: Remy Horton <remy.horton@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH v2 09/12] virtio: add Rx checksum offload support
From: Olivier Matz @ 2016-10-05 13:30 UTC (permalink / raw)
To: Maxime Coquelin, dev, yuanhan.liu
Cc: konstantin.ananyev, sugesh.chandran, bruce.richardson,
jianfeng.tan, helin.zhang, adrien.mazarguil, stephen, dprovan,
xiao.w.wang
In-Reply-To: <e96537d6-b99a-2668-e8bf-3a64529f0d55@redhat.com>
On 10/05/2016 03:27 PM, Maxime Coquelin wrote:
>> @@ -903,7 +905,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
>> **rx_pkts, uint16_t nb_pkts)
>> rte_vlan_strip(rxm);
>>
>> /* Update offload features */
>> - if (virtio_rx_offload(rxm, hdr) < 0) {
>> + if ((features & VIRTIO_NET_F_GUEST_CSUM) &&
> s/VIRTIO_NET_F_GUEST_CSUM/(1u << VIRTIO_NET_F_GUEST_CSUM)/
oooh good catch :)
> And don't forget to update the test for LRO patch.
yep
> Except this, it sounds good.
Thanks, I'll send a v3 soon.
Olivier
^ permalink raw reply
* Re: [PATCH v2 09/12] virtio: add Rx checksum offload support
From: Maxime Coquelin @ 2016-10-05 13:27 UTC (permalink / raw)
To: Olivier Matz, dev, yuanhan.liu
Cc: konstantin.ananyev, sugesh.chandran, bruce.richardson,
jianfeng.tan, helin.zhang, adrien.mazarguil, stephen, dprovan,
xiao.w.wang
In-Reply-To: <98db7fbb-42bd-512f-1d40-a1f3304a895e@6wind.com>
Hi Olivier,
On 10/05/2016 01:56 PM, Olivier Matz wrote:
> Hi Maxime,
>
> On 10/03/2016 02:51 PM, Maxime Coquelin wrote:
>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>> @@ -50,6 +50,7 @@
>>> #include <rte_string_fns.h>
>>> #include <rte_errno.h>
>>> #include <rte_byteorder.h>
>>> +#include <rte_net.h>
>>>
>>> #include "virtio_logs.h"
>>> #include "virtio_ethdev.h"
>>> @@ -627,6 +628,56 @@ virtio_update_packet_stats(struct virtnet_stats
>>> *stats, struct rte_mbuf *mbuf)
>>> }
>>> }
>>>
>>> +/* Optionally fill offload information in structure */
>>> +static int
>>> +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>>> +{
>>> + struct rte_net_hdr_lens hdr_lens;
>>> + uint32_t hdrlen, ptype;
>>> + int l4_supported = 0;
>>> +
>>> + /* nothing to do */
>>> + if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
>>> + return 0;
>> Maybe we could first check whether offload features were negotiated?
>> Doing this, we could return before accessing the header and so avoid a
>> cache miss.
>
> Yes, doing this would avoid reading the virtio header when the rx
> function is virtio_recv_pkts(). When using virtio_recv_mergeable_pkts(),
> it won't have a big impact since we already need to read hdr->num_buffers.
Right, it matters only for the non-mergeable buffers case.
>
>
> I plan to do something like this in both recv functions:
>
> @@ -854,6 +854,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts, uint16_t nb_pkts)
> int error;
> uint32_t i, nb_enqueued;
> uint32_t hdr_size;
> + uint64_t features;
> struct virtio_net_hdr *hdr;
>
> nb_used = VIRTQUEUE_NUSED(vq);
> @@ -872,6 +873,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts, uint16_t nb_pkts)
> nb_rx = 0;
> nb_enqueued = 0;
> hdr_size = hw->vtnet_hdr_size;
> + features = hw->guest_features;
>
> for (i = 0; i < num ; i++) {
> rxm = rcv_pkts[i];
> @@ -903,7 +905,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts, uint16_t nb_pkts)
> rte_vlan_strip(rxm);
>
> /* Update offload features */
> - if (virtio_rx_offload(rxm, hdr) < 0) {
> + if ((features & VIRTIO_NET_F_GUEST_CSUM) &&
s/VIRTIO_NET_F_GUEST_CSUM/(1u << VIRTIO_NET_F_GUEST_CSUM)/
And don't forget to update the test for LRO patch.
Except this, it sounds good.
Thanks,
Maxime
> + virtio_rx_offload(rxm, hdr) < 0) {
> virtio_discard_rxbuf(vq, rxm);
> rxvq->stats.errors++;
> continue;
>
> Thank you for the feedback.
> Olivier
>
^ permalink raw reply
* Re: Proposal: enable redirection of DPDK logs from the user app
From: Montorsi, Francesco @ 2016-10-05 13:26 UTC (permalink / raw)
To: Olivier Matz, dev@dpdk.org
In-Reply-To: <c9b811de-28d7-f4da-5a5c-2038779e4b6b@6wind.com>
Hi Olivier,
> On 10/04/2016 02:28 PM, Montorsi, Francesco wrote:
> > Yes, but to be honest, that seems a troublesome solution for something
> > as easy as logging a string; e.g. by using fopencookie() approach, you
> > don't have the concept of "log message", you just provide a function
> > that must write a block of bytes somewhere.
> > Typically instead, you need to know where a log message starts and
> > ends, to e.g., add prefixes/postfixes to it.
>
> I'm not sure that true if you call setbuf(log_stream, NULL).
>
> In that case, it looks easy to prefix / postfix messages with a fopencookie
> callback like:
>
> /* example on stdout */
> ssize_t
> simple_write(void *c, const char *buf, size_t size) {
> ssize_t ret1, ret2, ret3;
>
> ret1 = fwrite("<", 1, 1, stdout);
> if (ret1 == 0)
> return 0;
> ret2 = fwrite(buf, size, 1, stdout);
> if (ret2 == 0)
> return 0;
> ret3 = fwrite(">", 1, 1, stdout);
> if (ret3 == 0)
> return 0;
> return ret1 + ret2 + ret3;
> }
>
I didn't know about setbuf()... but are we sure that in this way the simple_write() function will always receive a full string? I mean: in the manpage for setbuf() it says:
"... When the first I/O operation occurs on a file, malloc(3) is called, and a buffer is obtained. .... If the argument buf is NULL, only the mode is affected; a new buffer will be allocated on the next read or write operation."
But: is it true that 1 write operation corresponds to 1 vfprintf() call? Maybe if you have a "long" a single vfprintf() call may translate to several simple_write() calls... I don't know honestly.
> > Indeed, most of the C/C++ (open source) libraries have some simple
> > hook that allows the user to have more control on logging... I think
> > DPDK should be no exception... :)
>
> I understand that the current API is a bit more complex, but I don't feel there
> is any blocking issue to do what you want. What do you think?
See above. Moreover, IMHO it would be much more user-friendly to have a simple function callback to implement vs having to dig into fopencookie()+setbuf()+etc etc .
> Also, I know you've said your patch needs some rework, but as you've also
> said you are using it, maybe it would be useful for you to know:
> - it makes use of a global variable 'log_buffer', shared by all the pthreads,
> which can lead to crashes
That's a good point. I will turn it into a __thread variable. Thanks for pointing out this.
> - it strips the log messages to 4095 chars
Correct, but in my experience DPDK never creates such a long line of log message...
Francesco
^ permalink raw reply
* Re: [RFC 0/7] changing mbuf pool handler
From: Hunt, David @ 2016-10-05 13:15 UTC (permalink / raw)
To: Hemant Agrawal, Olivier Matz, dev@dpdk.org; +Cc: jerin.jacob@caviumnetworks.com
In-Reply-To: <DB5PR04MB16050EBF3B62A4C68A3AD3A889C40@DB5PR04MB1605.eurprd04.prod.outlook.com>
On 5/10/2016 12:49 PM, Hemant Agrawal wrote:
> Hi Olivier,
>
>> -----Original Message-----
>> From: Hunt, David [mailto:david.hunt@intel.com]
>> Hi Olivier,
>>
>>
>> On 3/10/2016 4:49 PM, Olivier Matz wrote:
>>> Hi Hemant,
>>>
>>> Thank you for your feedback.
>>>
>>> On 09/22/2016 01:52 PM, Hemant Agrawal wrote:
>>>> Hi Olivier
>>>>
>>>> On 9/19/2016 7:12 PM, Olivier Matz wrote:
>>>>> Hello,
>>>>>
>>>>> Following discussion from [1] ("usages issue with external mempool").
>>>>>
>>>>> This is a tentative to make the mempool_ops feature introduced by
>>>>> David Hunt [2] more widely used by applications.
>>>>>
>>>>> It applies on top of a minor fix in mbuf lib [3].
>>>>>
>>>>> To sumarize the needs (please comment if I did not got it properly):
>>>>>
>>>>> - new hw-assisted mempool handlers will soon be introduced
>>>>> - to make use of it, the new mempool API [4]
>> (rte_mempool_create_empty,
>>>>> rte_mempool_populate, ...) has to be used
>>>>> - the legacy mempool API (rte_mempool_create) does not allow to
>> change
>>>>> the mempool ops. The default is "ring_<s|m>p_<s|m>c" depending on
>>>>> flags.
>>>>> - the mbuf helper (rte_pktmbuf_pool_create) does not allow to change
>>>>> them either, and the default is RTE_MBUF_DEFAULT_MEMPOOL_OPS
>>>>> ("ring_mp_mc")
>>>>> - today, most (if not all) applications and examples use either
>>>>> rte_pktmbuf_pool_create or rte_mempool_create to create the mbuf
>>>>> pool, making it difficult to take advantage of this feature with
>>>>> existing apps.
>>>>>
>>>>> My initial idea was to deprecate both rte_pktmbuf_pool_create() and
>>>>> rte_mempool_create(), forcing the applications to use the new API,
>>>>> which is more flexible. But after digging a bit, it appeared that
>>>>> rte_mempool_create() is widely used, and not only for mbufs.
>>>>> Deprecating it would have a big impact on applications, and
>>>>> replacing it with the new API would be overkill in many use-cases.
>>>> I agree with the proposal.
>>>>
>>>>> So I finally tried the following approach (inspired from a
>>>>> suggestion Jerin [5]):
>>>>>
>>>>> - add a new mempool_ops parameter to rte_pktmbuf_pool_create().
>> This
>>>>> unfortunatelly breaks the API, but I implemented an ABI compat layer.
>>>>> If the patch is accepted, we could discuss how to announce/schedule
>>>>> the API change.
>>>>> - update the applications and documentation to prefer
>>>>> rte_pktmbuf_pool_create() as much as possible
>>>>> - update most used examples (testpmd, l2fwd, l3fwd) to add a new
>> command
>>>>> line argument to select the mempool handler
>>>>>
>>>>> I hope the external applications would then switch to
>>>>> rte_pktmbuf_pool_create(), since it supports most of the use-cases
>>>>> (even priv_size != 0, since we can call rte_mempool_obj_iter() after) .
>>>>>
>>>> I will still prefer if you can add the "rte_mempool_obj_cb_t *obj_cb,
>>>> void *obj_cb_arg" into "rte_pktmbuf_pool_create". This single
>>>> consolidated wrapper will almost make it certain that applications
>>>> will not try to use rte_mempool_create for packet buffers.
>>> The patch changes the example applications. I'm not sure I understand
>>> why adding these arguments would force application to not use
>>> rte_mempool_create() for packet buffers. Do you have a application in
>> mind?
>>> For the mempool_ops parameter, we must pass it at init because we need
>>> to know the mempool handler before populating the pool. For object
>>> initialization, it can be done after, so I thought it was better to
>>> reduce the number of arguments to avoid to fall in the
>>> mempool_create() syndrom :)
>> I also agree with the proposal. Looks cleaner.
>>
>> I would lean to the side of keeping the parameters to the minimum, i.e.
>> not adding *obj_cb and *obj_cb_arg into rte_pktmbuf_pool_create.
>> Developers always have the option of going with rte_mempool_create if they
>> need more fine-grained control.
> [Hemant] The implementations with hw offloaded mempools don't want developer using *rte_mempool_create* for packet buffer pools.
> This API does not work for hw offloaded mempool.
>
> Also, *rte_mempool_create_empty* - may not be convenient for many application, as it requires calling 4+ APIs.
>
> Olivier is not in favor of deprecating the *rte_mempool_create*. I agree with concerns raised by him.
>
> Essentially, I was suggesting to upgrade * rte_pktmbuf_pool_create* to be like *rte_mempool_create* for packet buffers exclusively.
>
> This will provide a clear segregation for API usages w.r.t the packet buffer pool vs all other type of mempools.
Yes, it does sound like we need those extra parameters on
rte_pktmbuf_pool_create.
Regards,
Dave.
^ permalink raw reply
* Re: [PATCH 2/2] net/bonding: enable slave VLAN filter
From: Iremonger, Bernard @ 2016-10-05 12:53 UTC (permalink / raw)
To: Eric Kinzie, dev@dpdk.org; +Cc: Jan Blunck
In-Reply-To: <1470335083-6380-3-git-send-email-ehkinzie@gmail.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Eric Kinzie
> Sent: Thursday, August 4, 2016 7:25 PM
> To: dev@dpdk.org
> Cc: Jan Blunck <jblunck@infradead.org>
> Subject: [dpdk-dev] [PATCH 2/2] net/bonding: enable slave VLAN filter
>
> SR-IOV virtual functions cannot rely on promiscuous mode for the reception
> of VLAN tagged frames. Program the vlan filter for each slave when a vlan is
> configured for the bonding master.
>
> Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
^ permalink raw reply
* Re: [PATCH 1/2] net/bonding: validate speed after link up
From: Iremonger, Bernard @ 2016-10-05 12:53 UTC (permalink / raw)
To: Eric Kinzie, dev@dpdk.org; +Cc: Jan Blunck
In-Reply-To: <1470335083-6380-2-git-send-email-ehkinzie@gmail.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Eric Kinzie
> Sent: Thursday, August 4, 2016 7:25 PM
> To: dev@dpdk.org
> Cc: Jan Blunck <jblunck@infradead.org>
> Subject: [dpdk-dev] [PATCH 1/2] net/bonding: validate speed after link up
>
> It's possible for the bonding driver to mistakenly reject an interface based in
> it's, as yet, unnegotiated link speed and duplex. Always allow the interface
> to be added to the bonding interface but require link properties validation to
> succeed before slave is activated.
>
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>
> Signed-off-by: Eric Kinzie <ehkinzie@gmail.com>
Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
^ permalink raw reply
* Re: [PATCH] app/test: add mempool walk
From: Olivier Matz @ 2016-10-05 12:22 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
In-Reply-To: <1475528313-7759-1-git-send-email-thomas.monjalon@6wind.com>
Hi Thomas,
On 10/03/2016 10:58 PM, Thomas Monjalon wrote:
> The mempool function rte_mempool_walk was not tested.
> It will print the name of all mempools.
>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
^ permalink raw reply
* Re: [PATCH] mbuf: fix leak and errno on pool creation error
From: Thomas Monjalon @ 2016-10-05 12:22 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev, stable
In-Reply-To: <1474288481-19573-1-git-send-email-olivier.matz@6wind.com>
2016-09-19 14:34, Olivier Matz:
> On error, the mempool object has to be freed, and rte_errno should be a
> positive value.
>
> Fixes: 152ca517900b ("mbuf: use default mempool handler from config")
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Applied, thanks
^ permalink raw reply
* Re: Proposal: enable redirection of DPDK logs from the user app
From: Olivier Matz @ 2016-10-05 12:18 UTC (permalink / raw)
To: Montorsi, Francesco, dev@dpdk.org
In-Reply-To: <4b3ed268f2584b4aaf251ba39f8c90bf@bilemail1.empirix.com>
Hi Francesco,
On 10/04/2016 02:28 PM, Montorsi, Francesco wrote:
> Hi Olivier,
>
>> It seems the mailing list stripped your patch sent as attachment.
>> Can you please resend it again in the body of the mail?
> You're right sorry. It's attached at the end of this mail.
>
>> I think we can already redirect logs to a file by using
>> fopencookie() + rte_openlog_stream(). Did you already check these
>> functions?
>
> Yes, but to be honest, that seems a troublesome solution for
> something as easy as logging a string; e.g. by using fopencookie()
> approach, you don't have the concept of "log message", you just
> provide a function that must write a block of bytes somewhere.
> Typically instead, you need to know where a log message starts and
> ends, to e.g., add prefixes/postfixes to it.
I'm not sure that true if you call setbuf(log_stream, NULL).
In that case, it looks easy to prefix / postfix messages with a
fopencookie callback like:
/* example on stdout */
ssize_t
simple_write(void *c, const char *buf, size_t size)
{
ssize_t ret1, ret2, ret3;
ret1 = fwrite("<", 1, 1, stdout);
if (ret1 == 0)
return 0;
ret2 = fwrite(buf, size, 1, stdout);
if (ret2 == 0)
return 0;
ret3 = fwrite(">", 1, 1, stdout);
if (ret3 == 0)
return 0;
return ret1 + ret2 + ret3;
}
> Indeed, most of the C/C++ (open source) libraries have some simple
> hook that allows the user to have more control on logging... I think
> DPDK should be no exception... :)
I understand that the current API is a bit more complex, but I don't
feel there is any blocking issue to do what you want. What do you think?
Also, I know you've said your patch needs some rework, but as you've
also said you are using it, maybe it would be useful for you to know:
- it makes use of a global variable 'log_buffer', shared by all the
pthreads, which can lead to crashes
- it strips the log messages to 4095 chars
Regards,
Olivier
^ permalink raw reply
* [PATCH] ip_frag: fix missing dependency on librte_hash
From: Panu Matilainen @ 2016-10-05 12:14 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev
Not sure what exactly changed and where, but I've started getting
build failures on Fedora rawhide i386:
lib/librte_ip_frag/ip_frag_internal.c:36:23: fatal error:
rte_jhash.h: No such file or directory
#include <rte_jhash.h>
^
Looking at librte_ip_frag, it clearly depends on librte_hash so
its probably more a question of something commonly masking the issue.
Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
lib/librte_ip_frag/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_ip_frag/Makefile b/lib/librte_ip_frag/Makefile
index e97dfbd..43f8b1e 100644
--- a/lib/librte_ip_frag/Makefile
+++ b/lib/librte_ip_frag/Makefile
@@ -54,6 +54,7 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_IP_FRAG)-include += rte_ip_frag.h
DEPDIRS-$(CONFIG_RTE_LIBRTE_IP_FRAG) += lib/librte_eal
DEPDIRS-$(CONFIG_RTE_LIBRTE_IP_FRAG) += lib/librte_ether
+DEPDIRS-$(CONFIG_RTE_LIBRTE_IP_FRAG) += lib/librte_hash
DEPDIRS-$(CONFIG_RTE_LIBRTE_IP_FRAG) += lib/librte_mbuf
DEPDIRS-$(CONFIG_RTE_LIBRTE_IP_FRAG) += lib/librte_mempool
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 1/2] mbuf: add rte_pktmbuff_reset_headroom function
From: Olivier Matz @ 2016-10-05 12:02 UTC (permalink / raw)
To: Maxime Coquelin, dev; +Cc: stephen, mst, yuanhan.liu
In-Reply-To: <1475582724-5202-1-git-send-email-maxime.coquelin@redhat.com>
Hi Maxime,
On 10/04/2016 02:05 PM, Maxime Coquelin wrote:
> Some application use rte_mbuf_raw_alloc() function to improve
> performance by not resetting mbuf's fields to their default state.
>
> This can be however problematic for mbuf consumers that need some
> headroom, meaning that data_off field gets decremented after
> allocation. When the mbuf is re-used afterwards, there might not
> be enough room for the consumer to prepend anything, if the data_off
> field is not reset to its default value.
>
> This patch adds a new rte_pktmbuf_reset_headroom() function that
> applications can call to reset the data_off field.
> This patch also replaces current data_off affectations in the mbuf
> lib with a call to this function.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Series:
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Thanks
^ permalink raw reply
* Re: [PATCH] log: do not drop debug logs at compile time
From: Olivier Matz @ 2016-10-05 12:01 UTC (permalink / raw)
To: David Marchand, Wiles, Keith; +Cc: Thomas Monjalon, dev@dpdk.org
In-Reply-To: <CALwxeUvjw8_fRQ8mfV94jehS7WEm3-5S9kCUEYHgOazkEA4uBA@mail.gmail.com>
Hi David,
On 10/04/2016 10:28 AM, David Marchand wrote:
> On Mon, Oct 3, 2016 at 6:27 PM, Wiles, Keith <keith.wiles@intel.com> wrote:
>>> On Oct 3, 2016, at 10:37 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
>>> What makes you feel it's easier to add a log level instead of adding a
>>> new RTE_LOG_DP() function?
>>
>> It seems to me the log levels are for displaying logs at different levels adding a new macro to not log is just a hack because we do not have a log level for data path. This is why I would like to see a log level added and not a new macro.
>>
>> It also appears the new RTE_LOG() will always be in the code as you moved the test to the RTE_LOG_DP() macro. This would mean all RTE_LOG() in the code will always call rte_log(), correct?
>>
>> If using a new DEBUG_DP (maybe DATAPATH is a better log level name) level we can use the same macro as before and modify the level only. This way we can remove via the compiler any log that is below the default RTE_LOG_LEVEL. I see keeping the rte_log() could be a performance problem or code blot when you really want to remove them all.
>>
>> The DATAPATH log level would be above (smaller number) then DEBUG in the enum list. To remove all debug logs just set the RTE_LOG_LEVEL to RTE_LOG_DATAPATH.
>
> If I try to draw a parrallel to syslog (well, the log subsystem in eal
> has always been bound to syslog ...), what you propose here is like
> adding a new level in syslog while you have syslog facilities.
>
> With the current log api, we have types and levels, can't we filter at
> build time depending on the log "type" ?
> Here we would strip PMD type logs > INFO.
I think we may have dataplane logs in other part of the code: in the
application (log type = USER), and surely in some dpdk libraries.
Moreover, as the behavior of the macro changes (in one case it is
stripped at compilation, in the other case not), I'd say having
different functions is clearer for the developer than having a different
behavior depending on log level or log type.
Regards,
Olivier
^ permalink raw reply
* Re: [PATCH] log: do not drop debug logs at compile time
From: Olivier Matz @ 2016-10-05 11:57 UTC (permalink / raw)
To: David Marchand; +Cc: dev@dpdk.org
In-Reply-To: <CALwxeUv-_mV7e9mtXpNf+Ns9HLYPQYsXa59M7zriF1VQBGdYEw@mail.gmail.com>
Hi David,
On 10/04/2016 10:26 AM, David Marchand wrote:
> On Fri, Sep 16, 2016 at 9:43 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
>> Today, all logs whose level is lower than INFO are dropped at
>> compile-time. This prevents from enabling debug logs at runtime using
>> --log-level=8.
>>
>> The rationale was to remove debug logs from the data path at
>> compile-time, avoiding a test at run-time.
>>
>> This patch changes the behavior of RTE_LOG() to avoid the compile-time
>> optimization, and introduces the RTE_LOG_DP() macro that has the same
>> behavior than the previous RTE_LOG(), for the rare cases where debug
>> logs are in the data path.
>>
>> So it is now possible to enable debug logs at run-time by just
>> specifying --log-level=8. Some drivers still have special compile-time
>> options to enable more debug log. Maintainers may consider to
>> remove/reduce them.
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>> config/common_base | 1 +
>> doc/guides/faq/faq.rst | 2 +-
>> drivers/net/bnxt/bnxt_txr.c | 2 +-
>> drivers/net/nfp/nfp_net.c | 8 +++---
>> examples/distributor/main.c | 4 +--
>> examples/ipsec-secgw/esp.c | 2 +-
>> examples/ipsec-secgw/ipsec.c | 4 +--
>> examples/packet_ordering/main.c | 6 ++--
>> examples/quota_watermark/qw/main.c | 2 +-
>> examples/tep_termination/main.c | 4 +--
>> examples/vhost/main.c | 14 +++++-----
>> examples/vhost_xen/main.c | 20 +++++++-------
>> lib/librte_eal/common/include/rte_log.h | 49 +++++++++++++++++++++------------
>> 13 files changed, 67 insertions(+), 51 deletions(-)
>>
>> diff --git a/config/common_base b/config/common_base
>> index 7830535..04b71e9 100644
>> --- a/config/common_base
>> +++ b/config/common_base
>> @@ -89,6 +89,7 @@ CONFIG_RTE_MAX_MEMSEG=256
>> CONFIG_RTE_MAX_MEMZONE=2560
>> CONFIG_RTE_MAX_TAILQ=32
>> CONFIG_RTE_LOG_LEVEL=RTE_LOG_INFO
>> +CONFIG_RTE_LOG_DP_LEVEL=RTE_LOG_INFO
>> CONFIG_RTE_LOG_HISTORY=256
>> CONFIG_RTE_LIBEAL_USE_HPET=n
>> CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
>
> [snip]
>
>> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
>> index 919563c..76b198f 100644
>> --- a/lib/librte_eal/common/include/rte_log.h
>> +++ b/lib/librte_eal/common/include/rte_log.h
>
> [snip]
>
>> @@ -266,6 +257,30 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
>> * - Negative on error.
>> */
>> #define RTE_LOG(l, t, ...) \
>> + rte_log(RTE_LOG_ ## l, \
>> + RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
>> +
>> +/**
>> + * Generates a log message for data path.
>> + *
>> + * Similar to RTE_LOG(), except that it is removed at compilation time
>> + * if the RTE_LOG_DP_LEVEL configuration option is lower than the log
>> + * level argument.
>> + *
>> + * @param l
>> + * Log level. A value between EMERG (1) and DEBUG (8). The short name is
>> + * expanded by the macro, so it cannot be an integer value.
>> + * @param t
>> + * The log type, for example, EAL. The short name is expanded by the
>> + * macro, so it cannot be an integer value.
>> + * @param ...
>> + * The fmt string, as in printf(3), followed by the variable arguments
>> + * required by the format.
>> + * @return
>> + * - 0: Success.
>> + * - Negative on error.
>> + */
>> +#define RTE_LOG_DP(l, t, ...) \
>> (void)((RTE_LOG_ ## l <= RTE_LOG_LEVEL) ? \
>> rte_log(RTE_LOG_ ## l, \
>> RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) : \
>> --
>> 2.8.1
>
> Hum, I suppose RTE_LOG_DP should look at RTE_LOG_DP_LEVEL.
>
That's correct, good catch :)
I'll send a v2 with that fix.
Olivier
^ permalink raw reply
* Re: [PATCH v11 00/24] Introducing rte_driver/rte_device generalization
From: Shreyansh Jain @ 2016-10-05 11:57 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, viktorin, David Marchand, hemant.agrawal
In-Reply-To: <1978706.bfODzZLsTC@xps13>
Hi Thomas,
On Tuesday 04 October 2016 01:12 PM, Thomas Monjalon wrote:
> 2016-10-04 12:21, Shreyansh Jain:
>> Hi Thomas,
>>
>> On Monday 03 October 2016 07:58 PM, Thomas Monjalon wrote:
>>> Applied, thanks everybody for the great (re)work!
>>
>> Thanks!
>>
[...]
[...]
>>
>> It can be merged with changes for:
>> - drv_name
>> - EAL_ before _REGISTER_ macros
>> - eth_driver => rte_driver naming
>
> Good.
> Could you make it this week, please?
>
Certainly. At least some of those I can send within this week :)
-
Shreyansh
^ permalink raw reply
* Re: [PATCH v2 09/12] virtio: add Rx checksum offload support
From: Olivier Matz @ 2016-10-05 11:56 UTC (permalink / raw)
To: Maxime Coquelin, dev, yuanhan.liu
Cc: konstantin.ananyev, sugesh.chandran, bruce.richardson,
jianfeng.tan, helin.zhang, adrien.mazarguil, stephen, dprovan,
xiao.w.wang
In-Reply-To: <1da0b2c2-931b-3164-d211-4ceee7fd6864@redhat.com>
Hi Maxime,
On 10/03/2016 02:51 PM, Maxime Coquelin wrote:
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -50,6 +50,7 @@
>> #include <rte_string_fns.h>
>> #include <rte_errno.h>
>> #include <rte_byteorder.h>
>> +#include <rte_net.h>
>>
>> #include "virtio_logs.h"
>> #include "virtio_ethdev.h"
>> @@ -627,6 +628,56 @@ virtio_update_packet_stats(struct virtnet_stats
>> *stats, struct rte_mbuf *mbuf)
>> }
>> }
>>
>> +/* Optionally fill offload information in structure */
>> +static int
>> +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>> +{
>> + struct rte_net_hdr_lens hdr_lens;
>> + uint32_t hdrlen, ptype;
>> + int l4_supported = 0;
>> +
>> + /* nothing to do */
>> + if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
>> + return 0;
> Maybe we could first check whether offload features were negotiated?
> Doing this, we could return before accessing the header and so avoid a
> cache miss.
Yes, doing this would avoid reading the virtio header when the rx
function is virtio_recv_pkts(). When using virtio_recv_mergeable_pkts(),
it won't have a big impact since we already need to read hdr->num_buffers.
I plan to do something like this in both recv functions:
@@ -854,6 +854,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
**rx_pkts, uint16_t nb_pkts)
int error;
uint32_t i, nb_enqueued;
uint32_t hdr_size;
+ uint64_t features;
struct virtio_net_hdr *hdr;
nb_used = VIRTQUEUE_NUSED(vq);
@@ -872,6 +873,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
**rx_pkts, uint16_t nb_pkts)
nb_rx = 0;
nb_enqueued = 0;
hdr_size = hw->vtnet_hdr_size;
+ features = hw->guest_features;
for (i = 0; i < num ; i++) {
rxm = rcv_pkts[i];
@@ -903,7 +905,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
**rx_pkts, uint16_t nb_pkts)
rte_vlan_strip(rxm);
/* Update offload features */
- if (virtio_rx_offload(rxm, hdr) < 0) {
+ if ((features & VIRTIO_NET_F_GUEST_CSUM) &&
+ virtio_rx_offload(rxm, hdr) < 0) {
virtio_discard_rxbuf(vq, rxm);
rxvq->stats.errors++;
continue;
Thank you for the feedback.
Olivier
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox