* Re: [PATCH 3/3] net/virtio_user: fix wrongly set features
From: Yuanhan Liu @ 2016-12-08 8:32 UTC (permalink / raw)
To: Jianfeng Tan; +Cc: dev, ferruh.yigit, cunming.liang
In-Reply-To: <1480689075-66977-4-git-send-email-jianfeng.tan@intel.com>
On Fri, Dec 02, 2016 at 02:31:15PM +0000, Jianfeng Tan wrote:
> Before the commit 86d59b21468a ("net/virtio: support LRO"), features
> in virtio PMD, is decided and properly set at device initialization
> and will not be changed. But afterward, features could be changed in
> virtio_dev_configure(), and will be re-negotiated if it's changed.
>
> In virtio_user, host features is obtained at device initialization
> only once, but we did not store it. So the added feature bits in
> re-negotiation will fail.
I think you misunderstood the features negotiation logic here, since
the beginning.
The feature negotiation is a work of 3 parts (the virtio device, the
virtio driver and the vhost). Each part has it's own feature bits
to claim what kind of features it supports.
On the init stage, the driver will have a sync with the device, and
that's what the "get_features" and "set_features" for. Once they have
come to an agreement on the negotiated features, it will have a
last sync with the vhost backend, through the GET_FEATURES and
SET_FEATURES request.
Last, we would have a features bits that would work for all of the
3 parts.
That said, you should not mix the driver's features bits with the
device one. It's Okay to introduce host_features, but it should not
be in the driver layer, it should be in the device layer. And naming
it to something like "device_features" is better, IMO.
And maybe you should re-visit the whole virtio_user features negotiation
logic, to see what can be improved.
Besides, I would put the bug fixing patch in the first of a series,
so that it has minimal chance to introduce conflics while backporting
it to a stable branch.
--yliu
>
> Fixes: e9efa4d93821 ("net/virtio-user: add new virtual PCI driver")
>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +-
> drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
> drivers/net/virtio/virtio_user_ethdev.c | 4 ++--
> 3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 3aef5f6..a9157a9 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -224,7 +224,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> }
>
> if (vhost_call(dev->vid, VHOST_USER_GET_FEATURES,
> - &dev->features) < 0) {
> + &dev->host_features) < 0) {
> PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno));
> return -1;
> }
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index 80efb6e..d219432 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -47,6 +47,7 @@ struct virtio_user_dev {
> uint32_t queue_pairs;
> uint32_t queue_size;
> uint64_t features;
> + uint64_t host_features;
> uint8_t status;
> uint8_t mac_addr[ETHER_ADDR_LEN];
> char path[PATH_MAX];
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index 406beea..cfe2bfc 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -117,7 +117,7 @@ virtio_user_get_features(struct virtio_hw *hw)
> {
> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>
> - return dev->features;
> + return dev->host_features;
> }
>
> static void
> @@ -125,7 +125,7 @@ virtio_user_set_features(struct virtio_hw *hw, uint64_t features)
> {
> struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>
> - dev->features = features;
> + dev->features = features & dev->host_features;
> }
>
> static uint8_t
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH v2 0/5] example/ethtool: add bus info and fw version get
From: Remy Horton @ 2016-12-08 8:34 UTC (permalink / raw)
To: Qiming Yang, dev
In-Reply-To: <1481008582-69416-1-git-send-email-qiming.yang@intel.com>
On 06/12/2016 15:16, Qiming Yang wrote:
[..]
> Qiming Yang (5):
> ethdev: add firmware version get
> net/e1000: add firmware version get
> net/ixgbe: add firmware version get
> net/i40e: add firmware version get
> ethtool: dispaly bus info and firmware version
s/dispaly/display
doc/guides/rel_notes/release_17_02.rst ought to be updated as well. Code
itself looks ok though..
Acked-by: Remy Horton <remy.horton@intel.com>
^ permalink raw reply
* Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for mempool ops error
From: Hemant Agrawal @ 2016-12-08 8:57 UTC (permalink / raw)
To: Olivier Matz
Cc: dev@dpdk.org, jerin.jacob@caviumnetworks.com,
david.hunt@intel.com
In-Reply-To: <376759b0-5d49-b904-b793-4f907bea8537@6wind.com>
Hi Olivier,
Apology for a delayed response.
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, November 22, 2016 2:55 PM
> To: Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; david.hunt@intel.com
> Subject: Re: [PATCH v3 2/2] mempool: pktmbuf pool default fallback for
> mempool ops error
>
> Hi Hemant,
>
> Back on this topic, please see some comments below.
>
> On 11/07/2016 01:30 PM, Hemant Agrawal wrote:
> > Hi Olivier,
> >
> >> -----Original Message-----
> >> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >> Sent: Friday, October 14, 2016 5:41 PM
> >>> On 9/22/2016 6:42 PM, Hemant Agrawal wrote:
> >>>> Hi Olivier
> >>>>
> >>>> On 9/19/2016 7:27 PM, Olivier Matz wrote:
> >>>>> Hi Hemant,
> >>>>>
> >>>>> On 09/16/2016 06:46 PM, Hemant Agrawal wrote:
> >>>>>> In the rte_pktmbuf_pool_create, if the default external mempool
> >>>>>> is not available, the implementation can default to "ring_mp_mc",
> >>>>>> which is an software implementation.
> >>>>>>
> >>>>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >>>>>> ---
> >>>>>> Changes in V3:
> >>>>>> * adding warning message to say that falling back to default sw
> >>>>>> pool
> >>>>>> ---
> >>>>>> lib/librte_mbuf/rte_mbuf.c | 8 ++++++++
> >>>>>> 1 file changed, 8 insertions(+)
> >>>>>>
> >>>>>> diff --git a/lib/librte_mbuf/rte_mbuf.c
> >>>>>> b/lib/librte_mbuf/rte_mbuf.c index 4846b89..8ab0eb1 100644
> >>>>>> --- a/lib/librte_mbuf/rte_mbuf.c
> >>>>>> +++ b/lib/librte_mbuf/rte_mbuf.c
> >>>>>> @@ -176,6 +176,14 @@ rte_pktmbuf_pool_create(const char *name,
> >>>>>> unsigned n,
> >>>>>>
> >>>>>> rte_errno = rte_mempool_set_ops_byname(mp,
> >>>>>> RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
> >>>>>> +
> >>>>>> + /* on error, try falling back to the software based default
> >>>>>> pool */
> >>>>>> + if (rte_errno == -EOPNOTSUPP) {
> >>>>>> + RTE_LOG(WARNING, MBUF, "Default HW Mempool not
> supported. "
> >>>>>> + "falling back to sw mempool \"ring_mp_mc\"");
> >>>>>> + rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc",
> >>>>>> NULL);
> >>>>>> + }
> >>>>>> +
> >>>>>> if (rte_errno != 0) {
> >>>>>> RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> >>>>>> return NULL;
> >>>>>>
> >>>>>
> >>>>> Without adding a new method ".supported()", the first call to
> >>>>> rte_mempool_populate() could return the same error ENOTSUP. In
> >>>>> this case, it is still possible to fallback.
> >>>>>
> >>>> It will be bit late.
> >>>>
> >>>> On failure, than we have to set the default ops and do a goto
> >>>> before rte_pktmbuf_pool_init(mp, &mbp_priv);
> >>
> >> I still think we can do the job without adding the .supported() method.
> >> The following code is just an (untested) example:
> >>
> >> struct rte_mempool *
> >> rte_pktmbuf_pool_create(const char *name, unsigned n,
> >> unsigned cache_size, uint16_t priv_size, uint16_t data_room_size,
> >> int socket_id)
> >> {
> >> struct rte_mempool *mp;
> >> struct rte_pktmbuf_pool_private mbp_priv;
> >> unsigned elt_size;
> >> int ret;
> >> const char *ops[] = {
> >> RTE_MBUF_DEFAULT_MEMPOOL_OPS, "ring_mp_mc", NULL,
> >> };
> >> const char **op;
> >>
> >> if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
> >> RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
> >> priv_size);
> >> rte_errno = EINVAL;
> >> return NULL;
> >> }
> >> elt_size = sizeof(struct rte_mbuf) + (unsigned)priv_size +
> >> (unsigned)data_room_size;
> >> mbp_priv.mbuf_data_room_size = data_room_size;
> >> mbp_priv.mbuf_priv_size = priv_size;
> >>
> >> for (op = &ops[0]; *op != NULL; op++) {
> >> mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
> >> sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
> >> if (mp == NULL)
> >> return NULL;
> >>
> >> ret = rte_mempool_set_ops_byname(mp, *op, NULL);
> >> if (ret != 0) {
> >> RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> >> rte_mempool_free(mp);
> >> if (ret == -ENOTSUP)
> >> continue;
> >> rte_errno = -ret;
> >> return NULL;
> >> }
> >> rte_pktmbuf_pool_init(mp, &mbp_priv);
> >>
> >> ret = rte_mempool_populate_default(mp);
> >> if (ret < 0) {
> >> rte_mempool_free(mp);
> >> if (ret == -ENOTSUP)
> >> continue;
> >> rte_errno = -ret;
> >> return NULL;
> >> }
> >> }
> >>
> >> rte_mempool_obj_iter(mp, rte_pktmbuf_init, NULL);
> >>
> >> return mp;
> >> }
> >>
> >>
> > [Hemant] This look fine to me. Please submit a patch for the same.
> >
> >>>>> I've just submitted an RFC, which I think is quite linked:
> >>>>> http://dpdk.org/ml/archives/dev/2016-September/046974.html
> >>>>> Assuming a new parameter "mempool_ops" is added to
> >>>>> rte_pktmbuf_pool_create(), would it make sense to fallback to
> >>>>> "ring_mp_mc"? What about just returning ENOTSUP? The application
> >>>>> could do the job and decide which sw fallback to use.
> >>>>
> >>>> We ran into this issue when trying to run the standard DPDK
> >>>> examples
> >>>> (l3fwd) in VM. Do you think, is it practical to add fallback
> >>>> handling in each of the DPDK examples?
> >>
> >> OK. What is still unclear for me, is how the software is aware of the
> >> different hardware-assisted handlers. Moreover, we could imagine more
> >> software handlers, which could be used depending on the use case.
> >>
> >> I think this choice has to be made by the user or the application:
> >>
> >> - the application may want to use a specific (sw or hw) handler: in
> >> this case, it want to be notified if it fails, instead of having
> >> a quiet fallback to ring_mp_mc
> >> - if several handlers are available, the application may want to
> >> try them in a specific order
> >> - maybe some handlers will have some limitations with some
> >> configurations or driver? The application could decide to use
> >> a different handler according the configuration.
> >>
> >> So that's why I think this is an application decision.
> >>
> > [Hemant] We should simplify it: if the application has supplied the handler, it
> is application's responsibility to take care of failure. Only if the application want
> to use the default handler, the implementation can fallback. The fallback
> handler can again be configurable.
>
> Honestly, I'm not very convinced that having a quiet fallback is the proper
> solution: the application won't be notified that the fallback occured.
>
> I understand your patch solves your use-case, but my fear is that we just
> integrate this minimal patch and never do the harder work of solving all the use-
> cases. I think we need to give the tools to the applications to control what
> occurs because we also need to solve these issues:
> - you have a hardware handler, but you want to use the software
> handler
> - you have several software handlers, and you (as an administrator) or
> the application knows which one is the best to use in your case.
>
> An alternative (which I don't like either) to solve your issue without modifying
> the mbuf code is to have your own handler fallbacking to ring_mp_mc.
>
[Hemant] are you suggesting that we also configure a fallback handler in addition to default?
I agree that we need to provide full flexibility to the applications, who are willing to implement the fallback mechanism.
>
> >>>> Typically when someone is writing a application on host, he need
> >>>> not worry non-availability of the hw offloaded mempool. He may also
> >>>> want to run the same binary in virtual machine. In VM, it is not
> >>>> guaranteed that hw offloaded mempools will be available.
> >>
> >> Running the same binary is of course a need. But if your VM does not
> >> provide the same virtualized hardware than the host, I think the
> >> command line option makes sense.
> >>
> >> AFAIU, on the host, you can use a hw mempool handler or a sw one, and
> >> on the guest, only a sw one. So you need an option to select the
> >> behavior you want on the host, without recompiling.
> >>
> >> I understand that modifying all the applications is not a good option
> >> either. I'm thinking a a EAL parameter that would allow to configure
> >> a library (mbuf in this case). Something like kernel boot options.
> >> Example: testpmd -l 2,4 --opt mbuf.handler="ring_mp_mc" -- [app args]
> >>
> >> I don't know if this is feasible, this has to be discussed first, but
> >> what do you think on the principle? The value could also be an
> >> ordered list if we want a fallback, and this option could be
> >> overriden by the application before creating the mbuf pool. There was
> >> some discussions about a kind of dpdk global configuration some time ago.
> >>
> > [Hemant] I agree that command line option provide a better control in this
> case.
> > On the flipside, We need to be careful that we do not end up having too many
> command line options.
>
> Yes, we should avoid having too many command line arguments.
> I think we should go in the direction of having a sort of key/value database in
> EAL. It could be set by:
> - a generic command line argument
> - a config file (maybe later)
> - the application through an API
>
> Then, it would be used by:
> - dpdk libraries
> - the application (maybe)
>
> This has been discussed some times, the latest was probably:
> http://dpdk.org/ml/archives/dev/2016-June/040079.html
>
> I think it would be a good tool for dpdk configurability, and it would help to
> remove some compile-time options and maybe some eal options.
> Unfortunately, I don't have the time to work on this at the moment.
>
> Regards,
> Olivier
^ permalink raw reply
* Re: [PATCH 01/22] ethdev: introduce generic flow API
From: Xing, Beilei @ 2016-12-08 9:00 UTC (permalink / raw)
To: Adrien Mazarguil, dev@dpdk.org
Cc: Thomas Monjalon, De Lara Guarch, Pablo, Olivier Matz
In-Reply-To: <1c8a8e4fec73ed33836f1da9525b1b8b53048518.1479309720.git.adrien.mazarguil@6wind.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Thursday, November 17, 2016 12:23 AM
> To: dev@dpdk.org
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; De Lara Guarch,
> Pablo <pablo.de.lara.guarch@intel.com>; Olivier Matz
> <olivier.matz@6wind.com>
> Subject: [dpdk-dev] [PATCH 01/22] ethdev: introduce generic flow API
>
> This new API supersedes all the legacy filter types described in rte_eth_ctrl.h.
> It is slightly higher level and as a result relies more on PMDs to process and
> validate flow rules.
>
> Benefits:
>
> - A unified API is easier to program for, applications do not have to be
> written for a specific filter type which may or may not be supported by
> the underlying device.
>
> - The behavior of a flow rule is the same regardless of the underlying
> device, applications do not need to be aware of hardware quirks.
>
> - Extensible by design, API/ABI breakage should rarely occur if at all.
>
> - Documentation is self-standing, no need to look up elsewhere.
>
> Existing filter types will be deprecated and removed in the near future.
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
> MAINTAINERS | 4 +
> lib/librte_ether/Makefile | 3 +
> lib/librte_ether/rte_eth_ctrl.h | 1 +
> lib/librte_ether/rte_ether_version.map | 10 +
> lib/librte_ether/rte_flow.c | 159 +++++
> lib/librte_ether/rte_flow.h | 947 ++++++++++++++++++++++++++++
> lib/librte_ether/rte_flow_driver.h | 177 ++++++
> 7 files changed, 1301 insertions(+)
>
> +/**
> + * RTE_FLOW_ITEM_TYPE_ETH
> + *
> + * Matches an Ethernet header.
> + */
> +struct rte_flow_item_eth {
> + struct ether_addr dst; /**< Destination MAC. */
> + struct ether_addr src; /**< Source MAC. */
> + unsigned int type; /**< EtherType. */
Hi Adrien,
ETHERTYPE in ether header is 2 bytes, so I think "uint16_t type" is more appropriate here, what do you think?
Thanks,
Beilei Xing
> +};
> +
^ permalink raw reply
* Re: [PATCH] Force python scripts to run with python2
From: Mcnamara, John @ 2016-12-08 9:07 UTC (permalink / raw)
To: Martin Kletzander, Thomas Monjalon; +Cc: dev@dpdk.org
In-Reply-To: <20161208083941.GA20753@wheatley>
> -----Original Message-----
> From: Martin Kletzander [mailto:mkletzan@redhat.com]
> Sent: Thursday, December 8, 2016 8:40 AM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] Force python scripts to run with python2
>
> On Wed, Dec 07, 2016 at 03:48:37PM +0100, Thomas Monjalon wrote:
> >2016-12-07 12:04, Mcnamara, John:
> >> > -----Original Message-----
> >> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Martin
> >> > Kletzander
> >> > Sent: Wednesday, December 7, 2016 10:17 AM
> >> > To: dev@dpdk.org
> >> > Subject: [dpdk-dev] [PATCH] Force python scripts to run with
> >> > python2
> >> >
> >> > With python3 being the default in some distributions/installations,
> >> > shebang with just "python" will make the script run under version
> >> > of python that the scripts are not written for. In order to fix
> >> > that and mitigate future errors, use shebang properly and specify
> >> > the python version. That way the scripts will run in any
> >> > distro/install, no matter what python version is set as the default.
> >>
> >> I think a better approach would be to make the scripts Python 2 and
> Python 3 compatible.
> >>
> >> Some of the new ones already are.
> >
> >Yes
> >
> >The "solution" using python2 in the shebang does not work everywhere
> >because python2 can be an unknown command.
>
> I have not come across a system that would not have python2 as at least a
> symlink or an executable. However if that is the concern, I can have a
> look at making all scripts python3 compatible, although I'm not yet
> familiar enough with DPDK to be able to properly test all of them. So
> I'll try and ask if needed.
>
> Thanks for the input.
>
Hi Martin,
I'll work on adding Python2/3 compatibility, and while I'm at it PEP8 compatibility.
John
^ permalink raw reply
* Re: [PATCH v2 15/32] net/i40e: add VF vlan strip func
From: Chen, Jing D @ 2016-12-08 9:10 UTC (permalink / raw)
To: Yigit, Ferruh, Lu, Wenzhuo, dev@dpdk.org
In-Reply-To: <f89ba10a-0267-7330-fa98-379740d85dc0@intel.com>
HI, Ferruh,
Best Regards,
Mark
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 07, 2016 10:18 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Cc: Chen, Jing D <jing.d.chen@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 15/32] net/i40e: add VF vlan strip func
>
> On 12/7/2016 3:31 AM, Wenzhuo Lu wrote:
> > Add a function to configure vlan strip enable/disable for specific
> > SRIOV VF device.
> >
> > Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
> > ---
>
> <...>
>
> > +
> > +/* Set vlan strip on/off for specific VF from host */
> > +int
> > +rte_pmd_i40e_set_vf_vlan_stripq(uint8_t port, uint16_t vf_id, uint8_t on)
> > +{
> > + struct rte_eth_dev *dev;
> > + struct i40e_pf *pf;
> > + struct i40e_vsi *vsi;
> > +
> > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > +
> > + dev = &rte_eth_devices[port];
> > + pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > +
> > + if (vf_id > pf->vf_num - 1 || !pf->vfs) {
> > + PMD_DRV_LOG(ERR, "Invalid argument.");
> > + return -EINVAL;
> > + }
> > +
> > + vsi = pf->vfs[vf_id].vsi;
> > +
> > + if (vsi)
> > + return i40e_vsi_config_vlan_stripping(vsi, !!on);
> > + else
>
> if vd_if is valid, can vsi be NULL? If so this check may be required in
> some prev patches too.
It's a little impossible. This sanity check just make the code stronger.
>
> > + return -EINVAL;
> > +}
> > diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h
> > index ca5e05a..043ae62 100644
> > --- a/drivers/net/i40e/rte_pmd_i40e.h
> > +++ b/drivers/net/i40e/rte_pmd_i40e.h
> > @@ -187,4 +187,24 @@ int rte_pmd_i40e_set_vf_multicast_promisc(uint8_t port,
> > int rte_pmd_i40e_set_vf_mac_addr(uint8_t port, uint16_t vf_id,
> > struct ether_addr *mac_addr);
> >
> > +/**
> > + * Enable/Disable vf vlan strip for all queues in a pool
> > + *
> > + * @param port
> > + * The port identifier of the Ethernet device.
> > + * @param vf
> > + * ID specifying VF.
> > + * @param on
> > + * 1 - Enable VF's vlan strip on RX queues.
> > + * 0 - Disable VF's vlan strip on RX queues.
> > + *
> > + * @return
> > + * - (0) if successful.
> > + * - (-ENOTSUP) if hardware doesn't support this feature.
>
> Is this error type returned?
Good catch. Only -EINVAL and -ENODEV would be returned.
>
> > + * - (-ENODEV) if *port* invalid.
> > + * - (-EINVAL) if bad parameter.
> > + */
> <...>
^ permalink raw reply
* Re: [PATCH v2 27/32] net/i40e: change version number to support Linux VF
From: Chen, Jing D @ 2016-12-08 9:14 UTC (permalink / raw)
To: Yigit, Ferruh, Lu, Wenzhuo, dev@dpdk.org
In-Reply-To: <3cc33a4b-d45f-0008-05fc-033df4ccb197@intel.com>
Hi, Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 07, 2016 11:14 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Cc: Chen, Jing D <jing.d.chen@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 27/32] net/i40e: change version number to
> support Linux VF
>
> On 12/7/2016 3:32 AM, Wenzhuo Lu wrote:
> > i40e PF host only support to work with DPDK VF driver, Linux
> > VF driver is not supported. This change will enhance in version
> > number returned.
> >
> > Current version info returned won't be able to be recognized
> > by Linux VF driver, change to values that both DPDK VF and Linux
> > driver can recognize.
> >
> > The expense is original DPDK host specific feature like
> > CFG_VLAN_PVID and CONFIG_VSI_QUEUES_EXT will not available.
> >
> > DPDK VF also can't identify host driver by version number returned.
> > It always assume talking with Linux PF.
>
> I guess you mention from following code [1], should it be also updated
> to prevent it giving wrong information:
I'd like to update it into some docs.
>
> [1] i40e_ethdev_vf.c
> if (vf->version_major == I40E_DPDK_VERSION_MAJOR)
> PMD_DRV_LOG(INFO, "Peer is DPDK PF host");
> else if ((vf->version_major == I40E_VIRTCHNL_VERSION_MAJOR) &&
> (vf->version_minor <= I40E_VIRTCHNL_VERSION_MINOR))
> PMD_DRV_LOG(INFO, "Peer is Linux PF host");
> else {
>
> >
> > Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
> > ---
>
> <...>
^ permalink raw reply
* Re: [PATCH] doc: fix mistakes in contribution guide
From: Mcnamara, John @ 2016-12-08 9:20 UTC (permalink / raw)
To: Yong Wang; +Cc: dev@dpdk.org
In-Reply-To: <1481170768-21700-1-git-send-email-wang.yong19@zte.com.cn>
> -----Original Message-----
> From: Yong Wang [mailto:wang.yong19@zte.com.cn]
> Sent: Thursday, December 8, 2016 4:19 AM
> To: Mcnamara, John <john.mcnamara@intel.com>
> Cc: dev@dpdk.org; Yong Wang <wang.yong19@zte.com.cn>
> Subject: [PATCH] doc: fix mistakes in contribution guide
Thanks for the fixes. One minor comment below.
>
> Signed-off-by: Yong Wang <wang.yong19@zte.com.cn>
> ---
> doc/guides/contributing/documentation.rst | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/doc/guides/contributing/documentation.rst
> b/doc/guides/contributing/documentation.rst
> index 5ec0675..5f16a0c 100644
> --- a/doc/guides/contributing/documentation.rst
> +++ b/doc/guides/contributing/documentation.rst
> @@ -381,9 +381,9 @@ Lists
> #. Item one.
>
> #. Item two is a long line that is wrapped and then indented
> - to match the start of the e first line.
> + to match the start of the first line.
>
> - #. Item two is a long line that is wrapped and then indented to
> match
> + #. Item three is a long line that is wrapped and then indented to
> match
> the start of the previous line.
The middle item should probably be deleted. I would replace the list with the following:
#. Item one.
#. Item two is a long line that is wrapped and then indented to match
the start of the previous line.
#. Item three.
Thanks,
John
^ permalink raw reply
* Re: [PATCH 1/4] eal/common: introduce rte_memset on IA platform
From: Ananyev, Konstantin @ 2016-12-08 9:26 UTC (permalink / raw)
To: Yang, Zhiyong, Thomas Monjalon
Cc: dev@dpdk.org, yuanhan.liu@linux.intel.com, Richardson, Bruce,
De Lara Guarch, Pablo
In-Reply-To: <E182254E98A5DA4EB1E657AC7CB9BD2A3EB565EC@BGSMSX101.gar.corp.intel.com>
Hi Zhiyong,
>
> HI, Thomas:
> Sorry for late reply. I have been being always considering your suggestion.
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Friday, December 2, 2016 6:25 PM
> > To: Yang, Zhiyong <zhiyong.yang@intel.com>
> > Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; Richardson, Bruce
> > <bruce.richardson@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/4] eal/common: introduce rte_memset on
> > IA platform
> >
> > 2016-12-05 16:26, Zhiyong Yang:
> > > +#ifndef _RTE_MEMSET_X86_64_H_
> >
> > Is this implementation specific to 64-bit?
> >
>
> Yes.
>
> > > +
> > > +#define rte_memset memset
> > > +
> > > +#else
> > > +
> > > +static void *
> > > +rte_memset(void *dst, int a, size_t n);
> > > +
> > > +#endif
> >
> > If I understand well, rte_memset (as rte_memcpy) is using the most recent
> > instructions available (and enabled) when compiling.
> > It is not adapting the instructions to the run-time CPU.
> > There is no need to downgrade at run-time the instruction set as it is
> > obviously not a supported case, but it would be nice to be able to upgrade a
> > "default compilation" at run-time as it is done in rte_acl.
> > I explain this case more clearly for reference:
> >
> > We can have AVX512 supported in the compiler but disable it when compiling
> > (CONFIG_RTE_MACHINE=snb) in order to build a binary running almost
> > everywhere.
> > When running this binary on a CPU having AVX512 support, it will not benefit
> > of the AVX512 improvement.
> > Though, we can compile an AVX512 version of some functions and use them
> > only if the running CPU is capable.
> > This kind of miracle can be achieved in two ways:
> >
> > 1/ For generic C code compiled with a recent GCC, a function can be built for
> > several CPUs thanks to the attribute target_clones.
> >
> > 2/ For manually optimized functions using CPU-specific intrinsics or asm, it is
> > possible to build them with non-default flags thanks to the attribute target.
> >
> > 3/ For manually optimized files using CPU-specific intrinsics or asm, we use
> > specifics flags in the makefile.
> >
> > The function clone in case 1/ is dynamically chosen at run-time through ifunc
> > resolver.
> > The specific functions in cases 2/ and 3/ must chosen at run-time by
> > initializing a function pointer thanks to rte_cpu_get_flag_enabled().
> >
> > Note that rte_hash and software crypto PMDs have a run-time check with
> > rte_cpu_get_flag_enabled() but do not override CFLAGS in the Makefile.
> > Next step for these libraries?
> >
> > Back to rte_memset, I think you should try the solution 2/.
>
> I have read the ACL code, if I understand well , for complex algo implementation,
> it is good idea, but Choosing functions at run time will bring some overhead. For frequently called function
> Which consumes small cycles, the overhead maybe is more than the gains optimizations brings
> For example, for most applications in dpdk, memset only set N = 10 or 12bytes. It consumes fewer cycles.
But then what the point to have an rte_memset() using vector instructions at all?
>From what you are saying the most common case is even less then SSE register size.
Konstantin
>
> Thanks
> Zhiyong
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Nélio Laranjeiro @ 2016-12-08 9:30 UTC (permalink / raw)
To: dev, Ananyev, Konstantin, Bruce Richardson, Wiles, Keith,
Morten Brørup, Neil Horman
Cc: wenzhuo.lu, Olivier Matz
In-Reply-To: <a55f9700134c2b5e83b4806df573aefa0c4bab03.1478703591.git.nelio.laranjeiro@6wind.com>
Hi all,
Following previous discussions, I would like to gather requirements for
v2, currently we have:
1. Introduction of new typedefs.
2. Modification of network headers.
3. Modification of rte_*_to_*() functions.
Point 1. seems not to be an issue, everyone seems to agree on the fact
having those types could help to document some parts of the code.
Point 2. does not cause any ABI change as it is only a documentation
commit, not sure if anyone disagrees about this.
Point 3. documentation commit most people are uncomfortable with.
I propose to drop it from v2.
Any objection to this plan?
--
Nélio Laranjeiro
6WIND
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-12-08 9:30 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <20161207185303.GA7001@svelivela-lt.caveonetworks.com>
On Thu, Dec 08, 2016 at 12:23:03AM +0530, Jerin Jacob wrote:
> On Tue, Dec 06, 2016 at 04:51:19PM +0000, Bruce Richardson wrote:
> > On Tue, Dec 06, 2016 at 09:22:15AM +0530, Jerin Jacob wrote:
> > > In a polling model, lcores poll ethdev ports and associated
> > > rx queues directly to look for packet. In an event driven model,
> > > by contrast, lcores call the scheduler that selects packets for
> > > them based on programmer-specified criteria. Eventdev library
> > > adds support for event driven programming model, which offer
> > > applications automatic multicore scaling, dynamic load balancing,
> > > pipelining, packet ingress order maintenance and
> > > synchronization services to simplify application packet processing.
> > >
> > > By introducing event driven programming model, DPDK can support
> > > both polling and event driven programming models for packet processing,
> > > and applications are free to choose whatever model
> > > (or combination of the two) that best suits their needs.
> > >
> > > This patch adds the eventdev specification header file.
> > >
> > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > ---
> > > + /** WORD1 */
> > > + RTE_STD_C11
> > > + union {
> > > + uint64_t u64;
> > > + /**< Opaque 64-bit value */
> > > + uintptr_t event_ptr;
> > > + /**< Opaque event pointer */
> >
> > Since we have a uint64_t member of the union, might this be better as a
> > void * rather than uintptr_t?
>
> No strong opinion here. For me, uintptr_t looks clean.
> But, It is OK to change to void* as per your input.
>
> >
> > > + struct rte_mbuf *mbuf;
> > > + /**< mbuf pointer if dequeued event is associated with mbuf */
> > > + };
> > > +};
> > > +
> > <snip>
> > > +/**
> > > + * Link multiple source event queues supplied in *rte_event_queue_link*
> > > + * structure as *queue_id* to the destination event port designated by its
> > > + * *port_id* on the event device designated by its *dev_id*.
> > > + *
> > > + * The link establishment shall enable the event port *port_id* from
> > > + * receiving events from the specified event queue *queue_id*
> > > + *
> > > + * An event queue may link to one or more event ports.
> > > + * The number of links can be established from an event queue to event port is
> > > + * implementation defined.
> > > + *
> > > + * Event queue(s) to event port link establishment can be changed at runtime
> > > + * without re-configuring the device to support scaling and to reduce the
> > > + * latency of critical work by establishing the link with more event ports
> > > + * at runtime.
> >
> > I think this might need to be clarified. The device doesn't need to be
> > reconfigured, but does it need to be stopped? In SW implementation, this
> > affects how much we have to make things thread-safe. At minimum I think
> > we should limit this to having only one thread call the function at a
> > time, but we may allow enqueue dequeue ops from the data plane to run
> > in parallel.
>
> Cavium implementation can change it at runtime without re-configuring or stopping
> the device to support runtime load balancing from the application perspective.
>
> AFAIK, link establishment is _NOT_ fast path API. But the application
> can invoke it from worker thread whenever there is a need for re-wiring
> the queue to port connection for better explicit load balancing. IMO, A
> software implementation with lock is fine here as we don't use this in
> fastpath.
>
> Thoughts?
> >
I agree that it's obviously not fast-path. Therefore I suggest that we
document that this API should be safe to call while the data path is in
operation, but that it should not be called by multiple cores
simultaneously i.e. single-writer, multi-reader safe, but not
multi-writer safe. Does that seem reasonable to you?
/Bruce
^ permalink raw reply
* Re: [PATCH 1/4] eal/common: introduce rte_memset on IA platform
From: Yang, Zhiyong @ 2016-12-08 9:53 UTC (permalink / raw)
To: Ananyev, Konstantin, Thomas Monjalon
Cc: dev@dpdk.org, yuanhan.liu@linux.intel.com, Richardson, Bruce,
De Lara Guarch, Pablo
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E55B0@irsmsx105.ger.corp.intel.com>
Hi， Konstantin:
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, December 8, 2016 5:26 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; Richardson, Bruce
> <bruce.richardson@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 1/4] eal/common: introduce rte_memset on
> IA platform
>
>
> Hi Zhiyong,
>
> >
> > HI, Thomas:
> > Sorry for late reply. I have been being always considering your
> suggestion.
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Friday, December 2, 2016 6:25 PM
> > > To: Yang, Zhiyong <zhiyong.yang@intel.com>
> > > Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; Richardson, Bruce
> > > <bruce.richardson@intel.com>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; De Lara Guarch, Pablo
> > > <pablo.de.lara.guarch@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH 1/4] eal/common: introduce
> rte_memset
> > > on IA platform
> > >
> > > 2016-12-05 16:26, Zhiyong Yang:
> > > > +#ifndef _RTE_MEMSET_X86_64_H_
> > >
> > > Is this implementation specific to 64-bit?
> > >
> >
> > Yes.
> >
> > > > +
> > > > +#define rte_memset memset
> > > > +
> > > > +#else
> > > > +
> > > > +static void *
> > > > +rte_memset(void *dst, int a, size_t n);
> > > > +
> > > > +#endif
> > >
> > > If I understand well, rte_memset (as rte_memcpy) is using the most
> > > recent instructions available (and enabled) when compiling.
> > > It is not adapting the instructions to the run-time CPU.
> > > There is no need to downgrade at run-time the instruction set as it
> > > is obviously not a supported case, but it would be nice to be able
> > > to upgrade a "default compilation" at run-time as it is done in rte_acl.
> > > I explain this case more clearly for reference:
> > >
> > > We can have AVX512 supported in the compiler but disable it when
> > > compiling
> > > (CONFIG_RTE_MACHINE=snb) in order to build a binary running almost
> > > everywhere.
> > > When running this binary on a CPU having AVX512 support, it will not
> > > benefit of the AVX512 improvement.
> > > Though, we can compile an AVX512 version of some functions and use
> > > them only if the running CPU is capable.
> > > This kind of miracle can be achieved in two ways:
> > >
> > > 1/ For generic C code compiled with a recent GCC, a function can be
> > > built for several CPUs thanks to the attribute target_clones.
> > >
> > > 2/ For manually optimized functions using CPU-specific intrinsics or
> > > asm, it is possible to build them with non-default flags thanks to the
> attribute target.
> > >
> > > 3/ For manually optimized files using CPU-specific intrinsics or
> > > asm, we use specifics flags in the makefile.
> > >
> > > The function clone in case 1/ is dynamically chosen at run-time
> > > through ifunc resolver.
> > > The specific functions in cases 2/ and 3/ must chosen at run-time by
> > > initializing a function pointer thanks to rte_cpu_get_flag_enabled().
> > >
> > > Note that rte_hash and software crypto PMDs have a run-time check
> > > with
> > > rte_cpu_get_flag_enabled() but do not override CFLAGS in the Makefile.
> > > Next step for these libraries?
> > >
> > > Back to rte_memset, I think you should try the solution 2/.
> >
> > I have read the ACL code, if I understand well , for complex algo
> > implementation, it is good idea, but Choosing functions at run time
> > will bring some overhead. For frequently called function Which
> > consumes small cycles, the overhead maybe is more than the gains
> optimizations brings For example, for most applications in dpdk, memset only
> set N = 10 or 12bytes. It consumes fewer cycles.
>
> But then what the point to have an rte_memset() using vector instructions at
> all?
> From what you are saying the most common case is even less then SSE
> register size.
> Konstantin
For most cases, memset is used such as memset(address, 0, sizeof(struct xxx));
The use case here is small by accident, I only give an example here.
but rte_memset is introduced to need consider generic case.
sizeof(struct xxx) is not limited to very small size, such as less than SSE register size.
I just want to say that the size for the most use case is not very large, So cycles consumed
Is not large. It is not suited to choose function at run-time since overhead is considered.
thanks
Zhiyong
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-12-08 9:57 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <20161208014800.GB22031@svelivela-lt.caveonetworks.com>
On Thu, Dec 08, 2016 at 07:18:01AM +0530, Jerin Jacob wrote:
> On Wed, Dec 07, 2016 at 11:12:51AM +0000, Bruce Richardson wrote:
> > On Tue, Dec 06, 2016 at 09:22:15AM +0530, Jerin Jacob wrote:
> > > In a polling model, lcores poll ethdev ports and associated
> > > rx queues directly to look for packet. In an event driven model,
> > > by contrast, lcores call the scheduler that selects packets for
> > > them based on programmer-specified criteria. Eventdev library
> > > adds support for event driven programming model, which offer
> > > applications automatic multicore scaling, dynamic load balancing,
> > > pipelining, packet ingress order maintenance and
> > > synchronization services to simplify application packet processing.
> > >
> > > By introducing event driven programming model, DPDK can support
> > > both polling and event driven programming models for packet processing,
> > > and applications are free to choose whatever model
> > > (or combination of the two) that best suits their needs.
> > >
> > > This patch adds the eventdev specification header file.
> > >
> > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > ---
> > > +
> > > +/**
> > > + * Link multiple source event queues supplied in *rte_event_queue_link*
> > > + * structure as *queue_id* to the destination event port designated by its
> > > + * *port_id* on the event device designated by its *dev_id*.
> > > + *
> > > + * The link establishment shall enable the event port *port_id* from
> > > + * receiving events from the specified event queue *queue_id*
> > > + *
> > > + * An event queue may link to one or more event ports.
> > > + * The number of links can be established from an event queue to event port is
> > > + * implementation defined.
> > > + *
> > > + * Event queue(s) to event port link establishment can be changed at runtime
> > > + * without re-configuring the device to support scaling and to reduce the
> > > + * latency of critical work by establishing the link with more event ports
> > > + * at runtime.
> > > + *
> > > + * @param dev_id
> > > + * The identifier of the device.
> > > + *
> > > + * @param port_id
> > > + * Event port identifier to select the destination port to link.
> > > + *
> > > + * @param link
> > > + * Points to an array of *nb_links* objects of type *rte_event_queue_link*
> > > + * structure which contain the event queue to event port link establishment
> > > + * attributes.
> > > + * NULL value is allowed, in which case this function links all the configured
> > > + * event queues *nb_event_queues* which previously supplied to
> > > + * rte_event_dev_configure() to the event port *port_id* with normal servicing
> > > + * priority(RTE_EVENT_DEV_PRIORITY_NORMAL).
> > > + *
> > > + * @param nb_links
> > > + * The number of links to establish
> > > + *
> > > + * @return
> > > + * The number of links actually established. The return value can be less than
> > > + * the value of the *nb_links* parameter when the implementation has the
> > > + * limitation on specific queue to port link establishment or if invalid
> > > + * parameters are specified in a *rte_event_queue_link*.
> > > + * If the return value is less than *nb_links*, the remaining links at the end
> > > + * of link[] are not established, and the caller has to take care of them.
> > > + * If return value is less than *nb_links* then implementation shall update the
> > > + * rte_errno accordingly, Possible rte_errno values are
> > > + * (-EDQUOT) Quota exceeded(Application tried to link the queue configured with
> > > + * RTE_EVENT_QUEUE_CFG_FLAG_SINGLE_LINK to more than one event ports)
> > > + * (-EINVAL) Invalid parameter
> > > + *
> > > + */
> > > +int
> > > +rte_event_port_link(uint8_t dev_id, uint8_t port_id,
> > > + const struct rte_event_queue_link link[],
> > > + uint16_t nb_links);
> > > +
> >
> > Hi again Jerin,
> >
> > another small suggestion here. I'm not a big fan of using small
> > structures to pass parameters into functions, especially when not all
> > fields are always going to be used. Rather than use the event queue link
> > structure, can we just pass in two array parameters here - the list of
> > QIDs, and the list of priorities. In cases where the eventdev
> > implementation does not support link prioritization, or where the app
> > does not want different priority mappings , then the second
> > array can be null [implying NORMAL priority for the don't care case].
> >
> > int
> > rte_event_port_link(uint8_t dev_id, uint8_t port_id,
> > const uint8_t queues[], const uint8_t priorities[],
> > uint16_t nb_queues);
> >
> > This just makes mapping an array of queues easier, as we can just pass
> > an array of ints directly in, and it especially makes it easier to
> > create a single link via:
> >
> > rte_event_port_link(dev_id, port_id, &queue_id, NULL, 1);
>
> The reason why I thought of creating "struct rte_event_queue_link",
> - Its easy to add new parameter in link attributes if required
Make the priority value be in a struct, perhaps. That would allow for
future expansion, while still making it easier for the case where people
just want the mappings without any prioritization.
> - Its _easy_ to implement PAUSE and RESUME in application
>
> PAUSE:
> nr_links = rte_event_port_links_get(,,link)
> rte_event_port_unlink_all
>
> RESUME:
> rte_event_port_link(,,link, nr_links);
Ok, I had missed that implication. Since that is probably an important
operation we might want to do, perhaps links_get API should be updated
too to keep parameter matching.
/Bruce
^ permalink raw reply
* Re: [PATCH v2 3/6] eventdev: implement the northbound APIs
From: Bruce Richardson @ 2016-12-08 9:59 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <20161207170256.GA4930@svelivela-lt.caveonetworks.com>
On Wed, Dec 07, 2016 at 10:32:56PM +0530, Jerin Jacob wrote:
> On Tue, Dec 06, 2016 at 05:17:12PM +0000, Bruce Richardson wrote:
> > On Tue, Dec 06, 2016 at 09:22:17AM +0530, Jerin Jacob wrote:
> > > This patch implements northbound eventdev API interface using
> > > southbond driver interface
> > >
> > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > ---
> > > + /* Re allocate memory to store queue priority */
> > > + queues_prio = dev->data->queues_prio;
> > > + queues_prio = rte_realloc(queues_prio,
> > > + sizeof(queues_prio[0]) * nb_queues,
> > > + RTE_CACHE_LINE_SIZE);
> > > + if (queues_prio == NULL) {
> > > + RTE_EDEV_LOG_ERR("failed to realloc queue priority,"
> > > + " nb_queues %u", nb_queues);
> > > + return -(ENOMEM);
> > > + }
> > > + dev->data->queues_prio = queues_prio;
> > > +
> > > + if (nb_queues > old_nb_queues) {
> > > + uint8_t new_qs = nb_queues - old_nb_queues;
> > > +
> > > + memset(queues + old_nb_queues, 0,
> > > + sizeof(queues[0]) * new_qs);
> > > + memset(queues_prio + old_nb_queues, 0,
> > > + sizeof(queues_prio[0]) * new_qs);
> > > + }
> > > + } else if (dev->data->queues != NULL && nb_queues == 0) {
> > > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->queue_release, -ENOTSUP);
> > > +
> > > + queues = dev->data->queues;
> > > + for (i = nb_queues; i < old_nb_queues; i++)
> > > + (*dev->dev_ops->queue_release)(queues[i]);
> > > + }
> > > +
> > > + dev->data->nb_queues = nb_queues;
> > > + return 0;
> > > +}
> > > +
> > While the ports array makes sense to have available at the top level of
> > the API and allocated from rte_eventdev.c, I'm not seeing what the value
> > of having the queues allocated at that level is. The only time the queue
> > array is indexed by eventdev layer is when releasing a queue. Therefore,
> > I suggest just saving the number of queues for sanity checking and let
> > the queue array allocation and freeing be handled entirely in the
> > drivers themselves.
>
> I thought it would be useful for other drivers. I agree, If something is not
> common across all the driver lets remove it from common code.
> I will remove it in v3
>
It's not a big deal for us - just an extra assignment we need to do in
our code path, so if it provides benefit for your driver, leave it in. I
just found it strange that that array was never really used by the
eventdev APIs, which is why I thought it might be better as internal
only.
/Bruce
^ permalink raw reply
* Re: [RFC] ethdev: abstraction layer for QoS hierarchical scheduler
From: Bruce Richardson @ 2016-12-08 10:14 UTC (permalink / raw)
To: Alan Robertson; +Cc: Dumitrescu, Cristian, dev@dpdk.org
In-Reply-To: <6d862b500e1e4f34a4cbf790db8d5d48@EMEAWP-EXMB11.corp.brocade.com>
On Wed, Dec 07, 2016 at 10:58:49AM +0000, Alan Robertson wrote:
> Hi Cristian,
>
> Looking at points 10 and 11 it's good to hear nodes can be dynamically added.
>
> We've been trying to decide the best way to do this for support of qos on tunnels for
> some time now and the existing implementation doesn't allow this so effectively ruled
> out hierarchical queueing for tunnel targets on the output interface.
>
> Having said that, has thought been given to separating the queueing from being so closely
> tied to the Ethernet transmit process ? When queueing on a tunnel for example we may
> be working with encryption. When running with an anti-reply window it is really much
> better to do the QOS (packet reordering) before the encryption. To support this would
> it be possible to have a separate scheduler structure which can be passed into the
> scheduling API ? This means the calling code can hang the structure of whatever entity
> it wishes to perform qos on, and we get dynamic target support (sessions/tunnels etc).
>
Hi,
just to note that not all ethdevs need to be actual NICs (physical or
virtual). It was also for situations like this that the ring PMD was
created. For the QoS scheduler, the common "output port" type chosen was
the ethdev, to avoid having to support multiple underlying types. To use
a ring instead as the output port, just create a ring and then call
"rte_eth_from_ring" to get an ethdev port wrapper around the ring, and
which you can then use for just about any API that wants an ethdev.
[Note: the rte_eth_from_ring API is in the ring driver itself, so you do
need to link against that driver directly if using shared libs]
Regards,
/Bruce
^ permalink raw reply
* Re: [PATCH v2 00/12] Add crypto PMD optimized for ARMv8
From: Bruce Richardson @ 2016-12-08 10:24 UTC (permalink / raw)
To: zbigniew.bodek; +Cc: pablo.de.lara.guarch, jerin.jacob, dev
In-Reply-To: <1481077985-4224-1-git-send-email-zbigniew.bodek@caviumnetworks.com>
On Tue, Dec 06, 2016 at 06:32:53PM -0800, zbigniew.bodek@caviumnetworks.com wrote:
> From: Zbigniew Bodek <zbigniew.bodek@caviumnetworks.com>
>
> Introduce crypto poll mode driver using ARMv8
> cryptographic extensions. This PMD is optimized
> to provide performance boost for chained
> crypto operations processing, such as:
> * encryption + HMAC generation
> * decryption + HMAC validation.
> In particular, cipher only or hash only
> operations are not provided.
> Performance gain can be observed in tests
> against OpenSSL PMD which also uses ARM
> crypto extensions for packets processing.
>
Hi,
great to see more crypto drivers coming into DPDK, thanks.
Question: do you know if this code would have any export compliance
implications for DPDK - or for those repackaging DPDK? Up till now, all
the crypto code used by DPDK was actually packaged in separate libraries
that were re-used, meaning that DPDK didn't contain any crypto
algorithms itself.
Regards,
/Bruce
^ permalink raw reply
* Re: [PATCH 1/2] eth: get rid of goto's in rte_eth_dev_detach
From: Ferruh Yigit @ 2016-12-08 10:25 UTC (permalink / raw)
To: Stephen Hemminger, dev
In-Reply-To: <20161208014751.24285-2-stephen@networkplumber.org>
On 12/8/2016 1:47 AM, Stephen Hemminger wrote:
> Extra goto's to just a return are unnecessary.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> lib/librte_ether/rte_ethdev.c | 21 +++++++--------------
> 1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 4209ad0..40c7cc6 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -466,27 +466,20 @@ rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
> int
> rte_eth_dev_detach(uint8_t port_id, char *name)
> {
> - int ret = -1;
> + int ret;
>
> - if (name == NULL) {
> - ret = -EINVAL;
> - goto err;
> - }
> + if (name == NULL)
> + return -EINVAL;
>
> /* FIXME: move this to eal, once device flags are relocated there */
> - if (rte_eth_dev_is_detachable(port_id))
> - goto err;
> + ret = rte_eth_dev_is_detachable(port_id);
> + if (ret < 0)
rte_eth_dev_is_detachable() can return 1 to indicate device is not
detachable.
> + return ret;
>
> snprintf(name, sizeof(rte_eth_devices[port_id].data->name),
> "%s", rte_eth_devices[port_id].data->name);
> - ret = rte_eal_dev_detach(name);
> - if (ret < 0)
> - goto err;
>
> - return 0;
> -
> -err:
> - return ret;
> + return rte_eal_dev_detach(name);
> }
>
> static int
>
^ permalink raw reply
* Re: [PATCH 1/4] eal/common: introduce rte_memset on IA platform
From: Bruce Richardson @ 2016-12-08 10:27 UTC (permalink / raw)
To: Yang, Zhiyong
Cc: Ananyev, Konstantin, Thomas Monjalon, dev@dpdk.org,
yuanhan.liu@linux.intel.com, De Lara Guarch, Pablo
In-Reply-To: <E182254E98A5DA4EB1E657AC7CB9BD2A3EB586ED@BGSMSX101.gar.corp.intel.com>
On Thu, Dec 08, 2016 at 09:53:12AM +0000, Yang, Zhiyong wrote:
> Hi， Konstantin:
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Thursday, December 8, 2016 5:26 PM
> > To: Yang, Zhiyong <zhiyong.yang@intel.com>; Thomas Monjalon
> > <thomas.monjalon@6wind.com>
> > Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; Richardson, Bruce
> > <bruce.richardson@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH 1/4] eal/common: introduce rte_memset on
> > IA platform
> >
> >
> > Hi Zhiyong,
> >
> > >
> > > HI, Thomas:
> > > Sorry for late reply. I have been being always considering your
> > suggestion.
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > Sent: Friday, December 2, 2016 6:25 PM
> > > > To: Yang, Zhiyong <zhiyong.yang@intel.com>
> > > > Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; Richardson, Bruce
> > > > <bruce.richardson@intel.com>; Ananyev, Konstantin
> > > > <konstantin.ananyev@intel.com>; De Lara Guarch, Pablo
> > > > <pablo.de.lara.guarch@intel.com>
> > > > Subject: Re: [dpdk-dev] [PATCH 1/4] eal/common: introduce
> > rte_memset
> > > > on IA platform
> > > >
> > > > 2016-12-05 16:26, Zhiyong Yang:
> > > > > +#ifndef _RTE_MEMSET_X86_64_H_
> > > >
> > > > Is this implementation specific to 64-bit?
> > > >
> > >
> > > Yes.
> > >
> > > > > +
> > > > > +#define rte_memset memset
> > > > > +
> > > > > +#else
> > > > > +
> > > > > +static void *
> > > > > +rte_memset(void *dst, int a, size_t n);
> > > > > +
> > > > > +#endif
> > > >
> > > > If I understand well, rte_memset (as rte_memcpy) is using the most
> > > > recent instructions available (and enabled) when compiling.
> > > > It is not adapting the instructions to the run-time CPU.
> > > > There is no need to downgrade at run-time the instruction set as it
> > > > is obviously not a supported case, but it would be nice to be able
> > > > to upgrade a "default compilation" at run-time as it is done in rte_acl.
> > > > I explain this case more clearly for reference:
> > > >
> > > > We can have AVX512 supported in the compiler but disable it when
> > > > compiling
> > > > (CONFIG_RTE_MACHINE=snb) in order to build a binary running almost
> > > > everywhere.
> > > > When running this binary on a CPU having AVX512 support, it will not
> > > > benefit of the AVX512 improvement.
> > > > Though, we can compile an AVX512 version of some functions and use
> > > > them only if the running CPU is capable.
> > > > This kind of miracle can be achieved in two ways:
> > > >
> > > > 1/ For generic C code compiled with a recent GCC, a function can be
> > > > built for several CPUs thanks to the attribute target_clones.
> > > >
> > > > 2/ For manually optimized functions using CPU-specific intrinsics or
> > > > asm, it is possible to build them with non-default flags thanks to the
> > attribute target.
> > > >
> > > > 3/ For manually optimized files using CPU-specific intrinsics or
> > > > asm, we use specifics flags in the makefile.
> > > >
> > > > The function clone in case 1/ is dynamically chosen at run-time
> > > > through ifunc resolver.
> > > > The specific functions in cases 2/ and 3/ must chosen at run-time by
> > > > initializing a function pointer thanks to rte_cpu_get_flag_enabled().
> > > >
> > > > Note that rte_hash and software crypto PMDs have a run-time check
> > > > with
> > > > rte_cpu_get_flag_enabled() but do not override CFLAGS in the Makefile.
> > > > Next step for these libraries?
> > > >
> > > > Back to rte_memset, I think you should try the solution 2/.
> > >
> > > I have read the ACL code, if I understand well , for complex algo
> > > implementation, it is good idea, but Choosing functions at run time
> > > will bring some overhead. For frequently called function Which
> > > consumes small cycles, the overhead maybe is more than the gains
> > optimizations brings For example, for most applications in dpdk, memset only
> > set N = 10 or 12bytes. It consumes fewer cycles.
> >
> > But then what the point to have an rte_memset() using vector instructions at
> > all?
> > From what you are saying the most common case is even less then SSE
> > register size.
> > Konstantin
>
> For most cases, memset is used such as memset(address, 0, sizeof(struct xxx));
> The use case here is small by accident, I only give an example here.
> but rte_memset is introduced to need consider generic case.
> sizeof(struct xxx) is not limited to very small size, such as less than SSE register size.
> I just want to say that the size for the most use case is not very large, So cycles consumed
> Is not large. It is not suited to choose function at run-time since overhead is considered.
>
For small copies with sizes specified at compile time, do compilers not
fully inline the memset call with a fixed-size equivalent. I believe
some compilers used to do so with memcpy - which is why we had a macro
for it in DPDK, so that compile-time constant copies would use regular
memcpy. If that is also the case for memset, then we should perhaps
specify that rte_memset is only for relatively large copies, e.g. >64
bytes. In that case, run-time detection may be worthwhile.
/Bruce
^ permalink raw reply
* Re: [PATCH 1/4] eal/common: introduce rte_memset on IA platform
From: Ananyev, Konstantin @ 2016-12-08 10:30 UTC (permalink / raw)
To: Yang, Zhiyong, Thomas Monjalon
Cc: dev@dpdk.org, yuanhan.liu@linux.intel.com, Richardson, Bruce,
De Lara Guarch, Pablo
In-Reply-To: <E182254E98A5DA4EB1E657AC7CB9BD2A3EB586ED@BGSMSX101.gar.corp.intel.com>
> -----Original Message-----
> From: Yang, Zhiyong
> Sent: Thursday, December 8, 2016 9:53 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 1/4] eal/common: introduce rte_memset on IA platform
>
> Hi， Konstantin:
>
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Thursday, December 8, 2016 5:26 PM
> > To: Yang, Zhiyong <zhiyong.yang@intel.com>; Thomas Monjalon
> > <thomas.monjalon@6wind.com>
> > Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; Richardson, Bruce
> > <bruce.richardson@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH 1/4] eal/common: introduce rte_memset on
> > IA platform
> >
> >
> > Hi Zhiyong,
> >
> > >
> > > HI, Thomas:
> > > Sorry for late reply. I have been being always considering your
> > suggestion.
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > Sent: Friday, December 2, 2016 6:25 PM
> > > > To: Yang, Zhiyong <zhiyong.yang@intel.com>
> > > > Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; Richardson, Bruce
> > > > <bruce.richardson@intel.com>; Ananyev, Konstantin
> > > > <konstantin.ananyev@intel.com>; De Lara Guarch, Pablo
> > > > <pablo.de.lara.guarch@intel.com>
> > > > Subject: Re: [dpdk-dev] [PATCH 1/4] eal/common: introduce
> > rte_memset
> > > > on IA platform
> > > >
> > > > 2016-12-05 16:26, Zhiyong Yang:
> > > > > +#ifndef _RTE_MEMSET_X86_64_H_
> > > >
> > > > Is this implementation specific to 64-bit?
> > > >
> > >
> > > Yes.
> > >
> > > > > +
> > > > > +#define rte_memset memset
> > > > > +
> > > > > +#else
> > > > > +
> > > > > +static void *
> > > > > +rte_memset(void *dst, int a, size_t n);
> > > > > +
> > > > > +#endif
> > > >
> > > > If I understand well, rte_memset (as rte_memcpy) is using the most
> > > > recent instructions available (and enabled) when compiling.
> > > > It is not adapting the instructions to the run-time CPU.
> > > > There is no need to downgrade at run-time the instruction set as it
> > > > is obviously not a supported case, but it would be nice to be able
> > > > to upgrade a "default compilation" at run-time as it is done in rte_acl.
> > > > I explain this case more clearly for reference:
> > > >
> > > > We can have AVX512 supported in the compiler but disable it when
> > > > compiling
> > > > (CONFIG_RTE_MACHINE=snb) in order to build a binary running almost
> > > > everywhere.
> > > > When running this binary on a CPU having AVX512 support, it will not
> > > > benefit of the AVX512 improvement.
> > > > Though, we can compile an AVX512 version of some functions and use
> > > > them only if the running CPU is capable.
> > > > This kind of miracle can be achieved in two ways:
> > > >
> > > > 1/ For generic C code compiled with a recent GCC, a function can be
> > > > built for several CPUs thanks to the attribute target_clones.
> > > >
> > > > 2/ For manually optimized functions using CPU-specific intrinsics or
> > > > asm, it is possible to build them with non-default flags thanks to the
> > attribute target.
> > > >
> > > > 3/ For manually optimized files using CPU-specific intrinsics or
> > > > asm, we use specifics flags in the makefile.
> > > >
> > > > The function clone in case 1/ is dynamically chosen at run-time
> > > > through ifunc resolver.
> > > > The specific functions in cases 2/ and 3/ must chosen at run-time by
> > > > initializing a function pointer thanks to rte_cpu_get_flag_enabled().
> > > >
> > > > Note that rte_hash and software crypto PMDs have a run-time check
> > > > with
> > > > rte_cpu_get_flag_enabled() but do not override CFLAGS in the Makefile.
> > > > Next step for these libraries?
> > > >
> > > > Back to rte_memset, I think you should try the solution 2/.
> > >
> > > I have read the ACL code, if I understand well , for complex algo
> > > implementation, it is good idea, but Choosing functions at run time
> > > will bring some overhead. For frequently called function Which
> > > consumes small cycles, the overhead maybe is more than the gains
> > optimizations brings For example, for most applications in dpdk, memset only
> > set N = 10 or 12bytes. It consumes fewer cycles.
> >
> > But then what the point to have an rte_memset() using vector instructions at
> > all?
> > From what you are saying the most common case is even less then SSE
> > register size.
> > Konstantin
>
> For most cases, memset is used such as memset(address, 0, sizeof(struct xxx));
Ok then I suppose for such cases you don't need any special function and memset()
would still be the best choice, right?
> The use case here is small by accident, I only give an example here.
> but rte_memset is introduced to need consider generic case.
We can have rte_memset_huge() or so instead, and document that
it should be used for sizes greater than some cutoff point.
Inside it you can just call a function pointer installed at startup (same as rte_acl_classify() does).
For big sizes, I suppose the price of extra function pointer call would not affect performance much.
For sizes smaller then this cutoff point you still can use either rte_memset_scalar() or just normal rte_memset().
Something like that:
extern void *(*__rte_memset_vector)( (void *s, int c, size_t n);
static inline void*
rte_memset_huge(void *s, int c, size_t n)
{
return __rte_memset_vector(s, c, n);
}
static inline void *
rte_memset(void *s, int c, size_t n)
{
If (n < XXX)
return rte_memset_scalar(s, c, n);
else
return rte_memset_huge(s, c, n);
}
XXX could be either a define, or could also be a variable, so it can be setuped at startup,
depending on the architecture.
Would that work?
Konstantin
> sizeof(struct xxx) is not limited to very small size, such as less than SSE register size.
> I just want to say that the size for the most use case is not very large, So cycles consumed
> Is not large. It is not suited to choose function at run-time since overhead is considered.
>
> thanks
> Zhiyong
^ permalink raw reply
* Re: [PATCH 2/2] sched/malloc: remove unnecesary return statements
From: Ferruh Yigit @ 2016-12-08 10:33 UTC (permalink / raw)
To: Stephen Hemminger, dev
In-Reply-To: <20161208014751.24285-3-stephen@networkplumber.org>
On 12/8/2016 1:47 AM, Stephen Hemminger wrote:
> return statements at end of void functions are unnecessary and unwanted.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
But patch content is not just for sched/malloc as title suggests, it has
sched:
mem:
net/bonding:
^ permalink raw reply
* Re: [PATCH v2 15/32] net/i40e: add VF vlan strip func
From: Ferruh Yigit @ 2016-12-08 10:43 UTC (permalink / raw)
To: Chen, Jing D, Lu, Wenzhuo, dev@dpdk.org
In-Reply-To: <4341B239C0EFF9468EE453F9E9F4604D3A3D0F13@shsmsx102.ccr.corp.intel.com>
On 12/8/2016 9:10 AM, Chen, Jing D wrote:
> HI, Ferruh,
>
> Best Regards,
> Mark
>
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, December 07, 2016 10:18 PM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Cc: Chen, Jing D <jing.d.chen@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 15/32] net/i40e: add VF vlan strip func
>>
>> On 12/7/2016 3:31 AM, Wenzhuo Lu wrote:
>>> Add a function to configure vlan strip enable/disable for specific
>>> SRIOV VF device.
>>>
>>> Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
>>> ---
>>
>> <...>
>>
>>> +
>>> +/* Set vlan strip on/off for specific VF from host */
>>> +int
>>> +rte_pmd_i40e_set_vf_vlan_stripq(uint8_t port, uint16_t vf_id, uint8_t on)
>>> +{
>>> + struct rte_eth_dev *dev;
>>> + struct i40e_pf *pf;
>>> + struct i40e_vsi *vsi;
>>> +
>>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>> +
>>> + dev = &rte_eth_devices[port];
>>> + pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
>>> +
>>> + if (vf_id > pf->vf_num - 1 || !pf->vfs) {
>>> + PMD_DRV_LOG(ERR, "Invalid argument.");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + vsi = pf->vfs[vf_id].vsi;
>>> +
>>> + if (vsi)
>>> + return i40e_vsi_config_vlan_stripping(vsi, !!on);
>>> + else
>>
>> if vd_if is valid, can vsi be NULL? If so this check may be required in
>> some prev patches too.
>
> It's a little impossible. This sanity check just make the code stronger.
>
If it is impossible, do you agree to remove this? And if this can be
possible we must update other patches, almost all other patches assume
this can't be NULL.
>>
>>> + return -EINVAL;
>>> +}
>>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h
>>> index ca5e05a..043ae62 100644
>>> --- a/drivers/net/i40e/rte_pmd_i40e.h
>>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
>>> @@ -187,4 +187,24 @@ int rte_pmd_i40e_set_vf_multicast_promisc(uint8_t port,
>>> int rte_pmd_i40e_set_vf_mac_addr(uint8_t port, uint16_t vf_id,
>>> struct ether_addr *mac_addr);
>>>
>>> +/**
>>> + * Enable/Disable vf vlan strip for all queues in a pool
>>> + *
>>> + * @param port
>>> + * The port identifier of the Ethernet device.
>>> + * @param vf
>>> + * ID specifying VF.
>>> + * @param on
>>> + * 1 - Enable VF's vlan strip on RX queues.
>>> + * 0 - Disable VF's vlan strip on RX queues.
>>> + *
>>> + * @return
>>> + * - (0) if successful.
>>> + * - (-ENOTSUP) if hardware doesn't support this feature.
>>
>> Is this error type returned?
>
> Good catch. Only -EINVAL and -ENODEV would be returned.
>
>>
>>> + * - (-ENODEV) if *port* invalid.
>>> + * - (-EINVAL) if bad parameter.
>>> + */
>> <...>
^ permalink raw reply
* Re: [RFC] pci: remove unused UNBIND support
From: David Marchand @ 2016-12-08 10:53 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev@dpdk.org, Thomas Monjalon
In-Reply-To: <20161207180436.2960-1-stephen@networkplumber.org>
On Wed, Dec 7, 2016 at 7:04 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> No device driver sets the unbind flag in current public code base.
> Therefore it is good time to remove the unused dead code.
Yes, this has been unused for some time now.
I would say this is not subject to abi enforcement as this only
matters to driver api not application api.
So this can go into 17.02.
The patch looks good to me.
--
David Marchand
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Van Haaren, Harry @ 2016-12-08 11:02 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev@dpdk.org, thomas.monjalon@6wind.com, Richardson, Bruce,
hemant.agrawal@nxp.com, Eads, Gage
In-Reply-To: <20161208012413.GA22031@svelivela-lt.caveonetworks.com>
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Thursday, December 8, 2016 1:24 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
<snip>
> > * Operation and sched_type *increased* to 4 bits each (from previous value of 2) to
> allow future expansion without ABI changes
>
> Anyway it will break ABI if we add new operation. I would propose to keep 4bit
> reserved and add it when required.
Ok sounds good. I'll suggest to move it to the middle between operation or sched type, which would allow expanding operation without ABI breaks. On expanding the field would remain in the same place with the same bits available in that place (no ABI break), but new bits can be added into the currently reserved space.
> > * Restore flow_id to 24 bits of a 32 bit int (previous size was 20 bits)
> > * sub-event-type reduced to 4 bits (previous value was 8 bits). Can we think of
> situations where 16 values for application specified identifiers of each event-type is
> genuinely not enough?
> One packet will not go beyond 16 stages but an application may have more stages and
> each packet may go mutually exclusive stages. For example,
>
> packet 0: stagex_0 ->stagex_1
> packet 1: stagey_0 ->stagey_1
>
> In that sense, IMO, more than 16 is required.(AFIAK, VPP has any much larger limit on
> number of stages)
My understanding was that stages are linked to event queues, so the application can determine the stage the packet comes from by reading queue_id?
I'm not opposed to having an 8 bit sub_event_type, but it seems unnecessarily large from my point of view. If you have a use for it, I'm ok with 8 bits.
> > In my opinion this structure layout is more balanced, and will perform better due to
> less loads that will need masking to access the required value.
> OK. Considering more balanced layout and above points. I propose following scheme(based on
> your input)
>
> union {
> uint64_t event;
> struct {
> uint32_t flow_id: 20;
> uint32_t sub_event_type : 8;
> uint32_t event_type : 4;
>
> uint8_t rsvd: 4; /* for future additions */
> uint8_t operation : 2; /* new fwd drop */
> uint8_t sched_type : 2;
>
> uint8_t queue_id;
> uint8_t priority;
> uint8_t impl_opaque;
> };
> };
>
> Feedback and improvements welcomed,
So incorporating my latest suggestions on moving fields around, excluding sub_event_type *size* changes:
union {
uint64_t event;
struct {
uint32_t flow_id: 20;
uint32_t event_type : 4;
uint32_t sub_event_type : 8; /* 8 bits now naturally aligned */
uint8_t operation : 2; /* new fwd drop */
uint8_t rsvd: 4; /* for future additions, can be expanded into without ABI break */
uint8_t sched_type : 2;
uint8_t queue_id;
uint8_t priority;
uint8_t impl_opaque;
};
};
-Harry
^ permalink raw reply
* Re: [PATCH v2 1/5] ethdev: add firmware version get
From: Ferruh Yigit @ 2016-12-08 11:07 UTC (permalink / raw)
To: Qiming Yang, dev; +Cc: Thomas Monjalon
In-Reply-To: <1481008582-69416-2-git-send-email-qiming.yang@intel.com>
Hi Qiming,
On 12/6/2016 7:16 AM, Qiming Yang wrote:
> This patch adds a new API 'rte_eth_dev_fwver_get' for fetching firmware
> version by a given device.
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
<...>
> @@ -1444,6 +1448,7 @@ struct eth_dev_ops {
> /**< Get names of extended statistics. */
> eth_queue_stats_mapping_set_t queue_stats_mapping_set;
> /**< Configure per queue stat counter mapping. */
> + eth_fw_version_get_t fw_version_get; /**< Get firmware version. */
Hi Qiming,
Not sure if I am missing something but this change is for following [1]
deprecation notice, right?
If so, notice suggest updating rte_eth_dev_info_get() to include
fw_version, but this patch adds a new eth_dev_ops.
Is it agreed to add a new eth_dev_ops for this?
[1]
* In 17.02 ABI change is planned: the ``rte_eth_dev_info`` structure
will be extended with a new member ``fw_version`` in order to store
the NIC firmware version.
> eth_dev_infos_get_t dev_infos_get; /**< Get device info. */
> eth_dev_supported_ptypes_get_t dev_supported_ptypes_get;
> /**< Get packet types supported and identified by device*/
<...>
^ permalink raw reply
* Re: [PATCH v2 00/12] Add crypto PMD optimized for ARMv8
From: Zbigniew Bodek @ 2016-12-08 11:32 UTC (permalink / raw)
To: Bruce Richardson; +Cc: pablo.de.lara.guarch, jerin.jacob, dev
In-Reply-To: <20161208102419.GE55440@bricha3-MOBL3.ger.corp.intel.com>
On 08.12.2016 11:24, Bruce Richardson wrote:
> On Tue, Dec 06, 2016 at 06:32:53PM -0800, zbigniew.bodek@caviumnetworks.com wrote:
>> From: Zbigniew Bodek <zbigniew.bodek@caviumnetworks.com>
>>
>> Introduce crypto poll mode driver using ARMv8
>> cryptographic extensions. This PMD is optimized
>> to provide performance boost for chained
>> crypto operations processing, such as:
>> * encryption + HMAC generation
>> * decryption + HMAC validation.
>> In particular, cipher only or hash only
>> operations are not provided.
>> Performance gain can be observed in tests
>> against OpenSSL PMD which also uses ARM
>> crypto extensions for packets processing.
>>
> Hi,
>
> great to see more crypto drivers coming into DPDK, thanks.
>
> Question: do you know if this code would have any export compliance
> implications for DPDK - or for those repackaging DPDK? Up till now, all
> the crypto code used by DPDK was actually packaged in separate libraries
> that were re-used, meaning that DPDK didn't contain any crypto
> algorithms itself.
>
Hello Bruce,
I don't know to be honest. I didn't know the reasoning behind not
including crypto code for Intel for example. I thought it was due to
licensing and code control rather than export compliance.
Maybe someone from the DPDK community will know what are the constraints
related to including crypto algorithms to DPDK.
Kind regards
Zbigniew
^ 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