[dpdk-dev] [PATCHv4 4/5] Makefile: Do post processing on objects that register a driver

[Thomas Monjalon]

2016-05-25 15:43, Neil Horman:
> On Wed, May 25, 2016 at 08:56:25PM +0200, Thomas Monjalon wrote:
> > 2016-05-25 13:40, Neil Horman:
> > > On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote:
> > > > 2016-05-24 15:41, Neil Horman:
> > > > > + echo MODGEN $@; \
> > > > > + OBJF=`readlink -f $@`; \
> > > > > + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF 
> > > > > \$$OBJF.mod.c; \
> > > > 
> > > > Maybe .pmd.c would be more appropriate than .mod.c?
> > > fine
> > > > What means mod/MODGEN/MODBUILD?
> > > GENerate Module information & BUILD module information.
> > 
> > I think "module" is not appropriate here.
> > 
> This is starting to feel very much like bikeshedding.  What do you think would
> be more appropriate here?

pmd/PMDINFO//

> > > > It deserves to be in a shell script, at least to ease testing.
> > > What do you mean by "it" and why would it be easier to test in a shell 
> > > script?
> > 
> > "it" is mostly this whole patch.
> > With a shell script, we can test the behaviour on one file easily.
> > Maybe I'm wrong, but I don't like having too much lines in a Makefile rule.
> > We probably need more opinions.
> > 
> That makes no sense to me. Any such script would need to receive two 
> arguments:
> 1) The path to a C file for a pmd
> 2) The path to the corresponding object file for that pmd
> 
> Running any such script is then usless unles its predecated on first building
> all the object files in the pmd.  And if you want to run something by hand on
> the object files, it seems pretty straightforward to do so, just run:
> build/builttools/pmdinfogen /path/to/pmd/object/file
> 
> The rest of that code is really just a test to avoid having to run pmdinfo gen
> on any files other than the ones that contain the PMD_REGISTER_DRIVER macro

OK, no strong opinion here.
If you feel comfortable with multi-lines "sh -c" and escaping, up to you.
If I discover something wrong in this part and needs to do some maintenance
work, I'll probably think differently.

[dpdk-dev] [PATCH v3 13/35] mempool: store physical address in objects

[Olivier Matz]

Hi Deepak,

On 05/25/2016 07:51 PM, Jain, Deepak K wrote:
> Hi,
> 
> While running the QAT PMD tests, a system hang is observed when this commit 
> is used.
> 
> rte_mempool_virt2phy is used in qat_crypto.c.

>From what I see in the code, the second argument of the function
rte_mempool_virt2phy(mp, elt) is not a pointer to a element of
the mempool.

This should be the case according to the API (even before my patchset):

  * @param elt
  *   A pointer (virtual address) to the element of the pool.


Could you try to replace:

  s->cd_paddr = rte_mempool_virt2phy(mp, >cd)

By something like:

  s->cd_paddr = rte_mempool_virt2phy(mp, s) +
offsetof(struct qat_session, cd)



Regards,
Olivier

[dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility

[Thomas Monjalon]

2016-05-25 15:13, Neil Horman:
> On Wed, May 25, 2016 at 07:39:30PM +0200, Thomas Monjalon wrote:
> > 2016-05-25 13:22, Neil Horman:
> > > On Wed, May 25, 2016 at 03:21:19PM +0200, Thomas Monjalon wrote:
> > > > 2016-05-24 15:41, Neil Horman:
> > > > > +include $(RTE_SDK)/mk/rte.buildtools.mk
> > > > 
> > > > Why a new Makefile? Can you use rte.hostapp.mk?
> > > > 
> > > I don't know, maybe.  Nothing else currently uses rte.hostapp.mk, so I 
> > > missed
> > > its existance.  I make the argument that, that being the case, we should 
> > > stick
> > > with the Makefile I just tested with, and remove the rte.hostapp.mk file
> > 
> > No, rte.hostapp.mk has been used and tested in the history of the project.
> > Please try it.
> > 
> It works, but its really ugly (as it means that the buildtools directory gets
> install to the hostapp directory under the build).  I could move that of 
> course,
> but at this point, you are asking me to remove a working makefile to replace 
> it
> with another makefile that, by all rights should have been removed as part of
> commit efa2084a840fb83fd9be83adca57e5f23d3fa9fe:
> Author: Thomas Monjalon 
> Date:   Tue Mar 10 17:55:25 2015 +0100
> 
> scripts: remove useless build tools
> 
> test-framework.sh is an old script to check building of some dependencies.
> testhost is an old app used to check HOSTCC.
> 
> Let's clean the scripts directory.
> 
> Here you removed the only user of rte.hostapp.mk, but neglected to remove
> hostapp.mk itself.

Yes. I didn't really neglect to remove it. I thought it would be used later.

> I really fail to see why making me rework my current
> makefile setup, that matches the purpose of the tool is a superior solution to
> just getting rid of the unused makefile thats there right now.

I'm just trying to avoid creating a new makefile for each tool.
Is it possible to fix the directory in rte.hostapp.mk?
Every apps use the same makefile rte.app.mk. I think it should be the same
for host apps.

> > > > > +++ b/buildtools/pmdinfogen/pmdinfogen.c
> > > > [...]
> > > > > + /*
> > > > > +  * If this returns NULL, then this is a PMD_VDEV, because
> > > > > +  * it has no pci table reference
> > > > > +  */
> > > > 
> > > > We can imagine physical PMD not using PCI.
> > > > I think this comment should be removed.
> > > We can, but currently its a true statement.  we have two types of PMDs, a 
> > > PDEV
> > > and a VDEV, the former is a pci device, and the latter is a virtual 
> > > device, so
> > > you can imply the PDEV type from the presence of pci entries, and VDEV 
> > > from the
> > > alternative.  If we were to do something, I would recommend adding a 
> > > macro to
> > > explicitly ennumerate each pmds type.  I would prefer to wait until that 
> > > was a
> > > need however, as it can be done invisibly to the user.
> > 
> > We are removing the PMD types in the EAL rework.
> > So this comment will be outdated. Better to remove now.
> > 
> Then, I'm just not going to enumerate the type of driver at all, I'll remove
> that attribute entirely.

OK

> But I really don't like to write code for things that are 'predictive'.

Not really predictive as it is an older patch.

> > > > [...]
> > > > > + fprintf(ofd,"\\\"type\\\" : \\\"%s\\\", ", drv->pci_tbl 
> > > > > ? "PMD_PDEV" : "PMD_VDEV");
> > > > 
> > > > Please forget the naming PDEV/VDEV.
> > > > 
> > > I don't know what you mean here, you would rather they be named PCI and 
> > > Virtual,
> > > or something else?
> > 
> > Yes please.
> > 
> No, If you're removing the types, and you're sure of that, I'm just going to
> remove the description entirely.  If you're unsure about exactly whats going 
> to
> happen, we should reflect the state of the build now, and make the appropriate
> change when it lands.

OK to remove the type description.

> > > > > +++ b/buildtools/pmdinfogen/pmdinfogen.h
> > > > [...]
> > > > > +#define Elf_EhdrElf64_Ehdr
> > > > > +#define Elf_ShdrElf64_Shdr
> > > > > +#define Elf_Sym Elf64_Sym
> > > > > +#define Elf_AddrElf64_Addr
> > > > > +#define Elf_Sword   Elf64_Sxword
> > > > > +#define Elf_Section Elf64_Half
> > > > > +#define ELF_ST_BIND ELF64_ST_BIND
> > > > > +#define ELF_ST_TYPE ELF64_ST_TYPE
> > > > > +
> > > > > +#define Elf_Rel Elf64_Rel
> > > > > +#define Elf_RelaElf64_Rela
> > > > > +#define ELF_R_SYM   ELF64_R_SYM
> > > > > +#define ELF_R_TYPE  ELF64_R_TYPE
> > > > 
> > > > Why these defines are needed?
> > > > 
> > > Because I borrowed the code from modpost.c, which allows for both ELF32 
> > > and
> > > ELF64 compilation.  I wanted to keep it in place should DPDK ever target
> > > different sized architectures.
> > 
> > Maybe a comment is needed.
> > Is ELF32 used on 32-bit archs like i686 or ARMv7?
> It depends on exactly how its built, but that would be a common use, yes.

We have such 32-bit archs in DPDK. Is pmdinfogen working for them?

> > > > > +struct rte_pci_id {
> > > > > + 

[dpdk-dev] [PATCHv4 5/5] pmdinfo.py: Add tool to query binaries for hw and other support information

[Thomas Monjalon]

2016-05-25 13:47, Neil Horman:
> On Wed, May 25, 2016 at 07:22:39PM +0200, Thomas Monjalon wrote:
> > 2016-05-24 15:41, Neil Horman:
> > > Note that, in the case of dynamically linked applications, pmdinfo.py 
> > > will scan
> > > for implicitly linked PMDs by searching the specified binaries .dynamic 
> > > section
> > > for DT_NEEDED entries that contain the substring librte_pmd.
> > 
> > I don't know any DPDK app dynamically linked with a PMD (with DT_NEEDED).
> I know lots of them, they're all in the dpdk.  everything under app that uses 
> a
> virutal device links at link time to librte_pmd_bonding and librte_pmd_pipe 
> (and
> a few others), because they have additional apis that they need to resolve at
> load time.

Oh yes! you are right.

> > However it is a good idea to handle this case.
> > But relying on the name assumption "librte_pmd" is really weak.
> > 
> > > + $(Q)$(call rte_symlink,$(DESTDIR)$(datadir)/tools/pmdinfo.py, \
> > > +$(DESTDIR)$(bindir)/pmdinfo)
> > 
> > I think we must prefix the tool name with dpdk.
> > What about dpdk-objinfo or dpdk-pmdinfo?
> > 
> > > +from elftools.elf.elffile import ELFFile
> > > +from elftools.elf.dynamic import DynamicSection, DynamicSegment
> > > +from elftools.elf.enums import ENUM_D_TAG
> > > +from elftools.elf.segments import InterpSegment
> > > +from elftools.elf.sections import SymbolTableSection
> > 
> > Should it be possible to implement pmdinfogen with this
> > Python library?
> > 
> Sure, but that really doesn't buy us anything, as its already implemented in 
> C.
> In fact, I would assert its harmful, because it implies that the build
> environment needs to have python installed, as well as the pyelftools library,
> which we don't need if we build from C.

Right

[dpdk-dev] [PATCHv4 4/5] Makefile: Do post processing on objects that register a driver

[Thomas Monjalon]

2016-05-25 13:40, Neil Horman:
> On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote:
> > 2016-05-24 15:41, Neil Horman:
> > > + echo MODGEN $@; \
> > > + OBJF=`readlink -f $@`; \
> > > + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \
> > 
> > Maybe .pmd.c would be more appropriate than .mod.c?
> fine
> > What means mod/MODGEN/MODBUILD?
> GENerate Module information & BUILD module information.

I think "module" is not appropriate here.

> > It deserves to be in a shell script, at least to ease testing.
> What do you mean by "it" and why would it be easier to test in a shell script?

"it" is mostly this whole patch.
With a shell script, we can test the behaviour on one file easily.
Maybe I'm wrong, but I don't like having too much lines in a Makefile rule.
We probably need more opinions.

[dpdk-dev] [PATCH v2] enic: fix seg fault when releasing queues

[John Daley]

If device configuration failed due to a lack of resources, like if
there were more queues requested than available, the queue release
function is called with NULL pointers which were being dereferenced.

Skip releasing queues if they are NULL pointers. Also, if configuration
fails due to lack of resources, be more specific about which resources
are lacking.

Fixes: fefed3d1e62c ("enic: new driver")
Signed-off-by: John Daley 
---
v2: Log an error for all resource deficiencies not just the first one
found.

 drivers/net/enic/enic_main.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 996f999..411d23c 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -426,14 +426,16 @@ int enic_alloc_intr_resources(struct enic *enic)

 void enic_free_rq(void *rxq)
 {
-   struct vnic_rq *rq = (struct vnic_rq *)rxq;
-   struct enic *enic = vnic_dev_priv(rq->vdev);
+   if (rxq != NULL) {
+   struct vnic_rq *rq = (struct vnic_rq *)rxq;
+   struct enic *enic = vnic_dev_priv(rq->vdev);

-   enic_rxmbuf_queue_release(enic, rq);
-   rte_free(rq->mbuf_ring);
-   rq->mbuf_ring = NULL;
-   vnic_rq_free(rq);
-   vnic_cq_free(>cq[rq->index]);
+   enic_rxmbuf_queue_release(enic, rq);
+   rte_free(rq->mbuf_ring);
+   rq->mbuf_ring = NULL;
+   vnic_rq_free(rq);
+   vnic_cq_free(>cq[rq->index]);
+   }
 }

 void enic_start_wq(struct enic *enic, uint16_t queue_idx)
@@ -816,22 +818,29 @@ static void enic_dev_deinit(struct enic *enic)
 int enic_set_vnic_res(struct enic *enic)
 {
struct rte_eth_dev *eth_dev = enic->rte_dev;
+   int rc = 0;

-   if ((enic->rq_count < eth_dev->data->nb_rx_queues) ||
-   (enic->wq_count < eth_dev->data->nb_tx_queues)) {
-   dev_err(dev, "Not enough resources configured, aborting\n");
-   return -1;
+   if (enic->rq_count < eth_dev->data->nb_rx_queues) {
+   dev_err(dev, "Not enough Receive queues. Requested:%u, 
Configured:%u\n",
+   eth_dev->data->nb_rx_queues, enic->rq_count);
+   rc = -1;
+   }
+   if (enic->wq_count < eth_dev->data->nb_tx_queues) {
+   dev_err(dev, "Not enough Transmit queues. Requested:%u, 
Configured:%u\n",
+   eth_dev->data->nb_tx_queues, enic->wq_count);
+   rc = -1;
}

enic->rq_count = eth_dev->data->nb_rx_queues;
enic->wq_count = eth_dev->data->nb_tx_queues;
if (enic->cq_count < (enic->rq_count + enic->wq_count)) {
-   dev_err(dev, "Not enough resources configured, aborting\n");
-   return -1;
+   dev_err(dev, "Not enough Completion queues. Required:%u, 
Configured:%u\n",
+   enic->rq_count + enic->wq_count, enic->cq_count);
+   rc = -1;
}

enic->cq_count = enic->rq_count + enic->wq_count;
-   return 0;
+   return rc;
 }

 static int enic_dev_init(struct enic *enic)
-- 
2.7.0

[dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility

[Thomas Monjalon]

2016-05-25 13:22, Neil Horman:
> On Wed, May 25, 2016 at 03:21:19PM +0200, Thomas Monjalon wrote:
> > 2016-05-24 15:41, Neil Horman:
> > > --- a/GNUmakefile
> > > +++ b/GNUmakefile
> > > -ROOTDIRS-y := lib drivers app
> > > +ROOTDIRS-y := buildtools lib drivers app
> > 
> > Why a new directory?
> > It is not a script nor an end-user tool, I guess.
> Dependencies.  This tool has to be built prior to the rest of the dpdk, but 
> app
> already relies on dpdk libraries to be built, so you get circular 
> dependencies.
> I could have put it in scripts I guess, but its not a script.  Its own 
> directory
> seemed to make the most sense, given those two points

OK

> > > +include $(RTE_SDK)/mk/rte.buildtools.mk
> > 
> > Why a new Makefile? Can you use rte.hostapp.mk?
> > 
> I don't know, maybe.  Nothing else currently uses rte.hostapp.mk, so I missed
> its existance.  I make the argument that, that being the case, we should stick
> with the Makefile I just tested with, and remove the rte.hostapp.mk file

No, rte.hostapp.mk has been used and tested in the history of the project.
Please try it.

> > > +++ b/buildtools/pmdinfogen/pmdinfogen.c
> > [...]
> > > + /*
> > > +  * If this returns NULL, then this is a PMD_VDEV, because
> > > +  * it has no pci table reference
> > > +  */
> > 
> > We can imagine physical PMD not using PCI.
> > I think this comment should be removed.
> We can, but currently its a true statement.  we have two types of PMDs, a PDEV
> and a VDEV, the former is a pci device, and the latter is a virtual device, so
> you can imply the PDEV type from the presence of pci entries, and VDEV from 
> the
> alternative.  If we were to do something, I would recommend adding a macro to
> explicitly ennumerate each pmds type.  I would prefer to wait until that was a
> need however, as it can be done invisibly to the user.

We are removing the PMD types in the EAL rework.
So this comment will be outdated. Better to remove now.

> > [...]
> > > + fprintf(ofd,"\\\"type\\\" : \\\"%s\\\", ", drv->pci_tbl ? 
> > > "PMD_PDEV" : "PMD_VDEV");
> > 
> > Please forget the naming PDEV/VDEV.
> > 
> I don't know what you mean here, you would rather they be named PCI and 
> Virtual,
> or something else?

Yes please.

> > [...]
> > > + if (info.drivers) {
> > > + output_pmd_info_string(, argv[2]);
> > > + rc = 0;
> > > + } else {
> > > + fprintf(stderr, "Hmm, Appears to be a driver but no drivers 
> > > registered\n");
> > 
> > Why it appears to be a driver?
> > What means "no drivers registered" exactly?
> > 
> It means that the tool has identified this file as a driver based on some
> criteria (in this case the source code contained a use of the
> PMD_REGISTER_DRIVER macro, but for whatever reason, when this tool scanned it,
> it never located the pmd_driver_name symbol.  It should never happen, and
> serves as a indicator to the developer that they need to investigate either 
> the
> construction of the driver or the use of this tool.

OK

> > > +++ b/buildtools/pmdinfogen/pmdinfogen.h
> > [...]
> > > +#define Elf_EhdrElf64_Ehdr
> > > +#define Elf_ShdrElf64_Shdr
> > > +#define Elf_Sym Elf64_Sym
> > > +#define Elf_AddrElf64_Addr
> > > +#define Elf_Sword   Elf64_Sxword
> > > +#define Elf_Section Elf64_Half
> > > +#define ELF_ST_BIND ELF64_ST_BIND
> > > +#define ELF_ST_TYPE ELF64_ST_TYPE
> > > +
> > > +#define Elf_Rel Elf64_Rel
> > > +#define Elf_RelaElf64_Rela
> > > +#define ELF_R_SYM   ELF64_R_SYM
> > > +#define ELF_R_TYPE  ELF64_R_TYPE
> > 
> > Why these defines are needed?
> > 
> Because I borrowed the code from modpost.c, which allows for both ELF32 and
> ELF64 compilation.  I wanted to keep it in place should DPDK ever target
> different sized architectures.

Maybe a comment is needed.
Is ELF32 used on 32-bit archs like i686 or ARMv7?

> > > +struct rte_pci_id {
> > > + uint16_t vendor_id;   /**< Vendor ID or PCI_ANY_ID. */
> > > + uint16_t device_id;   /**< Device ID or PCI_ANY_ID. */
> > > + uint16_t subsystem_vendor_id; /**< Subsystem vendor ID or PCI_ANY_ID. */
> > > + uint16_t subsystem_device_id; /**< Subsystem device ID or PCI_ANY_ID. */
> > > +};
> > [...]
> > > +struct pmd_driver {
> > > + Elf_Sym *name_sym;
> > > + const char *name;
> > > + struct rte_pci_id *pci_tbl;
> > > + struct pmd_driver *next;
> > > +
> > > + const char* opt_vals[PMD_OPT_MAX];
> > > +};
> > 
> > Are you duplicating some structures from EAL?
> > It will be out of sync easily.
> > 
> Only the rte_pci_id, which hasn't changed since the initial public release of
> the DPDK.  We can clean this up later if you like, but I'm really not too
> worried about it.

I would prefer an include if possible.
rte_pci_id is changing in 16.07 ;)

> > > +struct elf_info {
> > > + unsigned long size;
> > > + Elf_Ehdr *hdr;
> > > + Elf_Shdr *sechdrs;
> > > + Elf_Sym  *symtab_start;
> > > + Elf_Sym  *symtab_stop;
> > > + Elf_Section  export_sec;
> 

[dpdk-dev] [vpp-dev] VLAN packets dropped... ?

[Ananyev, Konstantin]

> I suppose this has to do with what is expected usage of the PKT_RX_VLAN_PKT 
> offload flag. Is it set only for VLAN packets with the
> VLAN stripped or should always be set if VLAN is/was present in the received 
> packet. It seems that different DPDK drivers are
> behaving differently which will make it really hard for VPP to take advantage 
> of NIC and driver offload capability to provide better
> performance.

Yes, that's true ixgbe/igb from one side and i40e do raise PKT_RX_VLAN_PKT in a 
different manner.
There is an attempt to make it unified across all supported devices:
 http://dpdk.org/dev/patchwork/patch/12938/

Though, I am not sure it will help you with your issue.
As I understand, for you the desired behaviour is:
If it is a vlan packet, keep the packet intact (don't strip the vlan) and raise 
PKT_RX_VLAN_PK inside mbuf->ol_flags.
That's what ixgbe is doing with rte_eth_conf.rxmode.hw_vlan_strip==0.
Correct?
As far as I know, i40e HW doesn't provide such ability.
i40e Receive HW Descriptor can only flag was VLAN tag stripped from the packet 
or not,
but if stripping is disabled it wouldn't indicate in any way is VLAN tag is 
present inside the packet or not.
I am CC-ing it to dpdk.org in case I am missing something here.
Probably someone knows a way to make it work in that way.
Konstantin

> 
> If VPP cannot rely on offload flags for VLAN so packets always have to go 
> through ethernet-input node, there is a performance cost.
> For the 10GE case, before the inverse patch of the ixgbe driver, 10GE 
> Rx-vector path removed support of offload flag with DPDK 16.04
> and so ethernet-input node is always used. The 10GE IPv4 throughput rate 
> dropped from 6.17MPPSx2 to 4.92MPPSx2 for bidirectional
> traffic (2 streams each with a single IP address as destination) on a single 
> core on my server. Konstantin suggested at that time to use
> scalar mode which does support offload flags properly. The scalar mode did 
> by-pass ethernet-input and provided throughput of
> 5.72MPPS. We ended up inverse patched the ixgbe driver to restore vector mode 
> offload flag support as the original restriction (the
> reason offload flag support was removed) would not affect VPP.
> 
> I think for 40GE driver to provide offload flag support in vector mode but 
> not give indication of presence of VLAN tag is just wrong. This
> make the offload flag support useless for VPP.
> 
> Regards,
> John
> 
> -Original Message-
> From: Ananyev, Konstantin [mailto:konstantin.ananyev at intel.com]
> Sent: Wednesday, May 25, 2016 11:30 AM
> To: John Lo (loj); Wiles, Keith; Chandrasekar Kannan
> Cc: vpp-dev; Zhang, Helin
> Subject: RE: [vpp-dev] VLAN packets dropped... ?
> 
> 
> >
> > I see what you are getting at, Konstantin. The VPP init code does not
> > enable VLAN strip for Intel NICs as VLAN tag must be in the packet for
> > sub-interface lookup by ethernet-input node. I agree if we enable VLAN tag 
> > strip for the i40e driver, we can get around this problem
> but then all packets will be considered as received on the main interface.
> 
> I see...
> As far as I  know, when VLAN stripping is disabled,  i40e RXD doesn't contain 
> information does that packet contain a VLAN or not.
> So, if enabling vlan stripping is not an option for you guys, then I don't 
> see any other way on i40e to recognise is that  VLAN packet or
> not, except then parse the packet in SW.
> Helin, please correct me here, if I am missing something here.
> Thanks
> Konstantin
> 
> >
> > Regards,
> > John
> >
> > -Original Message-
> > From: Ananyev, Konstantin [mailto:konstantin.ananyev at intel.com]
> > Sent: Wednesday, May 25, 2016 10:35 AM
> > To: John Lo (loj); Wiles, Keith; Chandrasekar Kannan
> > Cc: vpp-dev
> > Subject: RE: [vpp-dev] VLAN packets dropped... ?
> >
> >
> > >
> > > Since this is the XL710 40GE NIC, I suppose the DPDK driver involved 
> > > would be the i40e driver and not ixgbe for 10GE NICs.
> >
> > Yes, I understand that you are facing problem on i40e, not ixgbe.
> > And the problem is that for i40e PKT_RX_VLAN_PKT flag is not set in 
> > mbuf->ol_flags, correct?
> > That's why I asked: are you running it with  
> > rte_eth_conf.rxmode.hw_vlan_strip==0 or not?
> > If yes, you can try with rte_eth_conf.rxmode.hw_vlan_strip=1 and see would 
> > it help you.
> >
> > >
> > > As explained before, ixgbe driver had the inverse patch added. It
> > > does recognize VLAN tag in the packet and set PKT_RX_VLAN_PKT offload 
> > > flag  properly:
> >
> > That patch has nothing to do with PKT_RX_VLAN_PKT and i40e.
> > So I don't think it is related to that problem at all.
> > Konstantin
> >
> > >
> > > 00:01:02:132370: dpdk-input
> > >   TenGigabitEthernet5/0/0 rx queue 0
> > >   buffer 0x44cff: current data 0, length 96, free-list 0, totlen-nifb 0, 
> > > trace 0x0
> > >   PKT MBUF: port 3, nb_segs 1, pkt_len 96
> > > buf_len 2176, data_len 96, ol_flags 0x1,
> > > packet_type 0x210
> > > Packet Offload 

[dpdk-dev] [PATCHv4 5/5] pmdinfo.py: Add tool to query binaries for hw and other support information

[Thomas Monjalon]

2016-05-24 15:41, Neil Horman:
> Note that, in the case of dynamically linked applications, pmdinfo.py will 
> scan
> for implicitly linked PMDs by searching the specified binaries .dynamic 
> section
> for DT_NEEDED entries that contain the substring librte_pmd.

I don't know any DPDK app dynamically linked with a PMD (with DT_NEEDED).
However it is a good idea to handle this case.
But relying on the name assumption "librte_pmd" is really weak.

> + $(Q)$(call rte_symlink,$(DESTDIR)$(datadir)/tools/pmdinfo.py, \
> +$(DESTDIR)$(bindir)/pmdinfo)

I think we must prefix the tool name with dpdk.
What about dpdk-objinfo or dpdk-pmdinfo?

> +from elftools.elf.elffile import ELFFile
> +from elftools.elf.dynamic import DynamicSection, DynamicSegment
> +from elftools.elf.enums import ENUM_D_TAG
> +from elftools.elf.segments import InterpSegment
> +from elftools.elf.sections import SymbolTableSection

Should it be possible to implement pmdinfogen with this
Python library?

I'll probably comment on the pmdinfo script details later.
Just knowing you did a tool is enough to assert that it is a good step :)
Thanks

[dpdk-dev] [PATCHv4 4/5] Makefile: Do post processing on objects that register a driver

[Thomas Monjalon]

2016-05-24 15:41, Neil Horman:
> --- a/mk/internal/rte.compile-pre.mk
> +++ b/mk/internal/rte.compile-pre.mk
> @@ -80,7 +80,8 @@ C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax 
> highlight
>  C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)","  HOSTCC $(@)")
>  else
>  C_TO_O = $(CC) -Wp,-MD,$(call obj2dep,$(@)).tmp $(CFLAGS) \
> - $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $<
> +  $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $<
> +

whitespace change?

>  C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight
>  C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)","  CC $(@)")
>  endif
> @@ -88,10 +89,26 @@ C_TO_O_CMD = 'cmd_$@ = $(C_TO_O_STR)'
>  C_TO_O_DO = @set -e; \
>   echo $(C_TO_O_DISP); \
>   $(C_TO_O) && \
> + sh -c "grep -q \"PMD_REGISTER_DRIVER(.*)\" $<; \
> + if [ \$$? -eq 0 ]; \
> + then \

It is preferred to keep "then" at the end of the previous line.

> + echo MODGEN $@; \
> + OBJF=`readlink -f $@`; \
> + ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \

Maybe .pmd.c would be more appropriate than .mod.c?
What means mod/MODGEN/MODBUILD?

> + if [ \$$? -eq 0 ]; \
> + then \
> + echo MODBUILD $@; \
> + $(CC) -c -o \$$OBJF.mod.o \$$OBJF.mod.c; \
> + $(CROSS)ld -r -o \$$OBJF.o \$$OBJF.mod.o \$$OBJF; \
> + mv -f \$$OBJF.o \$$OBJF; \
> + fi; \
> + fi; \
> + true" && \

Why "true"?

It deserves to be in a shell script, at least to ease testing.

[dpdk-dev] [PATCH v2 09/40] bnxt: add L2 filter alloc/init/free

[Bruce Richardson]

On Fri, May 13, 2016 at 03:45:58PM -0700, Stephen Hurd wrote:
> Add the L2 filter structure and the alloc/init/free functions for
> dealing with them.
> 

The DPDK ethdev API has filtering APIs, but this code is not made accessible
via those APIs. If that link is added via later patches, then that should be
documented in the commit message here.


> +/* hwrm_cfa_l2_filter_alloc */
> +/*
> + * Description: An L2 filter is a filter resource that is used to identify a
> + * vnic or ring for a packet based on layer 2 fields. Layer 2 fields for
> + * encapsulated packets include both outer L2 header and/or inner l2 header 
> of
> + * encapsulated packet. The L2 filter resource covers the following OS 
> specific
> + * L2 filters. Linux/FreeBSD (per function): # Broadcast enable/disable # 
> List
> + * of individual multicast filters # All multicast enable/disable filter #
> + * Unicast filters # Promiscuous mode VMware: # Broadcast enable/disable (per
> + * physical function) # All multicast enable/disable (per function) # Unicast
> + * filters per ring or vnic # Promiscuous mode per PF Windows: # Broadcast
> + * enable/disable (per physical function) # List of individual multicast 
> filters
> + * (Driver needs to advertise the maximum number of filters supported) # All
> + * multicast enable/disable per physical function # Unicast filters per vnic 
> #
> + * Promiscuous mode per PF Implementation notes on the use of VNIC in this
> + * command: # By default, these filters belong to default vnic for the 
> function.
> + * # Once these filters are set up, only destination VNIC can be modified. # 
> If
> + * the destination VNIC is not specified in this command, then the HWRM shall
> + * only create an l2 context id. HWRM Implementation notes for multicast
> + * filters: # The hwrm_filter_alloc command can be used to set up multicast
> + * filters (perfect match or partial match). Each individual function driver 
> can
> + * set up multicast filters independently. # The HWRM needs to keep track of
> + * multicast filters set up by function drivers and maintain multicast group
> + * replication records to enable a subset of functions to receive traffic 
> for a
> + * specific multicast address. # When a specific multicast filter cannot be 
> set,
> + * the HWRM shall return an error. In this error case, the driver should fall
> + * back to using one general filter (rather than specific) for all multicast
> + * traffic. # When the SR-IOV is enabled, the HWRM needs to additionally 
> track
> + * source knockout per multicast group record. Examples of setting unicast
> + * filters: For a unicast MAC based filter, one can use a combination of the
> + * fields and masks provided in this command to set up the filter. Below are
> + * some examples: # MAC + no VLAN filter: This filter is used to identify
> + * traffic that does not contain any VLAN tags and matches destination (or
> + * source) MAC address. This filter can be set up by setting only l2_addr 
> field
> + * to be a valid field. All other fields are not valid. The following value 
> is
> + * set for l2_addr. l2_addr = MAC # MAC + Any VLAN filter: This filter is 
> used
> + * to identify traffic that carries single VLAN tag and matches (destination 
> or
> + * source) MAC address. This filter can be set up by setting only l2_addr and
> + * l2_ovlan_mask fields to be valid fields. All other fields are not valid. 
> The
> + * following values are set for those two valid fields. l2_addr = MAC,
> + * l2_ovlan_mask = 0x # MAC + no VLAN or VLAN ID=0: This filter is used 
> to
> + * identify untagged traffic that does not contain any VLAN tags or a VLAN 
> tag
> + * with VLAN ID = 0 and matches destination (or source) MAC address. This 
> filter
> + * can be set up by setting only l2_addr and l2_ovlan fields to be valid 
> fields.
> + * All other fields are not valid. The following value are set for l2_addr 
> and
> + * l2_ovlan. l2_addr = MAC, l2_ovlan = 0x0 # MAC + no VLAN or any VLAN: This
> + * filter is used to identify traffic that contains zero or 1 VLAN tag and
> + * matches destination (or source) MAC address. This filter can be set up by
> + * setting only l2_addr, l2_ovlan, and l2_mask fields to be valid fields. All
> + * other fields are not valid. The following value are set for l2_addr,
> + * l2_ovlan, and l2_mask fields. l2_addr = MAC, l2_ovlan = 0x0, 
> l2_ovlan_mask =
> + * 0x # MAC + VLAN ID filter: This filter can be set up by setting only
> + * l2_addr, l2_ovlan, and l2_ovlan_mask fields to be valid fields. All other
> + * fields are not valid. The following values are set for those three valid
> + * fields. l2_addr = MAC, l2_ovlan = VLAN ID, l2_ovlan_mask = 0xF000
> + */
This comment could do with clean-up to improve formatting and readability. I'm
also not sure that an explanation of this size is best placed as a comment on
a function. However, I'm also not sure where this information is best placed as
these filter functions are all 

[dpdk-dev] [PATCH v2 08/40] bnxt: add completion ring support

[Bruce Richardson]

On Fri, May 13, 2016 at 03:45:57PM -0700, Stephen Hurd wrote:
> Structures, macros, and functions for working with completion rings
> in the driver.
> 
Can you add a bit more info in the commit message - and possibly in the docs 
too - about what completion rings are and how they are used. Even a few lines
of further explanation would help those looking to understand the driver.

/Bruce

[dpdk-dev] [PATCH v2] e1000: fix build with clang

[Thomas Monjalon]

2016-05-26 00:25, Hiroyuki Mikita:
> GCC_VERSION is empty in case of clang:
>   /bin/sh: line 0: test: -ge: unary operator expected
> 
> It is the same issue as http://dpdk.org/dev/patchwork/patch/5994/
> 
> Fixes: 366113dbfb69 ("e1000: suppress misleading indentation warning")
> 
> Signed-off-by: Hiroyuki Mikita 
> ---
> v2:
> * fix for cross compier

The output of git grep '(CC)' shows that there is some room for
cross-compilation fixes.
Any volunteer?

> +ifeq ($(findstring gcc, $(CC)), gcc)
>  ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1)

Looks good, thanks

[dpdk-dev] [PATCH] qede: fix build issue in the cross-compiling mode

[Ferruh Yigit]

On 5/25/2016 10:41 AM, Jerin Jacob wrote:
> In cross-compiling mode CC can be aarch64-*-linux-gnu-gcc
> instead of just gcc
> 
> Signed-off-by: Jerin Jacob 
> ---
>  drivers/net/qede/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/qede/Makefile b/drivers/net/qede/Makefile
> index c9b3b1c..10ced84 100644
> --- a/drivers/net/qede/Makefile
> +++ b/drivers/net/qede/Makefile
> @@ -47,7 +47,7 @@ endif
>  endif
>  endif
>  
> -ifneq (,$(filter gcc gcc48,$(CC)))
> +ifneq (,$(filter %gcc %gcc48,$(CC)))

What about: ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
This saves adding gcc version or cross compilation related check.

>  CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable
>  CFLAGS_BASE_DRIVER += -Wno-missing-declarations
>  CFLAGS_BASE_DRIVER += -Wno-maybe-uninitialized
> 

[dpdk-dev] [PATCH v3 2/4] ixgbe: implement vector PMD for arm architecture

[Jerin Jacob]

On Fri, May 06, 2016 at 11:55:46AM +0530, Jianbo Liu wrote:
> use ARM NEON intrinsic to implement ixgbe vPMD
> 
> Signed-off-by: Jianbo Liu 
> ---
>  drivers/net/ixgbe/Makefile  |   4 +
>  drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c | 561 
> 
>  2 files changed, 565 insertions(+)
>  create mode 100644 drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c
> 
> diff --git a/drivers/net/ixgbe/Makefile b/drivers/net/ixgbe/Makefile
> index 50bf51c..b1c7a60 100644
> --- a/drivers/net/ixgbe/Makefile
> +++ b/drivers/net/ixgbe/Makefile
> @@ -108,7 +108,11 @@ SRCS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += ixgbe_rxtx.c
>  SRCS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += ixgbe_ethdev.c
>  SRCS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += ixgbe_fdir.c
>  SRCS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += ixgbe_pf.c
> +ifeq ($(CONFIG_RTE_ARCH_ARM64),y)
> +SRCS-$(CONFIG_RTE_IXGBE_INC_VECTOR) += ixgbe_rxtx_vec_neon.c
> +else
>  SRCS-$(CONFIG_RTE_IXGBE_INC_VECTOR) += ixgbe_rxtx_vec.c
> +endif
>  
>  ifeq ($(CONFIG_RTE_NIC_BYPASS),y)
>  SRCS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += ixgbe_bypass.c
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c 
> b/drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c
> new file mode 100644
> index 000..11a6115
> --- /dev/null
> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c
> @@ -0,0 +1,561 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> + *   All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + *   notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + *   notice, this list of conditions and the following disclaimer in
> + *   the documentation and/or other materials provided with the
> + *   distribution.
> + * * Neither the name of Intel Corporation nor the names of its
> + *   contributors may be used to endorse or promote products derived
> + *   from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "ixgbe_ethdev.h"
> +#include "ixgbe_rxtx.h"
> +#include "ixgbe_rxtx_vec_common.h"
> +
> +#include 
> +
> +#pragma GCC diagnostic ignored "-Wcast-qual"
> +
> +static inline void
> +ixgbe_rxq_rearm(struct ixgbe_rx_queue *rxq)
> +{
> + int i;
> + uint16_t rx_id;
> + volatile union ixgbe_adv_rx_desc *rxdp;
> + struct ixgbe_rx_entry *rxep = >sw_ring[rxq->rxrearm_start];
> + struct rte_mbuf *mb0, *mb1;
> + uint64x2_t dma_addr0, dma_addr1;
> + uint64x2_t zero = vdupq_n_u64(0);
> + uint64_t paddr;
> + uint8x8_t p;
> +
> + rxdp = rxq->rx_ring + rxq->rxrearm_start;
> +
> + /* Pull 'n' more MBUFs into the software ring */
> + if (unlikely(rte_mempool_get_bulk(rxq->mb_pool,
> +   (void *)rxep,
> +   RTE_IXGBE_RXQ_REARM_THRESH) < 0)) {
> + if (rxq->rxrearm_nb + RTE_IXGBE_RXQ_REARM_THRESH >=
> + rxq->nb_rx_desc) {
> + for (i = 0; i < RTE_IXGBE_DESCS_PER_LOOP; i++) {
> + rxep[i].mbuf = >fake_mbuf;
> + vst1q_u64((uint64_t *)[i].read,
> +   zero);
> + }
> + }
> + rte_eth_devices[rxq->port_id].data->rx_mbuf_alloc_failed +=
> + RTE_IXGBE_RXQ_REARM_THRESH;
> + return;
> + }
> +
> + p = vld1_u8((uint8_t *)>mbuf_initializer);
> +
> + /* Initialize the mbufs in vector, process 2 mbufs in one loop */
> + for (i = 0; i < RTE_IXGBE_RXQ_REARM_THRESH; i += 2, rxep += 2) {
> + mb0 = rxep[0].mbuf;
> + mb1 = rxep[1].mbuf;
> +
> + /*
> +  * Flush mbuf with pkt template.
> +  * Data to be rearmed is 6 bytes long.
> +  * Though, 

[dpdk-dev] [PATCH v3 13/35] mempool: store physical address in objects

[Jain, Deepak K]

Hi,

While running the QAT PMD tests, a system hang is observed when this commit is 
used.

rte_mempool_virt2phy is used in qat_crypto.c.

regards,
Deepak


-Original Message-
From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Olivier Matz
Sent: Wednesday, May 18, 2016 12:05 PM
To: dev at dpdk.org
Cc: Richardson, Bruce ; stephen at 
networkplumber.org; Wiles, Keith 
Subject: [dpdk-dev] [PATCH v3 13/35] mempool: store physical address in objects

Store the physical address of the object in its header. It simplifies
rte_mempool_virt2phy() and prepares the removing of the paddr[] table in the 
mempool header.

Signed-off-by: Olivier Matz 
---
 lib/librte_mempool/rte_mempool.c | 17 +++--  
lib/librte_mempool/rte_mempool.h | 11 ++-
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 61e191e..ce12db5 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -133,19 +133,22 @@ static unsigned optimize_object_size(unsigned obj_size)  
typedef void (*rte_mempool_obj_iter_t)(void * /*obj_iter_arg*/,
void * /*obj_start*/,
void * /*obj_end*/,
-   uint32_t /*obj_index */);
+   uint32_t /*obj_index */,
+   phys_addr_t /*physaddr*/);

 static void
-mempool_add_elem(struct rte_mempool *mp, void *obj)
+mempool_add_elem(struct rte_mempool *mp, void *obj, phys_addr_t 
+physaddr)
 {
struct rte_mempool_objhdr *hdr;
struct rte_mempool_objtlr *tlr __rte_unused;

obj = (char *)obj + mp->header_size;
+   physaddr += mp->header_size;

/* set mempool ptr in header */
hdr = RTE_PTR_SUB(obj, sizeof(*hdr));
hdr->mp = mp;
+   hdr->physaddr = physaddr;
STAILQ_INSERT_TAIL(>elt_list, hdr, next);

 #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
@@ -175,6 +178,7 @@ rte_mempool_obj_mem_iter(void *vaddr, uint32_t elt_num, 
size_t total_elt_sz,
uint32_t pgn, pgf;
uintptr_t end, start, va;
uintptr_t pg_sz;
+   phys_addr_t physaddr;

pg_sz = (uintptr_t)1 << pg_shift;
va = (uintptr_t)vaddr;
@@ -210,9 +214,10 @@ rte_mempool_obj_mem_iter(void *vaddr, uint32_t elt_num, 
size_t total_elt_sz,
 * otherwise, just skip that chunk unused.
 */
if (k == pgn) {
+   physaddr = paddr[k] + (start & (pg_sz - 1));
if (obj_iter != NULL)
obj_iter(obj_iter_arg, (void *)start,
-   (void *)end, i);
+   (void *)end, i, physaddr);
va = end;
j += pgf;
i++;
@@ -249,11 +254,11 @@ rte_mempool_obj_iter(struct rte_mempool *mp,

 static void
 mempool_obj_populate(void *arg, void *start, void *end,
-   __rte_unused uint32_t idx)
+   __rte_unused uint32_t idx, phys_addr_t physaddr)
 {
struct rte_mempool *mp = arg;

-   mempool_add_elem(mp, start);
+   mempool_add_elem(mp, start, physaddr);
mp->elt_va_end = (uintptr_t)end;
 }

@@ -358,7 +363,7 @@ rte_mempool_xmem_size(uint32_t elt_num, size_t 
total_elt_sz, uint32_t pg_shift)
  */
 static void
 mempool_lelem_iter(void *arg, __rte_unused void *start, void *end,
-   __rte_unused uint32_t idx)
+   __rte_unused uint32_t idx, __rte_unused phys_addr_t physaddr)
 {
*(uintptr_t *)arg = (uintptr_t)end;
 }
diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 12215f6..4f95bdf 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -159,6 +159,7 @@ struct rte_mempool_objsz {  struct rte_mempool_objhdr {
STAILQ_ENTRY(rte_mempool_objhdr) next; /**< Next in list. */
struct rte_mempool *mp;  /**< The mempool owning the object. */
+   phys_addr_t physaddr;/**< Physical address of the object. */
 #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
uint64_t cookie; /**< Debug cookie. */
 #endif
@@ -1131,13 +1132,13 @@ rte_mempool_empty(const struct rte_mempool *mp)
  *   The physical address of the elt element.
  */
 static inline phys_addr_t
-rte_mempool_virt2phy(const struct rte_mempool *mp, const void *elt)
+rte_mempool_virt2phy(__rte_unused const struct rte_mempool *mp, const 
+void *elt)
 {
if (rte_eal_has_hugepages()) {
-   uintptr_t off;
-
-   off = (const char *)elt - (const char *)mp->elt_va_start;
-   return mp->elt_pa[off >> mp->pg_shift] + (off & mp->pg_mask);
+   const struct rte_mempool_objhdr *hdr;
+   hdr = (const struct rte_mempool_objhdr *)RTE_PTR_SUB(elt,
+   sizeof(*hdr));
+   return hdr->physaddr;
} else {
/*
 * If huge pages are disabled, we cannot assume the
--
2.8.0.rc3

[dpdk-dev] [PATCH v2 07/40] bnxt: declare ring structs and free() func

[Bruce Richardson]

On Fri, May 13, 2016 at 03:45:56PM -0700, Stephen Hurd wrote:
> Declare ring structures and a ring free() function.
> 

Are these rings used for packet RX and TX or something else?

> Signed-off-by: Stephen Hurd 
> Reviewed-by: Ajit Kumar Khaparde 
> ---
>  drivers/net/bnxt/Makefile|  1 +
>  drivers/net/bnxt/bnxt_ring.c | 51 
>  drivers/net/bnxt/bnxt_ring.h | 92 
> 
>  3 files changed, 144 insertions(+)
>  create mode 100644 drivers/net/bnxt/bnxt_ring.c
>  create mode 100644 drivers/net/bnxt/bnxt_ring.h
> 
> diff --git a/drivers/net/bnxt/Makefile b/drivers/net/bnxt/Makefile
> index c57afaa..757ea62 100644
> --- a/drivers/net/bnxt/Makefile
> +++ b/drivers/net/bnxt/Makefile
> @@ -50,6 +50,7 @@ EXPORT_MAP := rte_pmd_bnxt_version.map
>  #
>  SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_ethdev.c
>  SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_hwrm.c
> +SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_ring.c
>  SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_vnic.c
>  
>  #
> diff --git a/drivers/net/bnxt/bnxt_ring.c b/drivers/net/bnxt/bnxt_ring.c
> new file mode 100644
> index 000..0434b07
> --- /dev/null
> +++ b/drivers/net/bnxt/bnxt_ring.c
> @@ -0,0 +1,51 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) Broadcom Limited.
> + *   All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + *   notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + *   notice, this list of conditions and the following disclaimer in
> + *   the documentation and/or other materials provided with the
> + *   distribution.
> + * * Neither the name of Broadcom Corporation nor the names of its
> + *   contributors may be used to endorse or promote products derived
> + *   from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "bnxt.h"
> +#include "bnxt_ring.h"
> +
> +/*
> + * Generic ring handling
> + */
> +
> +void bnxt_free_ring(struct bnxt_ring_struct *ring)
> +{
> + /* The actual ring is reserved via rte_memzone_reserve API.
> +The current document/code indicates that:
> +"Note: A reserved zone cannot be freed."

I don't believe this is true any more. With recent changes to base memzones on
malloc library, the memzones should be free-able.

/Bruce

[dpdk-dev] [PATCH v2 06/40] bnxt: add vnic functions and structs

[Bruce Richardson]

On Fri, May 13, 2016 at 03:45:55PM -0700, Stephen Hurd wrote:
> Add functions to allocate, initialize, and free vnics.
> 
> Signed-off-by: Stephen Hurd 
> Reviewed-by: Ajit Kumar Khaparde 
> ---

Can you perhaps explain what is meant by vnics in this context. Does this patch
need a doc update to describe this functionality?

/Bruce

[dpdk-dev] [PATCH v2 02/40] bnxt: add HWRM init code

[Stephen Hurd]

On Wed, May 25, 2016 at 8:05 AM, Bruce Richardson <
bruce.richardson at intel.com> wrote:

>
> Checkpatch also highlights a few minor issues that might be worth
> addressing.
>
>   CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
>   #137: FILE: drivers/net/bnxt/bnxt.h:94:
>   +#define BNXT_FLAG_VF   (1<<1)
>   ^
>
>   WARNING:BLOCK_COMMENT_STYLE: Block comments use * on subsequent lines
>   #259: FILE: drivers/net/bnxt/bnxt_ethdev.c:143:
>   +   /*
>   +   eth_dev->rx_pkt_burst = _recv_pkts;
>
>   WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
>   #366: FILE: drivers/net/bnxt/bnxt_hwrm.c:56:
>   +   unsigned i;
>
>
What version of checkpatch.pl should we be using?  These were not found by
the one I had.

-- 
Stephen Hurd

[dpdk-dev] [PATCHv4 4/5] Makefile: Do post processing on objects that register a driver

[Neil Horman]

On Wed, May 25, 2016 at 10:04:11PM +0200, Thomas Monjalon wrote:
> 2016-05-25 15:43, Neil Horman:
> > On Wed, May 25, 2016 at 08:56:25PM +0200, Thomas Monjalon wrote:
> > > 2016-05-25 13:40, Neil Horman:
> > > > On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote:
> > > > > 2016-05-24 15:41, Neil Horman:
> > > > > > +   echo MODGEN $@; \
> > > > > > +   OBJF=`readlink -f $@`; \
> > > > > > +   ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF 
> > > > > > \$$OBJF.mod.c; \
> > > > > 
> > > > > Maybe .pmd.c would be more appropriate than .mod.c?
> > > > fine
> > > > > What means mod/MODGEN/MODBUILD?
> > > > GENerate Module information & BUILD module information.
> > > 
> > > I think "module" is not appropriate here.
> > > 
> > This is starting to feel very much like bikeshedding.  What do you think 
> > would
> > be more appropriate here?
> 
> pmd/PMDINFO//
> 
> > > > > It deserves to be in a shell script, at least to ease testing.
> > > > What do you mean by "it" and why would it be easier to test in a shell 
> > > > script?
> > > 
> > > "it" is mostly this whole patch.
> > > With a shell script, we can test the behaviour on one file easily.
> > > Maybe I'm wrong, but I don't like having too much lines in a Makefile 
> > > rule.
> > > We probably need more opinions.
> > > 
> > That makes no sense to me. Any such script would need to receive two 
> > arguments:
> > 1) The path to a C file for a pmd
> > 2) The path to the corresponding object file for that pmd
> > 
> > Running any such script is then usless unles its predecated on first 
> > building
> > all the object files in the pmd.  And if you want to run something by hand 
> > on
> > the object files, it seems pretty straightforward to do so, just run:
> > build/builttools/pmdinfogen /path/to/pmd/object/file
> > 
> > The rest of that code is really just a test to avoid having to run pmdinfo 
> > gen
> > on any files other than the ones that contain the PMD_REGISTER_DRIVER macro
> 
> OK, no strong opinion here.
> If you feel comfortable with multi-lines "sh -c" and escaping, up to you.
> If I discover something wrong in this part and needs to do some maintenance
> work, I'll probably think differently.
> 
You're welcome to assign the bug to me :)
Neil

[dpdk-dev] [PATCH v2 03/40] bnxt: add driver register/unregister support

[Bruce Richardson]

On Fri, May 13, 2016 at 03:45:52PM -0700, Stephen Hurd wrote:
> Move init() cleanup into uninit() function
> Fix .dev_private_size
> Add require hwrm calls:
>   bnxt_hwrm_func_driver_register()
>   bnxt_hwrm_func_driver_unregister()
> 
> Signed-off-by: Stephen Hurd 
> Reviewed-by: Ajit Kumar Khaparde 
> ---
>  drivers/net/bnxt/bnxt.h|   1 +
>  drivers/net/bnxt/bnxt_ethdev.c |  48 --
>  drivers/net/bnxt/bnxt_hwrm.c   |  50 ++
>  drivers/net/bnxt/bnxt_hwrm.h   |   3 +
>  drivers/net/bnxt/hsi_struct_def_dpdk.h | 277 
> -
>  5 files changed, 359 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt.h b/drivers/net/bnxt/bnxt.h
> index 0f816ed..ebddeab 100644
> --- a/drivers/net/bnxt/bnxt.h
> +++ b/drivers/net/bnxt/bnxt.h
> @@ -91,6 +91,7 @@ struct bnxt {
>   struct rte_pci_device   *pdev;
>  
>   uint32_tflags;
> +#define BNXT_FLAG_REGISTERED (1<<0)
>  #define BNXT_FLAG_VF (1<<1)
>  #define BNXT_PF(bp)  (!((bp)->flags & BNXT_FLAG_VF))
>  #define BNXT_VF(bp)  ((bp)->flags & BNXT_FLAG_VF)
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index a74cc6c..07519df 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -52,20 +52,12 @@ static struct rte_pci_id bnxt_pci_id_map[] = {
>   {.device_id = 0},
>  };
>  
> -static void bnxt_dev_close_op(struct rte_eth_dev *eth_dev)
> -{
> - struct bnxt *bp = (struct bnxt *)eth_dev->data->dev_private;
> -
> - rte_free(eth_dev->data->mac_addrs);
> - bnxt_free_hwrm_resources(bp);
> -}
> -

It seems strange to remove this code given that it was just added in the 
previous
commit. Does it need to be added in the first place?

Regards,
/Bruce

[dpdk-dev] flow director on X550

[Nishant Verma]

Hi All,

My system configuration is
==>#. SuperMicro 1U
   - BIOS: 1.0a
   - Processor: Intel(R) Xeon(R) CPU D-1540 @ 2.00GHz
   - Onboard NIC: Intel(R) X552/X557-AT (2x10G)
 - Firmware-version: 0x81cf
 - Device ID (PF/VF): 8086:15ad /8086:15a8
   - kernel driver version: 4.2.5 (ixgbe)

I am working on DPDK 16.04 & pktgen 3.0.0 version.


My intention is to test flow director based on just destination IP. It
means, i will use test-pmd and configure flow director and fdir mask, from
pktgen i will send packet and check if packets are going to right queue or
not.

Here is the procedure, that i follow.

1. I run testpmd
* ./testpmd -c 0x -n 4 -- -i  --portmask=0x3 --nb-cores=5
--disable-link-check  --rxq=5 --txq=5  --pkt-filter-mode=perfect*

2. executed command "*port stop all*"

3. After that added fdir mask
*flow_director_mask 0 mode IP vlan 0 src_mask 0.0.0.0 0:0:0:0:0:0:0:0 0x00
dst_mask 255.255.255.255 0:0:0:0:0:0:0:0 0x00*

4. Again executed command "*port start all*"

5. Now i added flow director filter.
*flow_director_filter 0 mode IP add flow ipv4-udp  src 0.0.0.0  dst
66.66.66.66  tos 0x00 ttl 0x00 vlan 0 flexbytes (0x00,0x00) fwd pf
queue 4 fd_id 4*

6. after that "*start*"

---
7. On Pktgen side i just change destination Address  to *66.66.66.66*,
protocol to *UDP* and issue command start 0.
---
Result
8.  But instead of packet to queue 4, packets are going to queue 0.
"
testpmd> *start*
  io packet forwarding - CRC stripping disabled - packets/burst=32
  nb forwarding cores=5 - nb forwarding ports=2
  RX queues=5 - RX desc=128 - RX free threshold=32
  RX threshold registers: pthresh=8 hthresh=8 wthresh=0
  TX queues=5 - TX desc=512 - TX free threshold=32
  TX threshold registers: pthresh=32 hthresh=0 wthresh=0
  TX RS bit threshold=32 - TXQ flags=0xf01
testpmd> stop
Telling cores to stop...
Waiting for lcores to finish...

  --- Forward Stats for *RX Port= 0/Queue= 0 *-> TX Port= 1/Queue= 0
---
  RX-packets: 14094137   TX-packets: 14094137   TX-dropped: 0
  -- Forward statistics for port 0
--
  RX-packets: 14094137   RX-dropped: 258361RX-total: 14352498
  TX-packets: 0  TX-dropped: 0 TX-total: 0



  -- Forward statistics for port 1
--
  RX-packets: 127RX-dropped: 6162236   RX-total: 6162363
  TX-packets: 14094137   TX-dropped: 0 TX-total: 14094137



  +++ Accumulated forward statistics for all
ports+++
  RX-packets: 14094264   RX-dropped: 6420597   RX-total: 20514861
  TX-packets: 14094137   TX-dropped: 0 TX-total: 14094137



Done.
testpmd>
"

I am sharing my port and flow dir info also

"
testpmd> *show port info 0*

* Infos for port 0  *
MAC address: 0C:C4:7A:73:EF:14
Connect to socket: 0
memory allocation on the socket: 0
Link status: up
Link speed: 1 Mbps
Link duplex: full-duplex
Promiscuous mode: enabled
Allmulticast mode: disabled
Maximum number of MAC addresses: 128
Maximum number of MAC addresses of hash filtering: 4096
VLAN offload:
  strip on
  filter on
  qinq(extend) off
Hash key size in bytes: 40
Redirection table size: 512
Supported flow types:
  ipv4
  ipv4-tcp
  ipv4-udp
  ipv6
  ipv6-tcp
  ipv6-udp
  unknown
  unknown
  unknown
Max possible RX queues: 128
Max possible number of RXDs per queue: 4096
Min possible number of RXDs per queue: 32
RXDs number alignment: 8
Max possible TX queues: 64
Max possible number of TXDs per queue: 4096
Min possible number of TXDs per queue: 32
TXDs number alignment: 8
testpmd>
testpmd> *show port fdir 0*

   FDIR infos for port 0

  MODE:   PERFECT
  SUPPORTED FLOW TYPE:  ipv4-tcp ipv4-udp ipv4-sctp ipv4-other ipv6-tcp
ipv6-udp ipv6-sctp ipv6-other
  FLEX PAYLOAD INFO:
  max_len:   2   payload_limit: 62
  payload_unit:  2   payload_seg:   1
  bitmask_unit:  0   bitmask_num:   0
  MASK:
vlan_tci: 0x, src_ipv4: 0x, dst_ipv4: 0x, src_port:
0x, dst_port: 0x
src_ipv6: 0x,0x,0x,0x, dst_ipv6:
0x,0x,0x,0x
  FLEX PAYLOAD SRC OFFSET:
RAW:12 13
  FLEX MASK CFG:
unknown: 00 00
  guarant_count: 1   best_count:0
  guarant_space: 2048best_space:0
  collision: 0   free:  2047
  maxhash:   0   maxlen:0
  add:   1   remove:0
  f_add: 0   f_remove:  0


testpmd>
"

Please suggest. Do 

[dpdk-dev] [PATCH v2 01/40] bnxt: new driver for Broadcom NetXtreme-C devices

[Bruce Richardson]

On Fri, May 13, 2016 at 03:45:50PM -0700, Stephen Hurd wrote:
> Initial skeleton simply fails init.
> Add nic guide and tie into build system.
> 
> Signed-off-by: Stephen Hurd 
> ---
>  MAINTAINERS |   5 ++
>  config/common_base  |   5 ++
>  doc/guides/nics/bnxt.rst|  49 +++
>  drivers/net/Makefile|   1 +
>  drivers/net/bnxt/Makefile   |  63 ++
>  drivers/net/bnxt/bnxt_ethdev.c  | 104 
> 
>  drivers/net/bnxt/rte_pmd_bnxt_version.map   |   4 +
>  lib/librte_eal/common/include/rte_pci_dev_ids.h |  40 +++--
>  mk/rte.app.mk   |   1 +
>  9 files changed, 267 insertions(+), 5 deletions(-)
>  create mode 100644 doc/guides/nics/bnxt.rst
>  create mode 100644 drivers/net/bnxt/Makefile
>  create mode 100644 drivers/net/bnxt/bnxt_ethdev.c
>  create mode 100644 drivers/net/bnxt/rte_pmd_bnxt_version.map
> 
Great to see this patchset split up finer grained, and it getting compiled from
the start. Thanks for the work.

One error that gets flagged by the automated patch checks here is that, although
you add in a new doc for the new driver in this patch, that document is not
included in the overall NIC guides document.

/home/bruce/next-net/dpdk-next-net/doc/guides/nics/bnxt.rst:: WARNING: 
document isn't included in any toctree

Regards,
/Bruce

[dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility

[Neil Horman]

On Wed, May 25, 2016 at 09:39:43PM +0200, Thomas Monjalon wrote:
> 2016-05-25 15:13, Neil Horman:
> > On Wed, May 25, 2016 at 07:39:30PM +0200, Thomas Monjalon wrote:
> > > 2016-05-25 13:22, Neil Horman:
> > > > On Wed, May 25, 2016 at 03:21:19PM +0200, Thomas Monjalon wrote:
> > > > > 2016-05-24 15:41, Neil Horman:
> > > > > > +include $(RTE_SDK)/mk/rte.buildtools.mk
> > > > > 
> > > > > Why a new Makefile? Can you use rte.hostapp.mk?
> > > > > 
> > > > I don't know, maybe.  Nothing else currently uses rte.hostapp.mk, so I 
> > > > missed
> > > > its existance.  I make the argument that, that being the case, we 
> > > > should stick
> > > > with the Makefile I just tested with, and remove the rte.hostapp.mk file
> > > 
> > > No, rte.hostapp.mk has been used and tested in the history of the project.
> > > Please try it.
> > > 
> > It works, but its really ugly (as it means that the buildtools directory 
> > gets
> > install to the hostapp directory under the build).  I could move that of 
> > course,
> > but at this point, you are asking me to remove a working makefile to 
> > replace it
> > with another makefile that, by all rights should have been removed as part 
> > of
> > commit efa2084a840fb83fd9be83adca57e5f23d3fa9fe:
> > Author: Thomas Monjalon 
> > Date:   Tue Mar 10 17:55:25 2015 +0100
> > 
> > scripts: remove useless build tools
> > 
> > test-framework.sh is an old script to check building of some 
> > dependencies.
> > testhost is an old app used to check HOSTCC.
> > 
> > Let's clean the scripts directory.
> > 
> > Here you removed the only user of rte.hostapp.mk, but neglected to remove
> > hostapp.mk itself.
> 
> Yes. I didn't really neglect to remove it. I thought it would be used later.
> 
Ok, thats fair.

> > I really fail to see why making me rework my current
> > makefile setup, that matches the purpose of the tool is a superior solution 
> > to
> > just getting rid of the unused makefile thats there right now.
> 
> I'm just trying to avoid creating a new makefile for each tool.
> Is it possible to fix the directory in rte.hostapp.mk?
> Every apps use the same makefile rte.app.mk. I think it should be the same
> for host apps.
> 
Yes, I could do that, I could fix up the directory path in rte.hostapp.mk so
that it installs to buildtools rather than hostapp, and that would be fine.  But
then if I were to additionally issue this command:
git mv mk/rte.hostapp.mk mk/rte.buildtools.mk

We would have exactly what I'm proposing anyway.  

I don't disagree that rte.buildtools.mk and rte.hostapp.mk are simmilar, they
are in fact almost identical, and I simply missed the latter because I didn't
see any uses of it.  What I am saying is that, due to their simmilarity, Its
pretty much an equivalent situation to use either makefile, and its less work
for me to remove hostapp.mk and just use what I have.

> > > > > > +++ b/buildtools/pmdinfogen/pmdinfogen.c
> > > > > [...]
> > > > > > +   /*
> > > > > > +* If this returns NULL, then this is a PMD_VDEV, because
> > > > > > +* it has no pci table reference
> > > > > > +*/
> > > > > 
> > > > > We can imagine physical PMD not using PCI.
> > > > > I think this comment should be removed.
> > > > We can, but currently its a true statement.  we have two types of PMDs, 
> > > > a PDEV
> > > > and a VDEV, the former is a pci device, and the latter is a virtual 
> > > > device, so
> > > > you can imply the PDEV type from the presence of pci entries, and VDEV 
> > > > from the
> > > > alternative.  If we were to do something, I would recommend adding a 
> > > > macro to
> > > > explicitly ennumerate each pmds type.  I would prefer to wait until 
> > > > that was a
> > > > need however, as it can be done invisibly to the user.
> > > 
> > > We are removing the PMD types in the EAL rework.
> > > So this comment will be outdated. Better to remove now.
> > > 
> > Then, I'm just not going to enumerate the type of driver at all, I'll remove
> > that attribute entirely.
> 
> OK
> 
> > But I really don't like to write code for things that are 'predictive'.
> 
> Not really predictive as it is an older patch.
And how many older patches never get integrated?  Or languish for long periods
of time?  We've debated this before.

Its really not reasonable to expect developers (myself or others) to
go through the mailing list and create an ordinal list of patches to apply
before doing our development work.  If that were the case, then they should just
be applied immediately so the HEAD of the git tree is an accurate representation
of the development state of the tree.  But thats not the case, and patches don't
always get applied in the order that they are posted.  So, if Davids Patch
series goes in ahead of mine, I'll gladly rebase, but I don't want to create
some artificial ordinality just because we touch the same code, especially if
his patch series has to go back for further revision.

> 
> > > > > [...]
> > > > > > +   

[dpdk-dev] [PATCHv4 4/5] Makefile: Do post processing on objects that register a driver

[Neil Horman]

On Wed, May 25, 2016 at 08:56:25PM +0200, Thomas Monjalon wrote:
> 2016-05-25 13:40, Neil Horman:
> > On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote:
> > > 2016-05-24 15:41, Neil Horman:
> > > > +   echo MODGEN $@; \
> > > > +   OBJF=`readlink -f $@`; \
> > > > +   ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF 
> > > > \$$OBJF.mod.c; \
> > > 
> > > Maybe .pmd.c would be more appropriate than .mod.c?
> > fine
> > > What means mod/MODGEN/MODBUILD?
> > GENerate Module information & BUILD module information.
> 
> I think "module" is not appropriate here.
> 
This is starting to feel very much like bikeshedding.  What do you think would
be more appropriate here?

> > > It deserves to be in a shell script, at least to ease testing.
> > What do you mean by "it" and why would it be easier to test in a shell 
> > script?
> 
> "it" is mostly this whole patch.
> With a shell script, we can test the behaviour on one file easily.
> Maybe I'm wrong, but I don't like having too much lines in a Makefile rule.
> We probably need more opinions.
> 
That makes no sense to me. Any such script would need to receive two arguments:
1) The path to a C file for a pmd
2) The path to the corresponding object file for that pmd

Running any such script is then usless unles its predecated on first building
all the object files in the pmd.  And if you want to run something by hand on
the object files, it seems pretty straightforward to do so, just run:
build/builttools/pmdinfogen /path/to/pmd/object/file

The rest of that code is really just a test to avoid having to run pmdinfo gen
on any files other than the ones that contain the PMD_REGISTER_DRIVER macro

Neil

[dpdk-dev] [PATCH v1 1/1] examples/l2fwd-crypto: improve random key generator

[Piotr Azarewicz]

This patch improve generate_random_key() function by replacing rand()
function with reading from /dev/urandom.

CID 120136 : Calling risky function (DC.WEAK_CRYPTO)
dont_call: rand should not be used for security related applications, as
linear congruential algorithms are too easy to break

Coverity issue: 120136

Signed-off-by: Piotr Azarewicz 
---
 examples/l2fwd-crypto/main.c |   18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/examples/l2fwd-crypto/main.c b/examples/l2fwd-crypto/main.c
index d18c813..e1f0a1e 100644
--- a/examples/l2fwd-crypto/main.c
+++ b/examples/l2fwd-crypto/main.c
@@ -45,6 +45,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 

 #include 
 #include 
@@ -581,10 +583,18 @@ l2fwd_simple_forward(struct rte_mbuf *m, unsigned portid)
 static void
 generate_random_key(uint8_t *key, unsigned length)
 {
-   unsigned i;
+   int fd;
+   int ret;
+
+   fd = open("/dev/urandom", O_RDONLY);
+   if (fd < 0)
+   rte_exit(EXIT_FAILURE, "Failed to generate random key\n");

-   for (i = 0; i < length; i++)
-   key[i] = rand() % 0xff;
+   ret = read(fd, key, length);
+   close(fd);
+
+   if (ret != (signed)length)
+   rte_exit(EXIT_FAILURE, "Failed to generate random key\n");
 }

 static struct rte_cryptodev_sym_session *
@@ -1180,8 +1190,6 @@ l2fwd_crypto_parse_timer_period(struct 
l2fwd_crypto_options *options,
 static void
 l2fwd_crypto_default_options(struct l2fwd_crypto_options *options)
 {
-   srand(time(NULL));
-
options->portmask = 0x;
options->nb_ports_per_lcore = 1;
options->refresh_period = 1;
-- 
1.7.9.5

[dpdk-dev] Crashing OVS+DPDK at the host, from inside of a KVM Guest

[Xie, Huawei]

On 5/25/2016 2:06 PM, Christian Ehrhardt wrote:
> Hi,
> ping ...
>
> Later on I want to look at it again once we upgraded to more recent
> releases of the software components involved, but those have to be made
> ready to use first :-/
>
> But the description is good and I wonder if anybody else could reproduce
> this and/or would have a hint on where this might come from or already
> existing related fixes.
>
> I mean in general nothing should be able to crash the host right?

Yes, we are taking care of these issues to avoid malicious or buggy
guest driver to corrupt vhost.
We have fixed some issues, and would continue to check if there are
other potential issues.

>
>
> P.S. yeah two list cross posting, but it is yet unclear which it belongs to
> so I'll keep it
>
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>
> On Sun, May 15, 2016 at 7:08 AM, Martinx - ?  gmail.com>
> wrote:
>
>> Guys,
>>
>>  If using OVS 2.5 with DPDK 2.2, on Ubuntu Xenial, it is possible to crash
>> the OVS running at the host, from inside of a KVM Guest.
>>
>>  Basically, what I'm trying to do, is to run OVS+DPDK at the host, and
>> also, inside of a KVM Guest, with multi-queue, but it doesn't work and
>> crashes.
>>
>>  Soon as you enable multi-queue at the guest, it crashes the OVS of the
>> host!
>>
>> OVS+DPDK segfault at the host, after running "ovs-vsctl set Open_vSwitch .
>> other_config:n-dpdk-rxqs=4" within a KVM Guest:
>>
>> https://bugs.launchpad.net/ubuntu/+source/openvswitch/+bug/1577088
>>
>> Thanks!
>> Thiago
>>

[dpdk-dev] [PATCH] virtio: use volatile to get used->idx in the loop

[Xie, Huawei]

On 5/25/2016 6:01 PM, Richardson, Bruce wrote:
> On Wed, May 25, 2016 at 12:50:02PM +0300, Michael S. Tsirkin wrote:
>> On Wed, May 25, 2016 at 10:47:30AM +0100, Bruce Richardson wrote:
>>> On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote:
 On Wed, May 25, 2016 at 08:25:20AM +, Xie, Huawei wrote:
> On 5/25/2016 4:12 PM, Xie, Huawei wrote:
>> There is no external function call or any barrier in the loop,
>> the used->idx would only be retrieved once.
>>
>> Signed-off-by: Huawei Xie 
>> ---
>>  drivers/net/virtio/virtio_ethdev.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c 
>> b/drivers/net/virtio/virtio_ethdev.c
>> index c3fb628..f6d6305 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -204,7 +204,8 @@ virtio_send_command(struct virtqueue *vq, struct 
>> virtio_pmd_ctrl *ctrl,
>>  usleep(100);
>>  }
>>  
>> -while (vq->vq_used_cons_idx != vq->vq_ring.used->idx) {
>> +while (vq->vq_used_cons_idx !=
>> +   *((volatile uint16_t *)(>vq_ring.used->idx))) {
>>  uint32_t idx, desc_idx, used_idx;
>>  struct vring_used_elem *uep;
>>  
> Find this issue when do the code rework of RX/TX queue.
> As in other places, we also have loop retrieving the value of avial->idx
> or used->idx, i prefer to declare the index in vq structure as volatile
> to avoid potential issue.
>>> Is there a reason why the value is not always volatile? I would have thought
>>> it would be generally safer to mark the actual value as volatile inside the
>>> structure definition itself? In any cases where we do want to store the 
>>> value
>>> locally and not re-access the structure, a local variable can be used.
>>>
>>> Regards,
>>> /Bruce
>> Linux generally discourages volatile as a general style guidance:
>> https://www.kernel.org/doc/Documentation/volatile-considered-harmful.txt
>> it doesn't have to apply to dpdk which has a different coding style
>> but IIUC this structure is inherited from linux, deviating
>> will make keeping things up to date harder.
> The prohibition on volatile indeed doesn't apply to DPDK, due to the fact that
> we so seldom use locks, and do a lot of direct register accesses in out PMDs.
> [I also still have the scars from previous issues where we had nice subtle 
> bugs
> in our PMDs - which only occurred with specific subversions of gcc - all due
> to a missing "volatile" on one structure element.]
>
> However, in this case, I take your point about keeping things consistent with
> the kernel. :-)

At least for virtio PMD, we have to support both Linux and FreeBSD, so
DPDK defines its own vring structure instead of including linux header file.
Two solutions for this volatile issue, 1) declare  used->idx and
avail->idx as volatile 2) define similar
access_once/read_once/write_once macro.
Would take the first one. In future, we could consider define
access_once, and apply to all other data structures if we want to use
the kernel style.

One thing i am confusing is other DPDK components include Linux header
files, do they compile on FreeBSD?

>
> /Bruce
>
 It might be a good idea to wrap this in a macro
 similar to ACCESS_ONCE in Linux.

> Stephen:
> Another question is why we need a loop here?
>
> /huawei
 -- 
 MST

[dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility

[Thomas Monjalon]

2016-05-24 15:41, Neil Horman:
> pmdinfogen is a tool used to parse object files and build json strings for 
> use in
> later determining hardware support in a dso or application binary.  pmdinfo
> looks for the non-exported symbol names this_pmd_name and this_pmd_tbl
> (where n is a integer counter).  It records the name of each of these tuples,
> using the later to find the symbolic name of the pci_table for physical 
> devices
> that the object supports.  With this information, it outputs a C file with a
> single line of the form:
> 
> static char *_driver_info[] __attribute__((used)) = " \
>   PMD_DRIVER_INFO=";
> 
> Where  is the arbitrary name of the pmd, and  is the 
> json
> encoded string that hold relevant pmd information, including the pmd name, 
> type
> and optional array of pci device/vendor ids that the driver supports.
> 
> This c file is suitable for compiling to object code, then relocatably linking
> into the parent file from which the C was generated.  This creates an entry in
> the string table of the object that can inform a later tool about hardware
> support.

This description is helpful and should be in the doc:
doc/guides/prog_guide/dev_kit_build_system.rst

> --- a/GNUmakefile
> +++ b/GNUmakefile
> -ROOTDIRS-y := lib drivers app
> +ROOTDIRS-y := buildtools lib drivers app

Why a new directory?
It is not a script nor an end-user tool, I guess.

I feel strange to build an app for the build system.
For information, do you know some Python lib to do this kind of tool?

> +++ b/buildtools/pmdinfogen/Makefile
> +#CFLAGS += $(WERROR_FLAGS) -g
> +CFLAGS += -g

Please choose one line or the other or none of them.

> +include $(RTE_SDK)/mk/rte.buildtools.mk

Why a new Makefile? Can you use rte.hostapp.mk?

> +++ b/buildtools/pmdinfogen/pmdinfogen.c
[...]
> + /*
> +  * If this returns NULL, then this is a PMD_VDEV, because
> +  * it has no pci table reference
> +  */

We can imagine physical PMD not using PCI.
I think this comment should be removed.

> + if (!tmpsym) {
> + drv->pci_tbl = NULL;
> + return 0;
> + }
[...]
> +
> +
> + return 0;
> + 
> +}

That's a lot of blank lines ;)

[...]
> + fprintf(ofd,"\\\"type\\\" : \\\"%s\\\", ", drv->pci_tbl ? 
> "PMD_PDEV" : "PMD_VDEV");

Please forget the naming PDEV/VDEV.

[...]
> + if (info.drivers) {
> + output_pmd_info_string(, argv[2]);
> + rc = 0;
> + } else {
> + fprintf(stderr, "Hmm, Appears to be a driver but no drivers 
> registered\n");

Why it appears to be a driver?
What means "no drivers registered" exactly?

> +++ b/buildtools/pmdinfogen/pmdinfogen.h
[...]
> +#define Elf_EhdrElf64_Ehdr
> +#define Elf_ShdrElf64_Shdr
> +#define Elf_Sym Elf64_Sym
> +#define Elf_AddrElf64_Addr
> +#define Elf_Sword   Elf64_Sxword
> +#define Elf_Section Elf64_Half
> +#define ELF_ST_BIND ELF64_ST_BIND
> +#define ELF_ST_TYPE ELF64_ST_TYPE
> +
> +#define Elf_Rel Elf64_Rel
> +#define Elf_RelaElf64_Rela
> +#define ELF_R_SYM   ELF64_R_SYM
> +#define ELF_R_TYPE  ELF64_R_TYPE

Why these defines are needed?

> +#define TO_NATIVE(x) (x)

Nice :) Why?

> +struct rte_pci_id {
> + uint16_t vendor_id;   /**< Vendor ID or PCI_ANY_ID. */
> + uint16_t device_id;   /**< Device ID or PCI_ANY_ID. */
> + uint16_t subsystem_vendor_id; /**< Subsystem vendor ID or PCI_ANY_ID. */
> + uint16_t subsystem_device_id; /**< Subsystem device ID or PCI_ANY_ID. */
> +};
[...]
> +struct pmd_driver {
> + Elf_Sym *name_sym;
> + const char *name;
> + struct rte_pci_id *pci_tbl;
> + struct pmd_driver *next;
> +
> + const char* opt_vals[PMD_OPT_MAX];
> +};

Are you duplicating some structures from EAL?
It will be out of sync easily.

> +struct elf_info {
> + unsigned long size;
> + Elf_Ehdr *hdr;
> + Elf_Shdr *sechdrs;
> + Elf_Sym  *symtab_start;
> + Elf_Sym  *symtab_stop;
> + Elf_Section  export_sec;
> + Elf_Section  export_unused_sec;
> + Elf_Section  export_gpl_sec;
> + Elf_Section  export_unused_gpl_sec;
> + Elf_Section  export_gpl_future_sec;
> + char *strtab;
> + char *modinfo;
> + unsigned int modinfo_len;

Why these fields?

> +++ b/mk/rte.buildtools.mk

This file must be removed I think.
We are going to be sick after digesting so much makefiles ;)

Last comment,
The MAINTAINERS file must be updated for this tool.

Thanks for taking care of tooling.

[dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility

[Neil Horman]

On Wed, May 25, 2016 at 07:39:30PM +0200, Thomas Monjalon wrote:
> 2016-05-25 13:22, Neil Horman:
> > On Wed, May 25, 2016 at 03:21:19PM +0200, Thomas Monjalon wrote:
> > > 2016-05-24 15:41, Neil Horman:
> > > > --- a/GNUmakefile
> > > > +++ b/GNUmakefile
> > > > -ROOTDIRS-y := lib drivers app
> > > > +ROOTDIRS-y := buildtools lib drivers app
> > > 
> > > Why a new directory?
> > > It is not a script nor an end-user tool, I guess.
> > Dependencies.  This tool has to be built prior to the rest of the dpdk, but 
> > app
> > already relies on dpdk libraries to be built, so you get circular 
> > dependencies.
> > I could have put it in scripts I guess, but its not a script.  Its own 
> > directory
> > seemed to make the most sense, given those two points
> 
> OK
> 
> > > > +include $(RTE_SDK)/mk/rte.buildtools.mk
> > > 
> > > Why a new Makefile? Can you use rte.hostapp.mk?
> > > 
> > I don't know, maybe.  Nothing else currently uses rte.hostapp.mk, so I 
> > missed
> > its existance.  I make the argument that, that being the case, we should 
> > stick
> > with the Makefile I just tested with, and remove the rte.hostapp.mk file
> 
> No, rte.hostapp.mk has been used and tested in the history of the project.
> Please try it.
> 
It works, but its really ugly (as it means that the buildtools directory gets
install to the hostapp directory under the build).  I could move that of course,
but at this point, you are asking me to remove a working makefile to replace it
with another makefile that, by all rights should have been removed as part of
commit efa2084a840fb83fd9be83adca57e5f23d3fa9fe:
Author: Thomas Monjalon 
Date:   Tue Mar 10 17:55:25 2015 +0100

scripts: remove useless build tools

test-framework.sh is an old script to check building of some dependencies.
testhost is an old app used to check HOSTCC.

Let's clean the scripts directory.

Here you removed the only user of rte.hostapp.mk, but neglected to remove
hostapp.mk itself.  I really fail to see why making me rework my current
makefile setup, that matches the purpose of the tool is a superior solution to
just getting rid of the unused makefile thats there right now.

> > > > +++ b/buildtools/pmdinfogen/pmdinfogen.c
> > > [...]
> > > > +   /*
> > > > +* If this returns NULL, then this is a PMD_VDEV, because
> > > > +* it has no pci table reference
> > > > +*/
> > > 
> > > We can imagine physical PMD not using PCI.
> > > I think this comment should be removed.
> > We can, but currently its a true statement.  we have two types of PMDs, a 
> > PDEV
> > and a VDEV, the former is a pci device, and the latter is a virtual device, 
> > so
> > you can imply the PDEV type from the presence of pci entries, and VDEV from 
> > the
> > alternative.  If we were to do something, I would recommend adding a macro 
> > to
> > explicitly ennumerate each pmds type.  I would prefer to wait until that 
> > was a
> > need however, as it can be done invisibly to the user.
> 
> We are removing the PMD types in the EAL rework.
> So this comment will be outdated. Better to remove now.
> 
Then, I'm just not going to enumerate the type of driver at all, I'll remove
that attribute entirely.  But I really don't like to write code for things that
are 'predictive'.

> > > [...]
> > > > +   fprintf(ofd,"\\\"type\\\" : \\\"%s\\\", ", drv->pci_tbl 
> > > > ? "PMD_PDEV" : "PMD_VDEV");
> > > 
> > > Please forget the naming PDEV/VDEV.
> > > 
> > I don't know what you mean here, you would rather they be named PCI and 
> > Virtual,
> > or something else?
> 
> Yes please.
> 
No, If you're removing the types, and you're sure of that, I'm just going to
remove the description entirely.  If you're unsure about exactly whats going to
happen, we should reflect the state of the build now, and make the appropriate
change when it lands.


> > > [...]
> > > > +   if (info.drivers) {
> > > > +   output_pmd_info_string(, argv[2]);
> > > > +   rc = 0;
> > > > +   } else {
> > > > +   fprintf(stderr, "Hmm, Appears to be a driver but no 
> > > > drivers registered\n");
> > > 
> > > Why it appears to be a driver?
> > > What means "no drivers registered" exactly?
> > > 
> > It means that the tool has identified this file as a driver based on some
> > criteria (in this case the source code contained a use of the
> > PMD_REGISTER_DRIVER macro, but for whatever reason, when this tool scanned 
> > it,
> > it never located the pmd_driver_name symbol.  It should never happen, and
> > serves as a indicator to the developer that they need to investigate either 
> > the
> > construction of the driver or the use of this tool.
> 
> OK
> 
> > > > +++ b/buildtools/pmdinfogen/pmdinfogen.h
> > > [...]
> > > > +#define Elf_EhdrElf64_Ehdr
> > > > +#define Elf_ShdrElf64_Shdr
> > > > +#define Elf_Sym Elf64_Sym
> > > > +#define Elf_AddrElf64_Addr
> > > > +#define Elf_Sword   Elf64_Sxword
> > > > +#define Elf_Section 

[dpdk-dev] [PATCH] qede: fix build issue in the cross-compiling mode

[Jerin Jacob]

In cross-compiling mode CC can be aarch64-*-linux-gnu-gcc
instead of just gcc

Signed-off-by: Jerin Jacob 
---
 drivers/net/qede/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/qede/Makefile b/drivers/net/qede/Makefile
index c9b3b1c..10ced84 100644
--- a/drivers/net/qede/Makefile
+++ b/drivers/net/qede/Makefile
@@ -47,7 +47,7 @@ endif
 endif
 endif

-ifneq (,$(filter gcc gcc48,$(CC)))
+ifneq (,$(filter %gcc %gcc48,$(CC)))
 CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable
 CFLAGS_BASE_DRIVER += -Wno-missing-declarations
 CFLAGS_BASE_DRIVER += -Wno-maybe-uninitialized
-- 
2.5.5

[dpdk-dev] [ovs-dev] If 1 KVM Guest loads the virtio-pci, on top of dpdkvhostuser OVS socket interface, it slows down everything!

[Bodireddy, Bhanuprakash]

I could reproduce the issue and this can be fixed as below

Firstly, the throughput issues observed with other VMs when a new VM is started 
can be fixed using the patch in the thread 
http://openvswitch.org/pipermail/dev/2016-May/071615.html.  I have put up an 
explanation in this thread for the cause of issue especially with multi VM 
setup on OVS DPDK. 

On a Multi VM setup even with the above patch applied, one might see aggregate 
throughput difference when vNIC is bind to igb_uio vs virtio-pci, this is for 
the fact that the interrupt overhead is significantly higher when virtio-pci is 
in use. 

More importantly if you have setup explicit flows matching VM's MAC/IP, 
disabling the flows to the VM that are idle would improve the aggregate 
throughput and lessen the burden on the pmd thread.   'watch -d 
./utilities/ovs-appctl dpctl/show -s' will show no. of packet stats.

Regards,
Bhanu Prakash.


>-Original Message-
>From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Christian
>Ehrhardt
>Sent: Wednesday, May 25, 2016 7:08 AM
>To: Martinx - ? 
>Cc:  ; dev ;
>qemu-stable at nongnu.org
>Subject: Re: [ovs-dev] If 1 KVM Guest loads the virtio-pci, on top of
>dpdkvhostuser OVS socket interface, it slows down everything!
>
>Hi again,
>another forgotten case.
>
>I currently I lack the HW to fully reproduce this, but the video summary is
>pretty good and shows the issue in an impressive way.
>
>Also the description is good and here as well I wonder if anybody else could
>reproduce this.
>Any hints / insights are welcome.
>
>P.S. and also again - two list cross posting, but here as well it is yet 
>unclear
>which it belongs to so I'll keep it as well
>
>Christian Ehrhardt
>Software Engineer, Ubuntu Server
>Canonical Ltd
>
>On Sun, May 22, 2016 at 6:35 PM, Martinx - ?
>
>wrote:
>
>> Guys,
>>
>>  I'm seeing a strange problem here, in my OVS+DPDK deployment, on top
>> of Ubuntu 16.04 (DPDK 2.2 and OVS 2.5).
>>
>>  Here is what I'm trying to do: run OVS with DPDK at the host, for KVM
>> Guests that also, will be running more DPDK Apps.
>>
>>  The host have 2 x 10G NICs, for OVS+DPDK and each KVM Guest receives
>> its own VLAN tagged traffic (or all tags).
>>
>>  There is an IXIA Traffic Generator sending 10G of traffic on both
>> directions (20G total).
>>
>>  Exemplifying, the problem is, lets say that I already have 2 VMs (or
>> 10) running DPDK Apps (on top of dpdkvhostuser), everything is working
>> as expected, then, if I boot the 3rd (or 11) KVM Guest, the OVS+DPDK
>> bridge at the host, slows down, a lot! The 3rd (or 11) VM affects not
>> only the host, but also, all the other neighbors VMs!!!
>>
>>  NOTE: This problem appear since the boot of VM 1.
>>
>>  Soon as you, inside of the 3rd VM, bind the VirtIO NIC to the
>> DPDK-Compative Drivers, the speed comes back to normal. If you bind it
>> back to "virtio-pci", boom! The OVS+DPDK at the host and all VMs loses
>> too much speed.
>>
>>  This problem is detailed at the following bug report:
>>
>> --
>> The OVS+DPDK dpdkvhostuser socket bridge, only works as expected, if
>> the KVM Guest also have DPDK drivers loaded:
>>
>> https://bugs.launchpad.net/ubuntu/+source/openvswitch/+bug/1577256
>> --
>>
>>  Also, I've recorded a ~15 min screen cast video about this problem,
>> so, you guys can see exactly what is happening here.
>>
>>
>https://www.youtube.com/v/yHnaSikd9XY?version=3=hd720=
>1
>>
>>  * At 5:25, I'm starting a VM that will boot up and load a DPDK App;
>>
>>  * At 5:33, OVS+DPDK is messed up, it loses speed;
>>The KVM running with virtio-pci drivers breaks OVS+DPDK at the
>> host;
>>
>>  * At 6:50, DPDK inside of the KVM guest loads up its drivers, kicking
>> "virtio-pci", speed back to normal at the host;
>>
>>  * At 7:43, started another KVM Guest, now, while virtio-pci driver is
>> running, the OVS+DPDK at the host and the other VM, are very, very
>> slow;
>>
>>  * At 8:52, the second VM loads up DPDK Drivers, kicking virtio-pci,
>> the speed is back to normal at the host, and on the other VM too;
>>
>>  * At 10:00, the Ubuntu VM loads up virtio-pci drivers on its boot,
>> the speed dropped at the hosts and on the other VMs;
>>
>>  * 11:57, I'm starting "service dpdk start" inside of the Ubuntu
>> guest, to kick up virtio-pci, and bang! Speed is back to normal
>> everywhere;
>>
>>  * 12:51, I'm trying to unbind the DPDK Drivers and return the
>> virtio-pci, I forgot the syntax while recording the video, which is:
>> "dpdk_nic_bind -b  virtio-pci", so, I just rebooted it. But both
>> "reboot" or "rebind to virtio-pci" triggers the bug.
>>
>>
>> NOTE: I tried to subscriber to qemu-devel but, it is not working, I'm
>> not receiving the confirmation e-mail, while qemu-stable worked. I
>> don't know if it worth sending it to Linux Kernel too...
>>
>>
>> Regards,
>> Thiago
>>
>___
>dev mailing list
>dev at openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev

[dpdk-dev] [PATCH 2/2] examples/ethtool: get reg width to allocate memory

[Remy Horton]

On 25/05/2016 07:36, zr at semihalf.com wrote:
> From: Zyta Szpak 
[..]
> Signed-off-by: Zyta Szpak 
> ---
>   examples/ethtool/lib/rte_ethtool.c | 6 +-
>   1 file changed, 5 insertions(+), 1 deletion(-)

Acked-by: Remy Horton 

[dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Remy Horton]

'noon,

Was expecting rte_eth_dev_get_reg_width() itself to default to 
sizeof(uint32_t) rather than -ENOTSUP, but that is purely personal taste 
which others might disagree with. You'll also need a documentation 
update & Fixes: line.


On 25/05/2016 07:36, zr at semihalf.com wrote:
> From: Zyta Szpak 
[..]
> Signed-off-by: Zyta Szpak 

Acked-by: Remy Horton 

[dpdk-dev] [PATCH] af_packet: add byte counters

[Rich Lane]

Signed-off-by: Rich Lane 
---
 drivers/net/af_packet/rte_eth_af_packet.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c 
b/drivers/net/af_packet/rte_eth_af_packet.c
index f17bd7e..2d7f344 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -78,6 +78,7 @@ struct pkt_rx_queue {

volatile unsigned long rx_pkts;
volatile unsigned long err_pkts;
+   volatile unsigned long rx_bytes;
 };

 struct pkt_tx_queue {
@@ -90,6 +91,7 @@ struct pkt_tx_queue {

volatile unsigned long tx_pkts;
volatile unsigned long err_pkts;
+   volatile unsigned long tx_bytes;
 };

 struct pmd_internals {
@@ -131,6 +133,7 @@ eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, 
uint16_t nb_pkts)
uint8_t *pbuf;
struct pkt_rx_queue *pkt_q = queue;
uint16_t num_rx = 0;
+   unsigned long num_rx_bytes = 0;
unsigned int framecount, framenum;

if (unlikely(nb_pkts == 0))
@@ -167,9 +170,11 @@ eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, 
uint16_t nb_pkts)
/* account for the receive frame */
bufs[i] = mbuf;
num_rx++;
+   num_rx_bytes += mbuf->pkt_len;
}
pkt_q->framenum = framenum;
pkt_q->rx_pkts += num_rx;
+   pkt_q->rx_bytes += num_rx_bytes;
return num_rx;
 }

@@ -186,6 +191,7 @@ eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, 
uint16_t nb_pkts)
struct pollfd pfd;
struct pkt_tx_queue *pkt_q = queue;
uint16_t num_tx = 0;
+   unsigned long num_tx_bytes = 0;
int i;

if (unlikely(nb_pkts == 0))
@@ -219,6 +225,7 @@ eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, 
uint16_t nb_pkts)
ppd = (struct tpacket2_hdr *) pkt_q->rd[framenum].iov_base;

num_tx++;
+   num_tx_bytes += mbuf->pkt_len;
rte_pktmbuf_free(mbuf);
}

@@ -229,6 +236,7 @@ eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, 
uint16_t nb_pkts)
pkt_q->framenum = framenum;
pkt_q->tx_pkts += num_tx;
pkt_q->err_pkts += nb_pkts - num_tx;
+   pkt_q->tx_bytes += num_tx_bytes;
return num_tx;
 }

@@ -287,13 +295,16 @@ eth_stats_get(struct rte_eth_dev *dev, struct 
rte_eth_stats *igb_stats)
 {
unsigned i, imax;
unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
+   unsigned long rx_bytes_total = 0, tx_bytes_total = 0;
const struct pmd_internals *internal = dev->data->dev_private;

imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
for (i = 0; i < imax; i++) {
igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
+   igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
rx_total += igb_stats->q_ipackets[i];
+   rx_bytes_total += igb_stats->q_ibytes[i];
}

imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
@@ -301,13 +312,17 @@ eth_stats_get(struct rte_eth_dev *dev, struct 
rte_eth_stats *igb_stats)
for (i = 0; i < imax; i++) {
igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
igb_stats->q_errors[i] = internal->tx_queue[i].err_pkts;
+   igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
tx_total += igb_stats->q_opackets[i];
tx_err_total += igb_stats->q_errors[i];
+   tx_bytes_total += igb_stats->q_obytes[i];
}

igb_stats->ipackets = rx_total;
+   igb_stats->ibytes = rx_bytes_total;
igb_stats->opackets = tx_total;
igb_stats->oerrors = tx_err_total;
+   igb_stats->obytes = tx_bytes_total;
 }

 static void
@@ -316,12 +331,15 @@ eth_stats_reset(struct rte_eth_dev *dev)
unsigned i;
struct pmd_internals *internal = dev->data->dev_private;

-   for (i = 0; i < internal->nb_queues; i++)
+   for (i = 0; i < internal->nb_queues; i++) {
internal->rx_queue[i].rx_pkts = 0;
+   internal->rx_queue[i].rx_bytes = 0;
+   }

for (i = 0; i < internal->nb_queues; i++) {
internal->tx_queue[i].tx_pkts = 0;
internal->tx_queue[i].err_pkts = 0;
+   internal->tx_queue[i].tx_bytes = 0;
}
 }

-- 
1.9.1

[dpdk-dev] [PATCH] e1000: fix build with clang

[Thomas Monjalon]

2016-05-24 23:48, Hiroyuki Mikita:
> GCC_VERSION is empty in case of clang:
>   /bin/sh: line 0: test: -ge: unary operator expected
> 
> It is the same issue as http://dpdk.org/dev/patchwork/patch/5994/

I did this patch but it looks broken for cross-compiler.
Please fix it. Thanks

[dpdk-dev] [PATCH v2 01/40] bnxt: new driver for Broadcom NetXtreme-C devices

[Stephen Hurd]

Bruce, is it better at this point to modify the existing patch that adds
bnxt.rst or to create a follow-on patch?

On Wed, May 25, 2016 at 8:02 AM, Bruce Richardson <
bruce.richardson at intel.com> wrote:

> On Fri, May 13, 2016 at 03:45:50PM -0700, Stephen Hurd wrote:
> > Initial skeleton simply fails init.
> > Add nic guide and tie into build system.
> >
> > Signed-off-by: Stephen Hurd 
> > ---
> >  MAINTAINERS |   5 ++
> >  config/common_base  |   5 ++
> >  doc/guides/nics/bnxt.rst|  49 +++
> >  drivers/net/Makefile|   1 +
> >  drivers/net/bnxt/Makefile   |  63 ++
> >  drivers/net/bnxt/bnxt_ethdev.c  | 104
> 
> >  drivers/net/bnxt/rte_pmd_bnxt_version.map   |   4 +
> >  lib/librte_eal/common/include/rte_pci_dev_ids.h |  40 +++--
> >  mk/rte.app.mk   |   1 +
> >  9 files changed, 267 insertions(+), 5 deletions(-)
> >  create mode 100644 doc/guides/nics/bnxt.rst
> >  create mode 100644 drivers/net/bnxt/Makefile
> >  create mode 100644 drivers/net/bnxt/bnxt_ethdev.c
> >  create mode 100644 drivers/net/bnxt/rte_pmd_bnxt_version.map
> >
> Great to see this patchset split up finer grained, and it getting compiled
> from
> the start. Thanks for the work.
>
> One error that gets flagged by the automated patch checks here is that,
> although
> you add in a new doc for the new driver in this patch, that document is not
> included in the overall NIC guides document.
>
> /home/bruce/next-net/dpdk-next-net/doc/guides/nics/bnxt.rst:: WARNING:
> document isn't included in any toctree
>
> Regards,
> /Bruce
>
>


-- 
Stephen Hurd
Principal Engineer - Software Development
Broadcom Corporation
949-926-8039
stephen.hurd at broadcom.com

[dpdk-dev] [PATCH v2] vhost: add support for dynamic vhost PMD creation

[Thomas Monjalon]

2016-05-25 12:41, Yuanhan Liu:
> On Tue, May 24, 2016 at 10:42:56AM +0100, Bruce Richardson wrote:
> > On Tue, May 24, 2016 at 01:11:26PM +0800, Yuanhan Liu wrote:
> > > On Mon, May 23, 2016 at 06:06:21PM +0100, Ferruh Yigit wrote:
> > > > On 5/23/2016 2:24 PM, Yuanhan Liu wrote:
> > > > > On Fri, May 20, 2016 at 11:37:47AM +0100, Bruce Richardson wrote:
> > > > >> On Thu, May 19, 2016 at 06:44:44PM +0200, Thomas Monjalon wrote:
> > > > >>> 2016-05-19 17:28, Ferruh Yigit:
> > > >  On 5/19/2016 9:33 AM, Thomas Monjalon wrote:
> > > > > 2016-05-18 18:10, Ferruh Yigit:
> > > > >> Add rte_eth_from_vhost() API to create vhost PMD dynamically from
> > > > >> applications.
> > > > >
> > > > > How is it different from rte_eth_dev_attach() calling 
> > > > > rte_eal_vdev_init()?
> > > > >
> > > > 
> > > >  When used rte_eth_dev_attach(), application also needs to do:
> > > >  rte_eth_dev_configure()
> > > >  rte_eth_rx_queue_setup()
> > > >  rte_eth_tx_queue_setup()
> > > >  rte_eth_dev_start()
> > > > 
> > > >  rte_eth_from_vhost() does these internally, easier to use for 
> > > >  applications.
> > > > >>>
> > > > >>> This argument is not sufficient.
> > > > >>> We are not going to add new APIs just for wrapping others.
> > > > >>
> > > > >> Why not - if there is a sufficient increase in developer usability 
> > > > >> by doing so?
> > > > >> Having one API that saves an app from having to call 5 other APIs 
> > > > >> looks like
> > > > >> something that should always be given fair consideration.
> > > > > 
> > > > > Good point. Judging that vhost is not the only virtual device we
> > > > > support, and it may also look reasonable to add something similar
> > > > > for others in future (say, IIRC, you proposed two more internally
> > > > > that also introduced similar APIs). So, instead of introducing a
> > > > > new API for each such vdev, may we introduce a common one? Say,
> > > > > a refined rte_eth_dev_attach(), including dev_configure(),
> > > > > queue_setup(), etc.
> > > > > 
> > > > 
> > > > This sounds good to me. If there is not objection, I will send a patch
> > > > and we can discuss based on patch.
> > > 
> > > Let's wait and gather some comments first?
> > > 
> > I'm not sure that such a general approach is likely to work,
> 
> Me, neither. Thus I threw it out for more discussion.
> 
> > as the parameters
> > needed for each individual driver are going to be different.
> 
> Well, if you plan to pass all necessary informations to the driver by
> parameters like this v1 does, then yes, that's true and a generic API
> is unlikely to work. But what I was thinking is that we feed it by
> strings, like the arguments for '--vdev' option. In such way, we could
> have an unified interface (if that works, which is something I'm not
> quite sure).

Yes, that is the plan with the EAL rework in progress.
Hotplugging is being redefined at EAL level and needs a configuration
API with devargs to be complete.

> OTOH, let's assume there is a switch that supports quite many such
> vdevs, as well as the ability to add a new device dynamically by
> corresponding API. And assume there is just one external interface
> from the switch to add a dynamical device (say, "ovs-vsctl add-port"),
> you then also need build some codes to invoke the right API, as well
> as constructing the right parameters, like what you said below.
> 
> This let me think of the vhost dequeue/enqueue API. Basically speaking,
> it has the same functionality the rte_eth_rx/tx_burst has, but just
> different API name and different parameters. This results to OVS has
> to write different netdev_class, one for NIC, another one for vhost-user.
> (actually, there is yet another one for vhost-cuse).
> 
> And now since we have vhost-pmd, we could just have one netdev_class
> at OVS, saving their (and other application's) effort to build/maintain
> similar codes.

Yes, it was a good improvement.

> Thus, I'm __just wondering__ could we add a generic interface to create
> vdev dynamically for all such vdevs? I was thinking something like:
> 
>   rte_create_vdev(type, char *options);

Actually, it has more sense to first create the device with an attach()
function and the configure it with devargs.
So neither attaching nor configuring are specific to vdev.
And devargs configuration can happen long after creating the device object.

I suggest to reject this patch and continue the EAL rework initiated
by David.

[dpdk-dev] [PATCH v3 2/4] ixgbe: implement vector PMD for arm architecture

[Bruce Richardson]

On Wed, May 25, 2016 at 05:59:38PM +0530, Jerin Jacob wrote:
> On Fri, May 06, 2016 at 11:55:46AM +0530, Jianbo Liu wrote:
> > use ARM NEON intrinsic to implement ixgbe vPMD
> > 
> > Signed-off-by: Jianbo Liu 
> > ---
> >  drivers/net/ixgbe/Makefile  |   4 +
> >  drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c | 561 
> > 
> >  2 files changed, 565 insertions(+)
> >  create mode 100644 drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c
> > 

> > +   for (pos = 0, nb_pkts_recd = 0; pos < nb_pkts;
> > +   pos += RTE_IXGBE_DESCS_PER_LOOP,
> > +   rxdp += RTE_IXGBE_DESCS_PER_LOOP) {
> > +   uint64x2_t descs[RTE_IXGBE_DESCS_PER_LOOP];
> > +   uint8x16_t pkt_mb1, pkt_mb2, pkt_mb3, pkt_mb4;
> > +   uint8x16x2_t sterr_tmp1, sterr_tmp2;
> > +   uint64x2_t mbp1, mbp2;
> > +   uint8x16_t staterr;
> > +   uint16x8_t tmp;
> > +   uint32_t stat;
> > +
> > +   /* B.1 load 1 mbuf point */
> > +   mbp1 = vld1q_u64((uint64_t *)_ring[pos]);
> > +
> > +   /* Read desc statuses backwards to avoid race condition */
> > +   /* A.1 load 4 pkts desc */
> > +   descs[3] =  vld1q_u64((uint64_t *)(rxdp + 3));
> > +   rte_rmb();
> 
> Any specific reason to add rte_rmb() here, If there is no performance
> drop then it makes sense to add before descs[3] uses it.i.e
> at rte_compiler_barrier() place in x86 code.
> 
> > +
> > +   /* B.2 copy 2 mbuf point into rx_pkts  */
> > +   vst1q_u64((uint64_t *)_pkts[pos], mbp1);
> > +
> > +   /* B.1 load 1 mbuf point */
> > +   mbp2 = vld1q_u64((uint64_t *)_ring[pos + 2]);
> > +
> > +   descs[2] =  vld1q_u64((uint64_t *)(rxdp + 2));
> > +   /* B.1 load 2 mbuf point */
> > +   descs[1] =  vld1q_u64((uint64_t *)(rxdp + 1));
> > +   descs[0] =  vld1q_u64((uint64_t *)(rxdp));
> > +
> > +   /* B.2 copy 2 mbuf point into rx_pkts  */
> > +   vst1q_u64((uint64_t *)_pkts[pos + 2], mbp2);
> > +
> > +   if (split_packet) {
> > +   rte_prefetch_non_temporal(_pkts[pos]->cacheline1);
> > +   rte_prefetch_non_temporal(_pkts[pos+1]->cacheline1);
> > +   rte_prefetch_non_temporal(_pkts[pos+2]->cacheline1);
> > +   rte_prefetch_non_temporal(_pkts[pos+3]->cacheline1);
> 
> replace with rte_mbuf_prefetch_part2 or equivalent
> 
Hi Jerin, Jianbo,

since this patch has already been applied and these are not critical issues with
it, can a new patch please be submitted to propose these additional changes on
top of what's on next-net now.

Thanks,
/Bruce

[dpdk-dev] Crashing OVS+DPDK at the host, from inside of a KVM Guest

[Traynor, Kevin]

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Christian
> Ehrhardt
> Sent: Wednesday, May 25, 2016 7:06 AM
> To: Martinx - ? 
> Cc:  ; dev 
> Subject: Re: [dpdk-dev] Crashing OVS+DPDK at the host, from inside of
> a KVM Guest
> 
> Hi,
> ping ...
> 
> Later on I want to look at it again once we upgraded to more recent
> releases of the software components involved, but those have to be
> made
> ready to use first :-/
> 
> But the description is good and I wonder if anybody else could
> reproduce
> this and/or would have a hint on where this might come from or already
> existing related fixes.
> 
> I mean in general nothing should be able to crash the host right?

Hi, I don't know if they are related to the issue that is being seen,
but Yuanhan made some fixes in DPDK 16.04 regarding a malicious guest
affecting the host. rte_vhost_dequeue_burst() is showing in the stack
trace so it might worth testing with the latest code to see if it's the
same issue and has been fixed.

Kevin.

> 
> 
> P.S. yeah two list cross posting, but it is yet unclear which it
> belongs to
> so I'll keep it
> 
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
> 
> On Sun, May 15, 2016 at 7:08 AM, Martinx - ?
> 
> wrote:
> 
> > Guys,
> >
> >  If using OVS 2.5 with DPDK 2.2, on Ubuntu Xenial, it is possible to
> crash
> > the OVS running at the host, from inside of a KVM Guest.
> >
> >  Basically, what I'm trying to do, is to run OVS+DPDK at the host,
> and
> > also, inside of a KVM Guest, with multi-queue, but it doesn't work
> and
> > crashes.
> >
> >  Soon as you enable multi-queue at the guest, it crashes the OVS of
> the
> > host!
> >
> > OVS+DPDK segfault at the host, after running "ovs-vsctl set
> Open_vSwitch .
> > other_config:n-dpdk-rxqs=4" within a KVM Guest:
> >
> > https://bugs.launchpad.net/ubuntu/+source/openvswitch/+bug/1577088
> >
> > Thanks!
> > Thiago
> >

[dpdk-dev] [PATCHv4 5/5] pmdinfo.py: Add tool to query binaries for hw and other support information

[Neil Horman]

On Wed, May 25, 2016 at 07:22:39PM +0200, Thomas Monjalon wrote:
> 2016-05-24 15:41, Neil Horman:
> > Note that, in the case of dynamically linked applications, pmdinfo.py will 
> > scan
> > for implicitly linked PMDs by searching the specified binaries .dynamic 
> > section
> > for DT_NEEDED entries that contain the substring librte_pmd.
> 
> I don't know any DPDK app dynamically linked with a PMD (with DT_NEEDED).
I know lots of them, they're all in the dpdk.  everything under app that uses a
virutal device links at link time to librte_pmd_bonding and librte_pmd_pipe (and
a few others), because they have additional apis that they need to resolve at
load time.

> However it is a good idea to handle this case.
> But relying on the name assumption "librte_pmd" is really weak.
> 
> > +   $(Q)$(call rte_symlink,$(DESTDIR)$(datadir)/tools/pmdinfo.py, \
> > +  $(DESTDIR)$(bindir)/pmdinfo)
> 
> I think we must prefix the tool name with dpdk.
> What about dpdk-objinfo or dpdk-pmdinfo?
> 
> > +from elftools.elf.elffile import ELFFile
> > +from elftools.elf.dynamic import DynamicSection, DynamicSegment
> > +from elftools.elf.enums import ENUM_D_TAG
> > +from elftools.elf.segments import InterpSegment
> > +from elftools.elf.sections import SymbolTableSection
> 
> Should it be possible to implement pmdinfogen with this
> Python library?
> 
Sure, but that really doesn't buy us anything, as its already implemented in C.
In fact, I would assert its harmful, because it implies that the build
environment needs to have python installed, as well as the pyelftools library,
which we don't need if we build from C.

> I'll probably comment on the pmdinfo script details later.
> Just knowing you did a tool is enough to assert that it is a good step :)
> Thanks
> 

[dpdk-dev] fast red autotest

[Thomas Monjalon]

2016-05-25 11:06, Kantecki, Tomasz:
> I had a quick look through these tests and it will need some effort to rework 
> them to run in shorter time.
> I agree to remove this suite from the fast_test as long as it gets exercised 
> in other test paths.

Is it possible to split them in
- short functional tests
- long run / perf tests
?
So we could keep some basic tests in fast_test.
I expect that tests which are not part of fast_test won't be run often
or in enough various environments.

[dpdk-dev] [PATCHv4 4/5] Makefile: Do post processing on objects that register a driver

[Neil Horman]

On Wed, May 25, 2016 at 07:08:19PM +0200, Thomas Monjalon wrote:
> 2016-05-24 15:41, Neil Horman:
> > --- a/mk/internal/rte.compile-pre.mk
> > +++ b/mk/internal/rte.compile-pre.mk
> > @@ -80,7 +80,8 @@ C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax 
> > highlight
> >  C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)","  HOSTCC $(@)")
> >  else
> >  C_TO_O = $(CC) -Wp,-MD,$(call obj2dep,$(@)).tmp $(CFLAGS) \
> > -   $(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $<
> > +$(CFLAGS_$(@)) $(EXTRA_CFLAGS) -o $@ -c $<
> > +
> 
> whitespace change?
> 

Looks like, I'll remove it

> >  C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight
> >  C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)","  CC $(@)")
> >  endif
> > @@ -88,10 +89,26 @@ C_TO_O_CMD = 'cmd_$@ = $(C_TO_O_STR)'
> >  C_TO_O_DO = @set -e; \
> > echo $(C_TO_O_DISP); \
> > $(C_TO_O) && \
> > +   sh -c "grep -q \"PMD_REGISTER_DRIVER(.*)\" $<; \
> > +   if [ \$$? -eq 0 ]; \
> > +   then \
> 
> It is preferred to keep "then" at the end of the previous line.
Very well.

> 
> > +   echo MODGEN $@; \
> > +   OBJF=`readlink -f $@`; \
> > +   ${RTE_OUTPUT}/buildtools/pmdinfogen \$$OBJF \$$OBJF.mod.c; \
> 
> Maybe .pmd.c would be more appropriate than .mod.c?
fine
> What means mod/MODGEN/MODBUILD?
GENerate Module information & BUILD module information.

> 
> > +   if [ \$$? -eq 0 ]; \
> > +   then \
> > +   echo MODBUILD $@; \
> > +   $(CC) -c -o \$$OBJF.mod.o \$$OBJF.mod.c; \
> > +   $(CROSS)ld -r -o \$$OBJF.o \$$OBJF.mod.o \$$OBJF; \
> > +   mv -f \$$OBJF.o \$$OBJF; \
> > +   fi; \
> > +   fi; \
> > +   true" && \
> 
> Why "true"?
Debugging statement, I'll remove it.

> 
> It deserves to be in a shell script, at least to ease testing.
What do you mean by "it" and why would it be easier to test in a shell script?

> 
> 

[dpdk-dev] [PATCHv4 2/5] drivers: Update driver registration macro usage

[Neil Horman]

On Wed, May 25, 2016 at 06:20:06PM +0200, Thomas Monjalon wrote:
> 2016-05-24 15:41, Neil Horman:
> > Modify the PMD_REGISTER_DRIVER macro, adding a name argument to it.  The
> > addition of a name argument creates a token that can be used for subsequent
> > macros in the creation of unique symbol names to export additional bits of
> > information for use by the pmdinfogen tool.  For example:
> > 
> > PMD_REGISTER_DRIVER(ena_driver, ena);
> > 
> > registers the ena_driver struct as it always did, and creates a symbol
> > const char this_pmd_name0[] __attribute__((used)) = "ena";
> > 
> > which pmdinfogen can search for and extract.
> 
> The EAL rework (http://dpdk.org/ml/archives/dev/2016-April/037691.html)
> integrates already a name:
> 
> +#define RTE_EAL_PCI_REGISTER(name, d) \
> +RTE_INIT(pciinitfn_ ##name); \
> +static void pciinitfn_ ##name(void) \
> +{ \
> + rte_eal_pci_register(); \
> +}
> 
> I think it would be better to rebase on top of it.
> 
Those patches are over a month old and still in the new state according to
patchwork.  I'm not very comfortable rebasing (and implicitly blocking)
acceptance of this patch on that one.  Its really a just two lines of conflict. 
 I
would suggest that, whichever patch gets integrated first, the other series can
rebase on the new head.  It should be a pretty easy fix either way.

> > The subsequent macro
> > 
> > DRIVER_REGISTER_PCI_TABLE(ena, ena_pci_id_map);
> > 
> > creates a symbol
> > const char ena_pci_tbl_export[] __attribute__((used)) = "ena_pci_id_map";
> > 
> > Which allows pmdinfogen to find the pci table of this driver
> > 
> > Using this pattern, we can export arbitrary bits of information.
> > 
> > pmdinfo uses this information to extract hardware support from an object 
> > file
> > and create a json string to make hardware support info discoverable later.
> 
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -34,4 +34,6 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >  DIRS-y += net
> >  DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += crypto
> >  
> > +DEPDIRS-y += buildtools/pmdinfo
> 
> Why pmdinfo is a build dependency?
> 
beause pmdinfogen has to be built and available for use prior to compiling the
rest of the dpdk.  I suppose we could build it after, and then go back through
and check all the objects for driver info, but I'd rather build it first, and
search the objects as they are built.

> > --- a/lib/librte_eal/common/include/rte_dev.h
> > +++ b/lib/librte_eal/common/include/rte_dev.h
> > @@ -48,7 +48,7 @@ extern "C" {
> >  
> >  #include 
> >  #include 
> > -
> > +#include 
> 
> Why not keep PCI stuff in rte_pci.h?
> 
I am.

> > +#define DRV_EXP_TAG(n, t) __##n##_##t
> > +
> > +#define DRIVER_REGISTER_PCI_TABLE(n, t) \
> > +static const char DRV_EXP_TAG(n, pci_tbl_export)[] __attribute__((used)) = 
> > RTE_STR(t)
> 
> I really dislike one-char variables, especially when there is no comment.
> Please choose comments or explicit variables.
> 

You mean you want the macro variables to be longer/more descriptive?  I suppose,
but in fairness, we have lots of macros that use single letter variables, I'm
not sure why your concerned about these specifically.  I'll change it though.

Neil

[dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility

[Neil Horman]

On Wed, May 25, 2016 at 03:21:19PM +0200, Thomas Monjalon wrote:
> 2016-05-24 15:41, Neil Horman:
> > pmdinfogen is a tool used to parse object files and build json strings for 
> > use in
> > later determining hardware support in a dso or application binary.  pmdinfo
> > looks for the non-exported symbol names this_pmd_name and this_pmd_tbl
> > (where n is a integer counter).  It records the name of each of these 
> > tuples,
> > using the later to find the symbolic name of the pci_table for physical 
> > devices
> > that the object supports.  With this information, it outputs a C file with a
> > single line of the form:
> > 
> > static char *_driver_info[] __attribute__((used)) = " \
> > PMD_DRIVER_INFO=";
> > 
> > Where  is the arbitrary name of the pmd, and  is the 
> > json
> > encoded string that hold relevant pmd information, including the pmd name, 
> > type
> > and optional array of pci device/vendor ids that the driver supports.
> > 
> > This c file is suitable for compiling to object code, then relocatably 
> > linking
> > into the parent file from which the C was generated.  This creates an entry 
> > in
> > the string table of the object that can inform a later tool about hardware
> > support.
> 
> This description is helpful and should be in the doc:
>   doc/guides/prog_guide/dev_kit_build_system.rst
Yeah, ok I can add that. 

> 
> > --- a/GNUmakefile
> > +++ b/GNUmakefile
> > -ROOTDIRS-y := lib drivers app
> > +ROOTDIRS-y := buildtools lib drivers app
> 
> Why a new directory?
> It is not a script nor an end-user tool, I guess.
Dependencies.  This tool has to be built prior to the rest of the dpdk, but app
already relies on dpdk libraries to be built, so you get circular dependencies.
I could have put it in scripts I guess, but its not a script.  Its own directory
seemed to make the most sense, given those two points

> 
> I feel strange to build an app for the build system.
Why?  I agree its not overly common, but theres lots of precident for it.
The linux and bsd kernels obviously do this for modules, and there are lots of
tools that convert generic descriptions in xml into platform native source code
prior to compilation.

> For information, do you know some Python lib to do this kind of tool?
> 
No, if there was I would have used it, but this sort of thing is project
specific, theres no 'generic' symbol stringification solution available.

> > +++ b/buildtools/pmdinfogen/Makefile
> > +#CFLAGS += $(WERROR_FLAGS) -g
> > +CFLAGS += -g
> 
> Please choose one line or the other or none of them.
> 
Oh, thats a debug error, I can fix that.

> > +include $(RTE_SDK)/mk/rte.buildtools.mk
> 
> Why a new Makefile? Can you use rte.hostapp.mk?
> 
I don't know, maybe.  Nothing else currently uses rte.hostapp.mk, so I missed
its existance.  I make the argument that, that being the case, we should stick
with the Makefile I just tested with, and remove the rte.hostapp.mk file


> > +++ b/buildtools/pmdinfogen/pmdinfogen.c
> [...]
> > +   /*
> > +* If this returns NULL, then this is a PMD_VDEV, because
> > +* it has no pci table reference
> > +*/
> 
> We can imagine physical PMD not using PCI.
> I think this comment should be removed.
We can, but currently its a true statement.  we have two types of PMDs, a PDEV
and a VDEV, the former is a pci device, and the latter is a virtual device, so
you can imply the PDEV type from the presence of pci entries, and VDEV from the
alternative.  If we were to do something, I would recommend adding a macro to
explicitly ennumerate each pmds type.  I would prefer to wait until that was a
need however, as it can be done invisibly to the user.

> 
> > +   if (!tmpsym) {
> > +   drv->pci_tbl = NULL;
> > +   return 0;
> > +   }
> [...]
> > +
> > +
> > +   return 0;
> > +   
> > +}
> 
> That's a lot of blank lines ;)
> 
My eyes were getting tired :)

> [...]
> > +   fprintf(ofd,"\\\"type\\\" : \\\"%s\\\", ", drv->pci_tbl ? 
> > "PMD_PDEV" : "PMD_VDEV");
> 
> Please forget the naming PDEV/VDEV.
> 
I don't know what you mean here, you would rather they be named PCI and Virtual,
or something else?


> [...]
> > +   if (info.drivers) {
> > +   output_pmd_info_string(, argv[2]);
> > +   rc = 0;
> > +   } else {
> > +   fprintf(stderr, "Hmm, Appears to be a driver but no drivers 
> > registered\n");
> 
> Why it appears to be a driver?
> What means "no drivers registered" exactly?
> 
It means that the tool has identified this file as a driver based on some
criteria (in this case the source code contained a use of the
PMD_REGISTER_DRIVER macro, but for whatever reason, when this tool scanned it,
it never located the pmd_driver_name symbol.  It should never happen, and
serves as a indicator to the developer that they need to investigate either the
construction of the driver or the use of this tool.



> > +++ b/buildtools/pmdinfogen/pmdinfogen.h
> [...]
> > +#define Elf_EhdrElf64_Ehdr
> > +#define Elf_Shdr

[dpdk-dev] [PATCH] enic: fix seg fault when releasing queues

[John Daley]

If device configuration failed due to a lack of resources, like if
there were more queues requested than available, the queue release
function is called with NULL pointers which were being dereferenced.

Skip releasing queues if they are NULL pointers. Also, if configuration
fails due to lack of resources, be more specific about which resources
are lacking.

Fixes: fefed3d1e62c ("enic: new driver")
Signed-off-by: John Daley 
---
 drivers/net/enic/enic_main.c | 30 +++---
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index bbbe660..a3eb4e3 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -467,14 +467,16 @@ int enic_alloc_intr_resources(struct enic *enic)

 void enic_free_rq(void *rxq)
 {
-   struct vnic_rq *rq = (struct vnic_rq *)rxq;
-   struct enic *enic = vnic_dev_priv(rq->vdev);
+   if (rxq != NULL) {
+   struct vnic_rq *rq = (struct vnic_rq *)rxq;
+   struct enic *enic = vnic_dev_priv(rq->vdev);

-   enic_rxmbuf_queue_release(enic, rq);
-   rte_free(rq->mbuf_ring);
-   rq->mbuf_ring = NULL;
-   vnic_rq_free(rq);
-   vnic_cq_free(>cq[rq->index]);
+   enic_rxmbuf_queue_release(enic, rq);
+   rte_free(rq->mbuf_ring);
+   rq->mbuf_ring = NULL;
+   vnic_rq_free(rq);
+   vnic_cq_free(>cq[rq->index]);
+   }
 }

 void enic_start_wq(struct enic *enic, uint16_t queue_idx)
@@ -841,16 +843,22 @@ int enic_set_vnic_res(struct enic *enic)
 {
struct rte_eth_dev *eth_dev = enic->rte_dev;

-   if ((enic->rq_count < eth_dev->data->nb_rx_queues) ||
-   (enic->wq_count < eth_dev->data->nb_tx_queues)) {
-   dev_err(dev, "Not enough resources configured, aborting\n");
+   if (enic->rq_count < eth_dev->data->nb_rx_queues) {
+   dev_err(dev, "Not enough Receive queues. Requested:%u, 
Configured:%u\n",
+   eth_dev->data->nb_rx_queues, enic->rq_count);
+   return -1;
+   }
+   if (enic->wq_count < eth_dev->data->nb_tx_queues) {
+   dev_err(dev, "Not enough Transmit queues. Requested:%u, 
Configured:%u\n",
+   eth_dev->data->nb_tx_queues, enic->wq_count);
return -1;
}

enic->rq_count = eth_dev->data->nb_rx_queues;
enic->wq_count = eth_dev->data->nb_tx_queues;
if (enic->cq_count < (enic->rq_count + enic->wq_count)) {
-   dev_err(dev, "Not enough resources configured, aborting\n");
+   dev_err(dev, "Not enough Completion queues. Required:%u, 
Configured:%u\n",
+   enic->rq_count + enic->wq_count, enic->cq_count);
return -1;
}

-- 
2.7.0

[dpdk-dev] [PATCH] virtio: use volatile to get used->idx in the loop

[Michael S. Tsirkin]

On Wed, May 25, 2016 at 10:47:30AM +0100, Bruce Richardson wrote:
> On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 25, 2016 at 08:25:20AM +, Xie, Huawei wrote:
> > > On 5/25/2016 4:12 PM, Xie, Huawei wrote:
> > > > There is no external function call or any barrier in the loop,
> > > > the used->idx would only be retrieved once.
> > > >
> > > > Signed-off-by: Huawei Xie 
> > > > ---
> > > >  drivers/net/virtio/virtio_ethdev.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio/virtio_ethdev.c 
> > > > b/drivers/net/virtio/virtio_ethdev.c
> > > > index c3fb628..f6d6305 100644
> > > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > > @@ -204,7 +204,8 @@ virtio_send_command(struct virtqueue *vq, struct 
> > > > virtio_pmd_ctrl *ctrl,
> > > > usleep(100);
> > > > }
> > > >  
> > > > -   while (vq->vq_used_cons_idx != vq->vq_ring.used->idx) {
> > > > +   while (vq->vq_used_cons_idx !=
> > > > +  *((volatile uint16_t *)(>vq_ring.used->idx))) {
> > > > uint32_t idx, desc_idx, used_idx;
> > > > struct vring_used_elem *uep;
> > > >  
> > > 
> > > Find this issue when do the code rework of RX/TX queue.
> > > As in other places, we also have loop retrieving the value of avial->idx
> > > or used->idx, i prefer to declare the index in vq structure as volatile
> > > to avoid potential issue.
> 
> Is there a reason why the value is not always volatile? I would have thought
> it would be generally safer to mark the actual value as volatile inside the
> structure definition itself? In any cases where we do want to store the value
> locally and not re-access the structure, a local variable can be used.
> 
> Regards,
> /Bruce

Linux generally discourages volatile as a general style guidance:
https://www.kernel.org/doc/Documentation/volatile-considered-harmful.txt
it doesn't have to apply to dpdk which has a different coding style
but IIUC this structure is inherited from linux, deviating
will make keeping things up to date harder.

> > 
> > It might be a good idea to wrap this in a macro
> > similar to ACCESS_ONCE in Linux.
> > 
> > > 
> > > Stephen:
> > > Another question is why we need a loop here?
> > > 
> > > /huawei
> > 
> > -- 
> > MST

[dpdk-dev] [PATCH] virtio: check if devargs is NULL before checking its value

[Thomas Monjalon]

> - dev->devargs->type != RTE_DEVTYPE_WHITELISTED_PCI) {
> + (!dev->devargs ||
> +  dev->devargs->type != RTE_DEVTYPE_WHITELISTED_PCI)) {

Should the title be something like "fix crash ..."?

I would also add
Reported-by: Vincent Li 

[dpdk-dev] [PATCH] i40e: fix unchecked return value

[Thomas Monjalon]

2016-05-24 09:25, Daniel Mrzyglod:
> Fixes: 71d35259ff67 ("i40e: tear down flow director")
> Coverity ID 13198

FYI, in recent commits I've started to standardize the coverity reports
like that:

Coverity issue: X
Fixes: ...

Please spread the word.

[dpdk-dev] [PATCH v2] vhost: add support for dynamic vhost PMD creation

[Yuanhan Liu]

On Tue, May 24, 2016 at 10:42:56AM +0100, Bruce Richardson wrote:
> On Tue, May 24, 2016 at 01:11:26PM +0800, Yuanhan Liu wrote:
> > On Mon, May 23, 2016 at 06:06:21PM +0100, Ferruh Yigit wrote:
> > > On 5/23/2016 2:24 PM, Yuanhan Liu wrote:
> > > > On Fri, May 20, 2016 at 11:37:47AM +0100, Bruce Richardson wrote:
> > > >> On Thu, May 19, 2016 at 06:44:44PM +0200, Thomas Monjalon wrote:
> > > >>> 2016-05-19 17:28, Ferruh Yigit:
> > >  On 5/19/2016 9:33 AM, Thomas Monjalon wrote:
> > > > 2016-05-18 18:10, Ferruh Yigit:
> > > >> Add rte_eth_from_vhost() API to create vhost PMD dynamically from
> > > >> applications.
> > > >
> > > > How is it different from rte_eth_dev_attach() calling 
> > > > rte_eal_vdev_init()?
> > > >
> > > 
> > >  When used rte_eth_dev_attach(), application also needs to do:
> > >  rte_eth_dev_configure()
> > >  rte_eth_rx_queue_setup()
> > >  rte_eth_tx_queue_setup()
> > >  rte_eth_dev_start()
> > > 
> > >  rte_eth_from_vhost() does these internally, easier to use for 
> > >  applications.
> > > >>>
> > > >>> This argument is not sufficient.
> > > >>> We are not going to add new APIs just for wrapping others.
> > > >>
> > > >> Why not - if there is a sufficient increase in developer usability by 
> > > >> doing so?
> > > >> Having one API that saves an app from having to call 5 other APIs 
> > > >> looks like
> > > >> something that should always be given fair consideration.
> > > > 
> > > > Good point. Judging that vhost is not the only virtual device we
> > > > support, and it may also look reasonable to add something similar
> > > > for others in future (say, IIRC, you proposed two more internally
> > > > that also introduced similar APIs). So, instead of introducing a
> > > > new API for each such vdev, may we introduce a common one? Say,
> > > > a refined rte_eth_dev_attach(), including dev_configure(),
> > > > queue_setup(), etc.
> > > > 
> > > 
> > > This sounds good to me. If there is not objection, I will send a patch
> > > and we can discuss based on patch.
> > 
> > Let's wait and gather some comments first?
> > 
> I'm not sure that such a general approach is likely to work,

Me, neither. Thus I threw it out for more discussion.

> as the parameters
> needed for each individual driver are going to be different.

Well, if you plan to pass all necessary informations to the driver by
parameters like this v1 does, then yes, that's true and a generic API
is unlikely to work. But what I was thinking is that we feed it by
strings, like the arguments for '--vdev' option. In such way, we could
have an unified interface (if that works, which is something I'm not
quite sure).

OTOH, let's assume there is a switch that supports quite many such
vdevs, as well as the ability to add a new device dynamically by
corresponding API. And assume there is just one external interface
from the switch to add a dynamical device (say, "ovs-vsctl add-port"),
you then also need build some codes to invoke the right API, as well
as constructing the right parameters, like what you said below.

This let me think of the vhost dequeue/enqueue API. Basically speaking,
it has the same functionality the rte_eth_rx/tx_burst has, but just
different API name and different parameters. This results to OVS has
to write different netdev_class, one for NIC, another one for vhost-user.
(actually, there is yet another one for vhost-cuse).

And now since we have vhost-pmd, we could just have one netdev_class
at OVS, saving their (and other application's) effort to build/maintain
similar codes.

Thus, I'm __just wondering__ could we add a generic interface to create
vdev dynamically for all such vdevs? I was thinking something like:

rte_create_vdev(type, char *options);

Which in turn will invoke the right function pointer for different
"type" to do the right setups.

--yliu

> For some devices,
> much of the parameters can be implied, while for others they may not be and 
> still
> others needed additional setup parameters. For the simplest case, take the
> rte_eth_from_ring API, which creates an ethdev backed by a single rte_ring. 
> The
> number of rx and tx queues and their sizes are all determined by the actual
> underlying ring, as is the numa node and all other parameters. On the other
> hand, we have something like a pcap PMD, where again none of the queue sizes
> need to be specified, but we do need additional parameters to provide the
> underlying pcap file/device to use. Other devices will similarly need 
> different
> options, including in some cases queue counts and sizes.
> 
> Therefore, I think trying to generalise the function is pointless. If you have
> to write your code to build up a specific set of parameters to pass to a 
> general
> API, then you are no better off than just calling a specific API directly. In
> both cases you need different code for each device type.
> 
> Regards,
> /Bruce

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

[Thomas Monjalon]

2016-05-24 21:38, Huawei Xie:
> We keep a common vq structure, containing only vq related fields,
> and then split others into RX, TX and control queue respectively.
> 
> Signed-off-by: Huawei Xie 

Is it a v2? There is neither changelog nor v2 in the title.

[dpdk-dev] [PATCH v8 0/3] i40e: Add floating VEB support for i40e

[Thomas Monjalon]

2016-05-24 12:22, Stephen Hemminger:
> kvargs are a very awkward API to use in a portable application.
> Good for Intel testing NIC's bad for DPDK users.

Yes. The alternative would be to have some driver-specific API.
We can live with devargs until driver API is introduced.

[dpdk-dev] [PATCH 6/6] testpmd: update documentation

[Thomas Monjalon]

2016-05-05 18:47, Zhihong Wang:
> This patch updates documentation for testpmd.

Please avoid such doc update patch.
It is preferred to have the doc changes in the patch changing the code.
Thanks for the good work!

[dpdk-dev] [PATCH 5/6] testpmd: show topology at forwarding start

[Thomas Monjalon]

2016-05-05 18:47, Zhihong Wang:
> This patch show topology at forwarding start.
> 
> "show config fwd" also does this, but showing it directly can reduce the
> possibility of misconfiguration.
[...]
> - fwd_config_setup();
> + fwd_config_display();
>   rxtx_config_display();

Having _display() calling _setup() is really strange.
Maybe it is worth to be fixed in this patch.

[dpdk-dev] [PATCH 4/6] testpmd: handle all rxqs in rss setup

[Thomas Monjalon]

2016-05-05 18:46, Zhihong Wang:
> This patch removes constraints in rxq handling when multiqueue is enabled
> to handle all the rxqs.
> 
> Current testpmd forces a dedicated core for each rxq, some rxqs may be
> ignored when core number is less than rxq number, and that causes confusion
> and inconvenience.

I have the feeling that "constraints", "confusion" and "inconvenience"
should be more explained.
Please give some examples with not enough and too much cores. Thanks

[dpdk-dev] [PATCH 2/6] testpmd: configurable tx_first burst number

[Thomas Monjalon]

2016-05-05 18:46, Zhihong Wang:
> This patch enables configurable tx_first burst number.
> 
> Use "start tx_first (burst_num)" to specify how many bursts of packets to
> be sent before forwarding start, or "start tx_first" like before for the
> default 1 burst send.

The idea here is to fill the loopback latency gap with bursts.
Would it be possible to make it automatic by detecting the first
received packets to stop Tx generator?

[dpdk-dev] [PATCH] virtio: use volatile to get used->idx in the loop

[Michael S. Tsirkin]

On Wed, May 25, 2016 at 08:25:20AM +, Xie, Huawei wrote:
> On 5/25/2016 4:12 PM, Xie, Huawei wrote:
> > There is no external function call or any barrier in the loop,
> > the used->idx would only be retrieved once.
> >
> > Signed-off-by: Huawei Xie 
> > ---
> >  drivers/net/virtio/virtio_ethdev.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c 
> > b/drivers/net/virtio/virtio_ethdev.c
> > index c3fb628..f6d6305 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -204,7 +204,8 @@ virtio_send_command(struct virtqueue *vq, struct 
> > virtio_pmd_ctrl *ctrl,
> > usleep(100);
> > }
> >  
> > -   while (vq->vq_used_cons_idx != vq->vq_ring.used->idx) {
> > +   while (vq->vq_used_cons_idx !=
> > +  *((volatile uint16_t *)(>vq_ring.used->idx))) {
> > uint32_t idx, desc_idx, used_idx;
> > struct vring_used_elem *uep;
> >  
> 
> Find this issue when do the code rework of RX/TX queue.
> As in other places, we also have loop retrieving the value of avial->idx
> or used->idx, i prefer to declare the index in vq structure as volatile
> to avoid potential issue.

It might be a good idea to wrap this in a macro
similar to ACCESS_ONCE in Linux.

> 
> Stephen:
> Another question is why we need a loop here?
> 
> /huawei

-- 
MST

[dpdk-dev] [PATCH 1/6] testpmd: add io_retry forwarding

[Thomas Monjalon]

2016-05-05 18:46, Zhihong Wang:
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
>  extern struct fwd_engine io_fwd_engine;
> +extern struct fwd_engine io_retry_fwd_engine;
>  extern struct fwd_engine mac_fwd_engine;
>  extern struct fwd_engine mac_retry_fwd_engine;
>  extern struct fwd_engine mac_swap_engine;

We now have 2 engines with "retry" behaviour.
It is maybe the way to go, but I want to ask the question:
Would it be possible to have "retry" as an engine parameter?

[dpdk-dev] [PATCHv4 0/5] Implement pmd hardware support exports

[Panu Matilainen]

On 05/24/2016 10:41 PM, Neil Horman wrote:
> Hey all-
>   So heres attempt number 2 at a method for exporting PMD hardware support
> information.  As we discussed previously, the consensus seems to be that pmd
> information should be:
>
> 1) Able to be interrogated on any ELF binary (application binary or individual
> DSO)
> 2) Equally functional on statically linked applications or on DSO's
> 3) Resilient to symbol stripping
> 4) Script friendly
> 5) Show kernel dependencies
> 6) List driver options
> 7) Show driver name
> 8) Offer human readable output
> 9) Show DPDK version
> 10) Show driver version
> 11) Allow for expansion
> 12) Not place additional build environment dependencies on an application
>
[...]
> v4)
>  * Modified the operation of the -p option. As much as I don't like implying
> that autoloaded pmds are guaranteed to be there at run time, I'm having a hard
> time seeing how we can avoid specifying the application file to scan for the
> autoload directory.  Without it we can't determine which library the user 
> means
> in a multiversion installation
>  * Cleaned up the help text
>  * Added a rule for an install target for pmdinfo
>  * Guarded against some tracebacks in pmdinfo
>  * Use DT_NEEDED entries to get versioned libraries in -p mode

Thank you! That's exactly what I've been asking for all along.

>  * Fixed traceback that occurs on lack of input arguments
>  * Fixed some erroneous macro usage in drivers that aren't in the default 
> build
>
> Signed-off-by: Neil Horman 
> CC: Bruce Richardson 
> CC: Thomas Monjalon 
> CC: Stephen Hemminger 
> CC: Panu Matilainen 

/me happy now, so:

Acked-by: Panu Matilainen 

As always there might be some refining to do as we get more experience 
with it but it seems like a fine starting point to me.

- Panu -

[dpdk-dev] [PATCH 0/6] vhost/virtio performance loopback utility

[Thomas Monjalon]

CC Pablo, testpmd maintainer

Pablo,
This patchset looks really valuable to improve performance debugging.
Would you have time to dig into a review please?


2016-05-05 18:46, Zhihong Wang:
> This patch enables vhost/virtio pmd performance loopback test in testpmd.
> All the features are for general usage.
> 
> The loopback test focuses on the maximum full-path packet forwarding
> performance between host and guest, it runs vhost/virtio pmd only without
> introducing extra overhead.
> 
> Therefore, the main requirement is traffic generation, since there's no
> other packet generators like IXIA to help.
> 
> In current testpmd, io-fwd is the ideal candidate to perform this loopback
> test because it's the fastest possible forwarding engine: Start testpmd
> io-fwd in host with 1 vhost pmd port, and start testpmd io-fwd in the
> connected guest with 1 corresponding virtio pmd port, and these 2 ports
> form a forwarding loop, packets received by the host vhost pmd port are
> forwarded to the guest virtio pmd port, and packets received by the guest
> virtio pmd port are sent to the host vhost pmd port.
> 
> As to traffic generation, "start tx_first" injects a burst of packets into
> the loop, which is the ideal way to do that.
> 
> However 2 issues remain:
> 
>1. If only 1 burst of packets are injected in the loop, there will
>   almost definitely be empty rx operations, e.g. When guest virtio pmd
>   port send burst to the host, then it starts the rx immediately, it's
>   likely the packets are still being forwarded by host vhost pmd port
>   and haven't reached the guest yet.
> 
>   We need to fill up the ring to keep all pmds busy.
> 
>2. io-fwd doesn't provide retry mechanism, so if packet loss occurs,
>   there won't be a full burst in the loop.
> 
> To address these issues, this patch:
> 
>1. Add an io_retry-fwd in testpmd to prevent most packet losses.
> 
>2. Add parameter to enable configurable tx_first burst number.
> 
> Other related improvements include:
> 
>1. Handle all rxqs when multiqueue is enabled: Current testpmd forces a
>   single core for each rxq which causes inconvenience and confusion.
> 
>2. Show topology at forwarding start: "show config fwd" also does this,
>   but show it directly can reduce the possibility of mis-configuration.
> 
>3. Add throughput information in port statistics display for "show port
>   stats (port_id|all)".
> 
> Finally there's documentation update.

[dpdk-dev] fast red autotest

[Kantecki, Tomasz]

Hi Thomas,

I had a quick look through these tests and it will need some effort to rework 
them to run in shorter time.
I agree to remove this suite from the fast_test as long as it gets exercised in 
other test paths.

Regards,
Tomasz

> -Original Message-
> From: Dumitrescu, Cristian
> Sent: Tuesday, May 24, 2016 6:08 PM
> To: Thomas Monjalon ; Kantecki, Tomasz
> 
> Cc: dev at dpdk.org
> Subject: RE: fast red autotest
> 
> Hi Thomas,
> 
> From my side, I am OK to remove RED from the fast autotest, as long as it is
> kept available as part of the normal/full autotest of DPDK.
> 
> Some of the RED autotests need a long time to run in order to train the
> history for the average queue size stochastic variable, therefore it is 
> difficult
> to shorten them. However, some tests are quick to execute, so those tests
> can still be included into the fast autotest. Tomazs, any comments/proposal?
> 
> Regards,
> Cristian
> 
> > -Original Message-
> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > Sent: Tuesday, May 24, 2016 4:24 PM
> > To: Dumitrescu, Cristian ; Kantecki,
> > Tomasz 
> > Cc: dev at dpdk.org
> > Subject: Re: fast red autotest
> >
> > Any news Tomasz, Cristian?
> >
> > 2016-05-11 10:15, Dumitrescu, Cristian:
> > > CC-ing Tomasz, who is the original author of RED implementation and
> > > its
> > autotest. Tomasz, what do you think?
> > >
> > > > -Original Message-
> > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > > Sent: Wednesday, May 11, 2016 8:45 AM
> > > > To: Dumitrescu, Cristian 
> > > > Cc: dev at dpdk.org
> > > > Subject: fast red autotest
> > > >
> > > > The autotest for librte_sched red takes more than a minute.
> > > > Would it be possible to reduce it to a second please?
> > > > If it is really impossible, it must be removed from fast_test.
> >

--
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

[dpdk-dev] [PATCH] virtio: use volatile to get used->idx in the loop

[Bruce Richardson]

On Wed, May 25, 2016 at 12:50:02PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 25, 2016 at 10:47:30AM +0100, Bruce Richardson wrote:
> > On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote:
> > > On Wed, May 25, 2016 at 08:25:20AM +, Xie, Huawei wrote:
> > > > On 5/25/2016 4:12 PM, Xie, Huawei wrote:
> > > > > There is no external function call or any barrier in the loop,
> > > > > the used->idx would only be retrieved once.
> > > > >
> > > > > Signed-off-by: Huawei Xie 
> > > > > ---
> > > > >  drivers/net/virtio/virtio_ethdev.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio/virtio_ethdev.c 
> > > > > b/drivers/net/virtio/virtio_ethdev.c
> > > > > index c3fb628..f6d6305 100644
> > > > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > > > @@ -204,7 +204,8 @@ virtio_send_command(struct virtqueue *vq, struct 
> > > > > virtio_pmd_ctrl *ctrl,
> > > > >   usleep(100);
> > > > >   }
> > > > >  
> > > > > - while (vq->vq_used_cons_idx != vq->vq_ring.used->idx) {
> > > > > + while (vq->vq_used_cons_idx !=
> > > > > +*((volatile uint16_t *)(>vq_ring.used->idx))) {
> > > > >   uint32_t idx, desc_idx, used_idx;
> > > > >   struct vring_used_elem *uep;
> > > > >  
> > > > 
> > > > Find this issue when do the code rework of RX/TX queue.
> > > > As in other places, we also have loop retrieving the value of avial->idx
> > > > or used->idx, i prefer to declare the index in vq structure as volatile
> > > > to avoid potential issue.
> > 
> > Is there a reason why the value is not always volatile? I would have thought
> > it would be generally safer to mark the actual value as volatile inside the
> > structure definition itself? In any cases where we do want to store the 
> > value
> > locally and not re-access the structure, a local variable can be used.
> > 
> > Regards,
> > /Bruce
> 
> Linux generally discourages volatile as a general style guidance:
> https://www.kernel.org/doc/Documentation/volatile-considered-harmful.txt
> it doesn't have to apply to dpdk which has a different coding style
> but IIUC this structure is inherited from linux, deviating
> will make keeping things up to date harder.

The prohibition on volatile indeed doesn't apply to DPDK, due to the fact that
we so seldom use locks, and do a lot of direct register accesses in out PMDs.
[I also still have the scars from previous issues where we had nice subtle bugs
in our PMDs - which only occurred with specific subversions of gcc - all due
to a missing "volatile" on one structure element.]

However, in this case, I take your point about keeping things consistent with
the kernel. :-)

/Bruce

> 
> > > 
> > > It might be a good idea to wrap this in a macro
> > > similar to ACCESS_ONCE in Linux.
> > > 
> > > > 
> > > > Stephen:
> > > > Another question is why we need a loop here?
> > > > 
> > > > /huawei
> > > 
> > > -- 
> > > MST

[dpdk-dev] [PATCH v1 1/2] rte_memcmp functions using Intel AVX and SSE intrinsics

[Thomas Monjalon]

2016-03-07 15:00, Ravi Kerur:
> v1:
> This patch adds memcmp functionality using AVX and SSE
> intrinsics provided by Intel. For other architectures
> supported by DPDK regular memcmp function is used.

Anyone to review this patch please? Zhihong?

[dpdk-dev] [PATCH v2 6/6] vhost: add pmd client and reconnect option

[Rich Lane]

>
> @@ -817,6 +821,9 @@ rte_pmd_vhost_devinit(const char *name, const char
> *params)
> int ret = 0;
> char *iface_name;
> uint16_t queues;
> +   uint64_t flags = 0;
> +   int client_mode;
> +   int reconnect;
>

client_mode and reconnect are not initialized if the arguments aren't
passed.

[dpdk-dev] [PATCH] app/test: fix +/-1 error in allocation

[Thomas Monjalon]

2016-05-04 10:15, David Marchand:
> On Tue, May 3, 2016 at 9:15 PM, Jan Viktorin  
> wrote:
> > A bug has been detected by valgrind:
[...]
> >  strlen(t->command) + strlen("#") + ONE_FOR_ZERO
> >
> > Fixes: 727909c59231 ("app/test: introduce dynamic commands list")
> >
> > Signed-off-by: Jan Viktorin 
> 
> Good catch.
> Acked-by: David Marchand 

Applied, thanks

[dpdk-dev] [PATCH] examples/l2fwd-crypto: enable AES-XCBC-MAC authentication algorithm

[Fan Zhang]

This patch enables AES-XCBC-MAC authentication algorithm support to
l2fwd-crypto sample application.

Signed-off-by: Fan Zhang 
---
 examples/l2fwd-crypto/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/examples/l2fwd-crypto/main.c b/examples/l2fwd-crypto/main.c
index d4e2d8d..dccba79 100644
--- a/examples/l2fwd-crypto/main.c
+++ b/examples/l2fwd-crypto/main.c
@@ -341,6 +341,8 @@ fill_supported_algorithm_tables(void)
strcpy(supported_auth_algo[RTE_CRYPTO_AUTH_AES_GCM], "AES_GCM");
strcpy(supported_auth_algo[RTE_CRYPTO_AUTH_MD5_HMAC], "MD5_HMAC");
strcpy(supported_auth_algo[RTE_CRYPTO_AUTH_NULL], "NULL");
+   strcpy(supported_auth_algo[RTE_CRYPTO_AUTH_AES_XCBC_MAC],
+   "AES_XCBC_MAC");
strcpy(supported_auth_algo[RTE_CRYPTO_AUTH_SHA1_HMAC], "SHA1_HMAC");
strcpy(supported_auth_algo[RTE_CRYPTO_AUTH_SHA224_HMAC], "SHA224_HMAC");
strcpy(supported_auth_algo[RTE_CRYPTO_AUTH_SHA256_HMAC], "SHA256_HMAC");
-- 
2.5.5

[dpdk-dev] [PATCH] doc: fix l2fwd-crypto sample command

[Fan Zhang]

Fixes ba7b86b1 ("doc: add l2fwd-crypto sample app guide")

Corrected a typo in application name.

Corrected authentication algorithm to fit the sample 16-byte
authentication key.

Signed-off-by: Fan Zhang 
---
 doc/guides/sample_app_ug/l2_forward_crypto.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/guides/sample_app_ug/l2_forward_crypto.rst 
b/doc/guides/sample_app_ug/l2_forward_crypto.rst
index 7cce51b..723376c 100644
--- a/doc/guides/sample_app_ug/l2_forward_crypto.rst
+++ b/doc/guides/sample_app_ug/l2_forward_crypto.rst
@@ -167,11 +167,11 @@ To run the application in linuxapp environment with 2 
lcores, 2 ports and 2 cryp

 .. code-block:: console

-$ ./build/l2fwd -c 0x3 -n 4 --vdev "cryptodev_aesni_mb_pmd" \
+$ ./build/l2fwd-crypto -c 0x3 -n 4 --vdev "cryptodev_aesni_mb_pmd" \
 --vdev "cryptodev_aesni_mb_pmd" -- -p 0x3 --chain CIPHER_HASH \
 --cipher_op ENCRYPT --cipher_algo AES_CBC \
 --cipher_key 00:01:02:03:04:05:06:07:08:09:0a:0b:0c:0d:0e:0f \
---auth_op GENERATE --auth_algo SHA1_HMAC \
+--auth_op GENERATE --auth_algo AES_XCBC_MAC \
 --auth_key 10:11:12:13:14:15:16:17:18:19:1a:1b:1c:1d:1e:1f

 Refer to the *DPDK Getting Started Guide* for general information on running 
applications
-- 
2.5.5

[dpdk-dev] [PATCH] examples/l2fwd-crypto: enable AES counter mode cipher algorithm

[Fan Zhang]

This patch enables AES counter mode algorithm support to l2fwd-crypto
sample application.

This patch depends on the following patches:
"qat: add AES counter mode capability"
(http://dpdk.org/dev/patchwork/patch/12464/)

"app/test: add test cases for AES CTR"
(http://dpdk.org/dev/patchwork/patch/12465/)

"aesni_mb: add counter mode support:
(http://dpdk.org/dev/patchwork/patch/12399/)

"app/test: add aes-ni multi-buffer pmd test cases for AES CTR"
(http://dpdk.org/dev/patchwork/patch/12400/)

Signed-off-by: Fan Zhang 
---
 examples/l2fwd-crypto/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/l2fwd-crypto/main.c b/examples/l2fwd-crypto/main.c
index d18c813..66fc874 100644
--- a/examples/l2fwd-crypto/main.c
+++ b/examples/l2fwd-crypto/main.c
@@ -352,6 +352,7 @@ fill_supported_algorithm_tables(void)
strcpy(supported_cipher_algo[i], "NOT_SUPPORTED");

strcpy(supported_cipher_algo[RTE_CRYPTO_CIPHER_AES_CBC], "AES_CBC");
+   strcpy(supported_cipher_algo[RTE_CRYPTO_CIPHER_AES_CTR], "AES_CTR");
strcpy(supported_cipher_algo[RTE_CRYPTO_CIPHER_AES_GCM], "AES_GCM");
strcpy(supported_cipher_algo[RTE_CRYPTO_CIPHER_NULL], "NULL");
strcpy(supported_cipher_algo[RTE_CRYPTO_CIPHER_SNOW3G_UEA2], 
"SNOW3G_UEA2");
-- 
2.5.5

[dpdk-dev] [PATCH 2/2] examples/ethtool: get reg width to allocate memory

[z...@semihalf.com]

From: Zyta Szpak 

Version 2 of fixing the fixed register width assumption.
Not every device uses 32-bit wide register. The app was allocating too
little space for 64-bit registers which resulted in memory corruption.
This commit resolves this by getting the size of register in bytes for
a specific device. If the device does not implement this function, it
fallsback to sizeof(uint32_t)

Signed-off-by: Zyta Szpak 
---
 examples/ethtool/lib/rte_ethtool.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/examples/ethtool/lib/rte_ethtool.c 
b/examples/ethtool/lib/rte_ethtool.c
index 42e05f1..59191ca 100644
--- a/examples/ethtool/lib/rte_ethtool.c
+++ b/examples/ethtool/lib/rte_ethtool.c
@@ -88,10 +88,14 @@ int
 rte_ethtool_get_regs_len(uint8_t port_id)
 {
int count_regs;
+   int reg_width;

count_regs = rte_eth_dev_get_reg_length(port_id);
+   reg_width = rte_eth_dev_get_reg_width(port_id);
+   if (reg_width < 0)
+   reg_width = sizeof(uint32_t);
if (count_regs > 0)
-   return count_regs * sizeof(uint32_t);
+   return count_regs * reg_width;
return count_regs;
 }

-- 
1.9.1

[dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[z...@semihalf.com]

From: Zyta Szpak 

Version 2 of fixing the fixed register width assumption.
rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks
do not provide register size to the app in any way. It is
needed to allocate proper number of bytes before retrieving
registers content with rte_eth_dev_get_reg.

Signed-off-by: Zyta Szpak 
---
 lib/librte_ether/rte_ethdev.c | 12 
 lib/librte_ether/rte_ethdev.h | 18 ++
 2 files changed, 30 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index a31018e..e0765f8 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3231,6 +3231,18 @@ rte_eth_dev_get_reg_length(uint8_t port_id)
 }

 int
+rte_eth_dev_get_reg_width(uint8_t port_id)
+{
+   struct rte_eth_dev *dev;
+
+   RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+   dev = _eth_devices[port_id];
+   RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_reg_width, -ENOTSUP);
+   return (*dev->dev_ops->get_reg_width)(dev);
+}
+
+int
 rte_eth_dev_get_reg_info(uint8_t port_id, struct rte_dev_reg_info *info)
 {
struct rte_eth_dev *dev;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 2757510..552eaed 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1292,6 +1292,9 @@ typedef int (*eth_timesync_write_time)(struct rte_eth_dev 
*dev,
 typedef int (*eth_get_reg_length_t)(struct rte_eth_dev *dev);
 /**< @internal Retrieve device register count  */

+typedef int (*eth_get_reg_width_t)(struct rte_eth_dev *dev);
+/**< @internal Retrieve device register byte number */
+
 typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev,
struct rte_dev_reg_info *info);
 /**< @internal Retrieve registers  */
@@ -1455,6 +1458,8 @@ struct eth_dev_ops {

eth_get_reg_length_t get_reg_length;
/**< Get # of registers */
+   eth_get_reg_width_t get_reg_width;
+   /**< Get # of bytes in register */
eth_get_reg_t get_reg;
/**< Get registers */
eth_get_eeprom_length_t get_eeprom_length;
@@ -3971,6 +3976,19 @@ int rte_eth_tx_queue_info_get(uint8_t port_id, uint16_t 
queue_id,
  */
 int rte_eth_dev_get_reg_length(uint8_t port_id);

+/*
+ * Retrieve the number of bytes in register for a specific device
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (>=0) number of registers if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_eth_dev_get_reg_width(uint8_t port_id);
+
 /**
  * Retrieve device registers and register attributes
  *
-- 
1.9.1

[dpdk-dev] [PATCH] virtio: use volatile to get used->idx in the loop

[Xie, Huawei]

On 5/25/2016 4:12 PM, Xie, Huawei wrote:
> There is no external function call or any barrier in the loop,
> the used->idx would only be retrieved once.
>
> Signed-off-by: Huawei Xie 
> ---
>  drivers/net/virtio/virtio_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c 
> b/drivers/net/virtio/virtio_ethdev.c
> index c3fb628..f6d6305 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -204,7 +204,8 @@ virtio_send_command(struct virtqueue *vq, struct 
> virtio_pmd_ctrl *ctrl,
>   usleep(100);
>   }
>  
> - while (vq->vq_used_cons_idx != vq->vq_ring.used->idx) {
> + while (vq->vq_used_cons_idx !=
> +*((volatile uint16_t *)(>vq_ring.used->idx))) {
>   uint32_t idx, desc_idx, used_idx;
>   struct vring_used_elem *uep;
>  

Find this issue when do the code rework of RX/TX queue.
As in other places, we also have loop retrieving the value of avial->idx
or used->idx, i prefer to declare the index in vq structure as volatile
to avoid potential issue.

Stephen:
Another question is why we need a loop here?

/huawei

[dpdk-dev] If 1 KVM Guest loads the virtio-pci, on top of dpdkvhostuser OVS socket interface, it slows down everything!

[Christian Ehrhardt]

Hi again,
another forgotten case.

I currently I lack the HW to fully reproduce this, but the video summary is
pretty good and shows the issue in an impressive way.

Also the description is good and here as well I wonder if anybody else
could reproduce this.
Any hints / insights are welcome.

P.S. and also again - two list cross posting, but here as well it is yet
unclear which it belongs to so I'll keep it as well

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Sun, May 22, 2016 at 6:35 PM, Martinx - ? 
wrote:

> Guys,
>
>  I'm seeing a strange problem here, in my OVS+DPDK deployment, on top of
> Ubuntu 16.04 (DPDK 2.2 and OVS 2.5).
>
>  Here is what I'm trying to do: run OVS with DPDK at the host, for KVM
> Guests that also, will be running more DPDK Apps.
>
>  The host have 2 x 10G NICs, for OVS+DPDK and each KVM Guest receives its
> own VLAN tagged traffic (or all tags).
>
>  There is an IXIA Traffic Generator sending 10G of traffic on both
> directions (20G total).
>
>  Exemplifying, the problem is, lets say that I already have 2 VMs (or 10)
> running DPDK Apps (on top of dpdkvhostuser), everything is working as
> expected, then, if I boot the 3rd (or 11) KVM Guest, the OVS+DPDK bridge at
> the host, slows down, a lot! The 3rd (or 11) VM affects not only the host,
> but also, all the other neighbors VMs!!!
>
>  NOTE: This problem appear since the boot of VM 1.
>
>  Soon as you, inside of the 3rd VM, bind the VirtIO NIC to the
> DPDK-Compative Drivers, the speed comes back to normal. If you bind it back
> to "virtio-pci", boom! The OVS+DPDK at the host and all VMs loses too much
> speed.
>
>  This problem is detailed at the following bug report:
>
> --
> The OVS+DPDK dpdkvhostuser socket bridge, only works as expected, if the
> KVM Guest also have DPDK drivers loaded:
>
> https://bugs.launchpad.net/ubuntu/+source/openvswitch/+bug/1577256
> --
>
>  Also, I've recorded a ~15 min screen cast video about this problem, so,
> you guys can see exactly what is happening here.
>
> https://www.youtube.com/v/yHnaSikd9XY?version=3=hd720=1
>
>  * At 5:25, I'm starting a VM that will boot up and load a DPDK App;
>
>  * At 5:33, OVS+DPDK is messed up, it loses speed;
>The KVM running with virtio-pci drivers breaks OVS+DPDK at the host;
>
>  * At 6:50, DPDK inside of the KVM guest loads up its drivers, kicking
> "virtio-pci", speed back to normal at the host;
>
>  * At 7:43, started another KVM Guest, now, while virtio-pci driver is
> running, the OVS+DPDK at the host and the other VM, are very, very slow;
>
>  * At 8:52, the second VM loads up DPDK Drivers, kicking virtio-pci, the
> speed is back to normal at the host, and on the other VM too;
>
>  * At 10:00, the Ubuntu VM loads up virtio-pci drivers on its boot, the
> speed dropped at the hosts and on the other VMs;
>
>  * 11:57, I'm starting "service dpdk start" inside of the Ubuntu guest, to
> kick up virtio-pci, and bang! Speed is back to normal everywhere;
>
>  * 12:51, I'm trying to unbind the DPDK Drivers and return the virtio-pci,
> I forgot the syntax while recording the video, which is: "dpdk_nic_bind -b
>  virtio-pci", so, I just rebooted it. But both "reboot" or "rebind to
> virtio-pci" triggers the bug.
>
>
> NOTE: I tried to subscriber to qemu-devel but, it is not working, I'm not
> receiving the confirmation e-mail, while qemu-stable worked. I don't know
> if it worth sending it to Linux Kernel too...
>
>
> Regards,
> Thiago
>

[dpdk-dev] Crashing OVS+DPDK at the host, from inside of a KVM Guest

[Christian Ehrhardt]

Hi,
ping ...

Later on I want to look at it again once we upgraded to more recent
releases of the software components involved, but those have to be made
ready to use first :-/

But the description is good and I wonder if anybody else could reproduce
this and/or would have a hint on where this might come from or already
existing related fixes.

I mean in general nothing should be able to crash the host right?


P.S. yeah two list cross posting, but it is yet unclear which it belongs to
so I'll keep it

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Sun, May 15, 2016 at 7:08 AM, Martinx - ? 
wrote:

> Guys,
>
>  If using OVS 2.5 with DPDK 2.2, on Ubuntu Xenial, it is possible to crash
> the OVS running at the host, from inside of a KVM Guest.
>
>  Basically, what I'm trying to do, is to run OVS+DPDK at the host, and
> also, inside of a KVM Guest, with multi-queue, but it doesn't work and
> crashes.
>
>  Soon as you enable multi-queue at the guest, it crashes the OVS of the
> host!
>
> OVS+DPDK segfault at the host, after running "ovs-vsctl set Open_vSwitch .
> other_config:n-dpdk-rxqs=4" within a KVM Guest:
>
> https://bugs.launchpad.net/ubuntu/+source/openvswitch/+bug/1577088
>
> Thanks!
> Thiago
>

[dpdk-dev] [PATCHv4 0/5] Implement pmd hardware support exports

[Neil Horman]

On Wed, May 25, 2016 at 11:32:06AM +0300, Panu Matilainen wrote:
> On 05/24/2016 10:41 PM, Neil Horman wrote:
> > Hey all-
> > So heres attempt number 2 at a method for exporting PMD hardware support
> > information.  As we discussed previously, the consensus seems to be that pmd
> > information should be:
> > 
> > 1) Able to be interrogated on any ELF binary (application binary or 
> > individual
> > DSO)
> > 2) Equally functional on statically linked applications or on DSO's
> > 3) Resilient to symbol stripping
> > 4) Script friendly
> > 5) Show kernel dependencies
> > 6) List driver options
> > 7) Show driver name
> > 8) Offer human readable output
> > 9) Show DPDK version
> > 10) Show driver version
> > 11) Allow for expansion
> > 12) Not place additional build environment dependencies on an application
> > 
> [...]
> > v4)
> >  * Modified the operation of the -p option. As much as I don't like implying
> > that autoloaded pmds are guaranteed to be there at run time, I'm having a 
> > hard
> > time seeing how we can avoid specifying the application file to scan for the
> > autoload directory.  Without it we can't determine which library the user 
> > means
> > in a multiversion installation
> >  * Cleaned up the help text
> >  * Added a rule for an install target for pmdinfo
> >  * Guarded against some tracebacks in pmdinfo
> >  * Use DT_NEEDED entries to get versioned libraries in -p mode
> 
> Thank you! That's exactly what I've been asking for all along.
> 
Well, don't thank me, I'm not a big fan of it, I just don't see a way around it
at this point, not without some heuristic thats going to be wrong half the time.

> >  * Fixed traceback that occurs on lack of input arguments
> >  * Fixed some erroneous macro usage in drivers that aren't in the default 
> > build
> > 
> > Signed-off-by: Neil Horman 
> > CC: Bruce Richardson 
> > CC: Thomas Monjalon 
> > CC: Stephen Hemminger 
> > CC: Panu Matilainen 
> 
> /me happy now, so:
> 
> Acked-by: Panu Matilainen 
> 
thanks
Neil

> As always there might be some refining to do as we get more experience with
> it but it seems like a fine starting point to me.
> 
>   - Panu -
> 

[dpdk-dev] [PATCH v8 3/3] i40e: add floating VEB extension support

[Zhe Tao]

To enable this feature, the user should pass a devargs parameter to the EAL
like "-w 84:00.0,enable_floating=1", and the application will make sure the PMD
will use the floating VEB feature for all the VFs created by this PF device.

Also you can specifiy which VF need to connect to this floating veb using
"floating_bitmap", every bit corresponding to one VF (e.g. bitn for VFn).
Like "-w 84:00.0,enable_floating=1,floating_bitmap=1", means only the VF0 
connect
to the floating VEB, VF1 connect to the legacy VEB.

Signed-off-by: Zhe Tao 
---
 doc/guides/nics/i40e.rst   |  5 +++-
 drivers/net/i40e/i40e_ethdev.c | 56 --
 drivers/net/i40e/i40e_ethdev.h |  1 +
 drivers/net/i40e/i40e_pf.c |  3 ++-
 4 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index 49a0598..0919a96 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -372,4 +372,7 @@ FVL can support floating VEB feature.
 To enable this feature, the user should pass a devargs parameter to the EAL
 like "-w 84:00.0,enable_floating=1", and the application will make sure the PMD
 will use the floating VEB feature for all the VFs created by this PF device.
-
+Also you can specify which VF need to connect to this floating veb using
+"floating_bitmap", every bit corresponding to one VF (e.g. bitn for VFn).
+Like "-w 84:00.0,enable_floating=1,floating_bitmap=1", means only the VF0 
connect
+to the floating VEB, VF1 connect to the legacy VEB.
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 8859936..39da1e0 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -750,6 +750,52 @@ i40e_add_tx_flow_control_drop_filter(struct i40e_pf *pf)
  " frames from VSIs.");
 }

+static int i40e_check_fbitmap_handler(__rte_unused const char *key,
+ const char *value,
+ void *opaque)
+{
+   errno = 0;
+   *(uint16_t *)opaque = strtoul(value, NULL, 0);
+   if (errno)
+   return -1;
+   return 0;
+}
+
+static uint16_t i40e_check_fbitmap(struct rte_devargs *devargs,
+  uint16_t floating)
+{
+   struct rte_kvargs *kvlist;
+   const char *floating_bitmap = "floating_bitmap";
+   /* default value for vf floating bitmap is -1 */
+   uint16_t vf_fbitmap = (uint16_t)-1;
+   uint16_t new_vf_fbitmap;
+
+   if (floating == false)
+   return 0;
+
+   if (devargs == NULL)
+   return vf_fbitmap;
+
+   kvlist = rte_kvargs_parse(devargs->args, NULL);
+   if (kvlist == NULL)
+   return vf_fbitmap;
+
+   if (!rte_kvargs_count(kvlist, floating_bitmap)) {
+   rte_kvargs_free(kvlist);
+   return vf_fbitmap;
+   }
+   /* Floating is enabled when there's key-value pair: enable_floating=1 */
+   if (rte_kvargs_process(kvlist, floating_bitmap,
+  i40e_check_fbitmap_handler,
+  _vf_fbitmap) < 0) {
+   rte_kvargs_free(kvlist);
+   return vf_fbitmap;
+   }
+   rte_kvargs_free(kvlist);
+
+   return new_vf_fbitmap;
+}
+
 static int i40e_check_floating_handler(__rte_unused const char *key,
   const char *value,
   __rte_unused void *opaque)
@@ -884,8 +930,11 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
/* Need the special FW version support floating VEB */
if (hw->aq.fw_maj_ver >= FLOATING_FW_MAJ) {
pf->floating = i40e_check_floating(pci_dev->devargs);
+   pf->vf_fbitmap = i40e_check_fbitmap(pci_dev->devargs,
+   pf->floating);
} else {
pf->floating = false;
+   pf->vf_fbitmap = 0;
}
/* Clear PXE mode */
i40e_clear_pxe_mode(hw);
@@ -3855,6 +3904,7 @@ i40e_vsi_release(struct i40e_vsi *vsi)
struct i40e_vsi_list *vsi_list;
int ret;
struct i40e_mac_filter *f;
+   uint16_t user_param = vsi->user_param;

if (!vsi)
return I40E_SUCCESS;
@@ -3886,7 +3936,8 @@ i40e_vsi_release(struct i40e_vsi *vsi)
rte_free(f);

if (vsi->type != I40E_VSI_MAIN &&
-   ((vsi->type != I40E_VSI_SRIOV) || !pf->floating)) {
+   ((vsi->type != I40E_VSI_SRIOV) ||
+   !(pf->vf_fbitmap && 1 << user_param))) {
/* Remove vsi from parent's sibling list */
if (vsi->parent_vsi == NULL || vsi->parent_vsi->veb == NULL) {
PMD_DRV_LOG(ERR, "VSI's parent VSI is NULL");
@@ -3901,7 +3952,8 @@ i40e_vsi_release(struct i40e_vsi *vsi)
PMD_DRV_LOG(ERR, "Failed to delete element");
}

-   if ((vsi->type == 

[dpdk-dev] [PATCH v8 2/3] i40e: Add floating VEB support in i40e

[Zhe Tao]

This patch add the support for floating VEB in i40e.
All the VFs VSIs can decide whether to connect to the legacy VEB/VEPA or
the floating VEB. When connect to the floating VEB a new floating VEB is
created. Now all the VFs need to connect to floating VEB or legacy VEB,
cannot connect to both of them. The PF and VMDQ,FD VSIs still connect to
the old legacy VEB/VEPA.

All the VEB/VEPA concepts are not specific for FVL, they are defined in the
802.1Qbg spec.

Now the floating VEB feature is only avaiable in the specific version of FW.

Signed-off-by: Zhe Tao 
---
 doc/guides/nics/i40e.rst   |   7 +++
 doc/guides/rel_notes/release_16_07.rst |   6 ++
 drivers/net/i40e/i40e_ethdev.c | 109 ++---
 drivers/net/i40e/i40e_ethdev.h |   2 +
 drivers/net/i40e/i40e_pf.c |  11 +++-
 5 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index 934eb02..49a0598 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -366,3 +366,10 @@ Delete all flow director rules on a port:

testpmd> flush_flow_director 0

+Floating VEB
+~
+FVL can support floating VEB feature.
+To enable this feature, the user should pass a devargs parameter to the EAL
+like "-w 84:00.0,enable_floating=1", and the application will make sure the PMD
+will use the floating VEB feature for all the VFs created by this PF device.
+
diff --git a/doc/guides/rel_notes/release_16_07.rst 
b/doc/guides/rel_notes/release_16_07.rst
index 30e78d4..8485b08 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -47,6 +47,12 @@ New Features
   * Dropped specific Xen Dom0 code.
   * Dropped specific anonymous mempool code in testpmd.

+* **Added floating VEB support for i40e PF driver.**
+
+  Now VFs for i40e can connect to the floating VEB.
+  With this feature, VFs can communicate with each other, but cannot access
+  outside network. When PF is down, and VFs can still forward pkts between each
+  other.

 Resolved Issues
 ---
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index e558c63..8859936 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3762,21 +3762,27 @@ i40e_veb_release(struct i40e_veb *veb)
struct i40e_vsi *vsi;
struct i40e_hw *hw;

-   if (veb == NULL || veb->associate_vsi == NULL)
+   if (veb == NULL)
return -EINVAL;

if (!TAILQ_EMPTY(>head)) {
PMD_DRV_LOG(ERR, "VEB still has VSI attached, can't remove");
return -EACCES;
}
+   /* associate_vsi field is NULL for floating VEB */
+   if (veb->associate_vsi != NULL) {
+   vsi = veb->associate_vsi;
+   hw = I40E_VSI_TO_HW(vsi);

-   vsi = veb->associate_vsi;
-   hw = I40E_VSI_TO_HW(vsi);
+   vsi->uplink_seid = veb->uplink_seid;
+   vsi->veb = NULL;
+   } else {
+   veb->associate_pf->main_vsi->floating_veb = NULL;
+   hw = I40E_VSI_TO_HW(veb->associate_pf->main_vsi);
+   }

-   vsi->uplink_seid = veb->uplink_seid;
i40e_aq_delete_element(hw, veb->seid, NULL);
rte_free(veb);
-   vsi->veb = NULL;
return I40E_SUCCESS;
 }

@@ -3788,9 +3794,9 @@ i40e_veb_setup(struct i40e_pf *pf, struct i40e_vsi *vsi)
int ret;
struct i40e_hw *hw;

-   if (NULL == pf || vsi == NULL) {
+   if (NULL == pf) {
PMD_DRV_LOG(ERR, "veb setup failed, "
-   "associated VSI shouldn't null");
+   "associated PF shouldn't null");
return NULL;
}
hw = I40E_PF_TO_HW(pf);
@@ -3802,11 +3808,19 @@ i40e_veb_setup(struct i40e_pf *pf, struct i40e_vsi *vsi)
}

veb->associate_vsi = vsi;
+   veb->associate_pf = pf;
TAILQ_INIT(>head);
-   veb->uplink_seid = vsi->uplink_seid;
+   veb->uplink_seid = vsi ? vsi->uplink_seid : 0;

-   ret = i40e_aq_add_veb(hw, veb->uplink_seid, vsi->seid,
-   I40E_DEFAULT_TCMAP, false, >seid, false, NULL);
+   /* create floating veb if vsi is NULL */
+   if (vsi != NULL) {
+   ret = i40e_aq_add_veb(hw, veb->uplink_seid, vsi->seid,
+ I40E_DEFAULT_TCMAP, false,
+ >seid, false, NULL);
+   } else {
+   ret = i40e_aq_add_veb(hw, 0, 0, I40E_DEFAULT_TCMAP,
+ true, >seid, false, NULL);
+   }

if (ret != I40E_SUCCESS) {
PMD_DRV_LOG(ERR, "Add veb failed, aq_err: %d",
@@ -3822,10 +3836,10 @@ i40e_veb_setup(struct i40e_pf *pf, struct i40e_vsi *vsi)
hw->aq.asq_last_status);
goto fail;
}
-
/* Get VEB bandwidth, to be implemented */
/* Now associated 

[dpdk-dev] [PATCH v8 1/3] i40e: support floating VEB config

[Zhe Tao]

Add the new floating related argument option in the devarg.
Using this parameter, all the samples can decide whether to use legacy VEB/VEPA
or floating VEB.
To enable this feature, the user should pass a devargs parameter to the EAL
like "-w 84:00.0,enable_floating=1", and the application will make sure the PMD
will use the floating VEB feature for all the VFs created by this PF device.

Signed-off-by: Zhe Tao 
---
 drivers/net/i40e/i40e_ethdev.c | 44 ++
 drivers/net/i40e/i40e_ethdev.h |  6 ++
 2 files changed, 50 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 24777d5..e558c63 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -750,6 +750,44 @@ i40e_add_tx_flow_control_drop_filter(struct i40e_pf *pf)
  " frames from VSIs.");
 }

+static int i40e_check_floating_handler(__rte_unused const char *key,
+  const char *value,
+  __rte_unused void *opaque)
+{
+   if (strcmp(value, "1"))
+   return -1;
+
+   return 0;
+}
+
+static int
+i40e_check_floating(struct rte_devargs *devargs)
+{
+   struct rte_kvargs *kvlist;
+   const char *floating_key = "enable_floating";
+
+   if (devargs == NULL)
+   return 0;
+
+   kvlist = rte_kvargs_parse(devargs->args, NULL);
+   if (kvlist == NULL)
+   return 0;
+
+   if (!rte_kvargs_count(kvlist, floating_key)) {
+   rte_kvargs_free(kvlist);
+   return 0;
+   }
+   /* Floating is enabled when there's key-value pair: enable_floating=1 */
+   if (rte_kvargs_process(kvlist, floating_key,
+  i40e_check_floating_handler, NULL) < 0) {
+   rte_kvargs_free(kvlist);
+   return 0;
+   }
+   rte_kvargs_free(kvlist);
+
+   return 1;
+}
+
 static int
 eth_i40e_dev_init(struct rte_eth_dev *dev)
 {
@@ -843,6 +881,12 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 ((hw->nvm.version >> 4) & 0xff),
 (hw->nvm.version & 0xf), hw->nvm.eetrack);

+   /* Need the special FW version support floating VEB */
+   if (hw->aq.fw_maj_ver >= FLOATING_FW_MAJ) {
+   pf->floating = i40e_check_floating(pci_dev->devargs);
+   } else {
+   pf->floating = false;
+   }
/* Clear PXE mode */
i40e_clear_pxe_mode(hw);

diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index cfd2399..8297c5f 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -36,6 +36,7 @@

 #include 
 #include 
+#include 

 #define I40E_VLAN_TAG_SIZE4

@@ -171,6 +172,10 @@ enum i40e_flxpld_layer_idx {
 #define I40E_QUEUE_ITR_INTERVAL_DEFAULT 32 /* 32 us */
 #define I40E_QUEUE_ITR_INTERVAL_MAX 8160 /* 8160 us */

+/* Special FW support this floating VEB feature */
+#define FLOATING_FW_MAJ 5
+#define FLOATING_FW_MIN 0
+
 struct i40e_adapter;

 /**
@@ -450,6 +455,7 @@ struct i40e_pf {
struct i40e_fc_conf fc_conf; /* Flow control conf */
struct i40e_mirror_rule_list mirror_list;
uint16_t nb_mirror_rule;   /* The number of mirror rules */
+   uint16_t floating; /* The flag to use the floating VEB */
 };

 enum pending_msg {
-- 
2.1.4

[dpdk-dev] [PATCH v8 0/3] i40e: Add floating VEB support for i40e

[Zhe Tao]

This patch-set add the support for floating VEB in i40e.
All the VFs VSIs can decide whether to connect to the legacy VEB/VEPA or
the floating VEB. When connect to the floating VEB a new floating VEB is
created. Now all the VFs need to connect to floating VEB or legacy VEB,
cannot connect to both of them. The PF and VMDQ,FD VSIs connect to
the old legacy VEB/VEPA.

All the VEB/VEPA concepts are not specific for FVL, they are defined in the
802.1Qbg spec.

This floating VEB only take effects on the specific version F/W.

Zhe Tao (3):
  Support floating VEB config
  Add floating VEB support in i40e
  Add floating VEB extention support for i40e

 doc/guides/nics/i40e.rst   |  10 ++
 doc/guides/rel_notes/release_16_07.rst |   6 +
 drivers/net/i40e/i40e_ethdev.c | 205 +
 drivers/net/i40e/i40e_ethdev.h |   9 ++
 drivers/net/i40e/i40e_pf.c |  12 +-
 5 files changed, 219 insertions(+), 23 deletions(-)

V2: Added the release notes and changed commit log. 
V3: Changed the VSI release operation. 
V4: Added the FW version check otherwise it will cause the segment fault.
V5: Edited the code for new share code APIs
V6: Changed the floating VEB configuration method 
V7: Added global reset for i40e 
V7: removed global reset and added floating VEB extension support 

-- 
2.1.4

[dpdk-dev] [PATCH] e1000: fix build with clang

[Lu, Wenzhuo]

Hi,


> -Original Message-
> From: Hiroyuki Mikita [mailto:h.mikita89 at gmail.com]
> Sent: Tuesday, May 24, 2016 10:48 PM
> To: Lu, Wenzhuo
> Cc: dev at dpdk.org
> Subject: [PATCH] e1000: fix build with clang
> 
> GCC_VERSION is empty in case of clang:
>   /bin/sh: line 0: test: -ge: unary operator expected
> 
> It is the same issue as http://dpdk.org/dev/patchwork/patch/5994/
> 
> Fixes: 366113dbfb69 ("e1000: suppress misleading indentation warning")
> 
> Signed-off-by: Hiroyuki Mikita 
Acked-by: Wenzhuo Lu 

[dpdk-dev] [PATCH] virtio: use volatile to get used->idx in the loop

[Huawei Xie]

There is no external function call or any barrier in the loop,
the used->idx would only be retrieved once.

Signed-off-by: Huawei Xie 
---
 drivers/net/virtio/virtio_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c 
b/drivers/net/virtio/virtio_ethdev.c
index c3fb628..f6d6305 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -204,7 +204,8 @@ virtio_send_command(struct virtqueue *vq, struct 
virtio_pmd_ctrl *ctrl,
usleep(100);
}

-   while (vq->vq_used_cons_idx != vq->vq_ring.used->idx) {
+   while (vq->vq_used_cons_idx !=
+  *((volatile uint16_t *)(>vq_ring.used->idx))) {
uint32_t idx, desc_idx, used_idx;
struct vring_used_elem *uep;

-- 
1.8.1.4

[dpdk-dev] [PATCH] e1000: fix build with clang

[Hiroyuki Mikita]

GCC_VERSION is empty in case of clang:
/bin/sh: line 0: test: -ge: unary operator expected

It is the same issue as http://dpdk.org/dev/patchwork/patch/5994/

Fixes: 366113dbfb69 ("e1000: suppress misleading indentation warning")

Signed-off-by: Hiroyuki Mikita 
---
 drivers/net/e1000/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/e1000/Makefile b/drivers/net/e1000/Makefile
index f4879e6..d580dea 100644
--- a/drivers/net/e1000/Makefile
+++ b/drivers/net/e1000/Makefile
@@ -50,11 +50,11 @@ ifeq ($(CC), icc)
 CFLAGS_BASE_DRIVER = -wd177 -wd181 -wd188 -wd869 -wd2259
 else
 #
-# CFLAGS for gcc
+# CFLAGS for gcc/clang
 #
 CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter
 CFLAGS_BASE_DRIVER += -Wno-unused-variable
-ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1)
+ifeq ($(shell test $(CC) = gcc && test $(GCC_VERSION) -ge 60 && echo 1), 1)
 CFLAGS_BASE_DRIVER += -Wno-misleading-indentation
 endif
 endif
-- 
1.9.1