* Re: [PATCH v3 2/7] vhost: get guest/host physical address mappings
From: linhaifeng @ 2016-11-29 3:10 UTC (permalink / raw)
To: dev
In-Reply-To: <1475998080-4644-3-git-send-email-yuanhan.liu@linux.intel.com>
在 2016/10/9 15:27, Yuanhan Liu 写道:
> + dev->nr_guest_pages = 0;
> + if (!dev->guest_pages) {
> + dev->max_guest_pages = 8;
> + dev->guest_pages = malloc(dev->max_guest_pages *
> + sizeof(struct guest_page));
> + }
> +
when to free guest_pages ?
^ permalink raw reply
* Re: [PATCH 2/4] eventdev: implement the northbound APIs
From: Jerin Jacob @ 2016-11-29 2:01 UTC (permalink / raw)
To: Eads, Gage
Cc: dev@dpdk.org, Richardson, Bruce, Van Haaren, Harry,
hemant.agrawal@nxp.com
In-Reply-To: <9184057F7FC11744A2107296B6B8EB1E01E33E96@FMSMSX108.amr.corp.intel.com>
On Mon, Nov 28, 2016 at 03:53:08PM +0000, Eads, Gage wrote:
> (Bruce's adviced heeded :))
>
> > > > >
> > > > > How would this check work? Wouldn't it prevent any core from
> > > > running the software scheduler in the centralized case?
> > > >
> > > > I guess you may not need RTE_EVENT_DEV_CAP here, instead need flag
> > > > for device configure here
> > > >
> > > > #define RTE_EVENT_DEV_CFG_DISTRIBUTED_SCHED (1ULL << 1)
> > > >
> > > > struct rte_event_dev_config config; config.event_dev_cfg =
> > > > RTE_EVENT_DEV_CFG_DISTRIBUTED_SCHED;
> > > > rte_event_dev_configure(.., &config);
> > > >
> > > > on the driver side on configure,
> > > > if (config.event_dev_cfg & RTE_EVENT_DEV_CFG_DISTRIBUTED_SCHED)
> > > > eventdev->schedule = NULL;
> > > > else // centralized case
> > > > eventdev->schedule = your_centrized_schedule_function;
> > > >
> > > > Does that work?
> > >
> > > Hm, I fear the API would give users the impression that they can select the
> > scheduling behavior of a given eventdev, when a software scheduler is more
> > likely to be either distributed or centralized -- not both.
> >
> > Even if it is capability flag then also it is per "device". Right ?
> > capability flag is more of read only too. Am i missing something here?
> >
>
> Correct, the capability flag I'm envisioning is per-device and read-only.
>
> > >
> > > What if we use the capability flag, and define rte_event_schedule() as the
> > scheduling function for centralized schedulers and rte_event_dequeue() as the
> > scheduling function for distributed schedulers? That way, the datapath could be
> > the simple dequeue -> process -> enqueue. Applications would check the
> > capability flag at configuration time to decide whether or not to launch an
> > lcore that calls rte_event_schedule().
> >
> > I am all for simple "dequeue -> process -> enqueue".
> > rte_event_schedule() added for SW scheduler only, now it may not make sense
> > to add one more check on top of "rte_event_schedule()" to see it is really need
> > or not in fastpath?
> >
>
> Yes, the additional check shouldn't be needed. In terms of the 'typical workflow' description, this is what I have in mind:
>
> *
> * An event driven based application has following typical workflow on fastpath:
> * \code{.c}
> * while (1) {
> *
> * rte_event_dequeue(...);
> *
> * (event processing)
> *
> * rte_event_enqueue(...);
> * }
> * \endcode
> *
> * The point at which events are scheduled to ports depends on the device. For
> * hardware devices, scheduling occurs asynchronously. Software schedulers can
> * either be distributed (each worker thread schedules events to its own port)
> * or centralized (a dedicated thread schedules to all ports). Distributed
> * software schedulers perform the scheduling in rte_event_dequeue(), whereas
> * centralized scheduler logic is located in rte_event_schedule(). The
> * RTE_EVENT_DEV_CAP_DISTRIBUTED_SCHED capability flag indicates whether a
> * device is centralized and thus needs a dedicated scheduling thread that
> * repeatedly calls rte_event_schedule().
Makes sense. I will change the existing schedule description to the
proposed one and add RTE_EVENT_DEV_CAP_DISTRIBUTED_SCHED capability flag
in v2.
Thanks Gage.
> *
> */
^ permalink raw reply
* Re: [PATCH 00/16] e1000 base code update
From: Lu, Wenzhuo @ 2016-11-29 0:30 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev@dpdk.org, Yigit, Ferruh
In-Reply-To: <1767220.91Wk0F26Z4@xps13>
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 28, 2016 7:34 PM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org; Yigit, Ferruh
> Subject: Re: [dpdk-dev] [PATCH 00/16] e1000 base code update
>
> 2016-11-25 12:58, Ferruh Yigit:
> > Can you also please send another patch to:
> > 1- add I219 to supported nics list
> > 2- announce new supported nic in release notes.
>
> Please update also the web site:
> http://dpdk.org/doc/nics
Didn't know that. How to update it? Thanks.
^ permalink raw reply
* Re: dpdk/vpp and cross-version migration for vhost
From: Thomas Monjalon @ 2016-11-28 22:18 UTC (permalink / raw)
To: Maxime Coquelin, Kavanagh, Mark B, Kevin Traynor, Yuanhan Liu
Cc: dev, Weglicki, MichalX, Michael S. Tsirkin, Stephen Hemminger,
qemu-devel, libvir-list, vpp-dev, Marc-André Lureau,
olivier.matz
In-Reply-To: <67cfb109-8729-916e-5535-80af620bafe5@redhat.com>
2016-11-28 16:28, Maxime Coquelin:
> On 11/24/2016 04:24 PM, Kavanagh, Mark B wrote:
> > DPDK v16.04 added support for vHost User TSO; as such, by default,
> > TSO is advertised to guest devices as an available feature during
> > feature negotiation with QEMU.
> > However, while the vHost user backend sets up the majority of the
> > mbuf fields that are required for TSO, there is still a reliance
> > on the associated DPDK application (i.e. in this case OvS-DPDK)
> > to set the remaining flags and/or offsets.
> > Since OvS-DPDK doesn't currently provide that functionality, it is
> > necessary to explicitly disable TSO; otherwise, undefined behaviour
> > will ensue.
>
> Thanks Mark for the clarification.
>
> In this case, maybe we could add a DPDK build option to disable Vhost's
> TSO support, that would be selected for OVS packages?
Why do you prefer a build-time option rather than the run-time config
with rte_vhost_feature_disable()? Because we need to lock the features?
Reminder: build-time configuration options are forbidden in DPDK for
such usage. It would prevent other applications from using the feature
in a given distribution, just because it is not implemented in OVS.
> Does that sound reasonable?
Maybe I'm missing something but I feel it is more reasonnable to implement
the missing code in OVS.
If something is missing in DPDK, do not hesitate to request or add more
helper functions.
^ permalink raw reply
* Re: [PATCH] net/ixgbe: ensure link status is updated
From: Laurent Hardy @ 2016-11-28 17:27 UTC (permalink / raw)
To: helin.zhang, konstantin.ananyev; +Cc: dev
In-Reply-To: <1479403792-11928-1-git-send-email-laurent.hardy@6wind.com>
Hello,
Is there anyone available to review this patch ?
regards,
Laurent
On 11/17/2016 06:29 PM, Laurent Hardy wrote:
> In case of link speed set to 1Gb at peer side (with autoneg or with
> defined speed) and cable not plugged-in when device is configured
> and started, then link status is not updated properly with new speed
> as no link setup is triggered.
>
> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link
> setup each time link_update() is triggered and current link status is
> down. When cable is plugged-in, link setup will be performed via
> ixgbe_setup_link().
>
> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++++++++++++
> drivers/net/ixgbe/ixgbe_ethdev.h | 1 +
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 52ebbe4..513d1d5 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2095,6 +2095,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
>
> /* set flag to update link status after init */
> intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
>
> /*
> * Initialize to TRUE. If any of Rx queues doesn't meet the bulk
> @@ -3117,8 +3118,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> struct rte_eth_link link, old;
> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
> + struct ixgbe_interrupt *intr =
> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> int link_up;
> int diag;
> + u32 speed = 0;
> + bool autoneg = false;
>
> link.link_status = ETH_LINK_DOWN;
> link.link_speed = 0;
> @@ -3128,6 +3133,19 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
>
> hw->mac.get_link_status = true;
>
> + if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) {
> + speed = hw->phy.autoneg_advertised;
> + if (!speed) {
> + ixgbe_get_link_capabilities(hw, &speed, &autoneg);
> + /* setup the highest link when no autoneg */
> + if (!autoneg) {
> + if (speed & IXGBE_LINK_SPEED_10GB_FULL)
> + speed = IXGBE_LINK_SPEED_10GB_FULL;
> + }
> + }
> + ixgbe_setup_link(hw, speed, true);
> + }
> +
> /* check if it needs to wait to complete, if lsc interrupt is enabled */
> if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0)
> diag = ixgbe_check_link(hw, &link_speed, &link_up, 0);
> @@ -3145,10 +3163,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
>
> if (link_up == 0) {
> rte_ixgbe_dev_atomic_write_link_status(dev, &link);
> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
> if (link.link_status == old.link_status)
> return -1;
> return 0;
> }
> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
> link.link_status = ETH_LINK_UP;
> link.link_duplex = ETH_LINK_FULL_DUPLEX;
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
> index e060c3d..9d335ba 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -43,6 +43,7 @@
> #define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0)
> #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1)
> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2)
> +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 3)
>
> /*
> * Defines that were not part of ixgbe_type.h as they are not used by the
^ permalink raw reply
* Re: [PATCH 2/4] eventdev: implement the northbound APIs
From: Eads, Gage @ 2016-11-28 15:53 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev@dpdk.org, Richardson, Bruce, Van Haaren, Harry,
hemant.agrawal@nxp.com
In-Reply-To: <20161122234331.GA20501@svelivela-lt.caveonetworks.com>
(Bruce's adviced heeded :))
> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Tuesday, November 22, 2016 5:44 PM
> To: Eads, Gage <gage.eads@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Van
> Haaren, Harry <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com
> Subject: Re: [dpdk-dev] [PATCH 2/4] eventdev: implement the northbound APIs
>
> On Tue, Nov 22, 2016 at 10:48:32PM +0000, Eads, Gage wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > Sent: Tuesday, November 22, 2016 2:00 PM
> > > To: Eads, Gage <gage.eads@intel.com>
> > > Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> > > Van Haaren, Harry <harry.van.haaren@intel.com>;
> > > hemant.agrawal@nxp.com
> > > Subject: Re: [dpdk-dev] [PATCH 2/4] eventdev: implement the
> > > northbound APIs
> > >
> > > On Tue, Nov 22, 2016 at 07:43:03PM +0000, Eads, Gage wrote:
> > > > > > > > > One open issue I noticed is the "typical workflow"
> > > > > description starting in > > rte_eventdev.h:204 conflicts with
> > > the > > centralized software PMD that Harry > > posted last week.
> > > > > Specifically, that PMD expects a single core to call the > >
> > > > > schedule function. We could extend the documentation to account
> > > for > > this > > alternative style of scheduler invocation, or
> > > discuss > > ways to make the software > > PMD work with the
> > > documented > > workflow. I prefer the former, but either way I >
> > > > think we > > ought to expose the scheduler's expected usage to
> > > the user -- > > perhaps > > through an RTE_EVENT_DEV_CAP flag?
> > > > > > > >
> > > > > > > > I prefer former too, you can propose the documentation
> > > > > change required for > > software PMD.
> > > > > >
> > > > > > Sure, proposal follows. The "typical workflow" isn't the
> > > most > > optimal by having a conditional in the fast-path, of
> > > course, but it > > demonstrates the idea simply.
> > > > > >
> > > > > > (line 204)
> > > > > > * An event driven based application has following typical
> > > > > workflow on > > fastpath:
> > > > > > * \code{.c}
> > > > > > * while (1) {
> > > > > > *
> > > > > > * if (dev_info.event_dev_cap &
> > > > > > * RTE_EVENT_DEV_CAP_DISTRIBUTED_SCHED)
> > > > > > * rte_event_schedule(dev_id);
> > > > >
> > > > > Yes, I like the idea of RTE_EVENT_DEV_CAP_DISTRIBUTED_SCHED.
> > > > > It can be input to application/subsystem to launch separate
> > > > > core(s) for schedule functions.
> > > > > But, I think, the "dev_info.event_dev_cap & > >
> > > RTE_EVENT_DEV_CAP_DISTRIBUTED_SCHED"
> > > > > check can be moved inside the implementation(to make the
> > > better > > decisions and avoiding consuming cycles on HW based
> schedulers.
> > > >
> > > > How would this check work? Wouldn't it prevent any core from
> > > running the software scheduler in the centralized case?
> > >
> > > I guess you may not need RTE_EVENT_DEV_CAP here, instead need flag
> > > for device configure here
> > >
> > > #define RTE_EVENT_DEV_CFG_DISTRIBUTED_SCHED (1ULL << 1)
> > >
> > > struct rte_event_dev_config config; config.event_dev_cfg =
> > > RTE_EVENT_DEV_CFG_DISTRIBUTED_SCHED;
> > > rte_event_dev_configure(.., &config);
> > >
> > > on the driver side on configure,
> > > if (config.event_dev_cfg & RTE_EVENT_DEV_CFG_DISTRIBUTED_SCHED)
> > > eventdev->schedule = NULL;
> > > else // centralized case
> > > eventdev->schedule = your_centrized_schedule_function;
> > >
> > > Does that work?
> >
> > Hm, I fear the API would give users the impression that they can select the
> scheduling behavior of a given eventdev, when a software scheduler is more
> likely to be either distributed or centralized -- not both.
>
> Even if it is capability flag then also it is per "device". Right ?
> capability flag is more of read only too. Am i missing something here?
>
Correct, the capability flag I'm envisioning is per-device and read-only.
> >
> > What if we use the capability flag, and define rte_event_schedule() as the
> scheduling function for centralized schedulers and rte_event_dequeue() as the
> scheduling function for distributed schedulers? That way, the datapath could be
> the simple dequeue -> process -> enqueue. Applications would check the
> capability flag at configuration time to decide whether or not to launch an
> lcore that calls rte_event_schedule().
>
> I am all for simple "dequeue -> process -> enqueue".
> rte_event_schedule() added for SW scheduler only, now it may not make sense
> to add one more check on top of "rte_event_schedule()" to see it is really need
> or not in fastpath?
>
Yes, the additional check shouldn't be needed. In terms of the 'typical workflow' description, this is what I have in mind:
*
* An event driven based application has following typical workflow on fastpath:
* \code{.c}
* while (1) {
*
* rte_event_dequeue(...);
*
* (event processing)
*
* rte_event_enqueue(...);
* }
* \endcode
*
* The events are injected to event device through the *enqueue* operation by
* event producers in the system. The typical event producers are ethdev
* subsystem for generating packet events, core(SW) for generating events based
* on different stages of application processing, cryptodev for generating
* crypto work completion notification etc
*
* The *dequeue* operation gets one or more events from the event ports.
* The application process the events and send to downstream event queue through
* rte_event_enqueue() if it is an intermediate stage of event processing, on
* the final stage, the application may send to different subsystem like ethdev
* to send the packet/event on the wire using ethdev rte_eth_tx_burst() API.
*
* The point at which events are scheduled to ports depends on the device. For
* hardware devices, scheduling occurs asynchronously. Software schedulers can
* either be distributed (each worker thread schedules events to its own port)
* or centralized (a dedicated thread schedules to all ports). Distributed
* software schedulers perform the scheduling in rte_event_dequeue(), whereas
* centralized scheduler logic is located in rte_event_schedule(). The
* RTE_EVENT_DEV_CAP_DISTRIBUTED_SCHED capability flag indicates whether a
* device is centralized and thus needs a dedicated scheduling thread that
* repeatedly calls rte_event_schedule().
*
*/
^ permalink raw reply
* Re: dpdk/vpp and cross-version migration for vhost
From: Maxime Coquelin @ 2016-11-28 15:28 UTC (permalink / raw)
To: Kavanagh, Mark B, Kevin Traynor, Yuanhan Liu, Weglicki, MichalX
Cc: Michael S. Tsirkin, dev@dpdk.org, Stephen Hemminger,
qemu-devel@nongnu.org, libvir-list@redhat.com,
vpp-dev@lists.fd.io, Marc-André Lureau
In-Reply-To: <DC5AD7FA266D86499789B1BCAEC715F85C75B841@irsmsx105.ger.corp.intel.com>
On 11/24/2016 04:24 PM, Kavanagh, Mark B wrote:
>>
>> On 11/24/2016 12:47 PM, Maxime Coquelin wrote:
>>>
>>>
>>> On 11/24/2016 01:33 PM, Yuanhan Liu wrote:
>>>> On Thu, Nov 24, 2016 at 09:30:49AM +0000, Kevin Traynor wrote:
>>>>>> On 11/24/2016 06:31 AM, Yuanhan Liu wrote:
>>>>>>>> On Tue, Nov 22, 2016 at 04:53:05PM +0200, Michael S. Tsirkin wrote:
>>>>>>>>>>>>>> You keep assuming that you have the VM started first and
>>>>>>>>>>>>>> figure out things afterwards, but this does not work.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Think about a cluster of machines. You want to start a VM in
>>>>>>>>>>>>>> a way that will ensure compatibility with all hosts
>>>>>>>>>>>>>> in a cluster.
>>>>>>>>>>>>
>>>>>>>>>>>> I see. I was more considering about the case when the dst
>>>>>>>>>>>> host (including the qemu and dpdk combo) is given, and
>>>>>>>>>>>> then determine whether it will be a successfull migration
>>>>>>>>>>>> or not.
>>>>>>>>>>>>
>>>>>>>>>>>> And you are asking that we need to know which host could
>>>>>>>>>>>> be a good candidate before starting the migration. In such
>>>>>>>>>>>> case, we indeed need some inputs from both the qemu and
>>>>>>>>>>>> vhost-user backend.
>>>>>>>>>>>>
>>>>>>>>>>>> For DPDK, I think it could be simple, just as you said, it
>>>>>>>>>>>> could be either a tiny script, or even a macro defined in
>>>>>>>>>>>> the source code file (we extend it every time we add a
>>>>>>>>>>>> new feature) to let the libvirt to read it. Or something
>>>>>>>>>>>> else.
>>>>>>>>>>
>>>>>>>>>> There's the issue of APIs that tweak features as Maxime
>>>>>>>>>> suggested.
>>>>>>>>
>>>>>>>> Yes, it's a good point.
>>>>>>>>
>>>>>>>>>> Maybe the only thing to do is to deprecate it,
>>>>>>>>
>>>>>>>> Looks like so.
>>>>>>>>
>>>>>>>>>> but I feel some way for application to pass info into
>>>>>>>>>> guest might be benefitial.
>>>>>>>>
>>>>>>>> The two APIs are just for tweaking feature bits DPDK supports
>>>>>> before
>>>>>>>> any device got connected. It's another way to disable some features
>>>>>>>> (the another obvious way is to through QEMU command lines).
>>>>>>>>
>>>>>>>> IMO, it's bit handy only in a case like: we have bunch of VMs.
>>>>>> Instead
>>>>>>>> of disabling something though qemu one by one, we could disable it
>>>>>>>> once in DPDK.
>>>>>>>>
>>>>>>>> But I doubt the useful of it. It's only used in DPDK's vhost
>>>>>> example
>>>>>>>> after all. Nor is it used in vhost pmd, neither is it used in OVS.
>>>>>>
>>>>>> rte_vhost_feature_disable() is currently used in OVS,
>>>>> lib/netdev-dpdk.c
>>>> Hmmm. I must have checked very old code ...
>>>>>>
>>>>>> netdev_dpdk_vhost_class_init(void)
>>>>>> {
>>>>>> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>>>>>>
>>>>>> /* This function can be called for different classes. The
>>>>>> initialization
>>>>>> * needs to be done only once */
>>>>>> if (ovsthread_once_start(&once)) {
>>>>>> rte_vhost_driver_callback_register(&virtio_net_device_ops);
>>>>>> rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4
>>>>>> | 1ULL << VIRTIO_NET_F_HOST_TSO6
>>>>>> | 1ULL << VIRTIO_NET_F_CSUM);
>>>> I saw the commit introduced such change, but it tells no reason why
>>>> it was added.
>>>
>>> I'm also interested to know the reason.
>>
>> I can't remember off hand, added Mark K or Michal W who should be able
>> to shed some light on it.
>
> DPDK v16.04 added support for vHost User TSO; as such, by default, TSO is advertised to guest devices as an available feature during feature negotiation with QEMU.
> However, while the vHost user backend sets up the majority of the mbuf fields that are required for TSO, there is still a reliance on the associated DPDK application (i.e. in this case OvS-DPDK) to set the remaining flags and/or offsets. Since OvS-DPDK doesn't currently provide that functionality, it is necessary to explicitly disable TSO; otherwise, undefined behaviour will ensue.
Thanks Mark for the clarification.
In this case, maybe we could add a DPDK build option to disable Vhost's
TSO support, that would be selected for OVS packages?
Does that sound reasonable?
Cheers,
Maxime
>>
>>> In any case, I think this is something that can/should be managed by
>>> the management tool, which should disable it in cmd parameters.
>>>
>>> Kevin, do you agree?
>>
>> I think best to find out the reason first. Because if no reason to
>> disable in the code, then no need to debate!
>>
>>>
>>> Cheers,
>>> Maxime
>
^ permalink raw reply
* Re: [PATCH] scripts: fix checkpatch from standard input
From: Olivier Matz @ 2016-11-28 15:21 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
In-Reply-To: <1479768161-6123-1-git-send-email-thomas.monjalon@6wind.com>
On Mon, 21 Nov 2016 23:42:41 +0100, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> When checking a valid patch from standard input,
> the footer lines of the report are not filtered out.
>
> The function check is called outside of any loop,
> so the statement continue has no effect and the footer is printed.
>
> Fixes: 8005feef421d ("scripts: add standard input to checkpatch")
>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
The 'continue' statement is not always without effect. On my machine
(but it looks it's not the same everywhere):
- with dash, the 'continue' acts like a return in that case
- with bash, it displays an error:
"continue: only meaningful in a `for', `while', or `until' loop"
- with bash --posix, the 'continue' is ignored...
In my case, checkpatches.sh was displaying "0/1 valid" although there
was no error. This patch solves the issue, thanks.
Acked-by: Olivier Matz <olivier.matz@6wind.com>
^ permalink raw reply
* Re: [PATCH] doc: introduce PVP reference benchmark
From: Thomas Monjalon @ 2016-11-28 14:15 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: yuanhan.liu, john.mcnamara, zhiyong.yang, dev, fbaudin
In-Reply-To: <a85125cc-2908-c802-f099-1170416a90bd@redhat.com>
2016-11-28 15:02, Maxime Coquelin:
>
> On 11/28/2016 12:22 PM, Thomas Monjalon wrote:
> > 2016-11-23 22:00, Maxime Coquelin:
> >> +You can use this qmp-vcpu-pin script to pin vCPUs:
> >> +
> >> + .. code-block:: python
> >> +
> >> + #!/usr/bin/python
> >> + # QEMU vCPU pinning tool
> >> + #
> >> + # Copyright (C) 2016 Red Hat Inc.
> >> + #
> >> + # Authors:
> >> + # Maxime Coquelin <maxime.coquelin@redhat.com>
> >> + #
> >> + # This work is licensed under the terms of the GNU GPL, version 2. See
> >> + # the COPYING file in the top-level directory
> >> + import argparse
> >> + import json
> >> + import os
> >> +
> >> + from subprocess import call
> >> + from qmp import QEMUMonitorProtocol
> >> +
> >> + pinned = []
> >> +
> >> + parser = argparse.ArgumentParser(description='Pin QEMU vCPUs to physical CPUs')
> >> + parser.add_argument('-s', '--server', type=str, required=True,
> >> + help='QMP server path or address:port')
> >> + parser.add_argument('cpu', type=int, nargs='+',
> >> + help='Physical CPUs IDs')
> >> + args = parser.parse_args()
> >> +
> >> + devnull = open(os.devnull, 'w')
> >> +
> >> + srv = QEMUMonitorProtocol(args.server)
> >> + srv.connect()
> >> +
> >> + for vcpu in srv.command('query-cpus'):
> >> + vcpuid = vcpu['CPU']
> >> + tid = vcpu['thread_id']
> >> + if tid in pinned:
> >> + print 'vCPU{}\'s tid {} already pinned, skipping'.format(vcpuid, tid)
> >> + continue
> >> +
> >> + cpuid = args.cpu[vcpuid % len(args.cpu)]
> >> + print 'Pin vCPU {} (tid {}) to physical CPU {}'.format(vcpuid, tid, cpuid)
> >> + try:
> >> + call(['taskset', '-pc', str(cpuid), str(tid)], stdout=devnull)
> >> + pinned.append(tid)
> >> + except OSError:
> >> + print 'Failed to pin vCPU{} to CPU{}'.format(vcpuid, cpuid)
> >>
> >
> >
> > No please do not introduce such useful script in a doc.
> > I think it must be a separate file in the DPDK repository or
> > in the QEMU repository.
>
> Ok. The patch is under review on Qemu ML.
> While it gets merged, I can add a link to its patchwork ID.
> Ok for you?
Perfect, thanks
^ permalink raw reply
* Re: [PATCH] doc: introduce PVP reference benchmark
From: Maxime Coquelin @ 2016-11-28 14:02 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: yuanhan.liu, john.mcnamara, zhiyong.yang, dev, fbaudin
In-Reply-To: <2842042.7AjAPCMeNN@xps13>
On 11/28/2016 12:22 PM, Thomas Monjalon wrote:
> 2016-11-23 22:00, Maxime Coquelin:
>> +You can use this qmp-vcpu-pin script to pin vCPUs:
>> +
>> + .. code-block:: python
>> +
>> + #!/usr/bin/python
>> + # QEMU vCPU pinning tool
>> + #
>> + # Copyright (C) 2016 Red Hat Inc.
>> + #
>> + # Authors:
>> + # Maxime Coquelin <maxime.coquelin@redhat.com>
>> + #
>> + # This work is licensed under the terms of the GNU GPL, version 2. See
>> + # the COPYING file in the top-level directory
>> + import argparse
>> + import json
>> + import os
>> +
>> + from subprocess import call
>> + from qmp import QEMUMonitorProtocol
>> +
>> + pinned = []
>> +
>> + parser = argparse.ArgumentParser(description='Pin QEMU vCPUs to physical CPUs')
>> + parser.add_argument('-s', '--server', type=str, required=True,
>> + help='QMP server path or address:port')
>> + parser.add_argument('cpu', type=int, nargs='+',
>> + help='Physical CPUs IDs')
>> + args = parser.parse_args()
>> +
>> + devnull = open(os.devnull, 'w')
>> +
>> + srv = QEMUMonitorProtocol(args.server)
>> + srv.connect()
>> +
>> + for vcpu in srv.command('query-cpus'):
>> + vcpuid = vcpu['CPU']
>> + tid = vcpu['thread_id']
>> + if tid in pinned:
>> + print 'vCPU{}\'s tid {} already pinned, skipping'.format(vcpuid, tid)
>> + continue
>> +
>> + cpuid = args.cpu[vcpuid % len(args.cpu)]
>> + print 'Pin vCPU {} (tid {}) to physical CPU {}'.format(vcpuid, tid, cpuid)
>> + try:
>> + call(['taskset', '-pc', str(cpuid), str(tid)], stdout=devnull)
>> + pinned.append(tid)
>> + except OSError:
>> + print 'Failed to pin vCPU{} to CPU{}'.format(vcpuid, cpuid)
>>
>
>
> No please do not introduce such useful script in a doc.
> I think it must be a separate file in the DPDK repository or
> in the QEMU repository.
Ok. The patch is under review on Qemu ML.
While it gets merged, I can add a link to its patchwork ID.
Ok for you?
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH v2] mempool: remove a redundant word "for" in comment
From: Olivier Matz @ 2016-11-28 13:25 UTC (permalink / raw)
To: Wei Zhao; +Cc: dev
In-Reply-To: <20161128094212.135ce16c@platinum>
Hi Wei,
On Mon, 28 Nov 2016 09:42:12 +0100
Olivier Matz <olivier.matz@6wind.com> wrote:
> Hi Wenzhuo,
First, sorry for the mistake in your name my previous mail.
Please find below some other comments about the patch (on the form).
> On Sun, 27 Nov 2016 10:43:47 +0800
> Wei Zhao <wei.zhao1@intel.com> wrote:
>
> > From: zhao wei <wei.zhao1@intel.com>
> >
> > There is a redundant repetition word "for" in commnet line of the
commnet -> comment
> > file rte_mempool.h after the definition of RTE_MEMPOOL_OPS_NAMESIZE.
> > The word "for"appear twice in line 359 and 360.One of them is
Missing space after '"for"' and after '360.'
> > redundant, so delete it.
> >
> > Fixes: 449c49b93a6b (" mempool: support handler operations")
We should have an empty line after the 'Fixes:' tag. The
check-git-log.sh can help you to notice these errors.
Also, it is important that no spaces are added in the title of the
commit. You can get the exact line with:
git log -1 --abbrev=12 --format='Fixes: %h (\"%s\")' <commitid>
> > Signed-off-by: zhao wei <wei.zhao1@intel.com>
The name in your .gitconfig should be the same than in you mail:
Wei Zhao <wei.zhao1@intel.com>
> > Acked-by: John McNamara <john.mcnamara@intel.com>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>
Please, could you also check the same comments in the other patch?
Last thing: when doing another version of the patch, you should add a
changelog that describes what was modified. They take place after the 3
dashes.
Thank you for contributing.
Regards,
Olivier
^ permalink raw reply
* Re: [PATCH 00/16] e1000 base code update
From: Thomas Monjalon @ 2016-11-28 11:33 UTC (permalink / raw)
To: Wenzhuo Lu; +Cc: dev, Ferruh Yigit
In-Reply-To: <b3bc9072-3532-544d-5c59-219eae9d56e3@intel.com>
2016-11-25 12:58, Ferruh Yigit:
> Can you also please send another patch to:
> 1- add I219 to supported nics list
> 2- announce new supported nic in release notes.
Please update also the web site:
http://dpdk.org/doc/nics
^ permalink raw reply
* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Thomas Monjalon @ 2016-11-28 11:30 UTC (permalink / raw)
To: Bruce Richardson
Cc: Jerin Jacob, dev, harry.van.haaren, hemant.agrawal, gage.eads
In-Reply-To: <20161128091610.GB168972@bricha3-MOBL3.ger.corp.intel.com>
2016-11-28 09:16, Bruce Richardson:
> On Sat, Nov 26, 2016 at 08:24:55AM +0530, Jerin Jacob wrote:
> > On Fri, Nov 25, 2016 at 11:00:53AM +0000, Bruce Richardson wrote:
> > > On Fri, Nov 25, 2016 at 05:53:34AM +0530, Jerin Jacob wrote:
> > > > On Thu, Nov 24, 2016 at 04:35:56PM +0100, Thomas Monjalon wrote:
> > > > > 2016-11-24 07:29, Jerin Jacob:
> > > > > > On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:
> > > > > > > 2016-11-18 11:14, Jerin Jacob:
> > > > > > > > +Eventdev API - EXPERIMENTAL
> > > > > > > > +M: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > > > > +F: lib/librte_eventdev/
> > > > > > >
> > > >
> > > > I don't think there is any portability issue here, I can explain.
> > > >
> > > > The application level, we have two more use case to deal with non burst
> > > > variant
> > > >
> > > > - latency critical work
> > > > - on dequeue, if application wants to deal with only one flow(i.e to
> > > > avoid processing two different application flows to avoid cache trashing)
> > > >
> > > > Selection of the burst variants will be based on
> > > > rte_event_dev_info_get() and rte_event_dev_configure()(see, max_event_port_dequeue_depth,
> > > > max_event_port_enqueue_depth, nb_event_port_dequeue_depth, nb_event_port_enqueue_depth )
> > > > So I don't think their is portability issue here and I don't want to waste my
> > > > CPU cycles on the for loop if application known to be working with non
> > > > bursts variant like below
> > > >
> > >
> > > If the application is known to be working on non-burst varients, then
> > > they always request a burst-size of 1, and skip the loop completely.
> > > There is no extra performance hit in that case in either the app or the
> > > driver (since the non-burst driver always returns 1, irrespective of the
> > > number requested).
> >
> > Hmm. I am afraid, There is.
> > On the app side, the const "1" can not be optimized by the compiler as
> > on downside it is function pointer based driver interface
> > On the driver side, the implementation would be for loop based instead
> > of plain access.
> > (compiler never can see the const "1" in driver interface)
> >
> > We are planning to implement burst mode as kind of emulation mode and
> > have a different scheme for burst and nonburst. The similar approach we have
> > taken in introducing rte_event_schedule() and split the responsibility so
> > that SW driver can work without additional performance overhead and neat
> > driver interface.
> >
> > If you are concerned about the usability part and regression on the SW
> > driver, then it's not the case, application will use nonburst variant only if
> > dequeue_depth == 1 and/or explicit case where latency matters.
> >
> > On the portability side, we support both case and application if written based
> > on dequeue_depth it will perform well in both implementations.IMO, There is
> > no another shortcut for performance optimized application running on different
> > set of model.I think it is not an issue as, in event model as each cores
> > identical and main loop can be changed based on dequeue_depth
> > if needs performance(anyway mainloop will be function pointer based).
> >
>
> Ok, I think I see your point now. Here is an alternative suggestion.
>
> 1. Keep the single user API.
> 2. Have both single and burst function pointers in the driver
> 3. Call appropriately in the eventdev layer based on parameters. For
> example:
>
> rte_event_dequeue_burst(..., int num)
> {
> if (num == 1 && single_dequeue_fn != NULL)
> return single_dequeue_fn(...);
> return burst_dequeue_fn(...);
> }
>
> This way drivers can optionally special-case the single dequeue case -
> the function pointer check will definitely be predictable in HW making
> that a near-zero-cost check - while not forcing all drivers to do so.
> It also reduces the public API surface, and gives us a single enqueue
> and dequeue function.
+1
^ permalink raw reply
* Re: [PATCH] doc: introduce PVP reference benchmark
From: Thomas Monjalon @ 2016-11-28 11:22 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: yuanhan.liu, john.mcnamara, zhiyong.yang, dev, fbaudin
In-Reply-To: <20161123210006.7113-1-maxime.coquelin@redhat.com>
2016-11-23 22:00, Maxime Coquelin:
> +You can use this qmp-vcpu-pin script to pin vCPUs:
> +
> + .. code-block:: python
> +
> + #!/usr/bin/python
> + # QEMU vCPU pinning tool
> + #
> + # Copyright (C) 2016 Red Hat Inc.
> + #
> + # Authors:
> + # Maxime Coquelin <maxime.coquelin@redhat.com>
> + #
> + # This work is licensed under the terms of the GNU GPL, version 2. See
> + # the COPYING file in the top-level directory
> + import argparse
> + import json
> + import os
> +
> + from subprocess import call
> + from qmp import QEMUMonitorProtocol
> +
> + pinned = []
> +
> + parser = argparse.ArgumentParser(description='Pin QEMU vCPUs to physical CPUs')
> + parser.add_argument('-s', '--server', type=str, required=True,
> + help='QMP server path or address:port')
> + parser.add_argument('cpu', type=int, nargs='+',
> + help='Physical CPUs IDs')
> + args = parser.parse_args()
> +
> + devnull = open(os.devnull, 'w')
> +
> + srv = QEMUMonitorProtocol(args.server)
> + srv.connect()
> +
> + for vcpu in srv.command('query-cpus'):
> + vcpuid = vcpu['CPU']
> + tid = vcpu['thread_id']
> + if tid in pinned:
> + print 'vCPU{}\'s tid {} already pinned, skipping'.format(vcpuid, tid)
> + continue
> +
> + cpuid = args.cpu[vcpuid % len(args.cpu)]
> + print 'Pin vCPU {} (tid {}) to physical CPU {}'.format(vcpuid, tid, cpuid)
> + try:
> + call(['taskset', '-pc', str(cpuid), str(tid)], stdout=devnull)
> + pinned.append(tid)
> + except OSError:
> + print 'Failed to pin vCPU{} to CPU{}'.format(vcpuid, cpuid)
>
No please do not introduce such useful script in a doc.
I think it must be a separate file in the DPDK repository or
in the QEMU repository.
^ permalink raw reply
* Re: [PATCH v2] ethdev: check number of queues less than RTE_ETHDEV_QUEUE_STAT_CNTRS
From: Thomas Monjalon @ 2016-11-28 11:13 UTC (permalink / raw)
To: Olivier Matz, dev; +Cc: Alejandro Lucero, Bert van Leeuwen
In-Reply-To: <1480006746.31853.14.camel@6wind.com>
2016-11-24 17:59, Olivier Matz:
> Hi,
>
> On Mon, 2016-11-21 at 09:59 +0000, Alejandro Lucero wrote:
> > From: Bert van Leeuwen <bert.vanleeuwen@netronome.com>
> >
> > Arrays inside rte_eth_stats have size=RTE_ETHDEV_QUEUE_STAT_CNTRS.
> > Some devices report more queues than that and this code blindly uses
> > the reported number of queues by the device to fill those arrays up.
> > This patch fixes the problem using MIN between the reported number of
> > queues and RTE_ETHDEV_QUEUE_STAT_CNTRS.
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> >
>
> Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
>
>
> As a next step, I'm wondering if it would be possible to remove
> this limitation. We could replace the tables in struct rte_eth_stats
> by a pointer to an array allocated dynamically at pmd setup.
Yes that's definitely the right way to handle these statistics.
> It would break the API, so it should be announced first. I'm thinking
> of something like:
>
> struct rte_eth_generic_stats {
> uint64_t ipackets;
> uint64_t opackets;
> uint64_t ibytes;
> uint64_t obytes;
> uint64_t imissed;
> uint64_t ierrors;
> uint64_t oerrors;
> uint64_t rx_nombuf
> };
>
> struct rte_eth_stats {
> struct rte_eth_generic_stats port_stats;
> struct rte_eth_generic_stats *queue_stats;
> };
>
> The queue_stats array would always be indexed by queue_id.
> The xstats would continue to report the generic stats per-port and
> per-queue.
>
> About the mapping API, either we keep it as-is, or it could
> become a driver-specific API.
Yes I agree to remove the queue statistics mapping which is very specific.
I will send a patch with a deprecation notice to move the mapping API
to a driver-specific API.
Any objection?
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Thomas Monjalon @ 2016-11-28 11:03 UTC (permalink / raw)
To: dev, Rahul Lakkireddy, Stephen Hurd, Jan Medala, Jakub Palider,
John Daley, Adrien Mazarguil, Alejandro Lucero, Harish Patil,
Rasesh Mody, Jerin Jacob, Yuanhan Liu, Yong Wang
Cc: Tomasz Kulasek, konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-1-git-send-email-tomaszx.kulasek@intel.com>
We need attention of every PMD developers on this thread.
Reminder of what Konstantin suggested:
"
- if the PMD supports TX offloads AND
- if to be able use any of these offloads the upper layer SW would have to:
* modify the contents of the packet OR
* obey HW specific restrictions
then it is a PMD developer responsibility to provide tx_prep() that would implement
expected modifications of the packet contents and restriction checks.
Otherwise, tx_prep() implementation is not required and can be safely set to NULL.
"
I copy/paste also my previous conclusion:
Before txprep, there is only one API: the application must prepare the
packets checksum itself (get_psd_sum in testpmd).
With txprep, the application have 2 choices: keep doing the job itself
or call txprep which calls a PMD-specific function.
The question is: does non-Intel drivers need a checksum preparation for TSO?
Will it behave well if txprep does nothing in these drivers?
When looking at the code, most of drivers handle the TSO flags.
But it is hard to know whether they rely on the pseudo checksum or not.
git grep -l 'PKT_TX_UDP_CKSUM\|PKT_TX_TCP_CKSUM\|PKT_TX_TCP_SEG' drivers/net/
drivers/net/bnxt/bnxt_txr.c
drivers/net/cxgbe/sge.c
drivers/net/e1000/em_rxtx.c
drivers/net/e1000/igb_rxtx.c
drivers/net/ena/ena_ethdev.c
drivers/net/enic/enic_rxtx.c
drivers/net/fm10k/fm10k_rxtx.c
drivers/net/i40e/i40e_rxtx.c
drivers/net/ixgbe/ixgbe_rxtx.c
drivers/net/mlx4/mlx4.c
drivers/net/mlx5/mlx5_rxtx.c
drivers/net/nfp/nfp_net.c
drivers/net/qede/qede_rxtx.c
drivers/net/thunderx/nicvf_rxtx.c
drivers/net/virtio/virtio_rxtx.c
drivers/net/vmxnet3/vmxnet3_rxtx.c
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"
^ permalink raw reply
* sriov on fm10k card
From: Shaham Fridenberg @ 2016-11-28 11:01 UTC (permalink / raw)
To: dev@dpdk.org
Hi all,
I have an fm10k card with dual 100G ports, I am running a DPDK application that binds 2 virtual functions to it. I have trouble receiving traffic in my vnfs on fm10k VFs.
Current analysis:
1. From rdif debugging I see that it doesn't add the port of the VF in the switching table.
2. Running l2fwd in the same vm 1. Doesn't occur. (the port info and mac is added properly to the switch manager and the traffic works.)
3. Port initialization code in my application and in l2fwd have been compared and equalized.
4. The entry for the port is added in the rdif when we call the function fm10k_update_uc_addr(hw, hw->mac.dglort_map, data->mac_addrs[mac_index].addr_bytes,vlan_id, on, 0);
In my application this function is called too, but I noticed that the values of hw->mac.dglort_map are different.
In l2fwd it's updated with FM10K_DGLORTMAP_ZERO, in function that handles a msg from PF: fm10k_msg_lport_state_vf (according to documentation, this handler is meant to capture the indication from the PF that we are ready to bring up the interface).
In the case of my app this handler is never called. For some reason the PF does not send this message and I suspect this could be the reason why the VF mac entry is not learned in the Switch.
Any hint/ ideas of what I could be missing?
Thanks
^ permalink raw reply
* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Thomas Monjalon @ 2016-11-28 10:54 UTC (permalink / raw)
To: Tomasz Kulasek; +Cc: dev, konstantin.ananyev, olivier.matz, bruce.richardson
In-Reply-To: <1479922585-8640-2-git-send-email-tomaszx.kulasek@intel.com>
Hi,
2016-11-23 18:36, Tomasz Kulasek:
> --- a/config/common_base
> +++ b/config/common_base
> @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> CONFIG_RTE_LIBRTE_IEEE1588=n
> CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> +CONFIG_RTE_ETHDEV_TX_PREPARE=y
Please, remind me why is there a configuration here.
It should be the responsibility of the application to call tx_prepare
or not. If the application choose to use this new API but it is
disabled, then the packets won't be prepared and there is no error code:
> +#else
> +
> +static inline uint16_t
> +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused uint16_t queue_id,
> + __rte_unused struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> +{
> + return nb_pkts;
> +}
> +
> +#endif
So the application is not aware of the issue and it will not use
any fallback.
^ permalink raw reply
* Re: [PATCH 00/22] Generic flow API (rte_flow)
From: Pei, Yulong @ 2016-11-28 10:03 UTC (permalink / raw)
To: Adrien Mazarguil, dev@dpdk.org
Cc: Thomas Monjalon, De Lara Guarch, Pablo, Olivier Matz,
Xing, Beilei
In-Reply-To: <cover.1479309719.git.adrien.mazarguil@6wind.com>
Hi Adrien,
I think that you already did test for your patchset, do you have any automated test scripts can be shared for validation since there did not have testpmd flow command documentation yet?
Best Regards
Yulong Pei
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
Sent: Thursday, November 17, 2016 12:23 AM
To: dev@dpdk.org
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Olivier Matz <olivier.matz@6wind.com>
Subject: [dpdk-dev] [PATCH 00/22] Generic flow API (rte_flow)
As previously discussed in RFC v1 [1], RFC v2 [2], with changes described in [3] (also pasted below), here is the first non-draft series for this new API.
Its capabilities are so generic that its name had to be vague, it may be called "Generic flow API", "Generic flow interface" (possibly shortened as "GFI") to refer to the name of the new filter type, or "rte_flow" from the prefix used for its public symbols. I personally favor the latter.
While it is currently meant to supersede existing filter types in order for all PMDs to expose a common filtering/classification interface, it may eventually evolve to cover the following ideas as well:
- Rx/Tx offloads configuration through automatic offloads for specific
packets, e.g. performing checksum on TCP packets could be expressed with
an egress rule with a TCP pattern and a kind of checksum action.
- RSS configuration (already defined actually). Could be global or per rule
depending on hardware capabilities.
- Switching configuration for devices with many physical ports; rules doing
both ingress and egress could even be used to completely bypass software
if supported by hardware.
[1] http://dpdk.org/ml/archives/dev/2016-July/043365.html
[2] http://dpdk.org/ml/archives/dev/2016-August/045383.html
[3] http://dpdk.org/ml/archives/dev/2016-November/050044.html
Changes since RFC v2:
- New separate VLAN pattern item (previously part of the ETH definition),
found to be much more convenient.
- Removed useless "any" field from VF pattern item, the same effect can be
achieved by not providing a specification structure.
- Replaced bit-fields from the VXLAN pattern item to avoid endianness
conversion issues on 24-bit fields.
- Updated struct rte_flow_item with a new "last" field to create inclusive
ranges. They are defined as the interval between (spec & mask) and
(last & mask). All three parameters are optional.
- Renamed ID action MARK.
- Renamed "queue" fields in actions QUEUE and DUP to "index".
- "rss_conf" field in RSS action is now const.
- VF action now uses a 32 bit ID like its pattern item counterpart.
- Removed redundant struct rte_flow_pattern, API functions now expect
struct
rte_flow_item lists terminated by END items.
- Replaced struct rte_flow_actions for the same reason, with struct
rte_flow_action lists terminated by END actions.
- Error types (enum rte_flow_error_type) have been updated and the cause
pointer in struct rte_flow_error is now const.
- Function prototypes (rte_flow_create, rte_flow_validate) have also been
updated for clarity.
Additions:
- Public wrapper functions rte_flow_{validate|create|destroy|flush|query}
are now implemented in rte_flow.c, with their symbols exported and
versioned. Related filter type RTE_ETH_FILTER_GENERIC has been added.
- A separate header (rte_flow_driver.h) has been added for driver-side
functionality, in particular struct rte_flow_ops which contains PMD
callbacks returned by RTE_ETH_FILTER_GENERIC query.
- testpmd now exposes most of this API through the new "flow" command.
What remains to be done:
- Using endian-aware integer types (rte_beX_t) where necessary for clarity.
- API documentation (based on RFC).
- testpmd flow command documentation (although context-aware command
completion should already help quite a bit in this regard).
- A few pattern item / action properties cannot be configured yet
(e.g. rss_conf parameter for RSS action) and a few completions
(e.g. possible queue IDs) should be added.
Adrien Mazarguil (22):
ethdev: introduce generic flow API
cmdline: add support for dynamic tokens
cmdline: add alignment constraint
app/testpmd: implement basic support for rte_flow
app/testpmd: add flow command
app/testpmd: add rte_flow integer support
app/testpmd: add flow list command
app/testpmd: add flow flush command
app/testpmd: add flow destroy command
app/testpmd: add flow validate/create commands
app/testpmd: add flow query command
app/testpmd: add rte_flow item spec handler
app/testpmd: add rte_flow item spec prefix length
app/testpmd: add rte_flow bit-field support
app/testpmd: add item any to flow command
app/testpmd: add various items to flow command
app/testpmd: add item raw to flow command
app/testpmd: add items eth/vlan to flow command
app/testpmd: add items ipv4/ipv6 to flow command
app/testpmd: add L4 items to flow command
app/testpmd: add various actions to flow command
app/testpmd: add queue actions to flow command
MAINTAINERS | 4 +
app/test-pmd/Makefile | 1 +
app/test-pmd/cmdline.c | 32 +
app/test-pmd/cmdline_flow.c | 2581 +++++++++++++++++++++++++++
app/test-pmd/config.c | 484 +++++
app/test-pmd/csumonly.c | 1 +
app/test-pmd/flowgen.c | 1 +
app/test-pmd/icmpecho.c | 1 +
app/test-pmd/ieee1588fwd.c | 1 +
app/test-pmd/iofwd.c | 1 +
app/test-pmd/macfwd.c | 1 +
app/test-pmd/macswap.c | 1 +
app/test-pmd/parameters.c | 1 +
app/test-pmd/rxonly.c | 1 +
app/test-pmd/testpmd.c | 6 +
app/test-pmd/testpmd.h | 27 +
app/test-pmd/txonly.c | 1 +
lib/librte_cmdline/cmdline_parse.c | 67 +-
lib/librte_cmdline/cmdline_parse.h | 21 +
lib/librte_ether/Makefile | 3 +
lib/librte_ether/rte_eth_ctrl.h | 1 +
lib/librte_ether/rte_ether_version.map | 10 +
lib/librte_ether/rte_flow.c | 159 ++
lib/librte_ether/rte_flow.h | 947 ++++++++++
lib/librte_ether/rte_flow_driver.h | 177 ++
25 files changed, 4521 insertions(+), 9 deletions(-) create mode 100644 app/test-pmd/cmdline_flow.c create mode 100644 lib/librte_ether/rte_flow.c create mode 100644 lib/librte_ether/rte_flow.h create mode 100644 lib/librte_ether/rte_flow_driver.h
--
2.1.4
^ permalink raw reply
* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-11-28 9:16 UTC (permalink / raw)
To: Jerin Jacob
Cc: Thomas Monjalon, dev, harry.van.haaren, hemant.agrawal, gage.eads
In-Reply-To: <20161126025454.GA13886@svelivela-lt.caveonetworks.com>
On Sat, Nov 26, 2016 at 08:24:55AM +0530, Jerin Jacob wrote:
> On Fri, Nov 25, 2016 at 11:00:53AM +0000, Bruce Richardson wrote:
> > On Fri, Nov 25, 2016 at 05:53:34AM +0530, Jerin Jacob wrote:
> > > On Thu, Nov 24, 2016 at 04:35:56PM +0100, Thomas Monjalon wrote:
> > > > 2016-11-24 07:29, Jerin Jacob:
> > > > > On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:
> > > > > > 2016-11-18 11:14, Jerin Jacob:
> > > > > > > +Eventdev API - EXPERIMENTAL
> > > > > > > +M: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > > > +F: lib/librte_eventdev/
> > > > > >
> > >
> > > I don't think there is any portability issue here, I can explain.
> > >
> > > The application level, we have two more use case to deal with non burst
> > > variant
> > >
> > > - latency critical work
> > > - on dequeue, if application wants to deal with only one flow(i.e to
> > > avoid processing two different application flows to avoid cache trashing)
> > >
> > > Selection of the burst variants will be based on
> > > rte_event_dev_info_get() and rte_event_dev_configure()(see, max_event_port_dequeue_depth,
> > > max_event_port_enqueue_depth, nb_event_port_dequeue_depth, nb_event_port_enqueue_depth )
> > > So I don't think their is portability issue here and I don't want to waste my
> > > CPU cycles on the for loop if application known to be working with non
> > > bursts variant like below
> > >
> >
> > If the application is known to be working on non-burst varients, then
> > they always request a burst-size of 1, and skip the loop completely.
> > There is no extra performance hit in that case in either the app or the
> > driver (since the non-burst driver always returns 1, irrespective of the
> > number requested).
>
> Hmm. I am afraid, There is.
> On the app side, the const "1" can not be optimized by the compiler as
> on downside it is function pointer based driver interface
> On the driver side, the implementation would be for loop based instead
> of plain access.
> (compiler never can see the const "1" in driver interface)
>
> We are planning to implement burst mode as kind of emulation mode and
> have a different scheme for burst and nonburst. The similar approach we have
> taken in introducing rte_event_schedule() and split the responsibility so
> that SW driver can work without additional performance overhead and neat
> driver interface.
>
> If you are concerned about the usability part and regression on the SW
> driver, then it's not the case, application will use nonburst variant only if
> dequeue_depth == 1 and/or explicit case where latency matters.
>
> On the portability side, we support both case and application if written based
> on dequeue_depth it will perform well in both implementations.IMO, There is
> no another shortcut for performance optimized application running on different
> set of model.I think it is not an issue as, in event model as each cores
> identical and main loop can be changed based on dequeue_depth
> if needs performance(anyway mainloop will be function pointer based).
>
Ok, I think I see your point now. Here is an alternative suggestion.
1. Keep the single user API.
2. Have both single and burst function pointers in the driver
3. Call appropriately in the eventdev layer based on parameters. For
example:
rte_event_dequeue_burst(..., int num)
{
if (num == 1 && single_dequeue_fn != NULL)
return single_dequeue_fn(...);
return burst_dequeue_fn(...);
}
This way drivers can optionally special-case the single dequeue case -
the function pointer check will definitely be predictable in HW making
that a near-zero-cost check - while not forcing all drivers to do so.
It also reduces the public API surface, and gives us a single enqueue
and dequeue function.
/Bruce
^ permalink raw reply
* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-11-28 9:10 UTC (permalink / raw)
To: Jerin Jacob
Cc: Thomas Monjalon, dev, harry.van.haaren, hemant.agrawal, gage.eads
In-Reply-To: <20161126005756.GA10955@svelivela-lt.caveonetworks.com>
On Sat, Nov 26, 2016 at 06:27:57AM +0530, Jerin Jacob wrote:
> On Fri, Nov 25, 2016 at 02:09:22PM +0100, Thomas Monjalon wrote:
> > 2016-11-25 11:00, Bruce Richardson:
> > > On Fri, Nov 25, 2016 at 05:53:34AM +0530, Jerin Jacob wrote:
> > > > On Thu, Nov 24, 2016 at 04:35:56PM +0100, Thomas Monjalon wrote:
> > > > > 2016-11-24 07:29, Jerin Jacob:
> > > > > > On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:
> > > > > > > 2016-11-18 11:14, Jerin Jacob:
> > > > > > > > +#define EVENTDEV_NAME_SKELETON_PMD event_skeleton
> > > > > > > > +/**< Skeleton event device PMD name */
> > > > > > >
> > > > > > > I do not understand this #define.
> > > > > >
> > > > > > Applications can explicitly request the a specific driver though driver
> > > > > > name. This will go as argument to rte_event_dev_get_dev_id(const char *name).
> > > > > > The reason for keeping this #define in rte_eventdev.h is that,
> > > > > > application needs to include only rte_eventdev.h not rte_eventdev_pmd.h.
> > > > >
> > > > > So each driver must register its name in the API?
> > > > > Is it really needed?
> > > >
> > > > Otherwise how application knows the name of the driver.
> > > > The similar scheme used in cryptodev.
> > > > http://dpdk.org/browse/dpdk/tree/lib/librte_cryptodev/rte_cryptodev.h#n53
> > > > No strong opinion here. Open for suggestions.
> > > >
> > >
> > > I like having a name registered. I think we need a scheme where an app
> > > can find and use an implementation using a specific driver.
> >
> > I do not like having the driver names in the API.
> > An API should not know its drivers.
> > If an application do some driver-specific processing, it knows
> > the driver name as well. The driver name is written in the driver.
>
> If Bruce don't have further objection, Then I will go with Thomas's
> suggestion.
>
Go with it.
^ permalink raw reply
* Re: [PATCH v2] mempool: remove a redundant word "for" in comment
From: Olivier Matz @ 2016-11-28 8:42 UTC (permalink / raw)
To: Wei Zhao; +Cc: dev
In-Reply-To: <1480214627-27666-1-git-send-email-wei.zhao1@intel.com>
Hi Wenzhuo,
On Sun, 27 Nov 2016 10:43:47 +0800
Wei Zhao <wei.zhao1@intel.com> wrote:
> From: zhao wei <wei.zhao1@intel.com>
>
> There is a redundant repetition word "for" in commnet line of the
> file rte_mempool.h after the definition of RTE_MEMPOOL_OPS_NAMESIZE.
> The word "for"appear twice in line 359 and 360.One of them is
> redundant, so delete it.
>
> Fixes: 449c49b93a6b (" mempool: support handler operations")
> Signed-off-by: zhao wei <wei.zhao1@intel.com>
> Acked-by: John McNamara <john.mcnamara@intel.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
^ permalink raw reply
* [PATCH v2] maintainers: update testpmd maintainer
From: Jingjing Wu @ 2016-11-28 6:00 UTC (permalink / raw)
To: dev; +Cc: jingjing.wu, helin.zhang, wei.dai
In-Reply-To: <1479904483-2600-1-git-send-email-wei.dai@intel.com>
Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
v2 change:
remove Wei Dai from claim.
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index d6bb8f8..398e628 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -615,6 +615,7 @@ F: app/test/virtual_pmd.h
Driver testing tool
M: Pablo de Lara <pablo.de.lara.guarch@intel.com>
+M: Jingjing Wu <jingjing.wu@intel.com>
F: app/test-pmd/
F: doc/guides/testpmd_app_ug/
--
2.4.11
^ permalink raw reply related
* Re: [PATCH] tools: add tags and cscope index file generation support
From: Yuanhan Liu @ 2016-11-28 5:50 UTC (permalink / raw)
To: Jerin Jacob; +Cc: dev, thomas.monjalon
In-Reply-To: <1480205562-32745-1-git-send-email-jerin.jacob@caviumnetworks.com>
On Sun, Nov 27, 2016 at 05:42:42AM +0530, Jerin Jacob wrote:
> This script generates cscope, gtags, and tags
> index files based on EAL environment.
> (architecture and OS(linux/bsd))
>
> Selection of the architecture and OS environment
> is based on dpdk configuration target(T=)
>
> example usage:
> make tags T=x86_64-native-linuxapp-gcc
> make cscope T=x86_64-native-linuxapp-gcc
> make gtags T=x86_64-native-linuxapp-gcc
>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
It's handy. Thanks!
Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
--yliu
^ permalink raw reply
* [PATCH] e1000/base: announce supported NICs
From: Wenzhuo Lu @ 2016-11-27 18:11 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, Wenzhuo Lu
Announce the support of I219 NICs. Also add all the
other supported NICs.
Add Intel I219 NICs support in release note too.
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
doc/guides/rel_notes/release_17_02.rst | 4 ++++
drivers/net/e1000/base/README | 21 +++++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/doc/guides/rel_notes/release_17_02.rst b/doc/guides/rel_notes/release_17_02.rst
index 3b65038..a9380a4 100644
--- a/doc/guides/rel_notes/release_17_02.rst
+++ b/doc/guides/rel_notes/release_17_02.rst
@@ -38,6 +38,10 @@ New Features
Also, make sure to start the actual text at the margin.
=========================================================
+ * **Support I219 NICs.**
+
+ Add the support for the I219 NICs, which is Intel 1 Gigabit NICs.
+
Resolved Issues
---------------
diff --git a/drivers/net/e1000/base/README b/drivers/net/e1000/base/README
index 6cdd9b3..de1ae4c 100644
--- a/drivers/net/e1000/base/README
+++ b/drivers/net/e1000/base/README
@@ -34,6 +34,27 @@ This directory contains source code of FreeBSD em & igb drivers of version
cid-shared-code.2016.11.22 released by ND. The sub-directory of base/
contains the original source package.
+This driver is valid for the product(s) listed below
+* Intel® Ethernet Controller 82540
+* Intel® Ethernet Controller 82545 Series
+* Intel® Ethernet Controller 82546 Series
+* Intel® Ethernet Controller 82571 Series
+* Intel® Ethernet Controller 82572 Series
+* Intel® Ethernet Controller 82573
+* Intel® Ethernet Controller 82574
+* Intel® Ethernet Controller 82583
+* Intel® Ethernet Controller I217 Series
+* Intel® Ethernet Controller I218 Series
+* Intel® Ethernet Controller I219 Series
+* Intel® Ethernet Controller 82576 Series
+* Intel® Ethernet Controller 82575 Series
+* Intel® Ethernet Controller 82580 Series
+* Intel® Ethernet Controller I350 Series
+* Intel® Ethernet Controller I210 Series
+* Intel® Ethernet Controller I211
+* Intel® Ethernet Controller I354 Series
+* Intel® Ethernet Controller DH89XXCC Series
+
Updating the driver
===================
--
1.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox