* Re: [PATCH] Scheduler: add driver for scheduler crypto pmd
From: Declan Doherty @ 2016-12-07 12:42 UTC (permalink / raw)
To: Neil Horman; +Cc: Bruce Richardson, Thomas Monjalon, Fan Zhang, dev
In-Reply-To: <20161205151209.GA4232@hmsreliant.think-freely.org>
On 05/12/16 15:12, Neil Horman wrote:
> On Fri, Dec 02, 2016 at 04:22:16PM +0000, Declan Doherty wrote:
>> On 02/12/16 14:57, Bruce Richardson wrote:
>>> On Fri, Dec 02, 2016 at 03:31:24PM +0100, Thomas Monjalon wrote:
>>>> 2016-12-02 14:15, Fan Zhang:
>>>>> This patch provides the initial implementation of the scheduler poll mode
>>>>> driver using DPDK cryptodev framework.
>>>>>
>>>>> Scheduler PMD is used to schedule and enqueue the crypto ops to the
>>>>> hardware and/or software crypto devices attached to it (slaves). The
>>>>> dequeue operation from the slave(s), and the possible dequeued crypto op
>>>>> reordering, are then carried out by the scheduler.
>>>>>
>>>>> The scheduler PMD can be used to fill the throughput gap between the
>>>>> physical core and the existing cryptodevs to increase the overall
>>>>> performance. For example, if a physical core has higher crypto op
>>>>> processing rate than a cryptodev, the scheduler PMD can be introduced to
>>>>> attach more than one cryptodevs.
>>>>>
>>>>> This initial implementation is limited to supporting the following
>>>>> scheduling modes:
>>>>>
>>>>> - CRYPTO_SCHED_SW_ROUND_ROBIN_MODE (round robin amongst attached software
>>>>> slave cryptodevs, to set this mode, the scheduler should have been
>>>>> attached 1 or more software cryptodevs.
>>>>>
>>>>> - CRYPTO_SCHED_HW_ROUND_ROBIN_MODE (round robin amongst attached hardware
>>>>> slave cryptodevs (QAT), to set this mode, the scheduler should have
>>>>> been attached 1 or more QATs.
>>>>
>>>> Could it be implemented on top of the eventdev API?
>>>>
>>> Not really. The eventdev API is for different types of scheduling
>>> between multiple sources that are all polling for packets, compared to
>>> this, which is more analgous - as I understand it - to the bonding PMD
>>> for ethdev.
>>>
>>> To make something like this work with an eventdev API you would need to
>>> use one of the following models:
>>> * have worker cores for offloading packets to the different crypto
>>> blocks pulling from the eventdev APIs. This would make it difficult to
>>> do any "smart" scheduling of crypto operations between the blocks,
>>> e.g. that one crypto instance may be better at certain types of
>>> operations than another.
>>> * move the logic in this driver into an existing eventdev instance,
>>> which uses the eventdev api rather than the crypto APIs and so has an
>>> extra level of "structure abstraction" that has to be worked though.
>>> It's just not really a good fit.
>>>
>>> So for this workload, I believe the pseudo-cryptodev instance is the
>>> best way to go.
>>>
>>> /Bruce
>>>
>>
>>
>> As Bruce says this is much more analogous to the ethdev bonding driver, the
>> main idea is to allow different crypto op scheduling mechanisms to be
>> defined transparently to an application. This could be load-balancing across
>> multiple hw crypto devices, or having a software crypto device to act as a
>> backup device for a hw accelerator if it becomes oversubscribed. I think the
>> main advantage of a crypto-scheduler approach means that the data path of
>> the application doesn't need to have any knowledge that scheduling is
>> happening at all, it is just using a different crypto device id, which is
>> then manages the distribution of crypto work.
>>
>>
>>
> This is a good deal like the bonding pmd, and so from a certain standpoint it
> makes sense to do this, but whereas the bonding pmd is meant to create a single
> path to a logical network over several physical networks, this pmd really only
> focuses on maximizing througput, and for that we already have tools. As Thomas
> mentions, there is the eventdev library, but from my view the distributor
> library already fits this bill. It already is a basic framework to process
> mbufs in parallel according to whatever policy you want to implement, which
> sounds like exactly what the goal of this pmd is.
>
> Neil
>
>
Hey Neil,
this is actually intended to act and look a good deal like the ethernet
bonding device but to handling the crypto scheduling use cases.
For example, take the case where multiple hw accelerators may be
available. We want to provide user applications with a mechanism to
transparently balance work across all devices without having to manage
the load balancing details or the guaranteeing of ordering of the
processed ops on the dequeue_burst side. In this case the application
would just use the crypto dev_id of the scheduler and it would look
after balancing the workload across the available hw accelerators.
+-------------------+
| Crypto Sch PMD |
| |
| ORDERING / RR SCH |
+-------------------+
^ ^ ^
| | |
+-+ | +-------------------------------+
| +---------------+ |
| | |
V V V
+---------------+ +---------------+ +---------------+
| Crypto HW PMD | | Crypto HW PMD | | Crypto HW PMD |
+---------------+ +---------------+ +---------------+
Another use case we hope to support is migration of processing from one
device to another where a hw and sw crypto pmd can be bound to the same
crypto scheduler and the crypto processing could be transparently
migrated from the hw to sw pmd. This would allow for hw accelerators to
be hot-plugged attached/detached in a Guess VM
+----------------+
| Crypto Sch PMD |
| |
| MIGRATION SCH |
+----------------+
| |
| +-----------------+
| |
V V
+---------------+ +---------------+
| Crypto HW PMD | | Crypto SW PMD |
| (Active) | | (Inactive) |
+---------------+ +---------------+
The main point is that isn't envisaged as just a mechanism for
scheduling crypto work loads across multiple cores, but a framework for
allowing different scheduling mechanisms to be introduced, to handle
different crypto scheduling problems, and done so in a way which is
completely transparent to the data path of an application. Like the eth
bonding driver we want to support creating the crypto scheduler from EAL
options, which allow specification of the scheduling mode and the crypto
pmds which are to be bound to that crypto scheduler.
^ permalink raw reply
* [PATCH v2] vhost: allow for many vhost user ports
From: Jan Wickbom @ 2016-12-07 12:46 UTC (permalink / raw)
To: yuanhan.liu; +Cc: dev, patrik.r.andersson, Jan Wickbom
In-Reply-To: <1480606010-6132-1-git-send-email-jan.wickbom@ericsson.com>
Currently select() is used to monitor file descriptors for vhostuser
ports. This limits the number of ports possible to create since the
fd number is used as index in the fd_set and we have seen fds > 1023.
This patch changes select() to poll(). This way we can keep an
packed (pollfd) array for the fds, e.g. as many fds as the size of
the array.
Also see:
http://dpdk.org/ml/archives/dev/2016-April/037024.html
Signed-off-by: Jan Wickbom <jan.wickbom@ericsson.com>
Reported-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
---
v2:
* removed unnecessary casts
* static array replacing allocated memory
lib/librte_vhost/fd_man.c | 142 +++++++++++++++++++++++++++-------------------
lib/librte_vhost/fd_man.h | 2 +-
2 files changed, 85 insertions(+), 59 deletions(-)
diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 2d3eeb7..3c743e3 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -35,16 +35,43 @@
#include <stdio.h>
#include <stdlib.h>
#include <sys/socket.h>
-#include <sys/select.h>
#include <sys/time.h>
#include <sys/types.h>
+#include <poll.h>
#include <unistd.h>
+#include <string.h>
#include <rte_common.h>
+#include <rte_malloc.h>
#include <rte_log.h>
#include "fd_man.h"
+#define FDPOLLERR (POLLERR | POLLHUP | POLLNVAL)
+
+
+static struct pollfd rwfds[MAX_FDS];
+
+/**
+ * Adjusts the highest index populated in the array of fds
+ * @return
+ * The new size of fdset.
+ */
+static int
+fdset_shrink(struct fdset *pfdset)
+{
+ int idx;
+
+ for (idx = pfdset->num - 1;
+ idx >= 0 && pfdset->fd[idx].fd == -1;
+ idx--)
+ ;
+
+ pfdset->num = idx + 1;
+
+ return pfdset->num;
+}
+
/**
* Returns the index in the fdset for a given fd.
* If fd is -1, it means to search for a free entry.
@@ -56,72 +83,62 @@
{
int i;
- if (pfdset == NULL)
- return -1;
-
- for (i = 0; i < MAX_FDS && pfdset->fd[i].fd != fd; i++)
+ for (i = 0; i < pfdset->num && pfdset->fd[i].fd != fd; i++)
;
- return i == MAX_FDS ? -1 : i;
+ return i == pfdset->num ? -1 : i;
}
static int
fdset_find_free_slot(struct fdset *pfdset)
{
- return fdset_find_fd(pfdset, -1);
+ if (pfdset->num < MAX_FDS)
+ return pfdset->num;
+ else
+ return fdset_find_fd(pfdset, -1);
}
-static int
-fdset_add_fd(struct fdset *pfdset, int idx, int fd,
+static void
+fdset_add_fd(struct fdset *pfdset, int idx, int fd,
fd_cb rcb, fd_cb wcb, void *dat)
{
struct fdentry *pfdentry;
- if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
- return -1;
-
pfdentry = &pfdset->fd[idx];
pfdentry->fd = fd;
pfdentry->rcb = rcb;
pfdentry->wcb = wcb;
pfdentry->dat = dat;
-
- return 0;
}
/**
- * Fill the read/write fd_set with the fds in the fdset.
+ * Compact the fdset and fill the read/write fds with the fds in the fdset.
* @return
- * the maximum fds filled in the read/write fd_set.
+ * the number of fds filled in the read/write fds.
*/
static int
-fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
+fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
{
struct fdentry *pfdentry;
- int i, maxfds = -1;
- int num = MAX_FDS;
-
- if (pfdset == NULL)
- return -1;
+ int i;
+ int num;
- for (i = 0; i < num; i++) {
+ for (i = 0, num = pfdset->num; i < num; i++) {
pfdentry = &pfdset->fd[i];
- if (pfdentry->fd != -1) {
- int added = 0;
- if (pfdentry->rcb && rfset) {
- FD_SET(pfdentry->fd, rfset);
- added = 1;
- }
- if (pfdentry->wcb && wfset) {
- FD_SET(pfdentry->fd, wfset);
- added = 1;
- }
- if (added)
- maxfds = pfdentry->fd < maxfds ?
- maxfds : pfdentry->fd;
+
+ if (pfdentry->fd < 0) {
+ /* Hole in the list. Move the last one here */
+
+ *pfdentry = pfdset->fd[num - 1];
+ pfdset->fd[num - 1].fd = -1;
+ num = fdset_shrink(pfdset);
}
+ rwfds[i].fd = pfdentry->fd;
+ rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
+ rwfds[i].events |= pfdentry->wcb ? POLLOUT : 0;
}
- return maxfds;
+
+ return i;
}
void
@@ -132,6 +149,8 @@
if (pfdset == NULL)
return;
+ pthread_mutex_init(&pfdset->fd_mutex, NULL);
+
for (i = 0; i < MAX_FDS; i++) {
pfdset->fd[i].fd = -1;
pfdset->fd[i].dat = NULL;
@@ -152,14 +171,15 @@
pthread_mutex_lock(&pfdset->fd_mutex);
- /* Find a free slot in the list. */
i = fdset_find_free_slot(pfdset);
- if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
+ if (i == -1) {
pthread_mutex_unlock(&pfdset->fd_mutex);
return -2;
}
- pfdset->num++;
+ fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
+ if (i == pfdset->num)
+ pfdset->num++;
pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -189,7 +209,7 @@
pfdset->fd[i].fd = -1;
pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
pfdset->fd[i].dat = NULL;
- pfdset->num--;
+ fdset_shrink(pfdset);
i = -1;
}
pthread_mutex_unlock(&pfdset->fd_mutex);
@@ -211,12 +231,12 @@
pfdset->fd[index].fd = -1;
pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
- pfdset->fd[index].dat = NULL;
- pfdset->num--;
+ fdset_shrink(pfdset);
pthread_mutex_unlock(&pfdset->fd_mutex);
}
+
/**
* This functions runs in infinite blocking loop until there is no fd in
* pfdset. It calls corresponding r/w handler if there is event on the fd.
@@ -229,44 +249,47 @@
void
fdset_event_dispatch(struct fdset *pfdset)
{
- fd_set rfds, wfds;
- int i, maxfds;
+ int i;
struct fdentry *pfdentry;
- int num = MAX_FDS;
+ int numfds;
fd_cb rcb, wcb;
void *dat;
int fd;
int remove1, remove2;
int ret;
+ int handled;
if (pfdset == NULL)
return;
+ memset(rwfds, 0, sizeof(rwfds));
+
while (1) {
- struct timeval tv;
- tv.tv_sec = 1;
- tv.tv_usec = 0;
- FD_ZERO(&rfds);
- FD_ZERO(&wfds);
pthread_mutex_lock(&pfdset->fd_mutex);
- maxfds = fdset_fill(&rfds, &wfds, pfdset);
+ numfds = fdset_fill(rwfds, pfdset);
pthread_mutex_unlock(&pfdset->fd_mutex);
/*
- * When select is blocked, other threads might unregister
+ * When poll is blocked, other threads might unregister
* listenfds from and register new listenfds into fdset.
- * When select returns, the entries for listenfds in the fdset
+ * When poll returns, the entries for listenfds in the fdset
* might have been updated. It is ok if there is unwanted call
* for new listenfds.
*/
- ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
+ ret = poll(rwfds, numfds, 1000 /* millisecs */);
+
if (ret <= 0)
continue;
- for (i = 0; i < num; i++) {
+ for (i = handled = 0; i < numfds && handled < ret; i++) {
+ if (!rwfds[i].revents)
+ continue;
+
+ handled++;
remove1 = remove2 = 0;
+
pthread_mutex_lock(&pfdset->fd_mutex);
pfdentry = &pfdset->fd[i];
fd = pfdentry->fd;
@@ -275,9 +298,12 @@
dat = pfdentry->dat;
pfdentry->busy = 1;
pthread_mutex_unlock(&pfdset->fd_mutex);
- if (fd >= 0 && FD_ISSET(fd, &rfds) && rcb)
+
+ if (fd >= 0 && rcb &&
+ rwfds[i].revents & (POLLIN | FDPOLLERR))
rcb(fd, dat, &remove1);
- if (fd >= 0 && FD_ISSET(fd, &wfds) && wcb)
+ if (fd >= 0 && wcb &&
+ rwfds[i].revents & (POLLOUT | FDPOLLERR))
wcb(fd, dat, &remove2);
pfdentry->busy = 0;
/*
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index bd66ed1..b5ba688 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -51,7 +51,7 @@ struct fdentry {
struct fdset {
struct fdentry fd[MAX_FDS];
pthread_mutex_t fd_mutex;
- int num; /* current fd number of this fdset */
+ int num; /* highest index occupied in fd array + 1 */
};
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v2 02/32] net/i40e: add callback to user on VF to PF mbox msg
From: Ferruh Yigit @ 2016-12-07 12:44 UTC (permalink / raw)
To: Wenzhuo Lu, dev
In-Reply-To: <1481081535-37448-3-git-send-email-wenzhuo.lu@intel.com>
On 12/7/2016 3:31 AM, Wenzhuo Lu wrote:
> The callback asks the user application if it is allowed to
> perform the mailbox messages.
>
> If the return value from user is RTE_PMD_I40E_MB_EVENT_PROCEED
> then continue. If ACK or NACK, do nothing and send
> not_supported to VF.
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
> drivers/net/i40e/i40e_pf.c | 230 ++++++++++++++++++++++++++++++++++------
> drivers/net/i40e/rte_pmd_i40e.h | 21 ++++
> 2 files changed, 216 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
> index f70712b..8b8a14f 100644
> --- a/drivers/net/i40e/i40e_pf.c
> +++ b/drivers/net/i40e/i40e_pf.c
> @@ -55,6 +55,7 @@
> #include "i40e_ethdev.h"
> #include "i40e_rxtx.h"
> #include "i40e_pf.h"
> +#include "rte_pmd_i40e.h"
>
> #define I40E_CFG_CRCSTRIP_DEFAULT 1
>
> @@ -272,14 +273,23 @@
> }
>
> static void
> -i40e_pf_host_process_cmd_version(struct i40e_pf_vf *vf)
> +i40e_pf_host_process_cmd_version(struct i40e_pf_vf *vf, bool b_op)
> {
> struct i40e_virtchnl_version_info info;
>
> info.major = I40E_DPDK_VERSION_MAJOR;
> info.minor = I40E_DPDK_VERSION_MINOR;
> - i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_VERSION,
> - I40E_SUCCESS, (uint8_t *)&info, sizeof(info));
> +
> + if (b_op)
> + i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_VERSION,
> + I40E_SUCCESS,
> + (uint8_t *)&info,
> + sizeof(info));
> + else
> + i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_VERSION,
> + I40E_NOT_SUPPORTED,
> + (uint8_t *)&info,
> + sizeof(info));
Even I40E_NOT_SUPPORTED returned to the VF, it seems VF is ignoring this
value, is it something should be fixed as part of this patch?
This path is a little complex, I may be missing something but stack
trace is:
VF:
i40evf_check_api_version()
i40evf_execute_vf_cmd(vfd_cmd_info arg)
i40e_aq_send_msg_to_pf(arg->op, retval, arg->in_msg)
desc <- op, retval
msg_in <- arg_in_msg
i40e_asq_send_command(desc, msg_in)
PF:
i40e_pf_host_handle_vf_msg(op, msg_in)
i40e_pf_host_process_cmd_version()
i40e_pf_host_send_msg_to_vf(op, retval, msg_out)
i40e_aq_send_msg_to_vf(op, retval, msg_out)
desc <- op, retval
i40e_asq_send_command(desc, msg_out)
VF:
data <- arg->out_xxx
i40evf_read_pfmsg(data)
event <- data->out_msg
op <-
retval <-
i40e_clean_arq_element(event)
event->desc <- desc
event->msg <- msg_out
data->result = retval <----------------
return 0;
ver = arg->out_msg
return 0;
So, as far as I can see I40E_NOT_SUPPORTED is somewhere in the stack but
not reached to the final VF function, is this OK?
> }
>
> static int
> @@ -292,13 +302,20 @@
> }
>
> static int
> -i40e_pf_host_process_cmd_get_vf_resource(struct i40e_pf_vf *vf)
> +i40e_pf_host_process_cmd_get_vf_resource(struct i40e_pf_vf *vf, bool b_op)
> {
> struct i40e_virtchnl_vf_resource *vf_res = NULL;
> struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf);
> uint32_t len = 0;
> int ret = I40E_SUCCESS;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(vf,
> + I40E_VIRTCHNL_OP_GET_VF_RESOURCES,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> /* only have 1 VSI by default */
> len = sizeof(struct i40e_virtchnl_vf_resource) +
> I40E_DEFAULT_VF_VSI_NUM *
> @@ -423,7 +440,8 @@
> static int
> i40e_pf_host_process_cmd_config_vsi_queues(struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf);
> struct i40e_vsi *vsi = vf->vsi;
> @@ -432,6 +450,13 @@
> struct i40e_virtchnl_queue_pair_info *vc_qpi;
> int i, ret = I40E_SUCCESS;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(vf,
> + I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (!msg || vc_vqci->num_queue_pairs > vsi->nb_qps ||
> vc_vqci->num_queue_pairs > I40E_MAX_VSI_QP ||
> msglen < I40E_VIRTCHNL_CONFIG_VSI_QUEUES_SIZE(vc_vqci,
> @@ -482,7 +507,8 @@
> static int
> i40e_pf_host_process_cmd_config_vsi_queues_ext(struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf);
> struct i40e_vsi *vsi = vf->vsi;
> @@ -491,6 +517,14 @@
> struct i40e_virtchnl_queue_pair_ext_info *vc_qpei;
> int i, ret = I40E_SUCCESS;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EXT,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (!msg || vc_vqcei->num_queue_pairs > vsi->nb_qps ||
> vc_vqcei->num_queue_pairs > I40E_MAX_VSI_QP ||
> msglen < I40E_VIRTCHNL_CONFIG_VSI_QUEUES_SIZE(vc_vqcei,
> @@ -539,12 +573,21 @@
>
> static int
> i40e_pf_host_process_cmd_config_irq_map(struct i40e_pf_vf *vf,
> - uint8_t *msg, uint16_t msglen)
> + uint8_t *msg, uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_irq_map_info *irqmap =
> (struct i40e_virtchnl_irq_map_info *)msg;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_CONFIG_IRQ_MAP,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (msg == NULL || msglen < sizeof(struct i40e_virtchnl_irq_map_info)) {
> PMD_DRV_LOG(ERR, "buffer too short");
> ret = I40E_ERR_PARAM;
> @@ -646,12 +689,21 @@
> static int
> i40e_pf_host_process_cmd_disable_queues(struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_queue_select *q_sel =
> (struct i40e_virtchnl_queue_select *)msg;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_DISABLE_QUEUES,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (msg == NULL || msglen != sizeof(*q_sel)) {
> ret = I40E_ERR_PARAM;
> goto send_msg;
> @@ -669,7 +721,8 @@
> static int
> i40e_pf_host_process_cmd_add_ether_address(struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_ether_addr_list *addr_list =
> @@ -678,6 +731,14 @@
> int i;
> struct ether_addr *mac;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_ADD_ETHER_ADDRESS,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> memset(&filter, 0 , sizeof(struct i40e_mac_filter_info));
>
> if (msg == NULL || msglen <= sizeof(*addr_list)) {
> @@ -707,7 +768,8 @@
> static int
> i40e_pf_host_process_cmd_del_ether_address(struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_ether_addr_list *addr_list =
> @@ -715,6 +777,14 @@
> int i;
> struct ether_addr *mac;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_DEL_ETHER_ADDRESS,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (msg == NULL || msglen <= sizeof(*addr_list)) {
> PMD_DRV_LOG(ERR, "delete_ether_address argument too short");
> ret = I40E_ERR_PARAM;
> @@ -739,7 +809,8 @@
>
> static int
> i40e_pf_host_process_cmd_add_vlan(struct i40e_pf_vf *vf,
> - uint8_t *msg, uint16_t msglen)
> + uint8_t *msg, uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_vlan_filter_list *vlan_filter_list =
> @@ -747,6 +818,14 @@
> int i;
> uint16_t *vid;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_ADD_VLAN,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (msg == NULL || msglen <= sizeof(*vlan_filter_list)) {
> PMD_DRV_LOG(ERR, "add_vlan argument too short");
> ret = I40E_ERR_PARAM;
> @@ -771,7 +850,8 @@
> static int
> i40e_pf_host_process_cmd_del_vlan(struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_vlan_filter_list *vlan_filter_list =
> @@ -779,6 +859,14 @@
> int i;
> uint16_t *vid;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_DEL_VLAN,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (msg == NULL || msglen <= sizeof(*vlan_filter_list)) {
> PMD_DRV_LOG(ERR, "delete_vlan argument too short");
> ret = I40E_ERR_PARAM;
> @@ -803,7 +891,8 @@
> i40e_pf_host_process_cmd_config_promisc_mode(
> struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_promisc_info *promisc =
> @@ -811,6 +900,14 @@
> struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf);
> bool unicast = FALSE, multicast = FALSE;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_CONFIG_PROMISCUOUS_MODE,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (msg == NULL || msglen != sizeof(*promisc)) {
> ret = I40E_ERR_PARAM;
> goto send_msg;
> @@ -836,13 +933,20 @@
> }
>
> static int
> -i40e_pf_host_process_cmd_get_stats(struct i40e_pf_vf *vf)
> +i40e_pf_host_process_cmd_get_stats(struct i40e_pf_vf *vf, bool b_op)
> {
> i40e_update_vsi_stats(vf->vsi);
>
> - i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_GET_STATS,
> - I40E_SUCCESS, (uint8_t *)&vf->vsi->eth_stats,
> - sizeof(vf->vsi->eth_stats));
> + if (b_op)
> + i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_GET_STATS,
> + I40E_SUCCESS,
> + (uint8_t *)&vf->vsi->eth_stats,
> + sizeof(vf->vsi->eth_stats));
> + else
> + i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_GET_STATS,
> + I40E_NOT_SUPPORTED,
> + (uint8_t *)&vf->vsi->eth_stats,
> + sizeof(vf->vsi->eth_stats));
>
> return I40E_SUCCESS;
> }
> @@ -851,12 +955,21 @@
> i40e_pf_host_process_cmd_cfg_vlan_offload(
> struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_vlan_offload_info *offload =
> (struct i40e_virtchnl_vlan_offload_info *)msg;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_CFG_VLAN_OFFLOAD,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (msg == NULL || msglen != sizeof(*offload)) {
> ret = I40E_ERR_PARAM;
> goto send_msg;
> @@ -877,12 +990,21 @@
> static int
> i40e_pf_host_process_cmd_cfg_pvid(struct i40e_pf_vf *vf,
> uint8_t *msg,
> - uint16_t msglen)
> + uint16_t msglen,
> + bool b_op)
> {
> int ret = I40E_SUCCESS;
> struct i40e_virtchnl_pvid_info *tpid_info =
> (struct i40e_virtchnl_pvid_info *)msg;
>
> + if (!b_op) {
> + i40e_pf_host_send_msg_to_vf(
> + vf,
> + I40E_VIRTCHNL_OP_CFG_VLAN_PVID,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + return ret;
> + }
> +
> if (msg == NULL || msglen != sizeof(*tpid_info)) {
> ret = I40E_ERR_PARAM;
> goto send_msg;
> @@ -923,6 +1045,8 @@
> struct i40e_pf_vf *vf;
> /* AdminQ will pass absolute VF id, transfer to internal vf id */
> uint16_t vf_id = abs_vf_id - hw->func_caps.vf_base_id;
> + struct rte_pmd_i40e_mb_event_param cb_param;
> + bool b_op = TRUE;
>
> if (vf_id > pf->vf_num - 1 || !pf->vfs) {
> PMD_DRV_LOG(ERR, "invalid argument");
> @@ -937,10 +1061,35 @@
> return;
> }
>
> + /**
> + * initialise structure to send to user application
> + * will return response from user in retval field
> + */
> + cb_param.retval = RTE_PMD_I40E_MB_EVENT_PROCEED;
> + cb_param.vfid = vf_id;
> + cb_param.msg_type = opcode;
> + cb_param.msg = (void *)msg;
> + cb_param.msglen = msglen;
> +
> + /**
> + * Ask user application if we're allowed to perform those functions.
> + * If we get cb_param.retval == RTE_PMD_I40E_MB_EVENT_PROCEED,
> + * then business as usual.
> + * If RTE_PMD_I40E_MB_EVENT_NOOP_ACK or RTE_PMD_I40E_MB_EVENT_NOOP_NACK,
> + * do nothing and send not_supported to VF. As PF must send a response
> + * to VF and ACK/NACK is not defined.
> + */
> + _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_VF_MBOX, &cb_param);
> + if (cb_param.retval != RTE_PMD_I40E_MB_EVENT_PROCEED) {
> + PMD_DRV_LOG(WARNING, "VF to PF message(%d) is not permitted!",
> + opcode);
> + b_op = FALSE;
> + }
> +
Is the reason you just not do [1], because final function requires valid
buffers? Can it be an option to fix them?
[1]
if (!b_op) {
i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_NOT_SUPPORTED, NULL, 0);
}
> switch (opcode) {
> case I40E_VIRTCHNL_OP_VERSION :
> PMD_DRV_LOG(INFO, "OP_VERSION received");
> - i40e_pf_host_process_cmd_version(vf);
> + i40e_pf_host_process_cmd_version(vf, b_op);
> break;
> case I40E_VIRTCHNL_OP_RESET_VF :
> PMD_DRV_LOG(INFO, "OP_RESET_VF received");
> @@ -948,61 +1097,72 @@
> break;
> case I40E_VIRTCHNL_OP_GET_VF_RESOURCES:
> PMD_DRV_LOG(INFO, "OP_GET_VF_RESOURCES received");
> - i40e_pf_host_process_cmd_get_vf_resource(vf);
> + i40e_pf_host_process_cmd_get_vf_resource(vf, b_op);
> break;
> case I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES:
> PMD_DRV_LOG(INFO, "OP_CONFIG_VSI_QUEUES received");
> - i40e_pf_host_process_cmd_config_vsi_queues(vf, msg, msglen);
> + i40e_pf_host_process_cmd_config_vsi_queues(vf, msg,
> + msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EXT:
> PMD_DRV_LOG(INFO, "OP_CONFIG_VSI_QUEUES_EXT received");
> i40e_pf_host_process_cmd_config_vsi_queues_ext(vf, msg,
> - msglen);
> + msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_CONFIG_IRQ_MAP:
> PMD_DRV_LOG(INFO, "OP_CONFIG_IRQ_MAP received");
> - i40e_pf_host_process_cmd_config_irq_map(vf, msg, msglen);
> + i40e_pf_host_process_cmd_config_irq_map(vf, msg, msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_ENABLE_QUEUES:
> PMD_DRV_LOG(INFO, "OP_ENABLE_QUEUES received");
> - i40e_pf_host_process_cmd_enable_queues(vf, msg, msglen);
> - i40e_notify_vf_link_status(dev, vf);
> + if (b_op) {
> + i40e_pf_host_process_cmd_enable_queues(vf, msg, msglen);
> + i40e_notify_vf_link_status(dev, vf);
> + } else {
> + i40e_pf_host_send_msg_to_vf(
> + vf, I40E_VIRTCHNL_OP_ENABLE_QUEUES,
> + I40E_NOT_SUPPORTED, NULL, 0);
> + }
> break;
> case I40E_VIRTCHNL_OP_DISABLE_QUEUES:
> PMD_DRV_LOG(INFO, "OP_DISABLE_QUEUE received");
> - i40e_pf_host_process_cmd_disable_queues(vf, msg, msglen);
> + i40e_pf_host_process_cmd_disable_queues(vf, msg, msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_ADD_ETHER_ADDRESS:
> PMD_DRV_LOG(INFO, "OP_ADD_ETHER_ADDRESS received");
> - i40e_pf_host_process_cmd_add_ether_address(vf, msg, msglen);
> + i40e_pf_host_process_cmd_add_ether_address(vf, msg,
> + msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_DEL_ETHER_ADDRESS:
> PMD_DRV_LOG(INFO, "OP_DEL_ETHER_ADDRESS received");
> - i40e_pf_host_process_cmd_del_ether_address(vf, msg, msglen);
> + i40e_pf_host_process_cmd_del_ether_address(vf, msg,
> + msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_ADD_VLAN:
> PMD_DRV_LOG(INFO, "OP_ADD_VLAN received");
> - i40e_pf_host_process_cmd_add_vlan(vf, msg, msglen);
> + i40e_pf_host_process_cmd_add_vlan(vf, msg, msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_DEL_VLAN:
> PMD_DRV_LOG(INFO, "OP_DEL_VLAN received");
> - i40e_pf_host_process_cmd_del_vlan(vf, msg, msglen);
> + i40e_pf_host_process_cmd_del_vlan(vf, msg, msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_CONFIG_PROMISCUOUS_MODE:
> PMD_DRV_LOG(INFO, "OP_CONFIG_PROMISCUOUS_MODE received");
> - i40e_pf_host_process_cmd_config_promisc_mode(vf, msg, msglen);
> + i40e_pf_host_process_cmd_config_promisc_mode(vf, msg,
> + msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_GET_STATS:
> PMD_DRV_LOG(INFO, "OP_GET_STATS received");
> - i40e_pf_host_process_cmd_get_stats(vf);
> + i40e_pf_host_process_cmd_get_stats(vf, b_op);
> break;
> case I40E_VIRTCHNL_OP_CFG_VLAN_OFFLOAD:
> PMD_DRV_LOG(INFO, "OP_CFG_VLAN_OFFLOAD received");
> - i40e_pf_host_process_cmd_cfg_vlan_offload(vf, msg, msglen);
> + i40e_pf_host_process_cmd_cfg_vlan_offload(vf, msg,
> + msglen, b_op);
> break;
> case I40E_VIRTCHNL_OP_CFG_VLAN_PVID:
> PMD_DRV_LOG(INFO, "OP_CFG_VLAN_PVID received");
> - i40e_pf_host_process_cmd_cfg_pvid(vf, msg, msglen);
> + i40e_pf_host_process_cmd_cfg_pvid(vf, msg, msglen, b_op);
> break;
> /* Don't add command supported below, which will
> * return an error code.
> diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h
> index 14852f2..eb7a72b 100644
> --- a/drivers/net/i40e/rte_pmd_i40e.h
> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> @@ -42,6 +42,27 @@
> #include <rte_ethdev.h>
>
> /**
> + * Response sent back to i40e driver from user app after callback
> + */
> +enum rte_pmd_i40e_mb_event_rsp {
> + RTE_PMD_I40E_MB_EVENT_NOOP_ACK, /**< skip mbox request and ACK */
> + RTE_PMD_I40E_MB_EVENT_NOOP_NACK, /**< skip mbox request and NACK */
> + RTE_PMD_I40E_MB_EVENT_PROCEED, /**< proceed with mbox request */
> + RTE_PMD_I40E_MB_EVENT_MAX /**< max value of this enum */
> +};
> +
> +/**
> + * Data sent to the user application when the callback is executed.
> + */
> +struct rte_pmd_i40e_mb_event_param {
> + uint16_t vfid; /**< Virtual Function number */
> + uint16_t msg_type; /**< VF to PF message type, see i40e_virtchnl_ops */
> + uint16_t retval; /**< return value */
> + void *msg; /**< pointer to message */
> + uint16_t msglen; /**< length of the message */
> +};
> +
> +/**
> * Notify VF when PF link status changes.
> *
> * @param port
>
^ permalink raw reply
* Re: [PATCH 10/32] net/dpaa2: introducing dpaa2 bus driver for fsl-mc bus
From: Hemant Agrawal @ 2016-12-07 12:32 UTC (permalink / raw)
To: David Marchand, Thomas Monjalon
Cc: Shreyansh Jain, dev@dpdk.org, Richardson, Bruce
In-Reply-To: <CALwxeUtqtkFEmYApn8_3UBU1fHQ_FepiBL=WheLBHzamgOrhog@mail.gmail.com>
> -----Original Message-----
> From: David Marchand [mailto:david.marchand@6wind.com]
> Sent: Wednesday, December 07, 2016 5:52 PM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: Shreyansh Jain <shreyansh.jain@nxp.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; dev@dpdk.org; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [PATCH 10/32] net/dpaa2: introducing dpaa2 bus driver for fsl-mc
> bus
>
> On Wed, Dec 7, 2016 at 11:40 AM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > 2016-12-07 15:43, Shreyansh Jain:
> >> IMO, the way Bus is kept is debatable.
> >> - should it be in EAL (lib/librte_eal/linuxapp/eal_pci.c like Bus
> >> patches) [1]?
> >> - Should it a 'handler/driver' parallel to device drivers?
> >>
> >> I personally prefer a clean layer for buses with:
> >>
> >> - RTE_SDK/drivers/net/dpaa2/
> >> - RTE_SDK/drivers/bus
> >> - RTE_SDK/drivers/bus/dpaa2/
> >> - RTE_SDK/drivers/bus/dpaa2/dpaa2_bus.c etc.
> >
> > I agree, it is a good idea.
>
> Indeed.
[Hemant] I will fix it in v2.
>
> >> For PCI, which is generic (or for other similar generic buses, like
> >> platform), we can keep the implementation within lib/librte_eal/linuxapp/*.
> >
> > I would be in favor of moving PCI and vdev code from EAL to drivers/bus/.
> > We can keep the API in EAL and implement the buses as drivers.
> >
> > Other opinions?
>
> The only issue I see for now is how to pass the configuration to these drivers,
> like vdev args or the pci blacklist/whitelist.
>
>
> --
> David Marchand
^ permalink raw reply
* Re: [PATCH 10/32] net/dpaa2: introducing dpaa2 bus driver for fsl-mc bus
From: David Marchand @ 2016-12-07 12:21 UTC (permalink / raw)
To: Thomas Monjalon
Cc: Shreyansh Jain, Hemant Agrawal, dev@dpdk.org, Richardson, Bruce
In-Reply-To: <11053786.o6LJ0UWG8a@xps13>
On Wed, Dec 7, 2016 at 11:40 AM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> 2016-12-07 15:43, Shreyansh Jain:
>> IMO, the way Bus is kept is debatable.
>> - should it be in EAL (lib/librte_eal/linuxapp/eal_pci.c like Bus
>> patches) [1]?
>> - Should it a 'handler/driver' parallel to device drivers?
>>
>> I personally prefer a clean layer for buses with:
>>
>> - RTE_SDK/drivers/net/dpaa2/
>> - RTE_SDK/drivers/bus
>> - RTE_SDK/drivers/bus/dpaa2/
>> - RTE_SDK/drivers/bus/dpaa2/dpaa2_bus.c etc.
>
> I agree, it is a good idea.
Indeed.
>> For PCI, which is generic (or for other similar generic buses, like
>> platform), we can keep the implementation within lib/librte_eal/linuxapp/*.
>
> I would be in favor of moving PCI and vdev code from EAL to drivers/bus/.
> We can keep the API in EAL and implement the buses as drivers.
>
> Other opinions?
The only issue I see for now is how to pass the configuration to these
drivers, like vdev args or the pci blacklist/whitelist.
--
David Marchand
^ permalink raw reply
* Re: [PATCH 00/13] Introducing EAL Bus-Device-Driver Model
From: David Marchand @ 2016-12-07 12:17 UTC (permalink / raw)
To: Shreyansh Jain; +Cc: dev@dpdk.org, Thomas Monjalon
In-Reply-To: <fd35ef7d-77a4-c79b-7d6f-a14a2a5a56ca@nxp.com>
Hello Shreyansh,
On Wed, Dec 7, 2016 at 10:55 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> On Wednesday 07 December 2016 02:22 AM, David Marchand wrote:
>>> 0002~0003: Introducing the basic Bus model and associated test case
>>> 0005: Support insertion of device rather than addition to tail
>>
>>
>> Patch 2 and 5 could be squashed.
>
>
> I deliberately kept them separate. I intent to extend the Patch 5 for
> hotplugging. But, if I don't end up adding support for that in this series,
> I will merge these two.
Fine.
>> The constructor priority stuff seems unneeded as long as we use
>> explicit reference to a global (or local, did not check) bus symbol
>> rather than a runtime lookup.
>
>
> I didn't understand your point here.
> IMO, constructor priority (or some other way to handle this) is important. I
> faced this issue while verifying it at my end when the drivers were getting
> registered before the bus.
>
> Can you elaborate more on '..use explicit reference to a global...'?
The drivers register themselves to a bus using this bus specific api.
For pci, this is rte_eal_pci_register().
The pci_bus object must be moved to eal_common_pci.c (we can stil
internally expose for bsd / linux specific implementations).
Then, rte_eal_pci_register() can add the pci driver to the pci_bus
drivers list even if this pci_bus object is not registered yet to the
buses list.
And no constructor order issue ?
>>
>>
>>> 0004: Add scan and match callbacks for the Bus and updated test case
>>
>>
>> Why do you push back the bus object in the 'scan' method ?
>> This method is bus specific which means that the code "knows" the
>> object registered with the callback.
>
>
> This 'knows' is the grey area for me.
> The bus (for example, PCI) after scanning needs to call
> rte_eal_bus_add_device() to link the device in bus's device_list.
>
> Two options:
> 1. Have a global reference to "pci" bus (rte_bus) somewhere in eal_pci.c
> 2. Call rte_eal_get_bus() every time someone needs the reference.
> 3. C++ style, 'this->'.
>
> I have taken the 3rd path. It simplifies my code to not assume a handle as
> well as not allow for reference fetch calls every now and then.
>
> As a disadvantage: it means passing this as argument - and some cases
> maintaining it as __rte_unused.
>
> Taking (1) or (2) is not advantageous than this approach.
1) is the simplest one.
When you write a pci_scan method and embed it in you pci_bus object,
but this pci_scan method still wonders which bus object it is supposed
to work on, this is a bit like Schizophrenia ;-).
>> Is is that you want to have a single scan method used by multiple buses ?
>
>
> Yes, but only as a use case. For example, platform devices are of various
> types - what if we have a south-bound bus over a platform bus. In which
> case, a hierarchical bus layout is possible.
> But, this is far-fetched idea for now.
Well, if you have no usecase at the moment, let's keep it simple, please.
>>
>>> 0006: Integrate bus scan/match with EAL, without any effective
>>> driver
>>
>>
>> Hard to find a right balance in patch splittng, but patch 4 and 6 are
>> linked, I would squash them into one.
>
>
> Yes, it is hard and sometimes there is simply no strong rationale for
> splitting or merging. This is one of those cases.
> My idea was that one patch _only_ introduces Bus services (structures,
> functions etc) and another should enable the calls to it from EAL.
> In that sense, I still think 4 and 6 should remain separate, may be
> consecutive, though.
Ok, will see in next version of the patchset.
>>
>>> 0007: rte_pci_driver->probe replaced with rte_driver->probe
>>
>>
>> This patch is too big, please separate in two patches: eal changes
>> then ethdev/driver changes.
>
>
> I don't think that can be done. One change is incomplete without the other.
>
> Changes to all files are only for rte_pci_driver->probe to rte_driver->probe
> movement. EAL changes is to allow rte_eth_dev_pci_probe function after such
> a change as rte_driver->probe has different arguments as compared to
> rte_pci_driver->probe. The patches won't compile if I split.
>
> Am I missing something?
>>
>> Why do you push back the driver object in the 'probe' method ? (idem
>> rte_bus->scan).
>
>
> I am assuming you are referring to rte_driver->probe().
> This is being done so that implementations (specific to drivers on a
> particular bus) can start extracting the rte_xxx_driver, if need be.
>
> For example, for e1000/em_ethdev.c, rte_driver->probe() have been set to
> rte_eth_dev_pci_probe() which requires rte_pci_driver to work with. In
> absence of the rte_driver object, this function cannot call
> rte_pci_driver->probe (for example) for driver specific operations.
Sorry, I am thinking a step ahead with eth_driver out of the picture.
But once eth_driver disappears, I can see no reason to keep this
driver in the probe method (Schizophrenia again).
--
David Marchand
^ permalink raw reply
* Re: [PATCH v12 6/6] testpmd: use Tx preparation in csum engine
From: Kulasek, TomaszX @ 2016-12-07 12:12 UTC (permalink / raw)
To: Mcnamara, John, Yigit, Ferruh, dev@dpdk.org
Cc: Ananyev, Konstantin, olivier.matz@6wind.com
In-Reply-To: <B27915DBBA3421428155699D51E4CFE20267036C@IRSMSX103.ger.corp.intel.com>
Hi John,
> -----Original Message-----
> From: Mcnamara, John
> Sent: Wednesday, December 7, 2016 13:01
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Kulasek, TomaszX
> <tomaszx.kulasek@intel.com>; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> olivier.matz@6wind.com
> Subject: RE: [dpdk-dev] [PATCH v12 6/6] testpmd: use Tx preparation in
> csum engine
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > Sent: Wednesday, December 7, 2016 11:14 AM
> > To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > olivier.matz@6wind.com
> > Subject: Re: [dpdk-dev] [PATCH v12 6/6] testpmd: use Tx preparation in
> > csum engine
> >
> > ...
> > <...>
> > > +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",
> >
> > Can you please format help string as:
> > "cmd fixed_string fixed|string|options <variable>: Description"
> > see commit 26faac80327f
> >
> > above becomes:
> > "csum txprep on|off: Enable/Disable tx preparation path for csum engine"
> >
>
>
> Also, does this require an update to the testpmd docs?
>
>
Yes, I think. I will add adequate description to the docs.
Tomasz
^ permalink raw reply
* Re: [PATCH] Force python scripts to run with python2
From: Mcnamara, John @ 2016-12-07 12:04 UTC (permalink / raw)
To: Martin Kletzander, dev@dpdk.org
In-Reply-To: <dffa9dad9885dd075d22b5d02aec531f60846985.1481105807.git.mkletzan@redhat.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Martin Kletzander
> Sent: Wednesday, December 7, 2016 10:17 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] Force python scripts to run with python2
>
> With python3 being the default in some distributions/installations,
> shebang with just "python" will make the script run under version of
> python that the scripts are not written for. In order to fix that and
> mitigate future errors, use shebang properly and specify the python
> version. That way the scripts will run in any distro/install, no matter
> what python version is set as the default.
I think a better approach would be to make the scripts Python 2 and Python 3 compatible.
Some of the new ones already are.
^ permalink raw reply
* Re: [PATCH v12 6/6] testpmd: use Tx preparation in csum engine
From: Kulasek, TomaszX @ 2016-12-07 12:00 UTC (permalink / raw)
To: Yigit, Ferruh, dev@dpdk.org; +Cc: Ananyev, Konstantin, olivier.matz@6wind.com
In-Reply-To: <067ddc1b-5b28-47fe-8947-84a218357488@intel.com>
Hi,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 7, 2016 12:14
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v12 6/6] testpmd: use Tx preparation in
> csum engine
>
> On 11/23/2016 5:36 PM, Tomasz Kulasek wrote:
> > 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
> <...>
> > +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",
>
> Can you please format help string as:
> "cmd fixed_string fixed|string|options <variable>: Description"
> see commit 26faac80327f
>
> above becomes:
> "csum txprep on|off: Enable/Disable tx preparation path for csum engine"
>
> <...>
Sure, thanks.
Tomasz
^ permalink raw reply
* Re: [PATCH v12 6/6] testpmd: use Tx preparation in csum engine
From: Mcnamara, John @ 2016-12-07 12:00 UTC (permalink / raw)
To: Yigit, Ferruh, Kulasek, TomaszX, dev@dpdk.org
Cc: Ananyev, Konstantin, olivier.matz@6wind.com
In-Reply-To: <067ddc1b-5b28-47fe-8947-84a218357488@intel.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Wednesday, December 7, 2016 11:14 AM
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>; dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v12 6/6] testpmd: use Tx preparation in
> csum engine
>
> ...
> <...>
> > +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",
>
> Can you please format help string as:
> "cmd fixed_string fixed|string|options <variable>: Description"
> see commit 26faac80327f
>
> above becomes:
> "csum txprep on|off: Enable/Disable tx preparation path for csum engine"
>
Also, does this require an update to the testpmd docs?
^ permalink raw reply
* Re: [PATCH 1/8] drivers/common/dpaa2: Run time assembler for Descriptor formation
From: Akhil Goyal @ 2016-12-07 11:44 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev@dpdk.org, declan.doherty@intel.com,
pablo.de.lara.guarch@intel.com, Hemant Agrawal, Horia Geantă
In-Reply-To: <13107823.ZnI407z9cY@xps13>
-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
Sent: Wednesday, December 07, 2016 2:03 PM
To: Akhil Goyal <akhil.goyal@nxp.com>
Cc: dev@dpdk.org; declan.doherty@intel.com; pablo.de.lara.guarch@intel.com; Hemant Agrawal <hemant.agrawal@nxp.com>; Horia Geantă <horia.geanta@nxp.com>
Subject: Re: [PATCH 1/8] drivers/common/dpaa2: Run time assembler for Descriptor formation
2016-12-07 06:24, Akhil Goyal:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> 2016-12-05 18:25, Akhil Goyal:
> > FLib is a library which helps in making the descriptors which is
> > understood by NXP's SEC hardware.
> > This patch provides header files for command words which can be used
> > for descritptor formation.
>
> It seems this code is old. Does it exist as a standalone library somewhere?
[Akhil] Let me correct here. This is not a library. This is a set of header files.
Yes this is an old code. This is generally shipped with NXP SDK.
> Where was it hosted before duplicating it in DPDK?
[Akhil] This is part of NXP SDK and also available at git.freescale.com.
>
> Why do you want to have a common directory drivers/common/dpaa2/flib instead of a sub-directory in the crypto driver?
[Akhil] I agree with your suggestion. This can be maintained within drivers/crypto as a common header files set for different NXP Architecture crypto drivers.
>
> [Akhil] This is not really a library. This is a set of header files which is required for compilation. We have 2 other cypto drivers (for different platforms viz: Non-DPAA and DPAA1_QORIQ) which uses the same flib. So we put it in common directory. We plan to send patches for other drivers in the upcoming releases.
Please Akhil, could you answer to the three questions?
^ permalink raw reply
* [PATCH] Force python scripts to run with python2
From: Martin Kletzander @ 2016-12-07 10:16 UTC (permalink / raw)
To: dev
With python3 being the default in some distributions/installations,
shebang with just "python" will make the script run under version of
python that the scripts are not written for. In order to fix that and
mitigate future errors, use shebang properly and specify the python
version. That way the scripts will run in any distro/install, no
matter what python version is set as the default.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
app/cmdline_test/cmdline_test.py | 2 +-
app/cmdline_test/cmdline_test_data.py | 2 +-
app/test/autotest.py | 2 +-
app/test/autotest_data.py | 2 +-
app/test/autotest_runner.py | 2 +-
app/test/autotest_test_funcs.py | 2 +-
examples/ip_pipeline/config/diagram-generator.py | 2 +-
examples/ip_pipeline/config/pipeline-to-core-mapping.py | 2 +-
tools/cpu_layout.py | 2 +-
tools/dpdk-devbind.py | 2 +-
tools/dpdk-pmdinfo.py | 2 +-
11 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/app/cmdline_test/cmdline_test.py b/app/cmdline_test/cmdline_test.py
index 8efc5ead4439..3a3937abc8fb 100755
--- a/app/cmdline_test/cmdline_test.py
+++ b/app/cmdline_test/cmdline_test.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python2
# BSD LICENSE
#
diff --git a/app/cmdline_test/cmdline_test_data.py b/app/cmdline_test/cmdline_test_data.py
index b1945a579f24..882b7355f83d 100644
--- a/app/cmdline_test/cmdline_test_data.py
+++ b/app/cmdline_test/cmdline_test_data.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python2
# BSD LICENSE
#
diff --git a/app/test/autotest.py b/app/test/autotest.py
index b9fd6b6f5b21..4891410dbbd6 100644
--- a/app/test/autotest.py
+++ b/app/test/autotest.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python2
# BSD LICENSE
#
diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
index 9e8fd946a063..6e84eccc6a2e 100644
--- a/app/test/autotest_data.py
+++ b/app/test/autotest_data.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python2
# BSD LICENSE
#
diff --git a/app/test/autotest_runner.py b/app/test/autotest_runner.py
index 21d3be2cb2cd..1eb64d3bedd5 100644
--- a/app/test/autotest_runner.py
+++ b/app/test/autotest_runner.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python2
# BSD LICENSE
#
diff --git a/app/test/autotest_test_funcs.py b/app/test/autotest_test_funcs.py
index 14cffd014565..8ee9b280706b 100644
--- a/app/test/autotest_test_funcs.py
+++ b/app/test/autotest_test_funcs.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python2
# BSD LICENSE
#
diff --git a/examples/ip_pipeline/config/diagram-generator.py b/examples/ip_pipeline/config/diagram-generator.py
index 6b7170b00486..7a2eb3fd6319 100755
--- a/examples/ip_pipeline/config/diagram-generator.py
+++ b/examples/ip_pipeline/config/diagram-generator.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python2
# BSD LICENSE
#
diff --git a/examples/ip_pipeline/config/pipeline-to-core-mapping.py b/examples/ip_pipeline/config/pipeline-to-core-mapping.py
index c2050b82a9fb..355061405d9d 100755
--- a/examples/ip_pipeline/config/pipeline-to-core-mapping.py
+++ b/examples/ip_pipeline/config/pipeline-to-core-mapping.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python2
# BSD LICENSE
#
diff --git a/tools/cpu_layout.py b/tools/cpu_layout.py
index d38d0b5a0cfe..97038b37aa28 100755
--- a/tools/cpu_layout.py
+++ b/tools/cpu_layout.py
@@ -1,4 +1,4 @@
-#! /usr/bin/python
+#!/usr/bin/env python2
#
# BSD LICENSE
#
diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py
index f1d374d6b08c..f520f9a99d5b 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py
@@ -1,4 +1,4 @@
-#! /usr/bin/python
+#!/usr/bin/env python2
#
# BSD LICENSE
#
diff --git a/tools/dpdk-pmdinfo.py b/tools/dpdk-pmdinfo.py
index 3db9819c61d3..0a8fba01f0ff 100755
--- a/tools/dpdk-pmdinfo.py
+++ b/tools/dpdk-pmdinfo.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python2
# -------------------------------------------------------------------------
#
# Utility to dump PMD_INFO_STRING support from an object file
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v12 6/6] testpmd: use Tx preparation in csum engine
From: Ferruh Yigit @ 2016-12-07 11:13 UTC (permalink / raw)
To: Tomasz Kulasek, dev; +Cc: konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-7-git-send-email-tomaszx.kulasek@intel.com>
On 11/23/2016 5:36 PM, Tomasz Kulasek wrote:
> 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
<...>
> +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",
Can you please format help string as:
"cmd fixed_string fixed|string|options <variable>: Description"
see commit 26faac80327f
above becomes:
"csum txprep on|off: Enable/Disable tx preparation path for csum engine"
<...>
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-12-07 11:12 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <1480996340-29871-2-git-send-email-jerin.jacob@caviumnetworks.com>
On Tue, Dec 06, 2016 at 09:22:15AM +0530, Jerin Jacob wrote:
> In a polling model, lcores poll ethdev ports and associated
> rx queues directly to look for packet. In an event driven model,
> by contrast, lcores call the scheduler that selects packets for
> them based on programmer-specified criteria. Eventdev library
> adds support for event driven programming model, which offer
> applications automatic multicore scaling, dynamic load balancing,
> pipelining, packet ingress order maintenance and
> synchronization services to simplify application packet processing.
>
> By introducing event driven programming model, DPDK can support
> both polling and event driven programming models for packet processing,
> and applications are free to choose whatever model
> (or combination of the two) that best suits their needs.
>
> This patch adds the eventdev specification header file.
>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
> MAINTAINERS | 3 +
> doc/api/doxy-api-index.md | 1 +
> doc/api/doxy-api.conf | 1 +
> lib/librte_eventdev/rte_eventdev.h | 1274 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 1279 insertions(+)
<snip>
> +
> +/** Structure to hold the queue to port link establishment attributes */
> +struct rte_event_queue_link {
> + uint8_t queue_id;
> + /**< Event queue identifier to select the source queue to link */
> + uint8_t priority;
> + /**< The priority of the event queue for this event port.
> + * The priority defines the event port's servicing priority for
> + * event queue, which may be ignored by an implementation.
> + * The requested priority should in the range of
> + * [RTE_EVENT_DEV_PRIORITY_HIGHEST, RTE_EVENT_DEV_PRIORITY_LOWEST].
> + * The implementation shall normalize the requested priority to
> + * implementation supported priority value.
> + */
> +};
> +
> +/**
> + * Link multiple source event queues supplied in *rte_event_queue_link*
> + * structure as *queue_id* to the destination event port designated by its
> + * *port_id* on the event device designated by its *dev_id*.
> + *
> + * The link establishment shall enable the event port *port_id* from
> + * receiving events from the specified event queue *queue_id*
> + *
> + * An event queue may link to one or more event ports.
> + * The number of links can be established from an event queue to event port is
> + * implementation defined.
> + *
> + * Event queue(s) to event port link establishment can be changed at runtime
> + * without re-configuring the device to support scaling and to reduce the
> + * latency of critical work by establishing the link with more event ports
> + * at runtime.
> + *
> + * @param dev_id
> + * The identifier of the device.
> + *
> + * @param port_id
> + * Event port identifier to select the destination port to link.
> + *
> + * @param link
> + * Points to an array of *nb_links* objects of type *rte_event_queue_link*
> + * structure which contain the event queue to event port link establishment
> + * attributes.
> + * NULL value is allowed, in which case this function links all the configured
> + * event queues *nb_event_queues* which previously supplied to
> + * rte_event_dev_configure() to the event port *port_id* with normal servicing
> + * priority(RTE_EVENT_DEV_PRIORITY_NORMAL).
> + *
> + * @param nb_links
> + * The number of links to establish
> + *
> + * @return
> + * The number of links actually established. The return value can be less than
> + * the value of the *nb_links* parameter when the implementation has the
> + * limitation on specific queue to port link establishment or if invalid
> + * parameters are specified in a *rte_event_queue_link*.
> + * If the return value is less than *nb_links*, the remaining links at the end
> + * of link[] are not established, and the caller has to take care of them.
> + * If return value is less than *nb_links* then implementation shall update the
> + * rte_errno accordingly, Possible rte_errno values are
> + * (-EDQUOT) Quota exceeded(Application tried to link the queue configured with
> + * RTE_EVENT_QUEUE_CFG_FLAG_SINGLE_LINK to more than one event ports)
> + * (-EINVAL) Invalid parameter
> + *
> + */
> +int
> +rte_event_port_link(uint8_t dev_id, uint8_t port_id,
> + const struct rte_event_queue_link link[],
> + uint16_t nb_links);
> +
Hi again Jerin,
another small suggestion here. I'm not a big fan of using small
structures to pass parameters into functions, especially when not all
fields are always going to be used. Rather than use the event queue link
structure, can we just pass in two array parameters here - the list of
QIDs, and the list of priorities. In cases where the eventdev
implementation does not support link prioritization, or where the app
does not want different priority mappings , then the second
array can be null [implying NORMAL priority for the don't care case].
int
rte_event_port_link(uint8_t dev_id, uint8_t port_id,
const uint8_t queues[], const uint8_t priorities[],
uint16_t nb_queues);
This just makes mapping an array of queues easier, as we can just pass
an array of ints directly in, and it especially makes it easier to
create a single link via:
rte_event_port_link(dev_id, port_id, &queue_id, NULL, 1);
Regards,
/Bruce
^ permalink raw reply
* Re: [RFC] ethdev: abstraction layer for QoS hierarchical scheduler
From: Alan Robertson @ 2016-12-07 10:58 UTC (permalink / raw)
To: Dumitrescu, Cristian; +Cc: dev@dpdk.org
In-Reply-To: <57688e98-15d5-1866-0c3a-9dda81621651@brocade.com>
Hi Cristian,
Looking at points 10 and 11 it's good to hear nodes can be dynamically added.
We've been trying to decide the best way to do this for support of qos on tunnels for
some time now and the existing implementation doesn't allow this so effectively ruled
out hierarchical queueing for tunnel targets on the output interface.
Having said that, has thought been given to separating the queueing from being so closely
tied to the Ethernet transmit process ? When queueing on a tunnel for example we may
be working with encryption. When running with an anti-reply window it is really much
better to do the QOS (packet reordering) before the encryption. To support this would
it be possible to have a separate scheduler structure which can be passed into the
scheduling API ? This means the calling code can hang the structure of whatever entity
it wishes to perform qos on, and we get dynamic target support (sessions/tunnels etc).
Regarding the structure allocation, would it be possible to make the number of queues
associated with a TC a compile time option which the scheduler would accommodate ?
We frequently only use one queue per tc which means 75% of the space allocated at
the queueing layer for that tc is never used. This may be specific to our implementation
but if other implementations do the same if folks could say we may get a better idea
if this is a common case.
Whilst touching on the scheduler, the token replenishment works using a division and
multiplication obviously to cater for the fact that it may be run after several tc windows
have passed. The most commonly used industrial scheduler simply does a lapsed on the tc
and then adds the bc. This relies on the scheduler being called within the tc window
though. It would be nice to have this as a configurable option since it's much for efficient
assuming the infra code from which it's called can guarantee the calling frequency.
I hope you'll consider these points for inclusion into a future road map. Hopefully in the
future my employer will increase the priority of some of the tasks and a PR may appear
on the mailing list.
Thanks,
Alan.
Subject:
[dpdk-dev] [RFC] ethdev: abstraction layer for QoS hierarchical scheduler
Date:
Wed, 30 Nov 2016 18:16:50 +0000
From:
Cristian Dumitrescu <cristian.dumitrescu@intel.com><mailto:cristian.dumitrescu@intel.com>
To:
dev@dpdk.org<mailto:dev@dpdk.org>
CC:
cristian.dumitrescu@intel.com<mailto:cristian.dumitrescu@intel.com>
This RFC proposes an ethdev-based abstraction layer for Quality of Service (QoS)
hierarchical scheduler. The goal of the abstraction layer is to provide a simple
generic API that is agnostic of the underlying HW, SW or mixed HW-SW complex
implementation.
Q1: What is the benefit for having an abstraction layer for QoS hierarchical
layer?
A1: There is growing interest in the industry for handling various HW-based,
SW-based or mixed hierarchical scheduler implementations using a unified DPDK
API.
Q2: Which devices are targeted by this abstraction layer?
A2: All current and future devices that expose a hierarchical scheduler feature
under DPDK, including NICs, FPGAs, ASICs, SOCs, SW libraries.
Q3: Which scheduler hierarchies are supported by the API?
A3: Hopefully any scheduler hierarchy can be described and covered by the
current API. Of course, functional correctness, accuracy and performance levels
depend on the specific implementations of this API.
Q4: Why have this abstraction layer into ethdev as opposed to a new type of
device (e.g. scheddev) similar to ethdev, cryptodev, eventdev, etc?
A4: Packets are sent to the Ethernet device using the ethdev API
rte_eth_tx_burst() function, with the hierarchical scheduling taking place
automatically (i.e. no SW intervention) in HW implementations. Basically, the
hierarchical scheduler is done as part of packet TX operation.
The hierarchical scheduler is typically the last stage before packet TX and it
is tightly integrated with the TX stage. The hierarchical scheduler is just
another offload feature of the Ethernet device, which needs to be accommodated
by the ethdev API similar to any other offload feature (such as RSS, DCB,
flow director, etc).
Once the decision to schedule a specific packet has been taken, this packet
cannot be dropped and it has to be sent over the wire as is, otherwise what
takes place on the wire is not what was planned at scheduling time, so the
scheduling is not accurate (Note: there are some devices which allow prepending
headers to the packet after the scheduling stage at the expense of sending
correction requests back to the scheduler, but this only strengthens the bond
between scheduling and TX).
Q5: Given that the packet scheduling takes place automatically for pure HW
implementations, how does packet scheduling take place for poll-mode SW
implementations?
A5: The API provided function rte_sched_run() is designed to take care of this.
For HW implementations, this function typically does nothing. For SW
implementations, this function is typically expected to perform dequeue of
packets from the hierarchical scheduler and their write to Ethernet device TX
queue, periodic flush of any buffers on enqueue-side into the hierarchical
scheduler for burst-oriented implementations, etc.
Q6: Which are the scheduling algorithms supported?
A6: The fundamental scheduling algorithms that are supported are Strict Priority
(SP) and Weighted Fair Queuing (WFQ). The SP and WFQ algorithms are supported at
the level of each node of the scheduling hierarchy, regardless of the node
level/position in the tree. The SP algorithm is used to schedule between sibling
nodes with different priority, while WFQ is used to schedule between groups of
siblings that have the same priority.
Algorithms such as Weighed Round Robin (WRR), byte-level WRR, Deficit WRR
(DWRR), etc are considered approximations of the ideal WFQ and are therefore
assimilated to WFQ, although an associated implementation-dependent accuracy,
performance and resource usage trade-off might exist.
Q7: Which are the supported congestion management algorithms?
A7: Tail drop, head drop and Weighted Random Early Detection (WRED). They are
available for every leaf node in the hierarchy, subject to the specific
implementation supporting them.
Q8: Is traffic shaping supported?
A8: Yes, there are a number of shapers (rate limiters) that can be supported for
each node in the hierarchy (built-in limit is currently set to 4 per node). Each
shaper can be private to a node (used only by that node) or shared between
multiple nodes.
Q9: What is the purpose of having shaper profiles and WRED profiles?
A9: In most implementations, many shapers typically share the same configuration
parameters, so defining shaper profiles simplifies the configuration task. Same
considerations apply to WRED contexts and profiles.
Q10: How is the scheduling hierarchy defined and created?
A10: Scheduler hierarchy tree is set up by creating new nodes and connecting
them to other existing nodes, which thus become parent nodes. The unique ID that
is assigned to each node when the node is created is further used to update the
node configuration or to connect children nodes to it. The leaf nodes of the
scheduler hierarchy are each attached to one of the Ethernet device TX queues.
Q11: Are on-the-fly changes of the scheduling hierarchy allowed by the API?
A11: Yes. The actual changes take place subject to the specific implementation
supporting them, otherwise error code is returned.
Q12: What is the typical function call sequence to set up and run the Ethernet
device scheduler?
A12: The typical simplified function call sequence is listed below:
i) Configure the Ethernet device and its TX queues: rte_eth_dev_configure(),
rte_eth_tx_queue_setup()
ii) Create WRED profiles and WRED contexts, shaper profiles and shapers:
rte_eth_sched_wred_profile_add(), rte_eth_sched_wred_context_add(),
rte_eth_sched_shaper_profile_add(), rte_eth_sched_shaper_add()
iii) Create the scheduler hierarchy nodes and tree: rte_eth_sched_node_add()
iv) Freeze the start-up hierarchy and ask the device whether it supports it:
rte_eth_sched_node_add()
v) Start the Ethernet port: rte_eth_dev_start()
vi) Run-time scheduler hierarchy updates: rte_eth_sched_node_add(),
rte_eth_sched_node_<attribute>_set()
vii) Run-time packet enqueue into the hierarchical scheduler: rte_eth_tx_burst()
viii) Run-time support for SW poll-mode implementations (see previous answer):
rte_sched_run()
Q13: Which are the possible options for the user when the Ethernet port does not
support the scheduling hierarchy required by the user?
A13: The following options are available to the user:
i) abort
ii) try out a new hierarchy (e.g. with less leaf nodes), if acceptable
iii) wrap the Ethernet device into a new type of Ethernet device that has a SW
front-end implementing the hierarchical scheduler (e.g. existing DPDK library
librte_sched); instantiate the new device type on-the-fly and check if the
hierarchy requirements can be met by the new device.
Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com><mailto:cristian.dumitrescu@intel.com>
---
lib/librte_ether/rte_ethdev.h | 794 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 794 insertions(+)
mode change 100644 => 100755 lib/librte_ether/rte_ethdev.h
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
old mode 100644
new mode 100755
index 9678179..d4d8604
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -182,6 +182,8 @@ extern "C" {
#include <rte_pci.h>
#include <rte_dev.h>
#include <rte_devargs.h>
+#include <rte_meter.h>
+#include <rte_red.h>
#include "rte_ether.h"
#include "rte_eth_ctrl.h"
#include "rte_dev_info.h"
@@ -1038,6 +1040,152 @@ TAILQ_HEAD(rte_eth_dev_cb_list, rte_eth_dev_callback);
/**< l2 tunnel forwarding mask */
#define ETH_L2_TUNNEL_FORWARDING_MASK 0x00000008
+/**
+ * Scheduler configuration
+ */
+
+/**< Max number of shapers per node */
+#define RTE_ETH_SCHED_SHAPERS_PER_NODE 4
+/**< Invalid shaper ID */
+#define RTE_ETH_SCHED_SHAPER_ID_NONE UINT32_MAX
+/**< Max number of WRED contexts per node */
+#define RTE_ETH_SCHED_WRED_CONTEXTS_PER_NODE 4
+/**< Invalid WRED context ID */
+#define RTE_ETH_SCHED_WRED_CONTEXT_ID_NONE UINT32_MAX
+/**< Invalid node ID */
+#define RTE_ETH_SCHED_NODE_NULL UINT32_MAX
+
+/**
+ * Congestion management (CMAN) mode
+ *
+ * This is used for controlling the admission of packets into a packet queue or
+ * group of packet queues on congestion. On request of writing a new packet
+ * into the current queue while the queue is full, the *tail drop* algorithm
+ * drops the new packet while leaving the queue unmodified, as opposed to *head
+ * drop* algorithm, which drops the packet at the head of the queue (the oldest
+ * packet waiting in the queue) and admits the new packet at the tail of the
+ * queue.
+ *
+ * The *Random Early Detection (RED)* algorithm works by proactively dropping
+ * more and more input packets as the queue occupancy builds up. When the queue
+ * is full or almost full, RED effectively works as *tail drop*. The *Weighted
+ * RED* algorithm uses a separate set of RED thresholds per packet color.
+ */
+enum rte_eth_sched_cman_mode {
+ RTE_ETH_SCHED_CMAN_TAIL_DROP = 0, /**< Tail drop */
+ RTE_ETH_SCHED_CMAN_HEAD_DROP, /**< Head drop */
+ RTE_ETH_SCHED_CMAN_WRED, /**< Weighted Random Early Detection (WRED) */
+};
+
+/**
+ * WRED profile
+ */
+struct rte_eth_sched_wred_params {
+ /**< One set of RED parameters per packet color */
+ struct rte_red_params red_params[e_RTE_METER_COLORS];
+};
+
+/**
+ * Shaper (rate limiter) profile
+ *
+ * Multiple shaper instances can share the same shaper profile. Each node can
+ * have multiple shapers enabled (up to RTE_ETH_SCHED_SHAPERS_PER_NODE). Each
+ * shaper can be private to a node (only one node using it) or shared (multiple
+ * nodes use the same shaper instance).
+ */
+struct rte_eth_sched_shaper_params {
+ uint64_t rate; /**< Token bucket rate (bytes per second) */
+ uint64_t size; /**< Token bucket size (bytes) */
+};
+
+/**
+ * Node parameters
+ *
+ * Each scheduler hierarchy node has multiple inputs (children nodes of the
+ * current parent node) and a single output (which is input to its parent
+ * node). The current node arbitrates its inputs using Strict Priority (SP)
+ * and Weighted Fair Queuing (WFQ) algorithms to schedule input packets on its
+ * output while observing its shaping/rate limiting constraints. Algorithms
+ * such as Weighted Round Robin (WRR), byte-level WRR, Deficit WRR (DWRR), etc
+ * are considered approximations of the ideal WFQ and are assimilated to WFQ,
+ * although an associated implementation-dependent trade-off on accuracy,
+ * performance and resource usage might exist.
+ *
+ * Children nodes with different priorities are scheduled using the SP
+ * algorithm, based on their priority, with zero (0) as the highest priority.
+ * Children with same priority are scheduled using the WFQ algorithm, based on
+ * their weight, which is relative to the sum of the weights of all siblings
+ * with same priority, with one (1) as the lowest weight.
+ */
+struct rte_eth_sched_node_params {
+ /**< Child node priority (used by SP). The highest priority is zero. */
+ uint32_t priority;
+ /**< Child node weight (used by WFQ), relative to some of weights of all
+ siblings with same priority). The lowest weight is one. */
+ uint32_t weight;
+ /**< Set of shaper instances enabled for current node. Each node shaper
+ can be disabled by setting it to RTE_ETH_SCHED_SHAPER_ID_NONE. */
+ uint32_t shaper_id[RTE_ETH_SCHED_SHAPERS_PER_NODE];
+ /**< Set to zero if current node is not a hierarchy leaf node, set to a
+ non-zero value otherwise. A leaf node is a hierarchy node that does
+ not have any children. A leaf node has to be connected to a valid
+ packet queue. */
+ int is_leaf;
+ /**< Parameters valid for leaf nodes only */
+ struct {
+ /**< Packet queue ID */
+ uint64_t queue_id;
+ /**< Congestion management mode */
+ enum rte_eth_sched_cman_mode cman;
+ /**< Set of WRED contexts enabled for current leaf node. Each
+ leaf node WRED context can be disabled by setting it to
+ RTE_ETH_SCHED_WRED_CONTEXT_ID_NONE. Only valid when
+ congestion management for current leaf node is set to WRED. */
+ uint32_t wred_context_id[RTE_ETH_SCHED_WRED_CONTEXTS_PER_NODE];
+ } leaf;
+};
+
+/**
+ * Node statistics counter type
+ */
+enum rte_eth_sched_stats_counter {
+ /**< Number of packets scheduled from current node. */
+ RTE_ETH_SCHED_STATS_COUNTER_N_PKTS = 1<< 0,
+ /**< Number of bytes scheduled from current node. */
+ RTE_ETH_SCHED_STATS_COUNTER_N_BYTES = 1 << 1,
+ RTE_ETH_SCHED_STATS_COUNTER_N_PKTS_DROPPED = 1 << 2,
+ RTE_ETH_SCHED_STATS_COUNTER_N_BYTES_DROPPED = 1 << 3,
+ /**< Number of packets currently waiting in the packet queue of current
+ leaf node. */
+ RTE_ETH_SCHED_STATS_COUNTER_N_PKTS_QUEUED = 1 << 4,
+ /**< Number of bytes currently waiting in the packet queue of current
+ leaf node. */
+ RTE_ETH_SCHED_STATS_COUNTER_N_BYTES_QUEUED = 1 << 5,
+};
+
+/**
+ * Node statistics counters
+ */
+struct rte_eth_sched_node_stats {
+ /**< Number of packets scheduled from current node. */
+ uint64_t n_pkts;
+ /**< Number of bytes scheduled from current node. */
+ uint64_t n_bytes;
+ /**< Statistics counters for leaf nodes only */
+ struct {
+ /**< Number of packets dropped by current leaf node. */
+ uint64_t n_pkts_dropped;
+ /**< Number of bytes dropped by current leaf node. */
+ uint64_t n_bytes_dropped;
+ /**< Number of packets currently waiting in the packet queue of
+ current leaf node. */
+ uint64_t n_pkts_queued;
+ /**< Number of bytes currently waiting in the packet queue of
+ current leaf node. */
+ uint64_t n_bytes_queued;
+ } leaf;
+};
+
/*
* Definitions of all functions exported by an Ethernet driver through the
* the generic structure of type *eth_dev_ops* supplied in the *rte_eth_dev*
@@ -1421,6 +1569,120 @@ typedef int (*eth_get_dcb_info)(struct rte_eth_dev *dev,
struct rte_eth_dcb_info *dcb_info);
/**< @internal Get dcb information on an Ethernet device */
+typedef int (*eth_sched_wred_profile_add_t)(struct rte_eth_dev *dev,
+ uint32_t wred_profile_id,
+ struct rte_eth_sched_wred_params *profile);
+/**< @internal Scheduler WRED profile add */
+
+typedef int (*eth_sched_wred_profile_delete_t)(struct rte_eth_dev *dev,
+ uint32_t wred_profile_id);
+/**< @internal Scheduler WRED profile delete */
+
+typedef int (*eth_sched_wred_context_add_t)(struct rte_eth_dev *dev,
+ uint32_t wred_context_id,
+ uint32_t wred_profile_id);
+/**< @internal Scheduler WRED context add */
+
+typedef int (*eth_sched_wred_context_delete_t)(struct rte_eth_dev *dev,
+ uint32_t wred_context_id);
+/**< @internal Scheduler WRED context delete */
+
+typedef int (*eth_sched_shaper_profile_add_t)(struct rte_eth_dev *dev,
+ uint32_t shaper_profile_id,
+ struct rte_eth_sched_shaper_params *profile);
+/**< @internal Scheduler shaper profile add */
+
+typedef int (*eth_sched_shaper_profile_delete_t)(struct rte_eth_dev *dev,
+ uint32_t shaper_profile_id);
+/**< @internal Scheduler shaper profile delete */
+
+typedef int (*eth_sched_shaper_add_t)(struct rte_eth_dev *dev,
+ uint32_t shaper_id,
+ uint32_t shaper_profile_id);
+/**< @internal Scheduler shaper instance add */
+
+typedef int (*eth_sched_shaper_delete_t)(struct rte_eth_dev *dev,
+ uint32_t shaper_id);
+/**< @internal Scheduler shaper instance delete */
+
+typedef int (*eth_sched_node_add_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint32_t parent_node_id,
+ struct rte_eth_sched_node_params *params);
+/**< @internal Scheduler node add */
+
+typedef int (*eth_sched_node_delete_t)(struct rte_eth_dev *dev,
+ uint32_t node_id);
+/**< @internal Scheduler node delete */
+
+typedef int (*eth_sched_hierarchy_set_t)(struct rte_eth_dev *dev,
+ int clear_on_fail);
+/**< @internal Scheduler hierarchy set */
+
+typedef int (*eth_sched_node_priority_set_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint32_t priority);
+/**< @internal Scheduler node priority set */
+
+typedef int (*eth_sched_node_weight_set_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint32_t weight);
+/**< @internal Scheduler node weight set */
+
+typedef int (*eth_sched_node_shaper_set_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint32_t shaper_pos,
+ uint32_t shaper_id);
+/**< @internal Scheduler node shaper set */
+
+typedef int (*eth_sched_node_queue_set_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint32_t queue_id);
+/**< @internal Scheduler node queue set */
+
+typedef int (*eth_sched_node_cman_set_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ enum rte_eth_sched_cman_mode cman);
+/**< @internal Scheduler node congestion management mode set */
+
+typedef int (*eth_sched_node_wred_context_set_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint32_t wred_context_pos,
+ uint32_t wred_context_id);
+/**< @internal Scheduler node WRED context set */
+
+typedef int (*eth_sched_stats_get_enabled_t)(struct rte_eth_dev *dev,
+ uint64_t *nonleaf_node_capability_stats_mask,
+ uint64_t *nonleaf_node_enabled_stats_mask,
+ uint64_t *leaf_node_capability_stats_mask,
+ uint64_t *leaf_node_enabled_stats_mask);
+/**< @internal Scheduler get set of stats counters enabled for all nodes */
+
+typedef int (*eth_sched_stats_enable_t)(struct rte_eth_dev *dev,
+ uint64_t nonleaf_node_enabled_stats_mask,
+ uint64_t leaf_node_enabled_stats_mask);
+/**< @internal Scheduler enable selected stats counters for all nodes */
+
+typedef int (*eth_sched_node_stats_get_enabled_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint64_t *capability_stats_mask,
+ uint64_t *enabled_stats_mask);
+/**< @internal Scheduler get set of stats counters enabled for specific node */
+
+typedef int (*eth_sched_node_stats_enable_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ uint64_t enabled_stats_mask);
+/**< @internal Scheduler enable selected stats counters for specific node */
+
+typedef int (*eth_sched_node_stats_read_t)(struct rte_eth_dev *dev,
+ uint32_t node_id,
+ struct rte_eth_sched_node_stats *stats,
+ int clear);
+/**< @internal Scheduler read stats counters for specific node */
+
+typedef int (*eth_sched_run_t)(struct rte_eth_dev *dev);
+/**< @internal Scheduler run */
+
/**
* @internal A structure containing the functions exported by an Ethernet driver.
*/
@@ -1547,6 +1809,53 @@ struct eth_dev_ops {
eth_l2_tunnel_eth_type_conf_t l2_tunnel_eth_type_conf;
/** Enable/disable l2 tunnel offload functions */
eth_l2_tunnel_offload_set_t l2_tunnel_offload_set;
+
+ /** Scheduler WRED profile add */
+ eth_sched_wred_profile_add_t sched_wred_profile_add;
+ /** Scheduler WRED profile delete */
+ eth_sched_wred_profile_delete_t sched_wred_profile_delete;
+ /** Scheduler WRED context add */
+ eth_sched_wred_context_add_t sched_wred_context_add;
+ /** Scheduler WRED context delete */
+ eth_sched_wred_context_delete_t sched_wred_context_delete;
+ /** Scheduler shaper profile add */
+ eth_sched_shaper_profile_add_t sched_shaper_profile_add;
+ /** Scheduler shaper profile delete */
+ eth_sched_shaper_profile_delete_t sched_shaper_profile_delete;
+ /** Scheduler shaper instance add */
+ eth_sched_shaper_add_t sched_shaper_add;
+ /** Scheduler shaper instance delete */
+ eth_sched_shaper_delete_t sched_shaper_delete;
+ /** Scheduler node add */
+ eth_sched_node_add_t sched_node_add;
+ /** Scheduler node delete */
+ eth_sched_node_delete_t sched_node_delete;
+ /** Scheduler hierarchy set */
+ eth_sched_hierarchy_set_t sched_hierarchy_set;
+ /** Scheduler node priority set */
+ eth_sched_node_priority_set_t sched_node_priority_set;
+ /** Scheduler node weight set */
+ eth_sched_node_weight_set_t sched_node_weight_set;
+ /** Scheduler node shaper set */
+ eth_sched_node_shaper_set_t sched_node_shaper_set;
+ /** Scheduler node queue set */
+ eth_sched_node_queue_set_t sched_node_queue_set;
+ /** Scheduler node congestion management mode set */
+ eth_sched_node_cman_set_t sched_node_cman_set;
+ /** Scheduler node WRED context set */
+ eth_sched_node_wred_context_set_t sched_node_wred_context_set;
+ /** Scheduler get statistics counter type enabled for all nodes */
+ eth_sched_stats_get_enabled_t sched_stats_get_enabled;
+ /** Scheduler enable selected statistics counters for all nodes */
+ eth_sched_stats_enable_t sched_stats_enable;
+ /** Scheduler get statistics counter type enabled for current node */
+ eth_sched_node_stats_get_enabled_t sched_node_stats_get_enabled;
+ /** Scheduler enable selected statistics counters for current node */
+ eth_sched_node_stats_enable_t sched_node_stats_enable;
+ /** Scheduler read statistics counters for current node */
+ eth_sched_node_stats_read_t sched_node_stats_read;
+ /** Scheduler run */
+ eth_sched_run_t sched_run;
};
/**
@@ -4336,6 +4645,491 @@ rte_eth_dev_l2_tunnel_offload_set(uint8_t port_id,
uint8_t en);
/**
+ * Scheduler WRED profile add
+ *
+ * Create a new WRED profile with ID set to *wred_profile_id*. The new profile
+ * is used to create one or several WRED contexts.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param wred_profile_id
+ * WRED profile ID for the new profile. Needs to be unused.
+ * @param profile
+ * WRED profile parameters. Needs to be pre-allocated and valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_wred_profile_add(uint8_t port_id,
+ uint32_t wred_profile_id,
+ struct rte_eth_sched_wred_params *profile);
+
+/**
+ * Scheduler WRED profile delete
+ *
+ * Delete an existing WRED profile. This operation fails when there is currently
+ * at least one user (i.e. WRED context) of this WRED profile.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param wred_profile_id
+ * WRED profile ID. Needs to be the valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_wred_profile_delete(uint8_t port_id,
+ uint32_t wred_profile_id);
+
+/**
+ * Scheduler WRED context add or update
+ *
+ * When *wred_context_id* is invalid, a new WRED context with this ID is created
+ * by using the WRED profile identified by *wred_profile_id*.
+ *
+ * When *wred_context_id* is valid, this WRED context is no longer using the
+ * profile previously assigned to it and is updated to use the profile
+ * identified by *wred_profile_id*.
+ *
+ * A valid WRED context is assigned to one or several scheduler hierarchy leaf
+ * nodes configured to use WRED as the congestion management mode.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param wred_context_id
+ * WRED context ID
+ * @param wred_profile_id
+ * WRED profile ID. Needs to be the valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_wred_context_add(uint8_t port_id,
+ uint32_t wred_context_id,
+ uint32_t wred_profile_id);
+
+/**
+ * Scheduler WRED context delete
+ *
+ * Delete an existing WRED context. This operation fails when there is currently
+ * at least one user (i.e. scheduler hierarchy leaf node) of this WRED context.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param wred_context_id
+ * WRED context ID. Needs to be the valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_wred_context_delete(uint8_t port_id,
+ uint32_t wred_context_id);
+
+/**
+ * Scheduler shaper profile add
+ *
+ * Create a new shaper profile with ID set to *shaper_profile_id*. The new
+ * shaper profile is used to create one or several shapers.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param shaper_profile_id
+ * Shaper profile ID for the new profile. Needs to be unused.
+ * @param profile
+ * Shaper profile parameters. Needs to be pre-allocated and valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_shaper_profile_add(uint8_t port_id,
+ uint32_t shaper_profile_id,
+ struct rte_eth_sched_shaper_params *profile);
+
+/**
+ * Scheduler shaper profile delete
+ *
+ * Delete an existing shaper profile. This operation fails when there is
+ * currently at least one user (i.e. shaper) of this shaper profile.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param shaper_profile_id
+ * Shaper profile ID. Needs to be the valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+/* no users (shapers) using this profile */
+int rte_eth_sched_shaper_profile_delete(uint8_t port_id,
+ uint32_t shaper_profile_id);
+
+/**
+ * Scheduler shaper add or update
+ *
+ * When *shaper_id* is not a valid shaper ID, a new shaper with this ID is
+ * created using the shaper profile identified by *shaper_profile_id*.
+ *
+ * When *shaper_id* is a valid shaper ID, this shaper is no longer using the
+ * shaper profile previously assigned to it and is updated to use the shaper
+ * profile identified by *shaper_profile_id*.
+ *
+ * A valid shaper is assigned to one or several scheduler hierarchy nodes.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param shaper_id
+ * Shaper ID
+ * @param shaper_profile_id
+ * Shaper profile ID. Needs to be the valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_shaper_add(uint8_t port_id,
+ uint32_t shaper_id,
+ uint32_t shaper_profile_id);
+
+/**
+ * Scheduler shaper delete
+ *
+ * Delete an existing shaper. This operation fails when there is currently at
+ * least one user (i.e. scheduler hierarchy node) of this shaper.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param shaper_id
+ * Shaper ID. Needs to be the valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_shaper_delete(uint8_t port_id,
+ uint32_t shaper_id);
+
+/**
+ * Scheduler node add or remap
+ *
+ * When *node_id* is not a valid node ID, a new node with this ID is created and
+ * connected as child to the existing node identified by *parent_node_id*.
+ *
+ * When *node_id* is a valid node ID, this node is disconnected from its current
+ * parent and connected as child to another existing node identified by
+ * *parent_node_id *.
+ *
+ * This function can be called during port initialization phase (before the
+ * Ethernet port is started) for building the scheduler start-up hierarchy.
+ * Subject to the specific Ethernet port supporting on-the-fly scheduler
+ * hierarchy updates, this function can also be called during run-time (after
+ * the Ethernet port is started).
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID
+ * @param parent_node_id
+ * Parent node ID. Needs to be the valid.
+ * @param params
+ * Node parameters. Needs to be pre-allocated and valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_add(uint8_t port_id,
+ uint32_t node_id,
+ uint32_t parent_node_id,
+ struct rte_eth_sched_node_params *params);
+
+/**
+ * Scheduler node delete
+ *
+ * Delete an existing node. This operation fails when this node currently has at
+ * least one user (i.e. child node).
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_delete(uint8_t port_id,
+ uint32_t node_id);
+
+/**
+ * Scheduler hierarchy set
+ *
+ * This function is called during the port initialization phase (before the
+ * Ethernet port is started) to freeze the scheduler start-up hierarchy.
+ *
+ * This function fails when the currently configured scheduler hierarchy is not
+ * supported by the Ethernet port, in which case the user can abort or try out
+ * another hierarchy configuration (e.g. a hierarchy with less leaf nodes),
+ * which can be build from scratch (when *clear_on_fail* is enabled) or by
+ * modifying the existing hierarchy configuration (when *clear_on_fail* is
+ * disabled).
+ *
+ * Note that, even when the configured scheduler hierarchy is supported (so this
+ * function is successful), the Ethernet port start might still fail due to e.g.
+ * not enough memory being available in the system, etc.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param clear_on_fail
+ * On function call failure, hierarchy is cleared when this parameter is
+ * non-zero and preserved when this parameter is equal to zero.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_hierarchy_set(uint8_t port_id,
+ int clear_on_fail);
+
+/**
+ * Scheduler node priority set
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid.
+ * @param priority
+ * Node priority. The highest node priority is zero. Used by the SP algorithm
+ * running on the parent of the current node for scheduling this child node.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_priority_set(uint8_t port_id,
+ uint32_t node_id,
+ uint32_t priority);
+
+/**
+ * Scheduler node weight set
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid.
+ * @param weight
+ * Node weight. The node weight is relative to the weight sum of all siblings
+ * that have the same priority. The lowest weight is zero. Used by the WFQ
+ * algorithm running on the parent of the current node for scheduling this
+ * child node.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_weight_set(uint8_t port_id,
+ uint32_t node_id,
+ uint32_t weight);
+
+/**
+ * Scheduler node shaper set
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid.
+ * @param shaper_pos
+ * Position in the shaper array of the current node
+ * (0 .. RTE_ETH_SCHED_SHAPERS_PER_NODE-1).
+ * @param shaper_id
+ * Shaper ID. Needs to be either valid shaper ID or set to
+ * RTE_ETH_SCHED_SHAPER_ID_NONE in order to invalidate the shaper on position
+ * *shaper_pos* within the current node.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_shaper_set(uint8_t port_id,
+ uint32_t node_id,
+ uint32_t shaper_pos,
+ uint32_t shaper_id);
+
+/**
+ * Scheduler node queue set
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid.
+ * @param queue_id
+ * Queue ID. Needs to be valid.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_queue_set(uint8_t port_id,
+ uint32_t node_id,
+ uint32_t queue_id);
+
+/**
+ * Scheduler node congestion management mode set
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid leaf node ID.
+ * @param cman
+ * Congestion management mode.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_cman_set(uint8_t port_id,
+ uint32_t node_id,
+ enum rte_eth_sched_cman_mode cman);
+
+/**
+ * Scheduler node WRED context set
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid leaf node ID that has WRED selected as the
+ * congestion management mode.
+ * @param wred_context_pos
+ * Position in the WRED context array of the current leaf node
+ * (0 .. RTE_ETH_SCHED_WRED_CONTEXTS_PER_NODE-1)
+ * @param wred_context_id
+ * WRED context ID. Needs to be either valid WRED context ID or set to
+ * RTE_ETH_SCHED_WRED_CONTEXT_ID_NONE in order to invalidate the WRED context
+ * on position *wred_context_pos* within the current leaf node.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_wred_context_set(uint8_t port_id,
+ uint32_t node_id,
+ uint32_t wred_context_pos,
+ uint32_t wred_context_id);
+
+/**
+ * Scheduler get statistics counter types enabled for all nodes
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param nonleaf_node_capability_stats_mask
+ * Statistics counter types available per node for all non-leaf nodes. Needs
+ * to be pre-allocated.
+ * @param nonleaf_node_enabled_stats_mask
+ * Statistics counter types currently enabled per node for each non-leaf node.
+ * This is a subset of *nonleaf_node_capability_stats_mask*. Needs to be
+ * pre-allocated.
+ * @param leaf_node_capability_stats_mask
+ * Statistics counter types available per node for all leaf nodes. Needs to
+ * be pre-allocated.
+ * @param leaf_node_enabled_stats_mask
+ * Statistics counter types currently enabled for each leaf node. This is
+ * a subset of *leaf_node_capability_stats_mask*. Needs to be pre-allocated.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_stats_get_enabled(uint8_t port_id,
+ uint64_t *nonleaf_node_capability_stats_mask,
+ uint64_t *nonleaf_node_enabled_stats_mask,
+ uint64_t *leaf_node_capability_stats_mask,
+ uint64_t *leaf_node_enabled_stats_mask);
+
+/**
+ * Scheduler enable selected statistics counters for all nodes
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param nonleaf_node_enabled_stats_mask
+ * Statistics counter types to be enabled per node for each non-leaf node.
+ * This needs to be a subset of the statistics counter types available per
+ * node for all non-leaf nodes. Any statistics counter type not included in
+ * this set is to be disabled for all non-leaf nodes.
+ * @param leaf_node_enabled_stats_mask
+ * Statistics counter types to be enabled per node for each leaf node. This
+ * needs to be a subset of the statistics counter types available per node for
+ * all leaf nodes. Any statistics counter type not included in this set is to
+ * be disabled for all leaf nodes.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_stats_enable(uint8_t port_id,
+ uint64_t nonleaf_node_enabled_stats_mask,
+ uint64_t leaf_node_enabled_stats_mask);
+
+/**
+ * Scheduler get statistics counter types enabled for current node
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid.
+ * @param capability_stats_mask
+ * Statistics counter types available for the current node. Needs to be pre-allocated.
+ * @param enabled_stats_mask
+ * Statistics counter types currently enabled for the current node. This is
+ * a subset of *capability_stats_mask*. Needs to be pre-allocated.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_stats_get_enabled(uint8_t port_id,
+ uint32_t node_id,
+ uint64_t *capability_stats_mask,
+ uint64_t *enabled_stats_mask);
+
+/**
+ * Scheduler enable selected statistics counters for current node
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid.
+ * @param enabled_stats_mask
+ * Statistics counter types to be enabled for the current node. This needs to
+ * be a subset of the statistics counter types available for the current node.
+ * Any statistics counter type not included in this set is to be disabled for
+ * the current node.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_stats_enable(uint8_t port_id,
+ uint32_t node_id,
+ uint64_t enabled_stats_mask);
+
+/**
+ * Scheduler node statistics counters read
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param node_id
+ * Node ID. Needs to be valid.
+ * @param stats
+ * When non-NULL, it contains the current value for the statistics counters
+ * enabled for the current node.
+ * @param clear
+ * When this parameter has a non-zero value, the statistics counters are
+ * cleared (i.e. set to zero) immediately after they have been read, otherwise
+ * the statistics counters are left untouched.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+int rte_eth_sched_node_stats_read(uint8_t port_id,
+ uint32_t node_id,
+ struct rte_eth_sched_node_stats *stats,
+ int clear);
+
+/**
+ * Scheduler run
+ *
+ * The packet enqueue side of the scheduler hierarchy is typically done through
+ * the Ethernet device TX function. For HW implementations, the packet dequeue
+ * side is typically done by the Ethernet device without any SW intervention,
+ * therefore this functions should not do anything.
+ *
+ * However, for poll-mode SW or mixed HW-SW implementations, the SW intervention
+ * is likely to be required for running the packet dequeue side of the scheduler
+ * hierarchy. Other potential task performed by this function is periodic flush
+ * of any packet enqueue-side buffers used by the burst-mode implementations.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @return
+ * 0 on success, non-zero error code otherwise.
+ */
+static inline int
+rte_eth_sched_run(uint8_t port_id)
+{
+ struct rte_eth_dev *dev;
+
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
+#endif
+
+ dev = &rte_eth_devices[port_id];
+
+ return (dev->dev_ops->sched_run)? dev->dev_ops->sched_run(dev) : 0;
+}
+
+/**
* Get the port id from pci adrress or device name
* Ex: 0000:2:00.0 or vdev name net_pcap0
*
--
2.5.0
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Van Haaren, Harry @ 2016-12-07 10:57 UTC (permalink / raw)
To: Jerin Jacob, dev@dpdk.org
Cc: thomas.monjalon@6wind.com, Richardson, Bruce,
hemant.agrawal@nxp.com, Eads, Gage
In-Reply-To: <1480996340-29871-2-git-send-email-jerin.jacob@caviumnetworks.com>
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
Hi Jerin,
Re v2 rte_event struct, there seems to be some changes in the struct layout and field sizes. I've investigated them, and would like to propose some changes to balance the byte-alignment and accessing of the fields.
These changes target only the first 64 bits of the rte_event struct. I've left the current v2 code for reference, please find my proposed changes below.
> +struct rte_event {
> + /** WORD0 */
> + RTE_STD_C11
> + union {
> + uint64_t event;
> + /** Event attributes for dequeue or enqueue operation */
> + struct {
> + uint64_t flow_id:20;
> + /**< Targeted flow identifier for the enqueue and
> + * dequeue operation.
> + * The value must be in the range of
> + * [0, nb_event_queue_flows - 1] which
> + * previously supplied to rte_event_dev_configure().
> + */
> + uint64_t sub_event_type:8;
> + /**< Sub-event types based on the event source.
> + * @see RTE_EVENT_TYPE_CPU
> + */
> + uint64_t event_type:4;
> + /**< Event type to classify the event source.
> + * @see RTE_EVENT_TYPE_ETHDEV, (RTE_EVENT_TYPE_*)
> + */
> + uint64_t sched_type:2;
> + /**< Scheduler synchronization type (RTE_SCHED_TYPE_*)
> + * associated with flow id on a given event queue
> + * for the enqueue and dequeue operation.
> + */
> + uint64_t queue_id:8;
> + /**< Targeted event queue identifier for the enqueue or
> + * dequeue operation.
> + * The value must be in the range of
> + * [0, nb_event_queues - 1] which previously supplied to
> + * rte_event_dev_configure().
> + */
> + uint64_t priority:8;
> + /**< Event priority relative to other events in the
> + * event queue. The requested priority should in the
> + * range of [RTE_EVENT_DEV_PRIORITY_HIGHEST,
> + * RTE_EVENT_DEV_PRIORITY_LOWEST].
> + * The implementation shall normalize the requested
> + * priority to supported priority value.
> + * Valid when the device has
> + * RTE_EVENT_DEV_CAP_FLAG_EVENT_QOS capability.
> + */
> + uint64_t op:2;
> + /**< The type of event enqueue operation - new/forward/
> + * etc.This field is not preserved across an instance
> + * and is undefined on dequeue.
> + * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> + */
> + uint64_t impl_opaque:12;
> + /**< Implementation specific opaque value.
> + * An implementation may use this field to hold
> + * implementation specific value to share between
> + * dequeue and enqueue operation.
> + * The application should not modify this field.
> + */
> + };
> + };
struct rte_event {
/** WORD0 */
RTE_STD_C11
union {
uint64_t event;
struct {
uint32_t flow_id: 24;
uint32_t impl_opaque : 8; /* not defined on deq */
uint8_t queue_id;
uint8_t priority;
uint8_t operation : 4; /* new fwd drop */
uint8_t sched_type : 4;
uint8_t event_type : 4;
uint8_t sub_event_type : 4;
};
};
/** word 1 */
<snip>
The changes made are as follows:
* Restore flow_id to 24 bits of a 32 bit int (previous size was 20 bits)
* Add impl_opaque to the remaining 8 bits of those 32 bits (previous size was 12 bits)
* QueueID and Priority remain 8 bit integers - but now accessible as 8 bit ints.
* Operation and sched_type *increased* to 4 bits each (from previous value of 2) to allow future expansion without ABI changes
* Event type remains constant at 4 bits
* sub-event-type reduced to 4 bits (previous value was 8 bits). Can we think of situations where 16 values for application specified identifiers of each event-type is genuinely not enough?
In my opinion this structure layout is more balanced, and will perform better due to less loads that will need masking to access the required value.
Feedback and improvements welcomed, -Harry
^ permalink raw reply
* [PATCH] crypto/openssl: fix extra bytes being written at end of data
From: Piotr Azarewicz @ 2016-12-07 10:45 UTC (permalink / raw)
To: pablo.de.lara.guarch; +Cc: dev, stable
Extra bytes are being written at end of data while process standard
openssl cipher encryption. This behaviour is unexpected.
This patch disable the padding feature in openssl library, which is
causing the problem.
Fixes: d61f70b4c918 ("crypto/libcrypto: add driver for OpenSSL library")
Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
---
drivers/crypto/openssl/rte_openssl_pmd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 5f8fa33..832ea1d 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -496,6 +496,8 @@
if (EVP_EncryptInit_ex(ctx, algo, NULL, key, iv) <= 0)
goto process_cipher_encrypt_err;
+ EVP_CIPHER_CTX_set_padding(ctx, 0);
+
if (EVP_EncryptUpdate(ctx, dst, &dstlen, src, srclen) <= 0)
goto process_cipher_encrypt_err;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 10/32] net/dpaa2: introducing dpaa2 bus driver for fsl-mc bus
From: Thomas Monjalon @ 2016-12-07 10:40 UTC (permalink / raw)
To: Shreyansh Jain; +Cc: Hemant Agrawal, dev, bruce.richardson, david.marchand
In-Reply-To: <6b58f8fa-4df0-684e-f1d3-4dc4dba4f0dd@nxp.com>
2016-12-07 15:43, Shreyansh Jain:
> IMO, the way Bus is kept is debatable.
> - should it be in EAL (lib/librte_eal/linuxapp/eal_pci.c like Bus
> patches) [1]?
> - Should it a 'handler/driver' parallel to device drivers?
>
> I personally prefer a clean layer for buses with:
>
> - RTE_SDK/drivers/net/dpaa2/
> - RTE_SDK/drivers/bus
> - RTE_SDK/drivers/bus/dpaa2/
> - RTE_SDK/drivers/bus/dpaa2/dpaa2_bus.c etc.
I agree, it is a good idea.
> For PCI, which is generic (or for other similar generic buses, like
> platform), we can keep the implementation within lib/librte_eal/linuxapp/*.
I would be in favor of moving PCI and vdev code from EAL to drivers/bus/.
We can keep the API in EAL and implement the buses as drivers.
Other opinions?
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Ananyev, Konstantin @ 2016-12-07 10:22 UTC (permalink / raw)
To: Yuanhan Liu
Cc: Yigit, Ferruh, Olivier Matz, Thomas Monjalon, dev@dpdk.org,
Jan Medala, Jakub Palider, Netanel Belgazal, Evgeny Schemeilin,
Alejandro Lucero, Yong Wang, Andrew Rybchenko, Hemant Agrawal,
Kulasek, TomaszX
In-Reply-To: <20161207101833.GL31182@yliu-dev.sh.intel.com>
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Wednesday, December 7, 2016 10:19 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Olivier Matz <olivier.matz@6wind.com>; Thomas Monjalon <thomas.monjalon@6wind.com>;
> dev@dpdk.org; Jan Medala <jan@semihalf.com>; Jakub Palider <jpa@semihalf.com>; Netanel Belgazal <netanel@amazon.com>; Evgeny
> Schemeilin <evgenys@amazon.com>; Alejandro Lucero <alejandro.lucero@netronome.com>; Yong Wang <yongwang@vmware.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Kulasek, TomaszX
> <tomaszx.kulasek@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v12 0/6] add Tx preparation
>
> On Wed, Dec 07, 2016 at 10:13:14AM +0000, Ananyev, Konstantin wrote:
> >
> > Hi Yliu,
> >
> > >
> > > On Tue, Dec 06, 2016 at 03:53:42PM +0000, Ferruh Yigit wrote:
> > > > > Please, we need a comment for each driver saying
> > > > > "it is OK, we do not need any checksum preparation for TSO"
> > > > > or
> > > > > "yes we have to implement tx_prepare or TSO will not work in this mode"
> > > > >
> > >
> > > Sorry for late. For virtio, I think it's not a must. The checksum stuff
> > > has been handled inside the Tx function. However, we may could move it
> > > to tx_prepare, which would actually recover the performance lost
> > > introduced while enabling TSO for the non-TSO case.
> > >
> >
> > So would you like to provide a patch for it,
> > Or would you like to keep tx_prepare() for virtio as NOP for now?
>
> Hi Konstantin,
>
> I'd keep it as it is for now. It should be a trivial patch after all, that
> I could provide it when everything are settled down.
Ok, thanks for clarification.
Konstantin
>
> --yliu
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Yuanhan Liu @ 2016-12-07 10:18 UTC (permalink / raw)
To: Ananyev, Konstantin
Cc: Yigit, Ferruh, Olivier Matz, Thomas Monjalon, dev@dpdk.org,
Jan Medala, Jakub Palider, Netanel Belgazal, Evgeny Schemeilin,
Alejandro Lucero, Yong Wang, Andrew Rybchenko, Hemant Agrawal,
Kulasek, TomaszX
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E4E4B@irsmsx105.ger.corp.intel.com>
On Wed, Dec 07, 2016 at 10:13:14AM +0000, Ananyev, Konstantin wrote:
>
> Hi Yliu,
>
> >
> > On Tue, Dec 06, 2016 at 03:53:42PM +0000, Ferruh Yigit wrote:
> > > > Please, we need a comment for each driver saying
> > > > "it is OK, we do not need any checksum preparation for TSO"
> > > > or
> > > > "yes we have to implement tx_prepare or TSO will not work in this mode"
> > > >
> >
> > Sorry for late. For virtio, I think it's not a must. The checksum stuff
> > has been handled inside the Tx function. However, we may could move it
> > to tx_prepare, which would actually recover the performance lost
> > introduced while enabling TSO for the non-TSO case.
> >
>
> So would you like to provide a patch for it,
> Or would you like to keep tx_prepare() for virtio as NOP for now?
Hi Konstantin,
I'd keep it as it is for now. It should be a trivial patch after all, that
I could provide it when everything are settled down.
--yliu
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Ananyev, Konstantin @ 2016-12-07 10:13 UTC (permalink / raw)
To: Yuanhan Liu, Yigit, Ferruh, Olivier Matz
Cc: Thomas Monjalon, dev@dpdk.org, Jan Medala, Jakub Palider,
Netanel Belgazal, Evgeny Schemeilin, Alejandro Lucero, Yong Wang,
Andrew Rybchenko, Hemant Agrawal, Kulasek, TomaszX
In-Reply-To: <20161207081144.GC31182@yliu-dev.sh.intel.com>
Hi Yliu,
>
> On Tue, Dec 06, 2016 at 03:53:42PM +0000, Ferruh Yigit wrote:
> > > Please, we need a comment for each driver saying
> > > "it is OK, we do not need any checksum preparation for TSO"
> > > or
> > > "yes we have to implement tx_prepare or TSO will not work in this mode"
> > >
>
> Sorry for late. For virtio, I think it's not a must. The checksum stuff
> has been handled inside the Tx function. However, we may could move it
> to tx_prepare, which would actually recover the performance lost
> introduced while enabling TSO for the non-TSO case.
>
So would you like to provide a patch for it,
Or would you like to keep tx_prepare() for virtio as NOP for now?
Thanks
Konstantin
^ permalink raw reply
* Re: [PATCH] vhost: allow for many vhost user ports
From: Yuanhan Liu @ 2016-12-07 10:12 UTC (permalink / raw)
To: Jan Wickbom; +Cc: dev, patrik.r.andersson
In-Reply-To: <1480606010-6132-1-git-send-email-jan.wickbom@ericsson.com>
On Thu, Dec 01, 2016 at 04:26:50PM +0100, Jan Wickbom wrote:
> static int
> -fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
> +fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
> {
> struct fdentry *pfdentry;
> - int i, maxfds = -1;
> - int num = MAX_FDS;
> -
> - if (pfdset == NULL)
> - return -1;
> + int i;
> + int num;
>
> - for (i = 0; i < num; i++) {
> + for (i = 0, num = pfdset->num; i < num; i++) {
> pfdentry = &pfdset->fd[i];
> - if (pfdentry->fd != -1) {
> - int added = 0;
> - if (pfdentry->rcb && rfset) {
> - FD_SET(pfdentry->fd, rfset);
> - added = 1;
> - }
> - if (pfdentry->wcb && wfset) {
> - FD_SET(pfdentry->fd, wfset);
> - added = 1;
> - }
> - if (added)
> - maxfds = pfdentry->fd < maxfds ?
> - maxfds : pfdentry->fd;
> +
> + if (pfdentry->fd < 0) {
> + /* Hole in the list. Move the last one here */
> +
> + *pfdentry = pfdset->fd[num - 1];
> + pfdset->fd[num - 1].fd = -1;
> + num = fdset_adjust_num(pfdset);
> }
> + rwfds[i].fd = pfdentry->fd;
> + rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
> + rwfds[i].events |= pfdentry->wcb ? POLLOUT : 0;
Another thing is we don't have to re-init this rwfds array again
and again. Instead, we could
- set it up correctly when fdset_add is invoked: set the fd and
events.
- reset revents when it's been handled at fdset_event_dispatch().
- swap with the last one and shrink the array on fd delete
Could you make a follow up patch for that?
Thanks.
--yliu
^ permalink raw reply
* Re: [PATCH 10/32] net/dpaa2: introducing dpaa2 bus driver for fsl-mc bus
From: Shreyansh Jain @ 2016-12-07 10:13 UTC (permalink / raw)
To: Hemant Agrawal, dev
Cc: thomas.monjalon, bruce.richardson, david.marchand@6wind.com
In-Reply-To: <1480875447-23680-11-git-send-email-hemant.agrawal@nxp.com>
+ CC: David Marchand
On Sunday 04 December 2016 11:47 PM, Hemant Agrawal wrote:
> The DPAA2 bus driver is a rte_bus driver which scans the fsl-mc bus.
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> drivers/net/Makefile | 2 +-
> drivers/net/dpaa2/Makefile | 60 ++++++++++++++
> drivers/net/dpaa2/dpaa2_bus.c | 99 +++++++++++++++++++++++
> drivers/net/dpaa2/rte_dpaa2.h | 121 ++++++++++++++++++++++++++++
> drivers/net/dpaa2/rte_pmd_dpaa2_version.map | 4 +
> mk/rte.app.mk | 1 +
> 6 files changed, 286 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/dpaa2/Makefile
> create mode 100644 drivers/net/dpaa2/dpaa2_bus.c
> create mode 100644 drivers/net/dpaa2/rte_dpaa2.h
> create mode 100644 drivers/net/dpaa2/rte_pmd_dpaa2_version.map
>
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index bc93230..2bcf67b 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -55,7 +55,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD) += thunderx
> DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
> DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
> DIRS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += xenvirt
> -
> +DIRS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += dpaa2
> ifeq ($(CONFIG_RTE_LIBRTE_VHOST),y)
> DIRS-$(CONFIG_RTE_LIBRTE_PMD_VHOST) += vhost
> endif # $(CONFIG_RTE_LIBRTE_VHOST)
> diff --git a/drivers/net/dpaa2/Makefile b/drivers/net/dpaa2/Makefile
> new file mode 100644
> index 0000000..a99ce22
> --- /dev/null
> +++ b/drivers/net/dpaa2/Makefile
> @@ -0,0 +1,60 @@
> +# BSD LICENSE
> +#
> +# Copyright (c) 2016 NXP. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in
> +# the documentation and/or other materials provided with the
> +# distribution.
> +# * Neither the name of NXP nor the names of its
> +# contributors may be used to endorse or promote products derived
> +# from this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +#
> +# library name
> +#
> +LIB = librte_pmd_dpaa2.a
> +
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +
> +CFLAGS += -I$(RTE_SDK)/drivers/net/dpaa2
> +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/
> +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal
> +
> +# versioning export map
> +EXPORT_MAP := rte_pmd_dpaa2_version.map
> +
> +# library version
> +LIBABIVER := 1
> +
> +
> +# Interfaces with DPDK
> +SRCS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += dpaa2_bus.c
> +
> +# library dependencies
> +DEPDIRS-y += lib/librte_eal
> +DEPDIRS-y += drivers/common/dpaa/mc
> +DEPDIRS-y += drivers/common/dpaa/qbman
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/net/dpaa2/dpaa2_bus.c b/drivers/net/dpaa2/dpaa2_bus.c
> new file mode 100644
> index 0000000..571066c
> --- /dev/null
> +++ b/drivers/net/dpaa2/dpaa2_bus.c
> @@ -0,0 +1,99 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright (c) 2016 NXP. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of NXP nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <string.h>
> +#include <dirent.h>
> +
> +#include <rte_log.h>
> +#include <rte_bus.h>
> +#include <rte_dpaa2.h>
> +#include <rte_eal_memconfig.h>
> +#include <rte_malloc.h>
> +#include <rte_devargs.h>
> +#include <rte_memcpy.h>
> +#include <rte_ethdev.h>
> +
> +#include "eal_filesystem.h"
> +#include "eal_private.h"
> +
> +void
> +rte_dpaa2_register(struct rte_dpaa2_driver *driver)
> +{
> + struct rte_bus *bus;
> +
> + bus = rte_eal_get_bus("dpaa2");
> + if (!bus) {
> + RTE_LOG(ERR, EAL, "DPAA2 bus not registered\n");
> + return;
> + }
> +
> + rte_eal_bus_add_driver(bus, &driver->driver);
> +}
> +
> +void
> +rte_dpaa2_unregister(struct rte_dpaa2_driver *driver)
> +{
> + struct rte_bus *bus;
> +
> + bus = driver->driver.bus;
> + if (!bus) {
> + RTE_LOG(ERR, EAL, "Unable to find bus for device\n");
> + return;
> + }
> +
> + rte_eal_bus_remove_driver(&driver->driver);
> +}
> +
> +int rte_dpaa2_probe(struct rte_driver *driver __rte_unused,
> + struct rte_device *device __rte_unused)
> +{
> + return 0;
> +}
> +
> +int rte_dpaa2_scan(struct rte_bus *bus_d __rte_unused)
> +{
> + return 0;
> +}
> +
> +int rte_dpaa2_match(struct rte_driver *driver __rte_unused,
> + struct rte_device *device __rte_unused)
> +{
> + return 0;
> +}
> +
> +struct rte_bus dpaa2_bus = {
> + .scan = rte_dpaa2_scan,
> + .match = rte_dpaa2_match,
> + .probe = rte_dpaa2_probe,
> +};
> +
> +RTE_REGISTER_BUS(dpaa2, dpaa2_bus);
> diff --git a/drivers/net/dpaa2/rte_dpaa2.h b/drivers/net/dpaa2/rte_dpaa2.h
> new file mode 100644
> index 0000000..b36eed8
> --- /dev/null
> +++ b/drivers/net/dpaa2/rte_dpaa2.h
> @@ -0,0 +1,121 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright (c) 2016 NXP. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of NXP nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTE_DPAA2_H_
> +#define _RTE_DPAA2_H_
> +
> +/**
> + * @file
> + *
> + * RTE DPAA2 Interface
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <sys/queue.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +
> +#include <rte_debug.h>
> +#include <rte_interrupts.h>
> +#include <rte_dev.h>
> +
> +
> +struct rte_dpaa2_driver;
> +/**
> + * A structure describing a DPAA2 device.
> + */
> +struct rte_dpaa2_device {
> + TAILQ_ENTRY(rte_dpaa2_device) next; /**< Next probed DPAA2 device. */
> + struct rte_device device; /**< Inherit core device */
> + uint16_t dev_type; /**< Device Type */
> + uint16_t object_id; /**< DPAA2 Object ID */
> + struct rte_intr_handle intr_handle; /**< Interrupt handle */
> + struct rte_dpaa2_driver *driver; /**< Associated driver */
> +};
> +
> +/**
> + * A structure describing a DPAA2 driver.
> + */
> +struct rte_dpaa2_driver {
> + TAILQ_ENTRY(rte_dpaa2_driver) next; /**< Next in list. */
> + struct rte_driver driver; /**< Inherit core driver. */
> + uint32_t drv_flags; /**< Flags contolling handling of device. */
> +};
> +
> +/**
> + * Register a DPAA2 driver.
> + *
> + * @param driver
> + * A pointer to a rte_dpaa2_driver structure describing the driver
> + * to be registered.
> + */
> +void rte_dpaa2_register(struct rte_dpaa2_driver *driver);
> +
> +/**
> + * Unregister a DPAA2 driver.
> + *
> + * @param driver
> + * A pointer to a rte_dpaa2_driver structure describing the driver
> + * to be unregistered.
> + */
> +void rte_dpaa2_unregister(struct rte_dpaa2_driver *driver);
> +
> +/**
> + *
> + */
> +int rte_dpaa2_probe(struct rte_driver *driver, struct rte_device *device);
> +int rte_dpaa2_match(struct rte_driver *driver, struct rte_device *device);
> +int rte_dpaa2_scan(struct rte_bus *bus);
> +
> +/** Helper for DPAA2 device registration from driver (eth, crypto) instance */
> +#define RTE_PMD_REGISTER_DPAA2(nm, dpaa2_drv) \
> +RTE_INIT(dpaa2initfn_ ##nm); \
> +static void dpaa2initfn_ ##nm(void) \
> +{\
> + (dpaa2_drv).driver.name = RTE_STR(nm);\
> + rte_dpaa2_register(&dpaa2_drv); \
> +} \
> +RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_DPAA2_H_ */
> diff --git a/drivers/net/dpaa2/rte_pmd_dpaa2_version.map b/drivers/net/dpaa2/rte_pmd_dpaa2_version.map
> new file mode 100644
> index 0000000..31eca32
> --- /dev/null
> +++ b/drivers/net/dpaa2/rte_pmd_dpaa2_version.map
> @@ -0,0 +1,4 @@
> +DPDK_17.02 {
> +
> + local: *;
> +};
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index f75f0e2..9e1c17c 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -101,6 +101,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE) += -lrte_cfgfile
>
> _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BOND) += -lrte_pmd_bond
> _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += -lrte_pmd_xenvirt -lxenstore
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += -lrte_pmd_dpaa2 -ldpaa2_mc -ldpaa2_qbman
>
> ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),n)
> # plugins (link only if static libraries)
>
IMO, the way Bus is kept is debatable.
- should it be in EAL (lib/librte_eal/linuxapp/eal_pci.c like Bus
patches) [1]?
- Should it a 'handler/driver' parallel to device drivers?
I personally prefer a clean layer for buses with:
- RTE_SDK/drivers/net/dpaa2/
- RTE_SDK/drivers/bus
- RTE_SDK/drivers/bus/dpaa2/
- RTE_SDK/drivers/bus/dpaa2/dpaa2_bus.c etc.
For PCI, which is generic (or for other similar generic buses, like
platform), we can keep the implementation within lib/librte_eal/linuxapp/*.
What do other think about this?
[1] http://dpdk.org/ml/archives/dev/2016-December/051359.html
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Adrien Mazarguil @ 2016-12-07 10:08 UTC (permalink / raw)
To: Ananyev, Konstantin
Cc: Thomas Monjalon, dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd,
Jan Medala, Jakub Palider, John Daley, Alejandro Lucero,
Harish Patil, Rasesh Mody, Jerin Jacob, Yuanhan Liu, Yong Wang,
Kulasek, TomaszX, olivier.matz@6wind.com
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E4C13@irsmsx105.ger.corp.intel.com>
On Tue, Dec 06, 2016 at 08:31:35PM +0000, Ananyev, Konstantin wrote:
> > Hi Konstantin,
> >
> > On Tue, Dec 06, 2016 at 10:56:26AM +0000, Ananyev, Konstantin wrote:
> > >
> > > Hi Adrien,
> > >
> > > >
> > > > On Mon, Dec 05, 2016 at 04:43:52PM +0000, Ananyev, Konstantin wrote:
> > > > [...]
> > > > > > On Fri, Dec 02, 2016 at 01:00:55AM +0000, Ananyev, Konstantin wrote:
> > > > > > [...]
> > > > > > > > On Wed, Nov 30, 2016 at 10:54:50AM +0000, Ananyev, Konstantin wrote:
> > > > > > > > [...]
> > > > > > > > > Do you have anything particular in mind here?
> > > > > > > >
> > > > > > > > Nothing in particular, so for the sake of the argument, let's suppose that I
> > > > > > > > would like to add a field to expose some limitation that only applies to my
> > > > > > > > PMD during TX but looks generic enough to make sense, e.g. maximum packet
> > > > > > > > size when VLAN tagging is requested.
> > > > > > >
> > > > > > > Hmm, I didn't hear about such limitations so far, but if it is real case -
> > > > > > > sure, feel free to submit the patch.
> > > > > >
> > > > > > I won't, that was hypothetical.
> > > > >
> > > > > Then why we discussing it? :)
> > > >
> > > > Just to make a point, which is that new limitations may appear anytime and
> > > > tx_prepare() can now be used to check for them. First patch of the series
> > > > does it:
> > > >
> > > > + 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 */
> > > >
> > > > And states that:
> > > >
> > > > + * 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.
> > >
> > > I think I already explained in my previous email why I think that
> > > nb_seg_max and nb_mtu_seg_max are not redundant because of tx_prepare().
> > > From my point they are complement to tx_prepare():
> > > Even if people do use tx_prepare() they still should take this information into account.
> > > As an example ixgbe can't TX packets with then 40 segments.
> > > tx_prepare() for ixgbe will flag that issue, but it can't make a decision on user behalf
> > > what to do in that case: drop the packet, try to coalesce it into the packet with less
> > > number of segments, split the packet into several smaller, etc.
> > > That's up to user to make such decision, and to make it, user might need this information.
> >
> > Yet tx_prepare() has already the ability to update mbuf contents, issue is
> > what will this function do in the future, where will it stop? It is defined
> > in a way that each PMD does what it wants to make mbufs edible for
> > tx_burst(), because of this applications will just always call it to be on
> > the safe side.
> >
> > > > It's like making this function mandatory IMO.
> > >
> > > That's probably where confusion starts: I don't think that
> > > tx_prepare() should be mandatory for the user to call.
> > > Yes, it should be a recommended way.
> > > But the user still should have the ability to by-pass it,
> > > if he believes there is no need for it, or he prefers to implement
> > > the same functionality on his own.
> > > As an example, if the user knows that he is going to send a group
> > > of one-segment packets that don't require any tx offloads, he can safely skip
> > > tx_prepare() for them.
> >
> > I understand your point, and agree with the example you provide. Many
> > applications do not know what's inside mbufs though, except perhaps that
> > they contain TCP and may want to perform TSO because of that. Those will
> > have to call tx_prepare() to be future-proof.
> >
> > > > > > > > PMDs are free to set that field to some
> > > > > > > > special value (say, 0) if they do not care.
> > > > > > > >
> > > > > > > > Since that field exists however, conscious applications should check its
> > > > > > > > value for each packet that needs to be transmitted. This extra code causes a
> > > > > > > > slowdown just by sitting in the data path. Since it is not the only field in
> > > > > > > > that structure, the performance impact can be significant.
> > >
> > > Conscious user will probably use this information at the stage of packet formation.
> > > He probably has to do this sort of things for large packets anyway:
> > > Check what is the underlying mtu, to decide does he need to split the packet,
> > > or enable tso for it, etc.
> >
> > There are already too many things to check,
>
> There always been, that patch exposes them, before that upper layer probably
> had to use some hard-coded defines.
>
> > applications probably won't mind
> > a little help from PMDs. If we keep adding fields to this structure, we'll
> > have to provide some sort of PMD-specific function that checks what is
> > relevant.
>
> Why PMD specific?
> These fields are generic enough and could be used by upper layer to consult
> when packet is formed.
I've used the wrong term here, I meant a generic function provided by PMDs,
just like tx_prepare(). Some sort of generic tx_check(), whose functionality
is currently partially covered by tx_prepare() if I'm not mistaken.
> > Furthermore, assuming most packets are fine and do not require any extra
> > processing, what is rejected by tx_burst() could enter some unlikely() path
> > that attempts to rectify and re-send them. That would at least optimize the
> > common scenario.
>
> It is up to the upper layer to decide what to do with ill-formed packets:
> drop/log/try to cure/etc.
> Obviously different applications would have different logic and make different decisions here.
> If you'd like to introduce a new function (in rte_net or whatever) that would be smart and
> generic enough to cure ill-formed packets - you are more than welcome to try.
> Though discussion of such fallback function is far of scope of that patch, I believe.
I agree it's up to the upper layer, so just to clarify, I meant the
tx_burst() function could fail some check and not transmit the remaining
buffers, leaving the application to do whatever with them in some unlikely()
path:
// tx_prepare() has never been called
n = tx_burst(pkts, num);
if (unlikely(n < num)) {
n += recover_here(pkts + n, num - n);
}
> > > > > > > > Even though this code is inside applications, it remains unfair to PMDs for
> > > > > > > > which these tests are irrelevant. This problem is identified and addressed
> > > > > > > > by tx_prepare().
> > > > > > >
> > > > > > > I suppose the question is why do we need:
> > > > > > > uint16_t nb_seg_max;
> > > > > > > uint16_t nb_mtu_seg_max;
> > > > > > > as we now have tx_prepare(), right?
> > > > > > >
> > > > > > > For two reasons:
> > > > > > > 1. Some people might feel that tx_prepare() is not good (smart/fast) enough
> > > > > > > for them and would prefer to do necessary preparations for TX offloads themselves.
> > > > > > >
> > > > > > > 2. Even if people do use tx_prepare() they still should take this information into accout.
> > > > > > > As an example ixgbe can't TX packets with then 40 segments.
> > > > > > > Obviously ixbge_tx_prep() performs that check and returns an error.
> > > > > >
> > > > > > Problem is that tx_prepare() also provides safeties which are not part of
> > > > > > tx_burst(), such as not going over nb_mtu_seg_max. Because of this and the
> > > > > > fact struct rte_eth_desc_lim can grow new fields anytime, application
> > > > > > developers will be tempted to just call tx_prepare() and focus on more
> > > > > > useful things.
> > > > >
> > > > > NP with that, that was an intention beyond introducing it.
> > > > >
> > > > > > Put another way, from a user's point of view, tx_prepare() is an opaque
> > > > > > function that greatly increases tx_burst()'s ability to send mbufs as
> > > > > > requested, with extra error checking on top; applications not written to run
> > > > > > on a specific PMD/device (all of them ideally) will thus call tx_prepare()
> > > > > > at some point.
> > > > > >
> > > > > > > But it wouldn't try to merge/reallocate mbufs for you.
> > > > > > > User still has to do it himself, or just prevent creating such long chains somehow.
> > > > > >
> > > > > > Yes, that's another debate. PMDs could still implement a software fallback
> > > > > > for unlikely slow events like these. The number of PMDs is not going to
> > > > > > decrease, each device having its own set of weird limitations in specific
> > > > > > cases, PMDs should do their best to process mbufs even if that means slowly
> > > > > > due to the lack of preparation.
> > > > > >
> > > > > > tx_prepare() has its uses but should really be optional, in the sense that
> > > > > > if that function is not called, tx_burst() should deal with it somehow.
> > > > >
> > > > > As I said before, I don't think it is a good idea to put everything in tx_burst().
> > > > > If PMD driver prefer things that way, yes tx_burst() can deal with each and
> > > > > possible offload requirement itself, but it shouldn't be mandatory.
> > > >
> > > > In effect, having to call tx_prepare() otherwise makes this step mandatory
> > > > anyway. Looks like we are not going to agree here.
> > > >
> > > > > > > > Thanks to tx_prepare(), these checks are moved back into PMDs where they
> > > > > > > > belong. PMDs that do not need them do not have to provide support for
> > > > > > > > tx_prepare() and do not suffer any performance impact as result;
> > > > > > > > applications only have to make sure tx_prepare() is always called at some
> > > > > > > > point before tx_burst().
> > > > > > > >
> > > > > > > > Once you reach this stage, you've effectively made tx_prepare() mandatory
> > > > > > > > before tx_burst(). If some bug occurs, then perhaps you forgot to call
> > > > > > > > tx_prepare(), you just need to add it. The total cost for doing TX is
> > > > > > > > therefore tx_prepare() + tx_burst().
> > > > > > > >
> > > > > > > > I'm perhaps a bit pessimistic mind you, but I do not think tx_prepare() will
> > > > > > > > remain optional for long. Sure, PMDs that do not implement it do not care,
> > > > > > > > I'm focusing on applications, for which the performance impact of calling
> > > > > > > > tx_prepare() followed by tx_burst() is higher than a single tx_burst()
> > > > > > > > performing all the necessary preparation at once.
> > > > > > > >
> > > > > > > > [...]
> > > > > > > > > > Following the same logic, why can't such a thing be made part of the TX
> > > > > > > > > > burst function as well (through a direct call to rte_phdr_cksum_fix()
> > > > > > > > > > whenever necessary). From an application standpoint, what are the advantages
> > > > > > > > > > of having to:
> > > > > > > > > >
> > > > > > > > > > if (tx_prep()) // iterate and update mbufs as needed
> > > > > > > > > > tx_burst(); // iterate and send
> > > > > > > > > >
> > > > > > > > > > Compared to:
> > > > > > > > > >
> > > > > > > > > > tx_burst(); // iterate, update as needed and send
> > > > > > > > >
> > > > > > > > > I think that was discussed extensively quite a lot previously here:
> > > > > > > > > As Thomas already replied - main motivation is to allow user
> > > > > > > > > to execute them on different stages of packet TX pipeline,
> > > > > > > > > and probably on different cores.
> > > > > > > > > I think that provides better flexibility to the user to when/where
> > > > > > > > > do these preparations and hopefully would lead to better performance.
> > > > > > > >
> > > > > > > > And I agree, I think this use case is valid but does not warrant such a high
> > > > > > > > penalty when your application does not need that much flexibility. Simple
> > > > > > > > (yet conscious) applications need the highest performance. Complex ones as
> > > > > > > > you described already suffer quite a bit from IPCs and won't mind a couple
> > > > > > > > of extra CPU cycles right?
> > > > > > >
> > > > > > > It would mean an extra cache-miss for every packet, so I think performance hit
> > > > > > > would be quite significant.
> > > > > >
> > > > > > A performance hit has to occur somewhere regardless, because something has
> > > > > > to be done in order to send packets that need it. Whether this cost is in
> > > > > > application code or in a PMD function, it remains part of TX.
> > > > >
> > > > > Depending on the place the final cost would differ quite a lot.
> > > > > If you call tx_prepare() somewhere close to the place where you fill the packet header
> > > > > contents, then most likely the data that tx_prepare() has to access will be already in the cache.
> > > > > So the performance penalty will be minimal.
> > > > > If you'll try to access the same data later (at tx_burst), then the possibility that it would still
> > > > > be in cache is much less.
> > > > > If you calling tx_burst() from other core then data would for sure be out of cache,
> > > > > and even worse can still be in another core cache.
> > > >
> > > > Well sure, that's why I also think tx_prepare() has its uses, only that
> > > > since tx_prepare() is optional, tx_burst() should provide the same
> > > > functionality when tx_prepare() is not called.
> > >
> > > As I understand, to implement what you are proposing (TX_PREPARED mbuf->ol_flag)
> > > it will be required:
> > >
> > > a) Modify all existing applications that do similar to tx_prepare() stuff on their own,
> > > otherwise they'll would hit performance penalty.
> > > b) Modify at least all Intel PMDs and might be some others too (vmxnet3?).
> > >
> > > Step b) probably wouldn't cause any significant performance impact straightway,
> > > but it's for sure wouldn't make things faster, and would increase tx_burst() code
> > > complexity quite a lot.
> > > From other side, I can't see any real benefit that we will have in return.
> > > So I still opposed to that idea.
> >
> > Applications gain the ability to perform tx_burst() with offloads without
> > having to prepare anything.
>
> What means 'without preparing anything'?
> This is just not possible I think.
> One way or another application would have to to decide
> what exactly it likes to TX and what HW offloads it likes to use for it.
> So at least, it still needs to fill relevant mbuf fields:
> pkt_len, data_len, nb_segs, ol_flags, tx_offload, etc.
>
> >Currently these applications either cannot use
> > offloads at all or need to perform PMD-specific voodoo first.
>
> That's why tx_prepare() is introduced.
Yes, and the fact it appeared made me wonder why tx_burst() could not
implement its functionality as well. Why can't tx_burst() call
rte_net_intel_cksum_prepare() directly? I mean, unless offloads are
requested, this additional code should not impact performance.
> > The generic
> > alternative to this scenario being tx_prepare(), PMDs have to make this step
> > as cheap as possible.
> >
> > Yes that would slow down existing applications, people may find it
> > acceptable since we're modifying the TX API here.
> >
> > > > > > > About the 'simple' case when tx_prep() and tx_burst() are called on the same core,
> > > > > > > Why do you believe that:
> > > > > > > tx_prep(); tx_burst(); would be much slower than tx_burst() {tx_prep(), ...}?
> > > > > >
> > > > > > I mean instead of two function calls with their own loops:
> > > > > >
> > > > > > tx_prepare() { foreach (pkt) { check(); extra_check(); ... } }
> > > > > >
> > > > > > tx_burst() { foreach (pkt) { check(); stuff(); ... } }
> > > > > >
> > > > > > You end up with one:
> > > > > >
> > > > > > tx_burst() { foreach (pkt) { check(); extra_check(); stuff(); ... } }
> > > > > >
> > > > > > Which usually is more efficient.
> > > > >
> > > > > I really doubt that.
> > > > > If it would be that, what is the point to process packet in bulks?
> > > > > Usually dividing processing into different stages and at each stage processing
> > > > > multiple packet at once helps to improve performance.
> > > > > At least for IA.
> > > > > Look for example how we had to change l3fwd to improve its performance.
> > > >
> > > > Depends quite a bit on usage pattern. It is less efficient for applications
> > > > that do not modify mbuf contents because of the additional function call and
> > > > inner loop.
> > >
> > > If the application doesn't modify mbuf contents that it can simply skip calling tx_prepare().
> >
> > What if that same application wants to enable some offload as well?
>
> Hmm, wasn't that your use-case when no offloads (modifications) are required just 3 lines above?
Not exactly, this is a case where an application does not touch mbuf
contents yet wants to perform some HW offload on them. I'll concede this
scenario is likely rare enough for existing offloads.
What is likely not, though, is when an application discovers it can request
a particular offload right before calling tx_burst().
> > > > Note that I'm only pushing for the ability to conveniently address both
> > > > cases with maximum performance.
> > > >
> > > > > > > tx_prep() itself is quite expensive, let say for Intel HW it includes:
> > > > > > > - read mbuf fileds (2 cache-lines),
> > > > > > > - read packet header (1/2 cache-lines)
> > > > > > > - calculate pseudo-header csum
> > > > > > > - update packet header
> > > > > > > Comparing to that price of extra function call seems neglectable
> > > > > > > (if we TX packets in bursts of course).
> > > > > >
> > > > > > We agree its performance is a critical issue then, sharing half the read
> > > > > > steps with tx_burst() would make sense to me.
> > > > >
> > > > > I didn't understand that sentence.
> > > >
> > > > I meant this step can be shared (in addition to loop etc):
> > > >
> > > > - read mbuf fileds (2 cache-lines),
> > >
> > > Ah ok, you still believe that mixing tx_burst and tx_prepare code together
> > > would give us noticeable performance benefit.
> > > As I said above, I don't think it would, but you are welcome to try and
> > > prove me wrong.
> >
> > Depends on what you call noticeable. I guess we can at least agree having
> > two separate functions and loops cause more instructions to be generated and
> > executed.
> >
> > Now for the number of spent CPU cycles, depends of course whether mbufs are
> > still hot into the cache or not, and as I told you in my opinion we'll
> > usually see applications calling tx_prepare() just before tx_burst() to
> > benefit from offloads.
>
> Honestly, Adrien we are going in cycles here.
> Just to be clear:
> Current patch introduces tx_prepare() without affecting in any way:
> 1) existing applications
> 2) exiting PMD code (tx_burst)
> Meanwhile I still believe it is useful and provide a big step forward in terms
> of generalizing usage of HW TX offloads.
Yes and agreed. I do not oppose this new API on the basis that it's better
than what was available before.
> What you propose requires modifications for both existing applications and existing PMD code
> (full-featured tx_burst() for at least all Intel PMDs and vmxnet3 has to be significantly modified).
> You believe that with these modifications that new tx_burst() implementation
> would be noticeably faster than just current: tx_prepare(); tx_burst();
> I personally doubt that it really would (at least on modern IA).
> But as I said, you are more than welcome to prove me wrong here.
> Let say, provide a patch for ixgbe (or i40e) full-featured tx_burst() implementation,
> so that it would combine both tx_prepare() and tx_burst() functionalities into one function.
> Then we can run some performance tests with current and yours patches and compare results.
> Without that, I don't see any point to discuss your proposition any further.
> I just won't agree for such big change in existing PMDs without some solid justification beyond it.
I worry that proving you wrong would make you even stronger :), you raise
good points and I'm also a bit tired of this discussion. Without performance
numbers, my concerns are baseless.
Since non-Intel/vmxnet3 PMDs do not need tx_prepare() (yet), standardizing
on the case where it might be needed instead of assuming tx_burst() can do
all that work remains not better from a usability standpoint. It is done
because moving that stuff inside tx_burst() would be expensive even when
offloads are not requested, right?
> > > > > > > > Yes they will, therefore we need a method that satisfies both cases.
> > > > > > > >
> > > > > > > > As a possible solution, a special mbuf flag could be added to each mbuf
> > > > > > > > having gone through tx_prepare(). That way, tx_burst() could skip some
> > > > > > > > checks and things it would otherwise have done.
> > > > > > >
> > > > > > > That's an interesting idea, but it has one drawback:
> > > > > > > As I understand, it means that from now on if user doing preparations on his own,
> > > > > > > he had to setup this flag, otherwise tx_burst() would do extra unnecessary work.
> > > > > > > So any existing applications that using TX offloads and do preparation by themselves
> > > > > > > would have to be modified to avoid performance loss.
> > > > > >
> > > > > > In my opinion, users should not do preparation on their own.
> > > > >
> > > > > People already do it now.
> > > >
> > > > But we do not want them to anymore thanks to this new API, for reasons
> > > > described in the motivation section of the cover letter, right?
> > >
> > > We probably wouldn't recommend that, but if people would like to use their own stuff,
> > > or shortcuts - I don't want to stop them here.
> > >
> > > >
> > > > > > If we provide a
> > > > > > generic method, it has to be fast enough to replace theirs. Perhaps not as
> > > > > > fast since it would work with all PMDs (usual trade-off), but acceptably so.
> > > > > >
> > > > > > > > Another possibility, telling the PMD first that you always intend to use
> > > > > > > > tx_prepare() and getting a simpler/faster tx_burst() callback as a result.
> > > > > > >
> > > > > > > That what we have right now (at least for Intel HW):
> > > > > > > it is a user responsibility to do the necessary preparations/checks before calling tx_burst().
> > > > > > > With tx_prepare() we just remove from user the headache to implement tx_prepare() on his own.
> > > > > > > Now he can use a 'proper' PMD provided function.
> > > > > > >
> > > > > > > My vote still would be for that model.
> > > > > >
> > > > > > OK, then in a nutshell:
> > > > > >
> > > > > > 1. Users are not expected to perform preparation/checks themselves anymore,
> > > > > > if they do, it's their problem.
> > > > >
> > > > > I think we need to be backward compatible here.
> > > > > If the existing app doing what tx_prepare() supposed to do, it should keep working.
> > > >
> > > > It should work, only if they keep doing it as well as call tx_burst()
> > > > directly, they will likely get lower performance.
> > > >
> > > > > > 2. If configured through an API to be defined, tx_burst() can be split in
> > > > > > two and applications must call tx_prepare() at some point before
> > > > > > tx_burst().
> > > > > >
> > > > > > 3. Otherwise tx_burst() should perform the necessary preparation and checks
> > > > > > on its own by default (when tx_prepare() is not expected).
> > > > >
> > > > > As I said before, I don't think it should be mandatory for tx_burst() to do what tx_prepare() does.
> > > > > If some particular implementation of tx_burst() prefers to do things that way - that's fine.
> > > > > But it shouldn't be required to.
> > > >
> > > > You're right, however applications might find it convenient. I think most
> > > > will end up with something like the following:
> > > >
> > > > if (tx_prepare(pkts))
> > > > tx_burst(pkts));
> > >
> > > Looking at existing DPDK apps - most of them do use some sort of TX bufferization.
> > > So, even in a simplistic app it would probably be:
> > >
> > > tx_prepare(pkts);
> > > tx_buffer(pkts);
> >
> > We're down to my word against yours here I guess, to leave the choice to
> > application developers, we'd need to provide tx_prepare() and a simpler
> > tx_burst() as well as the ability to call tx_burst() directly and still get
> > offloads.
>
> From what I've seen, most DPDK libs/apps do buffer data packets for TX in one or another way:
> mtcp, warp17, seastar.
I was arguing most applications wouldn't call tx_prepare() far from
tx_burst() and that merging them would therefore make sense, which seems to
be the case in these examples. Obviously they're doing bufferization.
- warp17 would call tx_prepare() inside pkt_flush_tx_q() right before
tx_burst(), it wouldn't make sense elsewhere.
- mtcp has an interesting loop on tx_burst(), however again, tx_prepare()
would be performed inside dpdk_send_pkts(), not where mbufs are
populated.
- seastar seems to expose the ability to send bursts as well,
dpdk_qp::_send() would also call tx_prepare() before tx_burst().
> Not to mention sample apps.
> But ok, as said above, if you can prove that tx_burst() you are proposing is really much faster then
> tx_prepare(); tx_burst();
> I'll be happy to reconsider.
Nowhere I wrote "much" faster, I was just pointing out it would be more
efficient if the work was done in a single function, at least for the use
cases found in several examples you've provided.
Now if you tell me it cannot be significant, fine, but still.
> > > > > > 4. We probably still need some mbuf flag to mark mbufs that cannot be
> > > > > > modified, the refcount could also serve as a hint.
> > > > >
> > > > > If mbuf can't be modified, you probably just wouldn't call the function that supposed to do that,
> > > > > tx_prepare() in that case.
> > > >
> > > > I think it would be easier to document what offload flags may cause the
> > > > tx_burst() function to modify mbuf contents, so applications have the
> > > > ability to set or strip these flags on a mbuf basis. That way there is no
> > > > need to call tx_prepare() without knowing exactly what it's going to do.
> > >
> > > Not sure I understand what exactly do you propose in the last paragraph?
> >
> > That for each TX offload flag, we document whether preparation might cause a
> > mbuf to be written to during the tx_prepare()/tx_burst() phase. One of the
> > reasons for tx_prepare() being:
> >
> > 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.
> >
> > If we determine what offloads may cause mbuf contents to change (all of them
> > perhaps?), then applications can easily strip those flags from outgoing
> > "const" mbufs. Then it becomes acceptable for tx_burst() to modify mbuf
> > contents as per user request, which removes one reason to rely on
> > tx_prepare() for these.
>
> Hmm, I didn't get you here.
> If let say I don't need TX TCP cksum offload, why would I set this flag inside mbuf at first place?
> If I do expect PMD to do TX TCP cksum offload for me, then I have to set that flag,
> otherwise how PMD would know that I did require that offload?
Let me take an example, if you assume a cloned mbuf must be sent, pointed
data is not owned by the TX function, and software preparation for TX
offloads might cause side effects elsewhere, I'm not talking about the
changes performed by HW to the outgoing frame here.
One reason for tx_prepare() according to 4) is to make this clear; by
calling tx_prepare(), applications fully acknowledge and take responsibility
for possible side-effects.
Hence my suggestion, we could make this acceptable for tx_burst() by
documenting that requesting offloads may cause it to modify mbuf contents as
well.
Before you reply, note that I've neither acked nor nacked that series, and
intend to leave this to other people. So far it's received several acks
already so I guess it's probably ready for inclusion.
I'm not comfortable with it as an application developer because of the extra
API call that I think could be included in tx_burst(), but as a PMD
maintainer I do not mind. Might even prove useful someday if some
corner-case offload too expensive to fully handle in tx_burst() had to be
supported. An optional offload if you will.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH 11/32] net/dpaa2: add dpaa2 vfio support
From: Hemant Agrawal @ 2016-12-07 10:04 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, bruce.richardson, shreyansh.jain
In-Reply-To: <2366721.OpDVYN8WCp@xps13>
On 12/7/2016 2:08 PM, Thomas Monjalon wrote:
> 2016-12-07 12:30, Hemant Agrawal:
>> On 12/7/2016 2:34 AM, Thomas Monjalon wrote:
>>> 2016-12-04 23:47, Hemant Agrawal:
>>>> Add support for using VFIO for dpaa2 based fsl-mc bus.
>>>
>>> Why do we need so much special code for interfacing VFIO on fsl-mc?
>>> Can you reuse some code from EAL VFIO?
>>>
>>
>> fsl-mc VFIO scans the objects. So, it is slightly different.
>>
>> Even though I have tried to minimize changes and re-use the code from
>> EAL VFIO. It is still refactoring work, which we will take up little later.
>
> Do you mean you could re-use some EAL code but do not want to do it now?
> It sounds like something heard too many times earlier (from others).
> The experience tells we must not wait to do things right.
>
I meant that I was able to use some of the EAL VFIO functions. There are
some changes which I am planning to propose in eal vfio and in our driver.
I will do it in subsequent patchsets of current series shortly.
^ 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