* Re: [PATCH v12 0/6] add Tx preparation
From: Ananyev, Konstantin @ 2016-11-30 10:54 UTC (permalink / raw)
To: Adrien Mazarguil, Thomas Monjalon
Cc: dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd, Jan Medala,
Jakub Palider, John Daley, Alejandro Lucero, Harish Patil,
Rasesh Mody, Jerin Jacob, Yuanhan Liu, Yong Wang,
Kulasek, TomaszX, olivier.matz@6wind.com
In-Reply-To: <20161130074003.GD10340@6wind.com>
Hi Adrien,
>
> On Mon, Nov 28, 2016 at 12:03:06PM +0100, Thomas Monjalon wrote:
> > We need attention of every PMD developers on this thread.
>
> I've been following this thread from the beginning while working on rte_flow
> and wanted to see where it was headed before replying. (I know, v11 was
> submitted about 1 month ago but still.)
>
> > 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.
>
> Something is definitely needed here, and only PMDs can provide it. I think
> applications should not have to clear checksum fields or initialize them to
> some magic value, same goes for any other offload or hardware limitation
> that needs to be worked around.
>
> tx_prep() is one possible answer to this issue, however as mentioned in the
> original patch it can be very expensive if exposed by the PMD.
>
> Another issue I'm more concerned about is the way limitations are managed
> (struct rte_eth_desc_lim). While not officially tied to tx_prep(), this
> structure contains new fields that are only relevant to a few devices, and I
> fear it will keep growing with each new hardware quirk to manage, breaking
> ABIs in the process.
Well, if some new HW capability/limitation would arise and we'd like to support
it in DPDK, then yes we probably would need to think how to incorporate it here.
Do you have anything particular in mind here?
>
> What are applications supposed to do, check each of them regardless before
> attempting to send a burst?
>
> I understand tx_prep() automates this process, however I'm wondering why
> isn't the TX burst function doing that itself. Using nb_mtu_seg_max as an
> example, tx_prep() has an extra check in case of TSO that the TX burst
> function does not perform. This ends up being much more expensive to
> applications due to the additional loop doing redundant testing on each
> mbuf.
>
> If, say as a performance improvement, we decided to leave the validation
> part to the TX burst function; what remains in tx_prep() is basically heavy
> "preparation" requiring mbuf changes (i.e. erasing checksums, for now).
>
> Following the same logic, why can't such a thing be made part of the TX
> burst function as well (through a direct call to rte_phdr_cksum_fix()
> whenever necessary). From an application standpoint, what are the advantages
> of having to:
>
> if (tx_prep()) // iterate and update mbufs as needed
> tx_burst(); // iterate and send
>
> Compared to:
>
> tx_burst(); // iterate, update as needed and send
I think that was discussed extensively quite a lot previously here:
As Thomas already replied - main motivation is to allow user
to execute them on different stages of packet TX pipeline,
and probably on different cores.
I think that provides better flexibility to the user to when/where
do these preparations and hopefully would lead to better performance.
Though, if you or any other PMD developer/maintainer would prefer
for particular PMD to combine both functionalities into tx_burst() and
keep tx_prep() as NOP - this is still possible too.
>
> Note that PMDs could still provide different TX callbacks depending on the
> set of enabled offloads so performance is not unnecessarily impacted.
>
> In my opinion the second approach is both faster to applications and more
> friendly from a usability perspective, am I missing something obvious?
>
> > 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"
>
> For both mlx4 and mlx5 then,
> "it is OK, we do not need any checksum preparation for TSO".
>
> Actually I do not think we'll ever need tx_prep() unless we add our own
> quirks to struct rte_eth_desc_lim (and friends) which are currently quietly
> handled by TX burst functions.
Ok, so MLX PMD is not affected by these changes and tx_prep for MLX can be safely
set to NULL, correct?
Thanks
Konstantin
>
> --
> Adrien Mazarguil
> 6WIND
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Kulasek, TomaszX @ 2016-11-30 10:30 UTC (permalink / raw)
To: Thomas Monjalon, Adrien Mazarguil
Cc: dev@dpdk.org, Ananyev, Konstantin, olivier.matz@6wind.com
In-Reply-To: <2285429.Z7YL4y4ekv@xps13>
Hi,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, November 30, 2016 09:50
> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Kulasek, TomaszX
> <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v12 0/6] add Tx preparation
>
> 2016-11-30 08:40, Adrien Mazarguil:
> [...]
> > I understand tx_prep() automates this process, however I'm wondering
> > why isn't the TX burst function doing that itself. Using
> > nb_mtu_seg_max as an example, tx_prep() has an extra check in case of
> > TSO that the TX burst function does not perform. This ends up being
> > much more expensive to applications due to the additional loop doing
> > redundant testing on each mbuf.
> >
> > If, say as a performance improvement, we decided to leave the
> > validation part to the TX burst function; what remains in tx_prep() is
> > basically heavy "preparation" requiring mbuf changes (i.e. erasing
> checksums, for now).
> >
> > Following the same logic, why can't such a thing be made part of the
> > TX burst function as well (through a direct call to
> > rte_phdr_cksum_fix() whenever necessary). From an application
> > standpoint, what are the advantages of having to:
> >
> > if (tx_prep()) // iterate and update mbufs as needed
> > tx_burst(); // iterate and send
> >
> > Compared to:
> >
> > tx_burst(); // iterate, update as needed and send
> >
> > Note that PMDs could still provide different TX callbacks depending on
> > the set of enabled offloads so performance is not unnecessarily
> impacted.
> >
> > In my opinion the second approach is both faster to applications and
> > more friendly from a usability perspective, am I missing something
> obvious?
>
> I think it was not clearly explained in this patchset, but this is my
> understanding:
> tx_prepare and tx_burst can be called at different stages of a pipeline,
> on different cores.
Yes, this API is intended to be used optionaly, not only just before tx_burst.
1. Separating both stages:
a) We may have a control over burst (packet content, validation) when needed.
b) For invalid packets we may restore them or do some another task if needed (even on early stage of processing).
c) Tx burst keep as simple as it should be.
2. Joining the functionality of tx_prepare and tx_burst have some disadvantages:
a) When packet is invalid it cannot be restored by application should be dropped.
b) Tx burst needs to modify the content of the packet.
c) We have no way to eliminate overhead of preparation (tx_prepare) for the application where performance is a key.
3. Using tx callbacks
a) We still need to have different implementations for different devices.
b) The overhead in performance (comparing to the pair tx_prepare/tx_burst) will not be better while both ways uses very similar mechanism.
In addition, tx_prepare mechanism can be turned off by compilation flag (as discussed with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory dereference and check can have significant impact on performance).
Tomasz
^ permalink raw reply
* Re: [PATCH v2] virtio: tx with can_push when VERSION_1 is set
From: Yuanhan Liu @ 2016-11-30 10:19 UTC (permalink / raw)
To: Pierre Pfister (ppfister); +Cc: dev@dpdk.org, Maxime Coquelin
In-Reply-To: <C1042323-AA70-46AF-B4D2-24A470ED8DB9@cisco.com>
On Wed, Nov 30, 2016 at 09:18:42AM +0000, Pierre Pfister (ppfister) wrote:
> Current virtio driver advertises VERSION_1 support,
> but does not handle device's VERSION_1 support when
> sending packets (it looks for ANY_LAYOUT feature,
> which is absent).
>
> This patch enables 'can_push' in tx path when VERSION_1
> is advertised by the device.
>
> This significantly improves small packets forwarding rate
> towards devices advertising VERSION_1 feature.
>
> Signed-off-by: Pierre Pfister <ppfister@cisco.com>
>From the virtio spec point of view, that VERSION_1 implies ANY_LAYOUT,
this patch is right and I think we could apply this patch. No objections?
--yliu
> ---
> drivers/net/virtio/virtio_rxtx.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 22d97a4..1e5a6b9 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -1015,7 +1015,8 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> }
>
> /* optimize ring usage */
> - if (vtpci_with_feature(hw, VIRTIO_F_ANY_LAYOUT) &&
> + if ((vtpci_with_feature(hw, VIRTIO_F_ANY_LAYOUT) ||
> + vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) &&
> rte_mbuf_refcnt_read(txm) == 1 &&
> RTE_MBUF_DIRECT(txm) &&
> txm->nb_segs == 1 &&
> --
> 2.7.4 (Apple Git-66)
^ permalink raw reply
* Re: Proposal for a new Committer model
From: Mcnamara, John @ 2016-11-30 9:58 UTC (permalink / raw)
To: Neil Horman; +Cc: dev@dpdk.org
In-Reply-To: <20161129191207.GA17132@hmsreliant.think-freely.org>
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Tuesday, November 29, 2016 7:12 PM
> To: Mcnamara, John <john.mcnamara@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] Proposal for a new Committer model
>
> > ...
> >
> > B) Designate alternates to serve as backups for the maintainer when
> > they are unavailable. This provides high-availablility, and sounds
> > very much like your proposal, but in the interests of clarity, there
> > is still a single maintainer at any one time, it just may change to
> > ensure the continued merging of patches, if the primary maintainer isn't
> available.
> > Ideally however, those backup alternates arent needed, because most of
> > the primary maintainers work in merging pull requests, which are done
> > based on the trust of the submaintainer, and done during a very
> > limited window of time. This also partially addreses multi-vendor
> > fairness if your subtree maintainers come from multiple participating
> companies.
> >
> > Regards
> > Neil
> >
> >
> >
>
> Soo, I feel like we're wandering away from this thread. Are you going to
> make a change to the comitter model?
Hi,
Yes. I think we have consensus on the main parts. I'll re-draft a proposal that we can discuss and then add to the contributors guide.
John
^ permalink raw reply
* Re: [PATCH 1/1] net/i40e: enable auto link update for XXV710
From: Ferruh Yigit @ 2016-11-30 9:56 UTC (permalink / raw)
To: Zhang, Qi Z, Wu, Jingjing, Zhang, Helin; +Cc: dev@dpdk.org
In-Reply-To: <039ED4275CED7440929022BC67E7061150672A60@SHSMSX103.ccr.corp.intel.com>
On 11/30/2016 2:26 AM, Zhang, Qi Z wrote:
> Hi Ferruh:
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, November 30, 2016 1:46 AM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
>> Zhang, Helin <helin.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 1/1] net/i40e: enable auto link update for
>> XXV710
>>
>> Hi Qi,
>>
>> On 11/24/2016 11:43 PM, Qi Zhang wrote:
>>> This patch remove the limitation that XXV710 device does
>>
>> XXV710 is 25G device, and support added in 16.11 (please correct me if this is
>> wrong.), but I can't find any DPDK documentation for this device.
>>
>> Can you please add some documentation, at least to http://dpdk.org/doc/nics
>> and http://dpdk.org/doc/guides/nics/i40e.html
>> in a different patch?
>
> For 16.11, since XXV710 is not officially supported, so they are missing in document.
> For 17.02 this will be updated. Thanks for remind.
If officially will be added on 17.02, can you please update release
notes too, to announce new device support?
>>
>>> not support auto link update.
>>
>> Can you please add more details that why we can remove the limitation now?
> Ok, will update in v2.
>>
>>>
>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>>> ---
>>> drivers/net/i40e/i40e_ethdev.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/i40e/i40e_ethdev.c
>>> b/drivers/net/i40e/i40e_ethdev.c index 67778ba..b7a916d 100644
>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>> @@ -1628,6 +1628,8 @@ i40e_phy_conf_link(struct i40e_hw *hw,
>>>
>>> /* use get_phy_abilities_resp value for the rest */
>>> phy_conf.phy_type = phy_ab.phy_type;
>>> + phy_conf.phy_type_ext = phy_ab.phy_type_ext;
>>> + phy_conf.fec_config = phy_ab.mod_type_ext;
>>
>> And these changes look like called for all device types, just to double check, are
>> these 25G specific?
>
> Actually only XXV710 need this two lines, but base on firmware engineer's input,
> this could be implemented in generic way since no impact for other i40e devices.
>>
>>> phy_conf.eee_capability = phy_ab.eee_capability;
>>> phy_conf.eeer = phy_ab.eeer_val;
>>> phy_conf.low_power_ctrl = phy_ab.d3_lpan; @@ -1653,8 +1655,7 @@
>>> i40e_apply_link_speed(struct rte_eth_dev *dev)
>>> struct rte_eth_conf *conf = &dev->data->dev_conf;
>>>
>>> speed = i40e_parse_link_speeds(conf->link_speeds);
>>> - if (!I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
>>> - abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
>>> + abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
>>> if (!(conf->link_speeds & ETH_LINK_SPEED_FIXED))
>>> abilities |= I40E_AQ_PHY_AN_ENABLED;
>>> abilities |= I40E_AQ_PHY_LINK_ENABLED; @@ -1990,8 +1991,7 @@
>>> i40e_dev_set_link_down(struct rte_eth_dev *dev)
>>> uint8_t abilities = 0;
>>> struct i40e_hw *hw =
>> I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>>
>>> - if (!I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
>>> - abilities = I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
>>> + abilities = I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
>>> return i40e_phy_conf_link(hw, abilities, speed); }
>>>
>>>
> Thanks!
> Qi
>
^ permalink raw reply
* [PATCH v2] virtio: tx with can_push when VERSION_1 is set
From: Pierre Pfister (ppfister) @ 2016-11-30 9:18 UTC (permalink / raw)
To: dev@dpdk.org; +Cc: Yuanhan Liu, Maxime Coquelin
In-Reply-To: <6D3BD8A2-1567-42FA-9672-EE682D827A84@cisco.com>
Current virtio driver advertises VERSION_1 support,
but does not handle device's VERSION_1 support when
sending packets (it looks for ANY_LAYOUT feature,
which is absent).
This patch enables 'can_push' in tx path when VERSION_1
is advertised by the device.
This significantly improves small packets forwarding rate
towards devices advertising VERSION_1 feature.
Signed-off-by: Pierre Pfister <ppfister@cisco.com>
---
drivers/net/virtio/virtio_rxtx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 22d97a4..1e5a6b9 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1015,7 +1015,8 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
}
/* optimize ring usage */
- if (vtpci_with_feature(hw, VIRTIO_F_ANY_LAYOUT) &&
+ if ((vtpci_with_feature(hw, VIRTIO_F_ANY_LAYOUT) ||
+ vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) &&
rte_mbuf_refcnt_read(txm) == 1 &&
RTE_MBUF_DIRECT(txm) &&
txm->nb_segs == 1 &&
--
2.7.4 (Apple Git-66)
^ permalink raw reply related
* Re: [PATCH] virtio: tx with can_push when VERSION_1 is set
From: Pierre Pfister (ppfister) @ 2016-11-30 9:09 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: Yuanhan Liu, dev@dpdk.org
In-Reply-To: <30923c66-8eaa-f7d4-2e6a-d65d8b254e8e@redhat.com>
> Le 22 nov. 2016 à 14:17, Maxime Coquelin <maxime.coquelin@redhat.com> a écrit :
>
> Hi Pierre,
>
> On 11/22/2016 10:54 AM, Pierre Pfister (ppfister) wrote:
>> Hello Maxime,
>>
>>> Le 9 nov. 2016 à 15:51, Maxime Coquelin <maxime.coquelin@redhat.com> a écrit :
>>>
>>> Hi Pierre,
>>>
>>> On 11/09/2016 01:42 PM, Pierre Pfister (ppfister) wrote:
>>>> Hello Maxime,
>>>>
>>>> Sorry for the late reply.
>>>>
>>>>
>>>>> Le 8 nov. 2016 à 10:44, Maxime Coquelin <maxime.coquelin@redhat.com> a écrit :
>>>>>
>>>>> Hi Pierre,
>>>>>
>>>>> On 11/08/2016 10:31 AM, Pierre Pfister (ppfister) wrote:
>>>>>> Current virtio driver advertises VERSION_1 support,
>>>>>> but does not handle device's VERSION_1 support when
>>>>>> sending packets (it looks for ANY_LAYOUT feature,
>>>>>> which is absent).
>>>>>>
>>>>>> This patch enables 'can_push' in tx path when VERSION_1
>>>>>> is advertised by the device.
>>>>>>
>>>>>> This significantly improves small packets forwarding rate
>>>>>> towards devices advertising VERSION_1 feature.
>>>>> I think it depends whether offloading is enabled or not.
>>>>> If no offloading enabled, I measured significant drop.
>>>>> Indeed, when no offloading is enabled, the Tx path in Virtio
>>>>> does not access the virtio header before your patch, as the header is memset to zero at device init time.
>>>>> With your patch, it gets memset to zero at every transmit in the hot
>>>>> path.
>>>>
>>>> Right. On the virtio side that is true, but on the device side, we have to access the header anyway.
>>> No more now, if no offload features have been negotiated.
>>> I have done a patch that landed in v16.11 to skip header parsing in
>>> this case.
>>> That said, we still have to access its descriptor.
>>>
>>>> And accessing two descriptors (with the address resolution and memory fetch which comes with it)
>>>> is a costy operation compared to a single one.
>>>> In the case indirect descriptors are used, this is 1 desc access instead or 3.
>>> I agree this is far from being optimal.
>>>
>>>> And in the case chained descriptors are used, this doubles the number of packets that you can put in your queue.
>>>>
>>>> Those are the results in my PHY -> VM (testpmd) -> PHY setup
>>>> Traffic is flowing bidirectionally. Numbers are for lossless-rates.
>>>>
>>>> When chained buffers are used for dpdk's TX: 2x2.13Mpps
>>>> When indirect descriptors are used for dpdk's TX: 2x2.38Mpps
>>>> When shallow buffers are used for dpdk's TX (with this patch): 2x2.42Mpps
>>> When I tried it, I also did PVP 0% benchmark, and I got opposite results. Chained and indirect cases were significantly better.
>>>
>>> My PVP setup was using a single NIC and single Virtio PMD, and NIC2VM
>>> forwarding was IO mode done with testpmd on host, and Rx->Tx forwarding
>>> was macswap mode on guest side.
>>>
>>> I also saw some perf regression when running simple tespmd test on both
>>> ends.
>>>
>>> Yuanhan, did you run some benchmark with your series enabling
>>> ANY_LAYOUT?
>>
>> It was enabled. But the specs specify that VERSION_1 includes ANY_LAYOUT.
>> Therefor, Qemu removes ANY_LAYOUT when VERSION_1 is set.
>>
>> We can keep arguing about which is fastest. I guess we have different setups and different results, so we probably are deadlocked here.
>> But in any case, the current code is inconsistent, as it uses single descriptor when ANY_LAYOUT is set, but not when VERSION_1 is set.
>>
>> I believe it makes sense to use single-descriptor any time it is possible, but you are free to think otherwise.
>> Please make a call and make the code consistent (removes single-descriptors all together, or use them when VERSION_1 is set too). Otherwise it just creates yet-another testing headache.
> I also think it makes sense to have a single descriptor, but I had to
> highlight that I noticed a significant performance degradation when
> using single descriptor on my setup.
>
> I'm fine we take your patch in virtio-next, so that more testing is conducted.
>
Thanks,
I just realised there was an indentation error in the patch.
Meaning that this patch didn't make it to 16.11 ...
I will send a new version.
- Pierre
> Thanks,
> Maxime
>
>>
>> Thanks,
>>
>> - Pierre
>>
>>>
>>>>
>>>> I must also note that qemu 2.5 does not seem to deal with VERSION_1 and ANY_LAYOUT correctly.
>>>> The patch I am proposing here works for qemu 2.7, but with qemu 2.5, testpmd still behaves as if ANY_LAYOUT (or VERSION_1) was not available. This is not catastrophic. But just note that you will not see performance in some cases with qemu 2.5.
>>>
>>> Thanks for the info.
>>>
>>> Regards,
>>> Maxime
>>
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Thomas Monjalon @ 2016-11-30 8:50 UTC (permalink / raw)
To: Adrien Mazarguil, Tomasz Kulasek; +Cc: dev, konstantin.ananyev, olivier.matz
In-Reply-To: <20161130074003.GD10340@6wind.com>
2016-11-30 08:40, Adrien Mazarguil:
[...]
> I understand tx_prep() automates this process, however I'm wondering why
> isn't the TX burst function doing that itself. Using nb_mtu_seg_max as an
> example, tx_prep() has an extra check in case of TSO that the TX burst
> function does not perform. This ends up being much more expensive to
> applications due to the additional loop doing redundant testing on each
> mbuf.
>
> If, say as a performance improvement, we decided to leave the validation
> part to the TX burst function; what remains in tx_prep() is basically heavy
> "preparation" requiring mbuf changes (i.e. erasing checksums, for now).
>
> Following the same logic, why can't such a thing be made part of the TX
> burst function as well (through a direct call to rte_phdr_cksum_fix()
> whenever necessary). From an application standpoint, what are the advantages
> of having to:
>
> if (tx_prep()) // iterate and update mbufs as needed
> tx_burst(); // iterate and send
>
> Compared to:
>
> tx_burst(); // iterate, update as needed and send
>
> Note that PMDs could still provide different TX callbacks depending on the
> set of enabled offloads so performance is not unnecessarily impacted.
>
> In my opinion the second approach is both faster to applications and more
> friendly from a usability perspective, am I missing something obvious?
I think it was not clearly explained in this patchset, but this is
my understanding:
tx_prepare and tx_burst can be called at different stages of a pipeline,
on different cores.
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Adrien Mazarguil @ 2016-11-30 7:40 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Rahul Lakkireddy, Stephen Hurd, Jan Medala, Jakub Palider,
John Daley, Alejandro Lucero, Harish Patil, Rasesh Mody,
Jerin Jacob, Yuanhan Liu, Yong Wang, Tomasz Kulasek,
konstantin.ananyev, olivier.matz
In-Reply-To: <8317180.L80Qf11uiu@xps13>
On Mon, Nov 28, 2016 at 12:03:06PM +0100, Thomas Monjalon wrote:
> We need attention of every PMD developers on this thread.
I've been following this thread from the beginning while working on rte_flow
and wanted to see where it was headed before replying. (I know, v11 was
submitted about 1 month ago but still.)
> 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.
Something is definitely needed here, and only PMDs can provide it. I think
applications should not have to clear checksum fields or initialize them to
some magic value, same goes for any other offload or hardware limitation
that needs to be worked around.
tx_prep() is one possible answer to this issue, however as mentioned in the
original patch it can be very expensive if exposed by the PMD.
Another issue I'm more concerned about is the way limitations are managed
(struct rte_eth_desc_lim). While not officially tied to tx_prep(), this
structure contains new fields that are only relevant to a few devices, and I
fear it will keep growing with each new hardware quirk to manage, breaking
ABIs in the process.
What are applications supposed to do, check each of them regardless before
attempting to send a burst?
I understand tx_prep() automates this process, however I'm wondering why
isn't the TX burst function doing that itself. Using nb_mtu_seg_max as an
example, tx_prep() has an extra check in case of TSO that the TX burst
function does not perform. This ends up being much more expensive to
applications due to the additional loop doing redundant testing on each
mbuf.
If, say as a performance improvement, we decided to leave the validation
part to the TX burst function; what remains in tx_prep() is basically heavy
"preparation" requiring mbuf changes (i.e. erasing checksums, for now).
Following the same logic, why can't such a thing be made part of the TX
burst function as well (through a direct call to rte_phdr_cksum_fix()
whenever necessary). From an application standpoint, what are the advantages
of having to:
if (tx_prep()) // iterate and update mbufs as needed
tx_burst(); // iterate and send
Compared to:
tx_burst(); // iterate, update as needed and send
Note that PMDs could still provide different TX callbacks depending on the
set of enabled offloads so performance is not unnecessarily impacted.
In my opinion the second approach is both faster to applications and more
friendly from a usability perspective, am I missing something obvious?
> 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"
For both mlx4 and mlx5 then,
"it is OK, we do not need any checksum preparation for TSO".
Actually I do not think we'll ever need tx_prep() unless we add our own
quirks to struct rte_eth_desc_lim (and friends) which are currently quietly
handled by TX burst functions.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: John Daley (johndale) @ 2016-11-30 5:48 UTC (permalink / raw)
To: Thomas Monjalon, dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd,
Jan Medala, Jakub Palider, Adrien Mazarguil, Alejandro Lucero,
Harish Patil, Rasesh Mody, Jerin Jacob, Yuanhan Liu, Yong Wang
Cc: Tomasz Kulasek, konstantin.ananyev@intel.com,
olivier.matz@6wind.com
In-Reply-To: <8317180.L80Qf11uiu@xps13>
Hi,
-john
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 28, 2016 3:03 AM
> To: dev@dpdk.org; Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>;
> Stephen Hurd <stephen.hurd@broadcom.com>; Jan Medala
> <jan@semihalf.com>; Jakub Palider <jpa@semihalf.com>; John Daley
> (johndale) <johndale@cisco.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; Alejandro Lucero
> <alejandro.lucero@netronome.com>; Harish Patil
> <harish.patil@qlogic.com>; Rasesh Mody <rasesh.mody@qlogic.com>; Jerin
> Jacob <jerin.jacob@caviumnetworks.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>; Yong Wang <yongwang@vmware.com>
> Cc: Tomasz Kulasek <tomaszx.kulasek@intel.com>;
> konstantin.ananyev@intel.com; olivier.matz@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
^ permalink raw reply
* [PATCH v2] doc: prog_guide: fix section heading
From: Baruch Siach @ 2016-11-30 5:43 UTC (permalink / raw)
To: dev; +Cc: John McNamara, Baruch Siach
This section only deals with Tx queues configuration.
Acked-by: John McNamara <john.mcnamara@intel.com>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2: Add John's ack
---
doc/guides/prog_guide/poll_mode_drv.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index bf3ea9fde25a..d4c92ea2cf4a 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -200,8 +200,8 @@ Ethernet* flow control (pause frame) can be configured on the individual port.
Refer to the testpmd source code for details.
Also, L4 (UDP/TCP/ SCTP) checksum offload by the NIC can be enabled for an individual packet as long as the packet mbuf is set up correctly. See `Hardware Offload`_ for details.
-Configuration of Transmit and Receive Queues
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Configuration of Transmit Queues
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Each transmit queue is independently configured with the following information:
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] vmxnet3: fix Rx deadlock
From: Yong Wang @ 2016-11-30 4:59 UTC (permalink / raw)
To: Stefan Puiu, dev@dpdk.org; +Cc: mac_leehk@yahoo.com.hk
In-Reply-To: <1479120376-48723-1-git-send-email-stefan.puiu@gmail.com>
> -----Original Message-----
> From: Stefan Puiu [mailto:stefan.puiu@gmail.com]
> Sent: Monday, November 14, 2016 2:46 AM
> To: dev@dpdk.org
> Cc: mac_leehk@yahoo.com.hk; Yong Wang <yongwang@vmware.com>;
> Stefan Puiu <stefan.puiu@gmail.com>
> Subject: [PATCH] vmxnet3: fix Rx deadlock
>
> Our use case is that we have an app that needs to keep mbufs around
> for a while. We've seen cases when calling vmxnet3_post_rx_bufs() from
> vmxet3_recv_pkts(), it might not succeed to add any mbufs to any RX
> descriptors (where it returns -err). Since there are no mbufs that the
> virtual hardware can use, and since nobody calls
> vmxnet3_post_rx_bufs() after that, no packets will be received after
The patch looks good overall.
I think a more accurate description is that the particular descriptor's generation bit never got flipped properly when an mbuf failed to be refilled which caused the rx stuck, rather than vmxnet3_post_rx_bufs() not being called afterwards.
> this. I call this a deadlock for lack of a better term - the virtual
> HW waits for free mbufs, while the app waits for the hardware to
> notify it for data. Note that after this, the app can't recover.
>
> This fix is a rework of this patch by Marco Lee:
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__dpdk.org_dev_patchwork_patch_6575_&d=CwIBAg&c=Sqcl0Ez6M0X8a
> eM67LKIiDJAXVeAw-YihVMNtXt-
> uEs&r=44mSO5N5yEs4CeCdtQE0xt0F7J0p67_mApYVAzyYms0&m=g2gi3ZErdx
> AKGY8d3wbhk2D6TLUVYBs3K-
> KMdiJwuvI&s=YLz0Wsl_kQUXPWij82nnO9ROB64AK5ZtDCyUvHuU8jA&e= . I
> had to forward port it,
> address review comments and also reverted the allocation failure
> handing to the first version of the patch
s/handing/handling
> (https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__dpdk.org_ml_archives_dev_2015-
> 2DJuly_022079.html&d=CwIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-
> YihVMNtXt-
> uEs&r=44mSO5N5yEs4CeCdtQE0xt0F7J0p67_mApYVAzyYms0&m=g2gi3ZErdx
> AKGY8d3wbhk2D6TLUVYBs3K-
> KMdiJwuvI&s=5HksZV8s99b3jVV7Pea60d18hKqXxp4eRpJWjz6sWLc&e= ),
> since that's
> the only approach that seems to work, and seems to be what other
> drivers are doing (I checked ixgbe and em). Reusing the mbuf that's
> getting passed to the application doesn't seem to make sense, and it
> was causing weird issues in our app. Also, reusing rxm without
> checking if it's NULL could cause the code to crash.
>
> Signed-off-by: Stefan Puiu <stefan.puiu@gmail.com>
> ---
> drivers/net/vmxnet3/vmxnet3_rxtx.c | 38
> ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> index b109168..c9d2488 100644
> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> @@ -518,6 +518,32 @@
> return nb_tx;
> }
>
> +static inline void
> +vmxnet3_renew_desc(vmxnet3_rx_queue_t *rxq, uint8_t ring_id,
> + struct rte_mbuf *mbuf)
Nit: align the params here to be consistent with other functions.
> +{
> + uint32_t val = 0;
Nit: extra space before "val"
> + struct vmxnet3_cmd_ring *ring = &rxq->cmd_ring[ring_id];
> + struct Vmxnet3_RxDesc *rxd =
> + (struct Vmxnet3_RxDesc *)(ring->base + ring->next2fill);
> + vmxnet3_buf_info_t *buf_info = &ring->buf_info[ring->next2fill];
> +
> + if (ring_id == 0)
> + val = VMXNET3_RXD_BTYPE_HEAD;
> + else
> + val = VMXNET3_RXD_BTYPE_BODY;
> +
> + buf_info->m = mbuf;
> + buf_info->len = (uint16_t)(mbuf->buf_len -
> RTE_PKTMBUF_HEADROOM);
> + buf_info->bufPA = rte_mbuf_data_dma_addr_default(mbuf);
> +
> + rxd->addr = buf_info->bufPA;
> + rxd->btype = val;
> + rxd->len = buf_info->len;
> + rxd->gen = ring->gen;
> +
> + vmxnet3_cmd_ring_adv_next2fill(ring);
> +}
> /*
> * Allocates mbufs and clusters. Post rx descriptors with buffer details
> * so that device can receive packets in those buffers.
> @@ -657,9 +683,17 @@
> }
>
> while (rcd->gen == rxq->comp_ring.gen) {
> + struct rte_mbuf *newm;
Nit: add a blank line here.
> if (nb_rx >= nb_pkts)
> break;
>
> + newm = rte_mbuf_raw_alloc(rxq->mp);
> + if (unlikely(newm == NULL)) {
> + PMD_RX_LOG(ERR, "Error allocating mbuf");
> + rxq->stats.rx_buf_alloc_failure++;
> + break;
> + }
> +
> idx = rcd->rxdIdx;
> ring_idx = (uint8_t)((rcd->rqID == rxq->qid1) ? 0 : 1);
> rxd = (Vmxnet3_RxDesc *)rxq->cmd_ring[ring_idx].base +
> idx;
> @@ -759,8 +793,8 @@
> VMXNET3_INC_RING_IDX_ONLY(rxq-
> >cmd_ring[ring_idx].next2comp,
> rxq->cmd_ring[ring_idx].size);
>
> - /* It's time to allocate some new buf and renew descriptors
> */
> - vmxnet3_post_rx_bufs(rxq, ring_idx);
> + /* It's time to renew descriptors */
Nit: extra space before "renew"
> + vmxnet3_renew_desc(rxq, ring_idx, newm);
> if (unlikely(rxq->shared->ctrl.updateRxProd)) {
> VMXNET3_WRITE_BAR0_REG(hw,
> rxprod_reg[ring_idx] + (rxq->queue_id * VMXNET3_REG_ALIGN),
> rxq->cmd_ring[ring_idx].next2fill);
> --
> 1.9.1
^ permalink raw reply
* [PATCH v2 1/1] net/i40e: enable auto link update for 25G
From: Qi Zhang @ 2016-11-29 20:26 UTC (permalink / raw)
To: jingjing.wu, helin.zhang; +Cc: dev, Qi Zhang
In previous patch for 25G (XXV710) enable
75d133dd329: ("net/i40e: enable 25G device"),
we intend to disable the auto linke update as a work around
for the issue that link can't be turn on when auto link update
is enabled. Now we know the root cause, there are interface
changes of AQ command "set_phy_config" and "get_phy_capabilities"
for 25G. So, this patch remove this limitation.
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
v2:
- use 25G to replace XXV710 in title to consist with previous patches.
- explain more detail in commit log.
drivers/net/i40e/i40e_ethdev.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 67778ba..b7a916d 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -1628,6 +1628,8 @@ i40e_phy_conf_link(struct i40e_hw *hw,
/* use get_phy_abilities_resp value for the rest */
phy_conf.phy_type = phy_ab.phy_type;
+ phy_conf.phy_type_ext = phy_ab.phy_type_ext;
+ phy_conf.fec_config = phy_ab.mod_type_ext;
phy_conf.eee_capability = phy_ab.eee_capability;
phy_conf.eeer = phy_ab.eeer_val;
phy_conf.low_power_ctrl = phy_ab.d3_lpan;
@@ -1653,8 +1655,7 @@ i40e_apply_link_speed(struct rte_eth_dev *dev)
struct rte_eth_conf *conf = &dev->data->dev_conf;
speed = i40e_parse_link_speeds(conf->link_speeds);
- if (!I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
- abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
+ abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
if (!(conf->link_speeds & ETH_LINK_SPEED_FIXED))
abilities |= I40E_AQ_PHY_AN_ENABLED;
abilities |= I40E_AQ_PHY_LINK_ENABLED;
@@ -1990,8 +1991,7 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev)
uint8_t abilities = 0;
struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
- if (!I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
- abilities = I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
+ abilities = I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
return i40e_phy_conf_link(hw, abilities, speed);
}
--
2.7.4
^ permalink raw reply related
* [PATCH v1] examples/ethtool: fix segfault querying non-PCI devices
From: Remy Horton @ 2016-11-30 2:47 UTC (permalink / raw)
To: dev
Doing a device information query on a non-PCI device such as
vhost was resulting in the dereferencing of a NULL pointer
(the absent PCI data), causing a segmentation fault.
Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
Signed-off-by: Remy Horton <remy.horton@intel.com>
---
doc/guides/rel_notes/release_17_02.rst | 3 +++
examples/ethtool/lib/rte_ethtool.c | 13 +++++++++----
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/doc/guides/rel_notes/release_17_02.rst b/doc/guides/rel_notes/release_17_02.rst
index 3b65038..f471e49 100644
--- a/doc/guides/rel_notes/release_17_02.rst
+++ b/doc/guides/rel_notes/release_17_02.rst
@@ -70,6 +70,9 @@ Libraries
Examples
~~~~~~~~
+ * **examples/ethtool Fixed crash with non-PCI devices.**
+ Querying a non-PCI device was dereferencing non-existent PCI data
+ resulting in a segmentation fault.
Other
~~~~~
diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
index a1f91d4..6f0ce84 100644
--- a/examples/ethtool/lib/rte_ethtool.c
+++ b/examples/ethtool/lib/rte_ethtool.c
@@ -61,10 +61,15 @@ rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo)
dev_info.driver_name);
snprintf(drvinfo->version, sizeof(drvinfo->version), "%s",
rte_version());
- snprintf(drvinfo->bus_info, sizeof(drvinfo->bus_info),
- "%04x:%02x:%02x.%x",
- dev_info.pci_dev->addr.domain, dev_info.pci_dev->addr.bus,
- dev_info.pci_dev->addr.devid, dev_info.pci_dev->addr.function);
+ if (dev_info.pci_dev)
+ snprintf(drvinfo->bus_info, sizeof(drvinfo->bus_info),
+ "%04x:%02x:%02x.%x",
+ dev_info.pci_dev->addr.domain,
+ dev_info.pci_dev->addr.bus,
+ dev_info.pci_dev->addr.devid,
+ dev_info.pci_dev->addr.function);
+ else
+ snprintf(drvinfo->bus_info, sizeof(drvinfo->bus_info), "N/A");
memset(®_info, 0, sizeof(reg_info));
rte_eth_dev_get_reg_info(port_id, ®_info);
--
2.5.5
^ permalink raw reply related
* Re: [PATCH 1/1] net/i40e: enable auto link update for XXV710
From: Zhang, Qi Z @ 2016-11-30 2:26 UTC (permalink / raw)
To: Yigit, Ferruh, Wu, Jingjing, Zhang, Helin; +Cc: dev@dpdk.org
In-Reply-To: <2219dc42-a414-0e23-6f77-b3706949af8e@intel.com>
Hi Ferruh:
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, November 30, 2016 1:46 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/1] net/i40e: enable auto link update for
> XXV710
>
> Hi Qi,
>
> On 11/24/2016 11:43 PM, Qi Zhang wrote:
> > This patch remove the limitation that XXV710 device does
>
> XXV710 is 25G device, and support added in 16.11 (please correct me if this is
> wrong.), but I can't find any DPDK documentation for this device.
>
> Can you please add some documentation, at least to http://dpdk.org/doc/nics
> and http://dpdk.org/doc/guides/nics/i40e.html
> in a different patch?
For 16.11, since XXV710 is not officially supported, so they are missing in document.
For 17.02 this will be updated. Thanks for remind.
>
> > not support auto link update.
>
> Can you please add more details that why we can remove the limitation now?
Ok, will update in v2.
>
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> > drivers/net/i40e/i40e_ethdev.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 67778ba..b7a916d 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -1628,6 +1628,8 @@ i40e_phy_conf_link(struct i40e_hw *hw,
> >
> > /* use get_phy_abilities_resp value for the rest */
> > phy_conf.phy_type = phy_ab.phy_type;
> > + phy_conf.phy_type_ext = phy_ab.phy_type_ext;
> > + phy_conf.fec_config = phy_ab.mod_type_ext;
>
> And these changes look like called for all device types, just to double check, are
> these 25G specific?
Actually only XXV710 need this two lines, but base on firmware engineer's input,
this could be implemented in generic way since no impact for other i40e devices.
>
> > phy_conf.eee_capability = phy_ab.eee_capability;
> > phy_conf.eeer = phy_ab.eeer_val;
> > phy_conf.low_power_ctrl = phy_ab.d3_lpan; @@ -1653,8 +1655,7 @@
> > i40e_apply_link_speed(struct rte_eth_dev *dev)
> > struct rte_eth_conf *conf = &dev->data->dev_conf;
> >
> > speed = i40e_parse_link_speeds(conf->link_speeds);
> > - if (!I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
> > - abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> > + abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> > if (!(conf->link_speeds & ETH_LINK_SPEED_FIXED))
> > abilities |= I40E_AQ_PHY_AN_ENABLED;
> > abilities |= I40E_AQ_PHY_LINK_ENABLED; @@ -1990,8 +1991,7 @@
> > i40e_dev_set_link_down(struct rte_eth_dev *dev)
> > uint8_t abilities = 0;
> > struct i40e_hw *hw =
> I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >
> > - if (!I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
> > - abilities = I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> > + abilities = I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> > return i40e_phy_conf_link(hw, abilities, speed); }
> >
> >
Thanks!
Qi
^ permalink raw reply
* [PATCH v2] net/i40evf: fix casting between structs
From: Jingjing Wu @ 2016-11-30 2:02 UTC (permalink / raw)
To: dev; +Cc: jingjing.wu, helin.zhang
In-Reply-To: <1480239317-7827-1-git-send-email-jingjing.wu@intel.com>
Casting from structs which lay out data in typed members
to structs which have flat memory buffers, will cause
problems if the alignment of the former isn't as expected.
This patch removes the casting between structs.
Fixes: ae19955e7c86 ("i40evf: support reporting PF reset")
Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
v2 change:
correct the arguments in log.
add more comments.
drivers/net/i40e/i40e_ethdev_vf.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index aa306d6..5ec5264 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1336,8 +1336,9 @@ i40evf_handle_aq_msg(struct rte_eth_dev *dev)
struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
struct i40e_arq_event_info info;
- struct i40e_virtchnl_msg *v_msg;
- uint16_t pending, opcode;
+ uint16_t pending, aq_opc;
+ enum i40e_virtchnl_ops msg_opc;
+ enum i40e_status_code msg_ret;
int ret;
info.buf_len = I40E_AQ_BUF_SZ;
@@ -1346,7 +1347,6 @@ i40evf_handle_aq_msg(struct rte_eth_dev *dev)
return;
}
info.msg_buf = vf->aq_resp;
- v_msg = (struct i40e_virtchnl_msg *)&info.desc;
pending = 1;
while (pending) {
@@ -1357,32 +1357,39 @@ i40evf_handle_aq_msg(struct rte_eth_dev *dev)
"ret: %d", ret);
break;
}
- opcode = rte_le_to_cpu_16(info.desc.opcode);
-
- switch (opcode) {
+ aq_opc = rte_le_to_cpu_16(info.desc.opcode);
+ /* For the message sent from pf to vf, opcode is stored in
+ * cookie_high of struct i40e_aq_desc, while return error code
+ * are stored in cookie_low, Which is done by
+ * i40e_aq_send_msg_to_vf in PF driver.*/
+ msg_opc = (enum i40e_virtchnl_ops)rte_le_to_cpu_32(
+ info.desc.cookie_high);
+ msg_ret = (enum i40e_status_code)rte_le_to_cpu_32(
+ info.desc.cookie_low);
+ switch (aq_opc) {
case i40e_aqc_opc_send_msg_to_vf:
- if (v_msg->v_opcode == I40E_VIRTCHNL_OP_EVENT)
+ if (msg_opc == I40E_VIRTCHNL_OP_EVENT)
/* process event*/
i40evf_handle_pf_event(dev, info.msg_buf,
info.msg_len);
else {
/* read message and it's expected one */
- if (v_msg->v_opcode == vf->pend_cmd) {
- vf->cmd_retval = v_msg->v_retval;
+ if (msg_opc == vf->pend_cmd) {
+ vf->cmd_retval = msg_ret;
/* prevent compiler reordering */
rte_compiler_barrier();
_clear_cmd(vf);
} else
PMD_DRV_LOG(ERR, "command mismatch,"
"expect %u, get %u",
- vf->pend_cmd, v_msg->v_opcode);
+ vf->pend_cmd, msg_opc);
PMD_DRV_LOG(DEBUG, "adminq response is received,"
- " opcode = %d\n", v_msg->v_opcode);
+ " opcode = %d\n", msg_opc);
}
break;
default:
PMD_DRV_LOG(ERR, "Request %u is not supported yet",
- opcode);
+ aq_opc);
break;
}
}
--
2.4.11
^ permalink raw reply related
* Re: [PATCH v2] i40e: Fix eth_i40e_dev_init sequence on ThunderX
From: Asok Tiyyagura @ 2016-11-30 1:23 UTC (permalink / raw)
To: konstantin.ananyev@intel.com
Cc: skoteshwar@caviumnetworks.com, helin.zhang@intel.com,
jingjing.wu@intel.com, jerin.jacob@caviumnetworks.com,
jianbo.liu@linaro.org, dev@dpdk.org
In-Reply-To: <mailman.4.1479684107.2722.dev@dpdk.org>
I can confirm we ran into a similar issue while using ixgbe.
-Asok
Hi
>
> i40e_asq_send_command: rd32 & wr32 under ThunderX gives unpredictable
> results. To solve this include rte memory barriers
>
> Signed-off-by: Satha Rao <skoteshwar@caviumnetworks.com>
> ---
> drivers/net/i40e/base/i40e_osdep.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
> index 38e7ba5..ffa3160 100644
> --- a/drivers/net/i40e/base/i40e_osdep.h
> +++ b/drivers/net/i40e/base/i40e_osdep.h
> @@ -158,7 +158,13 @@ do { \
> ((volatile uint32_t *)((char *)(a)->hw_addr + (reg)))
> static inline uint32_t i40e_read_addr(volatile void *addr)
> {
> +#if defined(RTE_ARCH_ARM64)
> + uint32_t val = rte_le_to_cpu_32(I40E_PCI_REG(addr));
> + rte_rmb();
> + return val;
If you really need an rmb/wmb with MMIO read/writes on ARM,
I think you can avoid #ifdefs here and use rte_smp_rmb/rte_smp_wmb.
BTW, I suppose if you need it for i40e, you would need it for other devices too.
Konstantin
> +#else
> return rte_le_to_cpu_32(I40E_PCI_REG(addr));
> +#endif
> }
> #define I40E_PCI_REG_WRITE(reg, value) \
> do { I40E_PCI_REG((reg)) = rte_cpu_to_le_32(value); } while (0)
> @@ -171,8 +177,16 @@ static inline uint32_t i40e_read_addr(volatile void *addr)
> I40E_PCI_REG_WRITE(I40E_PCI_REG_ADDR((hw), (reg)), (value))
>
> #define rd32(a, reg) i40e_read_addr(I40E_PCI_REG_ADDR((a), (reg)))
> +#if defined(RTE_ARCH_ARM64)
> +#define wr32(a, reg, value) \
> + do { \
> + I40E_PCI_REG_WRITE(I40E_PCI_REG_ADDR((a), (reg)), (value)); \
> + rte_wmb(); \
> + } while (0)
> +#else
> #define wr32(a, reg, value) \
> I40E_PCI_REG_WRITE(I40E_PCI_REG_ADDR((a), (reg)), (value))
> +#endif
> #define flush(a) i40e_read_addr(I40E_PCI_REG_ADDR((a), (I40E_GLGEN_STAT)))
>
> #define ARRAY_SIZE(arr) (sizeof(arr)/sizeof(arr[0]))
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH] net/i40evf: fix casting between structs
From: Wu, Jingjing @ 2016-11-30 0:35 UTC (permalink / raw)
To: Yigit, Ferruh, dev@dpdk.org; +Cc: Zhang, Helin
In-Reply-To: <2c330ced-2d11-446e-0e2b-c6dd5aa49f8b@intel.com>
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, November 30, 2016 12:08 AM
> To: Wu, Jingjing <jingjing.wu@intel.com>; dev@dpdk.org
> Cc: Zhang, Helin <helin.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/i40evf: fix casting between structs
>
> On 11/27/2016 9:35 AM, Jingjing Wu wrote:
> > Casting from structs which lay out data in typed members to structs
> > which have flat memory buffers, will cause problems if the alignment
> > of the former isn't as expected.
> > This patch removes the casting between structs.
> >
> > Fixes: ae19955e7c86 ("i40evf: support reporting PF reset")
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > ---
> > drivers/net/i40e/i40e_ethdev_vf.c | 27 +++++++++++++++------------
> > 1 file changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index aa306d6..53d7c87 100644
> > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > @@ -1336,8 +1336,9 @@ i40evf_handle_aq_msg(struct rte_eth_dev *dev)
> > struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> > struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
> >dev_private);
> > struct i40e_arq_event_info info;
> > - struct i40e_virtchnl_msg *v_msg;
> > - uint16_t pending, opcode;
> > + uint16_t pending, aq_opc;
> > + enum i40e_virtchnl_ops msg_opc;
> > + enum i40e_status_code msg_ret;
> > int ret;
> >
> > info.buf_len = I40E_AQ_BUF_SZ;
> > @@ -1346,7 +1347,6 @@ i40evf_handle_aq_msg(struct rte_eth_dev *dev)
> > return;
> > }
> > info.msg_buf = vf->aq_resp;
> > - v_msg = (struct i40e_virtchnl_msg *)&info.desc;
> >
> > pending = 1;
> > while (pending) {
> > @@ -1357,32 +1357,35 @@ i40evf_handle_aq_msg(struct rte_eth_dev *dev)
> > "ret: %d", ret);
> > break;
> > }
> > - opcode = rte_le_to_cpu_16(info.desc.opcode);
> > -
> > - switch (opcode) {
> > + aq_opc = rte_le_to_cpu_16(info.desc.opcode);
> > + msg_opc = (enum i40e_virtchnl_ops)rte_le_to_cpu_32(
> > + info.desc.cookie_high);
> > + msg_ret = (enum i40e_status_code)rte_le_to_cpu_32(
> > + info.desc.cookie_low);
>
> What do you think commenting cookie_high is opcode and cookie_low is
> return_value?
>
OK. Will add some comments.
> > + switch (aq_opc) {
> > case i40e_aqc_opc_send_msg_to_vf:
> > - if (v_msg->v_opcode == I40E_VIRTCHNL_OP_EVENT)
> > + if (msg_opc == I40E_VIRTCHNL_OP_EVENT)
> > /* process event*/
> > i40evf_handle_pf_event(dev, info.msg_buf,
> > info.msg_len);
> > else {
> > /* read message and it's expected one */
> > - if (v_msg->v_opcode == vf->pend_cmd) {
> > - vf->cmd_retval = v_msg->v_retval;
> > + if (msg_opc == vf->pend_cmd) {
> > + vf->cmd_retval = msg_ret;
> > /* prevent compiler reordering */
> > rte_compiler_barrier();
> > _clear_cmd(vf);
> > } else
> > PMD_DRV_LOG(ERR, "command
> mismatch,"
> > "expect %u, get %u",
> > - vf->pend_cmd, v_msg-
> >v_opcode);
> > + vf->pend_cmd, msg_ret);
>
> s/msg_ret/msg_opc/ ?
Yes, should use msg_opc here. Thanks!
Will update!
Thanks
Jingjing
^ permalink raw reply
* Re: [PATCH v9] drivers/net:new PMD using tun/tap host interface
From: Wiles, Keith @ 2016-11-29 22:16 UTC (permalink / raw)
To: Aws Ismail; +Cc: Yigit, Ferruh, DPDK
In-Reply-To: <CAMHMu0OrRz130WSE+uVR13o8t-5xqNS9xFv3NBg98smFBbxX2g@mail.gmail.com>
> On Nov 29, 2016, at 3:36 PM, Aws Ismail <aws.ismail@gmail.com> wrote:
>
> I have verified that adding just a single tap device works with testpmd. But as soon as I try more than one tap device, I would get a coredump, e.g.:
>
> root@localhost:~# testpmd -c f -n 4 --socket-mem 512 --vdev=net_tap,iface=tap0 --vdev=net_tap,iface=tap1 -- -i
> EAL: Detected 16 lcore(s)
> EAL: Probing VFIO support...
> EAL: VFIO support initialized
> EAL: cannot open /proc/self/numa_maps, consider that all memory is in socket_id 0
> PMD: Initializing pmd_tap for net_tap as dtap0
> PMD: net_tap: Create TAP Ethernet device with 32 queues on numa 0
> PMD: Initializing pmd_tap for net_tap as dtap1
> PMD: net_tap: Create TAP Ethernet device with 32 queues on numa 0
> EAL: failed to initialize net_tap device
> PANIC in rte_eal_init():
> Cannot init pmd devices
> 6: [testpmd() [0x409149]]
> 5: [/lib/libc.so.6(__libc_start_main+0xf0) [0x7f3e65fa8740]]
> 4: [testpmd() [0x408b21]]
> 3: [/usr/lib/librte_eal.so.3.1(rte_eal_init+0xe09) [0x7f3e68ceaea9]]
> 2: [/usr/lib/librte_eal.so.3.1(__rte_panic+0xc0) [0x7f3e68ce9b5a]]
> 1: [/usr/lib/librte_eal.so.3.1(rte_dump_stack+0x18) [0x7f3e68cf2078]]
It appears the call to rte_eth_dev_allocate() in the code is wrong. I did pass in the variable called tap_name created in the function, but I was told I needed to pass in ‘name’ that was passed into the function. The rte_eth_dev_allocate() needs a unique name for each call and name is all was the same.
Need to change that line to use tap_name instead or tell me the real way to handle this problem.
If you want a new patch I can try to get it done, but I am working on something else at this time and it could be a few days before I can get the patch out.
> Aborted (core dumped)
>
> root@localhost:~#
>
>
> On Fri, Nov 25, 2016 at 2:38 PM, Aws Ismail <aws.ismail@gmail.com> wrote:
> Keith,
>
> This won't build when integrated with v16.11. The register macro
> prefix has been renamed. a v10 is needed.
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 7f303db..297d4b6 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -752,5 +752,6 @@ static struct rte_vdev_driver pmd_tap_drv = {
> .remove = rte_pmd_tap_remove,
> };
>
> -DRIVER_REGISTER_VDEV(net_tap, pmd_tap_drv);
> -DRIVER_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
> +RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
> +RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
> +RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>
> On Mon, Nov 21, 2016 at 7:56 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > On 10/13/2016 11:03 PM, Keith Wiles wrote:
> >> The rte_eth_tap.c PMD creates a device using TUN/TAP interfaces
> >> on the local host. The PMD allows for DPDK and the host to
> >> communicate using a raw device interface on the host and in
> >> the DPDK application. The device created is a Tap device with
> >> a L2 packet header.
> >>
> >> v9 - Fix up the docs to use correct syntax
> >> v8 - Fix issue with tap_tx_queue_setup() not return zero on success.
> >> v7 - Reword the comment in common_base and fix the data->name issue
> >> v6 - fixed the checkpatch issues
> >> v5 - merge in changes from list review see related emails
> >> fixed many minor edits
> >> v4 - merge with latest driver changes
> >> v3 - fix includes by removing ifdef for other type besides Linux
> >> Fix the copyright notice in the Makefile
> >> v2 - merge all of the patches into one patch
> >> Fix a typo on naming the tap device
> >> Update the maintainers list
> >>
> >> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> >> ---
> >
> > Just a reminder, this is a new PMD and waiting for community review.
>
Regards,
Keith
^ permalink raw reply
* Re: [PATCH v9] drivers/net:new PMD using tun/tap host interface
From: Aws Ismail @ 2016-11-29 21:36 UTC (permalink / raw)
To: Keith Wiles; +Cc: Ferruh Yigit, DPDK
In-Reply-To: <CAMHMu0MzwExcOOqK51Z6kt=XJzWQ2J+jWZaJcCVb2_OZmtZhzw@mail.gmail.com>
I have verified that adding just a single tap device works with testpmd.
But as soon as I try more than one tap device, I would get a coredump, e.g.:
root@
localhost
:~# testpmd -c f -n 4 --socket-mem 512 --vdev=net_tap
,iface=tap0
--vdev=net_tap
,iface=tap1
-- -i
EAL: Detected 16 lcore(s)
EAL: Probing VFIO support...
EAL: VFIO support initialized
EAL: cannot open /proc/self/numa_maps, consider that all memory is in
socket_id 0
PMD: Initializing pmd_tap for net_tap as dtap0
PMD: net_tap: Create TAP Ethernet device with 32 queues on numa 0
PMD: Initializing pmd_tap for net_tap as dtap1
PMD: net_tap: Create TAP Ethernet device with 32 queues on numa 0
EAL: failed to initialize net_tap device
PANIC in rte_eal_init():
Cannot init pmd devices
6: [testpmd() [0x409149]]
5: [/lib/libc.so.6(__libc_start_main+0xf0) [0x7f3e65fa8740]]
4: [testpmd() [0x408b21]]
3: [/usr/lib/librte_eal.so.3.1(rte_eal_init+0xe09) [0x7f3e68ceaea9]]
2: [/usr/lib/librte_eal.so.3.1(__rte_panic+0xc0) [0x7f3e68ce9b5a]]
1: [/usr/lib/librte_eal.so.3.1(rte_dump_stack+0x18) [0x7f3e68cf2078]]
Aborted (core dumped)
root@
localhost
:~#
On Fri, Nov 25, 2016 at 2:38 PM, Aws Ismail <aws.ismail@gmail.com> wrote:
> Keith,
>
> This won't build when integrated with v16.11. The register macro
> prefix has been renamed. a v10 is needed.
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 7f303db..297d4b6 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -752,5 +752,6 @@ static struct rte_vdev_driver pmd_tap_drv = {
> .remove = rte_pmd_tap_remove,
> };
>
> -DRIVER_REGISTER_VDEV(net_tap, pmd_tap_drv);
> -DRIVER_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
> +RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
> +RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
> +RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>
> On Mon, Nov 21, 2016 at 7:56 AM, Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> > On 10/13/2016 11:03 PM, Keith Wiles wrote:
> >> The rte_eth_tap.c PMD creates a device using TUN/TAP interfaces
> >> on the local host. The PMD allows for DPDK and the host to
> >> communicate using a raw device interface on the host and in
> >> the DPDK application. The device created is a Tap device with
> >> a L2 packet header.
> >>
> >> v9 - Fix up the docs to use correct syntax
> >> v8 - Fix issue with tap_tx_queue_setup() not return zero on success.
> >> v7 - Reword the comment in common_base and fix the data->name issue
> >> v6 - fixed the checkpatch issues
> >> v5 - merge in changes from list review see related emails
> >> fixed many minor edits
> >> v4 - merge with latest driver changes
> >> v3 - fix includes by removing ifdef for other type besides Linux
> >> Fix the copyright notice in the Makefile
> >> v2 - merge all of the patches into one patch
> >> Fix a typo on naming the tap device
> >> Update the maintainers list
> >>
> >> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> >> ---
> >
> > Just a reminder, this is a new PMD and waiting for community review.
>
^ permalink raw reply
* Re: [PATCH] doc: remove wrong document description
From: Thomas Monjalon @ 2016-11-29 20:38 UTC (permalink / raw)
To: Baruch Siach; +Cc: dev, Mcnamara, John
In-Reply-To: <20161129105421.lzcgiuygbbjabaur@tarshish>
2016-11-29 12:54, Baruch Siach:
> On Tue, Nov 29, 2016 at 09:54:58AM +0000, Mcnamara, John wrote:
> > Acked-by: John McNamara <john.mcnamara@intel.com>
>
> Thanks. For some reason patchwork didn't get your ack.
That's because you are not registered to the mailing list so your email
was in a moderation queue while John sent his ack.
You should register to avoid moderation and such issues:
http://dpdk.org/ml/listinfo/dev
^ permalink raw reply
* Re: Proposal for a new Committer model
From: Neil Horman @ 2016-11-29 19:12 UTC (permalink / raw)
To: Mcnamara, John; +Cc: dev
In-Reply-To: <20161118161025.GC29049@hmsreliant.think-freely.org>
On Fri, Nov 18, 2016 at 01:09:35PM -0500, Neil Horman wrote:
> On Thu, Nov 17, 2016 at 09:20:50AM +0000, Mcnamara, John wrote:
> > Repost from the moving at dpdk.org mailing list to get a wider audience.
> > Original thread: http://dpdk.org/ml/archives/moving/2016-November/000059.html
> >
> >
> > Hi,
> >
> > I'd like to propose a change to the DPDK committer model. Currently we have one committer for the master branch of the DPDK project.
> >
> > One committer to master represents a single point of failure and at times can be inefficient. There is also no agreed cover for times when the committer is unavailable such as vacation, public holidays, etc. I propose that we change to a multi-committer model for the DPDK project. We should have three committers for each release that can commit changes to the master branch.
> >
> > There are a number of benefits:
> >
> > 1. Greater capacity to commit patches.
> > 2. No single points of failure - a committer should always be available if we have three.
> > 3. A more timely committing of patches. More committers should equal a faster turnaround - ideally, maintainers should also provide feedback on patches submitted within a 2-3 day period, as much as possible, to facilitate this.
> > 4. It follows best practice in creating a successful multi-vendor community - to achieve this we must ensure there is a level playing field for all participants, no single person should be required to make all of the decisions on patches to be included in the release.
> >
> > Having multiple committers will require some degree of co-ordination but there are a number of other communities successfully following this model such as Apache, OVS, FD.io, OpenStack etc. so the approach is workable.
> >
> > John
>
> I agree that the problems you are attempting to address exist and are
> worth finding a solution for. That said, I don't think the solution you
> are proposing is the ideal, or complete fix for any of the issues being
> addressed.
>
> If I may, I'd like to ennumerate the issues I think you are trying to
> address based on your comments above, then make a counter-proposal for a
> solution:
>
> Problems to address:
>
> 1) high-availability - There is a desire to make sure that, when patches
> are proposed, they are integrated in a timely fashion.
>
> 2) high-throughput - DPDK has a large volume of patches, more than one
> person can normally integrate. There is a desire to shard that work such
> that it is handled by multiple individuals
>
> 3) Multi-Vendor fairness - There is a desire for multiple vendors to feel
> as though the project tree maintainer isn't biased toward any individual
> vendor.
>
> To solve these I would propose the following solution (which is simmilar
> to, but not quite identical, to yours).
>
> A) Further promote subtree maintainership. This was a conversation that I
> proposed some time ago, but my proposed granularity was discarded in favor
> of something that hasn't worked as well (in my opinion). That is to say a
> few driver pmds (i40e and fm10k come to mind) have their own tree that
> send pull requests to Thomas. We should be sharding that at a much higher
> granularity and using it much more consistently. That is to say, that we
> should have a maintainer for all the ethernet pmds, and another for the
> crypto pmds, another for the core eal layer, another for misc libraries
> that have low patch volumes, etc. Each of those subdivisions should have
> their own list to communicate on, and each should have a tree that
> integrates patches for their own subsystem, and they should on a regular
> cycle send pull requests to Thomas. Thomas in turn should by and large,
> only be integrating pull requests. This should address our high-
> throughput issue, in that it will allow multiple maintainers to share the
> workload, and integration should be relatively easy.
>
> B) Designate alternates to serve as backups for the maintainer when they
> are unavailable. This provides high-availablility, and sounds very much
> like your proposal, but in the interests of clarity, there is still a
> single maintainer at any one time, it just may change to ensure the
> continued merging of patches, if the primary maintainer isn't available.
> Ideally however, those backup alternates arent needed, because most of the
> primary maintainers work in merging pull requests, which are done based on
> the trust of the submaintainer, and done during a very limited window of
> time. This also partially addreses multi-vendor fairness if your subtree
> maintainers come from multiple participating companies.
>
> Regards
> Neil
>
>
>
Soo, I feel like we're wandering away from this thread. Are you going to make a
change to the comitter model?
Neil
^ permalink raw reply
* Re: [PATCH] dpdk: Fix abort on double free.
From: Daniele Di Proietto @ 2016-11-29 18:57 UTC (permalink / raw)
To: Aaron Conole, Ilya Maximets
Cc: dev@openvswitch.org, Dyasly Sergey, Heetae Ahn, dev@dpdk.org,
Kevin Traynor
In-Reply-To: <f7tinr6uvmx.fsf@redhat.com>
On 29/11/2016 07:57, "Aaron Conole" <aconole@redhat.com> wrote:
>Hi Ilya,
>
>Ilya Maximets <i.maximets@samsung.com> writes:
>
>> On 28.11.2016 21:55, Aaron Conole wrote:
>>> Ilya Maximets <i.maximets@samsung.com> writes:
>>>
>>>> According to DPDK API (lib/librte_eal/common/include/rte_eal.h):
>>>>
>>>> "After the call to rte_eal_init(), all arguments argv[x]
>>>> with x < ret may be modified and should not be accessed
>>>> by the application."
>>>>
>>>> This means, that OVS must not free the arguments passed to DPDK.
>>>> In real world, 'rte_eal_init()' replaces the last argument in
>>>> 'dpdk_argv' with the first one by doing this:
>>>
>>> Thanks for spotting this error, Ilya.
>>>
>>>> # eal_parse_args() from lib/librte_eal/linuxapp/eal/eal.c
>>>>
>>>> char *prgname = argv[0];
>>>> ...
>>>> if (optind >= 0)
>>>> argv[optind-1] = prgname;
>>>>
>>>> This leads to double free inside 'deferred_argv_release()' and
>>>> possible ABORT at exit:
>>>
>>> I haven't seen this, which is both shocking and scary - the commit which
>>> does this copy is almost 4 years old; did you have to do anything
>>> specific for this behavior to occur? Did something change in DPDK
>>> recently that exposed this behavior? Just wondering how you reproduced
>>> it.
>>
>> Abort was caught up accidentally. I'm able to reproduce it on my a
>> little unusual testing system (ARMv8 + Fedora 21 + clang 3.5) without
>> any specific manipulations. The bug exists always but it's hard
>> for libc to detect double free here because there are many other
>> frees/allocations at exit time. I've used following patch to confirm
>> the issue if it wasn't detected by libc:
>
>Well, it's at least good that you can observe it consistently. Did you
>try my provided patch to see if that works as well?
>
>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>> index 49a589a..65d2d28 100644
>> --- a/lib/dpdk.c
>> +++ b/lib/dpdk.c
>> @@ -258,6 +258,8 @@ deferred_argv_release(void)
>> {
>> int result;
>> for (result = 0; result < dpdk_argc; ++result) {
>> + VLOG_INFO("DPDK ARGV release: %2d: 0x%" PRIx64 " (%s)",
>> + result, (intptr_t)dpdk_argv[result], dpdk_argv[result]);
>> free(dpdk_argv[result]);
>> }
>>
>
>It's quite glaring after studying the code. Really good catch!
I agree, thanks for spotting this
>
>>>
>>>> *** Error in `ovs-vswitchd': double free or corruption (fasttop) <...> ***
>>>> Program received signal SIGABRT, Aborted.
>>>>
>>>> #0 raise () from /lib64/libc.so.6
>>>> #1 abort () from /lib64/libc.so.6
>>>> #2 __libc_message () from /lib64/libc.so.6
>>>> #3 free () from /lib64/libc.so.6
>>>> #4 deferred_argv_release () at lib/dpdk.c:261
>>>> #5 __run_exit_handlers () from /lib64/libc.so.6
>>>> #6 exit () from /lib64/libc.so.6
>>>> #7 __libc_start_main () from /lib64/libc.so.6
>>>> #8 _start ()
>>>>
>>>> Fix that by not calling free for the memory passed to DPDK.
>>>>
>>>> CC: Aaron Conole <aconole@redhat.com>
>>>> Fixes: bab694097133 ("netdev-dpdk: Convert initialization from
>>>> cmdline to db")
>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>> ---
>>>
>>> We need to free the memory - I think that is not a question;
>>
>> Actually, it is. According to DPDK API (see above) 'rte_eal_init()'
>> takes the ownership of 'argv'. This means that we must not free
>> or use this memory.
>
>Apologies for the ranty-wall of text below.
>
>DPDK *cannot* take ownership of freeing this memory, unless 1) it expects a
>completely separate array from argv/argc than the one passed during
>program execution and initialization, or 2) it expects the hosted
>environment to give it the responsibility of cleaning this up. It
>explicitly claims that the argv/argc is what comes from main(), and
>therefore should obey the restrictions and privileges afforded those
>variables.
>
>In fact, I don't even see anywhere that dpdk preserves argv, *at all*.
>Looking through the history very quickly (admittedly just back to commit
>af75078fece3615088e561357c1e97603e43a5fe in dpdk) confirms that dpdk
>hasn't stored the arguments anywhere to do any processing.
>
>DPDK api guide does NOT state that it takes possession - and that matches
>with what happens in the code, BUT I will agree the statement
>
> 'all arguments argv[x] with x < ret may be modified and should not be
> accessed by the application'
>
>is a bit ambiguous. I think it's trying to say that the application should do
>its getopt()s parsing before calling the dpdk init routine, because DPDK libs
>will change the array. I don't see a reason for modifying the array in
>the code (the `argv[optind-1] = progname`), but if the dpdk library wants
>to do that, it is free to do so according to C99 5.1.2.2.1; I think
>it's best we always free what we allocate, which is why I suggested the
>side array patch which stores additional pointers to the data to be
>free'd up at exit.
>
>I am not sure which is more appropriate, since this is an exit condition,
>after all. The memory will get free()d up eventually by the
>environments on which OvS runs. It doesn't _feel_ correct to leave the
>memory dangling, since we can free it.
>
>Anyone else have thoughts on this?
I don't think it's a big deal to leak memory that has to be used until the process
terminates. There are other examples of this in OvS, such as 'timewarp_seq' in
lib/timeval.c. They should be reported by valgrind as "still reachable".
That said, at some point we might want to have 100% leak free valgrind runs, so
I think it's be better to free everything we allocate, so I would prefer Aaron's
solution. I don't think DPDK should expect the arguments to be available in exit
handlers, i.e. after main() returns.
I don't feel strongly about it though, since, if I'm not mistaken, valgrind doesn't
support DPDK yet.
>
>> Some thoughts:
>> DPDK internally doesn't free this memory, but it's not the reason to
>> touch it from the outside. Actually, DPDK API change required here to
>> support freeing of this resources if needed. But until there is no
>> 'rte_eal_uninit()' such API change isn't actually useful.
>>
>> Also, I forget to remove the variables. So, the following incremental
>> to my original patch required:
>>
>> ------------------------------------
>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>> index 2014946..4201149 100644
>> --- a/lib/dpdk.c
>> +++ b/lib/dpdk.c
>> @@ -250,9 +250,6 @@ get_dpdk_args(const struct smap *ovs_other_config, char ***argv,
>> return i + extra_argc;
>> }
>>
>> -static char **dpdk_argv;
>> -static int dpdk_argc;
>> -
>> static void
>> dpdk_init__(const struct smap *ovs_other_config)
>> {
>> @@ -370,9 +367,6 @@ dpdk_init__(const struct smap *ovs_other_config)
>> }
>> }
>>
>> - dpdk_argv = argv;
>> - dpdk_argc = argc;
>> -
>> rte_memzone_dump(stdout);
>>
>> /* We are called from the main thread here */
>> ------------------------------------
>>
>> Best regards, Ilya Maximets.
^ permalink raw reply
* Re: [PATCH 1/1] net/i40e: enable auto link update for XXV710
From: Ferruh Yigit @ 2016-11-29 17:45 UTC (permalink / raw)
To: Qi Zhang, jingjing.wu, helin.zhang; +Cc: dev
In-Reply-To: <1480031006-15702-1-git-send-email-qi.z.zhang@intel.com>
Hi Qi,
On 11/24/2016 11:43 PM, Qi Zhang wrote:
> This patch remove the limitation that XXV710 device does
XXV710 is 25G device, and support added in 16.11 (please correct me if
this is wrong.), but I can't find any DPDK documentation for this device.
Can you please add some documentation, at least to
http://dpdk.org/doc/nics and http://dpdk.org/doc/guides/nics/i40e.html
in a different patch?
> not support auto link update.
Can you please add more details that why we can remove the limitation now?
>
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> drivers/net/i40e/i40e_ethdev.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 67778ba..b7a916d 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -1628,6 +1628,8 @@ i40e_phy_conf_link(struct i40e_hw *hw,
>
> /* use get_phy_abilities_resp value for the rest */
> phy_conf.phy_type = phy_ab.phy_type;
> + phy_conf.phy_type_ext = phy_ab.phy_type_ext;
> + phy_conf.fec_config = phy_ab.mod_type_ext;
And these changes look like called for all device types, just to double
check, are these 25G specific?
> phy_conf.eee_capability = phy_ab.eee_capability;
> phy_conf.eeer = phy_ab.eeer_val;
> phy_conf.low_power_ctrl = phy_ab.d3_lpan;
> @@ -1653,8 +1655,7 @@ i40e_apply_link_speed(struct rte_eth_dev *dev)
> struct rte_eth_conf *conf = &dev->data->dev_conf;
>
> speed = i40e_parse_link_speeds(conf->link_speeds);
> - if (!I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
> - abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> + abilities |= I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> if (!(conf->link_speeds & ETH_LINK_SPEED_FIXED))
> abilities |= I40E_AQ_PHY_AN_ENABLED;
> abilities |= I40E_AQ_PHY_LINK_ENABLED;
> @@ -1990,8 +1991,7 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev)
> uint8_t abilities = 0;
> struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>
> - if (!I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
> - abilities = I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> + abilities = I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> return i40e_phy_conf_link(hw, abilities, speed);
> }
>
>
^ permalink raw reply
* Re: [PATCH] i40evf: add set maximum frame size support
From: Ferruh Yigit @ 2016-11-29 16:24 UTC (permalink / raw)
To: Michael Bieniek, helin.zhang, jingjing.wu; +Cc: dev
In-Reply-To: <1480106855-3287-1-git-send-email-michaelbieniekdpdk@gmail.com>
On 11/25/2016 8:47 PM, Michael Bieniek wrote:
> This adds the ability to set maximum frame size for an i40e virtual
> interface. This patch is based on the i40e physical function maximum
> frame size implementation. This was tested on an system configured with
> multiple i40e virtual functions. Verified that the MTU was configurable
> and that sending packets greater than the configured MTU resulted in a
> drop.
>
> Signed-off-by: Michael Bieniek <michaelbieniekdpdk@gmail.com>
Hi Michael,
I guess this is the first patch, welcome to the DPDK community.
> ---
> drivers/net/i40e/i40e_ethdev_vf.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
> index aa306d6..8477c98 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -158,6 +158,7 @@ i40evf_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id);
> static void i40evf_handle_pf_event(__rte_unused struct rte_eth_dev *dev,
> uint8_t *msg,
> uint16_t msglen);
> +static int i40evf_dev_mtu_set(struct rte_eth_dev *dev, uint16_t);
This is not something functional but, in declaration, if you used
parameter name for first argument, let's use parameter name for second too.
>
> /* Default hash key buffer for RSS */
> static uint32_t rss_key_default[I40E_VFQF_HKEY_MAX_INDEX + 1];
> @@ -225,6 +226,7 @@ static const struct eth_dev_ops i40evf_eth_dev_ops = {
> .reta_query = i40evf_dev_rss_reta_query,
> .rss_hash_update = i40evf_dev_rss_hash_update,
> .rss_hash_conf_get = i40evf_dev_rss_hash_conf_get,
> + .mtu_set = i40evf_dev_mtu_set,
> };
>
> /*
> @@ -2635,3 +2637,34 @@ i40evf_dev_rss_hash_conf_get(struct rte_eth_dev *dev,
>
> return 0;
> }
> +
> +static int
> +i40evf_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
> +{
> + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
hw is not used, and causing a build error.
> + struct rte_eth_dev_data *dev_data = dev->data;
> + uint32_t frame_size = mtu + ETHER_HDR_LEN
> + + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> + int ret = 0;
> +
> + /* check if mtu is within the allowed range */
> + if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
> + return -EINVAL;
> +
> + /* mtu setting is forbidden if port is started */
> + if (dev_data->dev_started) {
> + PMD_DRV_LOG(ERR,
> + "port %d must be stopped before configuration\n",
> + dev_data->port_id);
> + return -EBUSY;
> + }
> +
> + if (frame_size > ETHER_MAX_LEN)
> + dev_data->dev_conf.rxmode.jumbo_frame = 1;
> + else
> + dev_data->dev_conf.rxmode.jumbo_frame = 0;
> +
> + dev_data->dev_conf.rxmode.max_rx_pkt_len = frame_size;
> +
> + return ret;
> +}
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox