[dpdk-dev] [PATCH v12 0/6] add Tx preparation

2016-11-30 Thread John Daley (johndale)
Hi,
-john

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Monday, November 28, 2016 3:03 AM
> To: dev at dpdk.org; Rahul Lakkireddy ;
> Stephen Hurd ; Jan Medala
> ; Jakub Palider ; John Daley
> (johndale) ; Adrien Mazarguil
> ; Alejandro Lucero
> ; Harish Patil
> ; Rasesh Mody ; Jerin
> Jacob ; Yuanhan Liu
> ; Yong Wang 
> Cc: Tomasz Kulasek ;
> konstantin.ananyev at intel.com; olivier.matz at 6wind.com
> Subject: Re: [dpdk-dev] [PATCH v12 0/6] add Tx preparation
> 
> We need attention of every PMD developers on this thread.
> 
> Reminder of what Konstantin suggested:
> "
> - if the PMD supports TX offloads AND
> - if to be able use any of these offloads the upper layer SW would have to:
> * modify the contents of the packet OR
> * obey HW specific restrictions
> then it is a PMD developer responsibility to provide tx_prep() that would
> implement expected modifications of the packet contents and restriction
> checks.
> Otherwise, tx_prep() implementation is not required and can be safely set to
> NULL.
> "
> 
> I copy/paste also my previous conclusion:
> 
> Before txprep, there is only one API: the application must prepare the
> packets checksum itself (get_psd_sum in testpmd).
> With txprep, the application have 2 choices: keep doing the job itself or call
> txprep which calls a PMD-specific function.
> The question is: does non-Intel drivers need a checksum preparation for
> TSO?
> Will it behave well if txprep does nothing in these drivers?
> 
> When looking at the code, most of drivers handle the TSO flags.
> But it is hard to know whether they rely on the pseudo checksum or not.
> 
> git grep -l 'PKT_TX_UDP_CKSUM\|PKT_TX_TCP_CKSUM\|PKT_TX_TCP_SEG'
> drivers/net/
> 
> drivers/net/bnxt/bnxt_txr.c
> drivers/net/cxgbe/sge.c
> drivers/net/e1000/em_rxtx.c
> drivers/net/e1000/igb_rxtx.c
> drivers/net/ena/ena_ethdev.c
> drivers/net/enic/enic_rxtx.c
> drivers/net/fm10k/fm10k_rxtx.c
> drivers/net/i40e/i40e_rxtx.c
> drivers/net/ixgbe/ixgbe_rxtx.c
> drivers/net/mlx4/mlx4.c
> drivers/net/mlx5/mlx5_rxtx.c
> drivers/net/nfp/nfp_net.c
> drivers/net/qede/qede_rxtx.c
> drivers/net/thunderx/nicvf_rxtx.c
> drivers/net/virtio/virtio_rxtx.c
> drivers/net/vmxnet3/vmxnet3_rxtx.c
> 
> Please, we need a comment for each driver saying "it is OK, we do not need
> any checksum preparation for TSO"
> or
> "yes we have to implement tx_prepare or TSO will not work in this mode"

I like the idea of tx prep since it should make for cleaner apps.

For enic, I believe the answer is " it is OK, we do not need any checksum 
preparation".

Prior to now, it was necessary to set IP checksum to 0 and put in a TCP/UDP 
pseudo header. But there is a hardware overwrite of checksums option which 
makes preparation in software unnecessary and it is testing out well so far. I 
plan to enable it in 17.02. TSO is also being enabled for 17.02 and it does not 
look like any prep is required. So I'm going with " txprep NULL pointer is OK 
for enic", but may have to change my mind if something comes up in testing.

-john


[dpdk-dev] [opnfv-tech-discuss][apex][ovsnfv]Problem showed up with OVS/DPDK with Cisco VIC adapter

2016-10-20 Thread John Daley (johndale)
Hi,
Please see inline.
Thanks,
john

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas F Herbert
> Sent: Tuesday, October 18, 2016 1:35 PM
> To: dev at dpdk.org
> Cc: Keith Burns ; Edward Warnicke
> ; opnfv-tech-discuss at lists.opnfv.org
> Subject: [dpdk-dev] [opnfv-tech-discuss][apex][ovsnfv]Problem showed up
> with OVS/DPDK with Cisco VIC adapter
> 
> All:
> 
> This is not necessarily related to VPP but rather to OVS/DPDK.
> In OPNFV we found the following problem when using UCS NIC.
> The UCS fabric seems to set a VLAN tag on untagged packets.
> Any thoughts from DPDK and VPP folks would be appreciated.
> 
In a UCS fabric, all frames between the VIC and the Fabric Interconnect will be 
tagged. This is required to carry both VLAN information and, being a converged 
adapter supporting both Ethernet and FCoE, traffic class. For non-UCS fabric 
deployments, there is currently no way to turn off egress priority tagging on 
the VIC adapter. If a packet being sent from DPDK to the enic PMD is priority 
tagged (VLAN=0) or has no VLAN tag, the default VLAN tag (as set up in 
CIMC/UCSM manager) will be inserted.  This should only be an issue with 
C-series UCS servers connected point to point or through a switch that can't 
cope with priority tags. Is that the case here?

> ...
> --
> *Thomas F Herbert*
> SDN Group
> Office of Technology
> *Red Hat*


[dpdk-dev] [PATCH 4/4] net/enic: extend fdir support for 1300 series adapters

2016-10-11 Thread John Daley (johndale)


> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yigit at intel.com]
> Sent: Tuesday, October 11, 2016 2:22 AM
> To: John Daley (johndale) ;
> bruce.richardson at intel.com
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 4/4] net/enic: extend fdir support for 1300
> series adapters
> 
> Hi John,
> 
> On 9/29/2016 9:56 PM, John Daley wrote:
> > 1300 series Cisco adapter firmware version 2.0(13) for UCS C-series
> > servers and 3.1(2) for blade servers supports more filtering
> > capabilities. The feature can be enabled via Cisco CIMC or USCM with
> > the 'advanced filters' radio button. When enabled, the these
> > additional flow director modes are available:
> > RTE_ETH_FLOW_NONFRAG_IPV4_OTHER
> > RTE_ETH_FLOW_NONFRAG_IPV4_SCTP
> > RTE_ETH_FLOW_NONFRAG_IPV6_UDP
> > RTE_ETH_FLOW_NONFRAG_IPV6_TCP
> > RTE_ETH_FLOW_NONFRAG_IPV6_SCTP
> > RTE_ETH_FLOW_NONFRAG_IPV6_OTHER
> >
> > Changes:
> > - Detect and set an 'advanced filters' flag dependent on the adapter
> >   capability.
> > - Implement RTE_ETH_FILTER_INFO filter op to return the flow types
> >   available dependent on whether advanced filters are enabled.
> > - Use a function pointer to select how filters are added to the adapter:
> >   copy_fltr_v1() for older firmware/adapters or copy_fltr_v2() for
> >   adapters which support advanced filters.
> > - Apply fdir global masks to filters when in advanced filter mode.
> > - Update documentation.
> >
> > Signed-off-by: John Daley 
> > Reviewed-by: Nelson Escobar 
> > ---
> 
> <...>
> 
> >
> > +void enic_fdir_info_get(struct enic *enic, struct rte_eth_fdir_info
> > +*info) {
> > +   info->mode = enic->fdir.modes;
> 
> This cause a icc build error:
> .../drivers/net/enic/enic_clsf.c(77):
> error #188: enumerated type mixed with another type
> info->mode = enic->fdir.modes;
>^
> 
> Just casting to the enum fixes it:
> +   info->mode = (enum rte_fdir_mode)enic->fdir.modes;
> 
> Since the modification is trivial, if you agree with the change, we can apply 
> it
> without needing a new version of the patch?
> 

Looks good, thank you and sorry for the trouble.
-john

> <...>


[dpdk-dev] [RFC] Generic flow director/filtering/classification API

2016-08-19 Thread John Daley (johndale)
Hi, this is an old thread, but I'll reply to this instead of the RFC v2 since 
there is more context here.
Thanks for pushing the new api forward Adrien.
-john daley

> > >>> - Match range of Physical Functions (PFs) on the NIC in a single rule
> > >>>   via masks. For ex: match all traffic coming on several PFs.
> > >>
> > >> The PF and VF pattern items assume there is a single PF associated
> > >> with a DPDK port. VFs are identified with an ID. I basically took
> > >> the same definitions as the existing filter types, perhaps this is
> > >> not enough for Chelsio adapters.
> > >>
> > >> Do you expose more than one PF for a DPDK port?

The Cisco VIC can support multiple PFs per Ethernet port.  These are called 
virtual-nics (VNICs). It would be nice to be able to redirect matched Rx 
packets to another queue on another VNIC.

> > >>
> > >> Anyway, I'd suggest the same approach as above, automatic
> > >> aggregation of rules for performance reasons, otherwise new or
> > >> updated PF/VF pattern items, in which case it would be great if you
> > >> could provide ideal structure definitions for this use case.
> > >>
> > >
> > > In Chelsio hardware, all the ports of a device are exposed via
> > > single PF4. There could be many VFs attached to a PF.  Physical NIC
> > > functions are operational on PF4, while VFs can be attached to PFs 0-3.
> > > So, Chelsio hardware doesn't remain tied on a PF-to-Port, one-to-one
> > > mapping assumption.
> > >
> > > There already seems to be a PF meta-item, but it doesn't seem to
> > > accept any "spec" and "mask" field.  Similarly, the VF meta-item
> > > doesn't seem to accept a "mask" field.  We could probably enable
> > > these fields in the PF and VF meta-items to allow configuration.
> >
I would like to see an ID property added to the PF action meta-item, where 
perhaps a BDF can be specified. This would potentially allow matched Rx packets 
to be redirected to another VNIC and could be paired with the QUEUE action 
meta-item to redirect to a specific queue on a VNIC. The PF ID property set to 
0 would have the current specified behavior or redirecting to the current PF. 
Is something like this possible?



[dpdk-dev] [PATCH v2] doc: announce ABI change for mbuf structure

2016-07-28 Thread John Daley (johndale)
> 
> For 16.11, the mbuf structure will be modified implying ABI breakage.
> Some discussions already took place here:
> http://www.dpdk.org/dev/patchwork/patch/12878/
> 
> Signed-off-by: Olivier Matz 
> ---

Acked-by: John Daley 

Also, definitely +1 on trying to get m->next into the first cache line.



[dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling

2016-07-11 Thread John Daley (johndale)


> -Original Message-
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Monday, July 11, 2016 3:04 AM
> To: John Daley (johndale) ; dev at dpdk.org
> Cc: bruce.richardson at intel.com
> Subject: Re: [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference
> count before recycling
> 
> Hi John,
> 
> On 07/09/2016 12:22 AM, John Daley wrote:
> > In the Tx cleanup function, the reference count in mbufs to be
> > returned to the pool should to be decremented before they are
> > returned. Decrementing is not done by rte_mempool_put_bulk() so it
> > must be done separately using __rte_pktmbuf_prefree_seg().
> > If decrementing does not result in a 0 reference count the mbuf is not
> > returned to the pool and whatever has the last reference is
> > responsible for freeing.
> >
> > Fixes: 36935afbc53c ("net/enic: refactor Tx mbuf recycling")
> > Reviewed-by: Nelson Escobar 
> > Signed-off-by: John Daley 
> > ---
> > Since reference counts are set to 0 when mbufs are reallocated from
> > the pool, and sending packets with reference count not equal to 1 is
> > probably an application error, this patch may not be critical. But a
> > debug ASSERT caught it and it would be nice to have it fixed in 16.07.
> 
> Sending a packet with refcnt != 1 is not an error. It can happen when using
> mbuf clones. So indeed it would be better to have in 16.07.
> 
> For the same reason, I also wonder if enic_free_wq_buf() should also be
> updated with:
> 
> -   rte_mempool_put(mbuf->pool, mbuf);
> +   rte_pktmbuf_free(mbuf);
That is a very good point, thank you. I'll use rte_pktmubf_free_seg(mbuf) 
though, since we are walking an array of all mbuf segments. V2 coming 
momentarily.
-john
> 
> 
> Regards,
> Olivier


[dpdk-dev] [PATCH] net/enic: remove useless assert macro

2016-07-08 Thread John Daley (johndale)

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, July 06, 2016 8:25 AM
> To: John Daley (johndale) ; Nelson Escobar
> (neescoba) 
> Cc: dev at dpdk.org
> Subject: [PATCH] net/enic: remove useless assert macro
> 
> The macro ENIC_ASSERT does the same thing as RTE_ASSERT, thus it can be
> removed.
> 
> Signed-off-by: Thomas Monjalon 
Acked-by: John Daley 



[dpdk-dev] [PATCH 2/4] enic: set the max allowed MTU for the NIC

2016-06-24 Thread John Daley (johndale)
Hi Bruce,

> > * What was the MTU set to by default before this patch is applied? Was
> > it just set to 1518 or something else?
> > * What happens, if anything, if buffers bigger than the MTU size are sent
> down?
> This is obviously referring to buffers bigger than MTU on TX. There is also 
> the
> question of what happens if buffer sizes smaller than MTU are provided on
> RX.

I think I answered all your questions in the revised commit messages of the v2 
patchset (and then some) except this last one. Enic doesn't do any checking on 
Rx that buffers are greater than the MTU since it would affect performance. 
However if a packet is bigger than a buffer and Rx scatter is disabled, the 
packet will be dropped and 'imissed' incremented.

Thanks,
Johnd



[dpdk-dev] unchecked return value in enic driver

2016-06-20 Thread John Daley (johndale)


> -Original Message-
> From: Kerlin, MarcinX [mailto:marcinx.kerlin at intel.com]
> Sent: Monday, June 20, 2016 4:12 AM
> To: John Daley (johndale) ; Nelson Escobar
> (neescoba) 
> Cc: 'dev at dpdk.org' 
> Subject: RE: unchecked return value in enic driver
> 
> Hi John and Nelson,
> 
> > -Original Message-
> > From: Kerlin, MarcinX
> > Sent: Monday, June 13, 2016 1:18 PM
> > To: johndale at cisco.com; neescoba at cisco.com
> > Cc: dev at dpdk.org
> > Subject: unchecked return value in enic driver
> >
> > Hi John and Nelson,
> >
> > I have a question regarding Coverity defects:
> >
> > File: /drivers/net/enic/enic_ethdev.c
> > Line: 379
> >
> > CID 13197: Unchecked return value
> > (CHECKED_RETURN)1.?check_return:?Calling?rte_atomic64_cmpset?without
> > checking return value (as is done elsewhere 15 out of 17 times)
> >
> > Can I mark this error as "False Positive" in Coverity Classification ? 
> > reason:
> > 1. Function returns a void type so change the return type to int
> > requires changes all drivers 2. rte_atomic64_cmpset is at the end of
> > function so nonsense added a return
> >
> > What is your opinion?

I agree with marking it false positive for the reason you mention. 
Thanks!
John

> 
> I marked this Coverity as false-positive with an explanation. If in your 
> opinion
> it is not ok, you can reopen/change/fix it.
> 
> >
> > Regards,
> > Marcin


[dpdk-dev] [PATCH v3 07/13] enic: use Tx completion messages instead of descriptors

2016-06-10 Thread John Daley (johndale)


> -Original Message-
> From: Bruce Richardson [mailto:bruce.richardson at intel.com]
> Sent: Friday, June 10, 2016 2:18 PM
> To: John Daley (johndale) 
> Cc: dev at dpdk.org; bruce.richarsdon at intel.com
> Subject: Re: [dpdk-dev] [PATCH v3 07/13] enic: use Tx completion messages
> instead of descriptors
> 
> On Thu, Jun 02, 2016 at 05:22:51PM -0700, John Daley wrote:
> > The NIC can either DMA a separate completion message for each
> > completed send or periodically just DMA an index of the last completed
> > send. Switch to the second method which improves cache locality and
> > performance.
> >
> > Signed-off-by: John Daley 
> 
> Can you perhaps send me an updated wording for this commit message as
> the title and commit message conflict. The title says to use completion
> messages not descriptors, while the body talks about moving away from a
> completion message way of working.
> Is the former method a descriptor writeback method, while the latter a head
> pointer writeback? If so, I think the title could be:
> 
> "enic: use Tx head pointer not descriptor writeback"
> 
> or something similar.
> 
> Again, if you send on the updated commit text, I'll just update it on apply. 
> I'd
> ideally like to get this patchset pushed to next-net first thing Monday.

Ok, I agree that it is confusing.
We moved from having the hardware send a completion descriptor for every packet 
to having it send the index of the last completed packet every once in a while. 
We can use the word 'index' and 'message' to describe the 2 methods and drop 
the word 'descriptor'. Here is a suggestion:

enic: use Tx completion index instead of completion messages

The NIC can either DMA a separate completion message for each completed send or 
periodically just DMA an index of the last completed send. Switch to the latter 
method which improves cache locality and performance.

Thank you,
John
> 
> /Bruce



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

2016-05-26 Thread John Daley (johndale)
John,
As just discussed, what you suggest was considered but there are other apps 
depending a different behavior of the flag, so we thought the best thing to do 
is deprecate it. That is part of what Olivier's patch discussed in 
http://www.dpdk.org/ml/archives/dev/2016-May/038786.html does.  Adding a new 
ptype for VLAN tagged packets after the patch is accepted would allow VPP to 
check if the ptype is supported by the PMD and if so, use it to determine if 
the delivered packet actually has a VLAN tag in it. No need to know if 
stripping is enabled/disabled. If the ptype is not supported,  the app would 
have check the packet in SW.

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of John Lo (loj)
> Sent: Thursday, May 26, 2016 11:11 AM
> To: Ananyev, Konstantin ; Wiles, Keith
> ; Chandrasekar Kannan 
> Cc: vpp-dev ; Zhang, Helin ;
> dev at dpdk.org
> Subject: Re: [dpdk-dev] [vpp-dev] VLAN packets dropped... ?
> 
> Hi Konstantin,
> 
> Thanks for the link to DPDK discussion wrt this VLAN offload flag
> PKT_RX_VLAN_PKT.  It seems the proposal was to deprecate
> PKT_RX_VLAN_PKT  and introduce two new flags PKT_RX_VLAN_STRIPPED
> and PKT_RX_QINQ_STRIPPED.
> 
> It would be a really good idea to keep PKT_RX_VLAN_PKT to indicate at least
> one VLAN tag is present on the packet.  For the case of i40e where its HW
> cannot detect VLAN tag if strip is not enabled, it should be reasonable for 
> the
> i40e DPDK driver software to make a check and set this flag. I would think the
> overhead to check the 1st ethertype field in the MAC header against dot1q
> or dot1ad values in software be pretty minimal.
> 
> Regards,
> John
> 
> 
> -Original Message-
> From: Ananyev, Konstantin [mailto:konstantin.ananyev at intel.com]
> Sent: Wednesday, May 25, 2016 3:23 PM
> To: John Lo (loj); Wiles, Keith; Chandrasekar Kannan
> Cc: vpp-dev; Zhang, Helin; dev at dpdk.org
> Subject: RE: [vpp-dev] VLAN packets dropped... ?
> 
> 
> > 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 

[dpdk-dev] [RFC] mbuf: new flag when vlan is stripped

2016-05-12 Thread John Daley (johndale)
Hi Olivier,

> ...
> This is a draft patch that implements what was previously discussed, except
> the packet_type, which does not exist for vlan today (and I think it is not
> mandatory for now, let's do it in another patch).
> 
> After doing this patch, it appeared that ixgbe was the only driver that had a
> different behavior for the PKT_RX_VLAN_PKT flag. An alternative to this
> patch would be to only change the behavior of the ixgbe driver, and just
> document better document the PKT_RX_VLAN_PKT flags in rte_mbuf.h
> without adding new flags. I think this is a better option.
> 
> Comments are welcome.
>
There are applications depending on the current behavior of PKT_RX_VLAN_PKT as 
confusing as it may be.  I know of one that has VLAN stripping disabled and 
uses the flag to determine if the packet delivered to the app has a VLAN tag. 
This is actually how the flag behaves for ixgbe, and they patched enic and 
maybe other drivers to act accordingly. To avoid breaking the app (and any 
others like it), I think we should keep the flag behavior the same and add the 
new flag PKT_RX_VLAN_STRIPPED .

We should follow on with the new packet type since it enables a nice 
performance improvement by not forcing apps to break open the packet just to 
see if there is a VLAN tag.

Thanks,
john



[dpdk-dev] [RFC] mbuf: remove unused rx error flags

2016-05-12 Thread John Daley (johndale)
Hi,

> -Original Message-
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Tuesday, May 10, 2016 1:40 AM
> To: dev at dpdk.org
> Cc: konstantin.ananyev at intel.com; John Daley (johndale)
> ; helin.zhang at intel.com; arnon at qwilt.com;
> rolette at infinite.io
> Subject: [RFC] mbuf: remove unused rx error flags
> 
> Following the discussions from:
> http://dpdk.org/ml/archives/dev/2015-July/021721.html
> http://dpdk.org/ml/archives/dev/2016-April/038143.html
> 
> The value of these flags is 0, making them useless. Today, no example
> application checks them on RX, and only few drivers sets them, and silently
> give wrong packets to the application, which should not happen.
> 
> This patch removes the unused flags from rte_mbuf, and their use in the
> drivers. The enic driver is modified to drop bad packets, but i40e and fm10k
> are kept as they are today and should be fixed to drop bad packets.

Perhaps the change to enic to drop bad packets should be handled as a separate 
patch since it's not strictly related to not removing the use of the flags?

> 
> Fixes: c22265f6 ("mbuf: add new packet flags for i40e")
> Signed-off-by: Olivier Matz 
> ---
> 
> Hi,
> 
> Here is a draft patch that removes the rx mbuf flags, as discussed.
> 
> John, I did not test the patch on the enic driver, so please review it 
> carefully.
> 

The patch works for enic, thanks. There are a few minor changes for performance:
 - put an unlikely in the if (packet_error) conditional
- remove the if (!packet_error) conditional since it will always be true.
Let me know if you would prefer I submit the enic patch separately.

> Comments are welcome.
> 
> Olivier
> 
> 
>  drivers/net/enic/enic_rx.c | 31 ++-
>  drivers/net/fm10k/fm10k_rxtx.c | 16 
>  drivers/net/fm10k/fm10k_rxtx_vec.c |  2 +-
>  drivers/net/i40e/i40e_rxtx.c   | 15 ---
>  lib/librte_mbuf/rte_mbuf.c |  4 
>  lib/librte_mbuf/rte_mbuf.h |  5 +
>  6 files changed, 16 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/net/enic/enic_rx.c b/drivers/net/enic/enic_rx.c index
> b3ad9ea..bad802e 100644
> --- a/drivers/net/enic/enic_rx.c
> +++ b/drivers/net/enic/enic_rx.c
> @@ -144,20 +144,15 @@ enic_cq_rx_desc_n_bytes(struct cq_desc *cqd)  }
> 
>  static inline uint8_t
> -enic_cq_rx_to_pkt_err_flags(struct cq_desc *cqd, uint64_t
> *pkt_err_flags_out)
> +enic_cq_rx_check_err(struct cq_desc *cqd)
>  {
>   struct cq_enet_rq_desc *cqrd = (struct cq_enet_rq_desc *)cqd;
>   uint16_t bwflags;
> - int ret = 0;
> - uint64_t pkt_err_flags = 0;
> 
>   bwflags = enic_cq_rx_desc_bwflags(cqrd);
> - if (unlikely(enic_cq_rx_desc_packet_error(bwflags))) {
> - pkt_err_flags = PKT_RX_MAC_ERR;
> - ret = 1;
> - }
> - *pkt_err_flags_out = pkt_err_flags;
> - return ret;
> + if (unlikely(enic_cq_rx_desc_packet_error(bwflags)))
> + return 1;
> + return 0;
>  }
> 
>  /*
> @@ -253,7 +248,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>   struct enic *enic = vnic_dev_priv(rq->vdev);
>   unsigned int rx_id;
>   struct rte_mbuf *nmb, *rxmb;
> - uint16_t nb_rx = 0;
> + uint16_t nb_rx = 0, nb_err = 0;
>   uint16_t nb_hold;
>   struct vnic_cq *cq;
>   volatile struct cq_desc *cqd_ptr;
> @@ -269,7 +264,6 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>   volatile struct rq_enet_desc *rqd_ptr;
>   dma_addr_t dma_addr;
>   struct cq_desc cqd;
> - uint64_t ol_err_flags;
>   uint8_t packet_error;
> 
>   /* Check for pkts available */
> @@ -293,7 +287,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>   }
> 
>   /* A packet error means descriptor and data are untrusted */
> - packet_error = enic_cq_rx_to_pkt_err_flags(,
> _err_flags);
> + packet_error = enic_cq_rx_check_err();
> 
>   /* Get the mbuf to return and replace with one just allocated
> */
>   rxmb = rq->mbuf_ring[rx_id];
> @@ -318,6 +312,13 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>   rqd_ptr->address = rte_cpu_to_le_64(dma_addr);
>   rqd_ptr->length_type = cpu_to_le16(nmb->buf_len);
> 
> + /* Drop incoming bad packet */
> + if (packet_error) {
> + rte_pktmbuf_free(rxmb);
> + nb_err++;
> + continue;
> +   

[dpdk-dev] removing mbuf error flags

2016-04-29 Thread John Daley (johndale)
Hi,

> -Original Message-
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Friday, April 29, 2016 5:25 AM
> To: dev at dpdk.org; Zhang, Helin 
> Cc: Ananyev, Konstantin ; John Daley
> (johndale) 
> Subject: removing mbuf error flags
> 
> Hi,
> 
> In rte_mbuf.h, some rx flags are set to 0 since a long time since nearly 2
> years. It means nobody use them. They were introduced by the following
> commit:
> 
>   http://dpdk.org/browse/dpdk/commit/?id=c22265f6
> 
> As far as I understand, these flags were introduced to let the application
> know that a received packet is invalid.
> 
> The 2 drivers using them are i40e and enic. But as this flags are 0 today, it
> means that invalid packets are silently given to the application.
> 
> My opinion is that invalid packets should not be given to the application and
> only a statistic counter should be incremented.
> No application check these flags today (in examples, or testpmd).
> 
> I would like to remove these flags.
> Thoughs?

I agree. Enic needs a little work to increment a counter and update internal 
indexes correctly. If you are in a hurry, feel free to 's/PKT_RX_MAC_ERR/0/' in 
enic for now.

-John


[dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled

2016-04-26 Thread John Daley (johndale)
Hi Olivier and Ananyev,

I like the new packet types and how they work the same for VLAN and QINQ. Just 
so I understand your suggestion, X710 (as it seems to work today) would not set 
RTE_PTYPE_L2_ETHER_VLAN  in dev_supported_ptypes_get() because it does not know 
how to determine that packet type in the receive path if stripping is disabled? 
But if stripping was enabled, the application could still trust m->vlan_tci if 
the flag was set?

Re changing the meaning of the PKT_RX_VLAN_PKT flag- I think it could cause 
hard to find errors and confusion. I would rather see the flag deprecated and a 
new one defined. Perhaps the flag could be called PKT_RX_VLAN_STRIPPED*.

Maybe another less elegant but more compatible solution would be just keep the 
Niantic behavior and fix other pmd's to match its behavior. For X710, with vlan 
stripping disabled this might mean looking at each packet's Ethernet type and 
set the flag accordingly.  It might not be too expensive since Ethernet type is 
in the 1st cacheline and hopefully prefetched. Thoughts?

*In the future perhaps another flag could be added called 
PKT_RX_VLAN_TCI_VALID. This may not be the same as PKT_RX_VLAN_STRIPPED- enic 
and maybe some other nics are able to set vlan_tci even when not stripping vlan 
tags and this feature could be exposed with this separate flag.

-john

> -Original Message-
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Monday, April 25, 2016 6:51 AM
> To: Ananyev, Konstantin ; John Daley
> (johndale) ; dev at dpdk.org
> Subject: Re: [dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled
> 
> Hi,
> 
> On 04/25/2016 02:02 PM, Ananyev, Konstantin wrote:
> > Hi John,
> > From rte_mbuf.h:
> > #define PKT_RX_VLAN_PKT  (1ULL << 0)  /**< RX packet is a 802.1q VLAN
> packet. */
> > So yes, in theory it should be set up for vlan packet with both stripping
> on/off.
> > The problem is that (as far as I know) when VLAN stripping is disabled
> > FVL RXD doesn't contain information does that packet contain a VLAN or
> not.
> > Don't really know what is the best option in that case: keep things as
> > it is or change the meaning of the VLAN_PKT flag to indicate is
> mbuf.vlan_tci field is valid or not.
> > Konstantin
> 
> It seems the meaning of the PKT_RX_VLAN_PKT bit depends on the port
> configuration:
> - if vlan stripping is configured, it means VLAN is present in vlan_tci
>   mbuf field
> - if not configured, it means a VLAN is present in the packet
> 
> I don't think this is a good behavior since the application has to know the 
> port
> configuration to properly interpret the meaning of the flag.
> 
> I suggest to change the meaning of this flag to: "vlan was stripped by
> hardware, and vlan tag is now located in m->vlan_tci".
> 
> The same could apply to PKT_RX_QINQ_PKT and m->outer_vlan_tci.
> 
> We could add a new packet_type to tell if the mbuf is a VLAN/QinQ is
> detected in the packet but not stripped.
> 
> Example:
> 
> - vlan stripping is disabled
> 
>   - vlan packet recvd: flags=0, ptype=RTE_PTYPE_L2_ETHER_VLAN
>   - qinq packet recvd: flags=0, ptype=RTE_PTYPE_L2_ETHER_QINQ
> 
> - vlan stripping is enabled
> 
>   - vlan packet recvd: flags=PKT_RX_VLAN_PKT,
> ptype=RTE_PTYPE_L2_ETHER,
> m->vlan_tci=id
>   - qinq packet recvd: flags=PKT_RX_VLAN_PKT|PKT_RX_QINQ_PKT,
> ptype=RTE_PTYPE_L2_ETHER, m->vlan_tci=id, m->vlan_tci_outer=id
> 
> 
> Thoughts?
> 
> 
> 
> >
> >> -Original Message-
> >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of John Daley
> >> (johndale)
> >> Sent: Friday, April 22, 2016 12:37 AM
> >> To: dev at dpdk.org
> >> Subject: [dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled
> >>
> >> Hi,
> >>
> >> When VLAN stripping is disabled, X710 and 82599ES act differently for
> >> me in this case when receiving VLAN tagged packets. On 82599ES the flag
> is set but on X710 the flag not set.
> >>
> >> Do I maybe have old X710 firmware? Or is it not set for X710 on
> >> purpose in this case and instead the flag is used to indicate if vlan_tci 
> >> is
> valid? Right now the enic pmd does what my X710 does, which I think is
> incorrect and I want to fix it.
> >>
> >> Thanks,
> >> John
> >


[dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled

2016-04-22 Thread John Daley (johndale)
Hi,

When VLAN stripping is disabled, X710 and 82599ES act differently for me in 
this case when receiving VLAN tagged packets. On 82599ES the flag is set but on 
X710 the flag not set.

Do I maybe have old X710 firmware? Or is it not set for X710 on purpose in this 
case and instead the flag is used to indicate if vlan_tci is valid? Right now 
the enic pmd does what my X710 does, which I think is incorrect and I want to 
fix it.

Thanks,
John



[dpdk-dev] [PATCH v3] enic: receive path performance improvements

2016-02-21 Thread johndale
- Simplify and reduce code path length of receive function.
- Put most of the fast-path receive funtions in one file.
- Reduce the number of posted_index updates (pay attention to rx_free_thresh)
- Remove the unneeded container structure around the rq mbuf ring
- Prefetch next Mbuf and descriptors while processing the current one
- Lookup table for converting CQ flags to mbuf flags.

Signed-off-by: johndale 
---
Include sign-off

 drivers/net/enic/Makefile   |   1 +
 drivers/net/enic/base/vnic_rq.c |  99 ++
 drivers/net/enic/base/vnic_rq.h | 147 +-
 drivers/net/enic/enic.h |  16 +-
 drivers/net/enic/enic_ethdev.c  |  21 +-
 drivers/net/enic/enic_main.c| 319 +--
 drivers/net/enic/enic_res.h |  16 +-
 drivers/net/enic/enic_rx.c  | 413 
 8 files changed, 555 insertions(+), 477 deletions(-)
 create mode 100644 drivers/net/enic/enic_rx.c

diff --git a/drivers/net/enic/Makefile b/drivers/net/enic/Makefile
index f0ee093..f316274 100644
--- a/drivers/net/enic/Makefile
+++ b/drivers/net/enic/Makefile
@@ -53,6 +53,7 @@ VPATH += $(SRCDIR)/src
 #
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_ethdev.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_main.c
+SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_rx.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_clsf.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_res.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_cq.c
diff --git a/drivers/net/enic/base/vnic_rq.c b/drivers/net/enic/base/vnic_rq.c
index 1441604..cb62c5e 100644
--- a/drivers/net/enic/base/vnic_rq.c
+++ b/drivers/net/enic/base/vnic_rq.c
@@ -35,77 +35,21 @@
 #include "vnic_dev.h"
 #include "vnic_rq.h"

-static int vnic_rq_alloc_bufs(struct vnic_rq *rq)
-{
-   struct vnic_rq_buf *buf;
-   unsigned int i, j, count = rq->ring.desc_count;
-   unsigned int blks = VNIC_RQ_BUF_BLKS_NEEDED(count);
-
-   for (i = 0; i < blks; i++) {
-   rq->bufs[i] = kzalloc(VNIC_RQ_BUF_BLK_SZ(count), GFP_ATOMIC);
-   if (!rq->bufs[i])
-   return -ENOMEM;
-   }
-
-   for (i = 0; i < blks; i++) {
-   buf = rq->bufs[i];
-   for (j = 0; j < VNIC_RQ_BUF_BLK_ENTRIES(count); j++) {
-   buf->index = i * VNIC_RQ_BUF_BLK_ENTRIES(count) + j;
-   buf->desc = (u8 *)rq->ring.descs +
-   rq->ring.desc_size * buf->index;
-   if (buf->index + 1 == count) {
-   buf->next = rq->bufs[0];
-   break;
-   } else if (j + 1 == VNIC_RQ_BUF_BLK_ENTRIES(count)) {
-   buf->next = rq->bufs[i + 1];
-   } else {
-   buf->next = buf + 1;
-   buf++;
-   }
-   }
-   }
-
-   rq->to_use = rq->to_clean = rq->bufs[0];
-
-   return 0;
-}
-
-int vnic_rq_mem_size(struct vnic_rq *rq, unsigned int desc_count,
-   unsigned int desc_size)
-{
-   int mem_size = 0;
-
-   mem_size += vnic_dev_desc_ring_size(>ring, desc_count, desc_size);
-
-   mem_size += VNIC_RQ_BUF_BLKS_NEEDED(rq->ring.desc_count) *
-   VNIC_RQ_BUF_BLK_SZ(rq->ring.desc_count);
-
-   return mem_size;
-}
-
 void vnic_rq_free(struct vnic_rq *rq)
 {
struct vnic_dev *vdev;
-   unsigned int i;

vdev = rq->vdev;

vnic_dev_free_desc_ring(vdev, >ring);

-   for (i = 0; i < VNIC_RQ_BUF_BLKS_MAX; i++) {
-   if (rq->bufs[i]) {
-   kfree(rq->bufs[i]);
-   rq->bufs[i] = NULL;
-   }
-   }
-
rq->ctrl = NULL;
 }

 int vnic_rq_alloc(struct vnic_dev *vdev, struct vnic_rq *rq, unsigned int 
index,
unsigned int desc_count, unsigned int desc_size)
 {
-   int err;
+   int rc;
char res_name[NAME_MAX];
static int instance;

@@ -121,18 +65,9 @@ int vnic_rq_alloc(struct vnic_dev *vdev, struct vnic_rq 
*rq, unsigned int index,
vnic_rq_disable(rq);

snprintf(res_name, sizeof(res_name), "%d-rq-%d", instance++, index);
-   err = vnic_dev_alloc_desc_ring(vdev, >ring, desc_count, desc_size,
+   rc = vnic_dev_alloc_desc_ring(vdev, >ring, desc_count, desc_size,
rq->socket_id, res_name);
-   if (err)
-   return err;
-
-   err = vnic_rq_alloc_bufs(rq);
-   if (err) {
-   vnic_rq_free(rq);
-   return err;
-   }
-
-   return 0;
+   return rc;
 }

 void vnic_rq_init_start(struct vnic_rq *rq, unsigned int cq_index,
@@ -154,9 +89,6 @@ void vnic_rq_init_start(struct vnic_rq *rq, unsigned int 
cq_index,
iowrite32(fetch_index, >ctrl->

[dpdk-dev] [PATCH v3] enic: fix last packet being not sent bug

2016-02-21 Thread johndale
The last packet of the tx burst function array was not being
emitted until the subsequent call.  The nic descriptor index
was being set to the current tx descriptr instead of one past the
the descriptor as required by nic.

Signed-off-by: johndale 
---
Forgot sign-off, annotated message in wrong spot.
 drivers/net/enic/base/enic_vnic_wq.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/enic/base/enic_vnic_wq.h 
b/drivers/net/enic/base/enic_vnic_wq.h
index e3ea574..b019109 100644
--- a/drivers/net/enic/base/enic_vnic_wq.h
+++ b/drivers/net/enic/base/enic_vnic_wq.h
@@ -69,11 +69,11 @@ static inline void enic_vnic_post_wq(struct vnic_wq *wq,
buf->wr_id = wrid;

buf = buf->next;
-   if (cq_entry)
-   enic_vnic_post_wq_index(wq);
+   wq->ring.desc_avail -= desc_skip_cnt;
wq->to_use = buf;

-   wq->ring.desc_avail -= desc_skip_cnt;
+   if (cq_entry)
+   enic_vnic_post_wq_index(wq);
 }

 #endif /* _ENIC_VNIC_WQ_H_ */
-- 
2.7.0



[dpdk-dev] [PATCH v2] enic: fix last packet being not sent bug

2016-02-21 Thread johndale
Oops, forgot sign-off.

The last packet of the tx burst function array was not being
emitted until the subsequent call.  The nic descriptor index
was being set to the current tx descriptr instead of one past the
the descriptor as required by nic.

Signed-off-by: johndale 
---
 drivers/net/enic/base/enic_vnic_wq.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/enic/base/enic_vnic_wq.h 
b/drivers/net/enic/base/enic_vnic_wq.h
index e3ea574..b019109 100644
--- a/drivers/net/enic/base/enic_vnic_wq.h
+++ b/drivers/net/enic/base/enic_vnic_wq.h
@@ -69,11 +69,11 @@ static inline void enic_vnic_post_wq(struct vnic_wq *wq,
buf->wr_id = wrid;

buf = buf->next;
-   if (cq_entry)
-   enic_vnic_post_wq_index(wq);
+   wq->ring.desc_avail -= desc_skip_cnt;
wq->to_use = buf;

-   wq->ring.desc_avail -= desc_skip_cnt;
+   if (cq_entry)
+   enic_vnic_post_wq_index(wq);
 }

 #endif /* _ENIC_VNIC_WQ_H_ */
-- 
2.7.0



[dpdk-dev] [PATCH v2] enic: receive path performance improvements

2016-02-21 Thread johndale
- Simplify and reduce code path length of receive function.
- Put most of the fast-path receive funtions in one file.
- Reduce the number of posted_index updates (pay attention to rx_free_thresh)
- Remove the unneeded container structure around the rq mbuf ring
- Prefetch next Mbuf and descriptors while processing the current one
- Lookup table for converting CQ flags to mbuf flags.
---
 drivers/net/enic/Makefile   |   1 +
 drivers/net/enic/base/vnic_rq.c |  99 ++
 drivers/net/enic/base/vnic_rq.h | 147 +-
 drivers/net/enic/enic.h |  16 +-
 drivers/net/enic/enic_ethdev.c  |  21 +-
 drivers/net/enic/enic_main.c| 319 +--
 drivers/net/enic/enic_res.h |  16 +-
 drivers/net/enic/enic_rx.c  | 413 
 8 files changed, 555 insertions(+), 477 deletions(-)
 create mode 100644 drivers/net/enic/enic_rx.c

diff --git a/drivers/net/enic/Makefile b/drivers/net/enic/Makefile
index f0ee093..f316274 100644
--- a/drivers/net/enic/Makefile
+++ b/drivers/net/enic/Makefile
@@ -53,6 +53,7 @@ VPATH += $(SRCDIR)/src
 #
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_ethdev.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_main.c
+SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_rx.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_clsf.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_res.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_cq.c
diff --git a/drivers/net/enic/base/vnic_rq.c b/drivers/net/enic/base/vnic_rq.c
index 1441604..cb62c5e 100644
--- a/drivers/net/enic/base/vnic_rq.c
+++ b/drivers/net/enic/base/vnic_rq.c
@@ -35,77 +35,21 @@
 #include "vnic_dev.h"
 #include "vnic_rq.h"

-static int vnic_rq_alloc_bufs(struct vnic_rq *rq)
-{
-   struct vnic_rq_buf *buf;
-   unsigned int i, j, count = rq->ring.desc_count;
-   unsigned int blks = VNIC_RQ_BUF_BLKS_NEEDED(count);
-
-   for (i = 0; i < blks; i++) {
-   rq->bufs[i] = kzalloc(VNIC_RQ_BUF_BLK_SZ(count), GFP_ATOMIC);
-   if (!rq->bufs[i])
-   return -ENOMEM;
-   }
-
-   for (i = 0; i < blks; i++) {
-   buf = rq->bufs[i];
-   for (j = 0; j < VNIC_RQ_BUF_BLK_ENTRIES(count); j++) {
-   buf->index = i * VNIC_RQ_BUF_BLK_ENTRIES(count) + j;
-   buf->desc = (u8 *)rq->ring.descs +
-   rq->ring.desc_size * buf->index;
-   if (buf->index + 1 == count) {
-   buf->next = rq->bufs[0];
-   break;
-   } else if (j + 1 == VNIC_RQ_BUF_BLK_ENTRIES(count)) {
-   buf->next = rq->bufs[i + 1];
-   } else {
-   buf->next = buf + 1;
-   buf++;
-   }
-   }
-   }
-
-   rq->to_use = rq->to_clean = rq->bufs[0];
-
-   return 0;
-}
-
-int vnic_rq_mem_size(struct vnic_rq *rq, unsigned int desc_count,
-   unsigned int desc_size)
-{
-   int mem_size = 0;
-
-   mem_size += vnic_dev_desc_ring_size(>ring, desc_count, desc_size);
-
-   mem_size += VNIC_RQ_BUF_BLKS_NEEDED(rq->ring.desc_count) *
-   VNIC_RQ_BUF_BLK_SZ(rq->ring.desc_count);
-
-   return mem_size;
-}
-
 void vnic_rq_free(struct vnic_rq *rq)
 {
struct vnic_dev *vdev;
-   unsigned int i;

vdev = rq->vdev;

vnic_dev_free_desc_ring(vdev, >ring);

-   for (i = 0; i < VNIC_RQ_BUF_BLKS_MAX; i++) {
-   if (rq->bufs[i]) {
-   kfree(rq->bufs[i]);
-   rq->bufs[i] = NULL;
-   }
-   }
-
rq->ctrl = NULL;
 }

 int vnic_rq_alloc(struct vnic_dev *vdev, struct vnic_rq *rq, unsigned int 
index,
unsigned int desc_count, unsigned int desc_size)
 {
-   int err;
+   int rc;
char res_name[NAME_MAX];
static int instance;

@@ -121,18 +65,9 @@ int vnic_rq_alloc(struct vnic_dev *vdev, struct vnic_rq 
*rq, unsigned int index,
vnic_rq_disable(rq);

snprintf(res_name, sizeof(res_name), "%d-rq-%d", instance++, index);
-   err = vnic_dev_alloc_desc_ring(vdev, >ring, desc_count, desc_size,
+   rc = vnic_dev_alloc_desc_ring(vdev, >ring, desc_count, desc_size,
rq->socket_id, res_name);
-   if (err)
-   return err;
-
-   err = vnic_rq_alloc_bufs(rq);
-   if (err) {
-   vnic_rq_free(rq);
-   return err;
-   }
-
-   return 0;
+   return rc;
 }

 void vnic_rq_init_start(struct vnic_rq *rq, unsigned int cq_index,
@@ -154,9 +89,6 @@ void vnic_rq_init_start(struct vnic_rq *rq, unsigned int 
cq_index,
iowrite32(fetch_index, >ctrl->fetch_index);
iowrite32(posted_index, >ctrl->posted_index);

-   rq->to_use = rq->to_clean =
-   >bufs[fetch_index / VNIC_RQ_BUF_BLK_ENTRIES(count)]
-   

[dpdk-dev] [PATCH] enic: fix last packet being not sent bug

2016-02-21 Thread johndale
The last packet of the tx burst function array was not being
emitted until the subsequent call.  The nic descriptor index
was being set to the current tx descriptr instead of one past the
the descriptor as required by nic.

Signed-off-by: johndale 
---
 drivers/net/enic/base/enic_vnic_wq.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/enic/base/enic_vnic_wq.h 
b/drivers/net/enic/base/enic_vnic_wq.h
index e3ea574..b019109 100644
--- a/drivers/net/enic/base/enic_vnic_wq.h
+++ b/drivers/net/enic/base/enic_vnic_wq.h
@@ -69,11 +69,11 @@ static inline void enic_vnic_post_wq(struct vnic_wq *wq,
buf->wr_id = wrid;

buf = buf->next;
-   if (cq_entry)
-   enic_vnic_post_wq_index(wq);
+   wq->ring.desc_avail -= desc_skip_cnt;
wq->to_use = buf;

-   wq->ring.desc_avail -= desc_skip_cnt;
+   if (cq_entry)
+   enic_vnic_post_wq_index(wq);
 }

 #endif /* _ENIC_VNIC_WQ_H_ */
-- 
2.7.0



[dpdk-dev] [PATCH] ENIC PMD receive path performance improvements.

2016-02-15 Thread johndale
- Simplify and reduce code path length of receive function.
- Put all receive stuff in enic_rx.c
- Reduce the number of posted_index updates (pay attention to rx_free_thresh)
- Remove the container structure around the rq mbuf ring
- Prefetch next Mbuf and descriptors while processing the current one
- Lookup table for converting CQ flags to mbuf flags.

Signed-off-by: johndale 
---
 drivers/net/enic/Makefile   |   1 +
 drivers/net/enic/base/vnic_rq.c |  99 ++
 drivers/net/enic/base/vnic_rq.h | 147 +-
 drivers/net/enic/enic.h |  16 +-
 drivers/net/enic/enic_ethdev.c  |  21 +-
 drivers/net/enic/enic_main.c| 319 +--
 drivers/net/enic/enic_res.h |  16 +-
 drivers/net/enic/enic_rx.c  | 413 
 8 files changed, 555 insertions(+), 477 deletions(-)
 create mode 100644 drivers/net/enic/enic_rx.c

diff --git a/drivers/net/enic/Makefile b/drivers/net/enic/Makefile
index f0ee093..f316274 100644
--- a/drivers/net/enic/Makefile
+++ b/drivers/net/enic/Makefile
@@ -53,6 +53,7 @@ VPATH += $(SRCDIR)/src
 #
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_ethdev.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_main.c
+SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_rx.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_clsf.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_res.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_cq.c
diff --git a/drivers/net/enic/base/vnic_rq.c b/drivers/net/enic/base/vnic_rq.c
index 1441604..cb62c5e 100644
--- a/drivers/net/enic/base/vnic_rq.c
+++ b/drivers/net/enic/base/vnic_rq.c
@@ -35,77 +35,21 @@
 #include "vnic_dev.h"
 #include "vnic_rq.h"

-static int vnic_rq_alloc_bufs(struct vnic_rq *rq)
-{
-   struct vnic_rq_buf *buf;
-   unsigned int i, j, count = rq->ring.desc_count;
-   unsigned int blks = VNIC_RQ_BUF_BLKS_NEEDED(count);
-
-   for (i = 0; i < blks; i++) {
-   rq->bufs[i] = kzalloc(VNIC_RQ_BUF_BLK_SZ(count), GFP_ATOMIC);
-   if (!rq->bufs[i])
-   return -ENOMEM;
-   }
-
-   for (i = 0; i < blks; i++) {
-   buf = rq->bufs[i];
-   for (j = 0; j < VNIC_RQ_BUF_BLK_ENTRIES(count); j++) {
-   buf->index = i * VNIC_RQ_BUF_BLK_ENTRIES(count) + j;
-   buf->desc = (u8 *)rq->ring.descs +
-   rq->ring.desc_size * buf->index;
-   if (buf->index + 1 == count) {
-   buf->next = rq->bufs[0];
-   break;
-   } else if (j + 1 == VNIC_RQ_BUF_BLK_ENTRIES(count)) {
-   buf->next = rq->bufs[i + 1];
-   } else {
-   buf->next = buf + 1;
-   buf++;
-   }
-   }
-   }
-
-   rq->to_use = rq->to_clean = rq->bufs[0];
-
-   return 0;
-}
-
-int vnic_rq_mem_size(struct vnic_rq *rq, unsigned int desc_count,
-   unsigned int desc_size)
-{
-   int mem_size = 0;
-
-   mem_size += vnic_dev_desc_ring_size(>ring, desc_count, desc_size);
-
-   mem_size += VNIC_RQ_BUF_BLKS_NEEDED(rq->ring.desc_count) *
-   VNIC_RQ_BUF_BLK_SZ(rq->ring.desc_count);
-
-   return mem_size;
-}
-
 void vnic_rq_free(struct vnic_rq *rq)
 {
struct vnic_dev *vdev;
-   unsigned int i;

vdev = rq->vdev;

vnic_dev_free_desc_ring(vdev, >ring);

-   for (i = 0; i < VNIC_RQ_BUF_BLKS_MAX; i++) {
-   if (rq->bufs[i]) {
-   kfree(rq->bufs[i]);
-   rq->bufs[i] = NULL;
-   }
-   }
-
rq->ctrl = NULL;
 }

 int vnic_rq_alloc(struct vnic_dev *vdev, struct vnic_rq *rq, unsigned int 
index,
unsigned int desc_count, unsigned int desc_size)
 {
-   int err;
+   int rc;
char res_name[NAME_MAX];
static int instance;

@@ -121,18 +65,9 @@ int vnic_rq_alloc(struct vnic_dev *vdev, struct vnic_rq 
*rq, unsigned int index,
vnic_rq_disable(rq);

snprintf(res_name, sizeof(res_name), "%d-rq-%d", instance++, index);
-   err = vnic_dev_alloc_desc_ring(vdev, >ring, desc_count, desc_size,
+   rc = vnic_dev_alloc_desc_ring(vdev, >ring, desc_count, desc_size,
rq->socket_id, res_name);
-   if (err)
-   return err;
-
-   err = vnic_rq_alloc_bufs(rq);
-   if (err) {
-   vnic_rq_free(rq);
-   return err;
-   }
-
-   return 0;
+   return rc;
 }

 void vnic_rq_init_start(struct vnic_rq *rq, unsigned int cq_index,
@@ -154,9 +89,6 @@ void vnic_rq_init_start(struct vnic_rq *rq, unsigned int 
cq_index,
iowrite32(fetch_index, >ctrl->fetch_index);
iowrite32(posted_index, >ctrl->posted_index

[dpdk-dev] [PATCH] Enic PMD Rx performance improvements

2016-02-15 Thread johndale
This is a rewrite of the receive path of the Enic PMD to simplify the
code and improve packet rate. Sorry I couldn't figure a way to organize
it as a series of patches, so I'm submitting it as a single patch.
thanks,
john

johndale (1):
  ENIC PMD receive path performance improvements.

 drivers/net/enic/Makefile   |   1 +
 drivers/net/enic/base/vnic_rq.c |  99 ++
 drivers/net/enic/base/vnic_rq.h | 147 +-
 drivers/net/enic/enic.h |  16 +-
 drivers/net/enic/enic_ethdev.c  |  21 +-
 drivers/net/enic/enic_main.c| 319 +--
 drivers/net/enic/enic_res.h |  16 +-
 drivers/net/enic/enic_rx.c  | 413 
 8 files changed, 555 insertions(+), 477 deletions(-)
 create mode 100644 drivers/net/enic/enic_rx.c

-- 
2.7.0



[dpdk-dev] [PATCH] doc: add entry for enic PMD Tx improvement to the 2.2 release notes.

2015-11-06 Thread johndale
Signed-off-by: johndale 
---
 doc/guides/rel_notes/release_2_2.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/guides/rel_notes/release_2_2.rst 
b/doc/guides/rel_notes/release_2_2.rst
index 59dda59..8bc5fca 100644
--- a/doc/guides/rel_notes/release_2_2.rst
+++ b/doc/guides/rel_notes/release_2_2.rst
@@ -162,6 +162,9 @@ Drivers

   Fixed issue when releasing null control queue.

+* **enic: Improve Tx packet rate.**
+
+  Reduced frequency of Tx tail pointer updates to the nic.

 Libraries
 ~
-- 
2.4.3



[dpdk-dev] [PATCH] maintainers: Add maintainers for enic PMD

2015-11-05 Thread johndale
Acked-by: Thomas Monjalon 
Signed-off-by: johndale 
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c8be5d2..f3dd1b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -226,6 +226,8 @@ F: drivers/net/cxgbe/
 F: doc/guides/nics/cxgbe.rst

 Cisco enic
+M: John Daley 
+M: Sujith Sankar 
 F: drivers/net/enic/

 Intel e1000
-- 
2.4.3