* Re: [PATCH 1/3] mbuf: embedding timestamp into the packet
From: Oleg Kuporosov @ 2016-10-19 17:40 UTC (permalink / raw)
To: Pattan, Reshma, Olivier Matz
Cc: Richardson, Bruce, Ananyev, Konstantin, dev@dpdk.org
In-Reply-To: <3AEA2BF9852C6F48A459DA490692831F010AFE30@IRSMSX109.ger.corp.intel.com>
Hello Reshma
> I just read this mail chain, to make every one aware again, I am emphasizing
> on the point that I am also adding new "time_arraived" field to mbuf struct as
> part of below 17.02 Road map item.
Thanks for your work for extending statistics support in DPDK.
"time_arrived" points here for mostly RX path, while more common term used "timestamp"
Allows also using it for TX path as well in the future.
Best regards,
Oleg.
^ permalink raw reply
* Re: [PATCH 1/3] mbuf: embedding timestamp into the packet
From: Oleg Kuporosov @ 2016-10-19 17:08 UTC (permalink / raw)
To: Olivier Matz, dev@dpdk.org
In-Reply-To: <efb55b81-339b-6706-12ef-d67bae6e6c5f@6wind.com>
Hello Oliver
Great thanks for your review and con
> > - include uint64_t timestamp field into rte_mbuf with minimal impact to
> > throughput/latency. Keep it just simple uint64_t in ns (more than 580
> > years) would be enough for immediate needs while using full
> > struct timespec with twice bigger size would have much stronger
> > performance impact as missed cacheline0.
> >
> > - it is possible as there is 6-bytes gap in 1st cacheline (fast path)
> > and moving uint16_t vlan_tci_outer field to 2nd cacheline.
> >
> > - such move will only impact for pretty rare usable VLAN RX stripping
> > mode for outer TCI (it used only for one NIC i40e from the whole set and
> > allows to keep minimal performance impact for RX/TX timestamps.
>
> This argument is difficult to accept. One can say you are adding a field for a
> pretty rare case used by only one NIC :)
It may be looks so, but in fact not only for one NIC. Absence of timestamp there
Required from developers implement its support in out of DPDK data path with
Local DPDK patching which also may lead some penalty in accuracy.
Good example here is open source implementation of tracing library -
https://github.com/wanduow/libtrace/tree/libtrace4
These folks patched DPDK to have timestamp for every packet (Intel DPDK Patches folder)
And used it in out of band to store and later processing (lib/format_dpdk.c).
That was actually the starting point of my investigations.
Such approach is working for 1GBb case but not for 10-100 cases.
>
> Honestly, I'm not able to judge whether timestamp is more important than
> vlan_tci_outer. As room is tight in the first cache line, your patch submission
> is the occasion to raise the question: how to decide what should be in the
> first part of the mbuf? There are also some other candidates for moving: m-
> >seqn is only used in librte_reorder and it is not set in the RX part of a driver.
Agree, it is difficult to decide, my thoughts were:
- there is hole (6 bytes) which wasn't marked as reserved for any planned extensions;
-vlan_tci_outer is being used by only one NIC (i40e) so far, 2nd NIC (fm10k) is using it
With comment:
* mbuf->vlan_tci_outer is an idle field in fm10k driver,
* so it can be selected to store sglort value.
To store some another value under some specific "if".
Also for i40e it is under #ifndef RTE_LIBRTE_I40E_16BYTE_RX_DESC which per doc should be
Enabled for high throughput of small packets. So in default case (disabled) it anyway has
some performance penalty with using 32 bytes descriptor and moving it to 2nd CL would
not hit big additional penalty. Unfortunately I have no such NIC to measure.
Is there any data how often double tagging in being used in DPDK applications?
Another my thought was to have at the end of 1st CL enum which may hold
Reserved fields per specific use cases and data widths (uint8, 2xuint4, 4xuint2, 8xbytes).
>
> About the timestamp, it would be valuable to have other opinions, not only
> about the placement of the field in the structure, but also to check that this
> API is also usable for other NICs.
Sure, but I didn't change timesync/timestamping API itself.
> Have you measured the impact of having the timestamp in the second part
> of the mbuf?
Yes, the worst case with prefetching of 2nd CL it is 5.7-5.9 % penalty for combined RX+TX
And expectedly much worse without prefetching.
In the best case it is 0.3..0.5 % for RX only. It can be explained by much harder cache
trashing when TX is "on".
> Changing the mbuf structure should happen as rarely as possible, and we
> have to make sure we take the correct decisions. I think we will discuss this at
> dpdk userland 2016.
Oh, yes, please discuss, I would not be able to join. :(
> Apart from that, I wonder if an ol_flag should be added to tell that the
> timestamp field is valid in the mbuf.
Oliver, there is PKT_RX_IEEE1588_TMST flag already.
Best regards,
Oleg.
^ permalink raw reply
* Re: [opnfv-tech-discuss] [apex][ovsnfv][fuel]Problem showed up with OVS/DPDK with Cisco VIC adapter
From: Tim Rozet @ 2016-10-19 17:00 UTC (permalink / raw)
To: Michal Skalski
Cc: Thomas F Herbert, Gregory Elkinbard, Fedor Zhadaev, dev,
MichalSkalski, yunhong.jiang@linux.intel.com <yunhong.jiang@linux.intel.com>, opnfv-tech-discuss@lists.opnfv.org,
Edward Warnicke, Frank Brockners (fbrockne)
In-Reply-To: <DC4FF210-03E6-4864-AA2E-DF8BC6955E62@mirantis.com>
Right, makes sense. Thanks Michal.
Tim Rozet
Red Hat SDN Team
----- Original Message -----
From: "Michal Skalski" <mskalski@mirantis.com>
To: "Tim Rozet" <trozet@redhat.com>, "Thomas F Herbert" <therbert@redhat.com>
Cc: "Gregory Elkinbard" <gelkinbard@mirantis.com>, "Fedor Zhadaev" <fzhadaev@mirantis.com>, dev@dpdk.org, "MichalSkalski <mskalski@mirantis.com>, yunhong.jiang@linux.intel.com <yunhong.jiang@linux.intel.com>, opnfv-tech-discuss@lists.opnfv.org" <opnfv-tech-discuss@lists.opnfv.org>, "Edward Warnicke" <hagbard@gmail.com>, "Frank Brockners (fbrockne)" <fbrockne@cisco.com>
Sent: Wednesday, October 19, 2016 11:11:33 AM
Subject: Re: [opnfv-tech-discuss] [apex][ovsnfv][fuel]Problem showed up with OVS/DPDK with Cisco VIC adapter
Hi,
Im looking at your fix now [1] as I understand it is only visible when we are using vxlan segmentation right?
Currently Fuel support dpdk only with vlan segmentation, and this is how we deploy on linux foundation pod, so we may not seen this problem yet.
[1]https://github.com/trozet/opnfv-tht/blob/stable/colorado/puppet/extraconfig/pre_deploy/compute/manifests/dpdk_dataplanes.pp#L151-L157
Michal
> On 19 Oct 2016, at 16:27, Tim Rozet <trozet@redhat.com> wrote:
>
> Fuel also supports the ovs dpdk scenario.
>
> Greg, Michal, Fedor,
> How do does Fuel get around this on the LF UCS hardware?
>
> Tim Rozet
> Red Hat SDN Team
>
> ----- Original Message -----
> From: "Thomas F Herbert" <therbert@redhat.com>
> To: dev@dpdk.org
> Cc: opnfv-tech-discuss@lists.opnfv.org, "Edward Warnicke" <hagbard@gmail.com>
> Sent: Tuesday, October 18, 2016 4:35:04 PM
> Subject: [opnfv-tech-discuss] [apex][ovsnfv]Problem showed up with OVS/DPDK with Cisco VIC adapter
>
>
>
> All:
>
>
> This is not necessarily related to VPP but rather to OVS/DPDK.
>
> In OPNFV we found the following problem when using UCS NIC.
>
> The UCS fabric seems to set a VLAN tag on untagged packets.
>
>
> Any thoughts from DPDK and VPP folks would be appreciated.
>
>
> http://dpdk.org/doc/guides-16.07/nics/enic.html
>
>
> https://jira.opnfv.org/browse/APEX-333
>
>
> It is currently deploying DPDK 16.04 and OVS 2.5.90
>
> I could not find a user accessible setting to do this that would work. The workaround for now we came up with is to set a flow in OVS to strip the tag.
>
>
> https://gerrit.opnfv.org/gerrit/#/c/23315/
>
> Another solution is to set the nic to strip the vlan in rte eth code or write a DPDK "app" to set the nic.
>
>
> http://dpdk.org/doc/guides-16.07/nics/enic.html
>
>
> --TFH
>
> --
> Thomas F Herbert
> SDN Group
> Office of Technology
> Red Hat
>
> _______________________________________________
> opnfv-tech-discuss mailing list
> opnfv-tech-discuss@lists.opnfv.org
> https://lists.opnfv.org/mailman/listinfo/opnfv-tech-discuss
^ permalink raw reply
* [PATCH] vhost: use after free
From: Chas Williams @ 2016-10-19 16:09 UTC (permalink / raw)
To: dev; +Cc: Chas Williams
Don't dereference freed memory.
Fixes: a277c7159876 ("vhost: refactor code structure")
Signed-off-by: Chas Williams <3chas3@gmail.com>
---
lib/librte_vhost/socket.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 967cb65..aaa9c27 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -250,8 +250,8 @@ vhost_user_read_cb(int connfd, void *dat, int *remove)
vsocket->connfd = -1;
close(connfd);
*remove = 1;
- free(conn);
vhost_destroy_device(conn->vid);
+ free(conn);
if (vsocket->reconnect)
vhost_user_create_client(vsocket);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v6 1/6] ethdev: add Tx preparation
From: Kulasek, TomaszX @ 2016-10-19 15:42 UTC (permalink / raw)
To: Olivier Matz, dev@dpdk.org; +Cc: Ananyev, Konstantin, thomas.monjalon@6wind.com
In-Reply-To: <0e011fea-ddd4-283b-61db-8a7a67b652e2@6wind.com>
Hi Olivier,
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, October 18, 2016 16:57
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> thomas.monjalon@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v6 1/6] ethdev: add Tx preparation
>
> Hi Tomasz,
>
> I think the principle of tx_prep() is good, it may for instance help to
> remove the function virtio_tso_fix_cksum() from the virtio, and maybe even
> change the mbuf TSO/cksum API.
>
> I have some questions/comments below, I'm sorry it comes very late.
>
> On 10/14/2016 05:05 PM, Tomasz Kulasek wrote:
> > Added API for `rte_eth_tx_prep`
> >
> > uint16_t rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id,
> > struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> >
> > Added fields to the `struct rte_eth_desc_lim`:
> >
> > uint16_t nb_seg_max;
> > /**< Max number of segments per whole packet. */
> >
> > uint16_t nb_mtu_seg_max;
> > /**< Max number of segments per one MTU */
>
> Not sure I understand the second one. Is this is case of TSO?
>
> Is it a usual limitation in different network hardware?
> Can this info be retrieved/used by the application?
>
Yes, limitation of number of segments may differ depend of TSO/non TSO, e.g. for Fortville NIC. This information is available for application
> >
> > Created `rte_pkt.h` header with common used functions:
> >
> > int rte_validate_tx_offload(struct rte_mbuf *m)
> > to validate general requirements for tx offload in packet such a
> > flag completness. In current implementation this function is called
> > optionaly when RTE_LIBRTE_ETHDEV_DEBUG is enabled.
> >
> > int rte_phdr_cksum_fix(struct rte_mbuf *m)
> > to fix pseudo header checksum for TSO and non-TSO tcp/udp packets
> > before hardware tx checksum offload.
> > - for non-TSO tcp/udp packets full pseudo-header checksum is
> > counted and set.
> > - for TSO the IP payload length is not included.
>
> Why not in rte_net.h?
>
>
> > [...]
> >
> > @@ -2816,6 +2825,82 @@ rte_eth_tx_burst(uint8_t port_id, uint16_t
> queue_id,
> > return (*dev->tx_pkt_burst)(dev->data->tx_queues[queue_id], tx_pkts,
> > nb_pkts); }
> >
> > +/**
> > + * Process a burst of output packets on a transmit queue of an Ethernet
> device.
> > + *
> > + * The rte_eth_tx_prep() function is invoked to prepare output
> > +packets to be
> > + * transmitted on the output queue *queue_id* of the Ethernet device
> > +designated
> > + * by its *port_id*.
> > + * The *nb_pkts* parameter is the number of packets to be prepared
> > +which are
> > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, each of
> > +them
> > + * allocated from a pool created with rte_pktmbuf_pool_create().
> > + * For each packet to send, the rte_eth_tx_prep() function performs
> > + * the following operations:
> > + *
> > + * - Check if packet meets devices requirements for tx offloads.
>
> Do you mean hardware requirements?
> Can the application be aware of these requirements? I mean capability
> flags, or something in dev_infos?
Yes. If some offloads cannot be handled by hardware, it fails. Also if e.g. number of segments is invalid and so on.
>
> Maybe the comment could be more precise?
>
> > + * - Check limitations about number of segments.
> > + *
> > + * - Check additional requirements when debug is enabled.
>
> What kind of additional requirements?
>
We may assume, that application setts right, e.g. ip header version is required for most of checksum offloads. To not have additional performance overhead, these checks are done only when debug is on.
> > + *
> > + * - Update and/or reset required checksums when tx offload is set for
> packet.
> > + *
>
> By reading this, I think it may not be clear for the user about what
> should be set in the mbuf. In mbuf API, it is said:
>
> * TCP segmentation offload. To enable this offload feature for a
> * packet to be transmitted on hardware supporting TSO:
> * - set the PKT_TX_TCP_SEG flag in mbuf->ol_flags (this flag implies
> * PKT_TX_TCP_CKSUM)
> * - set the flag PKT_TX_IPV4 or PKT_TX_IPV6
> * - if it's IPv4, set the PKT_TX_IP_CKSUM flag and write the IP checksum
> * to 0 in the packet
> * - fill the mbuf offload information: l2_len, l3_len, l4_len, tso_segsz
> * - calculate the pseudo header checksum without taking ip_len in
> account,
> * and set it in the TCP header. Refer to rte_ipv4_phdr_cksum() and
> * rte_ipv6_phdr_cksum() that can be used as helpers.
>
>
> If I understand well, using tx_prep(), the user will have to do the same
> except writing the IP checksum to 0, and without setting the TCP pseudo
> header checksum, right?
>
Right, but this header information is still valid for tx_burst operation done without preparation stage.
>
> > + * The rte_eth_tx_prep() function returns the number of packets ready
> > + to be
> > + * sent. A return value equal to *nb_pkts* means that all packets are
> > + valid and
> > + * ready to be sent.
> > + *
> > + * @param port_id
> > + * The port identifier of the Ethernet device.
> > + * @param queue_id
> > + * The index of the transmit queue through which output packets must
> be
> > + * sent.
> > + * The value must be in the range [0, nb_tx_queue - 1] previously
> supplied
> > + * to rte_eth_dev_configure().
> > + * @param tx_pkts
> > + * The address of an array of *nb_pkts* pointers to *rte_mbuf*
> structures
> > + * which contain the output packets.
> > + * @param nb_pkts
> > + * The maximum number of packets to process.
> > + * @return
> > + * The number of packets correct and ready to be sent. The return
> value can be
> > + * less than the value of the *tx_pkts* parameter when some packet
> doesn't
> > + * meet devices requirements with rte_errno set appropriately.
> > + */
>
> Can we add the constraint that invalid packets are left untouched?
>
> I think most of the time there will be a software fallback in that case,
> so it would be good to ensure that this function does not change the flags
> or the packet data.
In current implementation, if packet is invalid, its data is never modified. Only checks are done. The only exception is when checksum needs to be updated or initialized, but it's done after packet validation.
If we want to use/restore packet in application it didn't should be changed in any way for invalid packets.
>
> Another thing that could be interesting for the caller is to know the
> reason of the failure. Maybe the different errno types could be detailed
> here. For instance:
> - EINVAL: offload flags are not correctly set (i.e. would fail whatever
> the hardware)
> - ENOTSUP: the offload feature is not supported by the hardware
> - ...
>
Ok.
> > +
> > +#ifdef RTE_ETHDEV_TX_PREP
> > +
> > +static inline uint16_t
> > +rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id, struct rte_mbuf
> **tx_pkts,
> > + uint16_t nb_pkts)
> > +{
> > + struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > +
> > + if (!dev->tx_pkt_prep)
> > + return nb_pkts;
> > +
> > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > + if (queue_id >= dev->data->nb_tx_queues) {
> > + RTE_PMD_DEBUG_TRACE("Invalid TX queue_id=%d\n", queue_id);
> > + rte_errno = -EINVAL;
> > + return 0;
> > + }
> > +#endif
>
> Why checking the queue_id but not the port_id?
>
> Maybe the API comment should also be modified to say that the port_id has
> to be valid, because most ethdev functions do the check.
>
I can add this check when debug is on to made it more complete, and update an API comment for the default case.
> > +
> > + return (*dev->tx_pkt_prep)(dev->data->tx_queues[queue_id],
> > + tx_pkts, nb_pkts);
> > +}
> > +
> > +#else
> > +
> > +static inline uint16_t
> > +rte_eth_tx_prep(__rte_unused uint8_t port_id, __rte_unused uint16_t
> queue_id,
> > + __rte_unused struct rte_mbuf **tx_pkts, uint16_t nb_pkts) {
> > + return nb_pkts;
> > +}
> > +
> > +#endif
> > +
>
> nit: I wonder if the #else part should be inside the function instead
> (with the proper RTE_SET_USED()), it would avoid to define the prototype
> twice.
>
> > typedef void (*buffer_tx_error_fn)(struct rte_mbuf **unsent, uint16_t
> count,
> > void *userdata);
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 109e666..cfd6284 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -276,6 +276,15 @@ extern "C" {
> > */
> > #define PKT_TX_OUTER_IPV4 (1ULL << 59)
> >
> > +#define PKT_TX_OFFLOAD_MASK ( \
> > + PKT_TX_IP_CKSUM | \
> > + PKT_TX_L4_MASK | \
> > + PKT_TX_OUTER_IP_CKSUM | \
> > + PKT_TX_TCP_SEG | \
> > + PKT_TX_QINQ_PKT | \
> > + PKT_TX_VLAN_PKT | \
> > + PKT_TX_TUNNEL_MASK)
> > +
>
> Could you add an API comment?
>
> > --- /dev/null
> > +++ b/lib/librte_net/rte_pkt.h
> >
> > [...]
> > +/**
> > + * Validate general requirements for tx offload in packet.
> > + */
>
> The API comment does not have the usual format.
>
> > +static inline int
> > +rte_validate_tx_offload(struct rte_mbuf *m)
>
> should be const struct rte_mbuf *m
>
> > +{
> > + uint64_t ol_flags = m->ol_flags;
> > +
> > + /* Does packet set any of available offloads? */
> > + if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
> > + return 0;
> > +
> > + /* IP checksum can be counted only for IPv4 packet */
> > + if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
> > + return -EINVAL;
> > +
> > + if (ol_flags & (PKT_TX_L4_MASK | PKT_TX_TCP_SEG))
> > + /* IP type not set */
> > + if (!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6)))
> > + return -EINVAL;
> > +
> > + if (ol_flags & PKT_TX_TCP_SEG)
> > + /* PKT_TX_IP_CKSUM offload not set for IPv4 TSO packet */
> > + if ((m->tso_segsz == 0) ||
> > + ((ol_flags & PKT_TX_IPV4) && !(ol_flags &
> PKT_TX_IP_CKSUM)))
> > + return -EINVAL;
> > +
> > + /* PKT_TX_OUTER_IP_CKSUM set for non outer IPv4 packet. */
> > + if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) && !(ol_flags &
> PKT_TX_OUTER_IPV4))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
>
> It looks this function is only used when RTE_LIBRTE_ETHDEV_DEBUG is set.
>
Yes, for performance reasons we expect, that application sets these values right. This is the most of "additional checks" mentioned before.
> I'd say this function should go in rte_mbuf.h, because it's purely related
> to mbuf flags, and does not rely on packet data or network headers.
>
>
> > +
> > +/**
> > + * Fix pseudo header checksum for TSO and non-TSO tcp/udp packets
> > +before
> > + * hardware tx checksum.
> > + * For non-TSO tcp/udp packets full pseudo-header checksum is counted
> and set.
> > + * For TSO the IP payload length is not included.
> > + */
>
> The API comment should be fixed.
Ok.
>
> > +static inline int
> > +rte_phdr_cksum_fix(struct rte_mbuf *m) {
> > + struct ipv4_hdr *ipv4_hdr;
> > + struct ipv6_hdr *ipv6_hdr;
> > + struct tcp_hdr *tcp_hdr;
> > + struct udp_hdr *udp_hdr;
> > + uint64_t ol_flags = m->ol_flags;
> > + uint64_t inner_l3_offset = m->l2_len;
> > +
> > + if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
> > + inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> > +
> > + if ((ol_flags & PKT_TX_UDP_CKSUM) == PKT_TX_UDP_CKSUM) {
> > + if (ol_flags & PKT_TX_IPV4) {
> > + ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
> > + inner_l3_offset);
> > +
> > + if (ol_flags & PKT_TX_IP_CKSUM)
> > + ipv4_hdr->hdr_checksum = 0;
> > +
> > + udp_hdr = (struct udp_hdr *)((char *)ipv4_hdr + m-
> >l3_len);
> > + udp_hdr->dgram_cksum = rte_ipv4_phdr_cksum(ipv4_hdr,
> ol_flags);
> > + } else {
> > + ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct ipv6_hdr *,
> > + inner_l3_offset);
> > + /* non-TSO udp */
> > + udp_hdr = rte_pktmbuf_mtod_offset(m, struct udp_hdr *,
> > + inner_l3_offset + m->l3_len);
> > + udp_hdr->dgram_cksum = rte_ipv6_phdr_cksum(ipv6_hdr,
> ol_flags);
> > + }
> > + } else if ((ol_flags & PKT_TX_TCP_CKSUM) ||
> > + (ol_flags & PKT_TX_TCP_SEG)) {
> > + if (ol_flags & PKT_TX_IPV4) {
> > + ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
> > + inner_l3_offset);
> > +
> > + if (ol_flags & PKT_TX_IP_CKSUM)
> > + ipv4_hdr->hdr_checksum = 0;
> > +
> > + /* non-TSO tcp or TSO */
> > + tcp_hdr = (struct tcp_hdr *)((char *)ipv4_hdr + m-
> >l3_len);
> > + tcp_hdr->cksum = rte_ipv4_phdr_cksum(ipv4_hdr, ol_flags);
> > + } else {
> > + ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct ipv6_hdr *,
> > + inner_l3_offset);
> > + /* non-TSO tcp or TSO */
> > + tcp_hdr = rte_pktmbuf_mtod_offset(m, struct tcp_hdr *,
> > + inner_l3_offset + m->l3_len);
> > + tcp_hdr->cksum = rte_ipv6_phdr_cksum(ipv6_hdr, ol_flags);
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +#endif /* _RTE_PKT_H_ */
> >
>
> The function expects that all the network headers are in the first, and
> that each of them is contiguous.
>
Yes, I see...
> Also, I had an interesting remark from Stephen [1] on a similar code.
> If the mbuf is a clone, it will modify the data of the direct mbuf, which
> should be read-only. Note that it is likely to happen in a TCP stack,
> because the packet is kept locally in case it has to be retransmitted.
> Cloning a mbuf is more efficient than duplicating it.
>
> I plan to fix it in virtio code by "uncloning" the headers.
>
> [1] http://dpdk.org/ml/archives/dev/2016-October/048873.html
>
>
>
> Regards,
> Olivier
Thanks,
Tomasz
^ permalink raw reply
* Re: [apex][ovsnfv][fuel]Problem showed up with OVS/DPDK with Cisco VIC adapter
From: Michal Skalski @ 2016-10-19 15:11 UTC (permalink / raw)
To: Tim Rozet, Thomas F Herbert
Cc: Edward Warnicke, dev-VfR2kkLFssw,
MichalSkalski <mskalski-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>, yunhong.jiang-VuQAYsv1563Yd54FQh9/CA@public.gmane.org <yunhong.jiang-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, opnfv-tech-discuss-ZwoEplunGu2fIMufGqXHGA@public.gmane.org
In-Reply-To: <323284952.3467106.1476887275706.JavaMail.zimbra-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Hi,
Im looking at your fix now [1] as I understand it is only visible when we are using vxlan segmentation right?
Currently Fuel support dpdk only with vlan segmentation, and this is how we deploy on linux foundation pod, so we may not seen this problem yet.
[1]https://github.com/trozet/opnfv-tht/blob/stable/colorado/puppet/extraconfig/pre_deploy/compute/manifests/dpdk_dataplanes.pp#L151-L157
Michal
> On 19 Oct 2016, at 16:27, Tim Rozet <trozet-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Fuel also supports the ovs dpdk scenario.
>
> Greg, Michal, Fedor,
> How do does Fuel get around this on the LF UCS hardware?
>
> Tim Rozet
> Red Hat SDN Team
>
> ----- Original Message -----
> From: "Thomas F Herbert" <therbert-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> To: dev-VfR2kkLFssw@public.gmane.org
> Cc: opnfv-tech-discuss-ZwoEplunGu2fIMufGqXHGA@public.gmane.org, "Edward Warnicke" <hagbard-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Sent: Tuesday, October 18, 2016 4:35:04 PM
> Subject: [opnfv-tech-discuss] [apex][ovsnfv]Problem showed up with OVS/DPDK with Cisco VIC adapter
>
>
>
> All:
>
>
> This is not necessarily related to VPP but rather to OVS/DPDK.
>
> In OPNFV we found the following problem when using UCS NIC.
>
> The UCS fabric seems to set a VLAN tag on untagged packets.
>
>
> Any thoughts from DPDK and VPP folks would be appreciated.
>
>
> http://dpdk.org/doc/guides-16.07/nics/enic.html
>
>
> https://jira.opnfv.org/browse/APEX-333
>
>
> It is currently deploying DPDK 16.04 and OVS 2.5.90
>
> I could not find a user accessible setting to do this that would work. The workaround for now we came up with is to set a flow in OVS to strip the tag.
>
>
> https://gerrit.opnfv.org/gerrit/#/c/23315/
>
> Another solution is to set the nic to strip the vlan in rte eth code or write a DPDK "app" to set the nic.
>
>
> http://dpdk.org/doc/guides-16.07/nics/enic.html
>
>
> --TFH
>
> --
> Thomas F Herbert
> SDN Group
> Office of Technology
> Red Hat
>
> _______________________________________________
> opnfv-tech-discuss mailing list
> opnfv-tech-discuss-ZwoEplunGu2fIMufGqXHGA@public.gmane.org
> https://lists.opnfv.org/mailman/listinfo/opnfv-tech-discuss
^ permalink raw reply
* [PATCH v2 2/2] app/test_pmd: change to the VF VLAN insert command
From: Bernard Iremonger @ 2016-10-19 14:47 UTC (permalink / raw)
To: dev, daniels, wenzhuo.lu, az5157; +Cc: Bernard Iremonger
In-Reply-To: <1476818007-13659-1-git-send-email-daniels@research.att.com>
The third parameter to the function rte_pmd_ixgbe_set_vf_vlan_insert
has changed to vlan_id from on|off.
The testpmd doc file has been changed to reflect this change.
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
app/test-pmd/cmdline.c | 19 +++++++++----------
doc/guides/testpmd_app_ug/testpmd_funcs.rst | 2 +-
2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index e0e4fe4..15dbd2c 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -286,7 +286,7 @@ static void cmd_help_long_parsed(void *parsed_result,
"set vf vlan stripq (port_id) (vf_id) (on|off)\n"
" Set the VLAN strip for all queues in a pool for a VF from the PF.\n\n"
- "set vf vlan insert (port_id) (vf_id) (on|off)\n"
+ "set vf vlan insert (port_id) (vf_id) (vlan_id)\n"
" Set VLAN insert for a VF from the PF.\n\n"
"set vf vlan antispoof (port_id) (vf_id) (on|off)\n"
@@ -11017,7 +11017,7 @@ struct cmd_vf_vlan_insert_result {
cmdline_fixed_string_t insert;
uint8_t port_id;
uint16_t vf_id;
- cmdline_fixed_string_t on_off;
+ uint16_t vlan_id;
};
/* Common CLI fields for vf vlan insert enable disable */
@@ -11045,10 +11045,10 @@ cmdline_parse_token_num_t cmd_vf_vlan_insert_vf_id =
TOKEN_NUM_INITIALIZER
(struct cmd_vf_vlan_insert_result,
vf_id, UINT16);
-cmdline_parse_token_string_t cmd_vf_vlan_insert_on_off =
- TOKEN_STRING_INITIALIZER
+cmdline_parse_token_num_t cmd_vf_vlan_insert_vlan_id =
+ TOKEN_NUM_INITIALIZER
(struct cmd_vf_vlan_insert_result,
- on_off, "on#off");
+ vlan_id, UINT16);
static void
cmd_set_vf_vlan_insert_parsed(
@@ -11058,14 +11058,13 @@ cmd_set_vf_vlan_insert_parsed(
{
struct cmd_vf_vlan_insert_result *res = parsed_result;
int ret;
- int is_on = (strcmp(res->on_off, "on") == 0) ? 1 : 0;
- ret = rte_pmd_ixgbe_set_vf_vlan_insert(res->port_id, res->vf_id, is_on);
+ ret = rte_pmd_ixgbe_set_vf_vlan_insert(res->port_id, res->vf_id, res->vlan_id);
switch (ret) {
case 0:
break;
case -EINVAL:
- printf("invalid vf_id %d or is_on %d\n", res->vf_id, is_on);
+ printf("invalid vf_id %d or vlan_id %d\n", res->vf_id, res->vlan_id);
break;
case -ENODEV:
printf("invalid port_id %d\n", res->port_id);
@@ -11078,7 +11077,7 @@ cmd_set_vf_vlan_insert_parsed(
cmdline_parse_inst_t cmd_set_vf_vlan_insert = {
.f = cmd_set_vf_vlan_insert_parsed,
.data = NULL,
- .help_str = "set vf vlan insert port_id vf_id on|off",
+ .help_str = "set vf vlan insert port_id vf_id vlan_id",
.tokens = {
(void *)&cmd_vf_vlan_insert_set,
(void *)&cmd_vf_vlan_insert_vf,
@@ -11086,7 +11085,7 @@ cmdline_parse_inst_t cmd_set_vf_vlan_insert = {
(void *)&cmd_vf_vlan_insert_insert,
(void *)&cmd_vf_vlan_insert_port_id,
(void *)&cmd_vf_vlan_insert_vf_id,
- (void *)&cmd_vf_vlan_insert_on_off,
+ (void *)&cmd_vf_vlan_insert_vlan_id,
NULL,
},
};
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index c04805b..9a45932 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -533,7 +533,7 @@ vlan set insert (for VF)
Set VLAN insert for a VF from the PF::
- testpmd> set vf vlan insert (port_id) (vf_id) (on|off)
+ testpmd> set vf vlan insert (port_id) (vf_id) (vlan_id)
vlan set antispoof (for VF)
~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
2.10.1
^ permalink raw reply related
* [PATCH v2 1/2] net/ixgbe: fix VLAN insert parameter type and its use
From: Bernard Iremonger @ 2016-10-19 14:47 UTC (permalink / raw)
To: dev, daniels, wenzhuo.lu, az5157
In-Reply-To: <1476818007-13659-1-git-send-email-daniels@research.att.com>
From: "E. Scott Daniels" <daniels@research.att.com>
The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t
and treated as a binary flag when it needs to be a uint16_t
and treated as a VLAN id. The data sheet (sect 8.2.3.27.13) describes
the right most 16 bits as the VLAN id that is to be inserted; the
16.11 code is accepting only a 1 or 0 thus effectively only
allowing the VLAN id 1 to be inserted (0 disables the insertion
setting).
This patch changes the final parm name to represent the data that
is being accepted (vlan_id), changes the type to permit all valid
VLAN ids, and validates the parameter based on the range of 0 to
4095. Corresponding changes to prototype and documentation in the
.h file.
Fixes: 49e248223e9f71 ("net/ixgbe: add API for VF management")
Signed-off-by: E. Scott Daniels <daniels@research.att.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++----
drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 4ca5747..316af73 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port, uint16_t vf, uint8_t on)
}
int
-rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
+rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t vlan_id)
{
struct ixgbe_hw *hw;
uint32_t ctrl;
@@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
if (vf >= dev_info.max_vfs)
return -EINVAL;
- if (on > 1)
+ if (vlan_id > 4095)
return -EINVAL;
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
- if (on) {
- ctrl = on;
+ if (vlan_id) {
+ ctrl = vlan_id;
ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
} else {
ctrl = 0;
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h b/drivers/net/ixgbe/rte_pmd_ixgbe.h
index 2fdf530..c2fb826 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
@@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port, uint16_t vf, uint8_t on);
* The port identifier of the Ethernet device.
* @param vf
* ID specifying VF.
- * @param on
- * 1 - Enable VF's vlan insert.
- * 0 - Disable VF's vlan insert
+ * @param vlan_id
+ * 0 - Disable VF's vlan insert.
+ * n - Enable; n is inserted as the vlan id.
*
* @return
* - (0) if successful.
* - (-ENODEV) if *port* invalid.
* - (-EINVAL) if bad parameter.
*/
-int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on);
+int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
+ uint16_t vlan_id);
/**
* Enable/Disable tx loopback
--
2.10.1
^ permalink raw reply related
* [PATCH v2 0/2] net/ixgbe: fix VF VLAN insert
From: Bernard Iremonger @ 2016-10-19 14:47 UTC (permalink / raw)
To: dev, daniels, wenzhuo.lu, az5157; +Cc: Bernard Iremonger
In-Reply-To: <1476818007-13659-1-git-send-email-daniels@research.att.com>
Changes in v2:
Add testpmd patch.
Update testpmd for change to rte_pmd_ixgbe_set_vf_vlan_insert function.
Bernard Iremonger (1):
app/test_pmd: change to the VF VLAN insert command
E. Scott Daniels (1):
net/ixgbe: fix VLAN insert parameter type and its use
app/test-pmd/cmdline.c | 19 +++++++++----------
doc/guides/testpmd_app_ug/testpmd_funcs.rst | 2 +-
drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++----
drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
4 files changed, 19 insertions(+), 19 deletions(-)
--
2.10.1
^ permalink raw reply
* Re: [PATCH RFC 0/2] Allow vectorized Rx with 4096 desc ring size on Intel NICs.
From: Ferruh Yigit @ 2016-10-19 14:30 UTC (permalink / raw)
To: Ilya Maximets, dev, Helin Zhang, Konstantin Ananyev, Jingjing Wu
Cc: Dyasly Sergey, Heetae Ahn, Bruce Richardson
In-Reply-To: <1476886037-4586-1-git-send-email-i.maximets@samsung.com>
On 10/19/2016 3:07 PM, Ilya Maximets wrote:
> Ilya Maximets (2):
> net/i40e: allow bulk alloc for the max size desc ring
> net/ixgbe: allow bulk alloc for the max size desc ring
>
> drivers/net/i40e/i40e_rxtx.c | 24 +++++++++++++-----------
> drivers/net/ixgbe/ixgbe_rxtx.c | 17 +----------------
> drivers/net/ixgbe/ixgbe_rxtx.h | 2 +-
> 3 files changed, 15 insertions(+), 28 deletions(-)
>
Hi Ilya,
Thank you for the patch, we are in post rc1 phase and this is a new
feature, so this patchset will be considered for next release (v17.02).
Thanks,
ferruh
^ permalink raw reply
* Re: [opnfv-tech-discuss] [apex][ovsnfv][fuel]Problem showed up with OVS/DPDK with Cisco VIC adapter
From: Tim Rozet @ 2016-10-19 14:27 UTC (permalink / raw)
To: Thomas F Herbert, Gregory Elkinbard, mskalski, fzhadaev
Cc: dev, opnfv-tech-discuss, Edward Warnicke,
Frank Brockners (fbrockne)
In-Reply-To: <2986fc44-b2a1-f58c-62e0-eaf238a4fe08@redhat.com>
Fuel also supports the ovs dpdk scenario.
Greg, Michal, Fedor,
How do does Fuel get around this on the LF UCS hardware?
Tim Rozet
Red Hat SDN Team
----- Original Message -----
From: "Thomas F Herbert" <therbert@redhat.com>
To: dev@dpdk.org
Cc: opnfv-tech-discuss@lists.opnfv.org, "Edward Warnicke" <hagbard@gmail.com>
Sent: Tuesday, October 18, 2016 4:35:04 PM
Subject: [opnfv-tech-discuss] [apex][ovsnfv]Problem showed up with OVS/DPDK with Cisco VIC adapter
All:
This is not necessarily related to VPP but rather to OVS/DPDK.
In OPNFV we found the following problem when using UCS NIC.
The UCS fabric seems to set a VLAN tag on untagged packets.
Any thoughts from DPDK and VPP folks would be appreciated.
http://dpdk.org/doc/guides-16.07/nics/enic.html
https://jira.opnfv.org/browse/APEX-333
It is currently deploying DPDK 16.04 and OVS 2.5.90
I could not find a user accessible setting to do this that would work. The workaround for now we came up with is to set a flow in OVS to strip the tag.
https://gerrit.opnfv.org/gerrit/#/c/23315/
Another solution is to set the nic to strip the vlan in rte eth code or write a DPDK "app" to set the nic.
http://dpdk.org/doc/guides-16.07/nics/enic.html
--TFH
--
Thomas F Herbert
SDN Group
Office of Technology
Red Hat
^ permalink raw reply
* [PATCH RFC 2/2] net/ixgbe: allow bulk alloc for the max size desc ring
From: Ilya Maximets @ 2016-10-19 14:07 UTC (permalink / raw)
To: dev, Helin Zhang, Konstantin Ananyev, Jingjing Wu
Cc: Dyasly Sergey, Heetae Ahn, Bruce Richardson, Ilya Maximets
In-Reply-To: <1476886037-4586-1-git-send-email-i.maximets@samsung.com>
The only reason why bulk alloc disabled for the rings with
more than (IXGBE_MAX_RING_DESC - RTE_PMD_IXGBE_RX_MAX_BURST)
descriptors is the possible out-of-bound access to the dma
memory. But it's the artificial limit and can be easily
avoided by allocating of RTE_PMD_IXGBE_RX_MAX_BURST more
descriptors in memory. This will not interfere the HW and,
as soon as all rings' memory zeroized, Rx functions will
work correctly.
This change allows to use vectorized Rx functions with
4096 descriptors in Rx ring which is important to achieve
zero packet drop rate in high-load installations.
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
drivers/net/ixgbe/ixgbe_rxtx.c | 17 +----------------
drivers/net/ixgbe/ixgbe_rxtx.h | 2 +-
2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 2ce8234..07c04c3 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2585,7 +2585,6 @@ check_rx_burst_bulk_alloc_preconditions(struct ixgbe_rx_queue *rxq)
* rxq->rx_free_thresh >= RTE_PMD_IXGBE_RX_MAX_BURST
* rxq->rx_free_thresh < rxq->nb_rx_desc
* (rxq->nb_rx_desc % rxq->rx_free_thresh) == 0
- * rxq->nb_rx_desc<(IXGBE_MAX_RING_DESC-RTE_PMD_IXGBE_RX_MAX_BURST)
* Scattered packets are not supported. This should be checked
* outside of this function.
*/
@@ -2607,15 +2606,6 @@ check_rx_burst_bulk_alloc_preconditions(struct ixgbe_rx_queue *rxq)
"rxq->rx_free_thresh=%d",
rxq->nb_rx_desc, rxq->rx_free_thresh);
ret = -EINVAL;
- } else if (!(rxq->nb_rx_desc <
- (IXGBE_MAX_RING_DESC - RTE_PMD_IXGBE_RX_MAX_BURST))) {
- PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions: "
- "rxq->nb_rx_desc=%d, "
- "IXGBE_MAX_RING_DESC=%d, "
- "RTE_PMD_IXGBE_RX_MAX_BURST=%d",
- rxq->nb_rx_desc, IXGBE_MAX_RING_DESC,
- RTE_PMD_IXGBE_RX_MAX_BURST);
- ret = -EINVAL;
}
return ret;
@@ -2632,12 +2622,7 @@ ixgbe_reset_rx_queue(struct ixgbe_adapter *adapter, struct ixgbe_rx_queue *rxq)
/*
* By default, the Rx queue setup function allocates enough memory for
* IXGBE_MAX_RING_DESC. The Rx Burst bulk allocation function requires
- * extra memory at the end of the descriptor ring to be zero'd out. A
- * pre-condition for using the Rx burst bulk alloc function is that the
- * number of descriptors is less than or equal to
- * (IXGBE_MAX_RING_DESC - RTE_PMD_IXGBE_RX_MAX_BURST). Check all the
- * constraints here to see if we need to zero out memory after the end
- * of the H/W descriptor ring.
+ * extra memory at the end of the descriptor ring to be zero'd out.
*/
if (adapter->rx_bulk_alloc_allowed)
/* zero out extra memory */
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 2608b36..1abc6f2 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -67,7 +67,7 @@
#define RTE_IXGBE_MAX_RX_BURST RTE_IXGBE_RXQ_REARM_THRESH
#endif
-#define RX_RING_SZ ((IXGBE_MAX_RING_DESC + RTE_IXGBE_DESCS_PER_LOOP - 1) * \
+#define RX_RING_SZ ((IXGBE_MAX_RING_DESC + RTE_PMD_IXGBE_RX_MAX_BURST) * \
sizeof(union ixgbe_adv_rx_desc))
#ifdef RTE_PMD_PACKET_PREFETCH
--
2.7.4
^ permalink raw reply related
* [PATCH RFC 1/2] net/i40e: allow bulk alloc for the max size desc ring
From: Ilya Maximets @ 2016-10-19 14:07 UTC (permalink / raw)
To: dev, Helin Zhang, Konstantin Ananyev, Jingjing Wu
Cc: Dyasly Sergey, Heetae Ahn, Bruce Richardson, Ilya Maximets
In-Reply-To: <1476886037-4586-1-git-send-email-i.maximets@samsung.com>
The only reason why bulk alloc disabled for the rings with
more than (I40E_MAX_RING_DESC - RTE_PMD_I40E_RX_MAX_BURST)
descriptors is the possible out-of-bound access to the dma
memory. But it's the artificial limit and can be easily
avoided by allocating of RTE_PMD_I40E_RX_MAX_BURST more
descriptors in memory. This will not interfere the HW and,
as soon as all rings' memory zeroized, Rx functions will
work correctly.
This change allows to use vectorized Rx functions with
4096 descriptors in Rx ring which is important to achieve
zero packet drop rate in high-load installations.
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
drivers/net/i40e/i40e_rxtx.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 7ae7d9f..1f76691 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -409,15 +409,6 @@ check_rx_burst_bulk_alloc_preconditions(__rte_unused struct i40e_rx_queue *rxq)
"rxq->rx_free_thresh=%d",
rxq->nb_rx_desc, rxq->rx_free_thresh);
ret = -EINVAL;
- } else if (!(rxq->nb_rx_desc < (I40E_MAX_RING_DESC -
- RTE_PMD_I40E_RX_MAX_BURST))) {
- PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions: "
- "rxq->nb_rx_desc=%d, "
- "I40E_MAX_RING_DESC=%d, "
- "RTE_PMD_I40E_RX_MAX_BURST=%d",
- rxq->nb_rx_desc, I40E_MAX_RING_DESC,
- RTE_PMD_I40E_RX_MAX_BURST);
- ret = -EINVAL;
}
#else
ret = -EINVAL;
@@ -1698,8 +1689,19 @@ i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
rxq->rx_deferred_start = rx_conf->rx_deferred_start;
/* Allocate the maximun number of RX ring hardware descriptor. */
- ring_size = sizeof(union i40e_rx_desc) * I40E_MAX_RING_DESC;
- ring_size = RTE_ALIGN(ring_size, I40E_DMA_MEM_ALIGN);
+ len = I40E_MAX_RING_DESC;
+
+#ifdef RTE_LIBRTE_I40E_RX_ALLOW_BULK_ALLOC
+ /**
+ * Allocating a little more memory because vectorized/bulk_alloc Rx
+ * functions doesn't check boundaries each time.
+ */
+ len += RTE_PMD_I40E_RX_MAX_BURST;
+#endif
+
+ ring_size = RTE_ALIGN(len * sizeof(union i40e_rx_desc),
+ I40E_DMA_MEM_ALIGN);
+
rz = rte_eth_dma_zone_reserve(dev, "rx_ring", queue_idx,
ring_size, I40E_RING_BASE_ALIGN, socket_id);
if (!rz) {
--
2.7.4
^ permalink raw reply related
* [PATCH RFC 0/2] Allow vectorized Rx with 4096 desc ring size on Intel NICs.
From: Ilya Maximets @ 2016-10-19 14:07 UTC (permalink / raw)
To: dev, Helin Zhang, Konstantin Ananyev, Jingjing Wu
Cc: Dyasly Sergey, Heetae Ahn, Bruce Richardson, Ilya Maximets
In-Reply-To: <CGME20161019140725eucas1p1cb53318d974fb1152f1a7b571f328fd5@eucas1p1.samsung.com>
Ilya Maximets (2):
net/i40e: allow bulk alloc for the max size desc ring
net/ixgbe: allow bulk alloc for the max size desc ring
drivers/net/i40e/i40e_rxtx.c | 24 +++++++++++++-----------
drivers/net/ixgbe/ixgbe_rxtx.c | 17 +----------------
drivers/net/ixgbe/ixgbe_rxtx.h | 2 +-
3 files changed, 15 insertions(+), 28 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use
From: Scott Daniels @ 2016-10-19 13:56 UTC (permalink / raw)
To: Iremonger, Bernard
Cc: Lu, Wenzhuo, Zhang, Helin, dev@dpdk.org, ZELEZNIAK, ALEX
In-Reply-To: <8CEF83825BEC744B83065625E567D7C21A093DA3@IRSMSX108.ger.corp.intel.com>
On Wed, 19 Oct 2016, Iremonger, Bernard wrote:
> Hi Scott,
>
> <snip>
>
>>>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter
>>>>> type and its use
>>>>>
>>>>> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t
>>>>> and treated as a binary flag when it needs to be a uint16_t and
>>>>> treated as a VLAN id. The data sheet (sect 8.2.3.27.13) describes
>>>>> the right most
>>>>> 16 bits as the VLAN id that is to be inserted; the
>>>>> 16.11 code is accepting only a 1 or 0 thus effectively only
>>>>> allowing the VLAN id 1 to be inserted (0 disables the insertion setting).
>>>>>
>>>>> This patch changes the final parm name to represent the data that is
>>>>> being accepted (vlan_id), changes the type to permit all valid VLAN
>>>>> ids, and validates the parameter based on the range of 0 to 4095.
>>>>> Corresponding changes to prototype and documentation in the .h file.
>>>>>
>>>>> Fixes: 49e248223e9f71 ("net/ixgbe: add API for VF management")
>>>>>
>>>>> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
>>>>> ---
>>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++----
>>>>> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
>>>>> 2 files changed, 9 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>> index 4ca5747..316af73 100644
>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>> @@ -4727,7 +4727,7 @@
>> rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
>>>>> port, uint16_t vf, uint8_t on) }
>>>>>
>>>>> int
>>>>> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t
>>>>> on)
>>>>> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>>>> +uint16_t
>>>>> +vlan_id)
>>>>> {
>>>>> struct ixgbe_hw *hw;
>>>>> uint32_t ctrl;
>>>>> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t
>>>> port,
>>>>> uint16_t vf, uint8_t on)
>>>>> if (vf >= dev_info.max_vfs)
>>>>> return -EINVAL;
>>>>>
>>>>> - if (on > 1)
>>>>> + if (vlan_id > 4095)
>>>>> return -EINVAL;
>>>>>
>>>>> hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>>>> ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
>>>>> - if (on) {
>>>>> - ctrl = on;
>>>>> + if (vlan_id) {
>>>>> + ctrl = vlan_id;
>>>> I believe this patch is a good idea of an enhancement. But you cannot
>>>> leverage the existing code like this.
>>>> The register IXGBE_VMVIR is only for enable/disable. We cannot write
>>>> the VLAN ID into it.
>>>> If you want to merge the 2 things, enabling/disabling and setting
>>>> VLAN ID together. I think we need a totally new implementation. So
>> NACK.
>>>
>>> Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is
>> correct.
>>> The NACK is not correct.
>>>
>>> However changing the API means that where it is called needs to be
>> changed too.
>>> It is called at present from app/testpmd/cmdline.c line 4731.
>>>
>>>
>> Bernard,
>> Should I add this change to the patch? (Didn't occur to me to look for use).
>>
>> Scott
>
> No, it would be better to make the testpmd change in a separate patch, and send a v2 patchset with two patches.
>
Will do, thanks. New to the patch world, so I appreciate your patience!
Scott
>
>>>>> ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
>>>>> } else {
>>>>> ctrl = 0;
>>>>> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>>>> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>>>> index 2fdf530..c2fb826 100644
>>>>> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>>>> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>>>> @@ -99,16 +99,17 @@ int
>> rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
>>>>> port, uint16_t vf, uint8_t on);
>>>>> * The port identifier of the Ethernet device.
>>>>> * @param vf
>>>>> * ID specifying VF.
>>>>> - * @param on
>>>>> - * 1 - Enable VF's vlan insert.
>>>>> - * 0 - Disable VF's vlan insert
>>>>> + * @param vlan_id
>>>>> + * 0 - Disable VF's vlan insert.
>>>>> + * n - Enable; n is inserted as the vlan id.
>>>>> *
>>>>> * @return
>>>>> * - (0) if successful.
>>>>> * - (-ENODEV) if *port* invalid.
>>>>> * - (-EINVAL) if bad parameter.
>>>>> */
>>>>> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>>>> uint8_t on);
>>>>> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>>>> + uint16_t vlan_id);
>>>>>
>>>>> /**
>>>>> * Enable/Disable tx loopback
>>>>> --
>>>>> 1.9.1
>
> Regards,
>
> Bernard.
>
>
^ permalink raw reply
* Re: [PATCH v4 04/32] net/qede/base: add HSI changes and register defines
From: Mody, Rasesh @ 2016-10-19 13:46 UTC (permalink / raw)
To: Ferruh Yigit, Rasesh Mody, thomas.monjalon@6wind.com,
bruce.richardson@intel.com
Cc: dev@dpdk.org, Dept-EngDPDKDev@qlogic.com
In-Reply-To: <e20fca51-a554-8aa7-40e8-569e1a9ae575@intel.com>
Hi Ferruh,
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Wednesday, October 19, 2016 5:38 AM
> To: Rasesh Mody <rasesh.mody@qlogic.com>;
> thomas.monjalon@6wind.com; bruce.richardson@intel.com
> Cc: dev@dpdk.org; Dept-EngDPDKDev@qlogic.com
> Subject: Re: [dpdk-dev] [PATCH v4 04/32] net/qede/base: add HSI changes
> and register defines
>
> On 10/19/2016 5:11 AM, Rasesh Mody wrote:
> > - add the hardware software interface(HSI) changes
> > - add register definitions
> >
> > These will be required for 8.10.9.0 FW upgrade.
> >
> > Signed-off-by: Rasesh Mody <rasesh.mody@qlogic.com>
>
> <...>
>
> > /*
> > * Igu cleanup bit values to distinguish between clean or producer
> consumer
> > */
> > @@ -1376,7 +1572,7 @@ struct dmae_cmd {
> > __le32 src_addr_hi;
> > __le32 dst_addr_lo;
> > __le32 dst_addr_hi;
> > - __le16 length /* Length in DW */;
> > + __le16 length_dw /* Length in DW */;
>
> This cause a compilation error [1] in patch by patch compilation, when debug
> is enabled. Fix seems straightforward [2].
>
> Do you confirm the fix?
> If you confirm, you don't need to send a new version, fix can be applied
> while getting the patch.
Please apply the fix [2] while getting the patch. The error [1] is due to debug option not enabled for individual patch compilation. We did test after applying entire series with all the config options enabled.
For our upcoming submissions, we'll take care of enabling the debug option when compile testing all the patches individually.
Thanks!
-Rasesh
>
> Thanks,
> ferruh
>
>
> [1]:
> ====
> In file included from .../drivers/net/qede/base/ecore.h:36:0,
> from .../drivers/net/qede/base/ecore_hw.c:12:
> .../drivers/net/qede/base/ecore_hw.c: In function
> 'ecore_dmae_post_command':
> .../drivers/net/qede/base/ecore_hw.c:478:20: error: 'struct dmae_cmd'
> has no member named 'length'; did you mean 'length_dw'?
> (int)p_command->length,
> ^
> .../drivers/net/qede/base/../qede_logs.h:47:11: note: in definition of macro
> 'DP_VERBOSE'
> ##__VA_ARGS__); \
> ^~~~~~~~~~~
> .../mk/internal/rte.compile-pre.mk:138: recipe for target 'base/ecore_hw.o'
> failed
>
>
> [2]:
> ====
> diff --git a/drivers/net/qede/base/ecore_hw.c
> b/drivers/net/qede/base/ecore_hw.c
> index 72bc6de..5412ed1 100644
> --- a/drivers/net/qede/base/ecore_hw.c
> +++ b/drivers/net/qede/base/ecore_hw.c
> @@ -475,7 +475,7 @@ ecore_dmae_post_command(struct ecore_hwfn
> *p_hwfn, struct ecore_ptt *p_ptt)
> "len=0x%x src=0x%x:%x dst=0x%x:%x\n",
> idx_cmd, (u32)p_command->opcode,
> (u16)p_command->opcode_b,
> - (int)p_command->length,
> + (int)p_command->length_dw,
> (int)p_command->src_addr_hi,
> (int)p_command->src_addr_lo,
> (int)p_command->dst_addr_hi, (int)p_command->dst_addr_lo);
>
^ permalink raw reply
* Re: [PATCH] drivers: make driver names consistent
From: Bruce Richardson @ 2016-10-19 13:32 UTC (permalink / raw)
To: Jan Blunck
Cc: Thomas Monjalon, Yuanhan Liu, De Lara Guarch, Pablo, dev,
Mcnamara, John
In-Reply-To: <CALe+Z00mtGMMWBveSwma2hjQgGpuotUOH+fz0nEKpxG4PDiY2w@mail.gmail.com>
On Tue, Oct 18, 2016 at 05:26:22PM +0200, Jan Blunck wrote:
> On Tue, Oct 18, 2016 at 3:42 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > 2016-10-18 21:06, Yuanhan Liu:
> >> On Tue, Oct 18, 2016 at 02:50:16PM +0200, Jan Blunck wrote:
> >> > >From my understanding this is a massive API breakage. This forces all
> >> > existing users of the virtual PMDs to change with zero benefit. Even
> >> > if that isn't enough it also makes it impossible to switch between
> >> > releases by recompiling.
> >> >
> >> > Can we please revert these changes and work on some aliasing support
> >> > for the PMDs to fix it long term?
> >>
> >> +1. Aliasing is also something I would suggest before making such renames.
> >
> > It is a brutal change, yes.
> > It was announced in 16.07 release notes though.
> >
>
> "But the plans were on display ..." (HHGTTG)
>
Yes, 16.07, better known as the "Beware of the leopard" release of DPDK.
:-)
/Bruce
^ permalink raw reply
* Re: [PATCH 1/3] mbuf: embedding timestamp into the packet
From: Ananyev, Konstantin @ 2016-10-19 13:31 UTC (permalink / raw)
To: Olivier Matz, dev@dpdk.org; +Cc: Oleg Kuporosov
In-Reply-To: <efb55b81-339b-6706-12ef-d67bae6e6c5f@6wind.com>
Hi lads,
>
>
>
> On 10/13/2016 04:35 PM, Oleg Kuporosov wrote:
> > The hard requirement of financial services industry is accurate
> > timestamping aligned with the packet itself. This patch is to satisfy
> > this requirement:
> >
> > - include uint64_t timestamp field into rte_mbuf with minimal impact to
> > throughput/latency. Keep it just simple uint64_t in ns (more than 580
> > years) would be enough for immediate needs while using full
> > struct timespec with twice bigger size would have much stronger
> > performance impact as missed cacheline0.
> >
> > - it is possible as there is 6-bytes gap in 1st cacheline (fast path)
> > and moving uint16_t vlan_tci_outer field to 2nd cacheline.
> >
> > - such move will only impact for pretty rare usable VLAN RX stripping
> > mode for outer TCI (it used only for one NIC i40e from the whole set and
> > allows to keep minimal performance impact for RX/TX timestamps.
>
> This argument is difficult to accept. One can say you are adding
> a field for a pretty rare case used by only one NIC :)
>
> Honestly, I'm not able to judge whether timestamp is more important than
> vlan_tci_outer. As room is tight in the first cache line, your patch
> submission is the occasion to raise the question: how to decide what
> should be in the first part of the mbuf? There are also some other
> candidates for moving: m->seqn is only used in librte_reorder and it
> is not set in the RX part of a driver.
>
> About the timestamp, it would be valuable to have other opinions,
> not only about the placement of the field in the structure, but also
> to check that this API is also usable for other NICs.
>
> Have you measured the impact of having the timestamp in the second part
> of the mbuf?
My vote also would be to have timestamp in the second cache line.
About moving seqn to the 2-nd cache line too - that's probably a fair point.
About the rest of the patch:
Do you really need to put that code into the PMDs itself?
Can't the same result be achieved by using RX callbacks?
Again that approach would work with any PMD and
there would be no need to modify PMD code itself.
Another thing, that I am in doubt why to use system time?
Wouldn't raw HW TSC value (rte_rdtsc()) do here?
Konstantin
>
> Changing the mbuf structure should happen as rarely as possible, and we
> have to make sure we take the correct decisions. I think we will
> discuss this at dpdk userland 2016.
>
>
> Apart from that, I wonder if an ol_flag should be added to tell that
> the timestamp field is valid in the mbuf.
>
> Regards,
> Olivier
^ permalink raw reply
* Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use
From: Iremonger, Bernard @ 2016-10-19 12:57 UTC (permalink / raw)
To: Scott Daniels; +Cc: Lu, Wenzhuo, Zhang, Helin, dev@dpdk.org, ZELEZNIAK, ALEX
In-Reply-To: <alpine.LSU.2.20.12.1610190848440.24883@localhost>
Hi Scott,
<snip>
> >>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter
> >>> type and its use
> >>>
> >>> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t
> >>> and treated as a binary flag when it needs to be a uint16_t and
> >>> treated as a VLAN id. The data sheet (sect 8.2.3.27.13) describes
> >>> the right most
> >>> 16 bits as the VLAN id that is to be inserted; the
> >>> 16.11 code is accepting only a 1 or 0 thus effectively only
> >>> allowing the VLAN id 1 to be inserted (0 disables the insertion setting).
> >>>
> >>> This patch changes the final parm name to represent the data that is
> >>> being accepted (vlan_id), changes the type to permit all valid VLAN
> >>> ids, and validates the parameter based on the range of 0 to 4095.
> >>> Corresponding changes to prototype and documentation in the .h file.
> >>>
> >>> Fixes: 49e248223e9f71 ("net/ixgbe: add API for VF management")
> >>>
> >>> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
> >>> ---
> >>> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++----
> >>> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
> >>> 2 files changed, 9 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> index 4ca5747..316af73 100644
> >>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> @@ -4727,7 +4727,7 @@
> rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
> >>> port, uint16_t vf, uint8_t on) }
> >>>
> >>> int
> >>> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t
> >>> on)
> >>> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> >>> +uint16_t
> >>> +vlan_id)
> >>> {
> >>> struct ixgbe_hw *hw;
> >>> uint32_t ctrl;
> >>> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t
> >> port,
> >>> uint16_t vf, uint8_t on)
> >>> if (vf >= dev_info.max_vfs)
> >>> return -EINVAL;
> >>>
> >>> - if (on > 1)
> >>> + if (vlan_id > 4095)
> >>> return -EINVAL;
> >>>
> >>> hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >>> ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
> >>> - if (on) {
> >>> - ctrl = on;
> >>> + if (vlan_id) {
> >>> + ctrl = vlan_id;
> >> I believe this patch is a good idea of an enhancement. But you cannot
> >> leverage the existing code like this.
> >> The register IXGBE_VMVIR is only for enable/disable. We cannot write
> >> the VLAN ID into it.
> >> If you want to merge the 2 things, enabling/disabling and setting
> >> VLAN ID together. I think we need a totally new implementation. So
> NACK.
> >
> > Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is
> correct.
> > The NACK is not correct.
> >
> > However changing the API means that where it is called needs to be
> changed too.
> > It is called at present from app/testpmd/cmdline.c line 4731.
> >
> >
> Bernard,
> Should I add this change to the patch? (Didn't occur to me to look for use).
>
> Scott
No, it would be better to make the testpmd change in a separate patch, and send a v2 patchset with two patches.
> >>> ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
> >>> } else {
> >>> ctrl = 0;
> >>> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> >>> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> >>> index 2fdf530..c2fb826 100644
> >>> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
> >>> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
> >>> @@ -99,16 +99,17 @@ int
> rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
> >>> port, uint16_t vf, uint8_t on);
> >>> * The port identifier of the Ethernet device.
> >>> * @param vf
> >>> * ID specifying VF.
> >>> - * @param on
> >>> - * 1 - Enable VF's vlan insert.
> >>> - * 0 - Disable VF's vlan insert
> >>> + * @param vlan_id
> >>> + * 0 - Disable VF's vlan insert.
> >>> + * n - Enable; n is inserted as the vlan id.
> >>> *
> >>> * @return
> >>> * - (0) if successful.
> >>> * - (-ENODEV) if *port* invalid.
> >>> * - (-EINVAL) if bad parameter.
> >>> */
> >>> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> >>> uint8_t on);
> >>> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
> >>> + uint16_t vlan_id);
> >>>
> >>> /**
> >>> * Enable/Disable tx loopback
> >>> --
> >>> 1.9.1
Regards,
Bernard.
^ permalink raw reply
* Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use
From: Scott Daniels @ 2016-10-19 12:50 UTC (permalink / raw)
To: Iremonger, Bernard
Cc: Lu, Wenzhuo, Zhang, Helin, dev@dpdk.org, ZELEZNIAK, ALEX
In-Reply-To: <8CEF83825BEC744B83065625E567D7C21A093CC1@IRSMSX108.ger.corp.intel.com>
------------------------------------------------------------------------
E. Scott Daniels
PMTS - Cloud Software Research
AT&T Labs - Research
daniels@research.att.com
440.389.0011
On Wed, 19 Oct 2016, Iremonger, Bernard wrote:
> Hi Wenzhuo, Scott,
>
> <snip>
>
>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type
>>> and its use
>>>
>>> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t and
>>> treated as a binary flag when it needs to be a uint16_t and treated as
>>> a VLAN id. The data sheet (sect 8.2.3.27.13) describes the right most
>>> 16 bits as the VLAN id that is to be inserted; the
>>> 16.11 code is accepting only a 1 or 0 thus effectively only allowing
>>> the VLAN id 1 to be inserted (0 disables the insertion setting).
>>>
>>> This patch changes the final parm name to represent the data that is
>>> being accepted (vlan_id), changes the type to permit all valid VLAN
>>> ids, and validates the parameter based on the range of 0 to 4095.
>>> Corresponding changes to prototype and documentation in the .h file.
>>>
>>> Fixes: 49e248223e9f71 ("net/ixgbe: add API for VF management")
>>>
>>> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
>>> ---
>>> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++----
>>> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
>>> 2 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>> index 4ca5747..316af73 100644
>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>> @@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
>>> port, uint16_t vf, uint8_t on) }
>>>
>>> int
>>> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t
>>> on)
>>> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t
>>> +vlan_id)
>>> {
>>> struct ixgbe_hw *hw;
>>> uint32_t ctrl;
>>> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t
>> port,
>>> uint16_t vf, uint8_t on)
>>> if (vf >= dev_info.max_vfs)
>>> return -EINVAL;
>>>
>>> - if (on > 1)
>>> + if (vlan_id > 4095)
>>> return -EINVAL;
>>>
>>> hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>> ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
>>> - if (on) {
>>> - ctrl = on;
>>> + if (vlan_id) {
>>> + ctrl = vlan_id;
>> I believe this patch is a good idea of an enhancement. But you cannot
>> leverage the existing code like this.
>> The register IXGBE_VMVIR is only for enable/disable. We cannot write the
>> VLAN ID into it.
>> If you want to merge the 2 things, enabling/disabling and setting VLAN ID
>> together. I think we need a totally new implementation. So NACK.
>
> Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is correct.
> The NACK is not correct.
>
> However changing the API means that where it is called needs to be changed too.
> It is called at present from app/testpmd/cmdline.c line 4731.
>
>
Bernard,
Should I add this change to the patch? (Didn't occur to me to look for
use).
Scott
>
>>> ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
>>> } else {
>>> ctrl = 0;
>>> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>> index 2fdf530..c2fb826 100644
>>> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>>> @@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t
>>> port, uint16_t vf, uint8_t on);
>>> * The port identifier of the Ethernet device.
>>> * @param vf
>>> * ID specifying VF.
>>> - * @param on
>>> - * 1 - Enable VF's vlan insert.
>>> - * 0 - Disable VF's vlan insert
>>> + * @param vlan_id
>>> + * 0 - Disable VF's vlan insert.
>>> + * n - Enable; n is inserted as the vlan id.
>>> *
>>> * @return
>>> * - (0) if successful.
>>> * - (-ENODEV) if *port* invalid.
>>> * - (-EINVAL) if bad parameter.
>>> */
>>> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>> uint8_t on);
>>> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>>> + uint16_t vlan_id);
>>>
>>> /**
>>> * Enable/Disable tx loopback
>>> --
>>> 1.9.1
>
> Regards,
>
> Bernard.
>
^ permalink raw reply
* Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use
From: Scott Daniels @ 2016-10-19 12:45 UTC (permalink / raw)
To: Lu, Wenzhuo
Cc: Zhang, Helin, Iremonger, Bernard, dev@dpdk.org, ZELEZNIAK, ALEX
In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com>
------------------------------------------------------------------------
E. Scott Daniels
PMTS - Cloud Software Research
AT&T Labs - Research
daniels@research.att.com
440.389.0011
On Wed, 19 Oct 2016, Lu, Wenzhuo wrote:
> Hi Daniels,
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of E. Scott Daniels
>> Sent: Wednesday, October 19, 2016 3:13 AM
>> To: Zhang, Helin; Iremonger, Bernard
>> Cc: dev@dpdk.org; az5157@att.com; E. Scott Daniels
>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use
>>
>> The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t and treated
>> as a binary flag when it needs to be a uint16_t and treated as a VLAN id. The
>> data sheet (sect 8.2.3.27.13) describes the right most 16 bits as the VLAN id that
>> is to be inserted; the
>> 16.11 code is accepting only a 1 or 0 thus effectively only allowing the VLAN id 1
>> to be inserted (0 disables the insertion setting).
>>
>> This patch changes the final parm name to represent the data that is being
>> accepted (vlan_id), changes the type to permit all valid VLAN ids, and validates
>> the parameter based on the range of 0 to 4095. Corresponding changes to
>> prototype and documentation in the .h file.
>>
>> Fixes: 49e248223e9f71 ("net/ixgbe: add API for VF management")
>>
>> Signed-off-by: E. Scott Daniels <daniels@research.att.com>
>> ---
>> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++----
>> drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++----
>> 2 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 4ca5747..316af73 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port,
>> uint16_t vf, uint8_t on) }
>>
>> int
>> -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on)
>> +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint16_t
>> +vlan_id)
>> {
>> struct ixgbe_hw *hw;
>> uint32_t ctrl;
>> @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port,
>> uint16_t vf, uint8_t on)
>> if (vf >= dev_info.max_vfs)
>> return -EINVAL;
>>
>> - if (on > 1)
>> + if (vlan_id > 4095)
>> return -EINVAL;
>>
>> hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>> ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf));
>> - if (on) {
>> - ctrl = on;
>> + if (vlan_id) {
>> + ctrl = vlan_id;
> I believe this patch is a good idea of an enhancement. But you cannot leverage the existing code like this.
> The register IXGBE_VMVIR is only for enable/disable. We cannot write the VLAN ID into it.
> If you want to merge the 2 things, enabling/disabling and setting VLAN ID together. I think we need a totally new implementation. So NACK.
>
I'm a bit confused. According to the data sheet (section 8.2.3.27.13)
this register accepts the "port VLAN tag to insert if the VLANA field =
01b" in the right most 16 bits. This allows the given tag to be inserted
in outgoing packets. The current implementation always sets this bit (via
the IXGBE_VMVIR_VLANA_DEFAULT constant) which causes the tag in the right
most bits to be inserted.
The current code, which I belive is broken, only ever inserts a 1 or 0
into the right most bits as it takes the value passed in and ORs it with
the constant, effectively only allowing the VLAN tag of 1 to be
inserted. The value passed in is not being used to set/reset the
always/never insert VLAN action bit.
The only way to insert VLAN tags 2 through 4095 is to modify the code to
accept the tag and insert it as described in the patch.
Scott
>> ctrl |= IXGBE_VMVIR_VLANA_DEFAULT;
>> } else {
>> ctrl = 0;
>> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>> b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>> index 2fdf530..c2fb826 100644
>> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h
>> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h
>> @@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t port,
>> uint16_t vf, uint8_t on);
>> * The port identifier of the Ethernet device.
>> * @param vf
>> * ID specifying VF.
>> - * @param on
>> - * 1 - Enable VF's vlan insert.
>> - * 0 - Disable VF's vlan insert
>> + * @param vlan_id
>> + * 0 - Disable VF's vlan insert.
>> + * n - Enable; n is inserted as the vlan id.
>> *
>> * @return
>> * - (0) if successful.
>> * - (-ENODEV) if *port* invalid.
>> * - (-EINVAL) if bad parameter.
>> */
>> -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t on);
>> +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf,
>> + uint16_t vlan_id);
>>
>> /**
>> * Enable/Disable tx loopback
>> --
>> 1.9.1
>
>
^ permalink raw reply
* Re: [PATCH v4 04/32] net/qede/base: add HSI changes and register defines
From: Ferruh Yigit @ 2016-10-19 12:37 UTC (permalink / raw)
To: Rasesh Mody, thomas.monjalon, bruce.richardson; +Cc: dev, Dept-EngDPDKDev
In-Reply-To: <1476850306-2141-5-git-send-email-rasesh.mody@qlogic.com>
On 10/19/2016 5:11 AM, Rasesh Mody wrote:
> - add the hardware software interface(HSI) changes
> - add register definitions
>
> These will be required for 8.10.9.0 FW upgrade.
>
> Signed-off-by: Rasesh Mody <rasesh.mody@qlogic.com>
<...>
> /*
> * Igu cleanup bit values to distinguish between clean or producer consumer
> */
> @@ -1376,7 +1572,7 @@ struct dmae_cmd {
> __le32 src_addr_hi;
> __le32 dst_addr_lo;
> __le32 dst_addr_hi;
> - __le16 length /* Length in DW */;
> + __le16 length_dw /* Length in DW */;
This cause a compilation error [1] in patch by patch compilation, when
debug is enabled. Fix seems straightforward [2].
Do you confirm the fix?
If you confirm, you don't need to send a new version, fix can be applied
while getting the patch.
Thanks,
ferruh
[1]:
====
In file included from .../drivers/net/qede/base/ecore.h:36:0,
from .../drivers/net/qede/base/ecore_hw.c:12:
.../drivers/net/qede/base/ecore_hw.c: In function ‘ecore_dmae_post_command’:
.../drivers/net/qede/base/ecore_hw.c:478:20: error: ‘struct dmae_cmd’
has no member named ‘length’; did you mean ‘length_dw’?
(int)p_command->length,
^
.../drivers/net/qede/base/../qede_logs.h:47:11: note: in definition of
macro ‘DP_VERBOSE’
##__VA_ARGS__); \
^~~~~~~~~~~
.../mk/internal/rte.compile-pre.mk:138: recipe for target
'base/ecore_hw.o' failed
[2]:
====
diff --git a/drivers/net/qede/base/ecore_hw.c
b/drivers/net/qede/base/ecore_hw.c
index 72bc6de..5412ed1 100644
--- a/drivers/net/qede/base/ecore_hw.c
+++ b/drivers/net/qede/base/ecore_hw.c
@@ -475,7 +475,7 @@ ecore_dmae_post_command(struct ecore_hwfn *p_hwfn,
struct ecore_ptt *p_ptt)
"len=0x%x src=0x%x:%x dst=0x%x:%x\n",
idx_cmd, (u32)p_command->opcode,
(u16)p_command->opcode_b,
- (int)p_command->length,
+ (int)p_command->length_dw,
(int)p_command->src_addr_hi,
(int)p_command->src_addr_lo,
(int)p_command->dst_addr_hi,
(int)p_command->dst_addr_lo);
^ permalink raw reply related
* Re: [PATCH] drivers: make driver names consistent
From: Thomas Monjalon @ 2016-10-19 12:26 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: Jan Blunck, De Lara Guarch, Pablo, dev, Mcnamara, John
In-Reply-To: <20161019100543.GQ16751@yliu-dev.sh.intel.com>
2016-10-19 18:05, Yuanhan Liu:
> On Tue, Oct 18, 2016 at 06:46:40PM +0200, Thomas Monjalon wrote:
> > 2016-10-18 22:18, Yuanhan Liu:
> > > On Tue, Oct 18, 2016 at 03:42:54PM +0200, Thomas Monjalon wrote:
> > > > 2016-10-18 21:06, Yuanhan Liu:
> > > > > On Tue, Oct 18, 2016 at 02:50:16PM +0200, Jan Blunck wrote:
> > > > > > >From my understanding this is a massive API breakage. This forces all
> > > > > > existing users of the virtual PMDs to change with zero benefit. Even
> > > > > > if that isn't enough it also makes it impossible to switch between
> > > > > > releases by recompiling.
> > > > > >
> > > > > > Can we please revert these changes and work on some aliasing support
> > > > > > for the PMDs to fix it long term?
> > > > >
> > > > > +1. Aliasing is also something I would suggest before making such renames.
> > > >
> > > > It is a brutal change, yes.
> > > > It was announced in 16.07 release notes though.
> > >
> > > Yes, but it still took me a while (by running git bisect) to figure out
> > > what went wrong: I wasn't aware of such note, that I was thinking maybe
> > > something is broken.
> > >
> > > Later I also got quite few same complains. It may also took them a while
> > > to know what's happened.
> > >
> > > Anyway, my point is, for this kind of change, we should have added the
> > > alias support firstly.
> >
> > Yes.
> >
> > > If that's been done, then the announcement is not
> > > needed at all?
> >
> > The announcement would be needed to remove the aliases, later.
>
> Why do you have to remove an aliase? What's wrong if they stay there
> forever. I think the naming will be switched (maybe slowly) when we
> haved updated the doc (whereas it states "net_vhost" but not "eth_vhost"
> only).
>
> After few releases, the name should have been switched fluently. And
> we should not care about those alias.
Yes
I was just thinking removing dead code later.
And it's generally simpler to end with only one way of doing things.
^ permalink raw reply
* Re: [PATCH v11 00/24] Introducing rte_driver/rte_device generalization
From: Shreyansh Jain @ 2016-10-19 11:13 UTC (permalink / raw)
To: Ferruh Yigit, Thomas Monjalon
Cc: dev@dpdk.org, viktorin@rehivetech.com, David Marchand,
Hemant Agrawal
In-Reply-To: <197a12fd-863a-b79a-f4b4-738636da7625@intel.com>
Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, October 18, 2016 2:53 PM
> To: Shreyansh Jain <shreyansh.jain@nxp.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org; viktorin@rehivetech.com; David Marchand
> <david.marchand@6wind.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH v11 00/24] Introducing rte_driver/rte_device
> generalization
>
> On 10/17/2016 6:29 PM, Shreyansh Jain wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >> Sent: Monday, October 17, 2016 7:13 PM
> >> To: Shreyansh Jain <shreyansh.jain@nxp.com>; Thomas Monjalon
> >> <thomas.monjalon@6wind.com>
> >> Cc: dev@dpdk.org; viktorin@rehivetech.com; David Marchand
> >> <david.marchand@6wind.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> >> Subject: Re: [dpdk-dev] [PATCH v11 00/24] Introducing
> rte_driver/rte_device
> >> generalization
> >>
> >> On 10/5/2016 12:57 PM, Shreyansh Jain wrote:
> >>> Hi Thomas,
> >>>
> >>> On Tuesday 04 October 2016 01:12 PM, Thomas Monjalon wrote:
> >>>> 2016-10-04 12:21, Shreyansh Jain:
> >>>>> Hi Thomas,
> >>>>>
> >>>>> On Monday 03 October 2016 07:58 PM, Thomas Monjalon wrote:
> >>>>>> Applied, thanks everybody for the great (re)work!
> >>>>>
> >>>>> Thanks!
> >>>>>
> >>> [...]
> >>> [...]
> >>>>>
> >>>>> It can be merged with changes for:
> >>>>> - drv_name
> >>>>> - EAL_ before _REGISTER_ macros
> >>>>> - eth_driver => rte_driver naming
> >>>>
> >>>> Good.
> >>>> Could you make it this week, please?
> >>>>
> >>>
> >>> Certainly. At least some of those I can send within this week :)
> >>>
> >>
> >>
> >> I caught while running ABI validation script today, I think this patch
> >> should increase LIBABIVER of:
> >> - lib/librte_cryptodev
> >> - lib/librte_eal
> >> - lib/librte_ether
> >
> > Should I be referring to [1] for understanding how/when to change the
> LIBABIVER?
> >
> > [1] http://dpdk.org/doc/guides/contributing/versioning.html
>
> Yes, this is the document.
>
> Briefly, if library becomes incompatible with existing applications,
> LIBABIVER needs to be increased to indicate this.
>
> Increasing LIBABIVER changes dynamic library name and so_name, and this
> cause existing application do not work with this new library. Not
> increasing the LIBABIVER, app may start running but can have segfault or
> can generate wrong values. So increasing LIBABIVER is to inform user and
> prevent surprises.
Thanks for explanation. I understand.
I will send across another patch for this.
-
Shreyansh
^ permalink raw reply
* Re: [PATCH 1/2] net/i40e: fix link status change interrupt
From: Ferruh Yigit @ 2016-10-19 10:56 UTC (permalink / raw)
To: Qiming Yang, dev; +Cc: Jingjing Wu
In-Reply-To: <1476338840-50003-1-git-send-email-qiming.yang@intel.com>
Hi Qiming,
On 10/13/2016 7:07 AM, Qiming Yang wrote:
> Previously, link status interrupt in i40e is achieved by checking
> LINK_STAT_CHANGE_MASK in PFINT_ICR0 register which is provided only
> for diagnostic use. Instead, drivers need to get the link status
> change notification by using LSE (Link Status Event).
>
> This patch enables LSE and calls LSC callback when the event is
> received. This patch also removes the processing on
> LINK_STAT_CHANGE_MASK.
>
> Fixes: 5c9222058df7 ("i40e: move to drivers/net/")
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
CC: Jingjing Wu <jingjing.wu@intel.com>
Can you please add maintainer(s) to CC when sending a patch, mail
traffic in dpdk.org is keep increasing and it is hard to follow. Adding
maintainer to the CC helps a lot to the maintainer.
I guess everybody knows, but just as a reminder, maintainer list kept in
file: http://dpdk.org/browse/dpdk/tree/MAINTAINERS
Thanks,
ferruh
^ 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