* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Stephen Hemminger @ 2016-11-23 19:21 UTC (permalink / raw)
To: Andrew Rybchenko; +Cc: Ferruh Yigit, dev
In-Reply-To: <5d041f12-ec8d-77da-47ac-671eb3a39a5b@solarflare.com>
On Wed, 23 Nov 2016 10:49:33 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> I've tried to explain it above in item (2):
>
> >>>
>
> 2. Another Solarflare PMD with in-kernel part (for control operations)
> is considered and could be added in the future. Code for data path
> should be shared by these two drivers. libefx-based PMD is put into
> 'efx' subdirectory to have a space for another PMD and shared code.
>
> <<<
>
> So, main reason is to have location for the code shared by two Solarflare
> network PMDs. May be it better to relocate when we really have it.
> I'm open for other ideas/suggestions.
So is this driver dependent on another non-upstream kernel driver to work?
Is this documented in docs directory. If it does depend on other non-upstream
components, then the default config should disable the driver.
^ permalink raw reply
* Re: [PATCH 2/4] eventdev: implement the northbound APIs
From: Thomas Monjalon @ 2016-11-23 19:18 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, bruce.richardson, harry.van.haaren, hemant.agrawal,
gage.eads
In-Reply-To: <1479447902-3700-3-git-send-email-jerin.jacob@caviumnetworks.com>
2016-11-18 11:15, Jerin Jacob:
> This patch set defines the southbound driver interface
> and implements the common code required for northbound
> eventdev API interface.
Please make two separate patches.
> +#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> +#define RTE_PMD_DEBUG_TRACE(...) \
> + rte_pmd_debug_trace(__func__, __VA_ARGS__)
> +#else
> +#define RTE_PMD_DEBUG_TRACE(...)
> +#endif
I would like to discuss the need for a debug option as there is
already a log level.
> +/* Logging Macros */
> +#define EDEV_LOG_ERR(fmt, args...) \
Every symbols and macros in an exported header must be prefixed by RTE_.
> +/* Macros to check for valid device */
> +#define RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, retval) do { \
Sometimes you use RTE_EVENT_DEV_ and sometimes RTE_EVENTDEV.
(I prefer the latter).
> +struct rte_eventdev_driver {
> + struct rte_pci_driver pci_drv; /**< The PMD is also a PCI driver. */
It must not be directly linked to the underlying bus.
^ permalink raw reply
* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Thomas Monjalon @ 2016-11-23 18:39 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, bruce.richardson, harry.van.haaren, hemant.agrawal,
gage.eads
In-Reply-To: <1479447902-3700-2-git-send-email-jerin.jacob@caviumnetworks.com>
Hi Jerin,
Thanks for bringing a big new piece in DPDK.
I made some comments below.
2016-11-18 11:14, Jerin Jacob:
> +Eventdev API - EXPERIMENTAL
> +M: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> +F: lib/librte_eventdev/
OK to mark it experimental.
What is the plan to remove the experimental word?
> + * RTE event device drivers do not use interrupts for enqueue or dequeue
> + * operation. Instead, Event drivers export Poll-Mode enqueue and dequeue
> + * functions to applications.
To the question "what makes DPDK different" it could be answered
that DPDK event drivers implement polling functions :)
> +#include <stdbool.h>
> +
> +#include <rte_pci.h>
> +#include <rte_dev.h>
> +#include <rte_memory.h>
Is it possible to remove some of these includes from the API?
> +
> +#define EVENTDEV_NAME_SKELETON_PMD event_skeleton
> +/**< Skeleton event device PMD name */
I do not understand this #define.
And it is not properly prefixed.
> +struct rte_event_dev_info {
> + const char *driver_name; /**< Event driver name */
> + struct rte_pci_device *pci_dev; /**< PCI information */
There is some work in progress to remove PCI information from ethdev.
Please do not add any PCI related structure in eventdev.
The generic structure is rte_device.
> +struct rte_event_dev_config {
> + uint32_t dequeue_wait_ns;
> + /**< rte_event_dequeue() wait for *dequeue_wait_ns* ns on this device.
Please explain exactly when the wait occurs and why.
> + * This value should be in the range of *min_dequeue_wait_ns* and
> + * *max_dequeue_wait_ns* which previously provided in
> + * rte_event_dev_info_get()
> + * \see RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT
I think the @see syntax would be more consistent than \see.
> + uint8_t nb_event_port_dequeue_depth;
> + /**< Number of dequeue queue depth for any event port on this device.
I think it deserves more explanations.
> + uint32_t event_dev_cfg;
> + /**< Event device config flags(RTE_EVENT_DEV_CFG_)*/
How this field differs from others in the struct?
Should it be named flags?
> + uint32_t event_queue_cfg; /**< Queue config flags(EVENT_QUEUE_CFG_) */
Same comment about the naming of this field for event_queue config sruct.
> +/** Event port configuration structure */
> +struct rte_event_port_conf {
> + int32_t new_event_threshold;
> + /**< A backpressure threshold for new event enqueues on this port.
> + * Use for *closed system* event dev where event capacity is limited,
> + * and cannot exceed the capacity of the event dev.
> + * Configuring ports with different thresholds can make higher priority
> + * traffic less likely to be backpressured.
> + * For example, a port used to inject NIC Rx packets into the event dev
> + * can have a lower threshold so as not to overwhelm the device,
> + * while ports used for worker pools can have a higher threshold.
> + * This value cannot exceed the *nb_events_limit*
> + * which previously supplied to rte_event_dev_configure()
> + */
> + uint8_t dequeue_depth;
> + /**< Configure number of bulk dequeues for this event port.
> + * This value cannot exceed the *nb_event_port_dequeue_depth*
> + * which previously supplied to rte_event_dev_configure()
> + */
> + uint8_t enqueue_depth;
> + /**< Configure number of bulk enqueues for this event port.
> + * This value cannot exceed the *nb_event_port_enqueue_depth*
> + * which previously supplied to rte_event_dev_configure()
> + */
> +};
The depth configuration is not clear to me.
> +/* Event types to classify the event source */
Why this classification is needed?
> +#define RTE_EVENT_TYPE_ETHDEV 0x0
> +/**< The event generated from ethdev subsystem */
> +#define RTE_EVENT_TYPE_CRYPTODEV 0x1
> +/**< The event generated from crypodev subsystem */
> +#define RTE_EVENT_TYPE_TIMERDEV 0x2
> +/**< The event generated from timerdev subsystem */
> +#define RTE_EVENT_TYPE_CORE 0x3
> +/**< The event generated from core.
What is core?
> +/* Event enqueue operations */
I feel a longer explanation is needed here to describe
what is an operation and where this data is useful.
> +#define RTE_EVENT_OP_NEW 0
> +/**< New event without previous context */
> +#define RTE_EVENT_OP_FORWARD 1
> +/**< Re-enqueue previously dequeued event */
> +#define RTE_EVENT_OP_RELEASE 2
There is no comment for the release operation.
> +/**
> + * Release the flow context associated with the schedule type.
> + *
[...]
> + */
There is no function declaration below this comment.
> +/**
> + * The generic *rte_event* structure to hold the event attributes
> + * for dequeue and enqueue operation
> + */
> +struct rte_event {
> + /** WORD0 */
> + RTE_STD_C11
> + union {
> + uint64_t event;
[...]
> + };
> + /** WORD1 */
> + RTE_STD_C11
> + union {
> + uintptr_t event_ptr;
I wonder if it can be a problem to have the size of this field
not constant across machines.
> + /**< Opaque event pointer */
> + struct rte_mbuf *mbuf;
> + /**< mbuf pointer if dequeued event is associated with mbuf */
How do we know that an event is associated with mbuf?
Does it mean that such events are always converted into mbuf even if the
application does not need it?
> +struct rte_eventdev_driver;
> +struct rte_eventdev_ops;
I think it is better to split API and driver interface in two files.
(we should do this split in ethdev)
> +/**
> + * Enqueue the event object supplied in the *rte_event* structure on an
> + * event device designated by its *dev_id* through the event port specified by
> + * *port_id*. The event object specifies the event queue on which this
> + * event will be enqueued.
> + *
> + * @param dev_id
> + * Event device identifier.
> + * @param port_id
> + * The identifier of the event port.
> + * @param ev
> + * Pointer to struct rte_event
> + *
> + * @return
> + * - 0 on success
> + * - <0 on failure. Failure can occur if the event port's output queue is
> + * backpressured, for instance.
> + */
> +static inline int
> +rte_event_enqueue(uint8_t dev_id, uint8_t port_id, struct rte_event *ev)
Is it really needed to have non-burst variant of enqueue/dequeue?
> +/**
> + * Converts nanoseconds to *wait* value for rte_event_dequeue()
> + *
> + * If the device is configured with RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT flag then
> + * application can use this function to convert wait value in nanoseconds to
> + * implementations specific wait value supplied in rte_event_dequeue()
Why is it implementation-specific?
Why this conversion is not internal in the driver?
End of review for this patch ;)
^ permalink raw reply
* [PATCH v12 6/6] testpmd: use Tx preparation in csum engine
From: Tomasz Kulasek @ 2016-11-23 17:36 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-1-git-send-email-tomaszx.kulasek@intel.com>
Added "csum txprep (on|off)" command which allows to switch to the
tx path using Tx preparation API.
By default unchanged implementation is used.
Using Tx preparation path, pseudo header calculation for udp/tcp/tso
packets from application, and used Tx preparation API for
packet preparation and verification.
Adding additional step to the csum engine costs about 3-4% of performance
drop, on my setup with ixgbe driver. It's caused mostly by the need
of reaccessing and modification of packet data.
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
app/test-pmd/cmdline.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
app/test-pmd/csumonly.c | 33 ++++++++++++++++++++++++-------
app/test-pmd/testpmd.c | 5 +++++
app/test-pmd/testpmd.h | 2 ++
4 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 63b55dc..373fc59 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -366,6 +366,10 @@ static void cmd_help_long_parsed(void *parsed_result,
"csum show (port_id)\n"
" Display tx checksum offload configuration\n\n"
+ "csum txprep (on|off)"
+ " Enable tx preparation path in csum forward engine"
+ "\n\n"
+
"tso set (segsize) (portid)\n"
" Enable TCP Segmentation Offload in csum forward"
" engine.\n"
@@ -3523,6 +3527,50 @@ struct cmd_csum_tunnel_result {
},
};
+/* Enable/disable tx preparation path */
+struct cmd_csum_txprep_result {
+ cmdline_fixed_string_t csum;
+ cmdline_fixed_string_t parse;
+ cmdline_fixed_string_t onoff;
+};
+
+static void
+cmd_csum_txprep_parsed(void *parsed_result,
+ __attribute__((unused)) struct cmdline *cl,
+ __attribute__((unused)) void *data)
+{
+ struct cmd_csum_txprep_result *res = parsed_result;
+
+ if (!strcmp(res->onoff, "on"))
+ tx_prepare = 1;
+ else
+ tx_prepare = 0;
+
+}
+
+cmdline_parse_token_string_t cmd_csum_txprep_csum =
+ TOKEN_STRING_INITIALIZER(struct cmd_csum_txprep_result,
+ csum, "csum");
+cmdline_parse_token_string_t cmd_csum_txprep_parse =
+ TOKEN_STRING_INITIALIZER(struct cmd_csum_txprep_result,
+ parse, "txprep");
+cmdline_parse_token_string_t cmd_csum_txprep_onoff =
+ TOKEN_STRING_INITIALIZER(struct cmd_csum_txprep_result,
+ onoff, "on#off");
+
+cmdline_parse_inst_t cmd_csum_txprep = {
+ .f = cmd_csum_txprep_parsed,
+ .data = NULL,
+ .help_str = "enable/disable tx preparation path for csum engine: "
+ "csum txprep on|off",
+ .tokens = {
+ (void *)&cmd_csum_txprep_csum,
+ (void *)&cmd_csum_txprep_parse,
+ (void *)&cmd_csum_txprep_onoff,
+ NULL,
+ },
+};
+
/* *** ENABLE HARDWARE SEGMENTATION IN TX NON-TUNNELED PACKETS *** */
struct cmd_tso_set_result {
cmdline_fixed_string_t tso;
@@ -11470,6 +11518,7 @@ struct cmd_set_vf_mac_addr_result {
(cmdline_parse_inst_t *)&cmd_csum_set,
(cmdline_parse_inst_t *)&cmd_csum_show,
(cmdline_parse_inst_t *)&cmd_csum_tunnel,
+ (cmdline_parse_inst_t *)&cmd_csum_txprep,
(cmdline_parse_inst_t *)&cmd_tso_set,
(cmdline_parse_inst_t *)&cmd_tso_show,
(cmdline_parse_inst_t *)&cmd_tunnel_tso_set,
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 57e6ae2..3afa9ab 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -372,8 +372,10 @@ struct simple_gre_hdr {
udp_hdr->dgram_cksum = 0;
if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_UDP_CKSUM) {
ol_flags |= PKT_TX_UDP_CKSUM;
- udp_hdr->dgram_cksum = get_psd_sum(l3_hdr,
- info->ethertype, ol_flags);
+ if (!tx_prepare)
+ udp_hdr->dgram_cksum = get_psd_sum(
+ l3_hdr, info->ethertype,
+ ol_flags);
} else {
udp_hdr->dgram_cksum =
get_udptcp_checksum(l3_hdr, udp_hdr,
@@ -385,12 +387,15 @@ struct simple_gre_hdr {
tcp_hdr->cksum = 0;
if (tso_segsz) {
ol_flags |= PKT_TX_TCP_SEG;
- tcp_hdr->cksum = get_psd_sum(l3_hdr, info->ethertype,
- ol_flags);
+ if (!tx_prepare)
+ tcp_hdr->cksum = get_psd_sum(l3_hdr,
+ info->ethertype, ol_flags);
+
} else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TCP_CKSUM) {
ol_flags |= PKT_TX_TCP_CKSUM;
- tcp_hdr->cksum = get_psd_sum(l3_hdr, info->ethertype,
- ol_flags);
+ if (!tx_prepare)
+ tcp_hdr->cksum = get_psd_sum(l3_hdr,
+ info->ethertype, ol_flags);
} else {
tcp_hdr->cksum =
get_udptcp_checksum(l3_hdr, tcp_hdr,
@@ -648,6 +653,7 @@ struct simple_gre_hdr {
void *l3_hdr = NULL, *outer_l3_hdr = NULL; /* can be IPv4 or IPv6 */
uint16_t nb_rx;
uint16_t nb_tx;
+ uint16_t nb_prep;
uint16_t i;
uint64_t rx_ol_flags, tx_ol_flags;
uint16_t testpmd_ol_flags;
@@ -857,7 +863,20 @@ struct simple_gre_hdr {
printf("\n");
}
}
- nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst, nb_rx);
+
+ if (tx_prepare) {
+ nb_prep = rte_eth_tx_prepare(fs->tx_port, fs->tx_queue,
+ pkts_burst, nb_rx);
+ if (nb_prep != nb_rx)
+ printf("Preparing packet burst to transmit failed: %s\n",
+ rte_strerror(rte_errno));
+
+ nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst,
+ nb_prep);
+ } else
+ nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst,
+ nb_rx);
+
/*
* Retry if necessary
*/
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index a0332c2..c18bc28 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -180,6 +180,11 @@ struct fwd_engine * fwd_engines[] = {
enum tx_pkt_split tx_pkt_split = TX_PKT_SPLIT_OFF;
/**< Split policy for packets to TX. */
+/*
+ * Enable Tx preparation path in the "csum" engine.
+ */
+uint8_t tx_prepare = 0;
+
uint16_t nb_pkt_per_burst = DEF_PKT_BURST; /**< Number of packets per burst. */
uint16_t mb_mempool_cache = DEF_MBUF_CACHE; /**< Size of mbuf mempool cache. */
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 9c1e703..488a6e1 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -383,6 +383,8 @@ enum tx_pkt_split {
extern enum tx_pkt_split tx_pkt_split;
+extern uint8_t tx_prepare;
+
extern uint16_t nb_pkt_per_burst;
extern uint16_t mb_mempool_cache;
extern int8_t rx_pthresh;
--
1.7.9.5
^ permalink raw reply related
* [PATCH v12 5/6] ixgbe: add Tx preparation
From: Tomasz Kulasek @ 2016-11-23 17:36 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-1-git-send-email-tomaszx.kulasek@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 3 ++
drivers/net/ixgbe/ixgbe_ethdev.h | 5 +++-
drivers/net/ixgbe/ixgbe_rxtx.c | 56 ++++++++++++++++++++++++++++++++++++++
drivers/net/ixgbe/ixgbe_rxtx.h | 2 ++
4 files changed, 65 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index edc9b22..a75f59d 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -517,6 +517,8 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
.nb_max = IXGBE_MAX_RING_DESC,
.nb_min = IXGBE_MIN_RING_DESC,
.nb_align = IXGBE_TXD_ALIGN,
+ .nb_seg_max = IXGBE_TX_MAX_SEG,
+ .nb_mtu_seg_max = IXGBE_TX_MAX_SEG,
};
static const struct eth_dev_ops ixgbe_eth_dev_ops = {
@@ -1103,6 +1105,7 @@ struct rte_ixgbe_xstats_name_off {
eth_dev->dev_ops = &ixgbe_eth_dev_ops;
eth_dev->rx_pkt_burst = &ixgbe_recv_pkts;
eth_dev->tx_pkt_burst = &ixgbe_xmit_pkts;
+ eth_dev->tx_pkt_prepare = &ixgbe_prep_pkts;
/*
* For secondary processes, we don't initialise any further as primary
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 4ff6338..e229cf5 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ * Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -396,6 +396,9 @@ uint16_t ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t ixgbe_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
+uint16_t ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
+
int ixgbe_dev_rss_hash_update(struct rte_eth_dev *dev,
struct rte_eth_rss_conf *rss_conf);
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index b2d9f45..dbe83e7 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -70,6 +70,7 @@
#include <rte_string_fns.h>
#include <rte_errno.h>
#include <rte_ip.h>
+#include <rte_net.h>
#include "ixgbe_logs.h"
#include "base/ixgbe_api.h"
@@ -87,6 +88,9 @@
PKT_TX_TCP_SEG | \
PKT_TX_OUTER_IP_CKSUM)
+#define IXGBE_TX_OFFLOAD_NOTSUP_MASK \
+ (PKT_TX_OFFLOAD_MASK ^ IXGBE_TX_OFFLOAD_MASK)
+
#if 1
#define RTE_PMD_USE_PREFETCH
#endif
@@ -905,6 +909,56 @@ static inline int __attribute__((always_inline))
/*********************************************************************
*
+ * TX prep functions
+ *
+ **********************************************************************/
+uint16_t
+ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
+{
+ int i, ret;
+ uint64_t ol_flags;
+ struct rte_mbuf *m;
+ struct ixgbe_tx_queue *txq = (struct ixgbe_tx_queue *)tx_queue;
+
+ for (i = 0; i < nb_pkts; i++) {
+ m = tx_pkts[i];
+ ol_flags = m->ol_flags;
+
+ /**
+ * Check if packet meets requirements for number of segments
+ *
+ * NOTE: for ixgbe it's always (40 - WTHRESH) for both TSO and non-TSO
+ */
+
+ if (m->nb_segs > IXGBE_TX_MAX_SEG - txq->wthresh) {
+ rte_errno = -EINVAL;
+ return i;
+ }
+
+ if (ol_flags & IXGBE_TX_OFFLOAD_NOTSUP_MASK) {
+ rte_errno = -ENOTSUP;
+ return i;
+ }
+
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+ ret = rte_validate_tx_offload(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+#endif
+ ret = rte_net_intel_cksum_prepare(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+ }
+
+ return i;
+}
+
+/*********************************************************************
+ *
* RX functions
*
**********************************************************************/
@@ -2282,6 +2336,7 @@ void __attribute__((cold))
if (((txq->txq_flags & IXGBE_SIMPLE_FLAGS) == IXGBE_SIMPLE_FLAGS)
&& (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)) {
PMD_INIT_LOG(DEBUG, "Using simple tx code path");
+ dev->tx_pkt_prepare = NULL;
#ifdef RTE_IXGBE_INC_VECTOR
if (txq->tx_rs_thresh <= RTE_IXGBE_TX_MAX_FREE_BUF_SZ &&
(rte_eal_process_type() != RTE_PROC_PRIMARY ||
@@ -2302,6 +2357,7 @@ void __attribute__((cold))
(unsigned long)txq->tx_rs_thresh,
(unsigned long)RTE_PMD_IXGBE_TX_MAX_BURST);
dev->tx_pkt_burst = ixgbe_xmit_pkts;
+ dev->tx_pkt_prepare = ixgbe_prep_pkts;
}
}
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
index 2608b36..7bbd9b8 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx.h
@@ -80,6 +80,8 @@
#define RTE_IXGBE_WAIT_100_US 100
#define RTE_IXGBE_VMTXSW_REGISTER_COUNT 2
+#define IXGBE_TX_MAX_SEG 40
+
#define IXGBE_PACKET_TYPE_MASK_82599 0X7F
#define IXGBE_PACKET_TYPE_MASK_X550 0X10FF
#define IXGBE_PACKET_TYPE_MASK_TUNNEL 0XFF
--
1.7.9.5
^ permalink raw reply related
* [PATCH v12 4/6] i40e: add Tx preparation
From: Tomasz Kulasek @ 2016-11-23 17:36 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-1-git-send-email-tomaszx.kulasek@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
drivers/net/i40e/i40e_ethdev.c | 3 ++
drivers/net/i40e/i40e_rxtx.c | 72 +++++++++++++++++++++++++++++++++++++++-
drivers/net/i40e/i40e_rxtx.h | 8 +++++
3 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 67778ba..5761357 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -943,6 +943,7 @@ static inline void i40e_GLQF_reg_init(struct i40e_hw *hw)
dev->dev_ops = &i40e_eth_dev_ops;
dev->rx_pkt_burst = i40e_recv_pkts;
dev->tx_pkt_burst = i40e_xmit_pkts;
+ dev->tx_pkt_prepare = i40e_prep_pkts;
/* for secondary processes, we don't initialise any further as primary
* has already done this work. Only check we don't need a different
@@ -2645,6 +2646,8 @@ static int i40e_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
.nb_max = I40E_MAX_RING_DESC,
.nb_min = I40E_MIN_RING_DESC,
.nb_align = I40E_ALIGN_RING_DESC,
+ .nb_seg_max = I40E_TX_MAX_SEG,
+ .nb_mtu_seg_max = I40E_TX_MAX_MTU_SEG,
};
if (pf->flags & I40E_FLAG_VMDQ) {
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 7ae7d9f..5827f2f 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ * Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -50,6 +50,8 @@
#include <rte_tcp.h>
#include <rte_sctp.h>
#include <rte_udp.h>
+#include <rte_ip.h>
+#include <rte_net.h>
#include "i40e_logs.h"
#include "base/i40e_prototype.h"
@@ -79,6 +81,17 @@
PKT_TX_TCP_SEG | \
PKT_TX_OUTER_IP_CKSUM)
+#define I40E_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)
+
+#define I40E_TX_OFFLOAD_NOTSUP_MASK \
+ (PKT_TX_OFFLOAD_MASK ^ I40E_TX_OFFLOAD_MASK)
+
static uint16_t i40e_xmit_pkts_simple(void *tx_queue,
struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
@@ -1411,6 +1424,61 @@ static inline int __attribute__((always_inline))
return nb_tx;
}
+/*********************************************************************
+ *
+ * TX prep functions
+ *
+ **********************************************************************/
+uint16_t
+i40e_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts)
+{
+ int i, ret;
+ uint64_t ol_flags;
+ struct rte_mbuf *m;
+
+ for (i = 0; i < nb_pkts; i++) {
+ m = tx_pkts[i];
+ ol_flags = m->ol_flags;
+
+ /**
+ * m->nb_segs is uint8_t, so m->nb_segs is always less than
+ * I40E_TX_MAX_SEG.
+ * We check only a condition for m->nb_segs > I40E_TX_MAX_MTU_SEG.
+ */
+ if (!(ol_flags & PKT_TX_TCP_SEG)) {
+ if (m->nb_segs > I40E_TX_MAX_MTU_SEG) {
+ rte_errno = -EINVAL;
+ return i;
+ }
+ } else if ((m->tso_segsz < I40E_MIN_TSO_MSS) ||
+ (m->tso_segsz > I40E_MAX_TSO_MSS)) {
+ /* MSS outside the range (256B - 9674B) are considered malicious */
+ rte_errno = -EINVAL;
+ return i;
+ }
+
+ if (ol_flags & I40E_TX_OFFLOAD_NOTSUP_MASK) {
+ rte_errno = -ENOTSUP;
+ return i;
+ }
+
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+ ret = rte_validate_tx_offload(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+#endif
+ ret = rte_net_intel_cksum_prepare(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+ }
+ return i;
+}
+
/*
* Find the VSI the queue belongs to. 'queue_idx' is the queue index
* application used, which assume having sequential ones. But from driver's
@@ -2763,9 +2831,11 @@ void __attribute__((cold))
PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
dev->tx_pkt_burst = i40e_xmit_pkts_simple;
}
+ dev->tx_pkt_prepare = NULL;
} else {
PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
dev->tx_pkt_burst = i40e_xmit_pkts;
+ dev->tx_pkt_prepare = i40e_prep_pkts;
}
}
diff --git a/drivers/net/i40e/i40e_rxtx.h b/drivers/net/i40e/i40e_rxtx.h
index ecdb13c..9df8a56 100644
--- a/drivers/net/i40e/i40e_rxtx.h
+++ b/drivers/net/i40e/i40e_rxtx.h
@@ -63,6 +63,12 @@
#define I40E_MIN_RING_DESC 64
#define I40E_MAX_RING_DESC 4096
+#define I40E_MIN_TSO_MSS 256
+#define I40E_MAX_TSO_MSS 9674
+
+#define I40E_TX_MAX_SEG UINT8_MAX
+#define I40E_TX_MAX_MTU_SEG 8
+
#undef container_of
#define container_of(ptr, type, member) ({ \
typeof(((type *)0)->member)(*__mptr) = (ptr); \
@@ -223,6 +229,8 @@ uint16_t i40e_recv_scattered_pkts(void *rx_queue,
uint16_t i40e_xmit_pkts(void *tx_queue,
struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
+uint16_t i40e_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
int i40e_tx_queue_init(struct i40e_tx_queue *txq);
int i40e_rx_queue_init(struct i40e_rx_queue *rxq);
void i40e_free_tx_resources(struct i40e_tx_queue *txq);
--
1.7.9.5
^ permalink raw reply related
* [PATCH v12 3/6] fm10k: add Tx preparation
From: Tomasz Kulasek @ 2016-11-23 17:36 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-1-git-send-email-tomaszx.kulasek@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
drivers/net/fm10k/fm10k.h | 6 +++++
drivers/net/fm10k/fm10k_ethdev.c | 5 ++++
drivers/net/fm10k/fm10k_rxtx.c | 50 +++++++++++++++++++++++++++++++++++++-
3 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/drivers/net/fm10k/fm10k.h b/drivers/net/fm10k/fm10k.h
index 05aa1a2..c6fed21 100644
--- a/drivers/net/fm10k/fm10k.h
+++ b/drivers/net/fm10k/fm10k.h
@@ -69,6 +69,9 @@
#define FM10K_MAX_RX_DESC (FM10K_MAX_RX_RING_SZ / sizeof(union fm10k_rx_desc))
#define FM10K_MAX_TX_DESC (FM10K_MAX_TX_RING_SZ / sizeof(struct fm10k_tx_desc))
+#define FM10K_TX_MAX_SEG UINT8_MAX
+#define FM10K_TX_MAX_MTU_SEG UINT8_MAX
+
/*
* byte aligment for HW RX data buffer
* Datasheet requires RX buffer addresses shall either be 512-byte aligned or
@@ -356,6 +359,9 @@ uint16_t fm10k_recv_scattered_pkts(void *rx_queue,
uint16_t fm10k_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
+uint16_t fm10k_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
+
int fm10k_rxq_vec_setup(struct fm10k_rx_queue *rxq);
int fm10k_rx_vec_condition_check(struct rte_eth_dev *);
void fm10k_rx_queue_release_mbufs_vec(struct fm10k_rx_queue *rxq);
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index 923690c..a116822 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -1447,6 +1447,8 @@ static int fm10k_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
.nb_max = FM10K_MAX_TX_DESC,
.nb_min = FM10K_MIN_TX_DESC,
.nb_align = FM10K_MULT_TX_DESC,
+ .nb_seg_max = FM10K_TX_MAX_SEG,
+ .nb_mtu_seg_max = FM10K_TX_MAX_MTU_SEG,
};
dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_2_5G |
@@ -2755,8 +2757,10 @@ static void __attribute__((cold))
fm10k_txq_vec_setup(txq);
}
dev->tx_pkt_burst = fm10k_xmit_pkts_vec;
+ dev->tx_pkt_prepare = NULL;
} else {
dev->tx_pkt_burst = fm10k_xmit_pkts;
+ dev->tx_pkt_prepare = fm10k_prep_pkts;
PMD_INIT_LOG(DEBUG, "Use regular Tx func");
}
}
@@ -2835,6 +2839,7 @@ static void __attribute__((cold))
dev->dev_ops = &fm10k_eth_dev_ops;
dev->rx_pkt_burst = &fm10k_recv_pkts;
dev->tx_pkt_burst = &fm10k_xmit_pkts;
+ dev->tx_pkt_prepare = &fm10k_prep_pkts;
/* only initialize in the primary process */
if (rte_eal_process_type() != RTE_PROC_PRIMARY)
diff --git a/drivers/net/fm10k/fm10k_rxtx.c b/drivers/net/fm10k/fm10k_rxtx.c
index 32cc7ff..144e5e6 100644
--- a/drivers/net/fm10k/fm10k_rxtx.c
+++ b/drivers/net/fm10k/fm10k_rxtx.c
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2013-2015 Intel Corporation. All rights reserved.
+ * Copyright(c) 2013-2016 Intel Corporation. All rights reserved.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -35,6 +35,7 @@
#include <rte_ethdev.h>
#include <rte_common.h>
+#include <rte_net.h>
#include "fm10k.h"
#include "base/fm10k_type.h"
@@ -65,6 +66,15 @@ static inline void dump_rxd(union fm10k_rx_desc *rxd)
}
#endif
+#define FM10K_TX_OFFLOAD_MASK ( \
+ PKT_TX_VLAN_PKT | \
+ PKT_TX_IP_CKSUM | \
+ PKT_TX_L4_MASK | \
+ PKT_TX_TCP_SEG)
+
+#define FM10K_TX_OFFLOAD_NOTSUP_MASK \
+ (PKT_TX_OFFLOAD_MASK ^ FM10K_TX_OFFLOAD_MASK)
+
/* @note: When this function is changed, make corresponding change to
* fm10k_dev_supported_ptypes_get()
*/
@@ -597,3 +607,41 @@ static inline void tx_xmit_pkt(struct fm10k_tx_queue *q, struct rte_mbuf *mb)
return count;
}
+
+uint16_t
+fm10k_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts)
+{
+ int i, ret;
+ struct rte_mbuf *m;
+
+ for (i = 0; i < nb_pkts; i++) {
+ m = tx_pkts[i];
+
+ if ((m->ol_flags & PKT_TX_TCP_SEG) &&
+ (m->tso_segsz < FM10K_TSO_MINMSS)) {
+ rte_errno = -EINVAL;
+ return i;
+ }
+
+ if (m->ol_flags & FM10K_TX_OFFLOAD_NOTSUP_MASK) {
+ rte_errno = -ENOTSUP;
+ return i;
+ }
+
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+ ret = rte_validate_tx_offload(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+#endif
+ ret = rte_net_intel_cksum_prepare(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+ }
+
+ return i;
+}
--
1.7.9.5
^ permalink raw reply related
* [PATCH v12 2/6] e1000: add Tx preparation
From: Tomasz Kulasek @ 2016-11-23 17:36 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-1-git-send-email-tomaszx.kulasek@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
drivers/net/e1000/e1000_ethdev.h | 11 ++++++++
drivers/net/e1000/em_ethdev.c | 5 +++-
drivers/net/e1000/em_rxtx.c | 48 ++++++++++++++++++++++++++++++++++-
drivers/net/e1000/igb_ethdev.c | 4 +++
drivers/net/e1000/igb_rxtx.c | 52 +++++++++++++++++++++++++++++++++++++-
5 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 6c25c8d..bd0f277 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -138,6 +138,11 @@
#define E1000_MISC_VEC_ID RTE_INTR_VEC_ZERO_OFFSET
#define E1000_RX_VEC_START RTE_INTR_VEC_RXTX_OFFSET
+#define IGB_TX_MAX_SEG UINT8_MAX
+#define IGB_TX_MAX_MTU_SEG UINT8_MAX
+#define EM_TX_MAX_SEG UINT8_MAX
+#define EM_TX_MAX_MTU_SEG UINT8_MAX
+
/* structure for interrupt relative data */
struct e1000_interrupt {
uint32_t flags;
@@ -315,6 +320,9 @@ int eth_igb_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
uint16_t eth_igb_xmit_pkts(void *txq, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
+uint16_t eth_igb_prep_pkts(void *txq, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
+
uint16_t eth_igb_recv_pkts(void *rxq, struct rte_mbuf **rx_pkts,
uint16_t nb_pkts);
@@ -376,6 +384,9 @@ int eth_em_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
uint16_t eth_em_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
+uint16_t eth_em_prep_pkts(void *txq, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
+
uint16_t eth_em_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
uint16_t nb_pkts);
diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index aee3d34..a004ee9 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ * Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -300,6 +300,7 @@ static int eth_em_set_mc_addr_list(struct rte_eth_dev *dev,
eth_dev->dev_ops = ð_em_ops;
eth_dev->rx_pkt_burst = (eth_rx_burst_t)ð_em_recv_pkts;
eth_dev->tx_pkt_burst = (eth_tx_burst_t)ð_em_xmit_pkts;
+ eth_dev->tx_pkt_prepare = (eth_tx_prep_t)ð_em_prep_pkts;
/* for secondary processes, we don't initialise any further as primary
* has already done this work. Only check we don't need a different
@@ -1079,6 +1080,8 @@ static int eth_em_set_mc_addr_list(struct rte_eth_dev *dev,
.nb_max = E1000_MAX_RING_DESC,
.nb_min = E1000_MIN_RING_DESC,
.nb_align = EM_TXD_ALIGN,
+ .nb_seg_max = EM_TX_MAX_SEG,
+ .nb_mtu_seg_max = EM_TX_MAX_MTU_SEG,
};
dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
index 41f51c0..7e271ad 100644
--- a/drivers/net/e1000/em_rxtx.c
+++ b/drivers/net/e1000/em_rxtx.c
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ * Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -66,6 +66,7 @@
#include <rte_udp.h>
#include <rte_tcp.h>
#include <rte_sctp.h>
+#include <rte_net.h>
#include <rte_string_fns.h>
#include "e1000_logs.h"
@@ -77,6 +78,14 @@
#define E1000_RXDCTL_GRAN 0x01000000 /* RXDCTL Granularity */
+#define E1000_TX_OFFLOAD_MASK ( \
+ PKT_TX_IP_CKSUM | \
+ PKT_TX_L4_MASK | \
+ PKT_TX_VLAN_PKT)
+
+#define E1000_TX_OFFLOAD_NOTSUP_MASK \
+ (PKT_TX_OFFLOAD_MASK ^ E1000_TX_OFFLOAD_MASK)
+
/**
* Structure associated with each descriptor of the RX ring of a RX queue.
*/
@@ -618,6 +627,43 @@ struct em_tx_queue {
/*********************************************************************
*
+ * TX prep functions
+ *
+ **********************************************************************/
+uint16_t
+eth_em_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts)
+{
+ int i, ret;
+ struct rte_mbuf *m;
+
+ for (i = 0; i < nb_pkts; i++) {
+ m = tx_pkts[i];
+
+ if (m->ol_flags & E1000_TX_OFFLOAD_NOTSUP_MASK) {
+ rte_errno = -ENOTSUP;
+ return i;
+ }
+
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+ ret = rte_validate_tx_offload(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+#endif
+ ret = rte_net_intel_cksum_prepare(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+ }
+
+ return i;
+}
+
+/*********************************************************************
+ *
* RX functions
*
**********************************************************************/
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 2fddf0c..015ef46 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -369,6 +369,8 @@ static void eth_igbvf_interrupt_handler(struct rte_intr_handle *handle,
.nb_max = E1000_MAX_RING_DESC,
.nb_min = E1000_MIN_RING_DESC,
.nb_align = IGB_RXD_ALIGN,
+ .nb_seg_max = IGB_TX_MAX_SEG,
+ .nb_mtu_seg_max = IGB_TX_MAX_MTU_SEG,
};
static const struct eth_dev_ops eth_igb_ops = {
@@ -760,6 +762,7 @@ struct rte_igb_xstats_name_off {
eth_dev->dev_ops = ð_igb_ops;
eth_dev->rx_pkt_burst = ð_igb_recv_pkts;
eth_dev->tx_pkt_burst = ð_igb_xmit_pkts;
+ eth_dev->tx_pkt_prepare = ð_igb_prep_pkts;
/* for secondary processes, we don't initialise any further as primary
* has already done this work. Only check we don't need a different
@@ -963,6 +966,7 @@ struct rte_igb_xstats_name_off {
eth_dev->dev_ops = &igbvf_eth_dev_ops;
eth_dev->rx_pkt_burst = ð_igb_recv_pkts;
eth_dev->tx_pkt_burst = ð_igb_xmit_pkts;
+ eth_dev->tx_pkt_prepare = ð_igb_prep_pkts;
/* for secondary processes, we don't initialise any further as primary
* has already done this work. Only check we don't need a different
diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index dbd37ac..8a3a3db 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ * Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -65,6 +65,7 @@
#include <rte_udp.h>
#include <rte_tcp.h>
#include <rte_sctp.h>
+#include <rte_net.h>
#include <rte_string_fns.h>
#include "e1000_logs.h"
@@ -78,6 +79,9 @@
PKT_TX_L4_MASK | \
PKT_TX_TCP_SEG)
+#define IGB_TX_OFFLOAD_NOTSUP_MASK \
+ (PKT_TX_OFFLOAD_MASK ^ IGB_TX_OFFLOAD_MASK)
+
/**
* Structure associated with each descriptor of the RX ring of a RX queue.
*/
@@ -616,6 +620,51 @@ struct igb_tx_queue {
/*********************************************************************
*
+ * TX prep functions
+ *
+ **********************************************************************/
+uint16_t
+eth_igb_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts)
+{
+ int i, ret;
+ struct rte_mbuf *m;
+
+ for (i = 0; i < nb_pkts; i++) {
+ m = tx_pkts[i];
+
+ /* Check some limitations for TSO in hardware */
+ if (m->ol_flags & PKT_TX_TCP_SEG)
+ if ((m->tso_segsz > IGB_TSO_MAX_MSS) || (m->l2_len + m->l3_len +
+ m->l4_len > IGB_TSO_MAX_HDRLEN)) {
+ rte_errno = -EINVAL;
+ return i;
+ }
+
+ if (m->ol_flags & IGB_TX_OFFLOAD_NOTSUP_MASK) {
+ rte_errno = -ENOTSUP;
+ return i;
+ }
+
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+ ret = rte_validate_tx_offload(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+#endif
+ ret = rte_net_intel_cksum_prepare(m);
+ if (ret != 0) {
+ rte_errno = ret;
+ return i;
+ }
+ }
+
+ return i;
+}
+
+/*********************************************************************
+ *
* RX functions
*
**********************************************************************/
@@ -1364,6 +1413,7 @@ struct igb_tx_queue {
igb_reset_tx_queue(txq, dev);
dev->tx_pkt_burst = eth_igb_xmit_pkts;
+ dev->tx_pkt_prepare = ð_igb_prep_pkts;
dev->data->tx_queues[queue_idx] = txq;
return 0;
--
1.7.9.5
^ permalink raw reply related
* [PATCH v12 1/6] ethdev: add Tx preparation
From: Tomasz Kulasek @ 2016-11-23 17:36 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-1-git-send-email-tomaszx.kulasek@intel.com>
Added API for `rte_eth_tx_prepare`
uint16_t rte_eth_tx_prepare(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 */
Added functions:
int rte_validate_tx_offload(struct rte_mbuf *m)
to validate general requirements for tx offload set in mbuf of packet
such a flag completness. In current implementation this function is
called optionaly when RTE_LIBRTE_ETHDEV_DEBUG is enabled.
int rte_net_intel_cksum_prepare(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.
PERFORMANCE TESTS
-----------------
This feature was tested with modified csum engine from test-pmd.
The packet checksum preparation was moved from application to Tx
preparation step placed before burst.
We may expect some overhead costs caused by:
1) using additional callback before burst,
2) rescanning burst,
3) additional condition checking (packet validation),
4) worse optimization (e.g. packet data access, etc.)
We tested it using ixgbe Tx preparation implementation with some parts
disabled to have comparable information about the impact of different
parts of implementation.
IMPACT:
1) For unimplemented Tx preparation callback the performance impact is
negligible,
2) For packet condition check without checksum modifications (nb_segs,
available offloads, etc.) is 14626628/14252168 (~2.62% drop),
3) Full support in ixgbe driver (point 2 + packet checksum
initialization) is 14060924/13588094 (~3.48% drop)
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
config/common_base | 1 +
lib/librte_ether/rte_ethdev.h | 106 +++++++++++++++++++++++++++++++++++++++++
lib/librte_mbuf/rte_mbuf.h | 64 +++++++++++++++++++++++++
lib/librte_net/rte_net.h | 85 +++++++++++++++++++++++++++++++++
4 files changed, 256 insertions(+)
diff --git a/config/common_base b/config/common_base
index 4bff83a..d609a88 100644
--- a/config/common_base
+++ b/config/common_base
@@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
CONFIG_RTE_LIBRTE_IEEE1588=n
CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
+CONFIG_RTE_ETHDEV_TX_PREPARE=y
#
# Support NIC bypass logic
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9678179..4ffc1b3 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -182,6 +182,7 @@
#include <rte_pci.h>
#include <rte_dev.h>
#include <rte_devargs.h>
+#include <rte_errno.h>
#include "rte_ether.h"
#include "rte_eth_ctrl.h"
#include "rte_dev_info.h"
@@ -702,6 +703,8 @@ struct rte_eth_desc_lim {
uint16_t nb_max; /**< Max allowed number of descriptors. */
uint16_t nb_min; /**< Min allowed number of descriptors. */
uint16_t nb_align; /**< Number of descriptors should be aligned to. */
+ 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 */
};
/**
@@ -1191,6 +1194,11 @@ typedef uint16_t (*eth_tx_burst_t)(void *txq,
uint16_t nb_pkts);
/**< @internal Send output packets on a transmit queue of an Ethernet device. */
+typedef uint16_t (*eth_tx_prep_t)(void *txq,
+ struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
+/**< @internal Prepare output packets on a transmit queue of an Ethernet device. */
+
typedef int (*flow_ctrl_get_t)(struct rte_eth_dev *dev,
struct rte_eth_fc_conf *fc_conf);
/**< @internal Get current flow control parameter on an Ethernet device */
@@ -1625,6 +1633,7 @@ struct rte_eth_rxtx_callback {
struct rte_eth_dev {
eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
+ eth_tx_prep_t tx_pkt_prepare; /**< Pointer to PMD transmit prepare function. */
struct rte_eth_dev_data *data; /**< Pointer to device data */
const struct eth_driver *driver;/**< Driver for this device */
const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
@@ -2819,6 +2828,103 @@ int rte_eth_dev_set_vlan_ether_type(uint8_t port_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_prepare() 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_prepare() function performs
+ * the following operations:
+ *
+ * - Check if packet meets devices requirements for tx offloads.
+ *
+ * - Check limitations about number of segments.
+ *
+ * - Check additional requirements when debug is enabled.
+ *
+ * - Update and/or reset required checksums when tx offload is set for packet.
+ *
+ * Since this function can modify packet data, provided mbufs must be safely
+ * writable (e.g. modified data cannot be in shared segment).
+ *
+ * The rte_eth_tx_prepare() 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, otherwise stops processing on the first invalid packet and
+ * leaves the rest packets untouched.
+ *
+ * When this functionality is not implemented in the driver, all packets are
+ * are returned untouched.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * The value must be a valid port id.
+ * @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:
+ * - -EINVAL: offload flags are not correctly set
+ * - -ENOTSUP: the offload feature is not supported by the hardware
+ *
+ */
+
+#ifdef RTE_ETHDEV_TX_PREPARE
+
+static inline uint16_t
+rte_eth_tx_prepare(uint8_t port_id, uint16_t queue_id,
+ struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
+{
+ struct rte_eth_dev *dev;
+
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+ if (!rte_eth_dev_is_valid_port(port_id)) {
+ RTE_PMD_DEBUG_TRACE("Invalid TX port_id=%d\n", port_id);
+ rte_errno = -EINVAL;
+ return 0;
+ }
+#endif
+
+ dev = &rte_eth_devices[port_id];
+
+#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
+
+ if (!dev->tx_pkt_prepare)
+ return nb_pkts;
+
+ return (*dev->tx_pkt_prepare)(dev->data->tx_queues[queue_id],
+ tx_pkts, nb_pkts);
+}
+
+#else
+
+static inline uint16_t
+rte_eth_tx_prepare(__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
+
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 ead7c6e..39ee5ed 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -283,6 +283,19 @@
*/
#define PKT_TX_OUTER_IPV6 (1ULL << 60)
+/**
+ * Bit Mask of all supported packet Tx offload features flags, which can be set
+ * for packet.
+ */
+#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)
+
#define __RESERVED (1ULL << 61) /**< reserved for future mbuf use */
#define IND_ATTACHED_MBUF (1ULL << 62) /**< Indirect attached mbuf */
@@ -1647,6 +1660,57 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
}
/**
+ * Validate general requirements for tx offload in mbuf.
+ *
+ * This function checks correctness and completeness of Tx offload settings.
+ *
+ * @param m
+ * The packet mbuf to be validated.
+ * @return
+ * 0 if packet is valid
+ */
+static inline int
+rte_validate_tx_offload(const struct rte_mbuf *m)
+{
+ uint64_t ol_flags = m->ol_flags;
+ uint64_t inner_l3_offset = m->l2_len;
+
+ /* Does packet set any of available offloads? */
+ if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
+ return 0;
+
+ if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
+ inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
+
+ /* Headers are fragmented */
+ if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
+ return -ENOTSUP;
+
+ /* IP checksum can be counted only for IPv4 packet */
+ if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
+ return -EINVAL;
+
+ /* IP type not set when required */
+ if (ol_flags & (PKT_TX_L4_MASK | PKT_TX_TCP_SEG))
+ if (!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6)))
+ return -EINVAL;
+
+ /* Check requirements for TSO packet */
+ if (ol_flags & PKT_TX_TCP_SEG)
+ 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;
+}
+
+/**
* Dump an mbuf structure to a file.
*
* Dump all fields for the given packet mbuf and all its associated
diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index d4156ae..85f356d 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -38,6 +38,11 @@
extern "C" {
#endif
+#include <rte_ip.h>
+#include <rte_udp.h>
+#include <rte_tcp.h>
+#include <rte_sctp.h>
+
/**
* Structure containing header lengths associated to a packet, filled
* by rte_net_get_ptype().
@@ -86,6 +91,86 @@ struct rte_net_hdr_lens {
uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
struct rte_net_hdr_lens *hdr_lens, uint32_t layers);
+/**
+ * Prepare pseudo header checksum
+ *
+ * This function prepares pseudo header checksum for TSO and non-TSO tcp/udp in
+ * provided mbufs packet data.
+ *
+ * - for non-TSO tcp/udp packets full pseudo-header checksum is counted and set
+ * in packet data,
+ * - for TSO the IP payload length is not included in pseudo header.
+ *
+ * This function expects that used headers are in the first data segment of
+ * mbuf, are not fragmented and can be safely modified.
+ *
+ * @param m
+ * The packet mbuf to be fixed.
+ * @return
+ * 0 if checksum is initialized properly
+ */
+static inline int
+rte_net_intel_cksum_prepare(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;
+}
+
#ifdef __cplusplus
}
#endif
--
1.7.9.5
^ permalink raw reply related
* [PATCH v12 0/6] add Tx preparation
From: Tomasz Kulasek @ 2016-11-23 17:36 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev, olivier.matz
In-Reply-To: <1477486575-25148-1-git-send-email-tomaszx.kulasek@intel.com>
As discussed in that thread:
http://dpdk.org/ml/archives/dev/2015-September/023603.html
Different NIC models depending on HW offload requested might impose
different requirements on packets to be TX-ed in terms of:
- Max number of fragments per packet allowed
- Max number of fragments per TSO segments
- The way pseudo-header checksum should be pre-calculated
- L3/L4 header fields filling
- etc.
MOTIVATION:
-----------
1) Some work cannot (and didn't should) be done in rte_eth_tx_burst.
However, this work is sometimes required, and now, it's an
application issue.
2) Different hardware may have different requirements for TX offloads,
other subset can be supported and so on.
3) Some parameters (e.g. number of segments in ixgbe driver) may hung
device. These parameters may be vary for different devices.
For example i40e HW allows 8 fragments per packet, but that is after
TSO segmentation. While ixgbe has a 38-fragment pre-TSO limit.
4) Fields in packet may require different initialization (like e.g. will
require pseudo-header checksum precalculation, sometimes in a
different way depending on packet type, and so on). Now application
needs to care about it.
5) Using additional API (rte_eth_tx_prep) before rte_eth_tx_burst let to
prepare packet burst in acceptable form for specific device.
6) Some additional checks may be done in debug mode keeping tx_burst
implementation clean.
PROPOSAL:
---------
To help user to deal with all these varieties we propose to:
1) Introduce rte_eth_tx_prepare() function to do necessary preparations
of packet burst to be safely transmitted on device for desired HW
offloads (set/reset checksum field according to the hardware
requirements) and check HW constraints (number of segments per
packet, etc).
While the limitations and requirements may differ for devices, it
requires to extend rte_eth_dev structure with new function pointer
"tx_pkt_prepare" which can be implemented in the driver to prepare
and verify packets, in devices specific way, before burst, what
should to prevent application to send malformed packets.
2) Also new fields will be introduced in rte_eth_desc_lim:
nb_seg_max and nb_mtu_seg_max, providing an information about max
segments in TSO and non-TSO packets acceptable by device.
This information is useful for application to not create/limit
malicious packet.
APPLICATION (CASE OF USE):
--------------------------
1) Application should to initialize burst of packets to send, set
required tx offload flags and required fields, like l2_len, l3_len,
l4_len, and tso_segsz
2) Application passes burst to the rte_eth_tx_prep to check conditions
required to send packets through the NIC.
3) The result of rte_eth_tx_prep can be used to send valid packets
and/or restore invalid if function fails.
e.g.
for (i = 0; i < nb_pkts; i++) {
/* initialize or process packet */
bufs[i]->tso_segsz = 800;
bufs[i]->ol_flags = PKT_TX_TCP_SEG | PKT_TX_IPV4
| PKT_TX_IP_CKSUM;
bufs[i]->l2_len = sizeof(struct ether_hdr);
bufs[i]->l3_len = sizeof(struct ipv4_hdr);
bufs[i]->l4_len = sizeof(struct tcp_hdr);
}
/* Prepare burst of TX packets */
nb_prep = rte_eth_tx_prepare(port, 0, bufs, nb_pkts);
if (nb_prep < nb_pkts) {
printf("Tx prepare failed\n");
/* nb_prep indicates here first invalid packet. rte_eth_tx_prep
* can be used on remaining packets to find another ones.
*/
}
/* Send burst of TX packets */
nb_tx = rte_eth_tx_burst(port, 0, bufs, nb_prep);
/* Free any unsent packets. */
v12 changes:
- renamed API function from "rte_eth_tx_prep" to "rte_eth_tx_prepare"
(to be not confused with "prepend")
- changed "rte_phdr_cksum_fix" to "rte_net_intel_cksum_prepare"
- added "csum txprep (on|off)" command to the csum engine allowing to
select txprep path for packet processing
v11 changed:
- updated comments
- added information to the API description about packet data
requirements/limitations.
v10 changes:
- moved drivers tx calback check in rte_eth_tx_prep after queue_id check
v9 changes:
- fixed headers structure fragmentation check
- moved fragmentation check into rte_validate_tx_offload()
v8 changes:
- mbuf argument in rte_validate_tx_offload declared as const
v7 changes:
- comments reworded/added
- changed errno values returned from Tx prep API
- added check in rte_phdr_cksum_fix if headers are in the first
data segment and can be safetly modified
- moved rte_validate_tx_offload to rte_mbuf
- moved rte_phdr_cksum_fix to rte_net.h
- removed rte_pkt.h new file as useless
v6 changes:
- added performance impact test results to the patch description
v5 changes:
- rebased csum engine modification
- added information to the csum engine about performance tests
- some performance improvements
v4 changes:
- tx_prep is now set to default behavior (NULL) for simple/vector path
in fm10k, i40e and ixgbe drivers to increase performance, when
Tx offloads are not intentionally available
v3 changes:
- reworked csum testpmd engine instead adding new one,
- fixed checksum initialization procedure to include also outer
checksum offloads,
- some minor formattings and optimalizations
v2 changes:
- rte_eth_tx_prep() returns number of packets when device doesn't
support tx_prep functionality,
- introduced CONFIG_RTE_ETHDEV_TX_PREP allowing to turn off tx_prep
Tomasz Kulasek (6):
ethdev: add Tx preparation
e1000: add Tx preparation
fm10k: add Tx preparation
i40e: add Tx preparation
ixgbe: add Tx preparation
testpmd: use Tx preparation in csum engine
app/test-pmd/cmdline.c | 49 ++++++++++++++++++
app/test-pmd/csumonly.c | 33 +++++++++---
app/test-pmd/testpmd.c | 5 ++
app/test-pmd/testpmd.h | 2 +
config/common_base | 1 +
drivers/net/e1000/e1000_ethdev.h | 11 ++++
drivers/net/e1000/em_ethdev.c | 5 +-
drivers/net/e1000/em_rxtx.c | 48 ++++++++++++++++-
drivers/net/e1000/igb_ethdev.c | 4 ++
drivers/net/e1000/igb_rxtx.c | 52 ++++++++++++++++++-
drivers/net/fm10k/fm10k.h | 6 +++
drivers/net/fm10k/fm10k_ethdev.c | 5 ++
drivers/net/fm10k/fm10k_rxtx.c | 50 +++++++++++++++++-
drivers/net/i40e/i40e_ethdev.c | 3 ++
drivers/net/i40e/i40e_rxtx.c | 72 +++++++++++++++++++++++++-
drivers/net/i40e/i40e_rxtx.h | 8 +++
drivers/net/ixgbe/ixgbe_ethdev.c | 3 ++
drivers/net/ixgbe/ixgbe_ethdev.h | 5 +-
drivers/net/ixgbe/ixgbe_rxtx.c | 56 ++++++++++++++++++++
drivers/net/ixgbe/ixgbe_rxtx.h | 2 +
lib/librte_ether/rte_ethdev.h | 106 ++++++++++++++++++++++++++++++++++++++
lib/librte_mbuf/rte_mbuf.h | 64 +++++++++++++++++++++++
lib/librte_net/rte_net.h | 85 ++++++++++++++++++++++++++++++
23 files changed, 662 insertions(+), 13 deletions(-)
--
1.7.9.5
^ permalink raw reply
* Re: KNI discussion in userspace event
From: Aws Ismail @ 2016-11-23 16:49 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Ferruh Yigit, DPDK
In-Reply-To: <20161028092525.696652d4@xeon-e3>
Hi Stephen, Ferruh,
As an end-user take on this (hence community comment) :), this ties into
the rte_eth_tap that Keith sent out and it has been acked and reviewed, So
I am trying to see the pros/cons of using this (kni pmd) vs. the tun/tap
PMD [1].
Previously, we were using Ferruh's KDP/KCP patches and those served our
purpose, but since the KDP/KCP idea has been rejected as yet another set of
out-of-tree kernel modules to maintain, it has not gotten much attention
since then.
We are hoping that tun/tap would be the way to go just because it looks
simple and easy to manage from a user app perspective.
Having said that, I am not sure what to make of this KNI PMD given
Stephen's comments. Could any one comment about the overall direction of
which solution to focus on? (tun/tap PMD or KNI pmd)?
[1]: http://dpdk.org/dev/patchwork/patch/16566/ (Keith's tun/tap PMD)
Thanks.
Aws\
On Fri, Oct 28, 2016 at 12:25 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:
> On Fri, 28 Oct 2016 15:31:50 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> > Discussed alternatives were:
> > * Tun/Tap
> > This won't be as fast as KNI and performance is an issue.
>
>
> That is a myth. Both require the some number of copies.
> TUN/TAP copies is a syscall and KNI copies is a kthread.
> Actually, the KNI method is worse because it has kernel thread
> always running chewing a CPU. I.e it is pure poll mode.
>
^ permalink raw reply
* Re: Proposal for a new Committer model
From: Ferruh Yigit @ 2016-11-23 16:21 UTC (permalink / raw)
To: Neil Horman; +Cc: Thomas Monjalon, dev, Mcnamara, John
In-Reply-To: <20161123153341.GC6961@hmsreliant.think-freely.org>
On 11/23/2016 3:33 PM, Neil Horman wrote:
> On Wed, Nov 23, 2016 at 02:01:44PM +0000, Ferruh Yigit wrote:
>> On 11/23/2016 1:48 PM, Neil Horman wrote:
>>> On Tue, Nov 22, 2016 at 08:56:23PM +0000, Ferruh Yigit wrote:
>>>> On 11/22/2016 7:52 PM, Neil Horman wrote:
>>>>> On Mon, Nov 21, 2016 at 09:52:41AM +0100, Thomas Monjalon wrote:
>>>>>> 2016-11-18 13:09, Neil Horman:
>>>>>>> A) Further promote subtree maintainership. This was a conversation that I
>>>>>>> proposed some time ago, but my proposed granularity was discarded in favor
>>>>>>> of something that hasn't worked as well (in my opinion). That is to say a
>>>>>>> few driver pmds (i40e and fm10k come to mind) have their own tree that
>>>>>>> send pull requests to Thomas.
>>>>>>
>>>>>> Yes we tried this fine granularity and stated that it was not working well.
>>>>>> We are now using the bigger granularity that you describe below.
>>>>>>
>>>>> Ok, thats good, but that must be _very_ new. Looking at your git tree, I see no
>>>>> merge commits. How are you pulling from those subtrees?
>>>>
>>>> next-net tree is active for last three releases.
>>>>
>>> What!? What is the purpose of holding patches in a subtree for multiple
>>> releases?
>>
>> :) Of course not holding them in the sub-tree.
>>
> Ok, glad that we're on the same page.
>
>> Briefly, process is:
>> - sub-tree gets patches during merge window
>> - sub-tree first merged into main tree in -rc1 and later in -r2
>>
>> next-net tree is actively in use for last three releases, and driver/net
>> patches delegated to this tree. You can see different commiters in main
>> tree.
>>
>>> If a given changeset isn't ready for merge to Thomas tree the people
>>> working on it should clone the subtree to some place they can all collaborate on
>>> it. Once it goes into a subtree there needs to be a defined workflow to get it
>>> into the canonical tree that Thomas maintains on a regular, short time frame.
>>> to do less is to confuse the process for everyone involved, and slow people
>>> down, rather than accelerate their work.
>>>
>>>> I guess following is the first commit to the sub-tree:
>>>> http://dpdk.org/ml/archives/dev/2016-February/032580.html
>>>>
>>>> sub-trees rebase on top of main tree regularly, that is why there is no
>>>> merge commit.
>>>>
>>> I'm not asking about merge commits in the sub-tree, I'm asking about merge
>>> commits in thomas's tree.
>>
>> Same, talking about Thomas' tree.
>>
>>> There should be a merge commit every time he pulls
>>> from a sub-tree (unless its a fast-forward I think, but with multiple subtrees
>>> and commits going to thomas directly, that should never really happen).
>>
>> That is what happening. Since sub-tree's rebase on top of main tree,
>> when Thomas merges, it is just plain fast-forward. So it is allowed to
>> re-write to history in sub-trees.
>>
> ok, I see what you're saying here, but I still don't see how this results in no
> merge commits. From what I can see we have at least 4 subtrees (next-crypto,
> next-net, next-eventdev, next-virtio). If you rebase all on lastest version of
> thomas's tree, and then they all make changes and send a pull request, the first
> to be merged will, by definition be a fast forward. The remaining three
> however, cannot be, because the prior merge has advanced the HEAD commit in
> Thomas's tree such that its divergent from the subsequent tree to be merged. So there
> should be some merge commits in Thomas's tree.
This is simple indeed, all can do fast-forward, because all sub-trees
touch to different files.
Currently:
next-net: drivers/net/* [except virtio and vhost]
next-crypto: drivers/crypto/*
next-virtio: drivers/net/virtio/*, drivers/net/vhost/*
Common files are in main tree, when all rebased on top of it, they all
can be merged as fast-forward.
>
> The only way I see around that, is if the merges are serialized (i.e. if you
> provide an order to the subtrees and each subtree rebases from thomas prior to
> applying its patches, then merges back to thomas's tree). Thats rather defeating
> of the purpose of parallel trees though. You can all rebase, but then you
> operate independently of each other. Thats how we realize a speedup here
>
>
> Neil
>
^ permalink raw reply
* Re: [PATCH] examples: fix ip_pipeline makefile typo
From: Ferruh Yigit @ 2016-11-23 15:43 UTC (permalink / raw)
To: matvejchikov, dev
In-Reply-To: <CAKh5naZ8aOmiN0WkrGKe8J7Mt0R604hhR08PpgAbdwO=6u6M0Q@mail.gmail.com>
On 11/23/2016 12:56 PM, Ilya Matveychikov wrote:
> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> ---
> examples/ip_pipeline/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/examples/ip_pipeline/Makefile b/examples/ip_pipeline/Makefile
> index 5827117..6657237 100644
> --- a/examples/ip_pipeline/Makefile
> +++ b/examples/ip_pipeline/Makefile
> @@ -36,7 +36,7 @@ endif
> # Default target, can be overridden by command line or environment
> RTE_TARGET ?= x86_64-native-linuxapp-gcc
>
> -DIRS-(CONFIG_RTE_LIBRTE_PIPELINE) += pipeline
> +DIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += pipeline
No need to fix, this line can be removed completely.
Because of:
VPATH += $(SRCDIR)/pipeline
and files handled in this makefile:
SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += pipeline_passthrough.c
When you fix DIR-y, you also need to add a makefile to pipeline folder
and update this makefile. I guess all those are not required. Just
removing that line is easier J
>
> include $(RTE_SDK)/mk/rte.vars.mk
>
> --
> 2.5.5
>
^ permalink raw reply
* Re: Proposal for a new Committer model
From: Yuanhan Liu @ 2016-11-23 15:41 UTC (permalink / raw)
To: Neil Horman
Cc: Mcnamara, John, Thomas Monjalon, dev@dpdk.org, Jerin Jacob,
Stephen Hemminger
In-Reply-To: <20161123141154.GB6961@hmsreliant.think-freely.org>
On Wed, Nov 23, 2016 at 09:11:54AM -0500, Neil Horman wrote:
> > Could we define some of the potential subtrees now and look to introduce them in the this release cycle? EAL and the Core libs, as suggested by Thomas, seem like 2 obvious ones.
> >
> Sure, I'd suggest the following:
I would pull the git history to see which components are in
active status in last release (or even, in last few release).
And try to make a sub-tree if corresponding component is hot.
# the 2nd volume shows how many patches prefixed with a related component
[yliu@yliu-dev ~/dpdk]$ git log --oneline v16.07..v16.11 | awk '{print $2}' | \
sort | uniq -c | sort -nr | head -30 | nl
1 52 doc:
2 40 net/ixgbe/base:
3 38 app/test:
4 37 kni:
5 27 vhost:
6 27 net/virtio:
7 27 net/mlx5:
8 26 app/testpmd:
9 25 net/i40e:
10 23 net/pcap:
11 22 net/bnxt:
12 20 net/enic:
13 18 net/qede:
14 17 net/thunderx:
15 16 net/qede/base:
16 16 eal:
17 15 net/ixgbe:
18 14 net:
19 14 crypto/qat:
20 13 scripts:
21 13 net/bnx2x:
22 12 net/i40e/base:
23 12 examples/ipsec-secgw:
24 11 mbuf:
25 11 hash:
26 10 lib:
27 10 examples/ip_pipeline:
28 10 ethdev:
29 9 pci:
30 7 net/vmxnet3:
...
46 3 pdump:
47 3 net/virtio_user:
48 3 net/ring:
49 3 net/nfp:
50 3 net/mlx:
51 3 net/ena:
52 3 net/e1000:
53 3 net/bonding:
...
56 2 sched:
57 2 port:
...
65 1 timer:
66 1 remove
67 1 pmdinfogen:
68 1 net/igb:
69 1 net/enic/base:
70 1 meter:
...
84 1 cfgfile:
85 1 app/procinfo:
86 1 app/proc_info:
87 1 acl:
Something obvious is that:
- "doc" deserves a sub-tree, and John is a perfect committer for that
if he's willing to.
- generally, I'd agree with Neil that most (if not all) pmds may need
a sub-tree. While, some others may not, for example, net/ring, net/pcap.
For those non-active pmds, I think it's okay to let the generic
pmd committer to cover them.
- it's not that wise to me to list all the components we have so far
and make a sub-tree for each of them.
For example, some components like librte_{port, pdump, cfgfile, acl,
and etc} just have few (or even, just one) commits in last release.
It makes no sense to me to introduce a tree for each of them.
Another thought is we could also create sub-trees based on category
but not on components like Neil suggested, especially that EAL looks
way too big to be maintained in one tree. Instead, it could be something
like:
- a tree for BSD
- a tree for ARM (and some other trees for other platforms)
- a tree for mem related (mempool, mbuf, hugepage, etc)
- a tree for BUS
- ...
Last but not the least, I think it's general good to have more and
more trees in the end. But I don't think it's a good idea to go
radically and create all those trees once (say in one release).
Something I would like to suggest is one or two (or a bit more) at
a release. For example, if I remember them well, we have next-net
tree at 16.04, and next-virtio (including vhost) at 16.07, and a
recent one, next-crypto at 16.11.
--yliu
> * net-pmds:
> - all network pmds located under drivers/net
> - librte_net
> - libtre_ether
> - librte_ip_frag
> - librte_pdump
> - librte_port
> * crypto-pmds:
> - all crypto pmds located under drivers/crypto
> - librte_cryptodev
> * eal:
> - librte_eal
> * core:
> - librte_cfgfile
> - librte_cmdline
> - librte_compat
> - librte_kvargs
> - librte_kni
> - librte_compat
> * misc:
> - librte_acl
> - librte_distributor
> - librte_hash
> - librte_jobstats
> - librte_lpm
> - librte_meter
> - librte_pipeline
> - librte_power
> - librte_reorder
> - librte_ring
> - librte_sched
> - librte_table
> - librte_timer
> - librte_vhost
>
> Thats just a rough stab mind, but perhaps it would get the ball rolling. I'd be
> willing to take maintainership of one of these subtrees if there is consensus
> around my doing so.
^ permalink raw reply
* [PATCH v2] log: do not drop debug logs at compile time
From: Olivier Matz @ 2016-11-23 15:34 UTC (permalink / raw)
To: david.marchand; +Cc: dev, thomas.monjalon, keith.wiles
In-Reply-To: <1474011832-29987-1-git-send-email-olivier.matz@6wind.com>
Today, all logs whose level is lower than INFO are dropped at
compile-time. This prevents from enabling debug logs at runtime using
--log-level=8.
The rationale was to remove debug logs from the data path at
compile-time, avoiding a test at run-time.
This patch changes the behavior of RTE_LOG() to avoid the compile-time
optimization, and introduces the RTE_LOG_DP() macro that has the same
behavior than the previous RTE_LOG(), for the rare cases where debug
logs are in the data path.
So it is now possible to enable debug logs at run-time by just
specifying --log-level=8. Some drivers still have special compile-time
options to enable more debug log. Maintainers may consider to
remove/reduce them.
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
v1 -> v2:
- fix test in RTE_LOG_DP() as pointed-out by David
config/common_base | 1 +
doc/guides/faq/faq.rst | 2 +-
drivers/net/bnxt/bnxt_txr.c | 2 +-
drivers/net/nfp/nfp_net.c | 8 +++---
examples/distributor/main.c | 4 +--
examples/ipsec-secgw/esp.c | 2 +-
examples/ipsec-secgw/ipsec.c | 4 +--
examples/packet_ordering/main.c | 6 ++--
examples/quota_watermark/qw/main.c | 2 +-
examples/tep_termination/main.c | 4 +--
examples/vhost/main.c | 14 ++++-----
examples/vhost_xen/main.c | 20 ++++++-------
lib/librte_eal/common/include/rte_log.h | 51 +++++++++++++++++++++------------
13 files changed, 68 insertions(+), 52 deletions(-)
diff --git a/config/common_base b/config/common_base
index 4bff83a..652a839 100644
--- a/config/common_base
+++ b/config/common_base
@@ -89,6 +89,7 @@ CONFIG_RTE_MAX_MEMSEG=256
CONFIG_RTE_MAX_MEMZONE=2560
CONFIG_RTE_MAX_TAILQ=32
CONFIG_RTE_LOG_LEVEL=RTE_LOG_INFO
+CONFIG_RTE_LOG_DP_LEVEL=RTE_LOG_INFO
CONFIG_RTE_LOG_HISTORY=256
CONFIG_RTE_LIBEAL_USE_HPET=n
CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
diff --git a/doc/guides/faq/faq.rst b/doc/guides/faq/faq.rst
index 8d1ea6c..0adc549 100644
--- a/doc/guides/faq/faq.rst
+++ b/doc/guides/faq/faq.rst
@@ -101,7 +101,7 @@ Yes, the option ``--log-level=`` accepts one of these numbers:
#define RTE_LOG_INFO 7U /* Informational. */
#define RTE_LOG_DEBUG 8U /* Debug-level messages. */
-It is also possible to change the maximum (and default level) at compile time
+It is also possible to change the default level at compile time
with ``CONFIG_RTE_LOG_LEVEL``.
diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
index 8bf8fee..0d15bb1 100644
--- a/drivers/net/bnxt/bnxt_txr.c
+++ b/drivers/net/bnxt/bnxt_txr.c
@@ -298,7 +298,7 @@ static int bnxt_handle_tx_cp(struct bnxt_tx_queue *txq)
if (CMP_TYPE(txcmp) == TX_CMPL_TYPE_TX_L2)
nb_tx_pkts++;
else
- RTE_LOG(DEBUG, PMD,
+ RTE_LOG_DP(DEBUG, PMD,
"Unhandled CMP type %02x\n",
CMP_TYPE(txcmp));
raw_cons = NEXT_RAW_CMP(raw_cons);
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 707be8b..e315dd8 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -1707,7 +1707,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
* DPDK just checks the queue is lower than max queues
* enabled. But the queue needs to be configured
*/
- RTE_LOG(ERR, PMD, "RX Bad queue\n");
+ RTE_LOG_DP(ERR, PMD, "RX Bad queue\n");
return -EINVAL;
}
@@ -1720,7 +1720,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
rxb = &rxq->rxbufs[idx];
if (unlikely(rxb == NULL)) {
- RTE_LOG(ERR, PMD, "rxb does not exist!\n");
+ RTE_LOG_DP(ERR, PMD, "rxb does not exist!\n");
break;
}
@@ -1740,7 +1740,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
*/
new_mb = rte_pktmbuf_alloc(rxq->mem_pool);
if (unlikely(new_mb == NULL)) {
- RTE_LOG(DEBUG, PMD, "RX mbuf alloc failed port_id=%u "
+ RTE_LOG_DP(DEBUG, PMD, "RX mbuf alloc failed port_id=%u "
"queue_id=%u\n", (unsigned)rxq->port_id,
(unsigned)rxq->qidx);
nfp_net_mbuf_alloc_failed(rxq);
@@ -1771,7 +1771,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
* responsibility of avoiding it. But we have
* to give some info about the error
*/
- RTE_LOG(ERR, PMD,
+ RTE_LOG_DP(ERR, PMD,
"mbuf overflow likely due to the RX offset.\n"
"\t\tYour mbuf size should have extra space for"
" RX offset=%u bytes.\n"
diff --git a/examples/distributor/main.c b/examples/distributor/main.c
index 537cee1..e7641d2 100644
--- a/examples/distributor/main.c
+++ b/examples/distributor/main.c
@@ -241,7 +241,7 @@ lcore_rx(struct lcore_params *p)
uint16_t sent = rte_ring_enqueue_burst(r, (void *)bufs, nb_ret);
app_stats.rx.enqueued_pkts += sent;
if (unlikely(sent < nb_ret)) {
- RTE_LOG(DEBUG, DISTRAPP,
+ RTE_LOG_DP(DEBUG, DISTRAPP,
"%s:Packet loss due to full ring\n", __func__);
while (sent < nb_ret)
rte_pktmbuf_free(bufs[sent++]);
@@ -274,7 +274,7 @@ flush_one_port(struct output_buffer *outbuf, uint8_t outp)
app_stats.tx.tx_pkts += nb_tx;
if (unlikely(nb_tx < outbuf->count)) {
- RTE_LOG(DEBUG, DISTRAPP,
+ RTE_LOG_DP(DEBUG, DISTRAPP,
"%s:Packet loss with tx_burst\n", __func__);
do {
rte_pktmbuf_free(outbuf->mbufs[nb_tx]);
diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
index ec5a2e6..9715ea9 100644
--- a/examples/ipsec-secgw/esp.c
+++ b/examples/ipsec-secgw/esp.c
@@ -78,7 +78,7 @@ esp_inbound(struct rte_mbuf *m, struct ipsec_sa *sa,
sizeof(struct esp_hdr) - sa->iv_len - sa->digest_len;
if ((payload_len & (sa->block_size - 1)) || (payload_len <= 0)) {
- RTE_LOG(DEBUG, IPSEC_ESP, "payload %d not multiple of %u\n",
+ RTE_LOG_DP(DEBUG, IPSEC_ESP, "payload %d not multiple of %u\n",
payload_len, sa->block_size);
return -EINVAL;
}
diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
index f49143b..144f0aa 100644
--- a/examples/ipsec-secgw/ipsec.c
+++ b/examples/ipsec-secgw/ipsec.c
@@ -65,7 +65,7 @@ create_session(struct ipsec_ctx *ipsec_ctx __rte_unused, struct ipsec_sa *sa)
return -1;
}
- RTE_LOG(DEBUG, IPSEC, "Create session for SA spi %u on cryptodev "
+ RTE_LOG_DP(DEBUG, IPSEC, "Create session for SA spi %u on cryptodev "
"%u qp %u\n", sa->spi,
ipsec_ctx->tbl[cdev_id_qp].id,
ipsec_ctx->tbl[cdev_id_qp].qp);
@@ -89,7 +89,7 @@ enqueue_cop(struct cdev_qp *cqp, struct rte_crypto_op *cop)
ret = rte_cryptodev_enqueue_burst(cqp->id, cqp->qp,
cqp->buf, cqp->len);
if (ret < cqp->len) {
- RTE_LOG(DEBUG, IPSEC, "Cryptodev %u queue %u:"
+ RTE_LOG_DP(DEBUG, IPSEC, "Cryptodev %u queue %u:"
" enqueued %u crypto ops out of %u\n",
cqp->id, cqp->qp,
ret, cqp->len);
diff --git a/examples/packet_ordering/main.c b/examples/packet_ordering/main.c
index 3c88b86..d4dc789 100644
--- a/examples/packet_ordering/main.c
+++ b/examples/packet_ordering/main.c
@@ -229,7 +229,7 @@ flush_tx_error_callback(struct rte_mbuf **unsent, uint16_t count,
/* free the mbufs which failed from transmit */
app_stats.tx.ro_tx_failed_pkts += count;
- RTE_LOG(DEBUG, REORDERAPP, "%s:Packet loss with tx_burst\n", __func__);
+ RTE_LOG_DP(DEBUG, REORDERAPP, "%s:Packet loss with tx_burst\n", __func__);
pktmbuf_free_bulk(unsent, count);
}
@@ -410,7 +410,7 @@ rx_thread(struct rte_ring *ring_out)
nb_rx_pkts = rte_eth_rx_burst(port_id, 0,
pkts, MAX_PKTS_BURST);
if (nb_rx_pkts == 0) {
- RTE_LOG(DEBUG, REORDERAPP,
+ RTE_LOG_DP(DEBUG, REORDERAPP,
"%s():Received zero packets\n", __func__);
continue;
}
@@ -522,7 +522,7 @@ send_thread(struct send_thread_args *args)
if (ret == -1 && rte_errno == ERANGE) {
/* Too early pkts should be transmitted out directly */
- RTE_LOG(DEBUG, REORDERAPP,
+ RTE_LOG_DP(DEBUG, REORDERAPP,
"%s():Cannot reorder early packet "
"direct enqueuing to TX\n", __func__);
outp = mbufs[i]->port;
diff --git a/examples/quota_watermark/qw/main.c b/examples/quota_watermark/qw/main.c
index 8ed0214..9162e28 100644
--- a/examples/quota_watermark/qw/main.c
+++ b/examples/quota_watermark/qw/main.c
@@ -81,7 +81,7 @@ static void send_pause_frame(uint8_t port_id, uint16_t duration)
struct ether_hdr *hdr;
struct ether_addr mac_addr;
- RTE_LOG(DEBUG, USER1, "Sending PAUSE frame (duration=%d) on port %d\n",
+ RTE_LOG_DP(DEBUG, USER1, "Sending PAUSE frame (duration=%d) on port %d\n",
duration, port_id);
/* Get a mbuf from the pool */
diff --git a/examples/tep_termination/main.c b/examples/tep_termination/main.c
index 1d6d463..bd1dc96 100644
--- a/examples/tep_termination/main.c
+++ b/examples/tep_termination/main.c
@@ -567,7 +567,7 @@ virtio_tx_route(struct vhost_dev *vdev, struct rte_mbuf *m)
unsigned len, ret = 0;
const uint16_t lcore_id = rte_lcore_id();
- RTE_LOG(DEBUG, VHOST_DATA, "(%d) TX: MAC address is external\n",
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%d) TX: MAC address is external\n",
vdev->vid);
/* Add packet to the port tx queue */
@@ -649,7 +649,7 @@ switch_worker(__rte_unused void *arg)
if (unlikely(diff_tsc > drain_tsc)) {
if (tx_q->len) {
- RTE_LOG(DEBUG, VHOST_DATA, "TX queue drained after "
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "TX queue drained after "
"timeout with burst size %u\n",
tx_q->len);
ret = overlay_options.tx_handle(ports[0],
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 0709859..ac1f6e2 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -832,17 +832,17 @@ virtio_tx_local(struct vhost_dev *vdev, struct rte_mbuf *m)
return -1;
if (vdev->vid == dst_vdev->vid) {
- RTE_LOG(DEBUG, VHOST_DATA,
+ RTE_LOG_DP(DEBUG, VHOST_DATA,
"(%d) TX: src and dst MAC is same. Dropping packet.\n",
vdev->vid);
return 0;
}
- RTE_LOG(DEBUG, VHOST_DATA,
+ RTE_LOG_DP(DEBUG, VHOST_DATA,
"(%d) TX: MAC address is local\n", dst_vdev->vid);
if (unlikely(dst_vdev->remove)) {
- RTE_LOG(DEBUG, VHOST_DATA,
+ RTE_LOG_DP(DEBUG, VHOST_DATA,
"(%d) device is marked for removal\n", dst_vdev->vid);
return 0;
}
@@ -867,7 +867,7 @@ find_local_dest(struct vhost_dev *vdev, struct rte_mbuf *m,
return 0;
if (vdev->vid == dst_vdev->vid) {
- RTE_LOG(DEBUG, VHOST_DATA,
+ RTE_LOG_DP(DEBUG, VHOST_DATA,
"(%d) TX: src and dst MAC is same. Dropping packet.\n",
vdev->vid);
return -1;
@@ -881,7 +881,7 @@ find_local_dest(struct vhost_dev *vdev, struct rte_mbuf *m,
*offset = VLAN_HLEN;
*vlan_tag = vlan_tags[vdev->vid];
- RTE_LOG(DEBUG, VHOST_DATA,
+ RTE_LOG_DP(DEBUG, VHOST_DATA,
"(%d) TX: pkt to local VM device id: (%d), vlan tag: %u.\n",
vdev->vid, dst_vdev->vid, *vlan_tag);
@@ -973,7 +973,7 @@ virtio_tx_route(struct vhost_dev *vdev, struct rte_mbuf *m, uint16_t vlan_tag)
}
}
- RTE_LOG(DEBUG, VHOST_DATA,
+ RTE_LOG_DP(DEBUG, VHOST_DATA,
"(%d) TX: MAC address is external\n", vdev->vid);
queue2nic:
@@ -1041,7 +1041,7 @@ drain_mbuf_table(struct mbuf_table *tx_q)
if (unlikely(cur_tsc - prev_tsc > MBUF_TABLE_DRAIN_TSC)) {
prev_tsc = cur_tsc;
- RTE_LOG(DEBUG, VHOST_DATA,
+ RTE_LOG_DP(DEBUG, VHOST_DATA,
"TX queue drained after timeout with burst size %u\n",
tx_q->len);
do_drain_mbuf_table(tx_q);
diff --git a/examples/vhost_xen/main.c b/examples/vhost_xen/main.c
index f4dbaa4..af05b00 100644
--- a/examples/vhost_xen/main.c
+++ b/examples/vhost_xen/main.c
@@ -525,7 +525,7 @@ gpa_to_vva(struct virtio_net *dev, uint64_t guest_pa)
break;
}
}
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") GPA %p| VVA %p\n",
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") GPA %p| VVA %p\n",
dev->device_fh, (void*)(uintptr_t)guest_pa, (void*)(uintptr_t)vhost_va);
return vhost_va;
@@ -555,7 +555,7 @@ virtio_dev_rx(struct virtio_net *dev, struct rte_mbuf **pkts, uint32_t count)
uint8_t success = 0;
void *userdata;
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") virtio_dev_rx()\n", dev->device_fh);
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") virtio_dev_rx()\n", dev->device_fh);
vq = dev->virtqueue_rx;
count = (count > MAX_PKT_BURST) ? MAX_PKT_BURST : count;
/* As many data cores may want access to available buffers, they need to be reserved. */
@@ -580,7 +580,7 @@ virtio_dev_rx(struct virtio_net *dev, struct rte_mbuf **pkts, uint32_t count)
res_end_idx);
} while (unlikely(success == 0));
res_cur_idx = res_base_idx;
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") Current Index %d| End Index %d\n",
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") Current Index %d| End Index %d\n",
dev->device_fh, res_cur_idx, res_end_idx);
/* Prefetch available ring to retrieve indexes. */
@@ -775,7 +775,7 @@ virtio_tx_local(struct virtio_net *dev, struct rte_mbuf *m)
/* Drop the packet if the TX packet is destined for the TX device. */
if (dev_ll->dev->device_fh == dev->device_fh) {
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") TX: "
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") TX: "
"Source and destination MAC addresses are the same. "
"Dropping packet.\n",
dev_ll->dev->device_fh);
@@ -783,12 +783,12 @@ virtio_tx_local(struct virtio_net *dev, struct rte_mbuf *m)
}
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") TX: "
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") TX: "
"MAC address is local\n", dev_ll->dev->device_fh);
if (dev_ll->dev->remove) {
/*drop the packet if the device is marked for removal*/
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") "
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") "
"Device is marked for removal\n",
dev_ll->dev->device_fh);
} else {
@@ -829,7 +829,7 @@ virtio_tx_route(struct virtio_net* dev, struct rte_mbuf *m, struct rte_mempool *
return;
}
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") TX: "
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") TX: "
"MAC address is external\n", dev->device_fh);
/*Add packet to the port tx queue*/
@@ -903,7 +903,7 @@ virtio_dev_tx(struct virtio_net* dev, struct rte_mempool *mbuf_pool)
if (vq->last_used_idx == avail_idx)
return;
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") virtio_dev_tx()\n",
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") virtio_dev_tx()\n",
dev->device_fh);
/* Prefetch available ring to retrieve head indexes. */
@@ -913,7 +913,7 @@ virtio_dev_tx(struct virtio_net* dev, struct rte_mempool *mbuf_pool)
free_entries = avail_idx - vq->last_used_idx;
free_entries = unlikely(free_entries < MAX_PKT_BURST) ? free_entries : MAX_PKT_BURST;
- RTE_LOG(DEBUG, VHOST_DATA, "(%" PRIu64 ") Buffers available %d\n",
+ RTE_LOG_DP(DEBUG, VHOST_DATA, "(%" PRIu64 ") Buffers available %d\n",
dev->device_fh, free_entries);
/* Retrieve all of the head indexes first to avoid caching issues. */
for (i = 0; i < free_entries; i++)
@@ -1003,7 +1003,7 @@ switch_worker(__attribute__((unused)) void *arg)
if (unlikely(diff_tsc > drain_tsc)) {
if (tx_q->len) {
- RTE_LOG(DEBUG, VHOST_DATA,
+ RTE_LOG_DP(DEBUG, VHOST_DATA,
"TX queue drained after timeout with burst size %u\n",
tx_q->len);
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 29f7d19..671e274 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -118,9 +118,8 @@ int rte_openlog_stream(FILE *f);
/**
* Set the global log level.
*
- * After this call, all logs that are lower or equal than level and
- * lower or equal than the RTE_LOG_LEVEL configuration option will be
- * displayed.
+ * After this call, logs with a level lower or equal than the level
+ * passed as argument will be displayed.
*
* @param level
* Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
@@ -184,9 +183,8 @@ int rte_log_cur_msg_logtype(void);
* The level argument determines if the log should be displayed or
* not, depending on the global rte_logs variable.
*
- * The preferred alternative is the RTE_LOG() function because debug logs may
- * be removed at compilation time if optimization is enabled. Moreover,
- * logs are automatically prefixed by type when using the macro.
+ * The preferred alternative is the RTE_LOG() because it adds the
+ * level and type in the logged string.
*
* @param level
* Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
@@ -217,8 +215,8 @@ int rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
* not, depending on the global rte_logs variable. A trailing
* newline may be added if needed.
*
- * The preferred alternative is the RTE_LOG() because debug logs may be
- * removed at compilation time.
+ * The preferred alternative is the RTE_LOG() because it adds the
+ * level and type in the logged string.
*
* @param level
* Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
@@ -239,15 +237,8 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
/**
* Generates a log message.
*
- * The RTE_LOG() is equivalent to rte_log() with two differences:
-
- * - RTE_LOG() can be used to remove debug logs at compilation time,
- * depending on RTE_LOG_LEVEL configuration option, and compilation
- * optimization level. If optimization is enabled, the tests
- * involving constants only are pre-computed. If compilation is done
- * with -O0, these tests will be done at run time.
- * - The log level and log type names are smaller, for example:
- * RTE_LOG(INFO, EAL, "this is a %s", "log");
+ * The RTE_LOG() is a helper that prefixes the string with the log level
+ * and type, and call rte_log().
*
* @param l
* Log level. A value between EMERG (1) and DEBUG (8). The short name is
@@ -263,7 +254,31 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
* - Negative on error.
*/
#define RTE_LOG(l, t, ...) \
- (void)((RTE_LOG_ ## l <= RTE_LOG_LEVEL) ? \
+ rte_log(RTE_LOG_ ## l, \
+ RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
+
+/**
+ * Generates a log message for data path.
+ *
+ * Similar to RTE_LOG(), except that it is removed at compilation time
+ * if the RTE_LOG_DP_LEVEL configuration option is lower than the log
+ * level argument.
+ *
+ * @param l
+ * Log level. A value between EMERG (1) and DEBUG (8). The short name is
+ * expanded by the macro, so it cannot be an integer value.
+ * @param t
+ * The log type, for example, EAL. The short name is expanded by the
+ * macro, so it cannot be an integer value.
+ * @param ...
+ * The fmt string, as in printf(3), followed by the variable arguments
+ * required by the format.
+ * @return
+ * - 0: Success.
+ * - Negative on error.
+ */
+#define RTE_LOG_DP(l, t, ...) \
+ (void)((RTE_LOG_ ## l <= RTE_LOG_DP_LEVEL) ? \
rte_log(RTE_LOG_ ## l, \
RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) : \
0)
--
2.8.1
^ permalink raw reply related
* Re: Proposal for a new Committer model
From: Neil Horman @ 2016-11-23 15:33 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Thomas Monjalon, dev, Mcnamara, John
In-Reply-To: <a62396ac-dbb2-0a0c-dee6-ecb9bdbf0c8c@intel.com>
On Wed, Nov 23, 2016 at 02:01:44PM +0000, Ferruh Yigit wrote:
> On 11/23/2016 1:48 PM, Neil Horman wrote:
> > On Tue, Nov 22, 2016 at 08:56:23PM +0000, Ferruh Yigit wrote:
> >> On 11/22/2016 7:52 PM, Neil Horman wrote:
> >>> On Mon, Nov 21, 2016 at 09:52:41AM +0100, Thomas Monjalon wrote:
> >>>> 2016-11-18 13:09, Neil Horman:
> >>>>> A) Further promote subtree maintainership. This was a conversation that I
> >>>>> proposed some time ago, but my proposed granularity was discarded in favor
> >>>>> of something that hasn't worked as well (in my opinion). That is to say a
> >>>>> few driver pmds (i40e and fm10k come to mind) have their own tree that
> >>>>> send pull requests to Thomas.
> >>>>
> >>>> Yes we tried this fine granularity and stated that it was not working well.
> >>>> We are now using the bigger granularity that you describe below.
> >>>>
> >>> Ok, thats good, but that must be _very_ new. Looking at your git tree, I see no
> >>> merge commits. How are you pulling from those subtrees?
> >>
> >> next-net tree is active for last three releases.
> >>
> > What!? What is the purpose of holding patches in a subtree for multiple
> > releases?
>
> :) Of course not holding them in the sub-tree.
>
Ok, glad that we're on the same page.
> Briefly, process is:
> - sub-tree gets patches during merge window
> - sub-tree first merged into main tree in -rc1 and later in -r2
>
> next-net tree is actively in use for last three releases, and driver/net
> patches delegated to this tree. You can see different commiters in main
> tree.
>
> > If a given changeset isn't ready for merge to Thomas tree the people
> > working on it should clone the subtree to some place they can all collaborate on
> > it. Once it goes into a subtree there needs to be a defined workflow to get it
> > into the canonical tree that Thomas maintains on a regular, short time frame.
> > to do less is to confuse the process for everyone involved, and slow people
> > down, rather than accelerate their work.
> >
> >> I guess following is the first commit to the sub-tree:
> >> http://dpdk.org/ml/archives/dev/2016-February/032580.html
> >>
> >> sub-trees rebase on top of main tree regularly, that is why there is no
> >> merge commit.
> >>
> > I'm not asking about merge commits in the sub-tree, I'm asking about merge
> > commits in thomas's tree.
>
> Same, talking about Thomas' tree.
>
> > There should be a merge commit every time he pulls
> > from a sub-tree (unless its a fast-forward I think, but with multiple subtrees
> > and commits going to thomas directly, that should never really happen).
>
> That is what happening. Since sub-tree's rebase on top of main tree,
> when Thomas merges, it is just plain fast-forward. So it is allowed to
> re-write to history in sub-trees.
>
ok, I see what you're saying here, but I still don't see how this results in no
merge commits. From what I can see we have at least 4 subtrees (next-crypto,
next-net, next-eventdev, next-virtio). If you rebase all on lastest version of
thomas's tree, and then they all make changes and send a pull request, the first
to be merged will, by definition be a fast forward. The remaining three
however, cannot be, because the prior merge has advanced the HEAD commit in
Thomas's tree such that its divergent from the subsequent tree to be merged. So there
should be some merge commits in Thomas's tree.
The only way I see around that, is if the merges are serialized (i.e. if you
provide an order to the subtrees and each subtree rebases from thomas prior to
applying its patches, then merges back to thomas's tree). Thats rather defeating
of the purpose of parallel trees though. You can all rebase, but then you
operate independently of each other. Thats how we realize a speedup here
Neil
^ permalink raw reply
* Re: [PATCH 00/56] Solarflare libefx-based PMD
From: Ferruh Yigit @ 2016-11-23 15:29 UTC (permalink / raw)
To: Andrew Rybchenko, dev
In-Reply-To: <1479740470-6723-1-git-send-email-arybchenko@solarflare.com>
On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
> The patch series adds Solarflare libefx-based network PMD.
>
> This version of the driver supports Solarflare SFN7xxx and SFN8xxx
> families of 10/40 Gbps adapters.
>
> libefx is a platform-independent library to implement drivers for
> Solarflare network adapters. It provides unified adapter family
> independent interface (if possible). FreeBSD [1] and illumos [2]
> drivers are built on top of the library.
>
> The patch series could be logically structured into 5 sub-series:
> 1. (1) add the driver skeleton including documentation
> 2. (2-30) import libefx and include it in build with the latest patch
> 3. (31-43) implement minimal device level operations in steps
> 4. (44-51) implement Rx subsystem
> 5. (52-56) implement Tx subsystem
>
> Functional driver with multi-queue support capable to send and receive
> traffic appears with the last patch in the series.
>
> The following design decisions are made during development:
>
> 1. Since libefx uses positive errno return codes, positive errno
> return codes are used inside the driver and coversion to negative
> is done on return from eth_dev_ops callbacks. We think that it
> is the less error-prone way.
>
> 2. Another Solarflare PMD with in-kernel part (for control operations)
> is considered and could be added in the future. Code for data path
> should be shared by these two drivers. libefx-based PMD is put into
> 'efx' subdirectory to have a space for another PMD and shared code.
>
> 3. Own event queue (a way to deliver events from HW to host CPU) is
> used for house-keeping (e.g. link status notifications), each Tx
> and each Rx queue. No locks on datapath are requires in this case.
>
> 4. Alarm is used to periodically poll house-keeping event queue.
> The event queue is used to deliver link status change notifications,
> Rx/Tx queue flush events, SRAM events. It is not used on datapath.
> The event queue polling is protected using spin-lock since
> concurrent access from different contexts is possible (e.g. device
> stop when polling alarm is running).
>
> [1] https://svnweb.freebsd.org/base/head/sys/dev/sfxge/common/
> [2] https://github.com/illumos/illumos-gate/tree/master/usr/src/uts/common/io/sfxge/common/
>
> ---
I would like to note that very well organized patchset. Thank you for
your effort.
Thanks,
ferruh
^ permalink raw reply
* Re: [PATCH 33/56] net/sfc: add device configure and close stubs
From: Ferruh Yigit @ 2016-11-23 15:26 UTC (permalink / raw)
To: Andrew Rybchenko, dev
In-Reply-To: <1479740470-6723-34-git-send-email-arybchenko@solarflare.com>
On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
<...>
> diff --git a/drivers/net/sfc/efx/sfc.h b/drivers/net/sfc/efx/sfc.h
> index 01d652d..d040f98 100644
> --- a/drivers/net/sfc/efx/sfc.h
> +++ b/drivers/net/sfc/efx/sfc.h
<...>
> @@ -131,7 +147,7 @@ sfc_adapter_unlock(struct sfc_adapter *sa)
> }
>
> static inline void
> -sfc_adapter_lock_destroy(struct sfc_adapter *sa)
> +sfc_adapter_lock_fini(struct sfc_adapter *sa)
Why not do proper naming in 32/56 at first place?
<...>
^ permalink raw reply
* Re: [PATCH 32/56] net/sfc: implement driver operation to init device on attach
From: Ferruh Yigit @ 2016-11-23 15:26 UTC (permalink / raw)
To: Andrew Rybchenko, dev
In-Reply-To: <1479740470-6723-33-git-send-email-arybchenko@solarflare.com>
On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
> The setup and configuration of the PMD is not performance sensitive,
> but is not thread safe either. It is possible that the multiple
> read/writes during PMD setup and configuration could be corrupted
> in a multi-thread environment.
Right, this is not thread-safe, but this is valid for all PMDs, it is
not expected to have initialization in multi-threaded environment, that
said so, synchronization also won't hurt, as you said this is not fast
path, just may not be necessary.
> Since this is not performance
> sensitive, the developer can choose to add their own layer to provide
> thread-safe setup and configuration. It is expected that, in most
> applications, the initial configuration of the network ports would be
> done by a single thread at startup.
>
> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
<...>
> diff --git a/drivers/net/sfc/efx/sfc_ethdev.c b/drivers/net/sfc/efx/sfc_ethdev.c
> index 0deff07..e5b609c 100644
> --- a/drivers/net/sfc/efx/sfc_ethdev.c
> +++ b/drivers/net/sfc/efx/sfc_ethdev.c
> @@ -31,6 +31,8 @@
> #include <rte_ethdev.h>
> #include <rte_pci.h>
>
> +#include "efx.h"
> +
> #include "sfc.h"
> #include "sfc_debug.h"
> #include "sfc_log.h"
> @@ -55,6 +57,8 @@ sfc_eth_dev_init(struct rte_eth_dev *dev)
> struct sfc_adapter *sa = dev->data->dev_private;
> struct rte_pci_device *pci_dev = dev->pci_dev;
> int rc;
> + const efx_nic_cfg_t *encp;
> + const struct ether_addr *from;
>
> /* Required for logging */
> sa->eth_dev = dev;
> @@ -73,11 +77,43 @@ sfc_eth_dev_init(struct rte_eth_dev *dev)
>
> sfc_log_init(sa, "entry");
>
> + dev->data->mac_addrs = rte_zmalloc("sfc", ETHER_ADDR_LEN, 0);
> + if (dev->data->mac_addrs == NULL) {
> + rc = ENOMEM;
> + goto fail_mac_addrs;
> + }
> +
> + sfc_adapter_lock_init(sa);
> + sfc_adapter_lock(sa);
> +
> + sfc_log_init(sa, "attaching");
> + rc = sfc_attach(sa);
> + if (rc != 0)
> + goto fail_attach;
> +
> + encp = efx_nic_cfg_get(sa->nic);
> +
> + /*
> + * The arguments are really reverse order in comparison to
> + * Linux kernel. Copy from NIC config to Ethernet device data.
> + */
Yes it is J, and agreed this is confusing.
> + from = (const struct ether_addr *)(encp->enc_mac_addr);
> + ether_addr_copy(from, &dev->data->mac_addrs[0]);
> +
<...>
^ permalink raw reply
* Re: [PATCH 31/56] net/sfc: implement dummy callback to get device information
From: Ferruh Yigit @ 2016-11-23 15:26 UTC (permalink / raw)
To: Andrew Rybchenko, dev
In-Reply-To: <1479740470-6723-32-git-send-email-arybchenko@solarflare.com>
On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
> Just a stub to be filled in when corresponding functionality is
> implemented.
What about merging this stub with real implementation?
Or perhaps replace with code that adds dummy .dev_configure?
>
> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> drivers/net/sfc/efx/sfc_ethdev.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/sfc/efx/sfc_ethdev.c b/drivers/net/sfc/efx/sfc_ethdev.c
> index ff20a13..0deff07 100644
> --- a/drivers/net/sfc/efx/sfc_ethdev.c
> +++ b/drivers/net/sfc/efx/sfc_ethdev.c
> @@ -37,9 +37,16 @@
> #include "sfc_kvargs.h"
>
>
> +static void
> +sfc_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> +{
> + struct sfc_adapter *sa = dev->data->dev_private;
> +
> + sfc_log_init(sa, "entry");
> +}
> +
> static const struct eth_dev_ops sfc_eth_dev_ops = {
> - /* Just dummy init to avoid build-time warning */
> - .dev_configure = NULL,
> + .dev_infos_get = sfc_dev_infos_get,
> };
>
> static int
>
^ permalink raw reply
* Re: [PATCH 30/56] net/sfc: include libefx in build
From: Ferruh Yigit @ 2016-11-23 15:26 UTC (permalink / raw)
To: Andrew Rybchenko, dev; +Cc: Artem Andreev
In-Reply-To: <1479740470-6723-31-git-send-email-arybchenko@solarflare.com>
On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
> From: Artem Andreev <Artem.Andreev@oktetlabs.ru>
>
> Implement efsys.h for the PMD.
>
> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
> Signed-off-by: Artem Andreev <Artem.Andreev@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> drivers/net/sfc/efx/Makefile | 54 +++
> drivers/net/sfc/efx/efsys.h | 767 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 821 insertions(+)
> create mode 100644 drivers/net/sfc/efx/efsys.h
>
> diff --git a/drivers/net/sfc/efx/Makefile b/drivers/net/sfc/efx/Makefile
> index 71f07ca..de95ea8 100644
> --- a/drivers/net/sfc/efx/Makefile
> +++ b/drivers/net/sfc/efx/Makefile
> @@ -33,6 +33,8 @@ include $(RTE_SDK)/mk/rte.vars.mk
> #
> LIB = librte_pmd_sfc_efx.a
>
> +CFLAGS += -I$(SRCDIR)/base/
> +CFLAGS += -I$(SRCDIR)
> CFLAGS += -O3
>
> # Enable basic warnings but disable some which are accepted
> @@ -60,6 +62,17 @@ CFLAGS += -Wstrict-prototypes
> CFLAGS += -Wundef
> CFLAGS += -Wwrite-strings
>
> +# Extra CFLAGS for base driver files
> +CFLAGS_BASE_DRIVER += -Wno-unused-variable
> +CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable
clang complain about this one:
warning: unknown warning option '-Wno-unused-but-set-variable'; did you
mean '-Wno-unused-const-variable'? [-Wunknown-warning-option]
> +
> +#
> +# List of base driver object files for which
> +# special CFLAGS above should be applied
> +#
> +BASE_DRIVER_OBJS=$(patsubst %.c,%.o,$(notdir $(wildcard $(SRCDIR)/base/*.c)))
> +$(foreach obj, $(BASE_DRIVER_OBJS), $(eval CFLAGS+=$(CFLAGS_BASE_DRIVER)))
This cause multiple "-Wno-unused-variable -Wno-unused-but-set-variable"
params in final command, I guess the intention is:
$(foreach obj, $(BASE_DRIVER_OBJS), $(eval
CFLAGS_$(obj)+=$(CFLAGS_BASE_DRIVER)))
Fixing this may generate a few compiler warnings.
<...>
> diff --git a/drivers/net/sfc/efx/efsys.h b/drivers/net/sfc/efx/efsys.h
> new file mode 100644
> index 0000000..2eef996
> --- /dev/null
> +++ b/drivers/net/sfc/efx/efsys.h
> @@ -0,0 +1,767 @@
<...>
I guess below is hardcoded compile time configuration for libefx, do you
think does it make sense to document this default configuration?
> +
> +#define EFSYS_OPT_NAMES 0
> +
> +#define EFSYS_OPT_SIENA 0
> +#define EFSYS_OPT_HUNTINGTON 1
> +#define EFSYS_OPT_MEDFORD 1
> +#ifdef RTE_LIBRTE_SFC_EFX_DEBUG
> +#define EFSYS_OPT_CHECK_REG 1
> +#else
> +#define EFSYS_OPT_CHECK_REG 0
> +#endif
> +
> +#define EFSYS_OPT_MCDI 1
> +#define EFSYS_OPT_MCDI_LOGGING 0
> +#define EFSYS_OPT_MCDI_PROXY_AUTH 0
> +
> +#define EFSYS_OPT_MAC_STATS 0
> +
> +#define EFSYS_OPT_LOOPBACK 0
> +
> +#define EFSYS_OPT_MON_MCDI 0
> +#define EFSYS_OPT_MON_STATS 0
> +
> +#define EFSYS_OPT_PHY_STATS 0
> +#define EFSYS_OPT_BIST 0
> +#define EFSYS_OPT_PHY_LED_CONTROL 0
> +#define EFSYS_OPT_PHY_FLAGS 0
> +
> +#define EFSYS_OPT_VPD 0
> +#define EFSYS_OPT_NVRAM 0
> +#define EFSYS_OPT_BOOTCFG 0
> +
> +#define EFSYS_OPT_DIAG 0
> +#define EFSYS_OPT_RX_SCALE 0
> +#define EFSYS_OPT_QSTATS 0
> +#define EFSYS_OPT_FILTER 1
> +#define EFSYS_OPT_RX_SCATTER 0
> +
> +#define EFSYS_OPT_EV_PREFETCH 0
> +
> +#define EFSYS_OPT_DECODE_INTR_FATAL 0
> +
> +#define EFSYS_OPT_LICENSING 0
> +
> +#define EFSYS_OPT_ALLOW_UNCONFIGURED_NIC 0
> +
> +#define EFSYS_OPT_RX_PACKED_STREAM 0
<...>
^ permalink raw reply
* Re: [PATCH 01/56] net/sfc: libefx-based PMD stub sufficient to build and init
From: Ferruh Yigit @ 2016-11-23 15:26 UTC (permalink / raw)
To: Andrew Rybchenko, dev
In-Reply-To: <1479740470-6723-2-git-send-email-arybchenko@solarflare.com>
On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
> The PMD is put into the sfc/efx subdirectory to have a place for
> the second PMD and library shared by both.
>
> Enable the PMD by default on supported configuratons.
>
> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> MAINTAINERS | 6 ++
> config/common_base | 6 ++
> config/defconfig_arm-armv7a-linuxapp-gcc | 1 +
> config/defconfig_arm64-armv8a-linuxapp-gcc | 1 +
> config/defconfig_i686-native-linuxapp-gcc | 5 +
> config/defconfig_i686-native-linuxapp-icc | 5 +
> config/defconfig_ppc_64-power8-linuxapp-gcc | 1 +
> config/defconfig_tile-tilegx-linuxapp-gcc | 1 +
> config/defconfig_x86_64-native-linuxapp-icc | 5 +
> config/defconfig_x86_x32-native-linuxapp-gcc | 5 +
> doc/guides/nics/features/sfc_efx.ini | 10 ++
> doc/guides/nics/index.rst | 1 +
> doc/guides/nics/sfc_efx.rst | 109 +++++++++++++++++++++
Can you also update release notes please, to announce new driver.
<...>
> diff --git a/drivers/net/sfc/efx/Makefile b/drivers/net/sfc/efx/Makefile
> new file mode 100644
> index 0000000..71f07ca
> --- /dev/null
> +++ b/drivers/net/sfc/efx/Makefile
> @@ -0,0 +1,81 @@
<...>
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +#
> +# library name
> +#
> +LIB = librte_pmd_sfc_efx.a
> +
> +CFLAGS += -O3
> +
> +# Enable basic warnings but disable some which are accepted
> +CFLAGS += -Wall
It is possible to use $(WERROR_FLAGS), which set automatically based on
selected compiler. See mk/toolchain/* .
And you can add extra options here, please keep in mind that there are
three compiler supported right now: gcc, clang and icc. You may require
to add compiler and version checks..
> +CFLAGS += -Wno-strict-aliasing
> +
> +# Enable extra warnings but disable some which are accepted
> +CFLAGS += -Wextra
> +CFLAGS += -Wno-empty-body
> +CFLAGS += -Wno-sign-compare
> +CFLAGS += -Wno-type-limits
> +CFLAGS += -Wno-unused-parameter
Is there a way to not disable these warnings but fix in the source code?
Or move to CFLAGS_BASE_DRIVER, if the reason is the base driver?
> +
> +# More warnings not enabled by above aggregators
> +CFLAGS += -Waggregate-return
> +CFLAGS += -Wbad-function-cast
> +CFLAGS += -Wcast-qual
> +CFLAGS += -Wdisabled-optimization
> +CFLAGS += -Wmissing-declarations
> +CFLAGS += -Wmissing-prototypes
> +CFLAGS += -Wnested-externs
> +CFLAGS += -Wold-style-definition
> +CFLAGS += -Wpointer-arith
> +CFLAGS += -Wstrict-prototypes
> +CFLAGS += -Wundef
> +CFLAGS += -Wwrite-strings
If you believe some can be useful for everybody, please feel free to add
to mk/toolchain/* .
> +
> +EXPORT_MAP := rte_pmd_sfc_efx_version.map
> +
> +LIBABIVER := 1
> +
> +#
> +# all source are stored in SRCS-y
> +#
> +SRCS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += sfc_ethdev.c
> +SRCS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += sfc_kvargs.c
> +
> +
> +# this lib depends upon:
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_eal
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_kvargs
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_ether
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_mempool
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_mbuf
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map b/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map
> new file mode 100644
> index 0000000..1901bcb
> --- /dev/null
> +++ b/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map
> @@ -0,0 +1,4 @@
> +DPDK_16.07 {
Now this become 17.02
> +
> + local: *;
> +};
> diff --git a/drivers/net/sfc/efx/sfc.h b/drivers/net/sfc/efx/sfc.h
> new file mode 100644
> index 0000000..16fd2bb
> --- /dev/null
> +++ b/drivers/net/sfc/efx/sfc.h
> @@ -0,0 +1,53 @@
<..>
> +
> +#ifndef _SFC_H
> +#define _SFC_H
s/^I/ /
This also exists in other locations and files..
<...>
^ permalink raw reply
* Re: Proposal for a new Committer model
From: Neil Horman @ 2016-11-23 14:11 UTC (permalink / raw)
To: Mcnamara, John
Cc: Thomas Monjalon, dev@dpdk.org, Jerin Jacob, Stephen Hemminger
In-Reply-To: <B27915DBBA3421428155699D51E4CFE2026648E4@IRSMSX103.ger.corp.intel.com>
On Wed, Nov 23, 2016 at 08:21:39AM +0000, Mcnamara, John wrote:
>
>
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Tuesday, November 22, 2016 7:52 PM
> > To: Thomas Monjalon <thomas.monjalon@6wind.com>
> > Cc: dev@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>
> > Subject: Re: [dpdk-dev] Proposal for a new Committer model
> >
> > On Mon, Nov 21, 2016 at 09:52:41AM +0100, Thomas Monjalon wrote:
> > > 2016-11-18 13:09, Neil Horman:
> > > > A) Further promote subtree maintainership. This was a conversation
> > > > that I proposed some time ago, but my proposed granularity was
> > > > discarded in favor of something that hasn't worked as well (in my
> > > > opinion). That is to say a few driver pmds (i40e and fm10k come to
> > > > mind) have their own tree that send pull requests to Thomas.
> > >
> > > Yes we tried this fine granularity and stated that it was not working
> > well.
> > > We are now using the bigger granularity that you describe below.
> > >
> > Ok, thats good, but that must be _very_ new. Looking at your git tree, I
> > see no merge commits. How are you pulling from those subtrees?
> >
>
>
> Hi Neil,
>
> It seems like the weight of consensus in around your proposal for further subtree maintainers and backups. If you don't mind I'll take your text and redraft it as a potential section on maintainership for a future Project Charter document. Or at least so that we have a documented maintainship process.
>
Sure, that sounds good to me. Thank you.
>
> > > > We should be sharding that at a much higher granularity and using it
> > > > much more consistently. That is to say, that we should have a
> > > > maintainer for all the ethernet pmds, and another for the crypto
> > > > pmds, another for the core eal layer, another for misc libraries
> > > > that have low patch volumes, etc.
> > >
> > > Yes we could open a tree for EAL and another one for the core libraries.
> > >
> > That could be worthwhile. Lets see how the net and crypto subtrees work
> > out (assuming again that these trees are newly founded)
>
> Could we define some of the potential subtrees now and look to introduce them in the this release cycle? EAL and the Core libs, as suggested by Thomas, seem like 2 obvious ones.
>
Sure, I'd suggest the following:
* net-pmds:
- all network pmds located under drivers/net
- librte_net
- libtre_ether
- librte_ip_frag
- librte_pdump
- librte_port
* crypto-pmds:
- all crypto pmds located under drivers/crypto
- librte_cryptodev
* eal:
- librte_eal
* core:
- librte_cfgfile
- librte_cmdline
- librte_compat
- librte_kvargs
- librte_kni
- librte_compat
* misc:
- librte_acl
- librte_distributor
- librte_hash
- librte_jobstats
- librte_lpm
- librte_meter
- librte_pipeline
- librte_power
- librte_reorder
- librte_ring
- librte_sched
- librte_table
- librte_timer
- librte_vhost
Thats just a rough stab mind, but perhaps it would get the ball rolling. I'd be
willing to take maintainership of one of these subtrees if there is consensus
around my doing so.
>
> >
> > > > Each of those subdivisions should have their own list to communicate
> > > > on, and each should have a tree that integrates patches for their
> > > > own subsystem, and they should on a regular cycle send pull requests
> > > > to Thomas.
> > >
> > > Yes I think it is now a good idea to split the mailing list traffic,
> > > at least for netdev and cryptodev.
>
> I'd prefer not to have split dev lists, for now at least. We can reevaluate that again in a few months though.
>
So, if thats a deal breaker, we can talk about it, but I need to point out that
multiple subtrees with a single development list creates a wide range of
problems that will later give the perception that creating subtrees in the first
place wasn't worthwhile. If you have multiple maintainers working on multiple
subtrees in parallel with the expectation that we will all merge to thomas's
tree during the devel cycle, then those maintainers absolutely must have a way
to identify which patches are destined for their tree, and which can be ignored.
To not have that invites duplicated work at best (as multiple maintainers apply
the same patch), and serious merge breakage at worst (as multiple massaged patch
applications conflict during the merge). We can manage it with tagged
submissions, but relying on developers to use those tags consistently is very
hit or miss. It would be much more desireable to split development into lists
that correspond with the subtrees, so that posting to a list has a 1:1
correlation with the tree it was intended to be merged to. As I noted
previously, subscribing to several lists and treating them as a single inbox is
a trivial operation.
>
> > >
> >
> > > > B) Designate alternates to serve as backups for the maintainer when
> > > > they are unavailable. This provides high-availablility, and sounds
> > > > very much like your proposal, but in the interests of clarity, there
> > > > is still a single maintainer at any one time, it just may change to
> > > > ensure the continued merging of patches, if the primary maintainer
> > isn't available.
> > > > Ideally however, those backup alternates arent needed, because most
> > > > of the primary maintainers work in merging pull requests, which are
> > > > done based on the trust of the submaintainer, and done during a very
> > > > limited window of time. This also partially addreses multi-vendor
> > > > fairness if your subtree maintainers come from multiple participating
> > companies.
>
>
> +1 on this apart from the limited merge window (for reasons similar to Thomas).
>
Yeah, apparently I misspoke here. I never meant to indicate that the merge
window could be smaller/shorter/what have you. I only meant to indicate that,
if Thomas's primary role is merging pull requests from subtrees, then his job
takes much less time than it otherwise would if he's applying individual
patches, which in turn makes the need for backups (due to
vacation/illness/whatever), less of a pressing issue.
> Should we have a call for volunteers for backup, on master and the sub-trees, followed by a simple +1 from community members to endorse them?
>
I would think that we could merge the roles into one. That is to say, if we
assume that sub-tree maintainers are implicitly trusted by Thomas to send good
pul requests, then they also would make good backups for him. i.e. if you
volunteer to be a sub-tree maintainer, you get added to the list of master tree
backups, in some order to be determined by Thomas should he need to take an
absence for any reason. Just my $0.02 on that, we can organize that however you
would like.
I'd be willing to be a subtree maintainer. I'll handle any tree, once we have
consensus on the segmentation, and list relationships.
Regards
Neil
>
> John
>
>
>
^ permalink raw reply
* Re: Proposal for a new Committer model
From: Ferruh Yigit @ 2016-11-23 14:01 UTC (permalink / raw)
To: Neil Horman; +Cc: Thomas Monjalon, dev, Mcnamara, John
In-Reply-To: <20161123134845.GA6961@hmsreliant.think-freely.org>
On 11/23/2016 1:48 PM, Neil Horman wrote:
> On Tue, Nov 22, 2016 at 08:56:23PM +0000, Ferruh Yigit wrote:
>> On 11/22/2016 7:52 PM, Neil Horman wrote:
>>> On Mon, Nov 21, 2016 at 09:52:41AM +0100, Thomas Monjalon wrote:
>>>> 2016-11-18 13:09, Neil Horman:
>>>>> A) Further promote subtree maintainership. This was a conversation that I
>>>>> proposed some time ago, but my proposed granularity was discarded in favor
>>>>> of something that hasn't worked as well (in my opinion). That is to say a
>>>>> few driver pmds (i40e and fm10k come to mind) have their own tree that
>>>>> send pull requests to Thomas.
>>>>
>>>> Yes we tried this fine granularity and stated that it was not working well.
>>>> We are now using the bigger granularity that you describe below.
>>>>
>>> Ok, thats good, but that must be _very_ new. Looking at your git tree, I see no
>>> merge commits. How are you pulling from those subtrees?
>>
>> next-net tree is active for last three releases.
>>
> What!? What is the purpose of holding patches in a subtree for multiple
> releases?
:) Of course not holding them in the sub-tree.
Briefly, process is:
- sub-tree gets patches during merge window
- sub-tree first merged into main tree in -rc1 and later in -r2
next-net tree is actively in use for last three releases, and driver/net
patches delegated to this tree. You can see different commiters in main
tree.
> If a given changeset isn't ready for merge to Thomas tree the people
> working on it should clone the subtree to some place they can all collaborate on
> it. Once it goes into a subtree there needs to be a defined workflow to get it
> into the canonical tree that Thomas maintains on a regular, short time frame.
> to do less is to confuse the process for everyone involved, and slow people
> down, rather than accelerate their work.
>
>> I guess following is the first commit to the sub-tree:
>> http://dpdk.org/ml/archives/dev/2016-February/032580.html
>>
>> sub-trees rebase on top of main tree regularly, that is why there is no
>> merge commit.
>>
> I'm not asking about merge commits in the sub-tree, I'm asking about merge
> commits in thomas's tree.
Same, talking about Thomas' tree.
> There should be a merge commit every time he pulls
> from a sub-tree (unless its a fast-forward I think, but with multiple subtrees
> and commits going to thomas directly, that should never really happen).
That is what happening. Since sub-tree's rebase on top of main tree,
when Thomas merges, it is just plain fast-forward. So it is allowed to
re-write to history in sub-trees.
> I don't
> see any Merge commits in the master branch of his tree, so I'm left wondering
> what mechanic is being used to migrate patches from net-next or crypo-next to
> his tree. Thomas, can you comment here?
>
>>>
>>>
>>>>> We should be sharding that at a much higher
>>>>> granularity and using it much more consistently. That is to say, that we
>>>>> should have a maintainer for all the ethernet pmds, and another for the
>>>>> crypto pmds, another for the core eal layer, another for misc libraries
>>>>> that have low patch volumes, etc.
>>>>
>>>> Yes we could open a tree for EAL and another one for the core libraries.
>>>>
>>> That could be worthwhile. Lets see how the net and crypto subtrees work out
>>> (assuming again that these trees are newly founded)
>>>
>>>
>>>>> Each of those subdivisions should have
>>>>> their own list to communicate on, and each should have a tree that
>>>>> integrates patches for their own subsystem, and they should on a regular
>>>>> cycle send pull requests to Thomas.
>>>>
>>>> Yes I think it is now a good idea to split the mailing list traffic,
>>>> at least for netdev and cryptodev.
>>>>
>>> Agreed, that serves two purposes, it lowers the volume for people with a
>>> specific interest (i.e. its a rudimentary filter), and it avoids confusion
>>> between you and the subtree maintainer (that is to say, you don't have to even
>>> consider pulling patches that go to the crypo and net lists, you just have to
>>> trust that they pull those patches in and send you appropriate pull requests).
>>
>> I still find single mail list more useful.
> Why? If you have interest in all the subsystems of a project, then its a small
> amount of overhead to subscribe to a set of mailing lists and dump them all to a
> single mail folder. If you only have interest in a subset, its much more
> difficult to filter them out, given that we have a plethora of prefix tags for
> patches to define subsystems that aren't always used consistently. Given that
> this thread is here because we've identified the patch volume as a problem, it
> seems fragmenting the list is the better solution.
>
>> Also with current process, after -rc2 release, patches directly merged
>> into main tree instead of sub-trees...
>>
> Thats fine, at that point, if everything works properly, Thomas should only be
> getting low volume patch flow for stabilization/bug fixing. If thats not the
> case, then perhaps we need to consider doing extra merges from the subtrees
> later in the cycle.
>
> Neil
>
^ permalink raw reply
* Re: Proposal for a new Committer model
From: Neil Horman @ 2016-11-23 13:48 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Thomas Monjalon, dev, Mcnamara, John
In-Reply-To: <52ed2fa2-da41-1301-2d56-0fec05b79ce5@intel.com>
On Tue, Nov 22, 2016 at 08:56:23PM +0000, Ferruh Yigit wrote:
> On 11/22/2016 7:52 PM, Neil Horman wrote:
> > On Mon, Nov 21, 2016 at 09:52:41AM +0100, Thomas Monjalon wrote:
> >> 2016-11-18 13:09, Neil Horman:
> >>> A) Further promote subtree maintainership. This was a conversation that I
> >>> proposed some time ago, but my proposed granularity was discarded in favor
> >>> of something that hasn't worked as well (in my opinion). That is to say a
> >>> few driver pmds (i40e and fm10k come to mind) have their own tree that
> >>> send pull requests to Thomas.
> >>
> >> Yes we tried this fine granularity and stated that it was not working well.
> >> We are now using the bigger granularity that you describe below.
> >>
> > Ok, thats good, but that must be _very_ new. Looking at your git tree, I see no
> > merge commits. How are you pulling from those subtrees?
>
> next-net tree is active for last three releases.
>
What!? What is the purpose of holding patches in a subtree for multiple
releases? If a given changeset isn't ready for merge to Thomas tree the people
working on it should clone the subtree to some place they can all collaborate on
it. Once it goes into a subtree there needs to be a defined workflow to get it
into the canonical tree that Thomas maintains on a regular, short time frame.
to do less is to confuse the process for everyone involved, and slow people
down, rather than accelerate their work.
> I guess following is the first commit to the sub-tree:
> http://dpdk.org/ml/archives/dev/2016-February/032580.html
>
> sub-trees rebase on top of main tree regularly, that is why there is no
> merge commit.
>
I'm not asking about merge commits in the sub-tree, I'm asking about merge
commits in thomas's tree. There should be a merge commit every time he pulls
from a sub-tree (unless its a fast-forward I think, but with multiple subtrees
and commits going to thomas directly, that should never really happen). I don't
see any Merge commits in the master branch of his tree, so I'm left wondering
what mechanic is being used to migrate patches from net-next or crypo-next to
his tree. Thomas, can you comment here?
> >
> >
> >>> We should be sharding that at a much higher
> >>> granularity and using it much more consistently. That is to say, that we
> >>> should have a maintainer for all the ethernet pmds, and another for the
> >>> crypto pmds, another for the core eal layer, another for misc libraries
> >>> that have low patch volumes, etc.
> >>
> >> Yes we could open a tree for EAL and another one for the core libraries.
> >>
> > That could be worthwhile. Lets see how the net and crypto subtrees work out
> > (assuming again that these trees are newly founded)
> >
> >
> >>> Each of those subdivisions should have
> >>> their own list to communicate on, and each should have a tree that
> >>> integrates patches for their own subsystem, and they should on a regular
> >>> cycle send pull requests to Thomas.
> >>
> >> Yes I think it is now a good idea to split the mailing list traffic,
> >> at least for netdev and cryptodev.
> >>
> > Agreed, that serves two purposes, it lowers the volume for people with a
> > specific interest (i.e. its a rudimentary filter), and it avoids confusion
> > between you and the subtree maintainer (that is to say, you don't have to even
> > consider pulling patches that go to the crypo and net lists, you just have to
> > trust that they pull those patches in and send you appropriate pull requests).
>
> I still find single mail list more useful.
Why? If you have interest in all the subsystems of a project, then its a small
amount of overhead to subscribe to a set of mailing lists and dump them all to a
single mail folder. If you only have interest in a subset, its much more
difficult to filter them out, given that we have a plethora of prefix tags for
patches to define subsystems that aren't always used consistently. Given that
this thread is here because we've identified the patch volume as a problem, it
seems fragmenting the list is the better solution.
> Also with current process, after -rc2 release, patches directly merged
> into main tree instead of sub-trees...
>
Thats fine, at that point, if everything works properly, Thomas should only be
getting low volume patch flow for stabilization/bug fixing. If thats not the
case, then perhaps we need to consider doing extra merges from the subtrees
later in the cycle.
Neil
^ 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