* Re: qos: traffic shaping at queue level
From: Dumitrescu, Cristian @ 2016-10-11 14:11 UTC (permalink / raw)
To: Nikhil Jagtap; +Cc: dev@dpdk.org, users@dpdk.org
In-Reply-To: <CANKBMf1KFMhiC+ypK8KPjMjR7hwp=Uu5=9TY8c49hSSrWf7Ruw@mail.gmail.com>
From: Nikhil Jagtap [mailto:nikhil.jagtap@gmail.com]
Sent: Wednesday, October 5, 2016 8:10 AM
To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
Cc: dev@dpdk.org; users@dpdk.org
Subject: Re: qos: traffic shaping at queue level
Hi Cristian,
Thanks for the info. A few more comments/questions inline.
On 3 October 2016 at 23:42, Dumitrescu, Cristian <cristian.dumitrescu@intel.com<mailto:cristian.dumitrescu@intel.com>> wrote:
From: Nikhil Jagtap [mailto:nikhil.jagtap@gmail.com<mailto:nikhil.jagtap@gmail.com>]
Sent: Friday, September 30, 2016 7:12 AM
To: dev@dpdk.org<mailto:dev@dpdk.org>; Dumitrescu, Cristian <cristian.dumitrescu@intel.com<mailto:cristian.dumitrescu@intel.com>>; users@dpdk.org<mailto:users@dpdk.org>
Subject: Re: qos: traffic shaping at queue level
Hi,
Can someone please answer my queries?
I tried using queue weights to distribute traffic-class bandwidth among the child queues, but did not get the desired results.
[Cristian] Can you please describe what issues you see?
[Nikhil] At the end of a 20 minute test, the total number of packets dequeued from the respective queues were not in the ratio 1:5.
In one other test where 4 equal-rate traffic-streams were hitting 4 different queues of the same TC configured with weights 1:2:4:8, I observed that the queue with highest weight had the least number of dequeued packets when in theory it should have been the one with highest packet count.
[Cristian] No idea why you get into this issue … Please keep me posted once you find the root cause of your issue, maybe there is something that we can improve here.
Regards,
Nikhil
On 27 September 2016 at 15:34, Nikhil Jagtap <nikhil.jagtap@gmail.com<mailto:nikhil.jagtap@gmail.com>> wrote:
Hi,
I have a few questions about the hierarchical scheduler. I am taking a simple example here to get a better understanding.
Reference example:
pipe rate = 30 mbps
tc 0 rate = 30 mbps
traffic-type 0 being queued to queue 0, tc 0.
traffic-type 1 being queued to queue 1, tc 0.
Assume traffic-type 0 is being received at the rate of 25 mbps.
Assume traffic-type 1 is also being received at the rate of 25 mbps.
Requirement:
To limit traffic-type 0 to (CIR = 5 mbps, PIR = 30 mbps), AND
limit traffic-type 1 to (CIR = 25 mbps, PIR = 30 mbps).
The questions:
1) I understand that with the scheduler, it is possible to do rate limiting only at the sub-port and pipe levels and not at the individual queue level.
[Cristian] Yes, correct, only subports and pipes own token buckets, with all the pipe traffic classes and queues sharing their pipe token bucket.
Is it possible to achieve rate limiting using the notion of queue weights? For the above example, will assigning weights in 1:5 ratio to the two queues help achieve shaping the two traffic-types at the two different rates?
[Cristian] Yes. However, getting the weight observed accurately relies on all the queues being backlogged (always having packets to dequeue). When a pipe and certain TC is examined for dequeuing, the relative weights are enforced between the queues that have packets at that precise moment in time, with the empty queues being ignored. The fully backlogged scenario is not taking place in practice, and the set of non-empty queues changes over time. As said it the past, having big relative weight ratios between queues helps (1:5 should be good).
[Nikhil] I see. So I guess not having fully backlogged queues could be one of the reasons for the observations I mentioned above where the weights-ratio does not directly translate into rate-ratio. I think I should also mention that there was no pipelining i.e. packet-processing, queueing, dequeing was all being done inline in a run-to-completion model.
a) Would having some kind of pipelining help achieve better rate-ratio? May be say atleast splitting the enqueue and dequeue operations?
b) If pipelining is not an option, what would be the recommended values for enqueue and dequeue packet count in the run-to-completion model? You have mentioned in one of your presentations to use different values for these two. If I go with (enqueue# > dequeue#), don't I run the risk of filling up the scheduler queues and failed enqueues even at rates lower than the scheduler pipe rates? In the other case where (dequeue# > enqueue#), we would end up dequeing all packets that were enqueued every time.
[Cristian]
a) In order to provide determinism for the hierarchical scheduler (e.g. frequent-enough calls of the enqueue and dequeue operations), I recommend dedicating a separate CPU core to run it, as opposed to running a lot of other stuff on the same core, which might result in the scheduler not being called regularly. This requires a pipeline of at least 2x CPU cores, i.e. one running your worker (run-to-completion) which feeds the second core running the scheduler.
b) As documented, for performance reasons, the API is not thread safe, so you need to run enqueue and dequeue of a given port on the same CPU core. Any (enqueue, dequeue) pair with enqueue > dequeue works. For DPDK apps using vector PMD, the burst size is usually 32, then we typically use e.g (32, 28), (32, 24); for apps not using vector PMD, we used in the past (64, 48), (64, 32); recently, in Cisco VPP we used (256, 240), as VPP typical burst size is 256 packets.
2) In continuation to previous question: if queue weights don't help, would it be possible to use metering to achieve rate limiting? Assume we meter individual traffic-types (using CIR-PIR config mentioned above) before queuing it to the scheduler queues. So to achieve the respective queue rates, the dequeuer would be expected to prioritise green packets over yellow.
Looking into the code, the packet color is used as an input to the dropper block, but does not seem to be used anywhere in the scheduler. So I guess it is not possible to prioritise green packets when dequeing?
[Cristian] Packet color is used by Weighted RED (WRED) congestion management scheme on the enqueue side, not on the dequeue side. Once the packet has been enqueued, it cannot be dropped (i.e. every enqueued packet will eventually be dequeued), so rate limiting cannot be enforced on the dequeue side.
Regards,
Nikhil
Thanks.
Nikhil
^ permalink raw reply
* Re: [PATCH v2 09/12] virtio: add Rx checksum offload support
From: Maxime Coquelin @ 2016-10-11 14:04 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: <1475485223-30566-10-git-send-email-olivier.matz@6wind.com>
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 14 ++++----
> drivers/net/virtio/virtio_ethdev.h | 2 +-
> drivers/net/virtio/virtio_rxtx.c | 69 ++++++++++++++++++++++++++++++++++++++
> drivers/net/virtio/virtqueue.h | 1 +
> 4 files changed, 78 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index fa56032..43cb096 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1262,7 +1262,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> eth_dev->data->dev_flags = dev_flags;
>
> /* reset device and negotiate default features */
> - ret = virtio_init_device(eth_dev, VIRTIO_PMD_GUEST_FEATURES);
> + ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
> if (ret < 0)
> return ret;
>
> @@ -1351,13 +1351,10 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> int ret;
>
> PMD_INIT_LOG(DEBUG, "configure");
> + req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
> + if (rxmode->hw_ip_checksum)
> + req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
>
> - if (rxmode->hw_ip_checksum) {
> - PMD_DRV_LOG(ERR, "HW IP checksum not supported");
> - return -EINVAL;
> - }
> -
> - req_features = VIRTIO_PMD_GUEST_FEATURES;
> /* if request features changed, reinit the device */
> if (req_features != hw->req_guest_features) {
> ret = virtio_init_device(dev, req_features);
> @@ -1578,6 +1575,9 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> dev_info->default_txconf = (struct rte_eth_txconf) {
> .txq_flags = ETH_TXQ_FLAGS_NOOFFLOADS
> };
> + dev_info->rx_offload_capa =
> + DEV_RX_OFFLOAD_TCP_CKSUM |
> + DEV_RX_OFFLOAD_UDP_CKSUM;
> }
>
> /*
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index 5d5e788..2fc9218 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -54,7 +54,7 @@
> #define VIRTIO_MAX_RX_PKTLEN 9728
>
> /* Features desired/implemented by this driver. */
> -#define VIRTIO_PMD_GUEST_FEATURES \
> +#define VIRTIO_PMD_DEFAULT_GUEST_FEATURES \
> (1u << VIRTIO_NET_F_MAC | \
> 1u << VIRTIO_NET_F_STATUS | \
> 1u << VIRTIO_NET_F_MQ | \
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 724517e..eda678a 100644
> --- 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;
> +
> + m->ol_flags |= PKT_RX_IP_CKSUM_UNKNOWN;
> +
> + ptype = rte_net_get_ptype(m, &hdr_lens, RTE_PTYPE_ALL_MASK);
> + m->packet_type = ptype;
> + if ((ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP ||
> + (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_UDP ||
> + (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_SCTP)
> + l4_supported = 1;
> +
> + if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> + hdrlen = hdr_lens.l2_len + hdr_lens.l3_len + hdr_lens.l4_len;
> + if (hdr->csum_start <= hdrlen && l4_supported) {
> + m->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> + } else {
> + /* Unknown proto or tunnel, do sw cksum. We can assume
> + * the cksum field is in the first segment since the
> + * buffers we provided to the host are large enough.
> + * In case of SCTP, this will be wrong since it's a CRC
> + * but there's nothing we can do.
> + */
> + uint16_t csum, off;
> +
> + csum = rte_raw_cksum_mbuf(m, hdr->csum_start,
> + rte_pktmbuf_pkt_len(m) - hdr->csum_start);
> + if (csum != 0xffff)
Why don't we do the 1-complement if 0xffff?
> + csum = ~csum;
> + off = hdr->csum_offset + hdr->csum_start;
> + if (rte_pktmbuf_data_len(m) >= off + 1)
> + *rte_pktmbuf_mtod_offset(m, uint16_t *,
> + off) = csum;
> + }
> + } else if (hdr->flags & VIRTIO_NET_HDR_F_DATA_VALID && l4_supported) {
> + m->ol_flags |= PKT_RX_L4_CKSUM_GOOD;
> + }
> +
> + return 0;
> +}
> +
^ permalink raw reply
* Re: [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL
From: Thomas Monjalon @ 2016-10-11 13:57 UTC (permalink / raw)
To: Neil Horman; +Cc: Shreyansh Jain, david.marchand@6wind.com, dev
In-Reply-To: <20161011133824.GA31866@hmsreliant.think-freely.org>
2016-10-11 09:38, Neil Horman:
> This also begs the question in my mind, is it really worth changing the macro?
> I really don't think it is. The registration macros are pretty descriptive as
> they stand, and have already changed 3 or 4 times in the last 6 months, which
> suggests to me that any change here is really just churn more than meaningful
> change. You can make the argument that the name might be more in line with the
> library its implemented in or what not, but in truth, its easy to understand
> what the macros do (in their previous or current incantations), and any change
> that just makes them the same as other macros in their naming is really more
> trouble than its worth.
Neil, the long term goal is to stop having some identifiers which do not
start with RTE_ in our exported .h files.
I think it is a reasonable policy, for a library, to live in a well defined
namespace.
^ permalink raw reply
* Re: [PATCH 1/2] bnx2x: fix maximum PF queues
From: Chas Williams @ 2016-10-11 13:56 UTC (permalink / raw)
To: Rasesh Mody, dev; +Cc: Dept-EngDPDKDev
In-Reply-To: <1475732197-668-1-git-send-email-rasesh.mody@qlogic.com>
On Wed, 2016-10-05 at 22:36 -0700, Rasesh Mody wrote:
> Fix the max number of PF rx/tx queues. Set the value based
> on BNX2X_MAX_RSS_COUNT() rather than hard coding it to 128.
>
> Fixes: 540a211 ("bnx2x: driver core")
>
> Signed-off-by: Rasesh Mody <rasesh.mody@qlogic.com>
> ---
> drivers/net/bnx2x/bnx2x.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c
> index a49a07f..2bb4a84 100644
> --- a/drivers/net/bnx2x/bnx2x.c
> +++ b/drivers/net/bnx2x/bnx2x.c
> @@ -9556,8 +9556,8 @@ static void bnx2x_init_rte(struct bnx2x_softc *sc)
> sc->max_rx_queues = min(BNX2X_VF_MAX_QUEUES_PER_VF,
> sc->igu_sb_cnt);
> } else {
> - sc->max_tx_queues = 128;
> - sc->max_rx_queues = 128;
> + sc->max_rx_queues = BNX2X_MAX_RSS_COUNT(sc);
> + sc->max_tx_queues = sc->max_rx_queues;
> }
> }
Technically, I think max_tx_queues would be max_rx_queues * max_cos.
However, I don't think there are any DPDK applications that would take
advantage of this.
Acked-By: Chas Williams <3chas3@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 14/15] ethdev: Support VFs on the different PCI domains
From: Kamil Rytarowski @ 2016-10-11 13:52 UTC (permalink / raw)
To: Ferruh Yigit, dev
Cc: maciej.czekaj, zyta.szpak, slawomir.rosek, rad, jerin.jacob,
john.mcnamara
In-Reply-To: <95a9ad76-5d2c-d174-7069-e116733ff4af@intel.com>
W dniu 10.10.2016 o 15:27, Ferruh Yigit pisze:
> On 10/10/2016 2:01 PM, Kamil Rytarowski wrote:
>>
>> W dniu 10.10.2016 o 12:19, Ferruh Yigit pisze:
>>> Hi Kamil,
>>>
>>> On 9/30/2016 1:05 PM, Kamil Rytarowski wrote:
>>>> It's possible to have the same numbers for bus, device id and function,
>>>> therefore we need to differentiate on domain.
>>>>
>>>> This enables DPDK with multiple VFs on ThunderX 2-socket hardware.
>>>>
>>>> Signed-off-by: Maciej Czekaj <maciej.czekaj@caviumnetworks.com>
>>>> Signed-off-by: Kamil Rytarowski <kamil.rytarowski@caviumnetworks.com>
>>>> Signed-off-by: Zyta Szpak <zyta.szpak@semihalf.com>
>>>> Signed-off-by: Slawomir Rosek <slawomir.rosek@semihalf.com>
>>>> Signed-off-by: Radoslaw Biernacki <rad@semihalf.com>
>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>> ---
>>>> lib/librte_ether/rte_ethdev.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>>>> index 382c959..01d5fb0 100644
>>>> --- a/lib/librte_ether/rte_ethdev.c
>>>> +++ b/lib/librte_ether/rte_ethdev.c
>>>> @@ -225,7 +225,7 @@ rte_eth_dev_create_unique_device_name(char *name, size_t size,
>>>> {
>>>> int ret;
>>>>
>>>> - ret = snprintf(name, size, "%d:%d.%d",
>>>> + ret = snprintf(name, size, "%d:%d:%d.%d", pci_dev->addr.domain,
>>>> pci_dev->addr.bus, pci_dev->addr.devid,
>>>> pci_dev->addr.function);
>>>> if (ret < 0)
>>>>
>>> Is it possible to separate this patch from patchset, this is a ethdev
>>> patch and it seems not directly related to the rest of the patchset?
>>>
>>> Thanks,
>>> ferruh
>> This patch is directly related with secondary queue set support on
>> ThunderX, but it can be skipped in this chain of patches and applied as
>> a standalone diff.
>>
>> Is disabling this one on patch work sufficient? Of course unless there
>> are no more comments to produce v3 of the original patch chain "Add
>> support for secondary queue set in nicvf thunderx driver".
> I think it is sufficient, at least I don't have any more comment for
> rest of the patchset and it looks good to me.
>
>> Should I resubmit it as a new standalone patch?
> Can you please resubmit just this one patch, so it can be properly reviewed.
>
> Thanks,
> ferruh
>
Hi,
I've performed the needed actions.
This patch should be disabled in the patch-chain and has been resent as
a new one.
^ permalink raw reply
* [PATCH] ethdev: Support VFs on the different PCI domains
From: Kamil Rytarowski @ 2016-10-11 13:49 UTC (permalink / raw)
To: dev
Cc: maciej.czekaj, zyta.szpak, slawomir.rosek, rad, jerin.jacob,
ferruh.yigit, Kamil Rytarowski, Kamil Rytarowski
It's possible to have the same numbers for bus, device id and function,
therefore we need to differentiate on domain.
This enables DPDK with multiple VFs on ThunderX 2-socket hardware.
Signed-off-by: Maciej Czekaj <maciej.czekaj@caviumnetworks.com>
Signed-off-by: Kamil Rytarowski <kamil.rytarowski@caviumnetworks.com>
Signed-off-by: Zyta Szpak <zyta.szpak@semihalf.com>
Signed-off-by: Slawomir Rosek <slawomir.rosek@semihalf.com>
Signed-off-by: Radoslaw Biernacki <rad@semihalf.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
lib/librte_ether/rte_ethdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 382c959..01d5fb0 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -225,7 +225,7 @@ rte_eth_dev_create_unique_device_name(char *name, size_t size,
{
int ret;
- ret = snprintf(name, size, "%d:%d.%d",
+ ret = snprintf(name, size, "%d:%d:%d.%d", pci_dev->addr.domain,
pci_dev->addr.bus, pci_dev->addr.devid,
pci_dev->addr.function);
if (ret < 0)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v2 08/12] app/testpmd: display lro segment size
From: Maxime Coquelin @ 2016-10-11 13:49 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: <1475485223-30566-9-git-send-email-olivier.matz@6wind.com>
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> In csumonly engine, display the value of LRO segment if the
> LRO flag is set.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> app/test-pmd/csumonly.c | 2 ++
> 1 file changed, 2 insertions(+)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v2 07/12] mbuf: new flag for LRO
From: Maxime Coquelin @ 2016-10-11 13:48 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: <1475485223-30566-8-git-send-email-olivier.matz@6wind.com>
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> When receiving coalesced packets in virtio, the original size of the
> segments is provided. This is a useful information because it allows to
> resegment with the same size.
>
> Add a RX new flag in mbuf, that can be set when packets are coalesced by
> a hardware or virtual driver when the m->tso_segsz field is valid and is
> set to the segment size of original packets.
>
> This flag is used in next commits in the virtio pmd.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> doc/guides/rel_notes/release_16_11.rst | 5 +++++
> lib/librte_mbuf/rte_mbuf.c | 2 ++
> lib/librte_mbuf/rte_mbuf.h | 7 +++++++
> 3 files changed, 14 insertions(+)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v2 06/12] app/testpmd: fix checksum stats in csum engine
From: Maxime Coquelin @ 2016-10-11 13:46 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: <1475485223-30566-7-git-send-email-olivier.matz@6wind.com>
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> ---
> app/test-pmd/csumonly.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index d5eb260..8c88ee8 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -679,8 +679,10 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> rx_ol_flags = m->ol_flags;
>
> /* Update the L3/L4 checksum error packet statistics */
> - rx_bad_ip_csum += ((rx_ol_flags & PKT_RX_IP_CKSUM_BAD) != 0);
> - rx_bad_l4_csum += ((rx_ol_flags & PKT_RX_L4_CKSUM_BAD) != 0);
> + if ((rx_ol_flags & PKT_RX_IP_CKSUM_MASK) == PKT_RX_IP_CKSUM_BAD)
> + rx_bad_ip_csum += 1;
> + if ((rx_ol_flags & PKT_RX_L4_CKSUM_MASK) == PKT_RX_L4_CKSUM_BAD)
> + rx_bad_l4_csum += 1;
>
> /* step 1: dissect packet, parsing optional vlan, ip4/ip6, vxlan
> * and inner headers */
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v2 05/12] mbuf: add new Rx checksum mbuf flags
From: Maxime Coquelin @ 2016-10-11 13:43 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: <1475485223-30566-6-git-send-email-olivier.matz@6wind.com>
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Following discussions in [1] and [2], introduce a new bit to
> describe the Rx checksum status in mbuf.
>
> Before this patch, only one flag was available:
> PKT_RX_L4_CKSUM_BAD: L4 cksum of RX pkt. is not OK.
>
> And same for L3:
> PKT_RX_IP_CKSUM_BAD: IP cksum of RX pkt. is not OK.
>
> This had 2 issues:
> - it was not possible to differentiate "checksum good" from
> "checksum unknown".
> - it was not possible for a virtual driver to say "the checksum
> in packet may be wrong, but data integrity is valid".
>
> This patch tries to solve this issue by having 4 states (2 bits)
> for the IP and L4 Rx checksums. New values are:
>
> - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
> -> the application should verify the checksum by sw
> - PKT_RX_L4_CKSUM_BAD: the L4 checksum in the packet is wrong
> -> the application can drop the packet without additional check
> - PKT_RX_L4_CKSUM_GOOD: the L4 checksum in the packet is valid
> -> the application can accept the packet without verifying the
> checksum by sw
> - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
> data, but the integrity of the L4 data is verified.
> -> the application can process the packet but must not verify the
> checksum by sw. It has to take care to recalculate the cksum
> if the packet is transmitted (either by sw or using tx offload)
>
> And same for L3 (replace L4 by IP in description above).
>
> This commit tries to be compatible with existing applications that
> only check the existing flag (CKSUM_BAD).
>
> [1] http://dpdk.org/ml/archives/dev/2016-May/039920.html
> [2] http://dpdk.org/ml/archives/dev/2016-June/040007.html
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> doc/guides/rel_notes/release_16_11.rst | 6 ++++
> lib/librte_mbuf/rte_mbuf.c | 16 +++++++++--
> lib/librte_mbuf/rte_mbuf.h | 51 ++++++++++++++++++++++++++++++++--
> 3 files changed, 68 insertions(+), 5 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_16_11.rst b/doc/guides/rel_notes/release_16_11.rst
> index f29b44c..2aff84c 100644
> --- a/doc/guides/rel_notes/release_16_11.rst
> +++ b/doc/guides/rel_notes/release_16_11.rst
> @@ -60,6 +60,12 @@ New Features
> Added a new function ``rte_raw_cksum_mbuf()`` to process the checksum of
> data embedded in an mbuf chain.
>
> +* **Added new Rx checksum mbuf flags.**
> +
> + Added new Rx checksum flags in mbufs to described more states: unknown,
s/described/describe/
With this typo fixed, it looks good to me:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL
From: Neil Horman @ 2016-10-11 13:38 UTC (permalink / raw)
To: Shreyansh Jain; +Cc: Thomas Monjalon, david.marchand@6wind.com, dev@dpdk.org
In-Reply-To: <cb6fab09-018d-aaf1-8b61-253263bed801@nxp.com>
On Tue, Oct 11, 2016 at 12:06:27PM +0530, Shreyansh Jain wrote:
> On Monday 10 October 2016 06:26 PM, Neil Horman wrote:
> > On Sat, Oct 08, 2016 at 01:00:59PM +0000, Shreyansh Jain wrote:
> > > Hi Thomas,
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > Sent: Friday, October 07, 2016 7:22 PM
> > > > To: Shreyansh Jain <shreyansh.jain@nxp.com>
> > > > Cc: david.marchand@6wind.com; dev@dpdk.org
> > > > Subject: Re: [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL
> > > >
> > > > 2016-10-07 19:11, Shreyansh Jain:
> > > > > --- a/mk/internal/rte.compile-pre.mk
> > > > > +++ b/mk/internal/rte.compile-pre.mk
> > > > > @@ -87,7 +87,7 @@ endif
> > > > > PMDINFO_GEN = $(RTE_SDK_BIN)/app/dpdk-pmdinfogen $@ $@.pmd.c
> > > > > PMDINFO_CC = $(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@.pmd.o $@.pmd.c
> > > > > PMDINFO_LD = $(CROSS)ld $(LDFLAGS) -r -o $@.o $@.pmd.o $@
> > > > > -PMDINFO_TO_O = if grep -q 'DRIVER_REGISTER_.*(.*)' $<; then \
> > > > > +PMDINFO_TO_O = if grep 'EAL_REGISTER_.*(.*)' $<; then \
> > > > > echo "$(if $V,$(PMDINFO_GEN), PMDINFO $@.pmd.c)" && \
> > > > > $(PMDINFO_GEN) && \
> > > > > echo "$(if $V,$(PMDINFO_CC), CC $@.pmd.o)" && \
> > > > >
> > > > > --->8---
> > > > > CC eal_pci_vfio.o
> > > > > PMDINFO eal_pci_vfio.o.pmd.c
> > > > > /bin/sh: 1:
> > > > > /home/shreyansh/build/DPDK/02_dpdk/x86_64-native-linuxapp-gcc/app/dpdk-
> > > > pmdinfogen:
> > > > > not found
> > > > > /home/shreyansh/build/DPDK/02_dpdk/mk/internal/rte.compile-pre.mk:138:
> > > > > recipe for target 'eal_pci_vfio.o' failed
> > > > > --->8---
> > > > >
> > > > > I don't think PMDINFO should be running on eal_pci_vfio file. Isn't it?
> > > >
> > > > Every files are scanned for the pattern.
> > >
> > > Sorry, I should have been clearer in my statement.
> > > I meant, I didn't think eal_pci_vfio.o had anything of interest for the PMD tool and hence the mk files would have skipped over it in absence of a match.
> > > I understand that PMDINFO would run on all files.
> > >
> > Thats incorrect, the Makefile does a REGEX search for appropriate registration
> > macros that imply the need for pmdinfo to run. If its running on an
> > inappropriate file its because your new macros inadvertently match the current
> > regex, hence my suggestion that the regex should be tuned to be more specific
>
> Agree. Thats is what I wanted to clarify as stated below: "...EAL_REGISTER_*
> (macro name has changed since) is matching EAL_REGISTER_TAILQ..".
>
> As for 'more specific' match - I did suggest [2] a longer more specific
> version but Thomas had a different view point [1]. You can have a look at
> [2] and let me know your suggestion or if that is wrong.
>
> [1] http://dpdk.org/ml/archives/dev/2016-October/048425.html
> [2] http://dpdk.org/ml/archives/dev/2016-October/048407.html
>
Right, so I'm on the same page as Thomas, in that the "RTE_PMD_REGISTER_.*("
regex is sufficient in the current state. However, your patches are changing
the registration macros from something that is sufficiently unique for pmdinfo
to detect to something that collides with an inappropriate match (the
EAL_REGISTER_TAILQ macro), and so you have to make the regex more specific.
This also begs the question in my mind, is it really worth changing the macro?
I really don't think it is. The registration macros are pretty descriptive as
they stand, and have already changed 3 or 4 times in the last 6 months, which
suggests to me that any change here is really just churn more than meaningful
change. You can make the argument that the name might be more in line with the
library its implemented in or what not, but in truth, its easy to understand
what the macros do (in their previous or current incantations), and any change
that just makes them the same as other macros in their naming is really more
trouble than its worth.
Neil
Neil
> >
> > Neil
> >
> > > >
> > > > > Is it because EAL_REGISTER_* is matching EAL_REGISTER_TAILQ like macro
> > > > > as well?
> > > >
> > > > Probably.
> > > > That's another argument in favor of good prefixes.
> > > > I think you should use RTE_DRIVER_REGISTER_ or better, RTE_PMD_REGISTER_
> > >
> > > I thought of EAL_PMD_REGISTER_* but dropped it because for PCI_TABLE like macros, the name got really long (EAL_PMD_REGISTER_PCI_TABLE()).
> > > Anyways, I will use RTE_PMD_REGISTER_* now and send another version.
> > >
> > > > >
> > > > > I am not very well versed with how PMDINFO is linking with the build
> > > > > system so any hints/insight into this would be really helpful.
> > > > >
> > > > > One I get more clarity on this, I will push a new version of this patch.
> > > >
> > > > Thanks
> > >
> > > -
> > > Shreyansh
> > >
> >
>
> -
> Shreyansh
>
^ permalink raw reply
* Re: [PATCH v2 04/12] net: add function to calculate a checksum in a mbuf
From: Olivier MATZ @ 2016-10-11 13:33 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: <b2d7d636-657d-b7da-80ce-97ca1300911e@redhat.com>
Hi Maxime,
On 10/11/2016 03:25 PM, Maxime Coquelin wrote:
>> /**
>> + * Compute the raw (non complemented) checksum of a packet.
>> + *
>> + * @param m
>> + * The pointer to the mbuf.
>> + * @param off
>> + * The offset in bytes to start the checksum.
>> + * @param len
>> + * The length in bytes of the data to ckecksum.
>> + */
>> +static inline uint16_t
>> +rte_raw_cksum_mbuf(const struct rte_mbuf *m, uint32_t off, uint32_t len)
>> +{
>> + const struct rte_mbuf *seg;
>> + const char *buf;
>> + uint32_t sum, tmp;
>> + uint32_t seglen, done;
>> +
>> + /* easy case: all data in the first segment */
>> + if (off + len <= rte_pktmbuf_data_len(m))
>> + return rte_raw_cksum(rte_pktmbuf_mtod_offset(m,
>> + const char *, off), len);
>> +
>> + if (off + len > rte_pktmbuf_pkt_len(m))
> unlikely?
Yes, will add it.
>> + return 0; /* invalid params, return a dummy value */
> Couldn't be better to return an error, so that the caller has a chance
> to see it is passing wrong arguments?
> The csum would be passed as an arg.
Looks much better indeed. I'll change it for next revision.
Thanks,
Olivier
^ permalink raw reply
* Re: [PATCH v2 04/12] net: add function to calculate a checksum in a mbuf
From: Maxime Coquelin @ 2016-10-11 13:25 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: <1475485223-30566-5-git-send-email-olivier.matz@6wind.com>
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> This function can be used to calculate the checksum of data embedded in
> mbuf, that can be composed of several segments.
>
> This function will be used by the virtio pmd in next commits to calculate
> the checksum in software in case the protocol is not recognized.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> doc/guides/rel_notes/release_16_11.rst | 5 +++
> lib/librte_net/rte_ip.h | 60 ++++++++++++++++++++++++++++++++++
> 2 files changed, 65 insertions(+)
>
> diff --git a/doc/guides/rel_notes/release_16_11.rst b/doc/guides/rel_notes/release_16_11.rst
> index 3d3c417..f29b44c 100644
> --- a/doc/guides/rel_notes/release_16_11.rst
> +++ b/doc/guides/rel_notes/release_16_11.rst
> @@ -55,6 +55,11 @@ New Features
> Added two new functions ``rte_get_rx_ol_flag_list()`` and
> ``rte_get_tx_ol_flag_list()`` to dump offload flags as a string.
>
> +* **Added a functions to calculate the checksum of data in a mbuf.**
> +
> + Added a new function ``rte_raw_cksum_mbuf()`` to process the checksum of
> + data embedded in an mbuf chain.
> +
> Resolved Issues
> ---------------
>
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 5b7554a..8499356 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -230,6 +230,66 @@ rte_raw_cksum(const void *buf, size_t len)
> }
>
> /**
> + * Compute the raw (non complemented) checksum of a packet.
> + *
> + * @param m
> + * The pointer to the mbuf.
> + * @param off
> + * The offset in bytes to start the checksum.
> + * @param len
> + * The length in bytes of the data to ckecksum.
> + */
> +static inline uint16_t
> +rte_raw_cksum_mbuf(const struct rte_mbuf *m, uint32_t off, uint32_t len)
> +{
> + const struct rte_mbuf *seg;
> + const char *buf;
> + uint32_t sum, tmp;
> + uint32_t seglen, done;
> +
> + /* easy case: all data in the first segment */
> + if (off + len <= rte_pktmbuf_data_len(m))
> + return rte_raw_cksum(rte_pktmbuf_mtod_offset(m,
> + const char *, off), len);
> +
> + if (off + len > rte_pktmbuf_pkt_len(m))
unlikely?
> + return 0; /* invalid params, return a dummy value */
Couldn't be better to return an error, so that the caller has a chance
to see it is passing wrong arguments?
The csum would be passed as an arg.
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v2 03/12] virtio: reinitialize the device in configure callback
From: Maxime Coquelin @ 2016-10-11 13:13 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: <1475485223-30566-4-git-send-email-olivier.matz@6wind.com>
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Add the ability to reset the virtio device in the configure callback
> if the features flag changed since previous reset. This will be possible
> with the introduction of offload support in next commits.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++++++-------
> drivers/net/virtio/virtio_pci.h | 1 +
> 2 files changed, 20 insertions(+), 7 deletions(-)
Looks good to me.
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v3 0/7] vhost: add dequeue zero copy support
From: Xu, Qian Q @ 2016-10-11 13:04 UTC (permalink / raw)
To: Yuanhan Liu, dev@dpdk.org; +Cc: Maxime Coquelin
In-Reply-To: <1475998080-4644-1-git-send-email-yuanhan.liu@linux.intel.com>
Tested-by: Qian Xu <qian.q.xu@intel.com>
- Apply patch to dpdk-next-virtio: Pass
- Compile: Pass
- OS: Ubuntu16.04 4.4.0-34-generic
- GCC: 5.4.0
Test Case - Pass, over 20% performance gain for big packet(1024B), and it's designed to improve big packet performance.
- Test case: Without NIC, Vhost dequeuer, virtio TXonly, mergeable=on, then see ~28% performance gains for packet size 1518B; for small packet 64B, similar performance as zero-copy=0.
- Test case: With Intel FVL 40G NIC, run PVP case, txd=128, mergeable=on, for packet size over 1K(1024B), we can see the performance benefits, for example, 1024 will get 18% performance gains; 1518B will get 26% performance gain compared with zero-copy=0, for small packet such as 64B, we will get 15% performance drop which is reasonable, and vhost zero-copy is not applicable for the small packet performance.
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yuanhan Liu
Sent: Sunday, October 9, 2016 8:28 AM
To: dev@dpdk.org
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
Subject: [dpdk-dev] [PATCH v3 0/7] vhost: add dequeue zero copy support
This patch set enables vhost dequeue zero copy. The majority work goes to patch 4: "vhost: add dequeue zero copy".
The basic idea of dequeue zero copy is, instead of copying data from the desc buf, here we let the mbuf reference the desc buf addr directly.
The major issue behind that is how and when to update the used ring.
You could check the commit log of patch 4 for more details.
Patch 5 introduces a new flag, RTE_VHOST_USER_DEQUEUE_ZERO_COPY, to enable dequeue zero copy, which is disabled by default.
The performance gain is quite impressive. For a simple dequeue workload (running rxonly in vhost-pmd and runnin txonly in guest testpmd), it yields
50+% performance boost for packet size 1500B. For VM2VM iperf test case,
it's even better: about 70% boost.
For small packets, the performance is worse (it's expected, as the extra overhead introduced by zero copy outweighs the benefits from saving few bytes copy).
v3: - rebase: mainly for removing conflicts with the Tx indirect patch
- don't update last_used_idx twice for zero-copy mode
- handle two mssiing "Tx -> dequeue" renames in log and usage
v2: - renamed "tx zero copy" to "dequeue zero copy", to reduce confusions.
- hnadle the case that a desc buf might across 2 host phys pages
- use MAP_POPULATE to let kernel populate the page tables
- updated release note
- doc-ed the limitations for the vm2nic case
- merge 2 continuous guest phys memory region
- and few more trivial changes, please see them in the corresponding
patches
---
Yuanhan Liu (7):
vhost: simplify memory regions handling
vhost: get guest/host physical address mappings
vhost: introduce last avail idx for dequeue
vhost: add dequeue zero copy
vhost: add a flag to enable dequeue zero copy
examples/vhost: add an option to enable dequeue zero copy
net/vhost: add an option to enable dequeue zero copy
doc/guides/prog_guide/vhost_lib.rst | 35 +++-
doc/guides/rel_notes/release_16_11.rst | 13 ++
drivers/net/vhost/rte_eth_vhost.c | 13 ++
examples/vhost/main.c | 19 +-
lib/librte_vhost/rte_virtio_net.h | 1 +
lib/librte_vhost/socket.c | 5 +
lib/librte_vhost/vhost.c | 12 ++
lib/librte_vhost/vhost.h | 102 ++++++++---
lib/librte_vhost/vhost_user.c | 315 ++++++++++++++++++++++-----------
lib/librte_vhost/virtio_net.c | 196 +++++++++++++++++---
10 files changed, 549 insertions(+), 162 deletions(-)
--
1.9.0
^ permalink raw reply
* Re: [PATCH v2 02/12] virtio: setup and start cq in configure callback
From: Maxime Coquelin @ 2016-10-11 12:47 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: <1475485223-30566-3-git-send-email-olivier.matz@6wind.com>
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Move the configuration of control queue in the configure callback.
> This is needed by next commit, which introduces the reinitialization
> of the device in the configure callback to change the feature flags.
> Therefore, the control queue will have to be restarted at the same
> place.
>
> As virtio_dev_cq_queue_setup() is called from a place where
> config->max_virtqueue_pairs is not available, we need to store this in
> the private structure. It replaces max_rx_queues and max_tx_queues which
> have the same value. The log showing the value of max_rx_queues and
> max_tx_queues is also removed since config->max_virtqueue_pairs is
> already displayed above.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 43 +++++++++++++++++++-------------------
> drivers/net/virtio/virtio_ethdev.h | 4 ++--
> drivers/net/virtio/virtio_pci.h | 3 +--
> 3 files changed, 24 insertions(+), 26 deletions(-)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v2 01/12] virtio: move device initialization in a function
From: Maxime Coquelin @ 2016-10-11 12:30 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: <1475485223-30566-2-git-send-email-olivier.matz@6wind.com>
Hi Olivier,
On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Move all code related to device initialization in a new function
> virtio_init_device().
>
> This commit brings no functional change, it prepares the next commits
> that will add the offload support. For that, it will be needed to
> reinitialize the device from ethdev->configure(), using this new
> function.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 99 ++++++++++++++++++++++----------------
> 1 file changed, 58 insertions(+), 41 deletions(-)
Makes sense, feel free to add my:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v4] drivers/net:new PMD using tun/tap host interface
From: Ferruh Yigit @ 2016-10-11 12:28 UTC (permalink / raw)
To: Keith Wiles, dev; +Cc: pmatilai, yuanhan.liu
In-Reply-To: <1475592311-25749-1-git-send-email-keith.wiles@intel.com>
On 10/4/2016 3:45 PM, Keith Wiles wrote:
> +/*
> + * Open a TAP interface device.
> + */
> +static int
> +rte_pmd_tap_devinit(const char *name, const char *params)
> +{
> + int ret = 0;
> + struct rte_kvargs *kvlist;
> + struct tap_info tap_info;
> +
> + /* Setup default values */
> + memset(&tap_info, 0, sizeof(tap_info));
> +
> + tap_info.speed = ETH_SPEED_NUM_10G;
> + snprintf(tap_info.name, sizeof(tap_info.name), "dtap%d", tap_unit++);
> +
> + if ((params == NULL) || (params[0] == '\0')) {
> + RTE_LOG(INFO, PMD, "Initializing pmd_tap for %s\n", name);
> +
> + ret = eth_dev_tap_create(name, &tap_info);
> + goto leave;
> + }
> +
> + RTE_LOG(INFO, PMD, "Initialize %s with params (%s)\n", name, params);
> +
> + kvlist = rte_kvargs_parse(params, valid_arguments);
> + if (!kvlist) {
> + ret = eth_dev_tap_create(name, &tap_info);
> + goto leave;
> + }
> +
> + if (rte_kvargs_count(kvlist, ETH_TAP_SPEED_ARG) == 1) {
> + ret = rte_kvargs_process(kvlist, ETH_TAP_SPEED_ARG,
> + &set_interface_speed, &tap_info);
> + if (ret < 0)
> + goto leave;
> + } else
> + set_interface_speed(NULL, NULL, &tap_info);
> +
> + if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
> + ret = rte_kvargs_process(kvlist, ETH_TAP_IFACE_ARG,
> + &set_interface_name, &tap_info);
> + if (ret < 0)
> + goto leave;
> + } else
> + set_interface_name(NULL, NULL, (void *)&tap_info);
Also there must be a eth_dev_tap_create() call after this point to use
tap_info struct with custom values, right?
"--vdev eth_tap0,iface=foo0" parameter shouldn't be working with this
code, right?
> +
> + rte_kvargs_free(kvlist);
> +
> +leave:
> + if (ret == -1)
> + RTE_LOG(INFO, PMD, "Failed to create pmd_tap for %s\n", name);
> +
> + return ret;
> +}
^ permalink raw reply
* Re: [PATCH v2 00/12] net/virtio: add offload support
From: Olivier MATZ @ 2016-10-11 12:14 UTC (permalink / raw)
To: Yuanhan Liu
Cc: dev, konstantin.ananyev, sugesh.chandran, bruce.richardson,
jianfeng.tan, helin.zhang, adrien.mazarguil, stephen, dprovan,
xiao.w.wang
In-Reply-To: <20161011113555.GC16751@yliu-dev.sh.intel.com>
Hi Yuanhan,
On 10/11/2016 01:35 PM, Yuanhan Liu wrote:
> Hi,
>
> Firstly, apologize for so late review. It's been forgotten :(
>
> BTW, please feel free to ping me in future if I made no response
> in one or two weeks!
>
> I haven't reviewed it carefully yet (something I will do tomorrow).
> Before that, few quick questions.
>
> Firstly, would you write down some test steps? Honestly, I'm not
> quite sure how that works without the TCP/IP stack.
Not sure I'm getting your question.
The test plan described in the cover letter works without any dpdk
tcp/ip stack. It uses testpmd, which is able to bridge packets and ask
for TCP segmentation.
> On Mon, Oct 03, 2016 at 11:00:11AM +0200, Olivier Matz wrote:
>> This patchset, targetted for 16.11, introduces the support of rx and tx
>> offload in virtio pmd. To achieve this, some new mbuf flags must be
>> introduced, as discussed in [1].
>>
>> It applies on top of:
>> - software packet type [2]
>> - testpmd enhancements [3]
>
> I didn't do the search. Have the two got merged?
As of now, it's not merged yet. I think Thomas is on it.
Regards,
Olivier
^ permalink raw reply
* Re: [PATCH v4] drivers/net:new PMD using tun/tap host interface
From: Ferruh Yigit @ 2016-10-11 11:49 UTC (permalink / raw)
To: Keith Wiles, dev; +Cc: pmatilai, yuanhan.liu
In-Reply-To: <1475592311-25749-1-git-send-email-keith.wiles@intel.com>
On 10/4/2016 3:45 PM, Keith Wiles wrote:
> The rte_eth_tap.c PMD creates a device using TUN/TAP interfaces
> on the local host. The PMD allows for DPDK and the host to
> communicate using a raw device interface on the host and in
> the DPDK application. The device created is a Tap device with
> a L2 packet header.
>
> v4 - merge with latest driver changes
> v3 - fix includes by removing ifdef for other type besides Linux.
> Fix the copyright notice in the Makefile
> v2 - merge all of the patches into one patch.
> Fix a typo on naming the tap device.
> Update the maintainers list
>
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
> MAINTAINERS | 5 +
> config/common_linuxapp | 2 +
> doc/guides/nics/tap.rst | 84 ++++
> drivers/net/Makefile | 1 +
> drivers/net/tap/Makefile | 57 +++
> drivers/net/tap/rte_eth_tap.c | 866 ++++++++++++++++++++++++++++++++
> drivers/net/tap/rte_pmd_tap_version.map | 4 +
> mk/rte.app.mk | 1 +
> 8 files changed, 1020 insertions(+)
> create mode 100644 doc/guides/nics/tap.rst
> create mode 100644 drivers/net/tap/Makefile
> create mode 100644 drivers/net/tap/rte_eth_tap.c
> create mode 100644 drivers/net/tap/rte_pmd_tap_version.map
>
<>
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index 2483dfa..59a2053 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -44,3 +44,5 @@ CONFIG_RTE_LIBRTE_PMD_VHOST=y
> CONFIG_RTE_LIBRTE_PMD_AF_PACKET=y
> CONFIG_RTE_LIBRTE_POWER=y
> CONFIG_RTE_VIRTIO_USER=y
> +CONFIG_RTE_LIBRTE_PMD_TAP=y
According existing config items, a default value of a config option
should go to config/common_base, and environment specific config file
overwrites it if required.
So this option needs to be added into config/common_base too as disabled
by default.
> +CONFIG_RTE_PMD_TAP_MAX_QUEUES=32
Is the number of max queues really needs to be a config option, I assume
in normal use case user won't need to update this and will use single
queue, if that is true what about pushing this into source code to not
make config file more complex?
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
<...>
> +.. code-block:: console
> +
> + The interfaced name can be changed by adding the iface=foo0
> + e.g. --vedv=eth_tap,iface=foo0 --vdev=eth_tap,iface=foo1, ...
s/vedv/vdev
eth_tap needs to be net_tap as part of unifying device names work
<...>
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index bc93230..b4afa98 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -55,6 +55,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD) += thunderx
> DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
> DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
> DIRS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += xenvirt
> +DIRS-$(CONFIG_RTE_LIBRTE_PMD_TAP) += tap
Rest of the PMDs sorted alphabetically, please do same.
>
> ifeq ($(CONFIG_RTE_LIBRTE_VHOST),y)
> DIRS-$(CONFIG_RTE_LIBRTE_PMD_VHOST) += vhost
<...>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
<...>
> +
> +static const char *drivername = "Tap PMD";
> +static int tap_unit = 0;
No need to initialize to zero.
<...>
> +
> +struct pmd_internals {
> + char name[RTE_ETH_NAME_MAX_LEN]; /* Internal Tap device name */
> + uint16_t nb_queues; /* Number of queues supported */
> + uint16_t pad0;
Why this padding? Is it reserved?
> + struct ether_addr eth_addr; /* Mac address of the device port */
> +
> + int if_index; /* IF_INDEX for the port */
> + int fds[RTE_PMD_TAP_MAX_QUEUES]; /* List of all file descriptors */
> +
> + struct rx_queue rxq[RTE_PMD_TAP_MAX_QUEUES]; /* List of RX queues */
> + struct tx_queue txq[RTE_PMD_TAP_MAX_QUEUES]; /* List of TX queues */
> +};
> +
> +/*
> + * Tun/Tap allocation routine
> + *
> + * name is the number of the interface to use, unless NULL to take the host
> + * supplied name.
> + */
> +static int
> +tun_alloc(char * name)
char *name
<...>
> +
> + /* Always set the fiile descriptor to non-blocking */
s/fiile/file
> + if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) {
> + RTE_LOG(ERR, PMD, "Unable to set to nonblocking\n");
> + perror("F_SETFL, NONBLOCK");
> + goto error;
> + }
> +
> + /* If the name is different that new name as default */
> + if (name && strcmp(name, ifr.ifr_name))
> + strcpy(name, ifr.ifr_name);
What about more secure copy?
> +
> + return fd;
> +
> +error:
> + if (fd > 0)
> + close(fd);
> + return -1;
> +}
> +
<...>
> +
> +static void
> +tap_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> +{
> + struct pmd_internals *internals = dev->data->dev_private;
> +
> + dev_info->driver_name = drivername;
> + dev_info->if_index = internals->if_index;
> + dev_info->max_mac_addrs = 1;
> + dev_info->max_rx_pktlen = (uint32_t)ETHER_MAX_VLAN_FRAME_LEN;
> + dev_info->max_rx_queues = (uint16_t)internals->nb_queues;
> + dev_info->max_tx_queues = (uint16_t)internals->nb_queues;
casting to uint16_t is not requires, it is already uint16_t.
> + dev_info->min_rx_bufsize = 0;
> + dev_info->pci_dev = NULL;
> +}
> +
> +static void
> +tap_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *igb_stats)
igb_stats?
> +{
> + unsigned i, imax;
> + unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
> + unsigned long rx_bytes_total = 0, tx_bytes_total = 0;
> + const struct pmd_internals *internal = dev->data->dev_private;
> +
> + imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
> + internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
> +
> + for (i = 0; i < imax; i++) {
> + igb_stats->q_ipackets[i] = internal->rxq[i].stats.ipackets;
> + igb_stats->q_ibytes[i] = internal->rxq[i].stats.ibytes;
> + rx_total += igb_stats->q_ipackets[i];
> + rx_bytes_total += igb_stats->q_ibytes[i];
> + }
> +
> + imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS) ?
> + internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS;
Do we need to duplicate imax calculation?
> +
> + for (i = 0; i < imax; i++) {
> + igb_stats->q_opackets[i] = internal->txq[i].stats.opackets;
> + igb_stats->q_errors[i] = internal->txq[i].stats.errs;
> + igb_stats->q_obytes[i] = internal->txq[i].stats.obytes;
> + tx_total += igb_stats->q_opackets[i];
> + tx_err_total += igb_stats->q_errors[i];
> + tx_bytes_total += igb_stats->q_obytes[i];
> + }
> +
> + igb_stats->ipackets = rx_total;
> + igb_stats->ibytes = rx_bytes_total;
> + igb_stats->opackets = tx_total;
> + igb_stats->oerrors = tx_err_total;
> + igb_stats->obytes = tx_bytes_total;
> +}
> +
<...>
> +
> +static int
> +rte_eth_dev_create(const char *name,
> + struct rte_eth_dev **eth_dev,
> + const struct eth_dev_ops *dev_ops,
> + void **internals, size_t internal_size,
> + uint16_t flag)
> +{
> + char buff[RTE_ETH_NAME_MAX_LEN];
> + int numa_node = rte_socket_id();
> + struct rte_eth_dev *dev = NULL;
> + struct rte_eth_dev_data *data = NULL;
> + void *priv = NULL;
> +
> + if ((name == NULL) || (eth_dev == NULL) || (dev_ops == NULL) ||
> + (internals == NULL) || (internal_size == 0)) {
> + RTE_PMD_DEBUG_TRACE("Paramters are invalid\n");
> + return -1;
> + }
> +
> + dev = rte_eth_dev_allocate(name);
> + if (dev == NULL) {
> + RTE_PMD_DEBUG_TRACE("%s: rte_eth_dev_allocate failed for %s\n",
> + name, buff);
> + goto error;
> + }
> +
> + if (flag & RTE_USE_PRIVATE_DATA) {
You may need to save this flag value somewhere in internals, to decide
how to free data later.
> + /*
> + * now do all data allocation - for eth_dev structure, dummy
> + * pci driver and internal (private) data
> + */
> + snprintf(buff, sizeof(buff), "D-%s-%d", name, numa_node);
> + data = rte_zmalloc_socket(buff, sizeof(struct rte_eth_dev_data),
> + 0, numa_node);
> + if (data == NULL) {
> + RTE_PMD_DEBUG_TRACE("%s: Unable to allocate memory\n",
> + name);
> + goto error;
> + }
> + /* move the current state of the structure to the new one */
> + rte_memcpy(data, dev->data, sizeof(struct rte_eth_dev_data));
Why do we need to copy, trying to preserve which data?
> + dev->data = data; /* Override the current data pointer */
> + } else
> + data = dev->data;
> +
> + snprintf(buff, sizeof(buff), "I-%s-%d", name, numa_node);
> + priv = rte_zmalloc_socket(buff, internal_size, 0, numa_node);
> + if (priv == NULL) {
> + RTE_PMD_DEBUG_TRACE("Unable to allocate internal memory %lu\n",
> + internal_size);
> + goto error;
> + }
> +
> + /* Setup some default values */
> + dev->dev_ops = dev_ops;
> + data->dev_private = priv;
> + data->port_id = dev->data->port_id;
> + memmove(data->name, dev->data->name, strlen(dev->data->name));
These two assignments are useless, needs to be done before "dev->data =
data" assignment.
> +
> + dev->driver = NULL;
> + data->dev_flags = RTE_ETH_DEV_DETACHABLE;
> + data->kdrv = RTE_KDRV_NONE;
> + data->numa_node = numa_node;
> +
> + *eth_dev = dev;
> + *internals = priv;
> +
> + return 0;
> +error:
> + rte_free(priv);
> +
> + if (flag & RTE_USE_PRIVATE_DATA)
> + rte_free(data);
> +
> + rte_eth_dev_release_port(dev);
> +
> + return -1;
> +}
> +
> +static int
> +pmd_init_internals(const char *name, struct tap_info *tap,
> + struct pmd_internals **internals,
> + struct rte_eth_dev **eth_dev)
> +{
> + struct rte_eth_dev *dev = NULL;
> + struct pmd_internals *internal = NULL;
> + struct rte_eth_dev_data *data = NULL;
> + int ret, i, fd = -1;
> +
> + RTE_LOG(INFO, PMD,
> + "%s: Create TUN/TAP Ethernet device with %d queues on numa %u\n",
> + name, RTE_PMD_TAP_MAX_QUEUES, rte_socket_id());
> +
> + pmd_link.link_speed = tap->speed;
> +
> + ret = rte_eth_dev_create(tap->name, &dev, &ops,
> + (void **)&internal, sizeof(struct pmd_internals),
Why rte_eth_dev_create() get "void **internals" which requires casting,
but not "struct pmd_internals **internals" ?
> + RTE_USE_PRIVATE_DATA);
> + if (ret < 0)
> + return -1;
> +
> + strncpy(internal->name, tap->name, sizeof(internal->name));
> +
> + internal->nb_queues = RTE_PMD_TAP_MAX_QUEUES;
> +
> + /* Create the first Tap device */
> + if ((fd = tun_alloc(dev->data->name)) < 0) {
> + RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", dev->data->name);
> + rte_free(internal);
rte_free(dev->data); ?
But needs to check RTE_USE_PRIVATE_DATA ..
> + rte_eth_dev_release_port(dev);
> + return -1;
> + }
> +
> + /* Presetup the fds to -1 as being not working */
> + for(i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
> + internal->fds[i] = -1;
> + internal->rxq[i].fd = -1;
> + internal->txq[i].fd = -1;
> + }
> +
> + /* Take the TUN/TAP fd and place in the first location */
> + internal->rxq[0].fd = fd;
> + internal->txq[0].fd = fd;
> + internal->fds[0] = fd;
> +
> + if (pmd_mac_address(fd, dev, &internal->eth_addr) < 0) {
> + rte_free(internal);
rte_free(dev->data); ?
> + rte_eth_dev_release_port(dev);
> + return -1;
> + }
> +
> + data = dev->data;
> +
> + data->dev_link = pmd_link;
> + data->mac_addrs = &internal->eth_addr;
> +
> + data->nb_rx_queues = (uint16_t)internal->nb_queues;
> + data->nb_tx_queues = (uint16_t)internal->nb_queues;
no cast required.
> + data->drv_name = drivername;
> +
> + *eth_dev = dev;
> + *internals = internal;
> +
> + return 0;
> +}
> +
<...>
> +
> +static int
> +set_interface_speed(const char *key __rte_unused,
> + const char *value,
> + void *extra_args __rte_unused)
need to drop __rte_unused for extra_args
> +{
> + struct tap_info *tap = (struct tap_info *)extra_args;
> +
> + pmd_link.link_speed = (value) ? atoi(value) : ETH_SPEED_NUM_10G;
> + tap->speed = pmd_link.link_speed;
> +
> + return 0;
> +}
> +
> +/*
> + * Open a TAP interface device.
> + */
> +static int
> +rte_pmd_tap_devinit(const char *name, const char *params)
> +{
> + int ret = 0;
> + struct rte_kvargs *kvlist;
> + struct tap_info tap_info;
> +
> + /* Setup default values */
> + memset(&tap_info, 0, sizeof(tap_info));
> +
> + tap_info.speed = ETH_SPEED_NUM_10G;
> + snprintf(tap_info.name, sizeof(tap_info.name), "dtap%d", tap_unit++);
What about extracting iface name "dtap" into a macro to make it more
visible.
> +
> + if ((params == NULL) || (params[0] == '\0')) {
> + RTE_LOG(INFO, PMD, "Initializing pmd_tap for %s\n", name);
> +
> + ret = eth_dev_tap_create(name, &tap_info);
This "name" is not used at all (except from RTE_LOG), instead tap->name
is used for interface name, so why carying this variable around?
> + goto leave;
> + }
> +
> + RTE_LOG(INFO, PMD, "Initialize %s with params (%s)\n", name, params);
> +
> + kvlist = rte_kvargs_parse(params, valid_arguments);
> + if (!kvlist) {
> + ret = eth_dev_tap_create(name, &tap_info);
> + goto leave;
> + }
> +
> + if (rte_kvargs_count(kvlist, ETH_TAP_SPEED_ARG) == 1) {
> + ret = rte_kvargs_process(kvlist, ETH_TAP_SPEED_ARG,
> + &set_interface_speed, &tap_info);
> + if (ret < 0)
> + goto leave;
> + } else
> + set_interface_speed(NULL, NULL, &tap_info);
This call is redundant, tap_info already has default speed value set.
> +
> + if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
> + ret = rte_kvargs_process(kvlist, ETH_TAP_IFACE_ARG,
> + &set_interface_name, &tap_info);
> + if (ret < 0)
> + goto leave;
> + } else
> + set_interface_name(NULL, NULL, (void *)&tap_info);
tap_info->name already set to default value (dtap%d), this call is not
required.
> +
> + rte_kvargs_free(kvlist);
> +
> +leave:
> + if (ret == -1)
> + RTE_LOG(INFO, PMD, "Failed to create pmd_tap for %s\n", name);
> +
> + return ret;
> +}
> +
> +/*
> + * detach a TAP device.
> + */
> +static int
> +rte_pmd_tap_devuninit(const char *name)
> +{
> + struct rte_eth_dev *eth_dev = NULL;
> + struct pmd_internals *internals;
> + int i;
> +
> + RTE_LOG(INFO, PMD, "Closing TUN/TAP Ethernet device on numa %u\n",
> + rte_socket_id());
> +
> + if (name == NULL)
This check is redundant, eal layer won't call this function with "name
== NULL"
> + return 0;
> +
> + /* find the ethdev entry */
> + eth_dev = rte_eth_dev_allocated(name);
> + if (eth_dev == NULL)
> + return 0;
> +
> + internals = eth_dev->data->dev_private;
> + for (i = 0; i < internals->nb_queues; i++)
> + if (internals->fds[i] != -1)
> + close(internals->fds[i]);
> +
> + rte_free(eth_dev->data->dev_private);
> + rte_free(eth_dev->data);
data can be shared?
Don't we need a RTE_USE_PRIVATE_DATA flag check?
> +
> + rte_eth_dev_release_port(eth_dev);
> +
> + return 0;
> +}
> +
> +static struct rte_vdev_driver pmd_tap_drv = {
> + .init = rte_pmd_tap_devinit,
> + .uninit = rte_pmd_tap_devuninit,
> +};
> +
> +DRIVER_REGISTER_VDEV(eth_tap, pmd_tap_drv);
name convention is now: "net_tap"
> +DRIVER_REGISTER_PARAM_STRING(eth_tap,
> + "iface=<string>,speed=N");
> diff --git a/drivers/net/tap/rte_pmd_tap_version.map b/drivers/net/tap/rte_pmd_tap_version.map
> new file mode 100644
> index 0000000..61463bf
> --- /dev/null
> +++ b/drivers/net/tap/rte_pmd_tap_version.map
> @@ -0,0 +1,4 @@
> +DPDK_16.11 {
> +
> + local: *;
> +};
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index 1a0095b..bd1d10f 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -129,6 +129,7 @@ ifeq ($(CONFIG_RTE_LIBRTE_VHOST),y)
> _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_VHOST) += -lrte_pmd_vhost
> endif # $(CONFIG_RTE_LIBRTE_VHOST)
> _LDLIBS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += -lrte_pmd_vmxnet3_uio
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_TAP) += -lrte_pmd_tap
please put in alphebetical order
>
> ifeq ($(CONFIG_RTE_LIBRTE_CRYPTODEV),y)
> _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_AESNI_MB) += -lrte_pmd_aesni_mb
>
^ permalink raw reply
* Re: [PATCH v2 00/12] net/virtio: add offload support
From: Yuanhan Liu @ 2016-10-11 11:35 UTC (permalink / raw)
To: Olivier Matz
Cc: dev, konstantin.ananyev, sugesh.chandran, bruce.richardson,
jianfeng.tan, helin.zhang, adrien.mazarguil, stephen, dprovan,
xiao.w.wang
In-Reply-To: <1475485223-30566-1-git-send-email-olivier.matz@6wind.com>
Hi,
Firstly, apologize for so late review. It's been forgotten :(
BTW, please feel free to ping me in future if I made no response
in one or two weeks!
I haven't reviewed it carefully yet (something I will do tomorrow).
Before that, few quick questions.
Firstly, would you write down some test steps? Honestly, I'm not
quite sure how that works without the TCP/IP stack.
On Mon, Oct 03, 2016 at 11:00:11AM +0200, Olivier Matz wrote:
> This patchset, targetted for 16.11, introduces the support of rx and tx
> offload in virtio pmd. To achieve this, some new mbuf flags must be
> introduced, as discussed in [1].
>
> It applies on top of:
> - software packet type [2]
> - testpmd enhancements [3]
I didn't do the search. Have the two got merged?
--yliu
>
> The new mbuf checksum flags are backward compatible for current
> applications that assume that unknown_csum = good_cum (since there
> was only a bad_csum flag). But it the patchset is integrated, we
> should consider updating the PMDs to match the new API for 16.11.
>
> [1] http://dpdk.org/ml/archives/dev/2016-May/039920.html
> [2] http://dpdk.org/ml/archives/dev/2016-October/048073.html
> [3] http://dpdk.org/ml/archives/dev/2016-September/046443.html
>
> changes v1 -> v2
> - change mbuf checksum calculation static inline
> - fix checksum calculation for protocol where csum=0 means no csum
> - move mbuf checksum calculation in librte_net
> - use RTE_MIN() to set max rx/tx queue
> - rebase on top of head
>
> Olivier Matz (12):
> virtio: move device initialization in a function
> virtio: setup and start cq in configure callback
> virtio: reinitialize the device in configure callback
> net: add function to calculate a checksum in a mbuf
> mbuf: add new Rx checksum mbuf flags
> app/testpmd: fix checksum stats in csum engine
> mbuf: new flag for LRO
> app/testpmd: display lro segment size
> virtio: add Rx checksum offload support
> virtio: add Tx checksum offload support
> virtio: add Lro support
> virtio: add Tso support
>
> app/test-pmd/csumonly.c | 8 +-
> doc/guides/rel_notes/release_16_11.rst | 16 ++
> drivers/net/virtio/virtio_ethdev.c | 182 +++++++++++++---------
> drivers/net/virtio/virtio_ethdev.h | 18 +--
> drivers/net/virtio/virtio_pci.h | 4 +-
> drivers/net/virtio/virtio_rxtx.c | 270 ++++++++++++++++++++++++++++++---
> drivers/net/virtio/virtqueue.h | 1 +
> lib/librte_mbuf/rte_mbuf.c | 18 ++-
> lib/librte_mbuf/rte_mbuf.h | 58 ++++++-
> lib/librte_net/rte_ip.h | 60 ++++++++
> 10 files changed, 526 insertions(+), 109 deletions(-)
>
> Test plan
> =========
>
> (not fully replayed on v2, but no major change)
>
> Platform description
> --------------------
>
> guest (dpdk)
> +----------------+
> | |
> | |
> | port0 +-----<---+
> | ixgbe / | |
> | directio | |
> | | |
> | port1 | ^ flow1
> +----------------+ | (flow2 is the reverse)
> | |
> | virtio |
> v |
> +----------------+ |
> | tap0 / | |
> |1.1.1.1 / | |
> |ns-tap / | |
> | / | |
> | / ixgbe2 +------>--+
> | / 1.1.1.2 |
> |/ ns-ixgbe |
> +----------------+
> host (linux, vhost-net)
>
>
> flow1:
> host -(ixgbe)-> guest -(virtio)-> host
> 1.1.1.2 -> 1.1.1.1
>
> flow2:
> host -(virtio)-> guest -(ixgbe)-> host
> 1.1.1.2 -> 1.1.1.1
>
> Host configuration
> ------------------
>
> Start qemu with:
>
> - a ne2k management interface to avoi any conflict with dpdk
> - 2 ixgbe interfaces given to with vm through vfio
> - a virtio net device, connected to a tap interface through vhost-net
>
> /usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm -m 1G -cpu host \
> -smp 3 -serial telnet::40564,server,nowait -serial null \
> -qmp tcp::44340,server,nowait -monitor telnet::49229,server,nowait \
> -device ne2k_pci,mac=de:ad:de:01:02:03,netdev=user.0,addr=03 \
> -netdev user,id=user.0,hostfwd=tcp::34965-:22 \
> -device vfio-pci,host=0000:04:00.0 -device vfio-pci,host=0000:04:00.1 \
> -netdev type=tap,id=vhostnet0,script=no,vhost=on,queues=8 \
> -device virtio-net-pci,netdev=vhostnet0,ioeventfd=on,mq=on,vectors=17 \
> -hda "/path/to/ubuntu-14.04-template.qcow2" \
> -snapshot -vga none -display none
>
> Move the tap interface in a netns, and configure it:
>
> ip netns add ns-tap
> ip netns exec ns-tap ip l set lo up
> ip link set tap0 netns ns-tap
> ip netns exec ns-tap ip l set tap0 down
> ip netns exec ns-tap ip l set addr 02:00:00:00:00:01 dev tap0
> ip netns exec ns-tap ip l set tap0 up
> ip netns exec ns-tap ip a a 1.1.1.1/24 dev tap0
> ip netns exec ns-tap arp -s 1.1.1.2 02:00:00:00:00:00
> ip netns exec ns-tap ip a
>
> Move the ixgbe interface in a netns, and configure it:
>
> IXGBE=ixgbe2
> ip netns add ns-ixgbe
> ip netns exec ns-ixgbe ip l set lo up
> ip link set ${IXGBE} netns ns-ixgbe
> ip netns exec ns-ixgbe ip l set ${IXGBE} down
> ip netns exec ns-ixgbe ip l set addr 02:00:00:00:00:00 dev ${IXGBE}
> ip netns exec ns-ixgbe ip l set ${IXGBE} up
> ip netns exec ns-ixgbe ip a a 1.1.1.2/24 dev ${IXGBE}
> ip netns exec ns-ixgbe arp -s 1.1.1.1 02:00:00:00:00:01
> ip netns exec ns-ixgbe ip a
>
> Guest configuration
> -------------------
>
> List of pci devices:
>
> 00:02.0 Ethernet controller [0200]: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection [8086:10fb] (rev 01)
> 00:03.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL-8029(AS) [10ec:8029]
> 00:04.0 Ethernet controller [0200]: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection [8086:10fb] (rev 01)
> 00:05.0 Ethernet controller [0200]: Red Hat, Inc Virtio network device [1af4:1000]
>
> Compile dpdk:
>
> cd dpdk.org
> make config T=x86_64-native-linuxapp-gcc
> make -j4
>
> Prepare environment:
>
> mkdir -p /mnt/huge
> mount -t hugetlbfs nodev /mnt/huge
> echo 256 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
> modprobe uio_pci_generic
> python tools/dpdk_nic_bind.py -b uio_pci_generic 0000:00:02.0
> python tools/dpdk_nic_bind.py -b uio_pci_generic 0000:00:05.0
>
> Run test
> ========
>
> The test uses iperf to validate connectivity between the 2 netns of the
> host and trough the guest.
>
> Iperf is run with:
>
> # flow1: host -(ixgbe)-> guest -(virtio)-> host
> ip netns exec ns-tap iperf -s
> ip netns exec ns-ixgbe iperf -c 1.1.1.1 -t 10
>
> # flow2: host -(virtio)-> guest -(ixgbe)-> host
> ip netns exec ns-ixgbe iperf -s
> ip netns exec ns-tap iperf -c 1.1.1.2 -t 10
>
> The guest runs testpmd with csum forward engine, its configuration
> depends on the test case.
>
> test1: large packets (lro/tso)
> ------------------------------
>
> Configuration of testpmd:
>
> ./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
> -i --port-topology=chained --disable-hw-vlan-filter \
> --disable-hw-vlan-strip --enable-rx-cksum --enable-lro \
> --crc-strip --txqflags=0
>
> set fwd csum
> tso set 1440 0
> csum set ip hw 0
> csum set tcp hw 0
> tso set 1440 1
> #csum set ip hw 1 # not supported by virtio
> csum set tcp hw 1
> start
>
> Iperf log:
>
> root@ubuntu1404:~# ip netns exec ns-ixgbe iperf -c 1.1.1.1 -t 10
> ------------------------------------------------------------
> Client connecting to 1.1.1.1, TCP port 5001
> TCP window size: 85.0 KByte (default)
> ------------------------------------------------------------
> [ 3] local 1.1.1.2 port 54460 connected with 1.1.1.1 port 5001
> [ ID] Interval Transfer Bandwidth
> [ 3] 0.0-10.0 sec 6.14 GBytes 5.27 Gbits/sec
> root@ubuntu1404:~# ip netns exec ns-tap iperf -c 1.1.1.2 -t 10
> ------------------------------------------------------------
> Client connecting to 1.1.1.2, TCP port 5001
> TCP window size: 85.0 KByte (default)
> ------------------------------------------------------------
> [ 3] local 1.1.1.1 port 58312 connected with 1.1.1.2 port 5001
> [ ID] Interval Transfer Bandwidth
> [ 3] 0.0-10.0 sec 6.70 GBytes 5.76 Gbits/sec
>
> Example of what we see with "set verbose 1" in testpmd:
>
> -- flow1: ixgbe2 -> port0 (ixgbe) -> testpmd -> port1 (virtio) <-> tap0
> port=0, mbuf=0x7f968ad9fdc0, pkt_len=24682, nb_segs=13:
> rx: l2_len=14 ethertype=800 l3_len=20 l4_proto=6 l4_len=32 flags=PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
> tx: m->l2_len=14 m->l3_len=20 m->l4_len=32
> tx: m->tso_segsz=1440
> tx: flags=PKT_TX_IP_CKSUM PKT_TX_L4_NO_CKSUM PKT_TX_TCP_SEG PKT_TX_IPV4
>
> -- flow2: tap0 -> port1 (virtio)-> testpmd -> port0 (ixgbe) -> ixgbe2
> port=1, mbuf=0x7f968acc9f40, pkt_len=42058, nb_segs=21:
> rx: l2_len=14 ethertype=800 l3_len=20 l4_proto=6 l4_len=32 flags=PKT_RX_L4_CKSUM_NONE PKT_RX_IP_CKSUM_UNKNOWN PKT_RX_LRO
> rx: m->lro_segsz=1440
> tx: m->l2_len=14 m->l3_len=20 m->l4_len=32
> tx: m->tso_segsz=1440
> tx: flags=PKT_TX_IP_CKSUM PKT_TX_L4_NO_CKSUM PKT_TX_TCP_SEG PKT_TX_IPV4
>
> test2: hardware checksum only
> -----------------------------
>
> Configuration of testpmd:
>
> ./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
> -i --port-topology=chained --disable-hw-vlan-filter \
> --disable-hw-vlan-strip --enable-rx-cksum --crc-strip --txqflags=0
>
> set fwd csum
> csum set ip hw 0
> csum set tcp hw 0
> csum set tcp hw 1
> start
>
> Iperf log:
>
> root@ubuntu1404:~# ip netns exec ns-ixgbe iperf -c 1.1.1.1 -t 10
> ------------------------------------------------------------
> Client connecting to 1.1.1.1, TCP port 5001
> TCP window size: 85.0 KByte (default)
> ------------------------------------------------------------
> [ 3] local 1.1.1.2 port 54462 connected with 1.1.1.1 port 5001
> [ ID] Interval Transfer Bandwidth
> [ 3] 0.0-10.0 sec 4.49 GBytes 3.86 Gbits/sec
> root@ubuntu1404:~# ip netns exec ns-tap iperf -c 1.1.1.2 -t 10
> ------------------------------------------------------------
> Client connecting to 1.1.1.2, TCP port 5001
> TCP window size: 85.0 KByte (default)
> ------------------------------------------------------------
> [ 3] local 1.1.1.1 port 58314 connected with 1.1.1.2 port 5001
> [ ID] Interval Transfer Bandwidth
> [ 3] 0.0-10.0 sec 6.66 GBytes 5.72 Gbits/sec
>
> Example of what we see with "set verbose 1" in testpmd:
>
> -- flow1: ixgbe2 -> port0 (ixgbe) -> testpmd -> port1 (virtio) <-> tap0
> port=0, mbuf=0x7f0adca89b40, pkt_len=1514, nb_segs=1:
> rx: l2_len=14 ethertype=800 l3_len=20 l4_proto=6 l4_len=32 flags=PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
> tx: m->l2_len=14 m->l3_len=20 m->l4_len=32
> tx: flags=PKT_TX_TCP_CKSUM PKT_TX_IPV4
>
> -- flow2: tap0 -> port1 (virtio)-> testpmd -> port0 (ixgbe) -> ixgbe2
> port=1, mbuf=0x7f0adcb98d80, pkt_len=1514, nb_segs=1:
> rx: l2_len=14 ethertype=800 l3_len=20 l4_proto=6 l4_len=32 flags=PKT_RX_L4_CKSUM_NONE PKT_RX_IP_CKSUM_UNKNOWN
> tx: m->l2_len=14 m->l3_len=20 m->l4_len=32
> tx: flags=PKT_TX_IP_CKSUM PKT_TX_TCP_CKSUM PKT_TX_IPV4
>
> test3: no offload
> -----------------
>
> Configuration of testpmd:
>
> ./build/app/testpmd -l 0,1 --log-level 8 -- --total-num-mbufs=16384 \
> -i --port-topology=chained --disable-hw-vlan-filter --disable-hw-vlan-strip
>
> set fwd csum
> start
>
> Iperf log:
>
> root@ubuntu1404:~# ip netns exec ns-ixgbe iperf -c 1.1.1.1 -t 10
> ------------------------------------------------------------
> Client connecting to 1.1.1.1, TCP port 5001
> TCP window size: 85.0 KByte (default)
> ------------------------------------------------------------
> [ 3] local 1.1.1.2 port 54466 connected with 1.1.1.1 port 5001
> [ ID] Interval Transfer Bandwidth
> [ 3] 0.0-10.0 sec 4.29 GBytes 3.68 Gbits/sec
> root@ubuntu1404:~# ip netns exec ns-tap iperf -c 1.1.1.2 -t 10
> ------------------------------------------------------------
> Client connecting to 1.1.1.2, TCP port 5001
> TCP window size: 85.0 KByte (default)
> ------------------------------------------------------------
> [ 3] local 1.1.1.1 port 58316 connected with 1.1.1.2 port 5001
> [ ID] Interval Transfer Bandwidth
> [ 3] 0.0-10.0 sec 6.66 GBytes 5.72 Gbits/sec
>
> Example of what we see with "set verbose 1" in testpmd:
>
> -- flow1: ixgbe2 -> port0 (ixgbe) -> testpmd -> port1 (virtio) <-> tap0
> port=0, mbuf=0x7faf38b3e700, pkt_len=1514, nb_segs=1:
> rx: l2_len=14 ethertype=800 l3_len=20 l4_proto=6 l4_len=32 flags=PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
> tx: flags=PKT_TX_L4_NO_CKSUM PKT_TX_IPV4
>
> -- flow2: tap0 -> port1 (virtio)-> testpmd -> port0 (ixgbe) -> ixgbe2
> port=1, mbuf=0x7faf38b71500, pkt_len=1514, nb_segs=1:
> rx: l2_len=14 ethertype=800 l3_len=20 l4_proto=6 l4_len=32 flags=PKT_RX_L4_CKSUM_UNKNOWN PKT_RX_IP_CKSUM_UNKNOWN
> tx: flags=PKT_TX_L4_NO_CKSUM PKT_TX_IPV4
>
> --
> 2.8.1
^ permalink raw reply
* Re: [PATCH v4] drivers/net:new PMD using tun/tap host interface
From: Michał Mirosław @ 2016-10-11 11:30 UTC (permalink / raw)
To: Keith Wiles; +Cc: dev, pmatilai, yuanhan.liu
In-Reply-To: <1475592311-25749-1-git-send-email-keith.wiles@intel.com>
2016-10-04 16:45 GMT+02:00, Keith Wiles <keith.wiles@intel.com>:
> The rte_eth_tap.c PMD creates a device using TUN/TAP interfaces
> on the local host. The PMD allows for DPDK and the host to
> communicate using a raw device interface on the host and in
> the DPDK application. The device created is a Tap device with
> a L2 packet header.
[...]
> +static uint16_t
> +pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> +{
> + int len, n;
> + struct rte_mbuf *mbuf;
> + struct rx_queue *rxq = queue;
> + struct pollfd pfd;
> + uint16_t num_rx;
> + unsigned long num_rx_bytes = 0;
> +
> + pfd.events = POLLIN;
> + pfd.fd = rxq->fd;
> + for (num_rx = 0; num_rx < nb_pkts; ) {
> + n = poll(&pfd, 1, 0);
> +
> + if (n <= 0)
> + break;
> +
Considering that syscalls are rather expensive, it would be cheaper to
allocate an mbuf here and free it when read() returns -1 instead of
calling poll() to check whether a packet is waiting. This way you
save a syscall per packet and replace one syscall with one mbuf free
per poll.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: 17.02 Roadmap
From: Pattan, Reshma @ 2016-10-11 10:25 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev@dpdk.org, O'Driscoll, Tim
In-Reply-To: <1998191.9HGrB6oKr3@xps13>
Hi Thomas,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, October 10, 2016 9:43 PM
> To: O'Driscoll, Tim <tim.odriscoll@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] 17.02 Roadmap
>
> Thanks Tim for the interesting inputs.
> Some of them may require a dedicated thread to continue the discussion based
> on some preliminary specifications or drafts.
>
> 2016-10-10 16:13, O'Driscoll, Tim:
> > Elastic Flow Distributor: The Elastic Flow Distributor (EFD) is a flow-based load
> balancing library which scales linearly for both lookup and insert with the
> number of threads or cores. EFD lookup uses a "perfect hashing" scheme where
> only the information needed to compute a key's value (and not the key itself) is
> stored in the lookup table, thus reducing CPU cache storage requirements.
>
> What is the scope of this library? Just apply rte_hash to a flow table?
> Or is it also sending the packets in some queues?
> Does it depend of librte_distributor?
>
> > Extended Stats (Latency and Bit Rate Statistics): Enhance the Extended NIC
> Stats (Xstats) implementation to support the collection and reporting of latency
> and bit rate measurements. Latency statistics will include min, max and average
> latency, and jitter. Bit rate statistics will include peak and average bit rate
> aggregated over a user-defined time period. This will be implemented for IXGBE
> and I40E.
>
> Are they retrieved from hardware or just computed in software?
Computed in software.
> Could we have some drivers hook to compute them in a generic layer?
Since more stats are coming into DPDK , we planned to avoid adding new code into ethdev library. So adding couple of new libraries to deal with future stats.
1)There will be a new stats library which will provide APIs like, stats registration, stats update and get stats .
2) Another latency stats, bitrate library. This new library uses rte_eth rx/tx callbacks to mark Rx timestamp in Rx callback and calculate latency in Tx callback.
The new stats library(1) shall be used by latency stats , bit rate stats (2) and further new stats to register themselves and push their values to stats library.
Stats library get APIs will be called by existing xstats get API of the ethdev library to display all new stats as part of xstats.
Thanks,
Reshma
^ permalink raw reply
* [PATCH v2] app/test: fix crypto mbuf pool size
From: Piotr Azarewicz @ 2016-10-11 10:06 UTC (permalink / raw)
To: fiona.trahe; +Cc: dev
In-Reply-To: <1475840080-180796-1-git-send-email-piotrx.t.azarewicz@intel.com>
This patch fix that created pool for crypto mbufs may be too big in some
environments.
To avoid the issue, mbuf pool is reverted to its previous size.
Moreover, here is added additional small pool with one large mbuf, what
is needed in large data test scenarios.
Fixes: ffbe3be0d4b5 ("app/test: add libcrypto")
Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
---
v2 change:
- changed check if aad.len > MBUF_SIZE
- added assertion check for rte_pktmbuf_alloc
app/test/test_cryptodev.c | 32 +++++++++++++++++++++++++++++---
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 2917454..7d03899 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -60,6 +60,7 @@ static enum rte_cryptodev_type gbl_cryptodev_type;
struct crypto_testsuite_params {
struct rte_mempool *mbuf_pool;
+ struct rte_mempool *large_mbuf_pool;
struct rte_mempool *op_mpool;
struct rte_cryptodev_config conf;
struct rte_cryptodev_qp_conf qp_conf;
@@ -169,7 +170,7 @@ testsuite_setup(void)
/* Not already created so create */
ts_params->mbuf_pool = rte_pktmbuf_pool_create(
"CRYPTO_MBUFPOOL",
- NUM_MBUFS, MBUF_CACHE_SIZE, 0, UINT16_MAX,
+ NUM_MBUFS, MBUF_CACHE_SIZE, 0, MBUF_SIZE,
rte_socket_id());
if (ts_params->mbuf_pool == NULL) {
RTE_LOG(ERR, USER1, "Can't create CRYPTO_MBUFPOOL\n");
@@ -177,6 +178,21 @@ testsuite_setup(void)
}
}
+ ts_params->large_mbuf_pool = rte_mempool_lookup(
+ "CRYPTO_LARGE_MBUFPOOL");
+ if (ts_params->large_mbuf_pool == NULL) {
+ /* Not already created so create */
+ ts_params->large_mbuf_pool = rte_pktmbuf_pool_create(
+ "CRYPTO_LARGE_MBUFPOOL",
+ 1, 0, 0, UINT16_MAX,
+ rte_socket_id());
+ if (ts_params->large_mbuf_pool == NULL) {
+ RTE_LOG(ERR, USER1,
+ "Can't create CRYPTO_LARGE_MBUFPOOL\n");
+ return TEST_FAILED;
+ }
+ }
+
ts_params->op_mpool = rte_crypto_op_pool_create(
"MBUF_CRYPTO_SYM_OP_POOL",
RTE_CRYPTO_OP_TYPE_SYMMETRIC,
@@ -5149,7 +5165,12 @@ test_AES_GMAC_authentication(const struct gmac_test_data *tdata)
if (retval < 0)
return retval;
- ut_params->ibuf = rte_pktmbuf_alloc(ts_params->mbuf_pool);
+ if (tdata->aad.len > MBUF_SIZE)
+ ut_params->ibuf = rte_pktmbuf_alloc(ts_params->large_mbuf_pool);
+ else
+ ut_params->ibuf = rte_pktmbuf_alloc(ts_params->mbuf_pool);
+ TEST_ASSERT_NOT_NULL(ut_params->ibuf,
+ "Failed to allocate input buffer in mempool");
memset(rte_pktmbuf_mtod(ut_params->ibuf, uint8_t *), 0,
rte_pktmbuf_tailroom(ut_params->ibuf));
@@ -5233,7 +5254,12 @@ test_AES_GMAC_authentication_verify(const struct gmac_test_data *tdata)
if (retval < 0)
return retval;
- ut_params->ibuf = rte_pktmbuf_alloc(ts_params->mbuf_pool);
+ if (tdata->aad.len > MBUF_SIZE)
+ ut_params->ibuf = rte_pktmbuf_alloc(ts_params->large_mbuf_pool);
+ else
+ ut_params->ibuf = rte_pktmbuf_alloc(ts_params->mbuf_pool);
+ TEST_ASSERT_NOT_NULL(ut_params->ibuf,
+ "Failed to allocate input buffer in mempool");
memset(rte_pktmbuf_mtod(ut_params->ibuf, uint8_t *), 0,
rte_pktmbuf_tailroom(ut_params->ibuf));
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/4] libcrypto_pmd: initial implementation of SW crypto device
From: Mrozowicz, SlawomirX @ 2016-10-11 9:50 UTC (permalink / raw)
To: Jastrzebski, MichalX K, Akhil Goyal, Azarewicz, PiotrX T,
Doherty, Declan, dev@dpdk.org
Cc: Kerlin, MarcinX, Kobylinski, MichalX, Kulasek, TomaszX,
Mrzyglod, DanielX T
In-Reply-To: <60ABE07DBB3A454EB7FAD707B4BB158213AEEE33@IRSMSX109.ger.corp.intel.com>
>-----Original Message-----
>From: Jastrzebski, MichalX K
>Sent: Wednesday, September 14, 2016 10:48 AM
>To: Akhil Goyal <akhil.goyal@nxp.com>; Azarewicz, PiotrX T
><piotrx.t.azarewicz@intel.com>; Doherty, Declan
><declan.doherty@intel.com>; dev@dpdk.org
>Cc: Kerlin, MarcinX <marcinx.kerlin@intel.com>; Mrozowicz, SlawomirX
><slawomirx.mrozowicz@intel.com>; Kobylinski, MichalX
><michalx.kobylinski@intel.com>; Kulasek, TomaszX
><tomaszx.kulasek@intel.com>; Mrzyglod, DanielX T
><danielx.t.mrzyglod@intel.com>
>Subject: RE: [dpdk-dev] [PATCH 1/4] libcrypto_pmd: initial implementation of
>SW crypto device
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
>> Sent: Monday, September 12, 2016 1:17 PM
>> To: Azarewicz, PiotrX T <piotrx.t.azarewicz@intel.com>; Doherty,
>> Declan <declan.doherty@intel.com>; dev@dpdk.org
>> Cc: Kerlin, MarcinX <marcinx.kerlin@intel.com>; Mrozowicz, SlawomirX
>> <slawomirx.mrozowicz@intel.com>; Kobylinski, MichalX
>> <michalx.kobylinski@intel.com>; Kulasek, TomaszX
>> <tomaszx.kulasek@intel.com>; Mrzyglod, DanielX T
>> <danielx.t.mrzyglod@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 1/4] libcrypto_pmd: initial
>> implementation of SW crypto device
>>
>> On 8/26/2016 12:51 PM, Piotr Azarewicz wrote:
>>
>> > +/** Provide session for operation */ static struct
>> > +libcrypto_session * get_session(struct libcrypto_qp *qp, struct
>> > +rte_crypto_op *op) {
>> > + struct libcrypto_session *sess = NULL;
>> > +
>> > + if (op->sym->sess_type == RTE_CRYPTO_SYM_OP_WITH_SESSION) {
>> > + /* get existing session */
>> > + if (!unlikely(op->sym->session == NULL ||
>> > + op->sym->session->dev_type !=
>> > + RTE_CRYPTODEV_LIBCRYPTO_PMD))
>> > + sess = (struct libcrypto_session *)
>> > + op->sym->session->_private;
>> > + } else {
>> > + /* provide internal session */
>> > + void *_sess = NULL;
>> > +
>> > + if (!rte_mempool_get(qp->sess_mp, (void **)&_sess)) {
>> > + sess = (struct libcrypto_session *)
>> > + ((struct rte_cryptodev_sym_session *)_sess)
>> > + ->_private;
>> > +
>> > + if (unlikely(libcrypto_set_session_parameters(
>> > + sess, op->sym->xform) != 0)) {
>> > + rte_mempool_put(qp->sess_mp, _sess);
>> > + sess = NULL;
>> > + } else
>> > + op->sym->session = _sess;
>> > + }
>> > + }
>> > +
>> > + if (sess == NULL)
>> > + op->status = RTE_CRYPTO_OP_STATUS_INVALID_SESSION;
>> > +
>> > + return sess;
>> > +}
>> > +
>> > +/*
>> > +
>> > +*------------------------------------------------------------------
>> > +------------
>> > + * Process Operations
>> > +
>> > +*------------------------------------------------------------------
>> > +------------
>> > + */
>> > +
>> > +/** Process standard libcrypto cipher encryption */ static int
>> > +process_libcrypto_cipher_encrypt(uint8_t *src, uint8_t *dst,
>> > + uint8_t *iv, uint8_t *key, int srclen,
>> > + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo) {
>> > + int dstlen, totlen;
>> > +
>> > + if (EVP_EncryptInit_ex(ctx, algo, NULL, key, iv) <= 0)
>> > + goto process_cipher_encrypt_err;
>> [Akhil] this EVP_EncryptInit_ex() can be done for each session instead
>> of each packet. This will improve the performance. Also if there is
>> some change in the parameters later then it can be called again here
>> with the updated parameters only.
>> Same comment is for all cases (hmac, auth, etc)
>
>Hi Akhil,
>Thank You for this comment. We will check if this is possible.
>
>Michal
>
[Sławomir]
We have done some investigation:
- At first glance it is valuable to call this kind of function
only once per session.
- We done test: just remove the functions like EVP_XXXInitXXX
and we reduce packet processing time between 1% to 10%
depending of processed algorithm.
- We analyzed calling only once per session also functions
like EVP_XXXFinalXXX but it is not possible because
this kind of functions finalize cipher or authenticate process
and return result.
- The functions like EVP_XXXFinalXXX change context and
It is not possible to proper continue processing without
reinitialize the context. So after call functions EVP_XXXFinalXXX
we need to call EVP_XXXInitXXX for next operation.
In sum we can't do the proposed performance improvement.
>> > +
>> > + if (EVP_EncryptUpdate(ctx, dst, &dstlen, src, srclen) <= 0)
>> > + goto process_cipher_encrypt_err;
>> > +
>> > + if (EVP_EncryptFinal_ex(ctx, dst + dstlen, &totlen) <= 0)
>> > + goto process_cipher_encrypt_err;
>> > +
>> > + return 0;
>> > +
>> > +process_cipher_encrypt_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto cipher encrypt failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process standard libcrypto cipher decryption */ static int
>> > +process_libcrypto_cipher_decrypt(uint8_t *src, uint8_t *dst,
>> > + uint8_t *iv, uint8_t *key, int srclen,
>> > + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo) {
>> > + int dstlen, totlen;
>> > +
>> > + if (EVP_DecryptInit_ex(ctx, algo, NULL, key, iv) <= 0)
>> > + goto process_cipher_decrypt_err;
>> > +
>> > + if (EVP_CIPHER_CTX_set_padding(ctx, 0) <= 0)
>> > + goto process_cipher_decrypt_err;
>> > +
>> > + if (EVP_DecryptUpdate(ctx, dst, &dstlen, src, srclen) <= 0)
>> > + goto process_cipher_decrypt_err;
>> > +
>> > + if (EVP_DecryptFinal_ex(ctx, dst + dstlen, &totlen) <= 0)
>> > + goto process_cipher_decrypt_err;
>> > +
>> > + return 0;
>> > +
>> > +process_cipher_decrypt_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto cipher decrypt failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process cipher des 3 ctr encryption, decryption algorithm */
>> > +static int process_libcrypto_cipher_des3ctr(uint8_t *src, uint8_t
>> > +*dst,
>> > + uint8_t *iv, uint8_t *key, int srclen, EVP_CIPHER_CTX *ctx) {
>> > + uint8_t ebuf[8], ctr[8];
>> > + int unused, n;
>> > +
>> > + /* We use 3DES encryption also for decryption.
>> > + * IV is not important for 3DES ecb
>> > + */
>> > + if (EVP_EncryptInit_ex(ctx, EVP_des_ede3_ecb(), NULL, key, NULL)
>> > +<=
>> 0)
>> > + goto process_cipher_des3ctr_err;
>> > +
>> > + memcpy(ctr, iv, 8);
>> > + n = 0;
>> > +
>> > + while (n < srclen) {
>> > + if (n % 8 == 0) {
>> > + if (EVP_EncryptUpdate(ctx, (unsigned char *)&ebuf,
>> &unused,
>> > + (const unsigned char *)&ctr, 8) <= 0)
>> > + goto process_cipher_des3ctr_err;
>> > + ctr_inc(ctr);
>> > + }
>> > + dst[n] = src[n] ^ ebuf[n % 8];
>> > + n++;
>> > + }
>> > +
>> > + return 0;
>> > +
>> > +process_cipher_des3ctr_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto cipher des 3 ede ctr failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process auth gmac algorithm */
>> > +static int
>> > +process_libcrypto_auth_gmac(uint8_t *src, uint8_t *dst,
>> > + uint8_t *iv, uint8_t *key, int srclen, int ivlen,
>> > + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo) {
>> > + int unused;
>> > +
>> > + if (EVP_EncryptInit_ex(ctx, algo, NULL, NULL, NULL) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_IVLEN, ivlen,
>> NULL) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_EncryptInit_ex(ctx, NULL, NULL, key, iv) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_EncryptUpdate(ctx, NULL, &unused, src, srclen) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_EncryptFinal_ex(ctx, NULL, &unused) <= 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_GET_TAG, 16, dst) <=
>> 0)
>> > + goto process_auth_gmac_err;
>> > +
>> > + return 0;
>> > +
>> > +process_auth_gmac_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto auth gmac failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process standard libcrypto auth algorithms */ static int
>> > +process_libcrypto_auth(uint8_t *src, uint8_t *dst,
>> > + __rte_unused uint8_t *iv, __rte_unused EVP_PKEY * pkey,
>> > + int srclen, EVP_MD_CTX *ctx, const EVP_MD *algo) {
>> > + size_t dstlen;
>> > +
>> > + if (EVP_DigestInit_ex(ctx, algo, NULL) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + if (EVP_DigestUpdate(ctx, (char *)src, srclen) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + if (EVP_DigestFinal_ex(ctx, dst, (unsigned int *)&dstlen) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + return 0;
>> > +
>> > +process_auth_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto auth failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/** Process standard libcrypto auth algorithms with hmac */ static
>> > +int process_libcrypto_auth_hmac(uint8_t *src, uint8_t *dst,
>> > + __rte_unused uint8_t *iv, EVP_PKEY *pkey,
>> > + int srclen, EVP_MD_CTX *ctx, const EVP_MD *algo)
>> > +{
>> > + size_t dstlen;
>> > +
>> > + if (EVP_DigestSignInit(ctx, NULL, algo, NULL, pkey) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + if (EVP_DigestSignUpdate(ctx, (char *)src, srclen) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + if (EVP_DigestSignFinal(ctx, dst, &dstlen) <= 0)
>> > + goto process_auth_err;
>> > +
>> > + return 0;
>> > +
>> > +process_auth_err:
>> > + LIBCRYPTO_LOG_ERR("Process libcrypto auth failed");
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/*-----------------------------------------------------------------
>> > +-----------*/
>> > +
>>
>>
^ 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