* [PATCH] net: introduce big and little endian types
From: Nelio Laranjeiro @ 2016-11-09 15:04 UTC (permalink / raw)
To: dev, Olivier Matz; +Cc: wenzhuo.lu, Adrien Mazarguil
This commit introduces new rte_{le,be}{16,32,64}_t types and updates
rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
accordingly.
Specific big/little endian types avoid uncertainty and conversion mistakes.
No ABI change since these are simply typedefs to the original types.
Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
.../common/include/generic/rte_byteorder.h | 31 +++++++++++-------
lib/librte_net/rte_arp.h | 15 +++++----
lib/librte_net/rte_ether.h | 10 +++---
lib/librte_net/rte_gre.h | 30 ++++++++---------
lib/librte_net/rte_icmp.h | 11 ++++---
lib/librte_net/rte_ip.h | 38 +++++++++++-----------
lib/librte_net/rte_net.c | 10 +++---
lib/librte_net/rte_sctp.h | 9 ++---
lib/librte_net/rte_tcp.h | 19 ++++++-----
lib/librte_net/rte_udp.h | 9 ++---
10 files changed, 97 insertions(+), 85 deletions(-)
diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h
index e00bccb..059c2a5 100644
--- a/lib/librte_eal/common/include/generic/rte_byteorder.h
+++ b/lib/librte_eal/common/include/generic/rte_byteorder.h
@@ -75,6 +75,13 @@
#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
#endif
+typedef uint16_t rte_be16_t;
+typedef uint32_t rte_be32_t;
+typedef uint64_t rte_be64_t;
+typedef uint16_t rte_le16_t;
+typedef uint32_t rte_le32_t;
+typedef uint64_t rte_le64_t;
+
/*
* An internal function to swap bytes in a 16-bit value.
*
@@ -143,65 +150,65 @@ static uint64_t rte_bswap64(uint64_t x);
/**
* Convert a 16-bit value from CPU order to little endian.
*/
-static uint16_t rte_cpu_to_le_16(uint16_t x);
+static rte_le16_t rte_cpu_to_le_16(uint16_t x);
/**
* Convert a 32-bit value from CPU order to little endian.
*/
-static uint32_t rte_cpu_to_le_32(uint32_t x);
+static rte_le32_t rte_cpu_to_le_32(uint32_t x);
/**
* Convert a 64-bit value from CPU order to little endian.
*/
-static uint64_t rte_cpu_to_le_64(uint64_t x);
+static rte_le64_t rte_cpu_to_le_64(uint64_t x);
/**
* Convert a 16-bit value from CPU order to big endian.
*/
-static uint16_t rte_cpu_to_be_16(uint16_t x);
+static rte_be16_t rte_cpu_to_be_16(uint16_t x);
/**
* Convert a 32-bit value from CPU order to big endian.
*/
-static uint32_t rte_cpu_to_be_32(uint32_t x);
+static rte_be32_t rte_cpu_to_be_32(uint32_t x);
/**
* Convert a 64-bit value from CPU order to big endian.
*/
-static uint64_t rte_cpu_to_be_64(uint64_t x);
+static rte_be64_t rte_cpu_to_be_64(uint64_t x);
/**
* Convert a 16-bit value from little endian to CPU order.
*/
-static uint16_t rte_le_to_cpu_16(uint16_t x);
+static uint16_t rte_le_to_cpu_16(rte_le16_t x);
/**
* Convert a 32-bit value from little endian to CPU order.
*/
-static uint32_t rte_le_to_cpu_32(uint32_t x);
+static uint32_t rte_le_to_cpu_32(rte_le32_t x);
/**
* Convert a 64-bit value from little endian to CPU order.
*/
-static uint64_t rte_le_to_cpu_64(uint64_t x);
+static uint64_t rte_le_to_cpu_64(rte_le64_t x);
/**
* Convert a 16-bit value from big endian to CPU order.
*/
-static uint16_t rte_be_to_cpu_16(uint16_t x);
+static uint16_t rte_be_to_cpu_16(rte_be16_t x);
/**
* Convert a 32-bit value from big endian to CPU order.
*/
-static uint32_t rte_be_to_cpu_32(uint32_t x);
+static uint32_t rte_be_to_cpu_32(rte_be32_t x);
/**
* Convert a 64-bit value from big endian to CPU order.
*/
-static uint64_t rte_be_to_cpu_64(uint64_t x);
+static uint64_t rte_be_to_cpu_64(rte_be64_t x);
#endif /* __DOXYGEN__ */
diff --git a/lib/librte_net/rte_arp.h b/lib/librte_net/rte_arp.h
index 1836418..95f123e 100644
--- a/lib/librte_net/rte_arp.h
+++ b/lib/librte_net/rte_arp.h
@@ -40,6 +40,7 @@
#include <stdint.h>
#include <rte_ether.h>
+#include <rte_byteorder.h>
#ifdef __cplusplus
extern "C" {
@@ -50,22 +51,22 @@ extern "C" {
*/
struct arp_ipv4 {
struct ether_addr arp_sha; /**< sender hardware address */
- uint32_t arp_sip; /**< sender IP address */
+ rte_be32_t arp_sip; /**< sender IP address */
struct ether_addr arp_tha; /**< target hardware address */
- uint32_t arp_tip; /**< target IP address */
+ rte_be32_t arp_tip; /**< target IP address */
} __attribute__((__packed__));
/**
* ARP header.
*/
struct arp_hdr {
- uint16_t arp_hrd; /* format of hardware address */
+ rte_be16_t arp_hrd; /* format of hardware address */
#define ARP_HRD_ETHER 1 /* ARP Ethernet address format */
- uint16_t arp_pro; /* format of protocol address */
- uint8_t arp_hln; /* length of hardware address */
- uint8_t arp_pln; /* length of protocol address */
- uint16_t arp_op; /* ARP opcode (command) */
+ rte_be16_t arp_pro; /* format of protocol address */
+ uint8_t arp_hln; /* length of hardware address */
+ uint8_t arp_pln; /* length of protocol address */
+ rte_be16_t arp_op; /* ARP opcode (command) */
#define ARP_OP_REQUEST 1 /* request to resolve address */
#define ARP_OP_REPLY 2 /* response to previous request */
#define ARP_OP_REVREQUEST 3 /* request proto addr given hardware */
diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index ff3d065..159e061 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -300,7 +300,7 @@ ether_format_addr(char *buf, uint16_t size,
struct ether_hdr {
struct ether_addr d_addr; /**< Destination address. */
struct ether_addr s_addr; /**< Source address. */
- uint16_t ether_type; /**< Frame type. */
+ rte_be16_t ether_type; /**< Frame type. */
} __attribute__((__packed__));
/**
@@ -309,8 +309,8 @@ struct ether_hdr {
* of the encapsulated frame.
*/
struct vlan_hdr {
- uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
- uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
+ rte_be16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
+ rte_be16_t eth_proto;/**< Ethernet type of encapsulated frame. */
} __attribute__((__packed__));
/**
@@ -319,8 +319,8 @@ struct vlan_hdr {
* Reserved fields (24 bits and 8 bits)
*/
struct vxlan_hdr {
- uint32_t vx_flags; /**< flag (8) + Reserved (24). */
- uint32_t vx_vni; /**< VNI (24) + Reserved (8). */
+ rte_be32_t vx_flags; /**< flag (8) + Reserved (24). */
+ rte_be32_t vx_vni; /**< VNI (24) + Reserved (8). */
} __attribute__((__packed__));
/* Ethernet frame types */
diff --git a/lib/librte_net/rte_gre.h b/lib/librte_net/rte_gre.h
index 46568ff..b651af0 100644
--- a/lib/librte_net/rte_gre.h
+++ b/lib/librte_net/rte_gre.h
@@ -45,23 +45,23 @@ extern "C" {
*/
struct gre_hdr {
#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
- uint16_t res2:4; /**< Reserved */
- uint16_t s:1; /**< Sequence Number Present bit */
- uint16_t k:1; /**< Key Present bit */
- uint16_t res1:1; /**< Reserved */
- uint16_t c:1; /**< Checksum Present bit */
- uint16_t ver:3; /**< Version Number */
- uint16_t res3:5; /**< Reserved */
+ uint16_t res2:4; /**< Reserved */
+ uint16_t s:1; /**< Sequence Number Present bit */
+ uint16_t k:1; /**< Key Present bit */
+ uint16_t res1:1; /**< Reserved */
+ uint16_t c:1; /**< Checksum Present bit */
+ uint16_t ver:3; /**< Version Number */
+ uint16_t res3:5; /**< Reserved */
#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
- uint16_t c:1; /**< Checksum Present bit */
- uint16_t res1:1; /**< Reserved */
- uint16_t k:1; /**< Key Present bit */
- uint16_t s:1; /**< Sequence Number Present bit */
- uint16_t res2:4; /**< Reserved */
- uint16_t res3:5; /**< Reserved */
- uint16_t ver:3; /**< Version Number */
+ uint16_t c:1; /**< Checksum Present bit */
+ uint16_t res1:1; /**< Reserved */
+ uint16_t k:1; /**< Key Present bit */
+ uint16_t s:1; /**< Sequence Number Present bit */
+ uint16_t res2:4; /**< Reserved */
+ uint16_t res3:5; /**< Reserved */
+ uint16_t ver:3; /**< Version Number */
#endif
- uint16_t proto; /**< Protocol Type */
+ rte_be16_t proto; /**< Protocol Type */
} __attribute__((__packed__));
#ifdef __cplusplus
diff --git a/lib/librte_net/rte_icmp.h b/lib/librte_net/rte_icmp.h
index 8b287f6..81bd907 100644
--- a/lib/librte_net/rte_icmp.h
+++ b/lib/librte_net/rte_icmp.h
@@ -74,6 +74,7 @@
*/
#include <stdint.h>
+#include <rte_byteorder.h>
#ifdef __cplusplus
extern "C" {
@@ -83,11 +84,11 @@ extern "C" {
* ICMP Header
*/
struct icmp_hdr {
- uint8_t icmp_type; /* ICMP packet type. */
- uint8_t icmp_code; /* ICMP packet code. */
- uint16_t icmp_cksum; /* ICMP packet checksum. */
- uint16_t icmp_ident; /* ICMP packet identifier. */
- uint16_t icmp_seq_nb; /* ICMP packet sequence number. */
+ uint8_t icmp_type; /* ICMP packet type. */
+ uint8_t icmp_code; /* ICMP packet code. */
+ rte_be16_t icmp_cksum; /* ICMP packet checksum. */
+ rte_be16_t icmp_ident; /* ICMP packet identifier. */
+ rte_be16_t icmp_seq_nb; /* ICMP packet sequence number. */
} __attribute__((__packed__));
/* ICMP packet types */
diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 4491b86..6f7da36 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -93,14 +93,14 @@ extern "C" {
struct ipv4_hdr {
uint8_t version_ihl; /**< version and header length */
uint8_t type_of_service; /**< type of service */
- uint16_t total_length; /**< length of packet */
- uint16_t packet_id; /**< packet ID */
- uint16_t fragment_offset; /**< fragmentation offset */
+ rte_be16_t total_length; /**< length of packet */
+ rte_be16_t packet_id; /**< packet ID */
+ rte_be16_t fragment_offset; /**< fragmentation offset */
uint8_t time_to_live; /**< time to live */
uint8_t next_proto_id; /**< protocol ID */
- uint16_t hdr_checksum; /**< header checksum */
- uint32_t src_addr; /**< source address */
- uint32_t dst_addr; /**< destination address */
+ rte_be16_t hdr_checksum; /**< header checksum */
+ rte_be32_t src_addr; /**< source address */
+ rte_be32_t dst_addr; /**< destination address */
} __attribute__((__packed__));
/** Create IPv4 address */
@@ -340,11 +340,11 @@ static inline uint16_t
rte_ipv4_phdr_cksum(const struct ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
{
struct ipv4_psd_header {
- uint32_t src_addr; /* IP address of source host. */
- uint32_t dst_addr; /* IP address of destination host. */
- uint8_t zero; /* zero. */
- uint8_t proto; /* L4 protocol type. */
- uint16_t len; /* L4 length. */
+ rte_be32_t src_addr; /* IP address of source host. */
+ rte_be32_t dst_addr; /* IP address of destination host. */
+ uint8_t zero; /* zero. */
+ uint8_t proto; /* L4 protocol type. */
+ rte_be16_t len; /* L4 length. */
} psd_hdr;
psd_hdr.src_addr = ipv4_hdr->src_addr;
@@ -398,12 +398,12 @@ rte_ipv4_udptcp_cksum(const struct ipv4_hdr *ipv4_hdr, const void *l4_hdr)
* IPv6 Header
*/
struct ipv6_hdr {
- uint32_t vtc_flow; /**< IP version, traffic class & flow label. */
- uint16_t payload_len; /**< IP packet length - includes sizeof(ip_header). */
- uint8_t proto; /**< Protocol, next header. */
- uint8_t hop_limits; /**< Hop limits. */
- uint8_t src_addr[16]; /**< IP address of source host. */
- uint8_t dst_addr[16]; /**< IP address of destination host(s). */
+ rte_be32_t vtc_flow; /**< IP version, traffic class & flow label. */
+ rte_be16_t payload_len; /**< IP packet length - includes sizeof(ip_header). */
+ uint8_t proto; /**< Protocol, next header. */
+ uint8_t hop_limits; /**< Hop limits. */
+ uint8_t src_addr[16]; /**< IP address of source host. */
+ uint8_t dst_addr[16]; /**< IP address of destination host(s). */
} __attribute__((__packed__));
/**
@@ -427,8 +427,8 @@ rte_ipv6_phdr_cksum(const struct ipv6_hdr *ipv6_hdr, uint64_t ol_flags)
{
uint32_t sum;
struct {
- uint32_t len; /* L4 length. */
- uint32_t proto; /* L4 protocol - top 3 bytes must be zero */
+ rte_be32_t len; /* L4 length. */
+ rte_be32_t proto; /* L4 protocol - top 3 bytes must be zero */
} psd_hdr;
psd_hdr.proto = (ipv6_hdr->proto << 24);
diff --git a/lib/librte_net/rte_net.c b/lib/librte_net/rte_net.c
index a8c7aff..9014ca5 100644
--- a/lib/librte_net/rte_net.c
+++ b/lib/librte_net/rte_net.c
@@ -153,8 +153,8 @@ ptype_inner_l4(uint8_t proto)
/* get the tunnel packet type if any, update proto and off. */
static uint32_t
-ptype_tunnel(uint16_t *proto, const struct rte_mbuf *m,
- uint32_t *off)
+ptype_tunnel(rte_be16_t *proto, const struct rte_mbuf *m,
+ uint32_t *off)
{
switch (*proto) {
case IPPROTO_GRE: {
@@ -208,8 +208,8 @@ ip4_hlen(const struct ipv4_hdr *hdr)
/* parse ipv6 extended headers, update offset and return next proto */
static uint16_t
-skip_ip6_ext(uint16_t proto, const struct rte_mbuf *m, uint32_t *off,
- int *frag)
+skip_ip6_ext(rte_be16_t proto, const struct rte_mbuf *m, uint32_t *off,
+ int *frag)
{
struct ext_hdr {
uint8_t next_hdr;
@@ -261,7 +261,7 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
struct ether_hdr eh_copy;
uint32_t pkt_type = RTE_PTYPE_L2_ETHER;
uint32_t off = 0;
- uint16_t proto;
+ rte_be16_t proto;
if (hdr_lens == NULL)
hdr_lens = &local_hdr_lens;
diff --git a/lib/librte_net/rte_sctp.h b/lib/librte_net/rte_sctp.h
index 688e126..8c646c7 100644
--- a/lib/librte_net/rte_sctp.h
+++ b/lib/librte_net/rte_sctp.h
@@ -81,15 +81,16 @@ extern "C" {
#endif
#include <stdint.h>
+#include <rte_byteorder.h>
/**
* SCTP Header
*/
struct sctp_hdr {
- uint16_t src_port; /**< Source port. */
- uint16_t dst_port; /**< Destin port. */
- uint32_t tag; /**< Validation tag. */
- uint32_t cksum; /**< Checksum. */
+ rte_be16_t src_port; /**< Source port. */
+ rte_be16_t dst_port; /**< Destin port. */
+ rte_be32_t tag; /**< Validation tag. */
+ rte_le32_t cksum; /**< Checksum. */
} __attribute__((__packed__));
#ifdef __cplusplus
diff --git a/lib/librte_net/rte_tcp.h b/lib/librte_net/rte_tcp.h
index 28b61e6..545d4ab 100644
--- a/lib/librte_net/rte_tcp.h
+++ b/lib/librte_net/rte_tcp.h
@@ -77,6 +77,7 @@
*/
#include <stdint.h>
+#include <rte_byteorder.h>
#ifdef __cplusplus
extern "C" {
@@ -86,15 +87,15 @@ extern "C" {
* TCP Header
*/
struct tcp_hdr {
- uint16_t src_port; /**< TCP source port. */
- uint16_t dst_port; /**< TCP destination port. */
- uint32_t sent_seq; /**< TX data sequence number. */
- uint32_t recv_ack; /**< RX data acknowledgement sequence number. */
- uint8_t data_off; /**< Data offset. */
- uint8_t tcp_flags; /**< TCP flags */
- uint16_t rx_win; /**< RX flow control window. */
- uint16_t cksum; /**< TCP checksum. */
- uint16_t tcp_urp; /**< TCP urgent pointer, if any. */
+ rte_be16_t src_port; /**< TCP source port. */
+ rte_be16_t dst_port; /**< TCP destination port. */
+ rte_be32_t sent_seq; /**< TX data sequence number. */
+ rte_be32_t recv_ack; /**< RX data acknowledgement sequence number. */
+ uint8_t data_off; /**< Data offset. */
+ uint8_t tcp_flags; /**< TCP flags */
+ rte_be16_t rx_win; /**< RX flow control window. */
+ rte_be16_t cksum; /**< TCP checksum. */
+ rte_be16_t tcp_urp; /**< TCP urgent pointer, if any. */
} __attribute__((__packed__));
#ifdef __cplusplus
diff --git a/lib/librte_net/rte_udp.h b/lib/librte_net/rte_udp.h
index bc5be4a..89fdded 100644
--- a/lib/librte_net/rte_udp.h
+++ b/lib/librte_net/rte_udp.h
@@ -77,6 +77,7 @@
*/
#include <stdint.h>
+#include <rte_byteorder.h>
#ifdef __cplusplus
extern "C" {
@@ -86,10 +87,10 @@ extern "C" {
* UDP Header
*/
struct udp_hdr {
- uint16_t src_port; /**< UDP source port. */
- uint16_t dst_port; /**< UDP destination port. */
- uint16_t dgram_len; /**< UDP datagram length */
- uint16_t dgram_cksum; /**< UDP datagram checksum */
+ rte_be16_t src_port; /**< UDP source port. */
+ rte_be16_t dst_port; /**< UDP destination port. */
+ rte_be16_t dgram_len; /**< UDP datagram length */
+ rte_be16_t dgram_cksum; /**< UDP datagram checksum */
} __attribute__((__packed__));
#ifdef __cplusplus
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] virtio: tx with can_push when VERSION_1 is set
From: Maxime Coquelin @ 2016-11-09 14:51 UTC (permalink / raw)
To: Pierre Pfister (ppfister), Yuanhan Liu; +Cc: dev@dpdk.org
In-Reply-To: <1FAE01C3-B03F-443A-A77C-579254A07E0E@cisco.com>
Hi Pierre,
On 11/09/2016 01:42 PM, Pierre Pfister (ppfister) wrote:
> Hello Maxime,
>
> Sorry for the late reply.
>
>
>> Le 8 nov. 2016 à 10:44, Maxime Coquelin <maxime.coquelin@redhat.com> a écrit :
>>
>> Hi Pierre,
>>
>> On 11/08/2016 10:31 AM, Pierre Pfister (ppfister) wrote:
>>> Current virtio driver advertises VERSION_1 support,
>>> but does not handle device's VERSION_1 support when
>>> sending packets (it looks for ANY_LAYOUT feature,
>>> which is absent).
>>>
>>> This patch enables 'can_push' in tx path when VERSION_1
>>> is advertised by the device.
>>>
>>> This significantly improves small packets forwarding rate
>>> towards devices advertising VERSION_1 feature.
>> I think it depends whether offloading is enabled or not.
>> If no offloading enabled, I measured significant drop.
>> Indeed, when no offloading is enabled, the Tx path in Virtio
>> does not access the virtio header before your patch, as the header is memset to zero at device init time.
>> With your patch, it gets memset to zero at every transmit in the hot
>> path.
>
> Right. On the virtio side that is true, but on the device side, we have to access the header anyway.
No more now, if no offload features have been negotiated.
I have done a patch that landed in v16.11 to skip header parsing in
this case.
That said, we still have to access its descriptor.
> And accessing two descriptors (with the address resolution and memory fetch which comes with it)
> is a costy operation compared to a single one.
> In the case indirect descriptors are used, this is 1 desc access instead or 3.
I agree this is far from being optimal.
> And in the case chained descriptors are used, this doubles the number of packets that you can put in your queue.
>
> Those are the results in my PHY -> VM (testpmd) -> PHY setup
> Traffic is flowing bidirectionally. Numbers are for lossless-rates.
>
> When chained buffers are used for dpdk's TX: 2x2.13Mpps
> When indirect descriptors are used for dpdk's TX: 2x2.38Mpps
> When shallow buffers are used for dpdk's TX (with this patch): 2x2.42Mpps
When I tried it, I also did PVP 0% benchmark, and I got opposite
results. Chained and indirect cases were significantly better.
My PVP setup was using a single NIC and single Virtio PMD, and NIC2VM
forwarding was IO mode done with testpmd on host, and Rx->Tx forwarding
was macswap mode on guest side.
I also saw some perf regression when running simple tespmd test on both
ends.
Yuanhan, did you run some benchmark with your series enabling
ANY_LAYOUT?
>
> I must also note that qemu 2.5 does not seem to deal with VERSION_1 and ANY_LAYOUT correctly.
> The patch I am proposing here works for qemu 2.7, but with qemu 2.5, testpmd still behaves as if ANY_LAYOUT (or VERSION_1) was not available. This is not catastrophic. But just note that you will not see performance in some cases with qemu 2.5.
Thanks for the info.
Regards,
Maxime
^ permalink raw reply
* Re: disable hugepages
From: Olivier Matz @ 2016-11-09 14:50 UTC (permalink / raw)
To: Keren Hochman, Christian Ehrhardt; +Cc: dev
In-Reply-To: <CAJq3SQ7r4e_Dr1wvwPYmiECWASEvAwvZomH4x6o1MFo35JQVXg@mail.gmail.com>
Hi Keren,
On 11/09/2016 03:40 PM, Keren Hochman wrote:
> On Wed, Nov 9, 2016 at 3:40 PM, Christian Ehrhardt <
> christian.ehrhardt@canonical.com> wrote:
>
>>
>> On Wed, Nov 9, 2016 at 1:55 PM, Keren Hochman <
>> keren.hochman@lightcyber.com> wrote:
>>
>>> how can I create mempool without hugepages?My application is running on a
>>> pcap file so no huge pages is needed ?
>>>
>>
>> Not sure if that is what you really want (Debug use only), but in general
>> no-huge is available as EAL arg
>>
>> From http://pktgen.readthedocs.io/en/latest/usage_eal.html :
>>
>> EAL options for DEBUG use only:
>> --no-huge : Use malloc instead of hugetlbfs
>>
> I need this option only for testing. How can I use rte_mempool_create if I
> use --no-huge?
When using --no-huge, the dpdk libraries (including mempool) allocate
its memory in standard memory. Just keep in mind the physical addresses
will be wrong, so this memory cannot be given to hw devices.
Regards,
Olivier
^ permalink raw reply
* Re: [PATCH] virtio: tx with can_push when VERSION_1 is set
From: Maxime Coquelin @ 2016-11-09 14:42 UTC (permalink / raw)
To: Pierre Pfister (ppfister); +Cc: dev@dpdk.org
In-Reply-To: <1FAE01C3-B03F-443A-A77C-579254A07E0E@cisco.com>
Hi Pierre,
On 11/09/2016 01:42 PM, Pierre Pfister (ppfister) wrote:
> Hello Maxime,
>
> Sorry for the late reply.
>
>
>> Le 8 nov. 2016 à 10:44, Maxime Coquelin <maxime.coquelin@redhat.com> a écrit :
>>
>> Hi Pierre,
>>
>> On 11/08/2016 10:31 AM, Pierre Pfister (ppfister) wrote:
>>> Current virtio driver advertises VERSION_1 support,
>>> but does not handle device's VERSION_1 support when
>>> sending packets (it looks for ANY_LAYOUT feature,
>>> which is absent).
>>>
>>> This patch enables 'can_push' in tx path when VERSION_1
>>> is advertised by the device.
>>>
>>> This significantly improves small packets forwarding rate
>>> towards devices advertising VERSION_1 feature.
>> I think it depends whether offloading is enabled or not.
>> If no offloading enabled, I measured significant drop.
>> Indeed, when no offloading is enabled, the Tx path in Virtio
>> does not access the virtio header before your patch, as the header is memset to zero at device init time.
>> With your patch, it gets memset to zero at every transmit in the hot
>> path.
>
> Right. On the virtio side that is true, but on the device side, we have to access the header anyway.
No more now, if no offload features have been negotiated.
I have done a patch that landed in v16.11 not to parse header in this case.
That said, we still have to access its descriptor.
> And accessing two descriptors (with the address resolution and memory fetch which comes with it)
> is a costy operation compared to a single one.
> In the case indirect descriptors are used, this is 1 desc access instead or 3.
> And in the case chained descriptors are used, this doubles the number of packets that you can put in your queue.
>
> Those are the results in my PHY -> VM (testpmd) -> PHY setup
> Traffic is flowing bidirectionally. Numbers are for lossless-rates.
>
> When chained buffers are used for dpdk's TX: 2x2.13Mpps
> When indirect descriptors are used for dpdk's TX: 2x2.38Mpps
> When shallow buffers are used for dpdk's TX (with this patch): 2x2.42Mpps
When I tried it, I also did PVP 0% benchmark, and I got opposite
results. Chained and indirect cases were significantly better.
My PVP setup was using a single NIC and single Virtio PMD, and NIC2VM
forwarding was IO mode done with testpmd on host, and Rx->Tx forwarding
was macswap mode on guest side.
That said, if I'm the only one seeing a performance regression, maybe
something is wrong with my setup.
Yuanhan, did you made some benchmarks when you implemented your
any_layout enabling series?
>
> I must also note that qemu 2.5 does not seem to deal with VERSION_1 and ANY_LAYOUT correctly.
> The patch I am proposing here works for qemu 2.7, but with qemu 2.5, testpmd still behaves as if ANY_LAYOUT (or VERSION_1) was not available. This is not catastrophic. But just note that you will not see performance in some cases with qemu 2.5.
Thanks for the info.
Regards,
Maxime
^ permalink raw reply
* Re: disable hugepages
From: Keren Hochman @ 2016-11-09 14:40 UTC (permalink / raw)
To: Christian Ehrhardt; +Cc: dev
In-Reply-To: <CAATJJ0LNAAbT9qnWgp-OtXeE-dX6irG0WxAwMW_euKiaVfEoVQ@mail.gmail.com>
I need this option only for testing. How can I use rte_mempool_create if I
use --no-huge?
Thank you, Keren
On Wed, Nov 9, 2016 at 3:40 PM, Christian Ehrhardt <
christian.ehrhardt@canonical.com> wrote:
>
> On Wed, Nov 9, 2016 at 1:55 PM, Keren Hochman <
> keren.hochman@lightcyber.com> wrote:
>
>> how can I create mempool without hugepages?My application is running on a
>> pcap file so no huge pages is needed ?
>>
>
> Not sure if that is what you really want (Debug use only), but in general
> no-huge is available as EAL arg
>
> From http://pktgen.readthedocs.io/en/latest/usage_eal.html :
>
> EAL options for DEBUG use only:
> --no-huge : Use malloc instead of hugetlbfs
>
>
>
>
^ permalink raw reply
* Re: disable hugepages
From: Christian Ehrhardt @ 2016-11-09 13:40 UTC (permalink / raw)
To: Keren Hochman; +Cc: dev
In-Reply-To: <CAJq3SQ6Wjueqs9sCF0Kn4S_O0sZqZ_4hV6ztAeA8a583OZQc9w@mail.gmail.com>
On Wed, Nov 9, 2016 at 1:55 PM, Keren Hochman <keren.hochman@lightcyber.com>
wrote:
> how can I create mempool without hugepages?My application is running on a
> pcap file so no huge pages is needed ?
>
Not sure if that is what you really want (Debug use only), but in general
no-huge is available as EAL arg
>From http://pktgen.readthedocs.io/en/latest/usage_eal.html :
EAL options for DEBUG use only:
--no-huge : Use malloc instead of hugetlbfs
^ permalink raw reply
* Re: [PATCH v7 00/21] Introduce SoC device/driver framework for EAL
From: Shreyansh Jain @ 2016-11-09 13:36 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, viktorin, david.marchand
In-Reply-To: <15406748.TzsDne6LfD@xps13>
Hello Thomas,
On Wednesday 09 November 2016 03:47 PM, Thomas Monjalon wrote:
> Hi Shreyansh,
>
> I realize that we had a lot of off-list discussions on this topic
> and there was no public explanation of the status of this series.
Thanks for your email. (and all the suggestions you have given
offline/IRC etc.)
I was beginning to wonder that probably only Jan and me were the ones
interested in this. Ironically, I felt that being EAL changes, a lot of
people would come and be critic of it - giving an opportunity to get it
widely accepted.
>
> 2016-10-28 17:56, Shreyansh Jain:
> [...]
>> As of now EAL is primarly focused on PCI initialization/probing.
>
> Right. DPDK was PCI centric.
> We must give PCI its right role: a bus as other ones.
> A first step was done in 16.11 (thanks to you Shreyansh, Jan and David)
> to have a better device model.
> The next step is to rework the bus abstraction.
Or, this change can be broken into multiple steps:
1. Create a PCI parallel layer for non-PCI devices. (call it SoC, or
whatever else - doesn't really matter. More below).
2. Generalize this 'soc' changeset into common
3. Complete generalization by introducing a Linux like model of
bus<=>device<=>driver.
Which was what my current approach was - expecting that SoC patchset
would allow me to introduce NXP PMD and parallel to it I can keep
pushing EAL generic changes towards generic bus arch.
>
> It seems a bus can be defined with the properties scan/match/notify,
> leading to initialize the devices.
'notify' is something which I am not completely clear with - but, in
principle, agree.
>
> More comments below your technical presentation.
>
> [...]
>> This patchset introduces SoC framework which would enable SoC drivers and
>> drivers to be plugged into EAL, very similar to how PCI drivers/devices are
>> done today.
>>
>> This is a stripped down version of PCI framework which allows the SoC PMDs
>> to implement their own routines for detecting devices and linking devices to
>> drivers.
>>
>> 1) Changes to EAL
>> rte_eal_init()
>> |- rte_eal_pci_init(): Find PCI devices from sysfs
>> |- rte_eal_soc_init(): Calls PMDs->scan_fn
>> |- ...
>> |- rte_eal_memzone_init()
>> |- ...
>> |- rte_eal_pci_probe(): Driver<=>Device initialization, PMD->devinit()
>> `- rte_eal_soc_probe(): Calls PMDs->match_fn and PMDs->devinit();
>>
>> 2) New device/driver structures:
>> - rte_soc_driver (inheriting rte_driver)
>> - rte_soc_device (inheriting rte_device)
>> - rte_eth_dev and eth_driver embedded rte_soc_device and rte_soc_driver,
>> respectively.
>>
>> 3) The SoC PMDs need to:
>> - define rte_soc_driver with necessary scan and match callbacks
>> - Register themselves using DRIVER_REGISTER_SOC()
>> - Implement respective bus scanning in the scan callbacks to add necessary
>> devices to SoC device list
>> - Implement necessary eth_dev_init/uninint for ethernet instances
>
> These callbacks are not specific to a SoC.
Agree; this is just a note - it exactly what a PCI PMD does.
> By the way a SoC defines nothing specific. You are using the SoC word
> as an equivalent of non-PCI.
Yes, primarily because SoC is a very broad word which can encompass a
variety of devices (buses/drivers). So, we have PCI set, VDEV set, and
everything else represented by 'SoC'.
The complete set is based on this principle: to have a generic subsystem
_parallel_ to PCI (so as not to impact it) which can represent all
devices not already included in PCI set'. - Actually, it is _nothing_
specific.
> We must have a bus abstraction like the one you are defining for the SoC
> but it must be generic and defined on top of PCI, so we can plug any
> bus in it: PCI, vdev (as a software bus), any other well-defined bus,
> and some driver-specific bus which can be implemented directly in the
> driver (the NXP case AFAIK).
Indeed - that is an ideal approach. And honestly, true attribution, it
is not my original idea. It is yours, from our IRC discussion.
And it was ironic as well because Declan came up with similar suggestion
much earlier but no one commented on it.
>
>> 4) Design considerations that are same as PCI:
>> - SoC initialization is being done through rte_eal_init(), just after PCI
>> initialization is done.
>> - As in case of PCI, probe is done after rte_eal_pci_probe() to link the
>> devices detected with the drivers registered.
>> - Device attach/detach functions are available and have been designed on
>> the lines of PCI framework.
>> - PMDs register using DRIVER_REGISTER_SOC, very similar to
>> DRIVER_REGISTER_PCI for PCI devices.
>> - Linked list of SoC driver and devices exists independent of the other
>> driver/device list, but inheriting rte_driver/rte_driver, these are
>> also part of a global list.
>>
>> 5) Design considerations that are different from PCI:
>> - Each driver implements its own scan and match function. PCI uses the BDF
>> format to read the device from sysfs, but this _may_not_ be a case for a
>> SoC ethernet device.
>> = This is an important change from initial proposal by Jan in [2].
>> Unlike his attempt to use /sys/bus/platform, this patch relies on the
>> PMD to detect the devices. This is because SoC may require specific or
>> additional info for device detection. Further, SoC may have embedded
>> devices/MACs which require initialization which cannot be covered
>> through sysfs parsing.
>> `-> Point (6) below is a side note to above.
>> = PCI based PMDs rely on EAL's capability to detect devices. This
>> proposal puts the onus on PMD to detect devices, add to soc_device_list
>> and wait for Probe. Matching, of device<=>driver is again PMD's
>> callback.
>
> These PCI considerations can be described as a PCI bus implementation in EAL.
Yes. Or, we can continue as it is for PCI and allow more buses to plug
in. And eventually, make the whole bus thing generic. Step-by-Step.
>
>> 6) Adding default scan and match helpers for PMDs
>> - The design warrrants the PMDs implement their own scan of devices
>> on bus, and match routines for probe implementation.
>> This patch introduces helpers which can be used by PMDs for scan of
>> the platform bus and matching devices against the compatible string
>> extracted from the scan.
>> - Intention is to make it easier to integrate known SoC which expose
>> platform bus compliant information (compat, sys/bus/platform...).
>> - PMDs which have deviations from this standard model can implement and
>> hook their bus scanning and probe match callbacks while registering
>> driver.
>
> Yes we can have EAL helpers to scan sys/bus/platform or other common formats.
> Then the PMD can use them to implement its specific bus.
And:
- Framework/Developers should be able to 'register/unregister' buses.
-- Just like drivers/net
-- So, probably a drivers/bus/pci, drivers/bus/xxx folder structure
-- and compatible compilation routine
-- Or maybe librte_bus?
--- I still haven't got around solving this.
- RTE_PMD_REGISTER_BUS probably.
- just EAL helpers are not enough - or at least, not generic enough.
- Drivers should be able to 'lookup' buses
-- PCI PMDs find a 'pci' bus during registration, for example.
-- Unlike Linux, a device tree doesn't exist to solve this issue of
which PMD is connected to which bus. This needs to solved using explicit
APIs
-- Once found, drivers can 'associate' with a bus.
- Not just bus<->device<->driver, there is pending work with respect
ethernet devices.
-- Removing eth_driver->pci_driver linkage. Making it generic.
-- Multiple places assume eth device to be PCI, rewriting that part.
-- device_insert changes for vdev
>
> I know that David, you and me are on the same page to agree on this generic
> design. I hope you will be able to achieve this work soon.
I agree with your suggestion in principle - bus model is nice to have.
There is always a better design. It is about how to achieve it.
I still think that current SoC patchset is a decent first step without
completely discounting any future changes. (Obviously, assuming it is
not NACK'd by someone because of some currently unforeseen technical
argument).
Because:
1. It doesn't break the existing PCI subsystem anywhere. It is disabled
by default.
2. It is independent - in the sense that changes are limited to SoC
specific files, with only a few changes in EAL. But, clean patches make
it easier to validate.
3. It would allow more people to check/validate the overall proposal of
non-PCI device existence.
4. It paves way for cleaner transit to Bus architecture
- rte_device/driver inheritance is complete after SoC patchset. Except
changes in eth_dev/driver which is a completely parallel change with
little overlap (but wide impact)
- rte_soc_driver introduces 'scan/match' which can then be generalized
to rte_driver in next step.
-- This is very similar to what happened in rte_device/driver change
where PCI things were generalized to common.
> The goal is to be able to plug a SoC driver in DPDK 17.02 with a clean design.
I am already working on that - now on a higher priority. Hopefully
within next few days I will send a RFC out. There are still some grey
ares for me (lack of DPDK internal understand, probably) - but I am
hoping I would get good support from others - at least this time.
Then, probably for 17.02, we can have either candidate - and we can
integrate whatever looks best (in terms of design and impact, both).
> My advice to make reviews and contributions easier, is to split the work in
> few steps:
> - clean PCI code (generalize non-PCI stuff)
> - add generic bus functions
> - plug PCI in generic bus abstraction
> - plug vdev in generic bus abstraction
> - plug the new NXP driver
I agree with above.
>
> Thanks
>
Thank you for your time and suggestions.
-
Shreyansh
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Björn Töpel @ 2016-11-09 13:09 UTC (permalink / raw)
To: Zhang, Helin, Ananyev, Konstantin, dev@dpdk.org
Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon@6wind.com
In-Reply-To: <F35DEAC7BCE34641BA9FAC6BCA4A12E717F4AED3@SHSMSX103.ccr.corp.intel.com>
Björn/Konstantin wrote:
>> Finally, why doesn't l3fwd have the CRC stripped?
>
> I don’t know any good reason for that for l3fwd or any other sample
> app. I think it is just a 'historical' reason.
Ok! Then I'd suggest changing the l3fwd default to actually *strip* CRC
instead of not doing it. Lei, any comments?
Helin wrote:
> Yes, i40e driver changed a little bit on that according to the
> review comments during implementation, comparing to igb and ixgbe.
> I'd suggest to re-invesitgate if we can do the similar thing in igb
> and ixgbe driver.
Good. Let's do that!
> Any critical issue now? Or just an improvement comments?
Not from my perspective. The issue is that Lei needs some kind of
work-around for l3fwd with i40evf, so I'll let Lei comment on how
critical it is.
Björn
^ permalink raw reply
* [PATCH] app/test: fix crash of lpm test
From: Olivier Matz @ 2016-11-09 13:08 UTC (permalink / raw)
To: dev, wei.dai; +Cc: bruce.richardson
The test recently added accesses to lpm->tbl8[ip >> 8] with is much
larger than the size of the table, causing a crash of the test
application.
Fix this typo by replacing tbl8 by tbl24.
Fixes: 231fa88ed522 ("app/test: verify LPM tbl8 recycle")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
Hi Wei,
I don't know lpm very well and I did not spend much time to understand
the test case. I guess that's the proper fix, but please check carefully
that I'm not doing something wrong :)
Thanks,
Olivier
app/test/test_lpm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/app/test/test_lpm.c b/app/test/test_lpm.c
index 80e0efc..41ae80f 100644
--- a/app/test/test_lpm.c
+++ b/app/test/test_lpm.c
@@ -1256,7 +1256,7 @@ test18(void)
rte_lpm_add(lpm, ip, depth, next_hop);
TEST_LPM_ASSERT(lpm->tbl24[ip>>8].valid_group);
- tbl8_group_index = lpm->tbl8[ip>>8].group_idx;
+ tbl8_group_index = lpm->tbl24[ip>>8].group_idx;
depth = 23;
next_hop = 2;
@@ -1272,7 +1272,7 @@ test18(void)
rte_lpm_add(lpm, ip, depth, next_hop);
TEST_LPM_ASSERT(lpm->tbl24[ip>>8].valid_group);
- TEST_LPM_ASSERT(tbl8_group_index == lpm->tbl8[ip>>8].group_idx);
+ TEST_LPM_ASSERT(tbl8_group_index == lpm->tbl24[ip>>8].group_idx);
depth = 24;
next_hop = 4;
@@ -1288,7 +1288,7 @@ test18(void)
rte_lpm_add(lpm, ip, depth, next_hop);
TEST_LPM_ASSERT(lpm->tbl24[ip>>8].valid_group);
- TEST_LPM_ASSERT(tbl8_group_index == lpm->tbl8[ip>>8].group_idx);
+ TEST_LPM_ASSERT(tbl8_group_index == lpm->tbl24[ip>>8].group_idx);
rte_lpm_free(lpm);
#undef group_idx
--
2.8.1
^ permalink raw reply related
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Zhang, Helin @ 2016-11-09 13:01 UTC (permalink / raw)
To: Topel, Bjorn, Ananyev, Konstantin, dev@dpdk.org
Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon@6wind.com
In-Reply-To: <b2592ded-badc-5445-526c-d0203b48f592@intel.com>
> -----Original Message-----
> From: Topel, Bjorn
> Sent: Wednesday, November 9, 2016 7:28 PM
> To: Ananyev, Konstantin; dev@dpdk.org; Zhang, Helin
> Cc: Xu, Qian Q; Yao, Lei A; Wu, Jingjing; thomas.monjalon@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for
> i40evf
>
> >> Correct, so the broader question would be "what is the correct
> >> behavior for an example application, when a port configuration isn't
> >> supported by the hardware?".
> >>
> >> My stand, FWIW, is that igb and ixgbe should have the same semantics
> >> as i40e currently has, i.e. return an error to the user if the port
> >> is mis-configured, NOT changing the setting behind the users back.
> >>
> >
> > Fine by me, but then it means that the fix haw to include changes for
> > all apps plus ixgbe and igb PMDs, correct? :)
>
> Ugh. Correct, I guess. :-)
>
> As for ixgbe and igb - they need a patch changing from silent ignore to
> explicit error. Regarding the apps, I guess all the apps that rely on that
> disabling CRC stripping always work, need some work. Or should all the
> example applications have CRC stripping *enabled* by default? I'd assume
> that all DPDK supported NICs has support for CRC stripping and I guess this is
> the rational for having it on by default for Intel VFs.
>
> In general, for the example applications, if an application relies on a property
> for a port, that the hardware doesn't support -- what would be the desired
> behavior? Or is it implied that the example applications only use a common,
> minimal set of features that are know to be supported by all DPDK supported
> hardware?
>
> Isn't it perfectly valid that some example applications wont run for all
> hardware?
>
> Finally, why doesn't l3fwd have the CRC stripped?
>
>
> Björn
Yes, i40e driver changed a little bit on that according to the review comments
during implementation, comparing to igb and ixgbe.
I'd suggest to re-invesitgate if we can do the similar thing in igb and ixgbe driver.
Any critical issue now? Or just an improvement comments?
Thanks,
Helin
^ permalink raw reply
* disable hugepages
From: Keren Hochman @ 2016-11-09 12:55 UTC (permalink / raw)
To: dev
Hi,
how can I create mempool without hugepages?My application is running on a
pcap file so no huge pages is needed ?
Thanks.
^ permalink raw reply
* Re: [PATCH] virtio: tx with can_push when VERSION_1 is set
From: Pierre Pfister (ppfister) @ 2016-11-09 12:42 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev@dpdk.org
In-Reply-To: <46762894-5016-f634-c355-77af881685e6@redhat.com>
Hello Maxime,
Sorry for the late reply.
> Le 8 nov. 2016 à 10:44, Maxime Coquelin <maxime.coquelin@redhat.com> a écrit :
>
> Hi Pierre,
>
> On 11/08/2016 10:31 AM, Pierre Pfister (ppfister) wrote:
>> Current virtio driver advertises VERSION_1 support,
>> but does not handle device's VERSION_1 support when
>> sending packets (it looks for ANY_LAYOUT feature,
>> which is absent).
>>
>> This patch enables 'can_push' in tx path when VERSION_1
>> is advertised by the device.
>>
>> This significantly improves small packets forwarding rate
>> towards devices advertising VERSION_1 feature.
> I think it depends whether offloading is enabled or not.
> If no offloading enabled, I measured significant drop.
> Indeed, when no offloading is enabled, the Tx path in Virtio
> does not access the virtio header before your patch, as the header is memset to zero at device init time.
> With your patch, it gets memset to zero at every transmit in the hot
> path.
Right. On the virtio side that is true, but on the device side, we have to access the header anyway.
And accessing two descriptors (with the address resolution and memory fetch which comes with it)
is a costy operation compared to a single one.
In the case indirect descriptors are used, this is 1 desc access instead or 3.
And in the case chained descriptors are used, this doubles the number of packets that you can put in your queue.
Those are the results in my PHY -> VM (testpmd) -> PHY setup
Traffic is flowing bidirectionally. Numbers are for lossless-rates.
When chained buffers are used for dpdk's TX: 2x2.13Mpps
When indirect descriptors are used for dpdk's TX: 2x2.38Mpps
When shallow buffers are used for dpdk's TX (with this patch): 2x2.42Mpps
I must also note that qemu 2.5 does not seem to deal with VERSION_1 and ANY_LAYOUT correctly.
The patch I am proposing here works for qemu 2.7, but with qemu 2.5, testpmd still behaves as if ANY_LAYOUT (or VERSION_1) was not available. This is not catastrophic. But just note that you will not see performance in some cases with qemu 2.5.
Cheers
- Pierre
>
> With offloading enabled, it does makes sense though, as the header will
> be accessed.
>
> This patch is for v17.02 anyway, and we may provide a way to enable and
> disable features at Virtio PMD init time by this release.
>
> Thanks,
> Maxime
>
>>
>> Signed-off-by: Pierre Pfister <ppfister@cisco.com>
>> ---
>> drivers/net/virtio/virtio_rxtx.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index 724517e..2fe0338 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -925,7 +925,8 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>> }
>>
>> /* optimize ring usage */
>> - if (vtpci_with_feature(hw, VIRTIO_F_ANY_LAYOUT) &&
>> + if ((vtpci_with_feature(hw, VIRTIO_F_ANY_LAYOUT) ||
>> + vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) &&
>> rte_mbuf_refcnt_read(txm) == 1 &&
>> RTE_MBUF_DIRECT(txm) &&
>> txm->nb_segs == 1 &&
>> --
>> 2.7.4 (Apple Git-66)
>>
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Ananyev, Konstantin @ 2016-11-09 12:13 UTC (permalink / raw)
To: Topel, Bjorn, dev@dpdk.org, Zhang, Helin
Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon@6wind.com
In-Reply-To: <b2592ded-badc-5445-526c-d0203b48f592@intel.com>
> -----Original Message-----
> From: Topel, Bjorn
> Sent: Wednesday, November 9, 2016 11:28 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>
> Cc: Xu, Qian Q <qian.q.xu@intel.com>; Yao, Lei A <lei.a.yao@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> thomas.monjalon@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
>
> >> Correct, so the broader question would be "what is the correct
> >> behavior for an example application, when a port configuration
> >> isn't supported by the hardware?".
> >>
> >> My stand, FWIW, is that igb and ixgbe should have the same
> >> semantics as i40e currently has, i.e. return an error to the user
> >> if the port is mis-configured, NOT changing the setting behind the
> >> users back.
> >>
> >
> > Fine by me, but then it means that the fix haw to include changes
> > for all apps plus ixgbe and igb PMDs, correct? :)
>
> Ugh. Correct, I guess. :-)
>
> As for ixgbe and igb - they need a patch changing from silent ignore to
> explicit error. Regarding the apps, I guess all the apps that rely on
> that disabling CRC stripping always work, need some work. Or should all
> the example applications have CRC stripping *enabled* by default? I'd
> assume that all DPDK supported NICs has support for CRC stripping
From the sources, it seems that only nfp doesn't support HW CRC stripping.
In fact, as I can see some non-Intel PMDs just ignore hw_stip_crc value
and assume that CRC strip is always on.
> and I
> guess this is the rational for having it on by default for Intel VFs.
>
> In general, for the example applications, if an application relies on a
> property for a port, that the hardware doesn't support -- what would be
> the desired behavior? Or is it implied that the example applications
> only use a common, minimal set of features that are know to be supported
> by all DPDK supported hardware?
>
> Isn't it perfectly valid that some example applications wont run for all
> hardware?
>
> Finally, why doesn't l3fwd have the CRC stripped?
I don’t know any good reason for that for l3fwd or any other sample app.
I think it is just a 'historical' reason.
Konstantin
>
>
> Björn
^ permalink raw reply
* Re: mbuf changes
From: Alejandro Lucero @ 2016-11-09 11:42 UTC (permalink / raw)
To: Bruce Richardson
Cc: Shreyansh Jain, Wiles, Keith, Morten Brørup, dev@dpdk.org,
Olivier Matz
In-Reply-To: <CAD+H9900j7LdYH79xBBO5wfHwomagLqpwqsDjuyJxozomL-dug@mail.gmail.com>
On Wed, Oct 26, 2016 at 10:28 AM, Alejandro Lucero <
alejandro.lucero@netronome.com> wrote:
>
>
> On Tue, Oct 25, 2016 at 2:05 PM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
>
>> On Tue, Oct 25, 2016 at 05:24:28PM +0530, Shreyansh Jain wrote:
>> > On Monday 24 October 2016 09:55 PM, Bruce Richardson wrote:
>> > > On Mon, Oct 24, 2016 at 04:11:33PM +0000, Wiles, Keith wrote:
>> > > >
>> > > > > On Oct 24, 2016, at 10:49 AM, Morten Brørup <
>> mb@smartsharesystems.com> wrote:
>> > > > >
>> > > > > First of all: Thanks for a great DPDK Userspace 2016!
>> > > > >
>> > > > >
>> > > > >
>> > > > > Continuing the Userspace discussion about Olivier Matz’s proposed
>> mbuf changes...
>> > >
>> > > Thanks for keeping the discussion going!
>> > > > >
>> > > > >
>> > > > >
>> > > > > 1.
>> > > > >
>> > > > > Stephen Hemminger had a noteworthy general comment about keeping
>> metadata for the NIC in the appropriate section of the mbuf: Metadata
>> generated by the NIC’s RX handler belongs in the first cache line, and
>> metadata required by the NIC’s TX handler belongs in the second cache line.
>> This also means that touching the second cache line on ingress should be
>> avoided if possible; and Bruce Richardson mentioned that for this reason
>> m->next was zeroed on free().
>> > > > >
>> > > Thinking about it, I suspect there are more fields we can reset on
>> free
>> > > to save time on alloc. Refcnt, as discussed below is one of them, but
>> so
>> > > too could be the nb_segs field and possibly others.
>> > >
>> > > > >
>> > > > >
>> > > > > 2.
>> > > > >
>> > > > > There seemed to be consensus that the size of m->refcnt should
>> match the size of m->port because a packet could be duplicated on all
>> physical ports for L3 multicast and L2 flooding.
>> > > > >
>> > > > > Furthermore, although a single physical machine (i.e. a single
>> server) with 255 physical ports probably doesn’t exist, it might contain
>> more than 255 virtual machines with a virtual port each, so it makes sense
>> extending these mbuf fields from 8 to 16 bits.
>> > > >
>> > > > I thought we also talked about removing the m->port from the mbuf
>> as it is not really needed.
>> > > >
>> > > Yes, this was mentioned, and also the option of moving the port value
>> to
>> > > the second cacheline, but it appears that NXP are using the port value
>> > > in their NIC drivers for passing in metadata, so we'd need their
>> > > agreement on any move (or removal).
>> >
>> > I am not sure where NXP's NIC came into picture on this, but now that
>> it is
>> > highlighted, this field is required for libevent implementation [1].
>> >
>> > A scheduler sending an event, which can be a packet, would only have
>> > information of a flow_id. From this matching it back to a port, without
>> > mbuf->port, would be very difficult (costly). There may be way around
>> this
>> > but at least in current proposal I think port would be important to
>> have -
>> > even if in second cache line.
>> >
>> > But, off the top of my head, as of now it is not being used for any
>> specific
>> > purpose in NXP's PMD implementation.
>> >
>> > Even the SoC patches don't necessarily rely on it except using it
>> because it
>> > is available.
>> >
>> > @Bruce: where did you get the NXP context here from?
>> >
>> Oh, I'm just mis-remembering. :-( It was someone else who was looking for
>> this - Netronome, perhaps?
>>
>> CC'ing Alejandro in the hope I'm remembering correctly second time
>> round!
>>
>>
> Yes. Thanks Bruce!
>
> So Netronome uses the port field and, as I commented on the user meeting,
> we are happy with the field going from 8 to 16 bits.
>
> In our case, this is something some clients have demanded, and if I'm not
> wrong (I'll double check this asap), the port value is for knowing where
> the packet is coming from. Think about a switch in the NIC, with ports
> linked to VFs/VMs, and one or more physical ports. That port value is not
> related to DPDK ports but to the switch ports. Code in the host (DPDK or
> not) can receive packets from the wire or from VFs through the NIC. This is
> also true for packets received by VMs, but I guess the port value is just
> interested for host code.
>
>
>
I consulted this functionality internally and it seems we do not need this
anymore. In fact, I will remove the metadata port handling soon from our
PMD.
> /Bruce
>>
>
>
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Björn Töpel @ 2016-11-09 11:27 UTC (permalink / raw)
To: Ananyev, Konstantin, dev@dpdk.org, Zhang, Helin
Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon@6wind.com
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0D2FD9@irsmsx105.ger.corp.intel.com>
>> Correct, so the broader question would be "what is the correct
>> behavior for an example application, when a port configuration
>> isn't supported by the hardware?".
>>
>> My stand, FWIW, is that igb and ixgbe should have the same
>> semantics as i40e currently has, i.e. return an error to the user
>> if the port is mis-configured, NOT changing the setting behind the
>> users back.
>>
>
> Fine by me, but then it means that the fix haw to include changes
> for all apps plus ixgbe and igb PMDs, correct? :)
Ugh. Correct, I guess. :-)
As for ixgbe and igb - they need a patch changing from silent ignore to
explicit error. Regarding the apps, I guess all the apps that rely on
that disabling CRC stripping always work, need some work. Or should all
the example applications have CRC stripping *enabled* by default? I'd
assume that all DPDK supported NICs has support for CRC stripping and I
guess this is the rational for having it on by default for Intel VFs.
In general, for the example applications, if an application relies on a
property for a port, that the hardware doesn't support -- what would be
the desired behavior? Or is it implied that the example applications
only use a common, minimal set of features that are know to be supported
by all DPDK supported hardware?
Isn't it perfectly valid that some example applications wont run for all
hardware?
Finally, why doesn't l3fwd have the CRC stripped?
Björn
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Ananyev, Konstantin @ 2016-11-09 11:08 UTC (permalink / raw)
To: Topel, Bjorn, dev@dpdk.org, Zhang, Helin
Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon@6wind.com
In-Reply-To: <5ad83b54-f13b-787e-c056-958f5cb8bd61@intel.com>
>
> Adding Helin to the conversation.
>
> > That's a common problem across Intel VF devices. Bothe igb and ixgbe
> > overcomes that problem just by forcing ' rxmode.hw_strip_crc = 1;' at
> > PMD itself:
>
> [...]
>
> > Wonder, can't i40e VF do the same?
>
> Right, however, and this (silent failure) approach was rejected by the
> maintainers. Please, refer to this thread:
> http://dpdk.org/ml/archives/dev/2016-April/037523.html
>
> > BTW, all other examples would experience same problem too, right?
>
> Correct, so the broader question would be "what is the correct behavior
> for an example application, when a port configuration isn't supported by
> the hardware?".
>
> My stand, FWIW, is that igb and ixgbe should have the same semantics as
> i40e currently has, i.e. return an error to the user if the port is
> mis-configured, NOT changing the setting behind the users back.
>
Fine by me, but then it means that the fix haw to include changes for all apps
plus ixgbe and igb PMDs, correct? :)
Konstantin
^ permalink raw reply
* Re: [RFC v2] Generic flow director/filtering/classification API
From: Adrien Mazarguil @ 2016-11-09 11:07 UTC (permalink / raw)
To: Zhang, Helin, Zhao1, Wei, Ajit Khaparde, Alejandro Lucero,
Evgeny Schemeilin, Jing Chen, Jingjing Wu, Konstantin Ananyev,
Maciej Czekaj, Matej Vido, Nelson Escobar, Rahul Lakkireddy,
Rasesh Mody, Sony Chacko, Wenzhuo Lu, Yong Wang, Yuanhan Liu,
Nelio Laranjeiro
Cc: dev@dpdk.org, Thomas Monjalon
In-Reply-To: <F35DEAC7BCE34641BA9FAC6BCA4A12E717F493A3@SHSMSX103.ccr.corp.intel.com>
Hi Helin and PMD maintainers,
On Tue, Nov 08, 2016 at 01:31:05AM +0000, Zhang, Helin wrote:
> Hi Adrien
>
> Any update on the v1 APIs? We are struggling on that, as we need that for our development.
> May I bring another idea to remove the blocking?
> Can we send out the APIs with PMD changes based on our understaning of the RFC we discussed recenlty on community? Then you can just update any modification on top of it, or ask the submittors to change with your review comments?
> Any comments on this idea? If not, then we may go this way. I guess this might be the most efficient way. Thank you very much!
Not wanting to hold back anyone's progress anymore (not that I was doing it
on purpose), here's my work tree with the updated and functional API
(rte_flow branch based on top of v16.11-rc3) while I'm preparing the
patchset for official submission:
https://github.com/am6/dpdk.org/tree/rte_flow
As a work in progress, this branch is subject to change.
API changes since RFC v2:
- New separate VLAN pattern item (previously part of the ETH definition),
found to be much more convenient.
- Removed useless "any" field from VF pattern item, the same effect can be
achieved by not providing a specification structure.
- Replaced bit-fields from the VXLAN pattern item to avoid endianness
conversion issues on 24-bit fields.
- Updated struct rte_flow_item with a new "last" field to create inclusive
ranges. They are defined as the interval between (spec & mask) and
(last & mask). All three parameters are optional.
- Renamed ID action MARK.
- Renamed "queue" fields in actions QUEUE and DUP to "index".
- "rss_conf" field in RSS action is now const.
- VF action now uses a 32 bit ID like its pattern item counterpart.
- Removed redundant struct rte_flow_pattern, API functions now expect struct
rte_flow_item lists terminated by END items.
- Replaced struct rte_flow_actions for the same reason, with struct
rte_flow_action lists terminated by END actions.
- Error types (enum rte_flow_error_type) have been updated and the cause
pointer in struct rte_flow_error is now const.
- Function prototypes (rte_flow_create, rte_flow_validate) have also been
updated for clarity.
Additions:
- Public wrapper functions rte_flow_{validate|create|destroy|flush|query}
are now implemented in rte_flow.c, with their symbols exported and
versioned. Related filter type RTE_ETH_FILTER_GENERIC has been added.
- A separate header (rte_flow_driver.h) has been added for driver-side
functionality, in particular struct rte_flow_ops which contains PMD
callbacks returned by RTE_ETH_FILTER_GENERIC query.
- testpmd now exposes most of this API through the new "flow" command.
What remains to be done:
- Using endian-aware integer types (rte_beX_t) where necessary for clarity.
- API documentation (based on RFC).
- testpmd flow command documentation (although context-aware command
completion should already help quite a bit in this regard).
- A few pattern item / action properties cannot be configured yet
(e.g. rss_conf parameter for RSS action) and a few completions
(e.g. possible queue IDs) should be added.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Thomas Monjalon @ 2016-11-09 10:22 UTC (permalink / raw)
To: Björn Töpel
Cc: Ananyev, Konstantin, dev, helin.zhang, Xu, Qian Q, Yao, Lei A,
Wu, Jingjing
In-Reply-To: <5ad83b54-f13b-787e-c056-958f5cb8bd61@intel.com>
2016-11-09 11:05, Björn Töpel:
> > BTW, all other examples would experience same problem too, right?
>
> Correct, so the broader question would be "what is the correct behavior
> for an example application, when a port configuration isn't supported by
> the hardware?".
>
> My stand, FWIW, is that igb and ixgbe should have the same semantics as
> i40e currently has, i.e. return an error to the user if the port is
> mis-configured, NOT changing the setting behind the users back.
Yes it sounds sane.
^ permalink raw reply
* Re: [PATCH v7 00/21] Introduce SoC device/driver framework for EAL
From: Thomas Monjalon @ 2016-11-09 10:17 UTC (permalink / raw)
To: Shreyansh Jain; +Cc: dev, viktorin, david.marchand
In-Reply-To: <1477657598-826-1-git-send-email-shreyansh.jain@nxp.com>
Hi Shreyansh,
I realize that we had a lot of off-list discussions on this topic
and there was no public explanation of the status of this series.
2016-10-28 17:56, Shreyansh Jain:
[...]
> As of now EAL is primarly focused on PCI initialization/probing.
Right. DPDK was PCI centric.
We must give PCI its right role: a bus as other ones.
A first step was done in 16.11 (thanks to you Shreyansh, Jan and David)
to have a better device model.
The next step is to rework the bus abstraction.
It seems a bus can be defined with the properties scan/match/notify,
leading to initialize the devices.
More comments below your technical presentation.
[...]
> This patchset introduces SoC framework which would enable SoC drivers and
> drivers to be plugged into EAL, very similar to how PCI drivers/devices are
> done today.
>
> This is a stripped down version of PCI framework which allows the SoC PMDs
> to implement their own routines for detecting devices and linking devices to
> drivers.
>
> 1) Changes to EAL
> rte_eal_init()
> |- rte_eal_pci_init(): Find PCI devices from sysfs
> |- rte_eal_soc_init(): Calls PMDs->scan_fn
> |- ...
> |- rte_eal_memzone_init()
> |- ...
> |- rte_eal_pci_probe(): Driver<=>Device initialization, PMD->devinit()
> `- rte_eal_soc_probe(): Calls PMDs->match_fn and PMDs->devinit();
>
> 2) New device/driver structures:
> - rte_soc_driver (inheriting rte_driver)
> - rte_soc_device (inheriting rte_device)
> - rte_eth_dev and eth_driver embedded rte_soc_device and rte_soc_driver,
> respectively.
>
> 3) The SoC PMDs need to:
> - define rte_soc_driver with necessary scan and match callbacks
> - Register themselves using DRIVER_REGISTER_SOC()
> - Implement respective bus scanning in the scan callbacks to add necessary
> devices to SoC device list
> - Implement necessary eth_dev_init/uninint for ethernet instances
These callbacks are not specific to a SoC.
By the way a SoC defines nothing specific. You are using the SoC word
as an equivalent of non-PCI.
We must have a bus abstraction like the one you are defining for the SoC
but it must be generic and defined on top of PCI, so we can plug any
bus in it: PCI, vdev (as a software bus), any other well-defined bus,
and some driver-specific bus which can be implemented directly in the
driver (the NXP case AFAIK).
> 4) Design considerations that are same as PCI:
> - SoC initialization is being done through rte_eal_init(), just after PCI
> initialization is done.
> - As in case of PCI, probe is done after rte_eal_pci_probe() to link the
> devices detected with the drivers registered.
> - Device attach/detach functions are available and have been designed on
> the lines of PCI framework.
> - PMDs register using DRIVER_REGISTER_SOC, very similar to
> DRIVER_REGISTER_PCI for PCI devices.
> - Linked list of SoC driver and devices exists independent of the other
> driver/device list, but inheriting rte_driver/rte_driver, these are
> also part of a global list.
>
> 5) Design considerations that are different from PCI:
> - Each driver implements its own scan and match function. PCI uses the BDF
> format to read the device from sysfs, but this _may_not_ be a case for a
> SoC ethernet device.
> = This is an important change from initial proposal by Jan in [2].
> Unlike his attempt to use /sys/bus/platform, this patch relies on the
> PMD to detect the devices. This is because SoC may require specific or
> additional info for device detection. Further, SoC may have embedded
> devices/MACs which require initialization which cannot be covered
> through sysfs parsing.
> `-> Point (6) below is a side note to above.
> = PCI based PMDs rely on EAL's capability to detect devices. This
> proposal puts the onus on PMD to detect devices, add to soc_device_list
> and wait for Probe. Matching, of device<=>driver is again PMD's
> callback.
These PCI considerations can be described as a PCI bus implementation in EAL.
> 6) Adding default scan and match helpers for PMDs
> - The design warrrants the PMDs implement their own scan of devices
> on bus, and match routines for probe implementation.
> This patch introduces helpers which can be used by PMDs for scan of
> the platform bus and matching devices against the compatible string
> extracted from the scan.
> - Intention is to make it easier to integrate known SoC which expose
> platform bus compliant information (compat, sys/bus/platform...).
> - PMDs which have deviations from this standard model can implement and
> hook their bus scanning and probe match callbacks while registering
> driver.
Yes we can have EAL helpers to scan sys/bus/platform or other common formats.
Then the PMD can use them to implement its specific bus.
I know that David, you and me are on the same page to agree on this generic
design. I hope you will be able to achieve this work soon.
The goal is to be able to plug a SoC driver in DPDK 17.02 with a clean design.
My advice to make reviews and contributions easier, is to split the work in
few steps:
- clean PCI code (generalize non-PCI stuff)
- add generic bus functions
- plug PCI in generic bus abstraction
- plug vdev in generic bus abstraction
- plug the new NXP driver
Thanks
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Björn Töpel @ 2016-11-09 10:05 UTC (permalink / raw)
To: Ananyev, Konstantin, dev@dpdk.org, helin.zhang
Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon@6wind.com
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0D2F6C@irsmsx105.ger.corp.intel.com>
Adding Helin to the conversation.
> That's a common problem across Intel VF devices. Bothe igb and ixgbe
> overcomes that problem just by forcing ' rxmode.hw_strip_crc = 1;' at
> PMD itself:
[...]
> Wonder, can't i40e VF do the same?
Right, however, and this (silent failure) approach was rejected by the
maintainers. Please, refer to this thread:
http://dpdk.org/ml/archives/dev/2016-April/037523.html
> BTW, all other examples would experience same problem too, right?
Correct, so the broader question would be "what is the correct behavior
for an example application, when a port configuration isn't supported by
the hardware?".
My stand, FWIW, is that igb and ixgbe should have the same semantics as
i40e currently has, i.e. return an error to the user if the port is
mis-configured, NOT changing the setting behind the users back.
Björn
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Ananyev, Konstantin @ 2016-11-09 9:46 UTC (permalink / raw)
To: Topel, Bjorn, dev@dpdk.org
Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon@6wind.com,
Topel, Bjorn
In-Reply-To: <20161109082341.19825-1-bjorn.topel@intel.com>
Hi,
>
> Commit 1bbcc5d21129 ("i40evf: report error for unsupported CRC
> stripping config") broke l3fwd, since it was forcing that CRC was
> kept. Now, if i40evf is running, CRC stripping will be enabled.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> examples/l3fwd/main.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 7223e773107e..b60278794135 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -906,6 +906,14 @@ main(int argc, char **argv)
> n_tx_queue = MAX_TX_QUEUE_PER_PORT;
> printf("Creating queues: nb_rxq=%d nb_txq=%u... ",
> nb_rx_queue, (unsigned)n_tx_queue );
> + rte_eth_dev_info_get(portid, &dev_info);
> + if (dev_info.driver_name &&
> + strcmp(dev_info.driver_name, "net_i40e_vf") == 0) {
> + /* i40evf require that CRC stripping is enabled. */
> + port_conf.rxmode.hw_strip_crc = 1;
> + } else {
> + port_conf.rxmode.hw_strip_crc = 0;
> + }
That's a common problem across Intel VF devices.
Bothe igb and ixgbe overcomes that problem just by forcing ' rxmode.hw_strip_crc = 1;'
at PMD itself:
static int
ixgbevf_dev_configure(struct rte_eth_dev *dev)
{
...
/*
* VF has no ability to enable/disable HW CRC
* Keep the persistent behavior the same as Host PF
*/
#ifndef RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC
if (!conf->rxmode.hw_strip_crc) {
PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip");
conf->rxmode.hw_strip_crc = 1;
}
#else
if (conf->rxmode.hw_strip_crc) {
PMD_INIT_LOG(NOTICE, "VF can't enable HW CRC Strip");
conf->rxmode.hw_strip_crc = 0;
}
#endif
Wonder, can't i40e VF do the same?
BTW, all other examples would experience same problem too, right?
Konstantin
> ret = rte_eth_dev_configure(portid, nb_rx_queue,
> (uint16_t)n_tx_queue, &port_conf);
> if (ret < 0)
> @@ -946,7 +954,6 @@ main(int argc, char **argv)
> printf("txq=%u,%d,%d ", lcore_id, queueid, socketid);
> fflush(stdout);
>
> - rte_eth_dev_info_get(portid, &dev_info);
> txconf = &dev_info.default_txconf;
> if (port_conf.rxmode.jumbo_frame)
> txconf->txq_flags = 0;
> --
> 2.9.3
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Björn Töpel @ 2016-11-09 9:39 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, qian.q.xu, lei.a.yao, jingjing.wu
In-Reply-To: <1500406.Wuc0qeu8Dv@xps13>
> Thanks for raising the issue. It is completely defeating the generic
> ethdev API. We must not have different behaviours depending of the
> driver. Why it cannot be fixed in the driver?
I should probably refer to the thread, where the concern was raised:
http://dpdk.org/ml/archives/dev/2016-July/044555.html
So, the issue is that i40evf *only support* CRC stripping for some
setups (i40e Linux driver for PF, i40evf DPDK driver VF).
The l3fwd application disables CRC stripping for all ports, which leads
to i40evf_dev_configure() failure -- which from my POV is correct.
Mis-configuring a port shouldn't be allowed.
I'm open to suggestions here. What would be a better way to solve this?
Maybe just adding a command-line option to the l3fwd application is a
better way around?
Björn
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Thomas Monjalon @ 2016-11-09 9:28 UTC (permalink / raw)
To: Björn Töpel; +Cc: dev, qian.q.xu, lei.a.yao, jingjing.wu
In-Reply-To: <20161109082341.19825-1-bjorn.topel@intel.com>
2016-11-09 09:23, Björn Töpel:
> Commit 1bbcc5d21129 ("i40evf: report error for unsupported CRC
> stripping config") broke l3fwd, since it was forcing that CRC was
> kept. Now, if i40evf is running, CRC stripping will be enabled.
[...]
> + rte_eth_dev_info_get(portid, &dev_info);
> + if (dev_info.driver_name &&
> + strcmp(dev_info.driver_name, "net_i40e_vf") == 0) {
> + /* i40evf require that CRC stripping is enabled. */
> + port_conf.rxmode.hw_strip_crc = 1;
> + } else {
> + port_conf.rxmode.hw_strip_crc = 0;
> + }
Thanks for raising the issue.
It is completely defeating the generic ethdev API.
We must not have different behaviours depending of the driver.
Why it cannot be fixed in the driver?
^ permalink raw reply
* Re: [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Yao, Lei A @ 2016-11-09 8:37 UTC (permalink / raw)
To: Topel, Bjorn, dev@dpdk.org
Cc: Xu, Qian Q, Wu, Jingjing, thomas.monjalon@6wind.com
In-Reply-To: <20161109082341.19825-1-bjorn.topel@intel.com>
Tested-by: Lei Yao <lei.a.yao@intel.com>
- Apply patch to v16.11-rc3
- Compile: Pass
- Host OS: VMware ESXi 6.0
- VM OS: Fedora 20
- GCC: 4.8.3
Tested with this patch, l3fwd sample can work with i40e VF with Fedora VM using VMware as the host.
-----Original Message-----
From: Topel, Bjorn
Sent: Wednesday, November 9, 2016 4:24 PM
To: dev@dpdk.org
Cc: Xu, Qian Q <qian.q.xu@intel.com>; Yao, Lei A <lei.a.yao@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; thomas.monjalon@6wind.com; Topel, Bjorn <bjorn.topel@intel.com>
Subject: [PATCH] examples/l3fwd: force CRC stripping for i40evf
Commit 1bbcc5d21129 ("i40evf: report error for unsupported CRC stripping config") broke l3fwd, since it was forcing that CRC was kept. Now, if i40evf is running, CRC stripping will be enabled.
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
examples/l3fwd/main.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c index 7223e773107e..b60278794135 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -906,6 +906,14 @@ main(int argc, char **argv)
n_tx_queue = MAX_TX_QUEUE_PER_PORT;
printf("Creating queues: nb_rxq=%d nb_txq=%u... ",
nb_rx_queue, (unsigned)n_tx_queue );
+ rte_eth_dev_info_get(portid, &dev_info);
+ if (dev_info.driver_name &&
+ strcmp(dev_info.driver_name, "net_i40e_vf") == 0) {
+ /* i40evf require that CRC stripping is enabled. */
+ port_conf.rxmode.hw_strip_crc = 1;
+ } else {
+ port_conf.rxmode.hw_strip_crc = 0;
+ }
ret = rte_eth_dev_configure(portid, nb_rx_queue,
(uint16_t)n_tx_queue, &port_conf);
if (ret < 0)
@@ -946,7 +954,6 @@ main(int argc, char **argv)
printf("txq=%u,%d,%d ", lcore_id, queueid, socketid);
fflush(stdout);
- rte_eth_dev_info_get(portid, &dev_info);
txconf = &dev_info.default_txconf;
if (port_conf.rxmode.jumbo_frame)
txconf->txq_flags = 0;
--
2.9.3
^ permalink raw reply
* [PATCH] examples/l3fwd: force CRC stripping for i40evf
From: Björn Töpel @ 2016-11-09 8:23 UTC (permalink / raw)
To: dev
Cc: qian.q.xu, lei.a.yao, jingjing.wu, thomas.monjalon,
Björn Töpel
Commit 1bbcc5d21129 ("i40evf: report error for unsupported CRC
stripping config") broke l3fwd, since it was forcing that CRC was
kept. Now, if i40evf is running, CRC stripping will be enabled.
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
examples/l3fwd/main.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 7223e773107e..b60278794135 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -906,6 +906,14 @@ main(int argc, char **argv)
n_tx_queue = MAX_TX_QUEUE_PER_PORT;
printf("Creating queues: nb_rxq=%d nb_txq=%u... ",
nb_rx_queue, (unsigned)n_tx_queue );
+ rte_eth_dev_info_get(portid, &dev_info);
+ if (dev_info.driver_name &&
+ strcmp(dev_info.driver_name, "net_i40e_vf") == 0) {
+ /* i40evf require that CRC stripping is enabled. */
+ port_conf.rxmode.hw_strip_crc = 1;
+ } else {
+ port_conf.rxmode.hw_strip_crc = 0;
+ }
ret = rte_eth_dev_configure(portid, nb_rx_queue,
(uint16_t)n_tx_queue, &port_conf);
if (ret < 0)
@@ -946,7 +954,6 @@ main(int argc, char **argv)
printf("txq=%u,%d,%d ", lcore_id, queueid, socketid);
fflush(stdout);
- rte_eth_dev_info_get(portid, &dev_info);
txconf = &dev_info.default_txconf;
if (port_conf.rxmode.jumbo_frame)
txconf->txq_flags = 0;
--
2.9.3
^ permalink raw reply related
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