* Re: [PATCH v2 3/4] app: give python apps a consistent shebang line
From: Mcnamara, John @ 2016-12-08 20:44 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev@dpdk.org, mkletzan@redhat.com
In-Reply-To: <3898678.2vNOkO5FOW@xps13>
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, December 8, 2016 4:21 PM
> To: Mcnamara, John <john.mcnamara@intel.com>
> Cc: dev@dpdk.org; mkletzan@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v2 3/4] app: give python apps a consistent
> shebang line
>
> 2016-12-08 16:03, John McNamara:
> > Add a consistent "env python" shebang line to the DPDK Python apps so
> > that they can call the default system python.
>
> The shebang is only useful for executable scripts.
> Those included by other python scripts should not have this line.
> Please could you remove the shebang for conf.py and data files?
Good point. In that case I'll squash 3/4 into 2/4 since the shebang change
only affects one executable file, even though it isn't strictly a Python 3
change.
John
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Jerin Jacob @ 2016-12-08 20:41 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <20161208093048.GA55440@bricha3-MOBL3.ger.corp.intel.com>
On Thu, Dec 08, 2016 at 09:30:49AM +0000, Bruce Richardson wrote:
> On Thu, Dec 08, 2016 at 12:23:03AM +0530, Jerin Jacob wrote:
> > On Tue, Dec 06, 2016 at 04:51:19PM +0000, Bruce Richardson wrote:
> > > 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>
> > > > ---
> > > > + /** WORD1 */
> > > > + RTE_STD_C11
> > > > + union {
> > > > + uint64_t u64;
> > > > + /**< Opaque 64-bit value */
> > > > + uintptr_t event_ptr;
> > > > + /**< Opaque event pointer */
> > >
> > > Since we have a uint64_t member of the union, might this be better as a
> > > void * rather than uintptr_t?
> >
> > No strong opinion here. For me, uintptr_t looks clean.
> > But, It is OK to change to void* as per your input.
> >
> > >
> > > > + struct rte_mbuf *mbuf;
> > > > + /**< mbuf pointer if dequeued event is associated with mbuf */
> > > > + };
> > > > +};
> > > > +
> > > <snip>
> > > > +/**
> > > > + * 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.
> > >
> > > I think this might need to be clarified. The device doesn't need to be
> > > reconfigured, but does it need to be stopped? In SW implementation, this
> > > affects how much we have to make things thread-safe. At minimum I think
> > > we should limit this to having only one thread call the function at a
> > > time, but we may allow enqueue dequeue ops from the data plane to run
> > > in parallel.
> >
> > Cavium implementation can change it at runtime without re-configuring or stopping
> > the device to support runtime load balancing from the application perspective.
> >
> > AFAIK, link establishment is _NOT_ fast path API. But the application
> > can invoke it from worker thread whenever there is a need for re-wiring
> > the queue to port connection for better explicit load balancing. IMO, A
> > software implementation with lock is fine here as we don't use this in
> > fastpath.
> >
> > Thoughts?
> > >
>
> I agree that it's obviously not fast-path. Therefore I suggest that we
> document that this API should be safe to call while the data path is in
> operation, but that it should not be called by multiple cores
> simultaneously i.e. single-writer, multi-reader safe, but not
> multi-writer safe. Does that seem reasonable to you?
If I understand it correctly, per "event port" their will be ONLY ONE
writer at time.
i.e, In the valid case, Following two can be invoked in parallel
rte_event_port_link(dev_id, 0 /*port_id*/,..)
rte_event_port_link(dev_id, 1 /*port_id*/,..)
But, not invoking rte_event_port_link() on the _same_ event port in parallel
Are we on same page?
Jerin
>
> /Bruce
^ permalink raw reply
* Re: Does DPDK 1.7.1 support jumboframes for VMXNET3
From: Yong Wang @ 2016-12-08 18:46 UTC (permalink / raw)
To: Kanika Singhal, dev@dpdk.org
In-Reply-To: <CACrtOHpiTCK4C5NBW5fYfcG3n=Qgjnk4TOXFR3SEJK-D1MyYTA@mail.gmail.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Kanika Singhal
> Sent: Wednesday, December 7, 2016 12:18 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] Does DPDK 1.7.1 support jumboframes for VMXNET3
>
> Hi,
>
> I am a new to DPDK.
>
> My product is using DPDK 1.6 and it does not support jumbo frames for
> VMXNET3.
>
> I have to upgrade to 1.7.1, but i am not able to find info on whether the
> jumbo frame support is being added for VMXNET3 or not.
Jumbo frame for vmxnet3 was introduced in commit
commit 2fdd835f992c2cdf55fc528fea9312c73313103c
Author: Stephen Hemminger <stephen@networkplumber.org>
Date: Fri Mar 4 10:08:00 2016 -0800
This is in release 16.04 and later. Either you need to upgrade to a version 16.04 or onwards or you need to backport that change to your release.
> I can see some jumbo frame related patches being posted for vmxnet3
>
> Do I need to apply the patches in 1.7.1 to make jumbo work/Is there any
> DPDK version which supports jumbo?
>
> Thanks,
> Kanika
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Yong Wang @ 2016-12-08 18:20 UTC (permalink / raw)
To: Ananyev, Konstantin, Yigit, Ferruh, Thomas Monjalon
Cc: Harish Patil, dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd,
Jan Medala, Jakub Palider, John Daley, Adrien Mazarguil,
Alejandro Lucero, Rasesh Mody, Jacob, Jerin, Yuanhan Liu,
Kulasek, TomaszX, olivier.matz@6wind.com
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E4E05@irsmsx105.ger.corp.intel.com>
> -----Original Message-----
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Wednesday, December 7, 2016 2:03 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Yong Wang
> <yongwang@vmware.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Cc: Harish Patil <harish.patil@qlogic.com>; dev@dpdk.org; Rahul Lakkireddy
> <rahul.lakkireddy@chelsio.com>; Stephen Hurd
> <stephen.hurd@broadcom.com>; Jan Medala <jan@semihalf.com>; Jakub
> Palider <jpa@semihalf.com>; John Daley <johndale@cisco.com>; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>; Alejandro Lucero
> <alejandro.lucero@netronome.com>; Rasesh Mody
> <rasesh.mody@qlogic.com>; Jacob, Jerin <Jerin.Jacob@cavium.com>;
> Yuanhan Liu <yuanhan.liu@linux.intel.com>; Kulasek, TomaszX
> <tomaszx.kulasek@intel.com>; olivier.matz@6wind.com
> Subject: RE: [dpdk-dev] [PATCH v12 0/6] add Tx preparation
>
>
> Hi Ferruh,
>
> >
> > On 12/6/2016 6:25 PM, Yong Wang wrote:
> > >> -----Original Message-----
> > >> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> > >> Sent: Sunday, December 4, 2016 4:11 AM
> > >> To: Yong Wang <yongwang@vmware.com>; Thomas Monjalon
> > >> <thomas.monjalon@6wind.com>
> > >> Cc: Harish Patil <harish.patil@qlogic.com>; dev@dpdk.org; Rahul
> Lakkireddy
> > >> <rahul.lakkireddy@chelsio.com>; Stephen Hurd
> > >> <stephen.hurd@broadcom.com>; Jan Medala <jan@semihalf.com>;
> Jakub
> > >> Palider <jpa@semihalf.com>; John Daley <johndale@cisco.com>;
> Adrien
> > >> Mazarguil <adrien.mazarguil@6wind.com>; Alejandro Lucero
> > >> <alejandro.lucero@netronome.com>; Rasesh Mody
> > >> <rasesh.mody@qlogic.com>; Jacob, Jerin <Jerin.Jacob@cavium.com>;
> > >> Yuanhan Liu <yuanhan.liu@linux.intel.com>; Kulasek, TomaszX
> > >> <tomaszx.kulasek@intel.com>; olivier.matz@6wind.com
> > >> Subject: RE: [dpdk-dev] [PATCH v12 0/6] add Tx preparation
> > >>
> > >> Hi
> > >>
> > >>
> > >>
> > >>>>
> > >>
> > >>>> 2016-11-30 17:42, Ananyev, Konstantin:
> > >>
> > >>>>>>> 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"
> > >>
> > >>>>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>> qede PMD doesn’t currently support TSO yet, it only supports Tx
> > >>
> > >>>> TCP/UDP/IP
> > >>
> > >>>>>> csum offloads.
> > >>
> > >>>>>> So Tx preparation isn’t applicable. So as of now -
> > >>
> > >>>>>> "it is OK, we do not need any checksum preparation for TSO"
> > >>
> > >>>>>
> > >>
> > >>>>> Thanks for the answer.
> > >>
> > >>>>> Though please note that it not only for TSO.
> > >>
> > >>>>
> > >>
> > >>>> Oh yes, sorry, my wording was incorrect.
> > >>
> > >>>> We need to know if any checksum preparation is needed prior
> > >>
> > >>>> offloading its final computation to the hardware or driver.
> > >>
> > >>>> So the question applies to TSO and simple checksum offload.
> > >>
> > >>>>
> > >>
> > >>>> We are still waiting answers for
> > >>
> > >>>> bnxt, cxgbe, ena, nfp, thunderx, virtio and vmxnet3.
> > >>
> > >>>
> > >>
> > >>> The case for a virtual device is a little bit more complicated as packets
> > >> offloaded from a virtual device can eventually be delivered to
> > >>
> > >>> another virtual NIC or different physical NICs that have different
> offload
> > >> requirements. In ESX, the hypervisor will enforce that the packets
> > >>
> > >>> offloaded will be something that the hardware expects. The contract
> for
> > >> vmxnet3 is that the guest needs to fill in pseudo header checksum
> > >>
> > >>> for both l4 checksum only and TSO + l4 checksum offload cases.
> > >>
> > >>
> > >>
> > >> Ok, so at first glance that looks to me very similar to Intel HW
> requirements.
> > >>
> > >> Could you confirm would rte_net_intel_cksum_prepare()
> > >>
> > >> also work for vmxnet3 or some extra modifications are required?
> > >>
> > >> You can look at it here:
> https://urldefense.proofpoint.com/v2/url?u=http-
> > >>
> 3A__dpdk.org_dev_patchwork_patch_17184_&d=DgIGaQ&c=uilaK90D4TOV
> > >>
> oH58JNXRgQ&r=v4BBYIqiDq552fkYnKKFBFyqvMXOR3UXSdFO2plFD1s&m=NS
> > >> 4zOl2je_tyGhnOJMSnu37HmJxOZf-1KLYcVsu8iYY&s=dL-NOC-
> > >> 18HclXUURQzuyW5Udw4NY13pKMndYvfgCfbA&e= .
> > >>
> > >> Note that for Intel HW the rules for pseudo-header csum calculation
> > >>
> > >> differ for TSO and non-TSO case.
> > >>
> > >> For TSO length inside pseudo-header are set to 0, while for non-tso case
> > >>
> > >> It should be set to L3 payload length.
> > >>
> > >> Is it the same for vmxnet3 or no?
> > >>
> > >> Thanks
> > >>
> > >> Konstantin
> > >>
> > >
> > > Yes and this is the same for vmxnet3.
> > >
> >
> > This means vmxnet3 PMD also should be updated, right?
>
> Yes, that's right.
>
> >Should that update
> > be part of tx_prep patchset? Or separate patch?
>
> Another question I suppose is who will do the actual patch for vmxnet3.
> Yong, are you ok to do the patch for vmxnet3, or prefer us to do that?
> Please note, that in both cases will need your help in testing/reviewing it.
> Konstantin
It will be great if you can put together a patch as part of the entire patchset on tx_prep() for vmxnet3 and I will definitely help review it.
Regarding testing, I can definitely help but I don't have a testing harness to cover the entire matrix (different ESX version, different vmxnet3 device version, VM-VM, VM-physical over different uplinks, etc.) so it will be limited. Related to this, I have the impression that Intel has some existing coverage for vmxnet3 as well as other NICs. Do we know if that will cover this use case as well?
> >
> > >>>
> > >>
> > >>>>> This is for any TX offload for which the upper layer SW would have
> > >>
> > >>>>> to modify the contents of the packet.
> > >>
> > >>>>> Though as I can see for qede neither PKT_TX_IP_CKSUM or
> > >>
> > >>>> PKT_TX_TCP_CKSUM
> > >>
> > >>>>> exhibits any extra requirements for the user.
> > >>
> > >>>>> Is that correct?
> > >>
> > >>
> > >
^ permalink raw reply
* Re: [PATCH v4] eal: restrict cores auto detection
From: Thomas Monjalon @ 2016-12-08 18:19 UTC (permalink / raw)
To: Jianfeng Tan; +Cc: dev, david.marchand, pmatilai, bruce.richardson
In-Reply-To: <1480700936-8450-1-git-send-email-jianfeng.tan@intel.com>
2016-12-02 17:48, Jianfeng Tan:
> This patch uses pthread_getaffinity_np() to narrow down used
> cores when none of below options is specified:
> * coremask (-c)
> * corelist (-l)
> * and coremap (--lcores)
>
> The purpose of this patch is to leave out these core related options
> when DPDK applications are deployed under container env, so that
> users do not need decide the core related parameters when developing
> applications. Instead, when applications are deployed in containers,
> use cpu-set to constrain which cores can be used inside this container
> instance. And DPDK application inside containers just rely on this
> auto detect mechanism to start polling threads.
>
> Note: previously, some users are using isolated CPUs, which could
> be excluded by default. Please add commands like taskset to use
> those cores.
>
> Test example:
> $ taskset 0xc0000 ./examples/helloworld/build/helloworld -m 1024
Bruce, what do you think of this version?
It requires taskset only if -c, -l and --lcores are not used.
> static int master_lcore_parsed;
> static int mem_parsed;
> +static int core_specified;
I think it's better to keep the word "parsed" as others.
^ permalink raw reply
* Re: [PATCH 00/22] Generic flow API (rte_flow)
From: Ferruh Yigit @ 2016-12-08 17:56 UTC (permalink / raw)
To: Adrien Mazarguil; +Cc: dev, Thomas Monjalon, Pablo de Lara, Olivier Matz
In-Reply-To: <20161208151935.GK10340@6wind.com>
On 12/8/2016 3:19 PM, Adrien Mazarguil wrote:
> Hi Ferruh,
>
> On Fri, Dec 02, 2016 at 04:58:53PM +0000, Ferruh Yigit wrote:
>> Hi Adrien,
>>
>> On 11/16/2016 4:23 PM, Adrien Mazarguil wrote:
>>> 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.
>>>
>>
>> <...>
>>
>> I was trying to check driver filter API patches, but hit a few compiler
>> errors with this patchset.
>>
>> [1] clang complains about variable bitfield value changed from -1 to 1.
>> Which is correct, but I guess that is intentional, but I don't know how
>> to tell this to clang?
>>
>> [2] shred library compilation error, because of missing rte_flow_flush
>> in rte_ether_version.map file
>>
>> [3] bunch of icc compilation errors, almost all are same type:
>> error #188: enumerated type mixed with another type
>
> Thanks for the report, I'll attempt to address them all in v2. However icc
> error #188 looks like a pain, I think I can work around it but do we really
> not tolerate the use of normal integers inside enum fields in DPDK?
If this warning is not improving the code, and community agree on it, it
is possible to disable warning by adding "-wd188" to test-pmd Makefile
for ICC compiler.
^ permalink raw reply
* Re: [PATCH 07/13] pci: replace probe and remove handlers with rte_driver
From: Ferruh Yigit @ 2016-12-08 17:50 UTC (permalink / raw)
To: Shreyansh Jain, dev; +Cc: thomas.monjalon, david.marchand, Adrien Mazarguil
In-Reply-To: <1480846288-2517-8-git-send-email-shreyansh.jain@nxp.com>
Hi Shreyansh,
On 12/4/2016 10:11 AM, Shreyansh Jain wrote:
> rte_pci_driver probe/remove callback are replaced with the rte_driver
> based probe/remove. This patch changes all the PCI drivers which reference
> rte_pci_driver->probe/remove.
>
> rte_pci_driver continues to have probe/remove callback which would be used
> once eth_driver is removed in later patches.
>
> Only changes to PCI are done. VDEV changes require a different
> model because of arguments being passed to vdev->probe/remove.
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
> drivers/net/bnx2x/bnx2x_ethdev.c | 12 ++++++++----
> drivers/net/bnxt/bnxt_ethdev.c | 6 ++++--
> drivers/net/cxgbe/cxgbe_ethdev.c | 6 ++++--
> drivers/net/e1000/em_ethdev.c | 6 ++++--
> drivers/net/e1000/igb_ethdev.c | 12 ++++++++----
> drivers/net/ena/ena_ethdev.c | 6 ++++--
> drivers/net/enic/enic_ethdev.c | 6 ++++--
> drivers/net/fm10k/fm10k_ethdev.c | 6 ++++--
> drivers/net/i40e/i40e_ethdev.c | 6 ++++--
> drivers/net/i40e/i40e_ethdev_vf.c | 6 ++++--
> drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++++++++----
> drivers/net/mlx4/mlx4.c | 6 ++++--
Generating compile error:
.../net/mlx4/mlx4.c:5913:13: error: subobject initialization overrides
initialization of other fields within its enclosing subobject
[-Werror,-Winitializer-overrides]
.driver = {
^
.../net/mlx4/mlx4.c:5909:13: note: previous initialization is here
.driver = {
^
> drivers/net/mlx5/mlx5.c | 4 +++-
.../net/mlx5/mlx5.c:736:13: error: subobject initialization overrides
initialization of other fields within its enclosing subobject
[-Werror,-Winitializer-overrides]
.driver = {
^
.../net/mlx5/mlx5.c:732:13: note: previous initialization is here
.driver = {
^
> drivers/net/nfp/nfp_net.c | 6 ++++--
> drivers/net/qede/qede_ethdev.c | 16 ++++++++++------
> drivers/net/szedata2/rte_eth_szedata2.c | 6 ++++--
> drivers/net/virtio/virtio_ethdev.c | 4 ++--
> drivers/net/vmxnet3/vmxnet3_ethdev.c | 6 ++++--
Following drivers seems missing:
drivers/net/thunderx/nicvf_ethdev.c
Also new added drivers/net/sfc/sfc_ethdev.c, but since sfc is not merged
to main branch yet, perhaps fix may wait your patches to be applied first.
> lib/librte_eal/common/eal_common_pci.c | 10 ++++++++--
> lib/librte_eal/common/include/rte_dev.h | 12 ++++++++++++
> lib/librte_ether/rte_ethdev.c | 16 ++++++++++++----
> lib/librte_ether/rte_ethdev.h | 6 +++---
> 22 files changed, 122 insertions(+), 54 deletions(-)
>
Also getting following with mlx5:
In file included from
.../x86_64-native-linuxapp-gcc/include/rte_mbuf.h:57:0,
from .../x86_64-native-linuxapp-gcc/include/rte_ether.h:52,
from .../drivers/net/mlx5/mlx5_trigger.c:38:
/usr/include/infiniband/verbs.h: In function ‘verbs_get_device’:
.../x86_64-native-linuxapp-gcc/include/rte_common.h:350:40: error:
initialization discards ‘const’ qualifier from pointer target type
[-Werror=discarded-qualifiers]
typeof(((type *)0)->member) *_ptr = (ptr); \
<...>
^ permalink raw reply
* Re: [PATCH v2 00/12] Add crypto PMD optimized for ARMv8
From: Jerin Jacob @ 2016-12-08 17:45 UTC (permalink / raw)
To: Zbigniew Bodek; +Cc: Bruce Richardson, pablo.de.lara.guarch, dev
In-Reply-To: <cffdbc51-2732-2917-f8f3-52328a534d1f@caviumnetworks.com>
On Thu, Dec 08, 2016 at 12:32:52PM +0100, Zbigniew Bodek wrote:
> On 08.12.2016 11:24, Bruce Richardson wrote:
> > On Tue, Dec 06, 2016 at 06:32:53PM -0800, zbigniew.bodek@caviumnetworks.com wrote:
> > > From: Zbigniew Bodek <zbigniew.bodek@caviumnetworks.com>
> > >
> > > Introduce crypto poll mode driver using ARMv8
> > > cryptographic extensions. This PMD is optimized
> > > to provide performance boost for chained
> > > crypto operations processing, such as:
> > > * encryption + HMAC generation
> > > * decryption + HMAC validation.
> > > In particular, cipher only or hash only
> > > operations are not provided.
> > > Performance gain can be observed in tests
> > > against OpenSSL PMD which also uses ARM
> > > crypto extensions for packets processing.
> > >
> > Hi,
> >
> > great to see more crypto drivers coming into DPDK, thanks.
> >
> > Question: do you know if this code would have any export compliance
> > implications for DPDK - or for those repackaging DPDK? Up till now, all
> > the crypto code used by DPDK was actually packaged in separate libraries
> > that were re-used, meaning that DPDK didn't contain any crypto
> > algorithms itself.
> >
>
> Hello Bruce,
>
> I don't know to be honest. I didn't know the reasoning behind not including
> crypto code for Intel for example. I thought it was due to licensing and
> code control rather than export compliance.
>
> Maybe someone from the DPDK community will know what are the constraints
> related to including crypto algorithms to DPDK.
One of the primary reason why we thought of going with this approach is
for out of the box "distribution" enablement. We thought, if the core crypto
algorithm sits in some git-hub code or public hosted tarball then the
PMD will never be added to standard distributions and which is a setback
for armv8 server ecosystem.
Having said that and as Zbigniew mentioned, We are open for revisiting
the crypto core algorithm and PMD split if there are community concerns
about export compliance. Let us know.
Jerin
>
> Kind regards
> Zbigniew
^ permalink raw reply
* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Olivier Matz @ 2016-12-08 17:24 UTC (permalink / raw)
To: Ananyev, Konstantin; +Cc: Thomas Monjalon, Kulasek, TomaszX, dev@dpdk.org
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E3405@irsmsx105.ger.corp.intel.com>
Hi Konstantin,
On Fri, 2 Dec 2016 16:17:51 +0000, "Ananyev, Konstantin"
<konstantin.ananyev@intel.com> wrote:
> Hi Olivier,
>
> > -----Original Message-----
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Friday, December 2, 2016 8:24 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; Kulasek, TomaszX
> > <tomaszx.kulasek@intel.com>; dev@dpdk.org Subject: Re: [dpdk-dev]
> > [PATCH v12 1/6] ethdev: add Tx preparation
> >
> > Hi Konstantin,
> >
> > On Fri, 2 Dec 2016 01:06:30 +0000, "Ananyev, Konstantin"
> > <konstantin.ananyev@intel.com> wrote:
> > > >
> > > > 2016-11-23 18:36, Tomasz Kulasek:
> > > > > +/**
> > > > > + * Process a burst of output packets on a transmit queue of
> > > > > an Ethernet device.
> > > > > + *
> > > > > + * The rte_eth_tx_prepare() function is invoked to prepare
> > > > > output packets to be
> > > > > + * transmitted on the output queue *queue_id* of the Ethernet
> > > > > device designated
> > > > > + * by its *port_id*.
> > > > > + * The *nb_pkts* parameter is the number of packets to be
> > > > > prepared which are
> > > > > + * supplied in the *tx_pkts* array of *rte_mbuf* structures,
> > > > > each of them
> > > > > + * allocated from a pool created with
> > > > > rte_pktmbuf_pool_create().
> > > > > + * For each packet to send, the rte_eth_tx_prepare() function
> > > > > performs
> > > > > + * the following operations:
> > > > > + *
> > > > > + * - Check if packet meets devices requirements for tx
> > > > > offloads.
> > > > > + *
> > > > > + * - Check limitations about number of segments.
> > > > > + *
> > > > > + * - Check additional requirements when debug is enabled.
> > > > > + *
> > > > > + * - Update and/or reset required checksums when tx offload
> > > > > is set for packet.
> > > > > + *
> > > > > + * Since this function can modify packet data, provided mbufs
> > > > > must be safely
> > > > > + * writable (e.g. modified data cannot be in shared
> > > > > segment).
> > > >
> > > > I think we will have to remove this limitation in next releases.
> > > > As we don't know how it could affect the API, I suggest to
> > > > declare this API EXPERIMENTAL.
> > >
> > > While I don't really mind to mart it as experimental, I don't
> > > really understand the reasoning: Why " this function can modify
> > > packet data, provided mbufs must be safely writable" suddenly
> > > becomes a problem? That seems like and obvious limitation to me
> > > and let say tx_burst() has the same one. Second, I don't see how
> > > you are going to remove it without introducing a heavy
> > > performance impact. Konstantin
> >
> > About tx_burst(), I don't think we should force the user to provide
> > a writable mbuf. There are many use cases where passing a clone
> > already works as of today and it avoids duplicating the mbuf data.
> > For instance: traffic generator, multicast, bridging/tap, etc...
> >
> > Moreover, this requirement would be inconsistent with the model you
> > are proposing in case of pipeline:
> > - tx_prepare() on core X, may update the data
> > - tx_burst() on core Y, should not touch the data to avoid cache
> > misses
>
> Probably I wasn't very clear in my previous mail.
> I am not saying that we should force the user to pass a writable mbuf.
> What I am saying that for tx_burst() current expectation is that
> after mbuf is handled to tx_burst() user shouldn't try to modify its
> buffer contents till TX engine is done with the buffer (mbuf_free()
> is called by TX func for it). For tx_prep(), I think, it is the same
> though restrictions are a bit more strict: user should not try to
> read/write to the mbuf while tx_prep() is not finished with it. What
> puzzles me is that why that should be the reason to mark tx_prep() as
> experimental. Konstantin
To be sure we're on the same page, let me reword:
- mbufs passed to tx_prepare() by the application must have their
headers (l2_len + l3_len + l4_len) writable because the phdr checksum
can be replaced. It could be precised in the api comment.
- mbufs passed to tx_burst() must not be modified by the driver/hw, nor
by the application.
About the API itself, I have one more question. I know you've
already discussed this a bit with Adrien, I don't want to spawn a new
big thread from here ;)
The API provides tx_prepare() to check the packets have the proper
format, and possibly modify them (ex: csum offload / tso) to match hw
requirements. So it does both checks (number of segments) and fixes
(csum/tso). What determines things that should be checked and things
that should be fixed?
The application gets few information from tx_prepare() about what should
be done to make the packet accepted by the hw, and the actions will
probably be different depending on hardware. So could we imagine that
in the future the function also tries to fix the packet? I've seen your
comment saying that it has to be an application decision, so what about
having a parameter saying "fix the packet" or "don't fix it"?
About rte_eth_desc_lim->nb_seg_max and
rte_eth_desc_lim->nb_mtu_seg_max, I'm still quite reserved, especially
for the 2nd one, because I don't see how it can be used by the
application. Well, it does not hurt to have them, but for me it looks a
bit useless.
Last thing, I think this API should become the default in the future.
For instance, it would prevent the application to calculate a phdr csum
that will not be used by the hw. Not calling tx_prepare() would require
the user/application to exactly knows the underlying hw and the kind of
packet that are generated. So for me it means we'll need to also update
other examples (other testpmd engines, l2fwd, ...). Do you agree?
Regards,
Olivier
^ permalink raw reply
* Re: [PATCH] net/ixgbe: fix typo in comment
From: Ferruh Yigit @ 2016-12-08 17:19 UTC (permalink / raw)
To: Lu, Wenzhuo, dev@dpdk.org
Cc: Zhang, Helin, Ananyev, Konstantin, stable@dpdk.org
In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093936147D@shsmsx102.ccr.corp.intel.com>
On 12/8/2016 12:49 AM, Lu, Wenzhuo wrote:
> Hi,
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>> Sent: Thursday, December 8, 2016 1:20 AM
>> To: dev@dpdk.org
>> Cc: Zhang, Helin; Ananyev, Konstantin; stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH] net/ixgbe: fix typo in comment
>>
>> Fixes: c03fcee9abbd ("ixgbe: remove CRC size from byte counters")
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>
Applied to dpdk-next-net/master, thanks.
^ permalink raw reply
* Re: [RFC] ethdev: abstraction layer for QoS hierarchical scheduler
From: Dumitrescu, Cristian @ 2016-12-08 17:18 UTC (permalink / raw)
To: Alan Robertson; +Cc: dev@dpdk.org, Thomas Monjalon
In-Reply-To: <ed5a51a8bda648d2a92504c42ada9381@EMEAWP-EXMB11.corp.brocade.com>
> Hi Cristian,
>
> The way qos works just now should be feasible for dynamic targets. That is
> similar functions
> to rte_sched_port_enqueue() and rte_sched_port_dequeue() would be
> called. The first to
> enqueue the mbufs onto the queues the second to dequeue. The qos
> structures and scheduler
> don't need to be as functionally rich though. I would have thought a simple
> pipe with child
> nodes should suffice for most. That would allow each tunnel/session to be
> shaped and the
> queueing and drop logic inherited from what is there just now.
>
> Thanks,
> Alan.
Hi Alan,
So just to make sure I get this right: you suggest that tunnels/sessions could simply be mapped as one of the layers under the port hierarchy?
Thanks,
Cristian
^ permalink raw reply
* Re: [PATCH] net/bonding: improve non-ip packets RSS
From: Ferruh Yigit @ 2016-12-08 17:13 UTC (permalink / raw)
To: Haifeng Lin, dev; +Cc: declan.doherty, jerry.lilijun
In-Reply-To: <1479460122-18780-1-git-send-email-haifeng.lin@huawei.com>
On 11/18/2016 9:08 AM, Haifeng Lin wrote:
> Most ethernet not support non-ip packets RSS and only first
> queue can used to receive. In this scenario lacp bond can
> only use one queue even if multi queue configured.
>
> We use below formula to change the map between bond_qid and
> slave_qid to let at least slave_num queues to receive packets:
>
> slave_qid = (bond_qid + slave_id) % queue_num
>
> Signed-off-by: Haifeng Lin <haifeng.lin@huawei.com>
> ---
Reminder for the patch ...
<..>
^ permalink raw reply
* Re: [PATCH] net/ixgbe: ensure link status is updated
From: Ferruh Yigit @ 2016-12-08 17:10 UTC (permalink / raw)
To: Laurent Hardy, helin.zhang, konstantin.ananyev
Cc: dev, Wenzhuo Lu, Zhang Qi, Xiao Wang
In-Reply-To: <583C68FF.5070907@6wind.com>
On 11/28/2016 5:27 PM, Laurent Hardy wrote:
> Hello,
> Is there anyone available to review this patch ?
Adding more developers into cc.
>
> regards,
> Laurent
>
<...>
^ permalink raw reply
* Re: [PATCH 01/22] ethdev: introduce generic flow API
From: Adrien Mazarguil @ 2016-12-08 17:07 UTC (permalink / raw)
To: Kevin Traynor
Cc: dev, Thomas Monjalon, Pablo de Lara, Olivier Matz,
sugesh.chandran
In-Reply-To: <7f65ba09-e6fe-d97a-6ab5-97e84a828a81@redhat.com>
On Fri, Dec 02, 2016 at 09:06:42PM +0000, Kevin Traynor wrote:
> On 12/01/2016 08:36 AM, Adrien Mazarguil wrote:
> > Hi Kevin,
> >
> > On Wed, Nov 30, 2016 at 05:47:17PM +0000, Kevin Traynor wrote:
> >> Hi Adrien,
> >>
> >> On 11/16/2016 04:23 PM, Adrien Mazarguil wrote:
> >>> This new API supersedes all the legacy filter types described in
> >>> rte_eth_ctrl.h. It is slightly higher level and as a result relies more on
> >>> PMDs to process and validate flow rules.
> >>>
> >>> Benefits:
> >>>
> >>> - A unified API is easier to program for, applications do not have to be
> >>> written for a specific filter type which may or may not be supported by
> >>> the underlying device.
> >>>
> >>> - The behavior of a flow rule is the same regardless of the underlying
> >>> device, applications do not need to be aware of hardware quirks.
> >>>
> >>> - Extensible by design, API/ABI breakage should rarely occur if at all.
> >>>
> >>> - Documentation is self-standing, no need to look up elsewhere.
> >>>
> >>> Existing filter types will be deprecated and removed in the near future.
> >>
> >> I'd suggest to add a deprecation notice to deprecation.rst, ideally with
> >> a target release.
> >
> > Will do, not a sure about the target release though. It seems a bit early
> > since no PMD really supports this API yet.
> >
> > [...]
> >>> diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
> >>> new file mode 100644
> >>> index 0000000..064963d
> >>> --- /dev/null
> >>> +++ b/lib/librte_ether/rte_flow.c
> >>> @@ -0,0 +1,159 @@
> >>> +/*-
> >>> + * BSD LICENSE
> >>> + *
> >>> + * Copyright 2016 6WIND S.A.
> >>> + * Copyright 2016 Mellanox.
> >>
> >> There's Mellanox copyright but you are the only signed-off-by - is that
> >> right?
> >
> > Yes, I'm the primary maintainer for Mellanox PMDs and this API was designed
> > on their behalf to expose several features from mlx4/mlx5 as the existing
> > filter types had too many limitations.
> >
> > [...]
> >>> +/* Get generic flow operations structure from a port. */
> >>> +const struct rte_flow_ops *
> >>> +rte_flow_ops_get(uint8_t port_id, struct rte_flow_error *error)
> >>> +{
> >>> + struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> >>> + const struct rte_flow_ops *ops;
> >>> + int code;
> >>> +
> >>> + if (unlikely(!rte_eth_dev_is_valid_port(port_id)))
> >>> + code = ENODEV;
> >>> + else if (unlikely(!dev->dev_ops->filter_ctrl ||
> >>> + dev->dev_ops->filter_ctrl(dev,
> >>> + RTE_ETH_FILTER_GENERIC,
> >>> + RTE_ETH_FILTER_GET,
> >>> + &ops) ||
> >>> + !ops))
> >>> + code = ENOTSUP;
> >>> + else
> >>> + return ops;
> >>> + rte_flow_error_set(error, code, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >>> + NULL, rte_strerror(code));
> >>> + return NULL;
> >>> +}
> >>> +
> >>
> >> Is it expected that the application or pmd will provide locking between
> >> these functions if required? I think it's going to have to be the app.
> >
> > Locking is indeed expected to be performed by applications. This API only
> > documents places where locking would make sense if necessary and expected
> > behavior.
> >
> > Like all control path APIs, this one assumes a single control thread.
> > Applications must take the necessary precautions.
>
> If you look at OVS now it's quite possible that you have 2 rx queues
> serviced by different threads, that would also install the flow rules in
> the software flow caches - possibly that could extend to adding hardware
> flows. There could also be another thread that is querying for stats. So
> anything that can be done to minimise the locking would be helpful -
> maybe query() could be atomic and not require any locking?
I think we need basic functions with as few constraints as possible on PMDs
first, this API being somewhat complex to implement on their side. That
covers the common use case where applications have a single control thread
or otherwise perform locking on their own.
Once the basics are there for most PMDs, we may add new functions, items,
properties and actions that provide additional constraints (timing,
multi-threading and so on), which remain to be defined according to
feedback. It is designed to be extended without causing ABI breakage.
As for query(), let's see how PMDs handle it first. A race between query()
and create() on a given device is almost unavoidable without locking, same
for queries that reset counters in a given flow rule. Basic parallel queries
should not cause any harm otherwise, although this cannot be guaranteed yet.
> > [...]
> >>> +/**
> >>> + * Flow rule attributes.
> >>> + *
> >>> + * Priorities are set on two levels: per group and per rule within groups.
> >>> + *
> >>> + * Lower values denote higher priority, the highest priority for both levels
> >>> + * is 0, so that a rule with priority 0 in group 8 is always matched after a
> >>> + * rule with priority 8 in group 0.
> >>> + *
> >>> + * Although optional, applications are encouraged to group similar rules as
> >>> + * much as possible to fully take advantage of hardware capabilities
> >>> + * (e.g. optimized matching) and work around limitations (e.g. a single
> >>> + * pattern type possibly allowed in a given group).
> >>> + *
> >>> + * Group and priority levels are arbitrary and up to the application, they
> >>> + * do not need to be contiguous nor start from 0, however the maximum number
> >>> + * varies between devices and may be affected by existing flow rules.
> >>> + *
> >>> + * If a packet is matched by several rules of a given group for a given
> >>> + * priority level, the outcome is undefined. It can take any path, may be
> >>> + * duplicated or even cause unrecoverable errors.
> >>
> >> I get what you are trying to do here wrt supporting multiple
> >> pmds/hardware implementations and it's a good idea to keep it flexible.
> >>
> >> Given that the outcome is undefined, it would be nice that the
> >> application has a way of finding the specific effects for verification
> >> and debugging.
> >
> > Right, however it was deemed a bit difficult to manage in many cases hence
> > the vagueness.
> >
> > For example, suppose two rules with the same group and priority, one
> > matching any IPv4 header, the other one any UDP header:
> >
> > - TCPv4 packets => rule #1.
> > - UDPv6 packets => rule #2.
> > - UDPv4 packets => both?
> >
> > That last one is perhaps invalid, checking that some unspecified protocol
> > combination does not overlap is expensive and may miss corner cases, even
> > assuming this is not an issue, what if the application guarantees that no
> > UDPv4 packets can ever hit that rule?
>
> that's fine - I don't expect the software to be able to know what the
> hardware will do with those rules. It's more about trying to get a dump
> from the hardware if something goes wrong. Anyway covered in comment later.
>
> >
> > Suggestions are welcome though, perhaps we can refine the description
> >
> >>> + *
> >>> + * Note that support for more than a single group and priority level is not
> >>> + * guaranteed.
> >>> + *
> >>> + * Flow rules can apply to inbound and/or outbound traffic (ingress/egress).
> >>> + *
> >>> + * Several pattern items and actions are valid and can be used in both
> >>> + * directions. Those valid for only one direction are described as such.
> >>> + *
> >>> + * Specifying both directions at once is not recommended but may be valid in
> >>> + * some cases, such as incrementing the same counter twice.
> >>> + *
> >>> + * Not specifying any direction is currently an error.
> >>> + */
> >>> +struct rte_flow_attr {
> >>> + uint32_t group; /**< Priority group. */
> >>> + uint32_t priority; /**< Priority level within group. */
> >>> + uint32_t ingress:1; /**< Rule applies to ingress traffic. */
> >>> + uint32_t egress:1; /**< Rule applies to egress traffic. */
> >>> + uint32_t reserved:30; /**< Reserved, must be zero. */
> >>> +};
> > [...]
> >>> +/**
> >>> + * RTE_FLOW_ITEM_TYPE_VF
> >>> + *
> >>> + * Matches packets addressed to a virtual function ID of the device.
> >>> + *
> >>> + * If the underlying device function differs from the one that would
> >>> + * normally receive the matched traffic, specifying this item prevents it
> >>> + * from reaching that device unless the flow rule contains a VF
> >>> + * action. Packets are not duplicated between device instances by default.
> >>> + *
> >>> + * - Likely to return an error or never match any traffic if this causes a
> >>> + * VF device to match traffic addressed to a different VF.
> >>> + * - Can be specified multiple times to match traffic addressed to several
> >>> + * specific VFs.
> >>> + * - Can be combined with a PF item to match both PF and VF traffic.
> >>> + *
> >>> + * A zeroed mask can be used to match any VF.
> >>
> >> can you refer explicitly to id
> >
> > If you mean "VF" to "VF ID" then yes, will do it for v2.
> >
> >>> + */
> >>> +struct rte_flow_item_vf {
> >>> + uint32_t id; /**< Destination VF ID. */
> >>> +};
> > [...]
> >>> +/**
> >>> + * Matching pattern item definition.
> >>> + *
> >>> + * A pattern is formed by stacking items starting from the lowest protocol
> >>> + * layer to match. This stacking restriction does not apply to meta items
> >>> + * which can be placed anywhere in the stack with no effect on the meaning
> >>> + * of the resulting pattern.
> >>> + *
> >>> + * A stack is terminated by a END item.
> >>> + *
> >>> + * The spec field should be a valid pointer to a structure of the related
> >>> + * item type. It may be set to NULL in many cases to use default values.
> >>> + *
> >>> + * Optionally, last can point to a structure of the same type to define an
> >>> + * inclusive range. This is mostly supported by integer and address fields,
> >>> + * may cause errors otherwise. Fields that do not support ranges must be set
> >>> + * to the same value as their spec counterparts.
> >>> + *
> >>> + * By default all fields present in spec are considered relevant.* This
> >>
> >> typo "*"
> >
> > No, that's an asterisk for a footnote below. Perhaps it is a bit unusual,
> > would something like "[1]" look better?
>
> oh, I thought it was the start of a comment line gone astray. Maybe "See
> note below", no big deal though.
OK, will change it anyway for clarity.
> >>> + * behavior can be altered by providing a mask structure of the same type
> >>> + * with applicable bits set to one. It can also be used to partially filter
> >>> + * out specific fields (e.g. as an alternate mean to match ranges of IP
> >>> + * addresses).
> >>> + *
> >>> + * Note this is a simple bit-mask applied before interpreting the contents
> >>> + * of spec and last, which may yield unexpected results if not used
> >>> + * carefully. For example, if for an IPv4 address field, spec provides
> >>> + * 10.1.2.3, last provides 10.3.4.5 and mask provides 255.255.0.0, the
> >>> + * effective range is 10.1.0.0 to 10.3.255.255.
> >>> + *
> >
> > See footnote below:
> >
> >>> + * * The defaults for data-matching items such as IPv4 when mask is not
> >>> + * specified actually depend on the underlying implementation since only
> >>> + * recognized fields can be taken into account.
> >>> + */
> >>> +struct rte_flow_item {
> >>> + enum rte_flow_item_type type; /**< Item type. */
> >>> + const void *spec; /**< Pointer to item specification structure. */
> >>> + const void *last; /**< Defines an inclusive range (spec to last). */
> >>> + const void *mask; /**< Bit-mask applied to spec and last. */
> >>> +};
> >>> +
> >>> +/**
> >>> + * Action types.
> >>> + *
> >>> + * Each possible action is represented by a type. Some have associated
> >>> + * configuration structures. Several actions combined in a list can be
> >>> + * affected to a flow rule. That list is not ordered.
> >>> + *
> >>> + * They fall in three categories:
> >>> + *
> >>> + * - Terminating actions (such as QUEUE, DROP, RSS, PF, VF) that prevent
> >>> + * processing matched packets by subsequent flow rules, unless overridden
> >>> + * with PASSTHRU.
> >>> + *
> >>> + * - Non terminating actions (PASSTHRU, DUP) that leave matched packets up
> >>> + * for additional processing by subsequent flow rules.
> >>> + *
> >>> + * - Other non terminating meta actions that do not affect the fate of
> >>> + * packets (END, VOID, MARK, FLAG, COUNT).
> >>> + *
> >>> + * When several actions are combined in a flow rule, they should all have
> >>> + * different types (e.g. dropping a packet twice is not possible). The
> >>> + * defined behavior is for PMDs to only take into account the last action of
> >>> + * a given type found in the list. PMDs still perform error checking on the
> >>> + * entire list.
> >>
> >> why do you define that the pmd will interpret multiple same type rules
> >> in this way...would it not make more sense for the pmd to just return
> >> EINVAL for an invalid set of rules? It seems more transparent for the
> >> application.
> >
> > Well, I had to define something as a default. The reason is that any number
> > of VOID actions may specified and did not want that to be a special case in
> > order to keep PMD parsers as simple as possible. I'll settle for EINVAL (or
> > some other error) if at least one PMD maintainer other than Nelio who
> > intends to implement this API is not convinced by this explanation, all
> > right?
>
> From an API perspective I think it's cleaner to pass or fail with the
> input rather than change it. But yes, please take pmd maintainers input
> as to what is reasonable to check also.
>
> >
> > [...]
> >>> +/**
> >>> + * RTE_FLOW_ACTION_TYPE_MARK
> >>> + *
> >>> + * Attaches a 32 bit value to packets.
> >>> + *
> >>> + * This value is arbitrary and application-defined. For compatibility with
> >>> + * FDIR it is returned in the hash.fdir.hi mbuf field. PKT_RX_FDIR_ID is
> >>> + * also set in ol_flags.
> >>> + */
> >>> +struct rte_flow_action_mark {
> >>> + uint32_t id; /**< 32 bit value to return with packets. */
> >>> +};
> >>
> >> One use case I thought we would be able to do for OVS is classification
> >> in hardware and the unique flow id is sent with the packet to software.
> >> But in OVS the ufid is 128 bits, so it means we can't and there is still
> >> the miniflow extract overhead. I'm not sure if there is a practical way
> >> around this.
> >>
> >> Sugesh (cc'd) has looked at this before and may be able to comment or
> >> correct me.
> >
> > Yes, we settled on 32 bit because currently no known hardware implementation
> > supports more than this. If that changes, another action with a larger type
> > shall be provided (no ABI breakage).
> >
> > Also since even 64 bit would not be enough for the use case you mention,
> > there is no choice but use this as an indirect value (such as an array or
> > hash table index/value).
>
> ok, cool. I think Sugesh has other ideas anyway!
>
> >
> > [...]
> >>> +/**
> >>> + * RTE_FLOW_ACTION_TYPE_RSS
> >>> + *
> >>> + * Similar to QUEUE, except RSS is additionally performed on packets to
> >>> + * spread them among several queues according to the provided parameters.
> >>> + *
> >>> + * Note: RSS hash result is normally stored in the hash.rss mbuf field,
> >>> + * however it conflicts with the MARK action as they share the same
> >>> + * space. When both actions are specified, the RSS hash is discarded and
> >>> + * PKT_RX_RSS_HASH is not set in ol_flags. MARK has priority. The mbuf
> >>> + * structure should eventually evolve to store both.
> >>> + *
> >>> + * Terminating by default.
> >>> + */
> >>> +struct rte_flow_action_rss {
> >>> + const struct rte_eth_rss_conf *rss_conf; /**< RSS parameters. */
> >>> + uint16_t queues; /**< Number of entries in queue[]. */
> >>> + uint16_t queue[]; /**< Queues indices to use. */
> >>
> >> I'd try and avoid queue and queues - someone will say "huh?" when
> >> reading code. s/queues/num ?
> >
> > Agreed, will update for v2.
> >
> >>> +};
> >>> +
> >>> +/**
> >>> + * RTE_FLOW_ACTION_TYPE_VF
> >>> + *
> >>> + * Redirects packets to a virtual function (VF) of the current device.
> >>> + *
> >>> + * Packets matched by a VF pattern item can be redirected to their original
> >>> + * VF ID instead of the specified one. This parameter may not be available
> >>> + * and is not guaranteed to work properly if the VF part is matched by a
> >>> + * prior flow rule or if packets are not addressed to a VF in the first
> >>> + * place.
> >>
> >> Not clear what you mean by "not guaranteed to work if...". Please return
> >> fail when this action is used if this is not going to work.
> >
> > Again, this is a case where it is difficult for a PMD to determine if the
> > entire list of flow rules makes sense. Perhaps it does, perhaps whatever
> > goes through has already been filtered out of possible issues.
> >
> > Here the documentation states the precautions an application should take to
> > guarantee it will work as intended. Perhaps it can be reworded (any
> > suggestion?), but a PMD can certainly not provide any strong guarantee.
>
> I see your point. Maybe for easy check things the pmd would return fail,
> but for more complex I agree it's too difficult.
>
> >
> >>> + *
> >>> + * Terminating by default.
> >>> + */
> >>> +struct rte_flow_action_vf {
> >>> + uint32_t original:1; /**< Use original VF ID if possible. */
> >>> + uint32_t reserved:31; /**< Reserved, must be zero. */
> >>> + uint32_t id; /**< VF ID to redirect packets to. */
> >>> +};
> > [...]
> >>> +/**
> >>> + * Check whether a flow rule can be created on a given port.
> >>> + *
> >>> + * While this function has no effect on the target device, the flow rule is
> >>> + * validated against its current configuration state and the returned value
> >>> + * should be considered valid by the caller for that state only.
> >>> + *
> >>> + * The returned value is guaranteed to remain valid only as long as no
> >>> + * successful calls to rte_flow_create() or rte_flow_destroy() are made in
> >>> + * the meantime and no device parameter affecting flow rules in any way are
> >>> + * modified, due to possible collisions or resource limitations (although in
> >>> + * such cases EINVAL should not be returned).
> >>> + *
> >>> + * @param port_id
> >>> + * Port identifier of Ethernet device.
> >>> + * @param[in] attr
> >>> + * Flow rule attributes.
> >>> + * @param[in] pattern
> >>> + * Pattern specification (list terminated by the END pattern item).
> >>> + * @param[in] actions
> >>> + * Associated actions (list terminated by the END action).
> >>> + * @param[out] error
> >>> + * Perform verbose error reporting if not NULL.
> >>> + *
> >>> + * @return
> >>> + * 0 if flow rule is valid and can be created. A negative errno value
> >>> + * otherwise (rte_errno is also set), the following errors are defined:
> >>> + *
> >>> + * -ENOSYS: underlying device does not support this functionality.
> >>> + *
> >>> + * -EINVAL: unknown or invalid rule specification.
> >>> + *
> >>> + * -ENOTSUP: valid but unsupported rule specification (e.g. partial
> >>> + * bit-masks are unsupported).
> >>> + *
> >>> + * -EEXIST: collision with an existing rule.
> >>> + *
> >>> + * -ENOMEM: not enough resources.
> >>> + *
> >>> + * -EBUSY: action cannot be performed due to busy device resources, may
> >>> + * succeed if the affected queues or even the entire port are in a stopped
> >>> + * state (see rte_eth_dev_rx_queue_stop() and rte_eth_dev_stop()).
> >>> + */
> >>> +int
> >>> +rte_flow_validate(uint8_t port_id,
> >>> + const struct rte_flow_attr *attr,
> >>> + const struct rte_flow_item pattern[],
> >>> + const struct rte_flow_action actions[],
> >>> + struct rte_flow_error *error);
> >>
> >> Why not just use rte_flow_create() and get an error? Is it less
> >> disruptive to do a validate and find the rule cannot be created, than
> >> using a create directly?
> >
> > The rationale can be found in the original RFC, which I'll convert to actual
> > documentation in v2. In short:
> >
> > - Calling rte_flow_validate() before rte_flow_create() is useless since
> > rte_flow_create() also performs validation.
> >
> > - We cannot possibly express a full static set of allowed flow rules, even
> > if we could, it usually depends on the current hardware configuration
> > therefore would not be static.
> >
> > - rte_flow_validate() is thus provided as a replacement for capability
> > flags. It can be used to determine during initialization if the underlying
> > device can support the typical flow rules an application might want to
> > provide later and do something useful with that information (e.g. always
> > use software fallback due to HW limitations).
> >
> > - rte_flow_validate() being a subset of rte_flow_create(), it is essentially
> > free to expose.
>
> make sense now, thanks.
>
> >
> >>> +
> >>> +/**
> >>> + * Create a flow rule on a given port.
> >>> + *
> >>> + * @param port_id
> >>> + * Port identifier of Ethernet device.
> >>> + * @param[in] attr
> >>> + * Flow rule attributes.
> >>> + * @param[in] pattern
> >>> + * Pattern specification (list terminated by the END pattern item).
> >>> + * @param[in] actions
> >>> + * Associated actions (list terminated by the END action).
> >>> + * @param[out] error
> >>> + * Perform verbose error reporting if not NULL.
> >>> + *
> >>> + * @return
> >>> + * A valid handle in case of success, NULL otherwise and rte_errno is set
> >>> + * to the positive version of one of the error codes defined for
> >>> + * rte_flow_validate().
> >>> + */
> >>> +struct rte_flow *
> >>> +rte_flow_create(uint8_t port_id,
> >>> + const struct rte_flow_attr *attr,
> >>> + const struct rte_flow_item pattern[],
> >>> + const struct rte_flow_action actions[],
> >>> + struct rte_flow_error *error);
> >>
> >> General question - are these functions threadsafe? In the OVS example
> >> you could have several threads wanting to create flow rules at the same
> >> time for same or different ports.
> >
> > No they aren't, applications have to perform their own locking. The RFC (to
> > be converted to actual documentation in v2) says that:
> >
> > - API operations are synchronous and blocking (``EAGAIN`` cannot be
> > returned).
> >
> > - There is no provision for reentrancy/multi-thread safety, although nothing
> > should prevent different devices from being configured at the same
> > time. PMDs may protect their control path functions accordingly.
>
> other comment above wrt locking.
>
> >
> >>> +
> >>> +/**
> >>> + * Destroy a flow rule on a given port.
> >>> + *
> >>> + * Failure to destroy a flow rule handle may occur when other flow rules
> >>> + * depend on it, and destroying it would result in an inconsistent state.
> >>> + *
> >>> + * This function is only guaranteed to succeed if handles are destroyed in
> >>> + * reverse order of their creation.
> >>
> >> How can the application find this information out on error?
> >
> > Without maintaining a list, they cannot. The specified case is the only
> > possible guarantee. That does not mean PMDs should not do their best to
> > destroy flow rules, only that ordering must remain consistent in case of
> > inability to destroy one.
> >
> > What do you suggest?
>
> I think if the app cannot remove a specific rule it may want to remove
> all rules and deal with flows in software for a time. So once the app
> knows it fails that should be enough.
OK, then since destruction may return an error already, is it fine?
Applications may call rte_flow_flush() (not supposed to fail unless there is
a serious issue, abort() in that case) and switch to SW fallback.
> >>> + *
> >>> + * @param port_id
> >>> + * Port identifier of Ethernet device.
> >>> + * @param flow
> >>> + * Flow rule handle to destroy.
> >>> + * @param[out] error
> >>> + * Perform verbose error reporting if not NULL.
> >>> + *
> >>> + * @return
> >>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> >>> + */
> >>> +int
> >>> +rte_flow_destroy(uint8_t port_id,
> >>> + struct rte_flow *flow,
> >>> + struct rte_flow_error *error);
> >>> +
> >>> +/**
> >>> + * Destroy all flow rules associated with a port.
> >>> + *
> >>> + * In the unlikely event of failure, handles are still considered destroyed
> >>> + * and no longer valid but the port must be assumed to be in an inconsistent
> >>> + * state.
> >>> + *
> >>> + * @param port_id
> >>> + * Port identifier of Ethernet device.
> >>> + * @param[out] error
> >>> + * Perform verbose error reporting if not NULL.
> >>> + *
> >>> + * @return
> >>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> >>> + */
> >>> +int
> >>> +rte_flow_flush(uint8_t port_id,
> >>> + struct rte_flow_error *error);
> >>
> >> rte_flow_destroy_all() would be more descriptive (but breaks your style)
> >
> > There are enough underscores as it is. I like flush, if enough people
> > complain we'll change it but it has to occur before the first public
> > release.
> >
> >>> +
> >>> +/**
> >>> + * Query an existing flow rule.
> >>> + *
> >>> + * This function allows retrieving flow-specific data such as counters.
> >>> + * Data is gathered by special actions which must be present in the flow
> >>> + * rule definition.
> >>
> >> re last sentence, it would be good if you can put a link to
> >> RTE_FLOW_ACTION_TYPE_COUNT
> >
> > Will do, I did not know how until very recently.
> >
> >>> + *
> >>> + * @param port_id
> >>> + * Port identifier of Ethernet device.
> >>> + * @param flow
> >>> + * Flow rule handle to query.
> >>> + * @param action
> >>> + * Action type to query.
> >>> + * @param[in, out] data
> >>> + * Pointer to storage for the associated query data type.
> >>
> >> can this be anything other than rte_flow_query_count?
> >
> > Likely in the future. I've only defined this one as a counterpart for
> > existing API functionality and because we wanted to expose it in mlx5.
> >
> >>> + * @param[out] error
> >>> + * Perform verbose error reporting if not NULL.
> >>> + *
> >>> + * @return
> >>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> >>> + */
> >>> +int
> >>> +rte_flow_query(uint8_t port_id,
> >>> + struct rte_flow *flow,
> >>> + enum rte_flow_action_type action,
> >>> + void *data,
> >>> + struct rte_flow_error *error);
> >>> +
> >>> +#ifdef __cplusplus
> >>> +}
> >>> +#endif
> >>
> >> I don't see a way to dump all the rules for a port out. I think this is
> >> neccessary for degbugging. You could have a look through dpif.h in OVS
> >> and see how dpif_flow_dump_next() is used, it might be a good reference.
> >
> > DPDK does not maintain flow rules and, depending on hardware capabilities
> > and level of compliance, PMDs do not necessarily do it either, particularly
> > since it requires space and application probably have a better method to
> > store these pointers for their own needs.
>
> understood
>
> >
> > What you see here is only a PMD interface. Depending on applications needs,
> > generic helper functions built on top of these may be added to manage flow
> > rules in the future.
>
> I'm thinking of the case where something goes wrong and I want to get a
> dump of all the flow rules from hardware, not query the rules I think I
> have. I don't see a way to do it or something to build a helper on top of?
Generic helper functions would exist on top of this API and would likely
maintain a list of flow rules themselves. The dump in that case would be
entirely implemented in software. I think that recovering flow rules from HW
may be complicated in many cases (even without taking storage allocation and
rules conversion issues into account), therefore if there is really a need
for it, we could perhaps add a dump() function that PMDs are free to
implement later.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH] net/qede: fix resource leak
From: Ferruh Yigit @ 2016-12-08 16:52 UTC (permalink / raw)
To: Yong Wang, harish.patil; +Cc: dev, Harish Patil
In-Reply-To: <1480509129-27623-1-git-send-email-wang.yong19@zte.com.cn>
On 11/30/2016 12:32 PM, Yong Wang wrote:
> Current code does not close 'fd' on function exit, leaking resources.
>
> Signed-off-by: Yong Wang <wang.yong19@zte.com.cn>
> ---
Add new mail address of the maintainer.
CC: Harish Patil <harish.patil@cavium.com>
<...>
^ permalink raw reply
* Re: [PATCH 1/7] net/qede: reduce noise in debug logs
From: Ferruh Yigit @ 2016-12-08 16:48 UTC (permalink / raw)
To: Harish Patil, dev; +Cc: Dept-EngDPDKDev, Rasesh Mody
In-Reply-To: <1480733039-13046-1-git-send-email-harish.patil@qlogic.com>
On 12/3/2016 2:43 AM, Harish Patil wrote:
> From: Rasesh Mody <Rasesh.Mody@cavium.com>
>
> Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
> where each bit represent a particular submodule to debug. Also move
> notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
>
> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
> ---
Is 32bit supported by driver?
If so it is throwing a compilation error for it [1], if not can you
please document it?
[1]
In file included from .../drivers/net/qede/base/ecore.h:35:0,
from .../drivers/net/qede/qede_ethdev.h:22,
from .../drivers/net/qede/qede_ethdev.c:9:
.../drivers/net/qede/qede_ethdev.c: In function ‘qede_rss_hash_update’:
.../drivers/net/qede/base/../qede_logs.h:33:3: error: format ‘%lx’
expects argument of type ‘long unsigned int’, but argument 7 has type
‘uint64_t {aka long long unsigned int}’ [-Werror=format=]
"[%s:%d(%s)]" fmt, \
^
.../drivers/net/qede/qede_ethdev.c:1472:2: note: in expansion of macro
‘DP_INFO’
DP_INFO(edev, "RSS hf = 0x%lx len = %u key = %p\n", hf, len, key);
^~~~~~~
cc1: all warnings being treated as errors
<...>
^ permalink raw reply
* Re: [PATCH 7/7] net/qede: restrict maximum queues for PF/VF
From: Ferruh Yigit @ 2016-12-08 16:45 UTC (permalink / raw)
To: Harish Patil, dev; +Cc: Dept-EngDPDKDev
In-Reply-To: <1480733039-13046-7-git-send-email-harish.patil@qlogic.com>
On 12/3/2016 2:43 AM, Harish Patil wrote:
> Fix to adverstise max_rx_queues by taking into account the number
s/adverstise/advertise
> of PF connections instead of returning max_queues supported by the
> HW.
Can you please describe what is the effect, what happens if this is not
fixed, and driver keeps reporting max_queues supported by the HW?
>
> Fixes: 2ea6f76a ("qede: add core driver")
>
> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> ---
<...>
^ permalink raw reply
* Re: [PATCH 6/7] net/qede: fix maximum VF count to 0
From: Ferruh Yigit @ 2016-12-08 16:45 UTC (permalink / raw)
To: Harish Patil, dev; +Cc: Dept-EngDPDKDev
In-Reply-To: <1480733039-13046-6-git-send-email-harish.patil@qlogic.com>
On 12/3/2016 2:43 AM, Harish Patil wrote:
> Set max_vfs to 0 since it is relevant only to SR-IOV PF
> which is not supported yet.
>
> Fixes: 2ea6f76a ("qede: add core driver")
>
> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
Can you please update patch title to indicate what has been fixed
instead of what has been done in the patch.
btw, while checking feature list, I have seen qede_vf supports SR-IOV,
is that correct?
> ---
> drivers/net/qede/qede_ethdev.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
> index ee8fb43..10abb8b 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -976,10 +976,7 @@ static int qede_dev_configure(struct rte_eth_dev *eth_dev)
> dev_info->max_rx_queues = (uint16_t)QEDE_MAX_RSS_CNT(qdev);
> dev_info->max_tx_queues = dev_info->max_rx_queues;
> dev_info->max_mac_addrs = qdev->dev_info.num_mac_addrs;
> - if (IS_VF(edev))
> - dev_info->max_vfs = 0;
> - else
> - dev_info->max_vfs = (uint16_t)NUM_OF_VFS(&qdev->edev);
> + dev_info->max_vfs = 0;
> dev_info->reta_size = ECORE_RSS_IND_TABLE_SIZE;
> dev_info->hash_key_size = ECORE_RSS_KEY_SIZE * sizeof(uint32_t);
> dev_info->flow_type_rss_offloads = (uint64_t)QEDE_RSS_OFFLOAD_ALL;
>
^ permalink raw reply
* Re: [PATCH 2/7] net/qede: refactor filtering code
From: Ferruh Yigit @ 2016-12-08 16:45 UTC (permalink / raw)
To: Harish Patil, dev; +Cc: Dept-EngDPDKDev
In-Reply-To: <1480733039-13046-2-git-send-email-harish.patil@qlogic.com>
On 12/3/2016 2:43 AM, Harish Patil wrote:
> The filter_config in qed_eth_ops_pass is a wrapper call driving all the
> filtering configuration. This requires defining multiple structures and
> passing different function arguments for each filter type which is
> unnecessary. So as part of this refactoring we remove filter_config from
> qed_eth_ops_pass and invoke base apis directly. Another change is to
> use a singly list for unicast/multicast macs and thereby prevent duplicate
singly linked list?
> entries.
>
> This change is primarily intended to support future tunneling support
> which makes use of existing L2 filtering/classifications.
>
> Fixes: 2ea6f76a ("qede: add core driver")
What is fixed in this patch, isn't it a refactor?
btw, all Fixes formats are wrong in the patchset, can you please use the
git alias provided:
http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body
>
> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> ---
<...>
^ permalink raw reply
* Re: [PATCH 1/7] net/qede: reduce noise in debug logs
From: Ferruh Yigit @ 2016-12-08 16:44 UTC (permalink / raw)
To: Harish Patil, dev; +Cc: Dept-EngDPDKDev, Rasesh Mody
In-Reply-To: <1480733039-13046-1-git-send-email-harish.patil@qlogic.com>
On 12/3/2016 2:43 AM, Harish Patil wrote:
> From: Rasesh Mody <Rasesh.Mody@cavium.com>
>
> Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
> where each bit represent a particular submodule to debug. Also move
> notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
>
> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
> ---
> config/common_base | 2 +-
> doc/guides/nics/qede.rst | 4 ++--
> drivers/net/qede/qede_ethdev.c | 4 ++--
> drivers/net/qede/qede_logs.h | 21 +++++----------------
> 4 files changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/config/common_base b/config/common_base
> index 4bff83a..2ffd557 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -320,7 +320,7 @@ CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB_L1=n
> CONFIG_RTE_LIBRTE_QEDE_PMD=y
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INIT=n
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n
> -CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n
> +CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL=0
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n
> #Provides abs path/name of the firmware file.
> diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst
> index d22ecdd..ddf4248 100644
> --- a/doc/guides/nics/qede.rst
> +++ b/doc/guides/nics/qede.rst
> @@ -103,9 +103,9 @@ enabling debugging options may affect system performance.
>
> Toggle display of generic debugging messages.
>
> -- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER`` (default **n**)
> +- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL`` (default **0**)
Does it make sense to document how DEBUG_VAL used?
Also commit log says bitmapped value to enable/disable a particular
submodule, you may want to document here which value enable/disable
which submodule.
>
> - Toggle display of ecore related messages.
> + Control driver debug verbosity using 32-bit bitmap flags.
>
> - ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX`` (default **n**)
>
<...>
> diff --git a/drivers/net/qede/qede_logs.h b/drivers/net/qede/qede_logs.h
> index 45c4af0..08fdf04 100644
> --- a/drivers/net/qede/qede_logs.h
> +++ b/drivers/net/qede/qede_logs.h
> @@ -16,15 +16,18 @@
> (p_dev)->name ? (p_dev)->name : "", \
> ##__VA_ARGS__)
>
> +#ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
Is "_INFO" carries any meaning in this config option, why not just
RTE_LIBRTE_QEDE_DEBUG?
> #define DP_NOTICE(p_dev, is_assert, fmt, ...) \
> rte_log(RTE_LOG_NOTICE, RTE_LOGTYPE_PMD,\
> "[QEDE PMD: (%s)]%s:" fmt, \
> (p_dev)->name ? (p_dev)->name : "", \
> __func__, \
> ##__VA_ARGS__)
> +#else
> +#define DP_NOTICE(p_dev, fmt, ...) do { } while (0)
> +#endif
>
> #ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
> -
> #define DP_INFO(p_dev, fmt, ...) \
> rte_log(RTE_LOG_INFO, RTE_LOGTYPE_PMD, \
> "[%s:%d(%s)]" fmt, \
> @@ -33,10 +36,8 @@
> ##__VA_ARGS__)
> #else
> #define DP_INFO(p_dev, fmt, ...) do { } while (0)
> -
> #endif
>
> -#ifdef RTE_LIBRTE_QEDE_DEBUG_DRIVER
Are you sure you want to enable DP_VERBOSE, I guess most verbose log
macro, by default? Perhaps may want to control it via
RTE_LIBRTE_QEDE_DEBUG_INFO?
> #define DP_VERBOSE(p_dev, module, fmt, ...) \
> do { \
> if ((p_dev)->dp_module & module) \
> @@ -46,9 +47,7 @@
> (p_dev)->name ? (p_dev)->name : "", \
> ##__VA_ARGS__); \
> } while (0)
> -#else
> -#define DP_VERBOSE(p_dev, fmt, ...) do { } while (0)
> -#endif
> +
>
<...>
^ permalink raw reply
* Re: [PATCH v2 3/4] app: give python apps a consistent shebang line
From: Thomas Monjalon @ 2016-12-08 16:20 UTC (permalink / raw)
To: John McNamara; +Cc: dev, mkletzan
In-Reply-To: <1481213032-14935-3-git-send-email-john.mcnamara@intel.com>
2016-12-08 16:03, John McNamara:
> Add a consistent "env python" shebang line to the DPDK Python
> apps so that they can call the default system python.
The shebang is only useful for executable scripts.
Those included by other python scripts should not have this line.
Please could you remove the shebang for conf.py and data files?
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Thomas Monjalon @ 2016-12-08 16:06 UTC (permalink / raw)
To: Wiles, Keith
Cc: dev, Nélio Laranjeiro, Ananyev, Konstantin,
Richardson, Bruce, Morten Brørup, Neil Horman, Lu, Wenzhuo,
Olivier Matz
In-Reply-To: <2772CCE2-A35D-449E-922F-6EEC1F03342C@intel.com>
2016-12-08 13:59, Wiles, Keith:
>
> > On Dec 8, 2016, at 3:30 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> >
> > Hi all,
> >
> > Following previous discussions, I would like to gather requirements for
> > v2, currently we have:
> >
> > 1. Introduction of new typedefs.
> > 2. Modification of network headers.
> > 3. Modification of rte_*_to_*() functions.
> >
> > Point 1. seems not to be an issue, everyone seems to agree on the fact
> > having those types could help to document some parts of the code.
>
> I never stated these new types were useful in any way, I still believe documentation of the code is the better solution then forcing yet another restriction in submitting patches.
It would not be a restriction, just a help for those wanting to document
some tricky parts by using these types.
I see 2 usages:
- in a struct:
rte_be32_t speed; /**< 0 for speed negotiation */
instead of
uint32_t speed; /**< [big endian] 0 for speed negotiation */
- in a function:
rte_be32_t decode_speed (void *);
[...]
speed = rte_be_to_cpu_32(decode_speed());
It is difficult to reject something which could help a bit.
Do you really think it would bring some confusion to have some code
using these endianed-types?
^ permalink raw reply
* [PATCH v2 4/4] doc: add required python versions to coding guidelines
From: John McNamara @ 2016-12-08 16:03 UTC (permalink / raw)
To: dev; +Cc: mkletzan, John McNamara
In-Reply-To: <1481212265-10229-1-git-send-email-john.mcnamara@intel.com>
Add a requirement to support both Python 2 and 3 to the
DPDK Python Coding Standards.
Signed-off-by: John McNamara <john.mcnamara@intel.com>
---
doc/guides/contributing/coding_style.rst | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
index 1eb67f3..4163960 100644
--- a/doc/guides/contributing/coding_style.rst
+++ b/doc/guides/contributing/coding_style.rst
@@ -690,6 +690,7 @@ Control Statements
Python Code
-----------
-All python code should be compliant with `PEP8 (Style Guide for Python Code) <https://www.python.org/dev/peps/pep-0008/>`_.
+All Python code should work with Python 2.7+ and 3.2+ and be compliant with
+`PEP8 (Style Guide for Python Code) <https://www.python.org/dev/peps/pep-0008/>`_.
The ``pep8`` tool can be used for testing compliance with the guidelines.
--
2.7.4
^ permalink raw reply related
* [PATCH v2 3/4] app: give python apps a consistent shebang line
From: John McNamara @ 2016-12-08 16:03 UTC (permalink / raw)
To: dev; +Cc: mkletzan, John McNamara
In-Reply-To: <1481212265-10229-1-git-send-email-john.mcnamara@intel.com>
Add a consistent "env python" shebang line to the DPDK Python
apps so that they can call the default system python.
Signed-off-by: John McNamara <john.mcnamara@intel.com>
---
app/test/autotest_test_funcs.py | 2 +-
doc/guides/conf.py | 2 ++
tools/dpdk-devbind.py | 3 ++-
3 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/app/test/autotest_test_funcs.py b/app/test/autotest_test_funcs.py
index c482ea8..1fa8cf0 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 python
# BSD LICENSE
#
diff --git a/doc/guides/conf.py b/doc/guides/conf.py
index 34c62de..97c5d0e 100644
--- a/doc/guides/conf.py
+++ b/doc/guides/conf.py
@@ -1,3 +1,5 @@
+#!/usr/bin/env python
+
# BSD LICENSE
# Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
# All rights reserved.
diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py
index 4f51a4b..a5b2af5 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py
@@ -1,4 +1,5 @@
-#! /usr/bin/python
+#!/usr/bin/env python
+
#
# BSD LICENSE
#
--
2.7.4
^ permalink raw reply related
* [PATCH v2 2/4] app: make python apps python2/3 compliant
From: John McNamara @ 2016-12-08 16:03 UTC (permalink / raw)
To: dev; +Cc: mkletzan, John McNamara
In-Reply-To: <1481212265-10229-1-git-send-email-john.mcnamara@intel.com>
Make all the DPDK Python apps work with Python 2 or 3 to
allow them to work with whatever is the system default.
Signed-off-by: John McNamara <john.mcnamara@intel.com>
---
app/cmdline_test/cmdline_test.py | 26 ++++++++++++------------
app/cmdline_test/cmdline_test_data.py | 2 +-
app/test/autotest.py | 10 ++++-----
app/test/autotest_runner.py | 37 +++++++++++++++++-----------------
tools/cpu_layout.py | 38 ++++++++++++++++++-----------------
tools/dpdk-pmdinfo.py | 12 ++++++-----
6 files changed, 64 insertions(+), 61 deletions(-)
diff --git a/app/cmdline_test/cmdline_test.py b/app/cmdline_test/cmdline_test.py
index 4729987..229f71f 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 python
# BSD LICENSE
#
@@ -32,7 +32,7 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# Script that runs cmdline_test app and feeds keystrokes into it.
-
+from __future__ import print_function
import cmdline_test_data
import os
import pexpect
@@ -81,38 +81,38 @@ def runHistoryTest(child):
# the path to cmdline_test executable is supplied via command-line.
if len(sys.argv) < 2:
- print "Error: please supply cmdline_test app path"
+ print("Error: please supply cmdline_test app path")
sys.exit(1)
test_app_path = sys.argv[1]
if not os.path.exists(test_app_path):
- print "Error: please supply cmdline_test app path"
+ print("Error: please supply cmdline_test app path")
sys.exit(1)
child = pexpect.spawn(test_app_path)
-print "Running command-line tests..."
+print("Running command-line tests...")
for test in cmdline_test_data.tests:
- print (test["Name"] + ":").ljust(30),
+ testname = (test["Name"] + ":").ljust(30)
try:
runTest(child, test)
- print "PASS"
+ print(testname, "PASS")
except:
- print "FAIL"
- print child
+ print(testname, "FAIL")
+ print(child)
sys.exit(1)
# since last test quits the app, run new instance
child = pexpect.spawn(test_app_path)
-print ("History fill test:").ljust(30),
+testname = ("History fill test:").ljust(30)
try:
runHistoryTest(child)
- print "PASS"
+ print(testname, "PASS")
except:
- print "FAIL"
- print child
+ print(testname, "FAIL")
+ print(child)
sys.exit(1)
child.close()
sys.exit(0)
diff --git a/app/cmdline_test/cmdline_test_data.py b/app/cmdline_test/cmdline_test_data.py
index 3ce6cbc..9cc966b 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 python
# BSD LICENSE
#
diff --git a/app/test/autotest.py b/app/test/autotest.py
index 3a00538..5c19a02 100644
--- a/app/test/autotest.py
+++ b/app/test/autotest.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
# BSD LICENSE
#
@@ -32,15 +32,15 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# Script that uses either test app or qemu controlled by python-pexpect
-
+from __future__ import print_function
import autotest_data
import autotest_runner
import sys
def usage():
- print"Usage: autotest.py [test app|test iso image]",
- print "[target] [whitelist|-blacklist]"
+ print("Usage: autotest.py [test app|test iso image] ",
+ "[target] [whitelist|-blacklist]")
if len(sys.argv) < 3:
usage()
@@ -63,7 +63,7 @@ def usage():
cmdline = "%s -c f -n 4" % (sys.argv[1])
-print cmdline
+print(cmdline)
runner = autotest_runner.AutotestRunner(cmdline, target, test_blacklist,
test_whitelist)
diff --git a/app/test/autotest_runner.py b/app/test/autotest_runner.py
index 55b63a8..7aeb0bd 100644
--- a/app/test/autotest_runner.py
+++ b/app/test/autotest_runner.py
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
# BSD LICENSE
#
@@ -271,15 +271,16 @@ def __process_results(self, results):
total_time = int(cur_time - self.start)
# print results, test run time and total time since start
- print ("%s:" % test_name).ljust(30),
- print result_str.ljust(29),
- print "[%02dm %02ds]" % (test_time / 60, test_time % 60),
+ result = ("%s:" % test_name).ljust(30)
+ result += result_str.ljust(29)
+ result += "[%02dm %02ds]" % (test_time / 60, test_time % 60)
# don't print out total time every line, it's the same anyway
if i == len(results) - 1:
- print "[%02dm %02ds]" % (total_time / 60, total_time % 60)
+ print(result,
+ "[%02dm %02ds]" % (total_time / 60, total_time % 60))
else:
- print ""
+ print(result)
# if test failed and it wasn't a "start" test
if test_result < 0 and not i == 0:
@@ -294,7 +295,7 @@ def __process_results(self, results):
f = open("%s_%s_report.rst" %
(self.target, test_name), "w")
except IOError:
- print "Report for %s could not be created!" % test_name
+ print("Report for %s could not be created!" % test_name)
else:
with f:
f.write(report)
@@ -360,12 +361,10 @@ def run_all_tests(self):
try:
# create table header
- print ""
- print "Test name".ljust(30),
- print "Test result".ljust(29),
- print "Test".center(9),
- print "Total".center(9)
- print "=" * 80
+ print("")
+ print("Test name".ljust(30), "Test result".ljust(29),
+ "Test".center(9), "Total".center(9))
+ print("=" * 80)
# make a note of tests start time
self.start = time.time()
@@ -407,11 +406,11 @@ def run_all_tests(self):
total_time = int(cur_time - self.start)
# print out summary
- print "=" * 80
- print "Total run time: %02dm %02ds" % (total_time / 60,
- total_time % 60)
+ print("=" * 80)
+ print("Total run time: %02dm %02ds" % (total_time / 60,
+ total_time % 60))
if self.fails != 0:
- print "Number of failed tests: %s" % str(self.fails)
+ print("Number of failed tests: %s" % str(self.fails))
# write summary to logfile
self.logfile.write("Summary\n")
@@ -420,8 +419,8 @@ def run_all_tests(self):
self.logfile.write("Failed tests: ".ljust(
15) + "%i\n" % self.fails)
except:
- print "Exception occurred"
- print sys.exc_info()
+ print("Exception occurred")
+ print(sys.exc_info())
self.fails = 1
# drop logs from all executions to a logfile
diff --git a/tools/cpu_layout.py b/tools/cpu_layout.py
index ccc22ec..0e049a6 100755
--- a/tools/cpu_layout.py
+++ b/tools/cpu_layout.py
@@ -1,4 +1,5 @@
-#! /usr/bin/python
+#!/usr/bin/env python
+
#
# BSD LICENSE
#
@@ -31,7 +32,7 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#
-
+from __future__ import print_function
import sys
sockets = []
@@ -55,7 +56,7 @@
for core in core_details:
for field in ["processor", "core id", "physical id"]:
if field not in core:
- print "Error getting '%s' value from /proc/cpuinfo" % field
+ print("Error getting '%s' value from /proc/cpuinfo" % field)
sys.exit(1)
core[field] = int(core[field])
@@ -68,29 +69,30 @@
core_map[key] = []
core_map[key].append(core["processor"])
-print "============================================================"
-print "Core and Socket Information (as reported by '/proc/cpuinfo')"
-print "============================================================\n"
-print "cores = ", cores
-print "sockets = ", sockets
-print ""
+print("============================================================")
+print("Core and Socket Information (as reported by '/proc/cpuinfo')")
+print("============================================================\n")
+print("cores = ", cores)
+print("sockets = ", sockets)
+print("")
max_processor_len = len(str(len(cores) * len(sockets) * 2 - 1))
max_core_map_len = max_processor_len * 2 + len('[, ]') + len('Socket ')
max_core_id_len = len(str(max(cores)))
-print " ".ljust(max_core_id_len + len('Core ')),
+output = " ".ljust(max_core_id_len + len('Core '))
for s in sockets:
- print "Socket %s" % str(s).ljust(max_core_map_len - len('Socket ')),
-print ""
+ output += " Socket %s" % str(s).ljust(max_core_map_len - len('Socket '))
+print(output)
-print " ".ljust(max_core_id_len + len('Core ')),
+output = " ".ljust(max_core_id_len + len('Core '))
for s in sockets:
- print "--------".ljust(max_core_map_len),
-print ""
+ output += " --------".ljust(max_core_map_len)
+ output += " "
+print(output)
for c in cores:
- print "Core %s" % str(c).ljust(max_core_id_len),
+ output = "Core %s" % str(c).ljust(max_core_id_len)
for s in sockets:
- print str(core_map[(s, c)]).ljust(max_core_map_len),
- print ""
+ output += " " + str(core_map[(s, c)]).ljust(max_core_map_len)
+ print(output)
diff --git a/tools/dpdk-pmdinfo.py b/tools/dpdk-pmdinfo.py
index 3d3ad7d..097982e 100755
--- a/tools/dpdk-pmdinfo.py
+++ b/tools/dpdk-pmdinfo.py
@@ -1,9 +1,11 @@
#!/usr/bin/env python
+
# -------------------------------------------------------------------------
#
# Utility to dump PMD_INFO_STRING support from an object file
#
# -------------------------------------------------------------------------
+from __future__ import print_function
import json
import os
import platform
@@ -54,7 +56,7 @@ def addDevice(self, deviceStr):
self.devices[devID] = Device(deviceStr)
def report(self):
- print self.ID, self.name
+ print(self.ID, self.name)
for id, dev in self.devices.items():
dev.report()
@@ -80,7 +82,7 @@ def __init__(self, deviceStr):
self.subdevices = {}
def report(self):
- print "\t%s\t%s" % (self.ID, self.name)
+ print("\t%s\t%s" % (self.ID, self.name))
for subID, subdev in self.subdevices.items():
subdev.report()
@@ -126,7 +128,7 @@ def __init__(self, vendor, device, name):
self.name = name
def report(self):
- print "\t\t%s\t%s\t%s" % (self.vendorID, self.deviceID, self.name)
+ print("\t\t%s\t%s\t%s" % (self.vendorID, self.deviceID, self.name))
class PCIIds:
@@ -154,7 +156,7 @@ def reportVendors(self):
"""Reports the vendors
"""
for vid, v in self.vendors.items():
- print v.ID, v.name
+ print(v.ID, v.name)
def report(self, vendor=None):
"""
@@ -185,7 +187,7 @@ def findDate(self, content):
def parse(self):
if len(self.contents) < 1:
- print "data/%s-pci.ids not found" % self.date
+ print("data/%s-pci.ids not found" % self.date)
else:
vendorID = ""
deviceID = ""
--
2.7.4
^ 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