RE: [EXT] [PATCH v2 0/2] crypto/scheduler: add support for security protocols
Hi Anoob, Thank you for that feedback - I was on extended leave so only just getting back to it now. See replies below. Regards, David > -Original Message- > From: Anoob Joseph > Sent: Friday, August 11, 2023 12:09 PM > To: Coyle, David ; dev@dpdk.org > Cc: Ji, Kai ; O'Sullivan, Kevin ; > Jerin Jacob Kollanukkaran > Subject: RE: [EXT] [PATCH v2 0/2] crypto/scheduler: add support for security > protocols > > Hi David, > > While it is desirable to add security under crypto/scheduler, would it be > functionally possible if the PMDs perform stateful processing? For example, > with lookaside protocol mode of IPsec, fields such as seq no & AR defines how > the crypto operation can be performed. Without two PMDs sharing this > (actively), how can the load balancing happen? [DC] So if some fields such as seq numbers are maintained within the PMDs for some protocols, then yes you are right - this would not work without some synchronization across PMD instances which I think we'd want to avoid at this point. I tried to find some cases where a crypto PMD that supports IPSec, for example, maintains some global stateful parameters, but I could not find these cases. I'm not at all familiar with these PMDs (cnxk, mvsam, dpaa_sec, dpaa2_sec) though, so maybe you could guide me as to where they are maintained? > > Said that, I agree utility of scheduler for stateless operations. My > understanding is, PDCP offload that is available today is not stateful and > that > can leverage this. I'm not sure of DOCSIS and MACsec. [DC] I notice that the PDCP security xform struct has a seq number related field, which would also suggest it could be stateful, but I could be wrong. >From a google search MACSec is stateless, but again I'm not an expert. The protocol I am familiar with is DOCSIS, and it is for this protocol that we have added security support to the cryptodev scheduler. DOCSIS is 100% stateless, so will work no problem with the scheduler. > > Should we make it such that only specific security sessions would be eligible > for > scheduler operation? [DC] Do you think it would be acceptable to limit the scheduler to the DOCSIS protocol only for now, and let the IPSec, MACSec and PDCP experts add these later if applicable? If you think this would be ok, I can easily make that change. > > Thanks, > Anoob > > > -Original Message- > > From: David Coyle > > Sent: Friday, August 11, 2023 3:54 PM > > To: dev@dpdk.org > > Cc: kai...@intel.com; kevin.osulli...@intel.com; David Coyle > > > > Subject: [EXT] [PATCH v2 0/2] crypto/scheduler: add support for > > security protocols > > > > External Email > > > > -- > > This patchset adds support to the cryptodev scheduler PMD and unit > > tests for the existing security protocols in the security library, > > namely IPSec, MACSec, PDCP and DOCSIS. > > > > v2: > > * Improve inclusion of rte_security header files > > * Fix typo in commit message > > > > David Coyle (2): > > crypto/scheduler: support security protocols > > test/crypto: add security tests for cryptodev scheduler > > > > app/test/test_cryptodev.c | 14 +- > > doc/guides/rel_notes/release_23_11.rst| 3 + > > drivers/crypto/scheduler/meson.build | 2 +- > > .../scheduler/rte_cryptodev_scheduler.c | 229 ++- > > drivers/crypto/scheduler/scheduler_failover.c | 12 +- > > .../crypto/scheduler/scheduler_multicore.c| 10 +- > > .../scheduler/scheduler_pkt_size_distr.c | 54 +-- > > drivers/crypto/scheduler/scheduler_pmd.c | 33 ++ > > drivers/crypto/scheduler/scheduler_pmd_ops.c | 375 > > +- .../crypto/scheduler/scheduler_pmd_private.h | 148 > --- > > .../crypto/scheduler/scheduler_roundrobin.c | 6 +- > > 11 files changed, 656 insertions(+), 230 deletions(-) > > > > -- > > 2.25.1
RE: [EXT] [PATCH v3 1/2] crypto/scheduler: support DOCSIS security protocol
Hi Anoob,
Thank you for the comments.
See inline below for replies.
Regards,
David
> -Original Message-
> From: Anoob Joseph
> Sent: Monday, September 18, 2023 12:03 PM
> To: Coyle, David
> Cc: Ji, Kai ; O'Sullivan, Kevin ;
> dev@dpdk.org; Jerin Jacob Kollanukkaran
> Subject: RE: [EXT] [PATCH v3 1/2] crypto/scheduler: support DOCSIS security
> protocol
>
> Hi David,
>
> Thanks for updating the patches based on the comments provided on
> previous version. Please see inline for some comments on code.
>
> Thanks,
> Anoob
>
> > -Original Message-
> > From: David Coyle
> > Sent: Thursday, September 14, 2023 8:52 PM
> > To: dev@dpdk.org
> > Cc: kai...@intel.com; Anoob Joseph ;
> > kevin.osulli...@intel.com; David Coyle
> > Subject: [EXT] [PATCH v3 1/2] crypto/scheduler: support DOCSIS
> > security protocol
> >
> > External Email
> >
> > --
> > Add support to the cryptodev scheduler PMD for the DOCSIS security
> > protocol. This includes adding the following to the scheduler:
> > - synchronization of worker's security capabilities
> > - retrieval of the scheduler's synchronized security capabilities
> > - retrieval of the security session size i.e. maximum session size
> > across all workers
> > - creation of security sessions on each worker
> > - deletion of security sessions on each worker
> >
> > Signed-off-by: David Coyle
> > Signed-off-by: Kevin O'Sullivan
> > ---
> > doc/guides/rel_notes/release_23_11.rst| 4 +
> > drivers/crypto/scheduler/meson.build | 2 +-
> > .../scheduler/rte_cryptodev_scheduler.c | 221 +-
> > drivers/crypto/scheduler/scheduler_failover.c | 12 +-
> > .../crypto/scheduler/scheduler_multicore.c| 10 +-
> > .../scheduler/scheduler_pkt_size_distr.c | 54 +--
> > drivers/crypto/scheduler/scheduler_pmd.c | 33 ++
> > drivers/crypto/scheduler/scheduler_pmd_ops.c | 381
> > +- .../crypto/scheduler/scheduler_pmd_private.h | 159
> +---
> > .../crypto/scheduler/scheduler_roundrobin.c | 6 +-
> > 10 files changed, 653 insertions(+), 229 deletions(-)
> >
>
>
>
> > diff --git a/drivers/crypto/scheduler/rte_cryptodev_scheduler.c
> > b/drivers/crypto/scheduler/rte_cryptodev_scheduler.c
> > index 258d6f8c43..e8b905af2f 100644
> > --- a/drivers/crypto/scheduler/rte_cryptodev_scheduler.c
> > +++ b/drivers/crypto/scheduler/rte_cryptodev_scheduler.c
> > @@ -5,11 +5,14 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "rte_cryptodev_scheduler.h"
> > #include "scheduler_pmd_private.h"
> >
> > +#define MAX_CAPS 256
> > +
> > /** update the scheduler pmd's capability with attaching device's
> > * capability.
> > * For each device to be attached, the scheduler's capability should
> > be @@ -
> > 59,7 +62,6 @@ sync_caps(struct rte_cryptodev_capabilities *caps,
> > cap->sym.auth.digest_size.max ?
> > s_cap->sym.auth.digest_size.max :
> > cap->sym.auth.digest_size.max;
> > -
> > }
> >
> > if (s_cap->sym.xform_type ==
> > @@ -81,25 +83,176 @@ sync_caps(struct rte_cryptodev_capabilities
> > *caps,
> >
> > memset(&caps[sync_nb_caps - 1], 0, sizeof(*cap));
> > sync_nb_caps--;
> > + i--;
> > }
> >
> > return sync_nb_caps;
> > }
> >
> > static int
> > -update_scheduler_capability(struct scheduler_ctx *sched_ctx)
> > +check_sec_cap_equal(const struct rte_security_capability *sec_cap1,
> > + struct rte_security_capability *sec_cap2) {
> > + if (sec_cap1->action != sec_cap2->action ||
> > + sec_cap1->protocol != sec_cap2->protocol ||
> > + sec_cap1->ol_flags != sec_cap2->ol_flags)
> > + return 0;
> > +
> > + if (sec_cap1->protocol == RTE_SECURITY_PROTOCOL_DOCSIS)
> > + return !memcmp(&sec_cap1->docsis, &sec_cap2->docsis,
> > + sizeof(sec_cap1->docsis));
> > + else
> > + return 0;
> > +}
> > +
> > +static void
> > +copy_sec_cap(struct rte_security_capabi
Re: [dpdk-dev] [PATCH v3 1/3] net/iavf: fix segment fault in AVX512
> -Original Message-
> From: dev On Behalf Of Wenzhuo Lu
> Sent: Tuesday, March 30, 2021 6:30 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo ; sta...@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 1/3] net/iavf: fix segment fault in AVX512
>
> Fix segment fault when failing to get the memory from the pool.
>
> Fixes: 31737f2b66fb ("net/iavf: enable AVX512 for legacy Rx")
> Cc: sta...@dpdk.org
>
> Reported-by: David Coyle
> Signed-off-by: Wenzhuo Lu
> ---
> drivers/net/iavf/iavf_rxtx_vec_avx2.c | 120 +--
> drivers/net/iavf/iavf_rxtx_vec_avx512.c | 5 +-
> drivers/net/iavf/iavf_rxtx_vec_common.h | 203
>
> 3 files changed, 209 insertions(+), 119 deletions(-)
>
The patch fixes the seg fault, but note I have only tested the default '#ifndef
RTE_LIBRTE_IAVF_16BYTE_RX_DESC' path
Tested-by: David Coyle
Re: [dpdk-dev] [PATCH v3 2/3] net/ice: fix segment fault in AVX512
> -Original Message-
> From: dev On Behalf Of Wenzhuo Lu
> Sent: Tuesday, March 30, 2021 6:30 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo ; sta...@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 2/3] net/ice: fix segment fault in AVX512
>
> Fix segment fault when failing to get the memory from the pool.
>
> Fixes: 7f85d5ebcfe1 ("net/ice: add AVX512 vector path")
> Cc: sta...@dpdk.org
>
> Reported-by: David Coyle
> Signed-off-by: Wenzhuo Lu
> ---
> drivers/net/ice/ice_rxtx_vec_avx2.c | 120 +---
> drivers/net/ice/ice_rxtx_vec_avx512.c | 5 +-
> drivers/net/ice/ice_rxtx_vec_common.h | 203
> ++
> 3 files changed, 209 insertions(+), 119 deletions(-)
>
The patch fixes the seg fault, but note I have only tested the default '#ifndef
RTE_LIBRTE_ICE_16BYTE_RX_DESC' path
Tested-by: David Coyle
Re: [dpdk-dev] [PATCH v3 3/3] net/i40e: fix segment fault in AVX512
> -Original Message-
> From: dev On Behalf Of Wenzhuo Lu
> Sent: Tuesday, March 30, 2021 6:30 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo ; sta...@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 3/3] net/i40e: fix segment fault in AVX512
>
> Fix segment fault when failing to get the memory from the pool.
>
> Fixes: e6a6a138919f ("net/i40e: add AVX512 vector path")
> Cc: sta...@dpdk.org
>
> Reported-by: David Coyle
> Signed-off-by: Wenzhuo Lu
> ---
> drivers/net/i40e/i40e_rxtx_vec_avx2.c | 117 +--
> drivers/net/i40e/i40e_rxtx_vec_avx512.c | 5 +-
> drivers/net/i40e/i40e_rxtx_vec_common.h | 201
>
> 3 files changed, 207 insertions(+), 116 deletions(-)
>
The patch fixes the seg fault, but note I have only tested the default '#ifndef
RTE_LIBRTE_I40E_16BYTE_RX_DESC ' path
Tested-by: David Coyle
Re: [dpdk-dev] [PATCH] test/crypto: refactor docsis to show hidden cases
Hi Rebecca, see below
> -Original Message-
> From: Troy, Rebecca
>
> In the current implementation, the docsis test cases are running and being
> reported as one test, despite the fact that multiple test cases are hidden
> inside i.e. "test_DOCSIS_PROTO_all" runs
> 52 test cases. Each docsis test case should be reported individually instead.
[DC] Should make "docsis" all uppercase in the commit message - "DOCSIS"
>
> This commit achieves this by removing the use of the
> test_DOCSIS_PROTO_all function and statically listing the test cases to run
> when building the test suite, which are then reported to the user by
> description.
>
> Signed-off-by: Rebecca Troy
> ---
> app/test/test_cryptodev.c | 265 +++---
> ...t_cryptodev_security_docsis_test_vectors.h | 159 +--
> 2 files changed, 241 insertions(+), 183 deletions(-)
>
>
> +
> static struct unit_test_suite docsis_proto_testsuite = {
> .suite_name = "Docsis Proto Unit Test Suite",
[DC] Outside the specific changes of this patch, but "Docsis" should be all
uppercase in the
suite name too - "DOCSIS"... could take this opportunity to fix up this minor
one
> .setup = docsis_proto_testsuite_setup,
> .unit_test_cases = {
> - TEST_CASE_ST(ut_setup_security, ut_teardown,
> - test_DOCSIS_PROTO_all),
> + /* Uplink */
> + ADD_UPLINK_TESTCASE(docsis_test_case_1)
> + ADD_UPLINK_TESTCASE(docsis_test_case_2)
> + ADD_UPLINK_TESTCASE(docsis_test_case_3)
> + ADD_UPLINK_TESTCASE(docsis_test_case_4)
>
> -struct docsis_test_data docsis_test_case_1 = {
> +const struct docsis_test_data docsis_test_case_1 = {
> + .test_descr_uplink = {"AES-DOCSIS-BPI-128 and CRC Verify (24-byte "
> + "frame, Small offset and runt block encryption)"},
> + .test_descr_downlink = {"CRC Generate and AES-DOCSIS-BPI-128
> (24-byte "
> + "frame, Small offset and runt block encryption)"},
[DC] This one is my fault when I supplied the descriptions, so apologies about
this,
but all the uplink descriptions should say "decryption" instead of "encryption"
Also I think all the descriptions should say "Uplink" or "Downlink" at the
start.
This can be inferred from the order of AES-DOCSIS-BPI and CRC in the
description, but
when I ran the test cases, I still had to think are these Uplink or Downlink
tests.
It would be much clearer if it's stated explicitly
Regards,
David
Re: [dpdk-dev] [PATCH v2] test/crypto: refactor DOCSIS to show hidden cases
> -Original Message- > From: Troy, Rebecca > Sent: Friday, October 29, 2021 10:04 AM > To: dev@dpdk.org > Cc: Power, Ciara ; Zhang, Roy Fan > ; Coyle, David ; Troy, > Rebecca ; Akhil Goyal ; > Doherty, Declan > Subject: [PATCH v2] test/crypto: refactor DOCSIS to show hidden cases > > In the current implementation, the DOCSIS test cases are running and being > reported as one test, despite the fact that multiple test cases are hidden > inside i.e. "test_DOCSIS_PROTO_all" runs > 52 test cases. Each DOCSIS test case should be reported individually instead. > > This commit achieves this by removing the use of the > test_DOCSIS_PROTO_all function and statically listing the test cases to run > when building the test suite, which are then reported to the user by > description. > > Signed-off-by: Rebecca Troy > Looks good Rebecca Reviewed-by: David Coyle
RE: [RFC PATCH] ring: adding TPAUSE instruction to ring dequeue
Hi Morten > -Original Message- > From: Morten Brørup > > > From: David Coyle [mailto:david.co...@intel.com] > > Sent: Wednesday, 3 May 2023 13.39 > > > > This is NOT for upstreaming. This is being submitted to allow early > > comparison testing with the preferred solution, which will add TAPUSE > > power management support to the ring library through the addition of > > callbacks. Initial stages of the preferred solution are available at > > http://dpdk.org/patch/125454. > > > > This patch adds functionality directly to rte_ring_dequeue functions > > to monitor the empty reads of the ring. When a configurable number of > > empty reads is reached, a TPAUSE instruction is triggered by using > > rte_power_pause() on supported architectures. rte_pause() is used on > > other architectures. The functionality can be included or excluded at > > compilation time using the RTE_RING_PMGMT flag. If included, the new > > API can be used to enable/disable the feature on a per-ring basis. > > Other related settings can also be configured using the API. > > I don't understand why DPDK developers keep spending time on trying to > invent methods to determine application busyness based on entry/exit > points in a variety of libraries, when the application is in a much better > position to determine busyness. All of these "busyness measuring" library > extensions have their own specific assumptions and weird limitations. > > I do understand that the goal is power saving, which certainly is relevant! I > only criticize the measuring methods. > > For reference, we implemented something very simple in our application > framework: > 1. When each pipeline stage has completed a burst, it reports if it was busy > or > not. > 2. If the pipeline busyness is low, we take a nap to save some power. > > And here is the magic twist to this simple algorithm: > 3. A pipeline stage is not considered busy unless it processed a full burst, > and > is ready to process more packets immediately. This interpretation of > busyness has a significant impact on the percentage of time spent napping > during the low-traffic hours. > > This algorithm was very quickly implemented. It might not be perfect, and we > do intend to improve it (also to determine CPU Utilization on a scale that the > end user can translate to a linear interpretation of how busy the system is). > But I seriously doubt that any of the proposed "busyness measuring" library > extensions are any better. > > So: The application knows better, please spend your precious time on > something useful instead. > > @David, my outburst is not directed at you specifically. Generally, I do > appreciate experimenting as a good way of obtaining knowledge. So thank > you for sharing your experiments with this audience! > > PS: If cruft can be disabled at build time, I generally don't oppose to it. [DC] Appreciate that feedback, and it is certainly another way of looking at and tackling the problem that we are ultimately trying to solve (i.e power saving) The problem however is that we work with a large number of ISVs and operators, each with their own workload architecture and implementation. That means we would have to work individually with each of these to integrate this type of pipeline-stage-busyness algorithm into their applications. And as these applications are usually commercial, non-open-source applications, that could prove to be very difficult. Also most ISVs and operators don't want to have to worry about changing their application, especially their fast-path dataplane, in order to get power savings. They prefer for it to just happen without them caring about the finer details. For these reasons, consolidating the busyness algorithms down into the DPDK libraries and PMDs is currently the preferred solution. As you say though, the libraries and PMDs may not be in the best position to determine the busyness of the pipeline, but it provides a good balance between achieving power savings and ease of adoption. It's also worth calling out again that this patch is only to allow early testing by some customers of the benefit of adding TPAUSE support to the ring library. We don't intend on this patch being upstreamed. The preferred longer term solution is to use callbacks from the ring library to initiate the pause (either via the DPDK power management API or through functions that an ISV may write themselves). This is mentioned in the commit message. Also, the pipeline stage busyness algorithm that you have added to your pipeline - have you ever considered implementing this into DPDK as a generic type library. This could certainly be of benefit to other DPDK application developers, and having this mechanism in DPDK could again ease the adoption and realisation of power savings for others. I understand though if this is your own secret sauce and you want to keep it like that :) David
RE: [RFC PATCH] ring: adding TPAUSE instruction to ring dequeue
Hi Morten
> -Original Message-
> From: Morten Brørup
>
> Power saving is important for the environment (to save the planet and all
> that), so everyone should contribute, if they have a good solution. So even if
> our algorithm had a significant degree of innovation, we would probably
> choose to make it public anyway. Open sourcing it also makes it possible for
> chip vendors like Intel to fine tune it more than we can ourselves, which also
> comes back to benefit us. All products need some sort of power saving in to
> stay competitive, but power saving algorithms is not an area we want to
> pursue for competitive purposes in our products.
>
> Our algorithm is too simple to make a library at this point, but I have been
> thinking about how we can make it a generic library when it has matured
> some more. I will take your information about the many customers' need to
> have it invisibly injected into consideration in this regard.
>
> Our current algorithm works like this:
>
> while (running) {
> int more = 0;
> more += stage1();
> more += stage2();
> more += stage3();
> if (!more) sleep();
> }
>
> Each pipeline stage only returns 1 if it processed a full burst. Furthermore,
> if a
> pipeline stage processed a full burst, but happens to know that no more data
> is readily available for it, it returns 0 instead.
>
> Obviously, the sleep() duration must be short enough to avoid that the NIC
> RX descriptor rings overflow before the ingress pipeline stage is serviced
> again.
>
> Changing the algorithm to "more" (1 = more work expected by the pipeline
> stage) from "busy" (1 = some work done by the pipeline stage) has the
> consequence that sleep() is called more often, which has the follow-on
> consequence that the ingress stage is called less often, and thus more often
> has a full burst to process.
>
> We know from our in-house profiler that processing a full burst provides
> *much* higher execution efficiency (cycles/packet) than processing a few
> packets. This is public knowledge - after all, this is the whole point of
> DPDK's
> vector packet processing design! Nonetheless, it might surprise some people
> how much the efficiency (cycles/packet) increases when processing a full
> burst compared to processing just a few packets. I will leave it up to the
> readers to make their own experiments. :-)
>
> Our initial "busy" algorithm behaved like this:
> Process a few packets (at low efficiency), don't sleep, Process a few packets
> (at low efficiency), don't sleep, Process a few packets (at low efficiency),
> don't sleep, Process a few packets (at low efficiency), don't sleep, Process a
> few packets (at low efficiency), don't sleep, Process a few packets (at low
> efficiency), don't sleep, Process a few packets (at low efficiency), don't
> sleep, Process a few packets (at low efficiency), don't sleep, No packets to
> process (we are lucky this time!), sleep briefly, Repeat.
>
> So we switched to our "more" algorithm, which behaves like this:
> Process a few packets (at low efficiency), sleep briefly, Process a full
> burst of
> packets (at high efficiency), don't sleep, Repeat.
>
> Instead of processing e.g. 8 small bursts per sleep, we now process only 2
> bursts per sleep. And the big of the two bursts is processed at higher
> efficiency.
>
> We can improve this algorithm in some areas...
>
> E.g. some of our pipeline stages also know that they are not going to do
> anymore work for the next X amount of nanoseconds; but we don't use that
> information in our power management algorithm yet. The sleep duration
> could depend on this.
>
> Also, we don't use the CPU power management states yet. I assume that
> doing some work for 20 us at half clock speed is more power conserving than
> doing the same work at full speed for 10 us and then sleeping for 10 us.
> That's another potential improvement.
>
>
> What we need in generic a power management helper library are functions
> to feed it with the application's perception of how much work is being done,
> and functions to tell if we can sleep and/or if we should change the power
> management states of the individual CPU cores.
>
> Such a unified power management helper (or "busyness") library could
> perhaps also be fed with data directly from the drivers and libraries to
> support the customer use cases you described.
[DC] Thank you for that detailed description, very interesting. There may
well be merit in upstreaming such an algorithm as a library once it has
matured as you said.
Configuration could include specifying what a "full burst"
actually is. Different stages of a pipeline may also have different definitions
of busyness, so that may also need to considered:
- Some stages may perform an operation (e.g. running an acl rule check) on a
burst of packets and then it is complete
- Other stages may be more asynchronous in nature e.g. enqueuing and
dequeuing to/from a crypto device or a QoS scheduler. The dequeue might
not
RE: [PATCH v2 0/6] crypto/security session framework rework
Hi Akhil/Fan Patchset verified for QAT and AESNI_MB sessions, with particular focus on Security Cipher-CRC For the series, Tested-by: David Coyle Tested-by: Kevin O'Sullivan Regards, David > -Original Message- > From: Akhil Goyal > Sent: Wednesday, September 21, 2022 4:11 PM > To: Akhil Goyal ; dev@dpdk.org; Zhang, Roy Fan > > Cc: tho...@monjalon.net; david.march...@redhat.com; > hemant.agra...@nxp.com; Vamsi Krishna Attunuru > ; ferruh.yi...@xilinx.com; > andrew.rybche...@oktetlabs.ru; konstantin.v.anan...@yandex.ru; > jiawe...@trustnetic.com; yisen.zhu...@huawei.com; Igor Russkikh > ; Jerin Jacob Kollanukkaran ; > Ankur Dwivedi ; maxime.coque...@redhat.com; > cha...@amd.com; ruifeng.w...@arm.com; ajit.khapa...@broadcom.com; > Anoob Joseph ; De Lara Guarch, Pablo > ; ma...@nvidia.com; g.si...@nxp.com; > Yang, Qiming ; Wu, Wenjun1 > ; jianw...@trustnetic.com; Wu, Jingjing > ; Xing, Beilei ; Nithin Kumar > Dabilpuram > Subject: RE: [PATCH v2 0/6] crypto/security session framework rework > > ++ Fan Zhang > I think I missed adding one of the major contributor for this cleanup. > > > -Original Message- > > From: Akhil Goyal > > Sent: Wednesday, September 21, 2022 8:33 PM > > To: dev@dpdk.org > > Cc: tho...@monjalon.net; david.march...@redhat.com; > > hemant.agra...@nxp.com; Vamsi Krishna Attunuru > > ; ferruh.yi...@xilinx.com; > > andrew.rybche...@oktetlabs.ru; konstantin.v.anan...@yandex.ru; > > jiawe...@trustnetic.com; yisen.zhu...@huawei.com; Igor Russkikh > > ; Jerin Jacob Kollanukkaran > > ; Ankur Dwivedi ; > > maxime.coque...@redhat.com; cha...@amd.com; > ruifeng.w...@arm.com; > > ajit.khapa...@broadcom.com; Anoob Joseph ; > > pablo.de.lara.gua...@intel.com; ma...@nvidia.com; g.si...@nxp.com; > > qiming.y...@intel.com; wenjun1...@intel.com; > jianw...@trustnetic.com; > > jingjing...@intel.com; beilei.x...@intel.com; Nithin Kumar Dabilpuram > > ; Akhil Goyal > > Subject: [PATCH v2 0/6] crypto/security session framework rework > > > > This patchset reworks the symmetric crypto and security session data > > structure to use a single virtual/physical contiguous buffer for > > symmetric crypto/security session and driver private data. > > In addition the session data structure is now private. > > The session is represented as an opaque pointer in the application. > > > > With the change the session is no longer supported to be accessed by > > multiple device drivers. For the same reason > > rte_cryptodev_sym_session_init/clear APIs are deprecated as > > rte_cryptodev_sym_session_create/free will initialize and clear the > > driver specific data field. > > > > The change was also submitted last year during DPDK 21.11 timeframe > > also[1], but was not applied due to lack of feedback from community. > > Please help in getting this cleanup merged in this cycle. > > > > Now the similar work was already done for asymmetric crypto. > > This patchset is rebased over current tree and fixes all the issues > > reported so far. > > This patchset is a v2 for the patch that was sent by Fan Zhang(Intel) > > with a few changes > > - Added security session rework also. > > - fixed issues in [2] reported on mailing list. > > - few other fixes. > > > > Please review and provide feedback as soon as possible as this is > > intended to be merged in DPDK 22.11 RC1. > > > > Currently the cnxk platform is tested with this change. > > Request everyone to review and test on their platform. > > > > Special note to ixgbe and txgbe maintainers. > > There is a wrong implementation for flow creation. Please check. > > A hack is added to bypass it. Please fix it separately. > > > > [1] > > https://patches.dpdk.org/project/dpdk/cover/20211018213452.2734720-1- > > gak...@marvell.com/ > > [2] > > https://patches.dpdk.org/project/dpdk/cover/20220829160645.378406-1- > > roy.fan.zh...@intel.com/ > > > > Akhil Goyal (5): > > cryptodev: rework session framework > > cryptodev: hide sym session structure > > security: remove priv mempool usage > > drivers/crypto: support security session get size op > > security: hide session structure > > > > Fan Zhang (1): > > crypto/scheduler: use unified session > > > > app/test-crypto-perf/cperf.h | 1 - > > app/test-crypto-perf/cperf_ops.c | 40 +-- > > app/test-crypto-perf/cperf_ops.h | 2 +- > > app/test-crypto-perf/cperf_test_latency.c | 9 +- > > app/test-crypto-perf/cperf_test_latency.h | 1 - > > .../cperf_test_pmd_cyclecount.c | 10 +- > > .../cperf_test_pmd_cyclecount.h | 1 - > > app/test-crypto-perf/cperf_test_throughput.c | 11 +- > > app/test-crypto-perf/cperf_test_throughput.h | 1 - > > app/test-crypto-perf/cperf_test_verify.c | 9 +- > > app/test-crypto-perf/cperf_test_verify.h | 1 - > > app/test-crypto-perf/main.c | 30 +- > > app/test-eventdev/test_perf_common.c | 35 +- > > app/test-eventdev/test_perf_common.h
Re: [dpdk-dev] [PATCH v2] sched : Initialize tc ov watermark.
> -Original Message- > From: dev On Behalf Of Savinay Dharmappa > Sent: Tuesday, March 9, 2021 4:10 PM > To: Singh, Jasvinder ; Dumitrescu, Cristian > ; dev@dpdk.org > Cc: Dharmappa, Savinay > Subject: [dpdk-dev] [PATCH v2] sched : Initialize tc ov watermark. > > tc ov watermark is initialized with computed value of max tc ov watermark. > > Signed-off-by: Savinay Dharmappa > --- > v2: fix spelling error. > --- > lib/librte_sched/rte_sched.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Tested-by: David Coyle
Re: [dpdk-dev] [PATCH 1/3] net/iavf: fix segment fault in AVX512
Hi Wenzhuo
> -Original Message-
> From: dev On Behalf Of Wenzhuo Lu
> Sent: Friday, March 12, 2021 1:27 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo ; sta...@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/3] net/iavf: fix segment fault in AVX512
>
> Fix segment fault when failing to get the memory from the pool.
>
> Fixes: 31737f2b66fb ("net/iavf: enable AVX512 for legacy Rx")
> Cc: sta...@dpdk.org
>
> Reported-by: David Coyle
> Signed-off-by: Wenzhuo Lu
> ---
> drivers/net/iavf/iavf_rxtx_vec_avx512.c | 130
>
> 1 file changed, 130 insertions(+)
>
> diff --git a/drivers/net/iavf/iavf_rxtx_vec_avx512.c
> b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
> index 5cb4c7c..6134520 100644
> --- a/drivers/net/iavf/iavf_rxtx_vec_avx512.c
> +++ b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
> @@ -25,6 +25,9 @@
>
> rxdp = rxq->rx_ring + rxq->rxrearm_start;
>
> + if (!cache)
> + goto normal;
[DC] In the Tx path, in iavf_tx_free_bufs_avx512(), it also checks for
cache->len == 0
Not sure if the extra check is necessary though - I don't know if 'cache' can
be valid pointer
but have a length of 0
if (!cache || cache->len == 0)
goto normal;
> +
> /* We need to pull 'n' more MBUFs into the software ring from
> mempool
>* We inline the mempool function here, so we can vectorize the
> copy
>* from the cache into the shadow ring.
> @@ -127,6 +130,133 @@
> cache->len -= IAVF_DESCS_PER_LOOP_AVX;
> }
>
> + goto done;
> +
> +normal:
> + /* Pull 'n' more MBUFs into the software ring */
> + if (rte_mempool_get_bulk(rxq->mp,
> + (void *)rxp,
> + IAVF_RXQ_REARM_THRESH) < 0) {
> + if (rxq->rxrearm_nb + IAVF_RXQ_REARM_THRESH >=
> + rxq->nb_rx_desc) {
> + __m128i dma_addr0;
> +
> + dma_addr0 = _mm_setzero_si128();
> + for (i = 0; i < IAVF_DESCS_PER_LOOP_AVX; i++) {
> + rxp[i] = &rxq->fake_mbuf;
> + _mm_store_si128((__m128i *)&rxdp[i].read,
> + dma_addr0);
> + }
> + }
> + rte_eth_devices[rxq->port_id].data->rx_mbuf_alloc_failed
> +=
> + IAVF_RXQ_REARM_THRESH;
> + return;
> + }
> +
> +#ifndef RTE_LIBRTE_IAVF_16BYTE_RX_DESC
> + struct rte_mbuf *mb0, *mb1;
> + __m128i dma_addr0, dma_addr1;
> + __m128i hdr_room = _mm_set_epi64x(RTE_PKTMBUF_HEADROOM,
> + RTE_PKTMBUF_HEADROOM);
> + /* Initialize the mbufs in vector, process 4 mbufs in one loop */
[DC] Comment above should say 2 mbufs I think
> + for (i = 0; i < IAVF_RXQ_REARM_THRESH; i += 2, rxp += 2) {
> + __m128i vaddr0, vaddr1;
> +
> + mb0 = rxp[0];
> + mb1 = rxp[1];
> +
> + /* load buf_addr(lo 64bit) and buf_physaddr(hi 64bit) */
> + RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, buf_iova) !=
> + offsetof(struct rte_mbuf, buf_addr) + 8);
> + vaddr0 = _mm_loadu_si128((__m128i *)&mb0->buf_addr);
> + vaddr1 = _mm_loadu_si128((__m128i *)&mb1->buf_addr);
> +
> + /* convert pa to dma_addr hdr/data */
> + dma_addr0 = _mm_unpackhi_epi64(vaddr0, vaddr0);
> + dma_addr1 = _mm_unpackhi_epi64(vaddr1, vaddr1);
> +
> + /* add headroom to pa values */
> + dma_addr0 = _mm_add_epi64(dma_addr0, hdr_room);
> + dma_addr1 = _mm_add_epi64(dma_addr1, hdr_room);
> +
> + /* flush desc with pa dma_addr */
> + _mm_store_si128((__m128i *)&rxdp++->read, dma_addr0);
> + _mm_store_si128((__m128i *)&rxdp++->read, dma_addr1);
> + }
[DC] Large blocks of the code above is the same as in avx2 file... any
possibility to have a
common function or functions?
> +#else
> + struct rte_mbuf *mb0, *mb1, *mb2, *mb3;
> + struct rte_mbuf *mb4, *mb5, *mb6, *mb7;
> + __m512i dma_addr0_3, dma_addr4_7;
> + __m512i hdr_room =
> _mm512_set1_epi64(RTE_PKTMBUF_HEADROOM);
> + /* Initialize the mbufs in vector, process 4 mbufs in one loop */
[DC] Comment above should say 8 mbufs
> + for (i = 0; i < IAVF_RXQ_REARM_THRESH;
> + i += 8, rxp += 8, rxdp += 8) {
> +
> + /**
> + * merge 0 & 1, by casting 0 to 256-bit and inserting 1
> + * into the high lanes. Similarly for 2 & 3
> + */
[DC] Comment above should say "Similarly for 2 & 3, 4 & 5, 6 & 7"
> + vaddr0_1 =
> +
> _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr0),
> + vaddr1, 1);
> + vaddr2_3 =
> +
The patch fixes the seg fault, but note I have only tested the default
'#ifndef RTE_LIBRTE_IAVF
Re: [dpdk-dev] [PATCH 2/3] net/ice: fix segment fault in AVX512
Hi Wenzhuo
> -Original Message-
> From: dev On Behalf Of Wenzhuo Lu
> Sent: Friday, March 12, 2021 1:27 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo ; sta...@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/3] net/ice: fix segment fault in AVX512
>
> Fix segment fault when failing to get the memory from the pool.
>
> Fixes: 7f85d5ebcfe1 ("net/ice: add AVX512 vector path")
> Cc: sta...@dpdk.org
>
> Reported-by: David Coyle
> Signed-off-by: Wenzhuo Lu
> ---
> drivers/net/ice/ice_rxtx_vec_avx512.c | 129
> ++
> 1 file changed, 129 insertions(+)
>
> diff --git a/drivers/net/ice/ice_rxtx_vec_avx512.c
> b/drivers/net/ice/ice_rxtx_vec_avx512.c
> index 0e5a676..7c458d5 100644
> --- a/drivers/net/ice/ice_rxtx_vec_avx512.c
> +++ b/drivers/net/ice/ice_rxtx_vec_avx512.c
> @@ -24,6 +24,9 @@
>
> rxdp = rxq->rx_ring + rxq->rxrearm_start;
>
> + if (!cache)
> + goto normal;
[DC] Same as IAVF, in the Tx path, in ice_tx_free_bufs_avx512(), it also checks
for
cache->len == 0.
Not sure if the extra check is necessary though - I don't know if 'cache' can
be valid pointer
but have a length of 0
if (!cache || cache->len == 0)
goto normal;
> +
> /* We need to pull 'n' more MBUFs into the software ring */
> if (cache->len < ICE_RXQ_REARM_THRESH) {
> uint32_t req = ICE_RXQ_REARM_THRESH + (cache->size -
> @@ -115,6 +118,132 @@
> rxep += 8, rxdp += 8, cache->len -= 8;
> }
>
> + goto done;
> +
> +normal:
> + /* Pull 'n' more MBUFs into the software ring */
> + if (rte_mempool_get_bulk(rxq->mp,
> + (void *)rxep,
> + ICE_RXQ_REARM_THRESH) < 0) {
> + if (rxq->rxrearm_nb + ICE_RXQ_REARM_THRESH >=
> + rxq->nb_rx_desc) {
> + __m128i dma_addr0;
> +
> + dma_addr0 = _mm_setzero_si128();
> + for (i = 0; i < ICE_DESCS_PER_LOOP; i++) {
> + rxep[i].mbuf = &rxq->fake_mbuf;
> + _mm_store_si128((__m128i *)&rxdp[i].read,
> + dma_addr0);
> + }
> + }
> + rte_eth_devices[rxq->port_id].data->rx_mbuf_alloc_failed
> +=
> + ICE_RXQ_REARM_THRESH;
> + return;
> + }
> +
> +#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
> + struct rte_mbuf *mb0, *mb1;
> + __m128i dma_addr0, dma_addr1;
> + __m128i hdr_room = _mm_set_epi64x(RTE_PKTMBUF_HEADROOM,
> + RTE_PKTMBUF_HEADROOM);
> + /* Initialize the mbufs in vector, process 4 mbufs in one loop */
[DC] Comment above should say 2 mbufs
> + for (i = 0; i < ICE_RXQ_REARM_THRESH; i += 2, rxep += 2) {
> + __m128i vaddr0, vaddr1;
> +
> + mb0 = rxep[0].mbuf;
> + mb1 = rxep[1].mbuf;
> +
> + /* load buf_addr(lo 64bit) and buf_physaddr(hi 64bit) */
> + RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, buf_iova) !=
> + offsetof(struct rte_mbuf, buf_addr) + 8);
> + vaddr0 = _mm_loadu_si128((__m128i *)&mb0->buf_addr);
> + vaddr1 = _mm_loadu_si128((__m128i *)&mb1->buf_addr);
> +
> + /* convert pa to dma_addr hdr/data */
> + dma_addr0 = _mm_unpackhi_epi64(vaddr0, vaddr0);
> + dma_addr1 = _mm_unpackhi_epi64(vaddr1, vaddr1);
> +
> + /* add headroom to pa values */
> + dma_addr0 = _mm_add_epi64(dma_addr0, hdr_room);
> + dma_addr1 = _mm_add_epi64(dma_addr1, hdr_room);
> +
> + /* flush desc with pa dma_addr */
> + _mm_store_si128((__m128i *)&rxdp++->read, dma_addr0);
> + _mm_store_si128((__m128i *)&rxdp++->read, dma_addr1);
> + }
[DC] As in IAVF, the code above is the same as in avx2 file... any possibility
to have a common function
or functions for the 2 files?
And there is also commonality between IAVF and ICE PMDs.
There doesn't seem to be any shared code between net PMDs at the moment though,
so maybe it's
practical to have common functions
> +#else
> + struct rte_mbuf *mb0, *mb1, *mb2, *mb3;
> + struct rte_mbuf *mb4, *mb5, *mb6, *mb7;
> + __m512i dma_addr0_3, dma_addr4_7;
> + __m512i hdr_room =
> _mm512_set1_epi64(RTE_PKTMBUF_HEADROOM);
> + /* Initialize the mbufs in vector, process 4 mbufs in one loop */
[DC] Comment above should say 8 mbufs
> + for (i = 0; i < ICE_RXQ_REARM_THRESH;
> + i += 8, rxep += 8, rxdp += 8) {
> +
> + /**
> + * merge 0 & 1, by casting 0 to 256-bit and inserting 1
> + * into the high lanes. Similarly for 2 & 3
> + */
[DC] Comment above should say "Similarly for 2 & 3, 4 & 5, 6 & 7"
> + vaddr0_1 =
> +
> _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr0),
> +
Re: [dpdk-dev] [PATCH 3/3] net/i40e: fix segment fault in AVX512
Hi Wenzhuo
> -Original Message-
> From: dev On Behalf Of Wenzhuo Lu
> Sent: Friday, March 12, 2021 1:27 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo ; sta...@dpdk.org
> Subject: [dpdk-dev] [PATCH 3/3] net/i40e: fix segment fault in AVX512
>
> Fix segment fault when failing to get the memory from the pool.
>
> Fixes: e6a6a138919f ("net/i40e: add AVX512 vector path")
> Cc: sta...@dpdk.org
>
> Reported-by: David Coyle
> Signed-off-by: Wenzhuo Lu
> ---
> drivers/net/i40e/i40e_rxtx_vec_avx512.c | 128
>
> 1 file changed, 128 insertions(+)
>
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> index 862c916..36521da 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> +++ b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> @@ -32,6 +32,9 @@
>
> rxdp = rxq->rx_ring + rxq->rxrearm_start;
>
> + if (!cache)
> + goto normal;
[DC] Like in IAVF and ICE, should we also check for cache->len == 0, like is
done in Tx path?
> +
> /* We need to pull 'n' more MBUFs into the software ring from
> mempool
>* We inline the mempool function here, so we can vectorize the
> copy
>* from the cache into the shadow ring.
> @@ -132,7 +135,132 @@
> #endif
> rxep += 8, rxdp += 8, cache->len -= 8;
> }
> + goto done;
> +
> +normal:
> + /* Pull 'n' more MBUFs into the software ring */
> + if (rte_mempool_get_bulk(rxq->mp,
> + (void *)rxep,
> + RTE_I40E_RXQ_REARM_THRESH) < 0) {
> + if (rxq->rxrearm_nb + RTE_I40E_RXQ_REARM_THRESH >=
> + rxq->nb_rx_desc) {
> + __m128i dma_addr0;
> +
> + dma_addr0 = _mm_setzero_si128();
> + for (i = 0; i < RTE_I40E_DESCS_PER_LOOP; i++) {
> + rxep[i].mbuf = &rxq->fake_mbuf;
> + _mm_store_si128((__m128i *)&rxdp[i].read,
> + dma_addr0);
> + }
> + }
> + rte_eth_devices[rxq->port_id].data->rx_mbuf_alloc_failed
> +=
> + RTE_I40E_RXQ_REARM_THRESH;
> + return;
> + }
> +
> +#ifndef RTE_LIBRTE_I40E_16BYTE_RX_DESC
> + struct rte_mbuf *mb0, *mb1;
> + __m128i dma_addr0, dma_addr1;
> + __m128i hdr_room = _mm_set_epi64x(RTE_PKTMBUF_HEADROOM,
> + RTE_PKTMBUF_HEADROOM);
> + /* Initialize the mbufs in vector, process 4 mbufs in one loop */
[DC] Comment should say 2 mbufs
> + for (i = 0; i < RTE_I40E_RXQ_REARM_THRESH; i += 2, rxep += 2) {
> + __m128i vaddr0, vaddr1;
> +
> + mb0 = rxep[0].mbuf;
> + mb1 = rxep[1].mbuf;
> +
> + /* load buf_addr(lo 64bit) and buf_physaddr(hi 64bit) */
> + RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, buf_iova) !=
> + offsetof(struct rte_mbuf, buf_addr) + 8);
> + vaddr0 = _mm_loadu_si128((__m128i *)&mb0->buf_addr);
> + vaddr1 = _mm_loadu_si128((__m128i *)&mb1->buf_addr);
> +
> + /* convert pa to dma_addr hdr/data */
> + dma_addr0 = _mm_unpackhi_epi64(vaddr0, vaddr0);
> + dma_addr1 = _mm_unpackhi_epi64(vaddr1, vaddr1);
> +
> + /* add headroom to pa values */
> + dma_addr0 = _mm_add_epi64(dma_addr0, hdr_room);
> + dma_addr1 = _mm_add_epi64(dma_addr1, hdr_room);
> +
> + /* flush desc with pa dma_addr */
> + _mm_store_si128((__m128i *)&rxdp++->read, dma_addr0);
> + _mm_store_si128((__m128i *)&rxdp++->read, dma_addr1);
> + }
> +#else
> + struct rte_mbuf *mb0, *mb1, *mb2, *mb3;
> + struct rte_mbuf *mb4, *mb5, *mb6, *mb7;
> + __m512i dma_addr0_3, dma_addr4_7;
> + __m512i hdr_room =
> _mm512_set1_epi64(RTE_PKTMBUF_HEADROOM);
> + /* Initialize the mbufs in vector, process 4 mbufs in one loop */
[DC] Comment should say 8 mbufs
> + for (i = 0; i < RTE_I40E_RXQ_REARM_THRESH;
> + i += 8, rxep += 8, rxdp += 8) {
> + __m128i vaddr0, vaddr1, vaddr2, vaddr3;
> + __m128i vaddr4, vaddr5, vaddr6, vaddr7;
> + vaddr6 = _mm_loadu_si128((__m128i *)&mb6->buf_addr);
> + vaddr7 = _mm_loadu_si128((__m128i *)&mb7->buf_addr);
> +
> + /**
> + * merge 0 & 1, by casting 0 to 256-bit and inserting 1
> + * into the high lanes. Similarly for 2 & 3
> + */
[DC] Comment should say "Similarly for 2 & 3, 4 & 5, 6 & 7"
> + vaddr0_1 =
> +
> _mm256_inserti128_si256(_mm256_castsi128_si256(vaddr0),
> + vaddr1, 1);
> + /* flush desc with pa dma_addr */
> + _mm512_store_si512((__m512i *)&rxdp->read,
> dma_addr0_3);
> + _mm512_store_si512((__m512i *)&(rxdp +
Re: [dpdk-dev] [PATCH] net/iavf: fix pkt len parsing in AVX512
Hi Leyi
> -Original Message-
> From: dev On Behalf Of Leyi Rong
> Sent: Wednesday, March 17, 2021 9:18 AM
> To: Zhang, Qi Z ; Lu, Wenzhuo
> ; Xing, Beilei
> Cc: dev@dpdk.org; Rong, Leyi
> Subject: [dpdk-dev] [PATCH] net/iavf: fix pkt len parsing in AVX512
>
> Fix pkt_len parsing when DEV_RX_OFFLOAD_KEEP_CRC is set in AVX512
> path.
>
> Fixes: 31737f2b66fb ("net/iavf: enable AVX512 for legacy Rx")
> Fixes: 6df587028e57 ("net/iavf: enable AVX512 for flexible Rx")
>
> Signed-off-by: Leyi Rong
> ---
> drivers/net/iavf/iavf_rxtx_vec_avx512.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/iavf/iavf_rxtx_vec_avx512.c
> b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
> index 5cb4c7cda6..67184ae3f4 100644
> --- a/drivers/net/iavf/iavf_rxtx_vec_avx512.c
> +++ b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
> @@ -380,7 +380,7 @@ _iavf_recv_raw_pkts_vec_avx512(struct
> iavf_rx_queue *rxq,
> len4_7);
> __m512i mb4_7 = _mm512_shuffle_epi8(desc4_7,
> shuf_msk);
>
> - mb4_7 = _mm512_add_epi16(mb4_7, crc_adjust);
> + mb4_7 = _mm512_add_epi32(mb4_7, crc_adjust);
> /**
>* to get packet types, shift 64-bit values down 30 bits
>* and so ptype is in lower 8-bits in each @@ -411,7 +411,7
> @@ _iavf_recv_raw_pkts_vec_avx512(struct iavf_rx_queue *rxq,
> len0_3);
> __m512i mb0_3 = _mm512_shuffle_epi8(desc0_3,
> shuf_msk);
>
> - mb0_3 = _mm512_add_epi16(mb0_3, crc_adjust);
> + mb0_3 = _mm512_add_epi32(mb0_3, crc_adjust);
> /* get the packet types */
> const __m512i ptypes0_3 = _mm512_srli_epi64(desc0_3, 30);
> const __m256i ptypes2_3 =
> _mm512_extracti64x4_epi64(ptypes0_3, 1); @@ -869,7 +869,7 @@
> _iavf_recv_raw_pkts_vec_avx512_flex_rxd(struct iavf_rx_queue *rxq,
>*/
> __m512i mb4_7 = _mm512_shuffle_epi8(raw_desc4_7,
> shuf_msk);
>
> - mb4_7 = _mm512_add_epi16(mb4_7, crc_adjust);
> + mb4_7 = _mm512_add_epi32(mb4_7, crc_adjust);
> /**
>* to get packet types, ptype is located in bit16-25
>* of each 128bits
> @@ -898,7 +898,7 @@ _iavf_recv_raw_pkts_vec_avx512_flex_rxd(struct
> iavf_rx_queue *rxq,
>*/
> __m512i mb0_3 = _mm512_shuffle_epi8(raw_desc0_3,
> shuf_msk);
>
> - mb0_3 = _mm512_add_epi16(mb0_3, crc_adjust);
> + mb0_3 = _mm512_add_epi32(mb0_3, crc_adjust);
> /**
>* to get packet types, ptype is located in bit16-25
>* of each 128bits
> --
> 2.25.1
This patch fixes the issue
Tested-by: David Coyle
Re: [dpdk-dev] [PATCH v2] net/i40e: fix avx2 driver check for rx rearm
-Original Message-
From: Van Haaren, Harry
Sent: Monday, July 30, 2018 6:34 PM
To: dev@dpdk.org
Cc: Van Haaren, Harry ; Richardson, Bruce
; sta...@dpdk.org; tho...@monjalon.net; Coyle,
David ; Xing, Beilei ; Zhang, Qi
Z
Subject: [PATCH v2] net/i40e: fix avx2 driver check for rx rearm
This commit fixes an infinite loop bug that could occur if the i40e AVX2 driver
is used, and high traffic rates cause the mempool from which the rxq pulls
mbufs to become empty.
The result would be an infinite loop of checking if we should perform an rx
rearm, calling the function and an error return due the the mempool being emtpy.
The fix is to align the code in the AVX2 driver with the SSE driver, where an
if() is used instead of a while(), allowing the thread to return from i40e rx
function even if the mempool is empty.
Fixes: dafadd73762e ("net/i40e: add AVX2 Rx function")
Cc: bruce.richard...@intel.com
Cc: sta...@dpdk.org
Reported-by: David Coyle
Signed-off-by: Harry van Haaren
Acked-by: Brendan Ryan
Tested-by: David Coyle
--
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
Re: [dpdk-dev] [PATCH v3 3/8] crypto/aesni_mb: add support for DOCSIS protocol
> diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> index 2d688f4d3..4b25c5e23 100644
> --- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> +++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
> @@ -1647,7 +1914,23 @@ cryptodev_aesni_mb_create(const char *name,
> RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING |
> RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT |
> RTE_CRYPTODEV_FF_SYM_CPU_CRYPTO |
> - RTE_CRYPTODEV_FF_SYM_SESSIONLESS;
> + RTE_CRYPTODEV_FF_SYM_SESSIONLESS
> +#ifdef AESNI_MB_DOCSIS_SEC_ENABLED
> + | RTE_CRYPTODEV_FF_SECURITY
> +#endif
> + ;
> +
> +#ifdef AESNI_MB_DOCSIS_SEC_ENABLED
> + security_instance = rte_malloc("aesni_mb_sec",
> + sizeof(struct rte_security_ctx), 0);
> + if (security_instance == NULL)
> + AESNI_MB_LOG(ERR, "rte_security_ctx memory alloc
> failed\n");
> +
> + security_instance->device = (void *)dev;
[DC] Possible NULL pointer dereference here... will fix in v4
> + security_instance->ops = rte_aesni_mb_pmd_sec_ops;
> + security_instance->sess_cnt = 0;
> + dev->security_ctx = security_instance; #endif
>
Re: [dpdk-dev] [PATCH v3 8/8] doc: add doc updates for DOCSIS security protocol
Hi Akhil, thank you for these comments > > This patch should be split and merged to relevant other patches in the series. > rte_security related in 1/8 > Like aesni-mb related changes should go in 3/8 qat related should be part of > 4/8 crypto-perf should be part of 7/8 And release notes should also be split > into 3 different entries and squashed into Rte_security, qat and aesni-mb > patches. [DC] I will make this change in v4. I will wait until there are more comments before submitting v4. > > + > > +The CRC is Ethernet CRC-32 as specified in Ethernet/[ISO/IEC 8802-3]. > > + > > +.. note:: > > + > > +* The CRC offset and length are specified via the auth offset and > > + length fields of the rte_crypto_sym_op. > > The above note is not correct. It should be > * The offset and length of data for which CRC need to be computed are > specified > via the auth offset and length fields of the rte_crypto_sym_op. [DC] Yes, that is a good clarification - I will make that update > > > > +* **Added support for DOCSIS protocol to rte_security.** > > + > > + Added support for combined crypto and CRC operations for the DOCSIS > > protocol > > + to ``rte_security``. Test and test-crypto-perf applications have > > + been updated for unit testing. > > Split this release note entry into two, 1 for 1/8 and one for 7/8 [DC] I will make this change
Re: [dpdk-dev] [PATCH v3 4/8] crypto/qat: add support for DOCSIS protocol
> diff --git a/drivers/crypto/qat/qat_sym_pmd.c
> b/drivers/crypto/qat/qat_sym_pmd.c
> index e887c880f..711d1585f 100644
> --- a/drivers/crypto/qat/qat_sym_pmd.c
> +++ b/drivers/crypto/qat/qat_sym_pmd.c
> @@ -308,7 +346,20 @@ qat_sym_dev_create(struct qat_pci_device
> *qat_pci_dev,
> RTE_CRYPTODEV_FF_OOP_SGL_IN_LB_OUT |
> RTE_CRYPTODEV_FF_OOP_LB_IN_SGL_OUT |
> RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT |
> - RTE_CRYPTODEV_FF_DIGEST_ENCRYPTED;
> + RTE_CRYPTODEV_FF_DIGEST_ENCRYPTED |
> + RTE_CRYPTODEV_FF_SECURITY;
> +
> +#ifdef RTE_LIBRTE_SECURITY
> + security_instance = rte_malloc("qat_sec",
> + sizeof(struct rte_security_ctx), 0);
> + if (security_instance == NULL)
> + QAT_LOG(ERR, "rte_security_ctx memory alloc failed\n");
> +
> + security_instance->device = (void *)cryptodev;
[DC] Possible NULL pointer dereference here... will fix in v4
> + security_instance->ops = &security_qat_ops;
> + security_instance->sess_cnt = 0;
> + cryptodev->security_ctx = security_instance; #endif
>
> internals = cryptodev->data->dev_private;
> internals->qat_dev = qat_pci_dev;
Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-function processing
Thank you Thomas for your input. We would like to request that the Tech-Board (CC'ed) also review the proposal to help us reach a consensus. If the current proposal is not acceptable, we would welcome feedback from the board on how to rework our proposal to something that would be acceptable. For the benefit of the Tech-Board here is the back-ground to our proposal for Rawdev-based multi-function processing: - The primary objective is to support the AESNI MB combined Crypto-CRC processing capability in DPDK and in future to add support for combined Crypto-CRC support in QAT. - The cryptodev API was considered unsuitable because CRC is not a cryptographic operation, and this would also preclude other non-crypto operations in the future such as compression. - The rte_security API was also not considered suitable for chaining of non-crypto operations such as CRC, as Declan pointed out below. - A new Accelerator API was proposed as an RFC but was not pursued due to community feedback that a new API would not be welcome for a single use-case. - Using Rawdev for multi-function processing was then proposed and, initially, as there was no opposition we implemented a patch-set for this approach. It was considered that a Rawdev-based multi-function approach would be suitable for the following reasons: 1) Multi-function processing for Crypto-CRC cases is not a good fit for any of the existing DPDK classes. 2) Rawdev was intended for such specialized acceleration processing that are not a good fit for existing DPDK classes. 3) Rawdev was also intended as somewhere that new use-cases like this could be prototyped and developed, such as Declan mentions below 4) The Rawdev-based multi-function proposal is extensible and we would hope that it can evolve to support new use-cases and target new devices in the future with the communities involvement. > -Original Message- > From: Doherty, Declan > Sent: Tuesday, April 21, 2020 5:46 PM > > On 15/04/2020 11:33 PM, Thomas Monjalon wrote: > > 16/04/2020 00:19, Doherty, Declan: > >> On 14/04/2020 3:44 PM, Thomas Monjalon wrote: > >>> 14/04/2020 16:02, Trahe, Fiona: > From: Thomas Monjalon > > 14/04/2020 15:04, Trahe, Fiona: > >>> 14/04/2020 12:21, Ferruh Yigit: > >>> > > > http://inbox.dpdk.org/dev/MN2PR11MB35507D4B96677A41E66440C5E3C30 > @M > > N2PR11MB3550.na > >>> mprd11.prod.outlook.com/ > >>> > >>> I am not convinced. > >>> I don't like rawdev in general. > >>> Rawdev is good only for hardware support which cannot be generic > >>> like SoC, FPGA management or DMA engine. > >> > >> [Fiona] CRC and BIP are not crypto algorithms, they are error > detection processes. > >> So there is no class in DPDK that these readily fit into. > >> There was resistance to adding another xxxddev, and even if one > >> had been added for error_detection_dev, there would still have > >> been another layer needed to couple this with cryptodev. Various > >> proposals for this have been discussed on the ML in RFC and recent > patches, there doesn't seem to be an appetite for this as a generic API. > >> So it seems that only Intel has software and hardware engines > >> that provide this specialised feature coupling. In that case > >> rawdev seems like the most appropriate vehicle to expose this. > > > > Adding some vendor-specific API is not a good answer. > > It will work in some cases, but it won't make DPDK better. > > What's the purpose of DPDK if it's not solving a common problem > > for different hardware? > > >> The current proposal in rawdev could easily be supported by any > >> hardware which supports chaining multiple functions/services into a > >> single operation, in this case symmetric crypto and error detection, > >> but it could conceivably support chaining symmetric/asymmetric crypto > >> operations or chaining symmetric crypto and compression operations. > >> > [Fiona] Based on that logic rawdev should be deprecated. > But the community has agreed that it has a place. > >>> > >>> No, as I said above, rawdev is good for SoC, FPGA management or DMA > engine. > >> > >> I distinctly remember when rawdev was being proposed one of the uses > >> cases proposed was that a new classes of APIs could be prototyped and > >> developed under rawdev and when a solid consensus was reached then > >> migrated to a mainstream DPDK library. I think every effort has been > >> made here to engage the community to develop a generic approach. As > >> Fiona notes there hasn't really been much of an appetite for this. > >> > >> Therefore I think the option to use rawdev makes sense, it allows an > >> initial proposal to be deployed, without a generic solution > >> agreement, it will also give others in the community to see how this > >> approach can work and hopefully lead to more engagement on a generic > >> solution. Also
Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-function processing
> -Original Message- > From: Thomas Monjalon > Sent: Tuesday, April 21, 2020 6:25 PM [PATCH v3 0/4] add AESNI-MB rawdev for multi- > function processing > > 21/04/2020 18:46, Doherty, Declan: > > On 15/04/2020 11:33 PM, Thomas Monjalon wrote: > > > 16/04/2020 00:19, Doherty, Declan: > > >> On 14/04/2020 3:44 PM, Thomas Monjalon wrote: > > >>> 14/04/2020 16:02, Trahe, Fiona: > > From: Thomas Monjalon > > > 14/04/2020 15:04, Trahe, Fiona: > > >>> 14/04/2020 12:21, Ferruh Yigit: > > >>> > > > > http://inbox.dpdk.org/dev/MN2PR11MB35507D4B96677A41E66440C5E3C30 > > > @MN2PR11MB3550.na > > >>> mprd11.prod.outlook.com/ > > >>> > > >>> I am not convinced. > > >>> I don't like rawdev in general. > > >>> Rawdev is good only for hardware support which cannot be > > >>> generic like SoC, FPGA management or DMA engine. > > >> > > >> [Fiona] CRC and BIP are not crypto algorithms, they are error > detection processes. > > >> So there is no class in DPDK that these readily fit into. > > >> There was resistance to adding another xxxddev, and even if one > > >> had been added for error_detection_dev, there would still have > > >> been another layer needed to couple this with cryptodev. > > >> Various proposals for this have been discussed on the ML in RFC > and recent patches, there doesn't seem to be an appetite for this as a > generic API. > > >> So it seems that only Intel has software and hardware engines > > >> that provide this specialised feature coupling. In that case > > >> rawdev seems like the most appropriate vehicle to expose this. > > > > > > Adding some vendor-specific API is not a good answer. > > > It will work in some cases, but it won't make DPDK better. > > > What's the purpose of DPDK if it's not solving a common problem > > > for different hardware? > > > > >> The current proposal in rawdev could easily be supported by any > > >> hardware which supports chaining multiple functions/services into a > > >> single operation, in this case symmetric crypto and error > > >> detection, but it could conceivably support chaining > > >> symmetric/asymmetric crypto operations or chaining symmetric crypto > and compression operations. > > >> > > [Fiona] Based on that logic rawdev should be deprecated. > > But the community has agreed that it has a place. > > >>> > > >>> No, as I said above, rawdev is good for SoC, FPGA management or > DMA engine. > > >> > > >> I distinctly remember when rawdev was being proposed one of the > > >> uses cases proposed was that a new classes of APIs could be > > >> prototyped and developed under rawdev and when a solid consensus > > >> was reached then migrated to a mainstream DPDK library. I think > > >> every effort has been made here to engage the community to develop > > >> a generic approach. As Fiona notes there hasn't really been much of an > appetite for this. > > >> > > >> Therefore I think the option to use rawdev makes sense, it allows > > >> an initial proposal to be deployed, without a generic solution > > >> agreement, it will also give others in the community to see how > > >> this approach can work and hopefully lead to more engagement on a > > >> generic solution. Also as APIs in rawdev are essentially treated as > > >> private APIs the onus is on Intel to support this going forward. > > > > > > Because hardware support is pending, we should accept an Intel-only > > > "temporary" solution, opening the door to more vendor-specific APIs? > > > > > > What is the benefit for the DPDK project? > > > > Sorry I don't agree with this sentiment, David has made every attempt > > to solicit feedback an to engage the community in this. > > Really? > > These are the recipients of the first patch: > dev@dpdk.org, declan.dohe...@intel.com, fiona.tr...@intel.com In > next patches, only Intel and NXP are Cc'ed. > Stephen and Jerin, who gave good comments on first patch, were not Cc'ed > in next versions. > > Was it presented in an event? > Was it brought to the techboard? > Please don't exagerate and admit you are trying to push something which is > specific and convenient for Intel QuickAssist. [DC] This is being brought to the TechBoard tomorrow (22/04) > > > > I also don't agree in classifying this as a "temporary solution" as > > this is a solid proposal for an approach to chaining multiple > > operations together, but I guess the fact remains that we only > > currently have a single use-case, but it is difficult to generate a > > generic solution in this case. > > > > While there is only a single use case it is targeting two devices so > > that drove the need for a common interface withing rawdev. > > > > The advantage of using rawdev is that it allows this to be consumed > > through DPDK, which enables DPDK project consumers, but also leaves > > the door open to other contributors to have their say on how this > > should evolve. For
Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-function processing
Hi Akhil, > -Original Message- > From: Akhil Goyal > Sent: Wednesday, April 22, 2020 11:51 AM > Hi David, > > > >> > > > >> I don't agree rte_security addresses the problem of different > > > >> device types supporting the same services. The problem being > > > >> addressed here is a single device which supports the chaining of > > > >> multiple services (sym crypto & error detection) > > > > > > > > Doing IPsec processing in Rx or Tx of a NIC is not chaining? > > > > > > > I wouldn't consider an inline crypto offload or full IPsec offload a > > > chained operation in the vein being proposed here where completely > > > independent services (in the view of DPDK which are currently on > > > independent devices and APIs) are linked together. > > > > > > We did look at using rte_security here but it wasn't considered > > > suitable for a chaining of non-crypto operations such as CRC or > > > possibly compression in the future, as it would still run into the > > > issue of having to use the cryptodev enq/deq API in the lookaside offload > case. > > > > > > > I did not look at your patches completely, but looking at the ops that you > have added For rawdev are pretty much same as that of a crypto device. > > I see that there are 2 types of ops that you need > - session create/destroy > - enq/deq > > On the first impression of your patchset, I see that you want to enq to driver > only once for both The operations - CRC and crypto. > > So what is the issue in using the cryptodev_enqueue for processing in the > existing AESNI-MB driver. > For session creation, the cryptodev layer will not give flexibility to add > CRC+crypto kind of sessions. > But in case of rte_security, you can define your new session xform based on > your requirement. > > And while doing the cryptodev enq/deq, based on the session type, you can > process the packet Specific to your usecase in your aesni-mb PMD > > Now if you want to add compression also along with crypto, then you can > define another xform which Will be combination of crypto+compression and > the aesni-mb PMD can have another mode which Can make sessions based > on the new xform and the enq and deq can be done using the cryptodev > enq/deq. > For all your cases you will be having only one action type - lookaside > protocol > and can define different Protocols (that may not be standard). > > So to conclude, your AESNI-MB will have 3 types of operations > - plain crypto > - crc+crypto > - compression+crypto > > I believe this is doable or did I miss something very obvious? [DC] Thank you for this feedback I have done this exact same analysis on rte_security and how we could use it. The main issue of this approach (and it may be possible to easily overcome) is that ultimately crypto_op's need to be enqueued into cryptodev. This means we can't easily control the CRC (or compression in the future) at the operation level - application developers using this API would create a Crypto+CRC security xform session for a particular flow but may want to turn off the CRC part for some packets in that flow. There are a number of ways this issue could possibly be overcome: 1) the auth offset/length fields in a rte_crypto_op could be overloaded to control the CRC part of the combined operation - this is not the cleanest approach 2) we add a "security" op struct of some type to the union at end of the rte_crypto_op - to avoid any circular dependencies, this would need to be opaque to rte_cryptodev - rte_cryptodev should not be aware of rte_security Number 2 above is probably the cleaner and more preferable approach. The other approach is that CRC is either on/off at the session level. That limitation would then need to be adhered by application developers, which is something we would ideally like to avoid. The rawdev multi-function approach did not have these issues which is one of the reasons we have pursued this approach to date. However, we think the rte_security approach is workable. It still requires some deeper analysis but with your support, we think we can overcome the challenges. > > Regards, > Akhil
Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-function processing
Hi Akhil
> -Original Message-
> From: Akhil Goyal
> Sent: Wednesday, April 22, 2020 2:44 PM
>
> Hi David,
> > Hi Akhil,
> >
> > > > >
> > > I did not look at your patches completely, but looking at the ops
> > > that you have added For rawdev are pretty much same as that of a crypto
> device.
> > >
> > > I see that there are 2 types of ops that you need
> > > - session create/destroy
> > > - enq/deq
> > >
> > > On the first impression of your patchset, I see that you want to enq
> > > to driver only once for both The operations - CRC and crypto.
> > >
> > > So what is the issue in using the cryptodev_enqueue for processing
> > > in the existing AESNI-MB driver.
> > > For session creation, the cryptodev layer will not give flexibility
> > > to add
> > > CRC+crypto kind of sessions.
> > > But in case of rte_security, you can define your new session xform
> > > based on your requirement.
> > >
> > > And while doing the cryptodev enq/deq, based on the session type,
> > > you can process the packet Specific to your usecase in your aesni-mb
> > > PMD
> > >
> > > Now if you want to add compression also along with crypto, then you
> > > can define another xform which Will be combination of
> > > crypto+compression and the aesni-mb PMD can have another mode
> which
> > > Can make sessions based on the new xform and the enq and deq can be
> > > done using the cryptodev enq/deq.
> > > For all your cases you will be having only one action type -
> > > lookaside protocol and can define different Protocols (that may not be
> standard).
> > >
> > > So to conclude, your AESNI-MB will have 3 types of operations
> > > - plain crypto
> > > - crc+crypto
> > > - compression+crypto
> > >
> > > I believe this is doable or did I miss something very obvious?
> >
> > [DC] Thank you for this feedback
> >
> > I have done this exact same analysis on rte_security and how we could use
> it.
> >
> > The main issue of this approach (and it may be possible to easily
> > overcome) is that ultimately crypto_op's need to be enqueued into
> > cryptodev. This means we can't easily control the CRC (or compression
> > in the future) at the operation level - application developers using
> > this API would create a
> > Crypto+CRC security xform session for a
> > particular flow but may want to turn off the CRC part for some packets
> > in that flow.
> >
> > There are a number of ways this issue could possibly be overcome:
> > 1) the auth offset/length fields in a rte_crypto_op could be
> > overloaded to control the CRC part of the combined operation
> > - this is not the cleanest approach
> > 2) we add a "security" op struct of some type to the union at end of
> > the rte_crypto_op
> > - to avoid any circular dependencies, this would need to be opaque
> > to rte_cryptodev
> > - rte_cryptodev should not be aware of rte_security
> >
> > Number 2 above is probably the cleaner and more preferable approach.
>
> Yes, it is preferred, but it should be a union to
> rte_crypto_sym_op/rte_crypto_asym_op.
> Crypto_op->type as RTE_CRYPTO_OP_TYPE_SECURITY and sess_type as
> RTE_CRYPTO_OP_SECURITY_SESSION The size of rte_crypto_op will remain
> as is and there will be no ABI breakage I guess.
[DC] Yes we would add to this union at the end of rte_crypto_op
__extension__
union {
struct rte_crypto_sym_op sym[0];
/**< Symmetric operation parameters */
struct rte_crypto_asym_op asym[0];
/**< Asymmetric operation parameters */
}; /**< operation specific parameters */
I haven't figured out the finer details yet, but it should be straightforward
to add some security element here.
As these are zero length arrays, we won't be affecting the size of
rte_crypto_op if we add another zero length array.
We should not include rte_security.h and add something like struct
rte_security_op sec[0] here though, as that would
cause a circular dependency between rte_cryptodev and rte_security.
This should be resolvable though
>
> One more thing that can be looked into is the recently added CPU crypto
> process API If that could of any use, we may extend that if need be.
[DC] This is also being targeted at QAT and we would like to maintain the same
Interface for these use-cases for both AESNI-MB and QAT.
So I think the traditional enqueue/dequeue API is what we would initially use
as it
means users of this API can easily switch between AESNI-MB and QAT. However, we
may look at the CPU crypto API for AESNI-MB in the future.
>
> >
> > The other approach is that CRC is either on/off at the session level.
> > That limitation would then need to be adhered by application
> > developers, which is something we would ideally like to avoid.
>
> You mean that CRC can be on/off per session as well as per packet?
> I think that can also be handled when you are defining your own security_op
> for per packet.
[DC] I meant that if we didn't take the approach defining a secu
Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-function processing
Hi Kevin, > -Original Message- > From: Kevin Traynor > Sent: Wednesday, April 22, 2020 3:02 PM > > Hi David, > > On 21/04/2020 18:23, Coyle, David wrote: > > Thank you Thomas for your input. > > > > We would like to request that the Tech-Board (CC'ed) also review the > proposal to help us reach a consensus. > > > > The discussion on the mailing list still looks active and I think that's > where it > should continue until there is no reasonable hope of consensus. > I'm not sure discussing over irc at TB will find a better technical solution. [DC] Yes, there has been some further proposals and discussions today, mainly around the use of rte_security. Although there are a few challenges to work around here, it is an approach we had already considered and it would seem like a good compromise to the rawdev approach. With that in mind, we are happy to let this play out further on the mailing list for now. If we can reach consensus on using rte_security, then we are happy to go with that and investigate and develop it further. > > > If the current proposal is not acceptable, we would welcome feedback > > from the board on how to rework our proposal to something that would be > acceptable. > > > > For the benefit of the Tech-Board here is the back-ground to our > > proposal for Rawdev-based multi-function > > processing: > > - The primary objective is to support the AESNI MB combined Crypto-CRC > processing capability in DPDK and > >in future to add support for combined Crypto-CRC support in QAT. > > - The cryptodev API was considered unsuitable because CRC is not a > cryptographic operation, and this would > >also preclude other non-crypto operations in the future such as > compression. > > - The rte_security API was also not considered suitable for chaining of non- > crypto operations such as CRC, > >as Declan pointed out below. > > - A new Accelerator API was proposed as an RFC but was not pursued due > to community feedback that a > >new API would not be welcome for a single use-case. > > - Using Rawdev for multi-function processing was then proposed and, > initially, as there was no opposition > >we implemented a patch-set for this approach. > > > > It was considered that a Rawdev-based multi-function approach would be > suitable for the following reasons: > > 1) Multi-function processing for Crypto-CRC cases is not a good fit for any > > of > the existing DPDK classes. > > 2) Rawdev was intended for such specialized acceleration processing that > are not a good fit for existing DPDK > > classes. > > 3) Rawdev was also intended as somewhere that new use-cases like this > could be prototyped and developed, > > such as Declan mentions below > > 4) The Rawdev-based multi-function proposal is extensible and we would > hope that it can evolve to support > > new use-cases and target new devices in the future with the > communities involvement. > > > > This is a useful summary and explaining your approach but it doesn't mention > the counter arguments, so it doesn't seem balanced. Of course people can > read that in the ML thread. [DC] That is a fair point, and something we will keep in mind for the future if we need to come back to the tech-board. > > Kevin. >
Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-function processing
Hi Fan & Akhil, > -Original Message- > From: Zhang, Roy Fan > Sent: Friday, May 1, 2020 2:18 PM > > Hi Akhil, > > > -Original Message- > > From: dev On Behalf Of Akhil Goyal > > Sent: Wednesday, April 22, 2020 2:44 PM > > To: Coyle, David ; Doherty, Declan > > ; Thomas Monjalon > ; > > Yigit, Ferruh ; Trahe, Fiona > > > > Cc: techbo...@dpdk.org; dev@dpdk.org; De Lara Guarch, Pablo > > ; Ryan, Brendan > > ; Hemant Agrawal > ; > > Anoob Joseph ; Ruifeng Wang > > ; Liron Himi ; Nagadheeraj > > Rottela ; Srikanth Jampala > > ; Gagandeep Singh ; Jay > Zhou > > ; Ravi Kumar ; > > Richardson, Bruce ; > > olivier.m...@6wind.com; honnappa.nagaraha...@arm.com; Stephen > > Hemminger ; al...@mellanox.com > > Subject: Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi- > > function processing > ... > > Yes, it is preferred, but it should be a union to > > rte_crypto_sym_op/rte_crypto_asym_op. > > Crypto_op->type as RTE_CRYPTO_OP_TYPE_SECURITY and sess_type as > > RTE_CRYPTO_OP_SECURITY_SESSION The size of rte_crypto_op will remain > > as is and there will be no ABI breakage I guess. > > > [Fan: with this way the PMD will have to do rte_crypto_op.type check, and > then look into rte_security_op field, only when it find the security_op type > is > crypto_crc, it will process the security_op data. Would that being too many > reads and checking for a single op? Can we create a new API for rte_security > to process rte_security_ops for Crypto_CRC or future needs?] ... [DC] If we were to add new enqueue/dequeue APIs to rte_security, then this may cause extra churn and extra paths of code in a customer's application. For the DOCSIS Crypto-CRC use-case which is currently supported by IPSecMB, only the AES-DOCSISBPI cipher algorithm is supported. For these Crypto-CRC ops, they would create rte_security sessions, attach these to rte_security_ops and enqueue/dequeue using the new APIs in rte_security. However, the customer may also be using the legacy DES-DOCSISBPI cipher algorithm for some subscribers, and this algorithm is not supported in the chained Crypto-CRC functionality in IPSecMB (and most likely never will be). So for these the customer would need to create cryptodev sessions, attach these to rte_crypto_ops and enqueue/ dequeue with the cryptodev enq/deq APIs. That is 2 different paths of code now in the application datapath, where some packets in a batch need to be enqueued through rte_security and some need to be enqueued through cryptodev. If rte_crypto_ops are always used and enqueued/dequeued through cryptodev, then the only thing that changes is the type of session that is created and either the security session or the cryptodev session gets attached to the crypto_op. Now, we could add support to rte_security for DES-DOCSISBPI too, but it would not be a combined operation with CRC - it would be a simple cipher operation going through rte_security. But that, to me, does not seem like a good use of rte_security. For DOCSIS Crypto-CRC, we may also want to take advantage of the rte_cryptodev_sym_cpu_crypto_process() API which was added to cryptodev recently to avoid the enqueue/dequeue overhead. A similar API would also then need to be added to rte_security. Taking all of the above into account, I feel keeping the normal cryptodev enqueue/dequeue would be best. Having said all that, we do need to consider performance in the PMD of the extra op type checks. Take aesni_mb PMD as an example. It would need to check rte_crypto_op->type and if it's not RTE_CRYPTO_OP_TYPE_SECURITY, then it can assume it's an RTE_CRYPTO_OP_TYPE_SYMMETRIC op and carry on as normal for existing symmetric operations. Security ops will need some extra parsing but this is new functionality. The impact on existing functionality of the extra checks would certainly need to be tested though, but as all the op data will be in the same cache line, I don't see any major impact. Akhil & Fan (& others), I would be interested to hear your feedback on this. Regards, David > > Regards, > Fan
Re: [dpdk-dev] [PATCH v2 0/4] introduce multi-function processing support
Hi Ferruh, see below > > > > While DPDK's rte_cryptodev and rte_compressdev allow many > > cryptographic and compression algorithms to be chained together in one > > operation, there is no way to chain these with any error detection or > > checksum algorithms. And there is no way to chain crypto and > > compression algorithms together. The multi-function interface will > > allow these chains to be created, and also allow any future type of > operation to be easily added. > > I was thinking if the cryptodev can be used instead but this paragraph already > seems explained it. But again can you please elaborate why rawdev is used? [DC] There are a number of reasons the rawdev approach was ultimately chosen: 1) As the paragraph above explains, our primary use-case was to chain a crypto operation with error detection algorithms such as CRC or BIP as this could leverage optimized multi-function implementations such as in the IPSec Multi-Buffer library and have a significant impact on performance of network access dataplane processing such as for vCMTS (DOCSIS MAC). However such error detection algorithms are not Crypto functions so some early advice we took was that it would not be suitable to add these to cryptodev. Also, with a view to the future, the multi-function rawdev approach allows crypto operations to be chained with compression operations. Again, neither cryptodev or compressdev allows this type chaining. 2) An earlier version of multi-function suggested adding a new library called rte_accelerator, as described here http://mails.dpdk.org/archives/dev/2020-February/157045.html We received some comments on the dev mailing list that we should not add yet another acceleration library to DPDK. And we also subsequently felt that the rawdev approach is better - that rationale is described below. rte_accelerator was also built on top of crypto and compress devices which already existed e.g. drivers/crypto/aesni_mb, drivers/crypto/qat and drivers/compress/qat . We subsequently realized that this was somewhat confusing when performing multi-function type operations. For example, for combined Crypto-Compression operations in the future, it would use either an existing crypto or compress device, but neither really made sense when the operations are combined. What was needed was a raw device which allowed an application to configure any type of device and it's queue pairs and send any type of operation to that device. For both of these reasons, we decided to go down the rawdev route, with a multi-function interface which can be used by several raw device drivers. 3) rawdev is the ideal place to try out a new approach like this to accessing devices. Adding it here allows potential consumers of this such as VNF solution providers to study and try out this approach, and take advantage of the multi-function operations already supported in the IPSec Multi-Buffer library such as Crypto-CRC and Crypto-CRC-BIP, all without DPDK committing to a new library upfront. We would hope that the multi-function rawdev approach will mature over time (through feedback from customers, new use-cases arising etc.), at which point it could be potentially be moved into the main DPDK library set. > > > > > > > > > <...>
Re: [dpdk-dev] [PATCH v2 0/4] introduce multi-function processing support
Thanks for the detailed review Fiona. Based on your feedback, we will reduce the scope of our plans for multi-function processing support in DPDK. We will focus on implementing a rawdev-based AESNI-MB PMD for Crypto-CRC and Crypto-CRC-BIP processing and we will add QAT Crypto-CRC support in a later release. This functionality is specific to accelerated dataplane processing for DOCSIS and PON MAC workloads. We also note that there hasn't been much community engagement in the broader scope, so these simpler rawdev PMDs should be sufficient. If the DPDK community is interested in expanding this concept later, then this can be explored, but it would not seem necessary for now. We will also remove crypto-perf-tester updates to test rawdev multi-function processing as this would seem like too much code churn on that test tool. > -Original Message- > From: Trahe, Fiona > Sent: Tuesday, April 7, 2020 7:06 PM > > Hi David, Ferruh, > > > -Original Message- > > From: Coyle, David > > Sent: Tuesday, April 7, 2020 12:28 PM > > To: Yigit, Ferruh ; dev@dpdk.org > > > > Hi Ferruh, see below > > > > > > > > > > While DPDK's rte_cryptodev and rte_compressdev allow many > > > > cryptographic and compression algorithms to be chained together in > > > > one operation, there is no way to chain these with any error > > > > detection or checksum algorithms. And there is no way to chain > > > > crypto and compression algorithms together. The multi-function > > > > interface will allow these chains to be created, and also allow > > > > any future type of > > > operation to be easily added. > > > > > > I was thinking if the cryptodev can be used instead but this > > > paragraph already seems explained it. But again can you please elaborate > why rawdev is used? > > > > [DC] There are a number of reasons the rawdev approach was ultimately > chosen: > > > > 1) As the paragraph above explains, our primary use-case was to chain > > a crypto operation with error detection algorithms such as CRC or BIP > > as this could leverage optimized multi-function implementations such > > as in the IPSec Multi-Buffer library and have a significant impact on > performance of network access dataplane processing such as for vCMTS > (DOCSIS MAC). > > However such error detection algorithms are not Crypto functions so > > some early advice we took was that it would not be suitable to add these to > cryptodev. > > Also, with a view to the future, the multi-function rawdev approach > > allows crypto operations to be chained with compression operations. > > Again, neither cryptodev or compressdev allows this type chaining. > > > > 2) An earlier version of multi-function suggested adding a new library > > called rte_accelerator, as described here > > http://mails.dpdk.org/archives/dev/2020-February/157045.html > > We received some comments on the dev mailing list that we should not > > add yet another acceleration library to DPDK. > > And we also subsequently felt that the rawdev approach is better - that > rationale is described below. > > > > rte_accelerator was also built on top of crypto and compress devices which > already existed e.g. > > drivers/crypto/aesni_mb, drivers/crypto/qat and drivers/compress/qat . > > We subsequently realized that this was somewhat confusing when > > performing multi-function type operations. For example, for combined > > Crypto-Compression operations in the future, it would use either an > > existing crypto or compress device, but neither really made sense when > the operations are combined. > > What was needed was a raw device which allowed an application to > > configure any type of device and it's queue pairs and send any type of > operation to that device. > > > > For both of these reasons, we decided to go down the rawdev route, > > with a multi-function interface which can be used by several raw device > drivers. > > > > 3) rawdev is the ideal place to try out a new approach like this to > > accessing > devices. > > Adding it here allows potential consumers of this such as VNF solution > > providers to study and try out this approach, and take advantage of > > the multi-function operations already supported in the IPSec > > Multi-Buffer library such as Crypto-CRC and Crypto-CRC-BIP, all without > DPDK committing to a new library upfront. > > We would hope that the multi-function rawdev approach will mature over > > time (through feedback from customers, new use-cases arising etc.), at > > which point it
Re: [dpdk-dev] [PATCH v2 0/4] introduce multi-function processing support
Hi Fiona, see below > -Original Message- > From: Trahe, Fiona > Sent: Thursday, April 9, 2020 10:37 AM > > Hi David, > > Answer inline below > > > -Original Message- > > From: Coyle, David > > Sent: Thursday, April 9, 2020 10:26 AM > > > > Thanks for the detailed review Fiona. > > > > Based on your feedback, we will reduce the scope of our plans for > > multi-function processing support in DPDK. > > > > We will focus on implementing a rawdev-based AESNI-MB PMD for > > Crypto-CRC and Crypto-CRC-BIP processing and we will add QAT Crypto- > CRC support in a later release. > > This functionality is specific to accelerated dataplane processing for > > DOCSIS > and PON MAC workloads. > > > > We also note that there hasn't been much community engagement in the > > broader scope, so these simpler rawdev PMDs should be sufficient. > > If the DPDK community is interested in expanding this concept later, > > then this can be explored, but it would not seem necessary for now. > > > > We will also remove crypto-perf-tester updates to test rawdev > > multi-function processing as this would seem like too much code churn on > that test tool. > > [Fiona] That sounds like a good idea. In that case my comments B, D and E are > not relevant as assuming a broader scope. > Comments A, C and F can still be considered, but are just suggestions, not > blockers to this being applied in 20.05, they could easily be done in a later > release. [DC] For 20.05, I plan to address A, C and F from below. We will look to address D in a later release when we add QAT multi-function PMD to see if unit test extensibility can be improved. And B and E are now no longer applicable due to reduced scope. > > ///snip/// > > > > I do have some concerns, but these are resolvable in my opinion. > > > (A)as there's no rawdev capability APIs and capabilities are > > > essentially > > > opaque to the rawdev API, the application uses explicit device > > > naming to create or find a device that it knows will fulfil the > > > multifunction APIs. I can see how this works for rawdevs which > > > expect to have only one PMD that will fulfil the service, however > > > I'd expect multi-fn to have at least 2 driver types, probably more > > > eventually. To be extensible I'd suggest a naming convention for a > > > class of devices. E.g. all devices and drivers that implement > > > multi-fn should create a rawdev named mfn_xxx, e.g. mfn_aesni_mb, > > > mfn_qat. The "mfn_" string should be defined in the mfn hdr. This > > > would allow creation of apis like rte_multi_fn_count() which could find > rawdevs which implement mfn_ without hardcoding specific driver names. [DC] The AESNI-MB rawdev will be renamed to rawdev_mfn_aesni_mb. Keeping "rawdev_" as first prefix keeps this consistent with other rawdevs Adding "mfn_" allows rawdevs implementing multi-function be found as you suggested > > > (B)version control of the multi-function APIs. Putting the > > > multifn API > into > > > the drivers/raw/common directory gives a lot of freedom while it's > > > experimental. But can it benefit from API/ABI breakage > > > infrastructure once the experimental tag is removed? Is there any > > > reason not to move the common files to a lib/librte_multi_fn API? [DC] As stated above, this is no longer applicable due to reduced scope > > > (C)xstat name strings should be moved from aesni_mb PMD to > common > > > and maybe use same naming convention, so appl can query same stats > > > from any device, e.g. "mfn_successful_enqueues" could be > implemented > > > by all PMDs. If PMDs want to add driver-specific stats they can add > > > their own without the mfn_, instead create their own unique stat name. [DC] This is a good suggestion as these same stats will also be needed by the QAT PMD. I will make this change. > > > (D)The unit test code is not extensible - again probably as based > > > on > > > previous rawdevs where there's only 1 implementation. For mfn I'd > > > suggest replacing test_rawdev_selftest_aesni_mb() with a > > > test_rawdev_selftest_multi_function(), which finds and/or creates > > > all the raw PMDs implementing the mfn API and runs a test on each. > > > And move the test files from the drivers/raw/aesni_mb dir to > > > app/test and make generic so can run against any device named mfn_xxx [DC] As stated above we will look at making the unit tests
Re: [dpdk-dev] [PATCH v2 1/4] raw/common: add multi-function interface
Hi Pablo
Thank you for reviewing and the comments - see below for resolutions.
The changes will be available in v3 shortly
David
> -Original Message-
> From: De Lara Guarch, Pablo
> Sent: Monday, April 6, 2020 5:09 PM
>
> Hi David,
>
> > -Original Message-
> > From: Coyle, David
> > Sent: Friday, April 3, 2020 5:37 PM
> >
> > The multi-function interface provides a flexible and extensible way of
> > combining one or more packet processing functions into a single
> > operation. The interface can be used by applications to send the
> > combined operations to a optimized software or hardware accelerator via a
> raw device.
> >
> > Signed-off-by: David Coyle
> > Signed-off-by: Mairtin o Loingsigh
> > ---
> >
> > In particular, looking for feedback on the meson script changes that
> > were required to build the drivers/raw/common/multi_fn directory. Thank
> you.
> >
> > config/common_base| 5 +
> > drivers/meson.build | 5 +
> > drivers/raw/Makefile | 1 +
> > drivers/raw/common/Makefile | 8 +
> > drivers/raw/common/meson.build| 7 +
> > drivers/raw/common/multi_fn/Makefile | 27 ++
> > drivers/raw/common/multi_fn/meson.build | 9 +
> > .../multi_fn/rte_common_multi_fn_version.map | 11 +
> > drivers/raw/common/multi_fn/rte_multi_fn.c| 166 +
> > drivers/raw/common/multi_fn/rte_multi_fn.h| 350
> ++
> > .../raw/common/multi_fn/rte_multi_fn_driver.h | 55 +++
> > meson.build | 4 +
> > mk/rte.app.mk | 1 +
> > 13 files changed, 649 insertions(+)
> > create mode 100644 drivers/raw/common/Makefile create mode 100644
> > drivers/raw/common/meson.build create mode 100644
> > drivers/raw/common/multi_fn/Makefile
> > create mode 100644 drivers/raw/common/multi_fn/meson.build
> > create mode 100644
> > drivers/raw/common/multi_fn/rte_common_multi_fn_version.map
> > create mode 100644 drivers/raw/common/multi_fn/rte_multi_fn.c
> > create mode 100644 drivers/raw/common/multi_fn/rte_multi_fn.h
> > create mode 100644 drivers/raw/common/multi_fn/rte_multi_fn_driver.h
> >
> > diff --git a/config/common_base b/config/common_base index
> > c31175f9d..4f004968b 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -818,6 +818,11 @@
> > CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV=y
> > #
> > CONFIG_RTE_LIBRTE_PMD_NTB_RAWDEV=y
> >
> > +#
> > +# Compile multi-fn raw device interface #
> > +CONFIG_RTE_LIBRTE_MULTI_FN_COMMON=n
>
> This can be enabled by default, right? It doesn't have any external
> dependency.
[DC] That is true, so yes this is now enabled by default
>
> ...
>
> > +++
> b/drivers/raw/common/multi_fn/rte_common_multi_fn_version.map
> > @@ -0,0 +1,11 @@
> > +EXPERIMENTAL {
> > + global:
> > +
> > + rte_multi_fn_session_create;
> > + rte_multi_fn_session_destroy;
> > + rte_multi_fn_op_pool_create;
> > + rte_multi_fn_op_bulk_alloc;
> > + rte_multi_fn_op_free;
>
> This list should be sorted alphabetically.
[DC] Fixed
>
> > +
> > + local: *;
> > +};
> > diff --git a/drivers/raw/common/multi_fn/rte_multi_fn.c
> > b/drivers/raw/common/multi_fn/rte_multi_fn.c
> > new file mode 100644
> > index 0..4f8e7fd94
> > --- /dev/null
> > +++ b/drivers/raw/common/multi_fn/rte_multi_fn.c
> > @@ -0,0 +1,166 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2020 Intel Corporation.
> > + */
> > +
> > +#include
> > +#include
>
> ...
>
> > +#include
>
> A bunch of these includes are not needed.
> From what I could see, only , , ,
> and are needed, apart from the two
> below.
[DC] Most of the includes weren't needed... these have been tidied up now
>
>
> > +
> > +#include "rte_multi_fn_driver.h"
> > +#include "rte_multi_fn.h"
>
> ...
>
> > +++ b/drivers/raw/common/multi_fn/rte_multi_fn.h
> > @@ -0,0 +1,350 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2020 Intel Corporation.
> > + */
> > +
> > +#ifndef _RTE_MULTI_FN_H_
> > +#define _RTE_MULTI_FN_H_
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include
> > +#include
> &g
Re: [dpdk-dev] [PATCH v2 2/4] raw/aesni_mb: add aesni_mb raw device
Hi Pablo
Thank you for reviewing and the comments - see below for resolutions.
The changes will be available in v3 shortly
David
> -Original Message-
> From: De Lara Guarch, Pablo
> Sent: Tuesday, April 7, 2020 7:51 PM
>
> Hi David,
>
> > -Original Message-
> > From: Coyle, David
> > Sent: Friday, April 3, 2020 5:37 PM
> >
> > Adding an AESNI-MB raw device, thereby exposing AESNI-MB to the
> rawdev
> > API. The AESNI-MB raw device will use the multi-function interface to
> > allow combined operations be sent to the AESNI-MB software library.
> >
> > Signed-off-by: David Coyle
> > Signed-off-by: Mairtin o Loingsigh
> > ---
> > config/common_base|6 +
> > drivers/raw/Makefile |2 +
> > drivers/raw/aesni_mb/Makefile | 47 +
> > drivers/raw/aesni_mb/aesni_mb_rawdev.c| 1536
> +
> > drivers/raw/aesni_mb/aesni_mb_rawdev.h| 112 ++
> > drivers/raw/aesni_mb/aesni_mb_rawdev_test.c | 1102
> > .../aesni_mb/aesni_mb_rawdev_test_vectors.h | 1183 +
> > drivers/raw/aesni_mb/meson.build | 26 +
> > .../aesni_mb/rte_rawdev_aesni_mb_version.map |3 +
> > drivers/raw/meson.build |3 +-
> > mk/rte.app.mk |2 +
>
> You missed adding the PMD to the MAINTAINERS file.
[DC] Added the new directories to MAINTAINERS file
>
> > 11 files changed, 4021 insertions(+), 1 deletion(-) create mode
> > 100644 drivers/raw/aesni_mb/Makefile create mode 100644
> > drivers/raw/aesni_mb/aesni_mb_rawdev.c
> > create mode 100644 drivers/raw/aesni_mb/aesni_mb_rawdev.h
> > create mode 100644 drivers/raw/aesni_mb/aesni_mb_rawdev_test.c
> > create mode 100644
> > drivers/raw/aesni_mb/aesni_mb_rawdev_test_vectors.h
> > create mode 100644 drivers/raw/aesni_mb/meson.build create mode
> > 100644 drivers/raw/aesni_mb/rte_rawdev_aesni_mb_version.map
> >
...
> > diff --git a/drivers/raw/aesni_mb/aesni_mb_rawdev.c
> > b/drivers/raw/aesni_mb/aesni_mb_rawdev.c
> > new file mode 100644
> > index 0..946bdd871
> > --- /dev/null
> > +++ b/drivers/raw/aesni_mb/aesni_mb_rawdev.c
> > @@ -0,0 +1,1536 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2020 Intel Corporation.
> > + */
> > +
> > +#include
> > +
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> No need for , , ,
> and .
> I think is missing, though, for "rte_crypto_sym_xform".
[DC] Removed only hexdump and dev. All the others would seem to be needed.
rte_cpu_get_flag_enabled() is called from cpuflags
And lots of stuff from multi_fn is used throughout this file
>
> ...
>
> > +static bool
> > +docsis_crc_crypto_encrypt_check(struct rte_multi_fn_xform *xform) {
> > + struct rte_crypto_sym_xform *crypto_sym;
> > + struct rte_multi_fn_err_detect_xform *err_detect;
> > + struct rte_multi_fn_xform *next;
> > +
> > + if (xform->type == RTE_MULTI_FN_XFORM_TYPE_ERR_DETECT) {
> > +
> > + err_detect = &xform->err_detect;
> > + next = xform->next;
> > +
> > + if (err_detect->algo ==
> > + RTE_MULTI_FN_ERR_DETECT_CRC32_ETH
> &&
> > + err_detect->op ==
> > + RTE_MULTI_FN_ERR_DETECT_OP_GENERATE
> > &&
>
> I don't think leading spaces are allowed. Generally, double tab is used in
> multi-line if's. Same applies in other parts of the code.
[DC] Indentation of multi-line if statements have been fixed here and in other
patches
>
> > + next != NULL &&
> > + next->type ==
> RTE_MULTI_FN_XFORM_TYPE_CRYPTO_SYM) {
> > +
>
> ...
>
> > +static bool
> > +docsis_crypto_decrypt_crc_check(struct rte_multi_fn_xform *xform) {
> > + struct rte_crypto_sym_xform *crypto_sym;
> > + struct rte_multi_fn_err_detect_xform *err_detect;
> > + struct rte_multi_fn_xform *next;
> > +
> > + if (xform->type == RTE_MULTI_FN_XFORM_TYPE_CRYPTO_SYM) {
>
> I think in order to reduce this many indentation levels, you can check for the
> opposite here and return false.
[DC] This was a good
Re: [dpdk-dev] [PATCH v2 4/4] app/crypto-perf: add support for multi-function processing
Hi Pablo, Thank you for reviewing and the comments - see below for resolutions. The changes will be available in v3 shortly David > -Original Message- > From: De Lara Guarch, Pablo > Sent: Tuesday, April 7, 2020 7:55 PM > > > -Original Message----- > > From: Coyle, David > > Sent: Friday, April 3, 2020 5:37 PM > > > > Support for multi-function operations, via a raw device, has been > > added to the test-crypto-perf app. > > > > A new optype has been added: multi-fn > > A new parameter has been added for multi-fn mode: > > --multi-fn-params > > > > The field specify what type of multi-function processing is > > required and the options associated with that. Currently the following > > are supported: > > > > docsis-cipher-crc,, > > pon-cipher-crc-bip, > > > > Signed-off-by: David Coyle > > Signed-off-by: Mairtin o Loingsigh > > --- > > Could you update the document to reflect the new changes in the command > line? [DC] We have removed support from the crypto-perf tool in v3 due to the amount of code churn was required to the tool to add multi-function support. I have, however, add documentation for the AESNI-MB Multi-Function Rawdev PMD in v3 > > Thanks, > Pablo
Re: [dpdk-dev] [PATCH v2 1/4] raw/common: add multi-function interface
Hi Pablo Thank you for reviewing and the comments - see below for resolutions. The changes will be available in v3 shortly David > -Original Message- > From: De Lara Guarch, Pablo > Sent: Tuesday, April 7, 2020 7:56 PM > > Hi David, > > > -Original Message- > > From: Coyle, David > > Sent: Friday, April 3, 2020 5:37 PM > > > > The multi-function interface provides a flexible and extensible way of > > combining one or more packet processing functions into a single > > operation. The interface can be used by applications to send the > > combined operations to a optimized software or hardware accelerator via a > raw device. > > > > Signed-off-by: David Coyle > > Signed-off-by: Mairtin o Loingsigh > > --- > > > > Forgot to say to update Release Notes document, which these changes. [DC] Release notes have been updated > > Thanks! > Pablo >
Re: [dpdk-dev] [RFC] Accelerator API to chain packet processing functions
Hi Jerin, Thanks for the comments. Please see replies below. Kind Regards, David > On Tue, Feb 4, 2020 at 8:15 PM David Coyle wrote: > > > > Introduction > > > > > > This RFC introduces a new DPDK library, rte_accelerator. > > > > The main aim of this library is to provide a flexible and extensible way of > combining one or more packet-processing functions into a single operation, > thereby allowing these to be performed in parallel in optimized software > libraries or in a hardware accelerator. These functions can include > cryptography, compression and CRC/checksum calculation, while others can > potentially be added in the future. Performing these functions in parallel as > a > single operation can enable a significant performance improvement. > > > > > > Background > > == > > > > There are a number of byte-wise operations which are present and > common across many access network data-plane pipelines, such as Cipher, > Authentication, CRC, Bit-Interleaved-Parity (BIP), other checksums etc. Some > prototyping has been done at Intel in relation to the 01.org access-network- > dataplanes project to prove that a significant performance improvement is > possible when such byte-wise operations are combined into a single pass of > packet data processing. This performance boost has been prototyped for > both XGS-PON MAC data-plane and DOCSIS MAC data-plane pipelines. > > > Could you share the relative performance numbers to show the gain? [DC] As mentioned above, the main performance gains are when the packet processing operations can be combined into a single pass of the packet. Both Crypto-CRC-BIP (for XGS-PON MAC) and Crypto-CRC (for DOCSIS MAC) have been implemented in the AESNI MB library as single pass operation chains. We have modified the dpdk-crypto-perf-tester as part of our prototyping to test the cases where: 1) each packet processing function is done as an independent stage (e.g. calling rte_net_crc for CRC, AESNI MB through rte_cryptodev for cipher, and a C function to calculate the BIP) 2) all packet processing functions done as a single-pass operation in AESNI MB through rte_cryptodev We see the following results for 1024 byte input frames from dpdk-crypto-perf-tester: - XGS-PON MAC (Crypto-CRC-BIP): - 3 independent stages: 1429 cycles/buf (13.75Gbps) - 1 single-pass stage: 896 cycles/buf (21.9Gbps) 37% cycle reduction - DOCSIS MAC (Crypto-CRC): - 2 independent stages: 1421 cycles/buf (13.84Gbps) - 1 single-pass stage: 1133 cycles/buf (17.34Gbps) 20% cycle reduction Adding the accelerator API will allow vendors gain the benefits of these cycle savings > > > > > The prototypes used some protocol-specific modifications to the DPDK > cryptodev library. In order to make this performance improvement > consumable by network access equipment vendors, a more extensible and > correct solution is required that can be upstreamed into DPDK. > > > > Hence, the introduction of rte_accelerator. > > > > > > Use Cases > > = > > > > The primary use cases for this new library have already been mentioned. > These are: > > > > - DOCSIS MAC: Crypto-CRC > > - Order: > > - Downstream: CRC, Encrypt > > - Upstream: Decrypt, CRC > > - Specifications: > > - Crypto: 128-bit AES-CFB encryption variant for DOCSIS as > described in section 11.1 of DOCSIS 3.1 Security Specification > (https://apps.cablelabs.com/specification/CM-SP-SECv3.1) > > - CRC: Ethernet 32-bit CRC as defined in > > Ethernet/[ISO/IEC 8802-3] > > > > - XGS-PON MAC: Crypto-CRC-BIP > > - Order: > > - Downstream: CRC, Encrypt, BIP > > I understand if the chain has two operations then it may possible to have > handcrafted SW code to do both operations in one pass. > I understand the spec is agnostic on a number of passes it does require to > enable the xfrom but To understand the SW/HW capability, In the above > case, "CRC, Encrypt, BIP", It is done in one pass in SW or three passes in SW > or one pass using HW? [DC] The CRC, Encrypt, BIP is also currently done as 1 pass in AESNI MB library SW. However, this could also be performed as a single pass in a HW accelerator > > > > > - Upstream: BIP, Decrypt, CRC > > - Specifications: > > - Crypto: AES-128 [NIST FIPS-197] cipher, used in counter > > mode > (AES-CTR), as described in [NIST SP800-38A]. > > - CRC: Ethernet 32-bit CRC as defined in Ethernet/[ISO/IEC > > 8802-3] > > - BIP: 4-byte bit-interleaved even parity (BIP) field > > computed over the entire FS frame, refer to ITU-T G.989.3, sections 8.1.1.5 > and 8.1.2.3 (https://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC- > G.989.3-201510-I!!PDF-E) > > -- In
Re: [dpdk-dev] [RFC] Accelerator API to chain packet processing functions
Hi Jerin, see reply below > On Thu, Feb 6, 2020 at 3:35 PM Coyle, David wrote: > > > > Hi Jerin, > > Hi David, > > > Thanks for the comments. Please see replies below. > > > > Kind Regards, > > David > > > > > On Tue, Feb 4, 2020 at 8:15 PM David Coyle > wrote: > > > > > > > > Introduction > > > > > > > > > > > > This RFC introduces a new DPDK library, rte_accelerator. > > > > > > > > The main aim of this library is to provide a flexible and > > > > extensible way of > > > combining one or more packet-processing functions into a single > > > operation, thereby allowing these to be performed in parallel in > > > optimized software libraries or in a hardware accelerator. These > > > functions can include cryptography, compression and CRC/checksum > > > calculation, while others can potentially be added in the future. > > > Performing these functions in parallel as a single operation can enable a > significant performance improvement. > > > > > > > > > > > > Background > > > > == > > > > > > > > There are a number of byte-wise operations which are present and > > > common across many access network data-plane pipelines, such as > > > Cipher, Authentication, CRC, Bit-Interleaved-Parity (BIP), other > > > checksums etc. Some prototyping has been done at Intel in relation > > > to the 01.org access-network- dataplanes project to prove that a > > > significant performance improvement is possible when such byte-wise > > > operations are combined into a single pass of packet data > > > processing. This performance boost has been prototyped for both XGS- > PON MAC data-plane and DOCSIS MAC data-plane pipelines. > > > > > > > > > Could you share the relative performance numbers to show the gain? > > > > [DC] As mentioned above, the main performance gains are when the > packet processing operations can be combined into a single pass of the > packet. > > Both Crypto-CRC-BIP (for XGS-PON MAC) and Crypto-CRC (for DOCSIS > MAC) have been implemented in the AESNI MB library as single pass > operation chains. > > > > We have modified the dpdk-crypto-perf-tester as part of our prototyping > to test the cases where: > > 1) each packet processing function is done as an independent stage > > (e.g. calling rte_net_crc for CRC, AESNI MB through rte_cryptodev for > > cipher, and a C function to calculate the BIP) > > 2) all packet processing functions done as a single-pass operation in > > AESNI MB through rte_cryptodev > > > > We see the following results for 1024 byte input frames from dpdk-crypto- > perf-tester: > > - XGS-PON MAC (Crypto-CRC-BIP): > > - 3 independent stages: 1429 cycles/buf (13.75Gbps) > > - 1 single-pass stage: 896 cycles/buf (21.9Gbps) > > 37% cycle reduction > > > > - DOCSIS MAC (Crypto-CRC): > > - 2 independent stages: 1421 cycles/buf (13.84Gbps) > > - 1 single-pass stage: 1133 cycles/buf (17.34Gbps) > > 20% cycle reduction > > > > Adding the accelerator API will allow vendors gain the benefits of > > these cycle savings > > Numbers make sense. I have seen a similar performance improvement doing > in one pass with CPU instructions. > > > > > > - XGS-PON MAC: Crypto-CRC-BIP > > > > - Order: > > > > - Downstream: CRC, Encrypt, BIP > > > > > > I understand if the chain has two operations then it may possible to > > > have handcrafted SW code to do both operations in one pass. > > > I understand the spec is agnostic on a number of passes it does > > > require to enable the xfrom but To understand the SW/HW capability, > > > In the above case, "CRC, Encrypt, BIP", It is done in one pass in SW > > > or three passes in SW or one pass using HW? > > > > [DC] The CRC, Encrypt, BIP is also currently done as 1 pass in AESNI MB > library SW. > > However, this could also be performed as a single pass in a HW > > accelerator > > As a specification, cascading the xform chains make sense. > Do we have any HW that does support chaining the xforms more than "two" > in one pass? > i.e real chaining function where two blocks of HWs work hand in hand for > chaining. > If none, it may be better to abstract as synonymous API(No dequeue, no > enqueue) for the CPU use case. [DC] I'm not aware of any HW that supports this at the moment, but that's not to say it couldn't in the future - if anyone else has any examples though, please feel free to share. Regardless, I don't see why we would introduce a different API for SW devices and HW devices. It would be up to each underlying PMD to decide if/how it supports a particular accelerator xform chain, but from an application's point of view, the accelerator API is always the same
Re: [dpdk-dev] [RFC] Accelerator API to chain packet processing functions
Hi Jerin, see below > > On Thu, Feb 6, 2020 at 10:01 PM Coyle, David > wrote: > > Hi David, > > > > > > > > > > > > > - XGS-PON MAC: Crypto-CRC-BIP > > > > > > - Order: > > > > > > - Downstream: CRC, Encrypt, BIP > > > > > > > > > > I understand if the chain has two operations then it may > > > > > possible to have handcrafted SW code to do both operations in one > pass. > > > > > I understand the spec is agnostic on a number of passes it does > > > > > require to enable the xfrom but To understand the SW/HW > > > > > capability, In the above case, "CRC, Encrypt, BIP", It is done > > > > > in one pass in SW or three passes in SW or one pass using HW? > > > > > > > > [DC] The CRC, Encrypt, BIP is also currently done as 1 pass in > > > > AESNI MB > > > library SW. > > > > However, this could also be performed as a single pass in a HW > > > > accelerator > > > > > > As a specification, cascading the xform chains make sense. > > > Do we have any HW that does support chaining the xforms more than > "two" > > > in one pass? > > > i.e real chaining function where two blocks of HWs work hand in hand > > > for chaining. > > > If none, it may be better to abstract as synonymous API(No dequeue, > > > no > > > enqueue) for the CPU use case. > > > > [DC] I'm not aware of any HW that supports this at the moment, but that's > not to say it couldn't in the future - if anyone else has any examples though, > please feel free to share. > > Regardless, I don't see why we would introduce a different API for SW > devices and HW devices. > > There is a risk in drafting API that meant for HW without any HW exists. > Because there could be inefficiency on the metadata and fast path API for > both models. > For example, In the case of CPU based scheme, it will be pure overhead > emulate the "queue"(the enqueue and dequeue) for the sake of abstraction > where CPU works better in the synchronous model and I have doubt that the > session-based scheme will work for HW or not as both difference HW needs > to work hand in hand(IOMMU aspects for two PCI device) [DC] I understand what you are saying about the overhead of emulating the "sw queue" but this same model is already used in many of the existing device PMDs. In the case of SW devices, such as AESNI-MB or NULL for crypto or zlib for compression, the enqueue/dequeue in the PMD is emulated through an rte_ring which is very efficient. The accelerator API will use the existing device PMDs so keeping the same model seems like a sensible approach. From an application's point of view, this abstraction of the underlying device type is important for usability and maintainability - the application doesn't need to know the device type as such and therefore doesn't need to make different API calls. The enqueue/dequeue type API was also used with QAT in mind. While QAT HW doesn't support these xform chains at the moment, it could potentially do so in the future. As a side note, as part of the work of adding the accelerator API, the QAT PMD will be updated to support the DOCSIS Crypto-CRC accelerator xform chain, where the Crypto is done on QAT HW and the CRC will be done in SW, most likely through a call to the optimized rte_net_crc library. This will give a consistent API for the DOCSIS-MAC data-plane pipeline prototype we have developed, which uses both AESNI-MB and QAT for benchmarks. We will take your feedback on the enqueue/dequeue approach for SW devices into consideration though during development. Finally, I'm unsure what you mean by this line: "I have doubt that the session-based scheme will work for HW or not as both difference HW needs to work hand in hand(IOMMU aspects for two PCI device)" What do mean by different HW working "hand in hand" and "two PCI device"? The intention is that 1 HW device (or it's PMD) would have to support the accel xform chain > > Having said that, I agree with the need for use case and API for CPU case. > Till > we find a HW spec, we need to make the solution as CPU specific and latter > extend based on HW metadata required. > Accelerator API sounds like HW accelerator and there is no HW support then > it may not good. We can change the API that works for the use cases that we > know how it works efficiently. > > > > > > > > > It would be up to each underlying PMD to decide if/how it supports a > > particular accelerator xform chain, but from an application's point of > > view, the accelerator API is always the same > > > >
Re: [dpdk-dev] [PATCH 2/3] cryptodev: add security operation to crypto operation
Hi Konstantin, see below
> -Original Message-
> From: Ananyev, Konstantin
> Sent: Tuesday, June 9, 2020 2:23 PM
>
>
> >
> > /** Status of crypto operation */
> > @@ -121,6 +123,13 @@ struct rte_crypto_op {
> > struct rte_crypto_asym_op asym[0];
> > /**< Asymmetric operation parameters */
> >
> > +#ifdef RTE_LIBRTE_SECURITY
> > + uint8_t security[0];
> > + /**< Security operation parameters
> > +* - Must be accessed through a rte_security_op pointer
> > +*/
> > +#endif
> > +
> > }; /**< operation specific parameters */ };
>
> Is there any point to have this extra level of indirection?
> Might be simply:
>
> enum rte_crypto_op_type {
>
> + RTE_CRYPTO_OP_TYPE_SEC_DOCSIS,
> };
> ...
> struct rte_crypto_op {
>
> __extension__
> union {
> struct rte_crypto_sym_op sym[0];
> /**< Symmetric operation parameters */
>
> struct rte_crypto_asym_op asym[0];
> /**< Asymmetric operation parameters */
>
> + struct rte_security_docsis_op docsis[0];
>
> }; /**< operation specific parameters */
>
> ?
[DC] This was to allow some form of extensibility and not to limit this to just
DOCSIS.
If it's felt that having the extra level of indirection is overkill, it can be
easily changed.
However, we cannot include a struct of type 'struct rte_security_docsis_op' (or
'struct rte_security_op')
directly here, without creating nasty circular dependency of includes between
rte_cryptodev and
rte_security.
I had tried defining an opaque version 'struct rte_security_op' (i.e. no fields
within the struct) here
in rte_crypto.h, but the compiler complained that it couldn't determine the
size of the struct,
even though it's a zero length array.
That is why I had to use the uint8_t in 'uint8_t security[0];' - I don't like
this, but I couldn't find
another way that kept the compiler happy and didn't create a circular
dependency.
Re: [dpdk-dev] [PATCH 2/3] cryptodev: add security operation to crypto operation
Hi Konstantin,
> > >
> > > >
> > > > /** Status of crypto operation */ @@ -121,6 +123,13 @@ struct
> > > > rte_crypto_op {
> > > > struct rte_crypto_asym_op asym[0];
> > > > /**< Asymmetric operation parameters */
> > > >
> > > > +#ifdef RTE_LIBRTE_SECURITY
> > > > + uint8_t security[0];
> > > > + /**< Security operation parameters
> > > > +* - Must be accessed through a rte_security_op pointer
> > > > +*/
> > > > +#endif
> > > > +
> > > > }; /**< operation specific parameters */ };
> > >
> > > Is there any point to have this extra level of indirection?
> > > Might be simply:
> > >
> > > enum rte_crypto_op_type {
> > >
> > > + RTE_CRYPTO_OP_TYPE_SEC_DOCSIS,
> > > };
> > > ...
> > > struct rte_crypto_op {
> > >
> > > __extension__
> > > union {
> > > struct rte_crypto_sym_op sym[0];
> > > /**< Symmetric operation parameters */
> > >
> > > struct rte_crypto_asym_op asym[0];
> > > /**< Asymmetric operation parameters */
> > >
> > > + struct rte_security_docsis_op docsis[0];
> > >
> > > }; /**< operation specific parameters */
> > >
> > > ?
> > [DC] This was to allow some form of extensibility and not to limit this to
> > just
> DOCSIS.
> > If it's felt that having the extra level of indirection is overkill, it can
> > be easily
> changed.
> >
> > However, we cannot include a struct of type 'struct
> > rte_security_docsis_op' (or 'struct rte_security_op') directly here,
> > without creating nasty circular dependency of includes between
> rte_cryptodev and rte_security.
> >
> > I had tried defining an opaque version 'struct rte_security_op' (i.e.
> > no fields within the struct) here in rte_crypto.h, but the compiler
> > complained that it couldn't determine the size of the struct, even though
> it's a zero length array.
> >
> > That is why I had to use the uint8_t in 'uint8_t security[0];' - I
> > don't like this, but I couldn't find another way that kept the compiler
> > happy
> and didn't create a circular dependency.
>
> I see... would it be an option to name this struct 'struct rte_sym_docsis_op
> and and move actual definition inside
> lib/librte_cryptodev/rte_crypto_sym.h?
>
[DC] It's certainly an option and would work but I don't think it's a good idea
to be putting
protocol specific structs like this in rte_cryptodev - that's what rte_security
is for.
Do you think it would be ok to do this?
I'd be interested to hear what cryptodev/security maintainers and others think
too.
Akhil/Declan - any thoughts on best approach here?
>
Re: [dpdk-dev] [PATCH 2/3] cryptodev: add security operation to crypto operation
Hi Konstantin,
> > > > >
> > > > > >
> > > > > > /** Status of crypto operation */ @@ -121,6 +123,13 @@ struct
> > > > > > rte_crypto_op {
> > > > > > struct rte_crypto_asym_op asym[0];
> > > > > > /**< Asymmetric operation parameters */
> > > > > >
> > > > > > +#ifdef RTE_LIBRTE_SECURITY
> > > > > > + uint8_t security[0];
> > > > > > + /**< Security operation parameters
> > > > > > +* - Must be accessed through a rte_security_op
> pointer
> > > > > > +*/
> > > > > > +#endif
> > > > > > +
> > > > > > }; /**< operation specific parameters */ };
> > > > >
> > > > > Is there any point to have this extra level of indirection?
> > > > > Might be simply:
> > > > >
> > > > > enum rte_crypto_op_type {
> > > > >
> > > > > + RTE_CRYPTO_OP_TYPE_SEC_DOCSIS,
> > > > > };
> > > > > ...
> > > > > struct rte_crypto_op {
> > > > >
> > > > > __extension__
> > > > > union {
> > > > > struct rte_crypto_sym_op sym[0];
> > > > > /**< Symmetric operation parameters */
> > > > >
> > > > > struct rte_crypto_asym_op asym[0];
> > > > > /**< Asymmetric operation parameters */
> > > > >
> > > > > + struct rte_security_docsis_op docsis[0];
> > > > >
> > > > > }; /**< operation specific parameters */
> > > > >
> > > > > ?
> > > > [DC] This was to allow some form of extensibility and not to limit
> > > > this to just
> > > DOCSIS.
> > > > If it's felt that having the extra level of indirection is
> > > > overkill, it can be easily
> > > changed.
> > > >
> > > > However, we cannot include a struct of type 'struct
> > > > rte_security_docsis_op' (or 'struct rte_security_op') directly
> > > > here, without creating nasty circular dependency of includes
> > > > between
> > > rte_cryptodev and rte_security.
> > > >
> > > > I had tried defining an opaque version 'struct rte_security_op' (i.e.
> > > > no fields within the struct) here in rte_crypto.h, but the
> > > > compiler complained that it couldn't determine the size of the
> > > > struct, even though
> > > it's a zero length array.
> > > >
> > > > That is why I had to use the uint8_t in 'uint8_t security[0];' - I
> > > > don't like this, but I couldn't find another way that kept the
> > > > compiler happy
> > > and didn't create a circular dependency.
> > >
> > > I see... would it be an option to name this struct 'struct
> > > rte_sym_docsis_op and and move actual definition inside
> > > lib/librte_cryptodev/rte_crypto_sym.h?
> > >
> > [DC] It's certainly an option and would work but I don't think it's a
> > good idea to be putting protocol specific structs like this in
> > rte_cryptodev -
> that's what rte_security is for.
> > Do you think it would be ok to do this?
>
> I personally don't see a problem with this.
> In fact, as an extra thought - why we can't have docsis xform defined in
> lib/librte_cryptodev/rte_crypto_sym.h too, and then just have it as a
> member inside struct rte_crypto_sym_xform union?
> Then we can have rte_cryptodev_sym_session that supports docsis stuff.
>
[DC] Because DOCSIS protocol and CRC are not specifically crypto related is why
we initially went down the rawdev/multi-fn route and now the rte_security route.
I think adding docsis xforms/ops and CRC related data to cryptodev would be
adding
too much non-crypto algorithm related stuff to this library. There would then
be some
protocols like IPSec and PDCP with their definitions in rte_security and others
like
DOCSIS in rte_cryptodev - that doesn't seem good to me.
Yes, from a DOCSIS equipment vendors point-of-view, who already use cryptodev
for just encryption/decryption, adding DOCSIS to cryptodev would be best
for them in order to get better DOCSIS support in DPDK as it would mean less
churn for their applications. However, from a DPDK point-of-view, I don't think
it
would be correct to do this.
That's just my opinion, and again I'd be interested to hear other people's
thoughts.
> >
> > I'd be interested to hear what cryptodev/security maintainers and others
> think too.
> > Akhil/Declan - any thoughts on best approach here?
Re: [dpdk-dev] [PATCH v2 0/6] add support for DOCSIS protocol
> -Original Message- > From: David Marchand > Sent: Tuesday, June 23, 2020 3:52 PM > > > A number of approaches to combine DOCSIS Crypto and CRC functions > have > > been discussed in the DPDK community to date, namely: > > 1) adding a new rte_accelerator API, to provide a generic interface for > >combining operations of different types > > 2) using rawdev through a multi-function interface, again to provide a > >generic interface for combining operations of different types > > 3) adding support for DOCSIS Crypto-CRC to rte_security > > > > The third option above is the preferred approach for the following > > reasons: > > - it addresses the immediate use case to add DOCSIS Crypto-CRC support to > > DPDK so that it can be consumed easily by cable equipment vendors > > - it uses an already existing framework in DPDK > > - it will mean much less code churn in DOCSIS applications, which already > > use rte_cryptodev for encryption/decryption > > I guess https://patchwork.dpdk.org/project/dpdk/list/?series=9304 can be > marked Superseded then. > Thanks. [DC] Yes it can - I have tried to set it to Superseded but don't have permissions to do that - guess one of the Maintainers needs to do that. > > -- > David Marchand
Re: [dpdk-dev] [PATCH v2 0/6] add support for DOCSIS protocol
> -Original Message- > From: David Marchand > Sent: Tuesday, June 23, 2020 4:39 PM > > > I guess https://patchwork.dpdk.org/project/dpdk/list/?series=9304 > > > can be marked Superseded then. > > > Thanks. > > > > [DC] Yes it can - I have tried to set it to Superseded but don't have > > permissions to do that - guess one of the Maintainers needs to do that. > > You need to be logged in patchwork and have the same mail address that > sent the series attached to your account. > [DC] I have tried attach my email address to my account but I think Intel has some blocker on emails coming from patchwork, so I'm not receiving the confirmation email. This same problem with emails getting blocked also hit me before when I originally signed up to patchwork. Thanks for the info on this btw.
Re: [dpdk-dev] [PATCH v2 0/6] add support for DOCSIS protocol
> -Original Message- > From: David Marchand > Sent: Tuesday, June 23, 2020 5:22 PM > To: Coyle, David > > > > > I guess > > > > > https://patchwork.dpdk.org/project/dpdk/list/?series=9304 > > > > > can be marked Superseded then. > > > > > Thanks. > > > > > > > > [DC] Yes it can - I have tried to set it to Superseded but don't > > > > have permissions to do that - guess one of the Maintainers needs to do > that. > > > > > > You need to be logged in patchwork and have the same mail address > > > that sent the series attached to your account. > > > > > [DC] I have tried attach my email address to my account but I think > > Intel has some blocker on emails coming from patchwork, so I'm not > receiving the confirmation email. > > This same problem with emails getting blocked also hit me before when > > I originally signed up to patchwork. > > I saw some offlist exchanges about this. > It might be the right time to push your IT about this issue :-). [DC] I think you are right - I will try get this resolved. > > I marked the series as superseded. [DC] Thank you for doing that and apologies for the inconvenience.
Re: [dpdk-dev] [PATCH 2/3] cryptodev: add security operation to crypto operation
Hi Akhil > -Original Message- > From: Akhil Goyal > Sent: Tuesday, June 23, 2020 7:38 PM > > > > > > > [DC] It's certainly an option and would work but I don't think it's > > > a good idea to > > be putting > > > protocol specific structs like this in rte_cryptodev - that's what > > > rte_security is > > for. > > > Do you think it would be ok to do this? > > > > I personally don't see a problem with this. > > In fact, as an extra thought - why we can't have docsis xform defined > > in lib/librte_cryptodev/rte_crypto_sym.h too, and then just have it > > as a member inside struct rte_crypto_sym_xform union? > > Then we can have rte_cryptodev_sym_session that supports docsis stuff. > > Adding DOCSIS alone is not an issue in the cryptodev. The intent of this > patchset and Previous RFCs was chaining of two - DOCSIS and CRC which are > supposed to be separate Blocks and we need a way to combine the two and > use it in the application. > rte_security provides a way to handle such protocols for algo combinations. > However, IMO we do not really need a separate rte_security_docsis_op > structure, As it has parameters which are already there in the > rte_crypto_sym_op. This new op Struct is just adding extra bytes which can > be avoided if we use sym_op->auth.data.offset And sym_op- > >auth.data.length in place of crc offset and crc length. > We may just need to add comment in the struct definition about its usage for > CRC cases. > [DC] I take your point that introducing the rte_security_docsis_op (and the outer rte_security_op) structure is just adding extra bytes and as Konstantin mentioned, unnecessary levels of indirection. I am happy to go with the approach of using the auth offset and length from the sym_op for the CRC values, if there are no major objections from others on this. This simplifies things and also means we can now remove the 'uint8_t security[0]' field from rte_crypto_op which was never a nice thing. Konstantin also suggested moving the docsis xform to cryptodev. However, I feel this would be a step too far for cryptodev and propose we keep the docsis xform in rte_security. It is then consistent with the other protocols like IPSec.
Re: [dpdk-dev] [PATCH v2 2/6] security: add support for DOCSIS protocol
Hi Akhil,
> -Original Message-
> From: Akhil Goyal
> Sent: Tuesday, June 23, 2020 7:07 PM
> >
> > +/**
> > + * DOCSIS operation parameters
> > + */
> > +struct rte_security_docsis_op {
> > + struct rte_crypto_sym_op crypto_sym;
> > + /**< Symmetric crypto operation parameters */
> > +
> > + struct {
> > + uint16_t offset;
> > + /**<
> > +* Starting point for CRC processing, specified
> > +* as the number of bytes from start of the packet in
> > +* the source mbuf in crypto_sym
> > +*/
> > + uint16_t length;
> > + /**<
> > +* The length, in bytes, of the source mbuf on which the
> > +* CRC will be computed
> > +*/
> > + } crc;
> > + /**< CRC operation parameters */
>
> As per my understanding, CRC is a kind of authentication. Can we reuse the
> fields of rte_crypto_sym_op Auth.data.offset and auth.data.length. This way
> you can save the unnecessary 4 bytes here. Probably add Comment in the
> structure definition that it can be used as offset and length for CRC.
>
> And if you feel that reserved field is needed in near future, then you can add
> a proper name to it or else You can do away with the rte_security_docsis_op
> itself as there will be no other fields in it.
[DC] As per my reply on the v1 patchset, I am happy to use the auth offset and
length fields for CRC if there are no objections from others on this approach.
Strictly speaking, a CRC is not an authentication algorithm like the other auth
algos in cryptodev - if it were we would have just added CRC as a new auth algo.
However, using the auth offset and length fields of the crypto op does simplify
things, removes unnecessary bytes and levels of indirection. It also means the
'uint8_t security[0]' field can be removed from rte_crypto_op.
The 'reserved' field was to accommodate other DOCSIS protocol features which
could be offloaded in the future - such as the DOCSIS header checksum. For this
feature, we would need to know the DOCSIS header length. The header length
though is equal to the CRC offset, so we can get the header length that way.
The reserved field and the entire rte_security_docsis_op can therefore be
removed
Re: [dpdk-dev] [PATCH v2 3/6] crypto/aesni_mb: add support for DOCSIS protocol
Hi Pablo, thank you for the comments
> -Original Message-
> From: De Lara Guarch, Pablo
> Sent: Tuesday, June 23, 2020 6:57 PM
>
> > +static inline void
> > +verify_docsis_sec_crc(JOB_AES_HMAC *job, uint16_t crc_len, uint8_t
> > +*status) {
> > + uint16_t crc_offset;
> > + uint8_t *crc;
> > +
> > + if (!job->msg_len_to_hash_in_bytes)
> > + return;
> > +
> > + crc_offset = job->hash_start_src_offset_in_bytes +
> > + job->msg_len_to_hash_in_bytes -
> > + job->cipher_start_src_offset_in_bytes;
> > + crc = job->dst + crc_offset;
> > +
> > + /* Verify CRC (at the end of the message) */
> > + if (memcmp(job->auth_tag_output, crc, crc_len) != 0)
>
> I'd say we can use direct RTE_ETHER_CRC_LEN here, as there is no other
> possible case, right?
> It should perform better.
[DC] You are correct - I have changed this to use RTE_ETHER_CRC_LEN.
I had been thinking about removing the crc_size from the
rte_security_docsis_xform
and the docsis capabilities completely and your comment here has made me realize
I should do this, as there is only 1 CRC length that can be used for DOCSIS.
So these have been removed.
These changes will be in v3 early next week
>
> > + *status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED; } #endif
> > +
> > static inline void
> > verify_digest(JOB_AES_HMAC *job, void *digest, uint16_t len, uint8_t
> > *status) { @@ -1196,9 +1452,27 @@ static inline struct rte_crypto_op
> > * post_process_mb_job(struct aesni_mb_qp *qp, JOB_AES_HMAC *job) {
> > struct rte_crypto_op *op = (struct rte_crypto_op *)job->user_data;
> > - struct aesni_mb_session *sess = get_sym_session_private_data(
> > - op->sym->session,
> > - cryptodev_driver_id);
> > + struct aesni_mb_session *sess = NULL;
> > +
> > +#ifdef AESNI_MB_DOCSIS_SEC_ENABLED
> > + struct rte_security_op *sec_op = NULL;
> > +
> > + if (unlikely(op->type == RTE_CRYPTO_OP_TYPE_SECURITY)) {
>
> Not sure if this unlikely is actually needed. I don't expect to have multiple
> types enqueued in the same queue, so this or the other branch will always
> be taken.
[DC] That's a fair point - I have removed the unlikely
Again, the change will be available in v3
>
> > + /*
> > +* Assuming at this point that if it's a security type op, that
> > +* this is for DOCSIS
> > +*/
> > + sec_op = (struct rte_security_op *)&op->security;
> > + struct rte_crypto_sym_op *crypto_sym =
> > + &sec_op-
> >docsis.crypto_sym;
> > + sess = get_sec_session_private_data(crypto_sym-
> > >sec_session);
>
> ...
>
> > - retval = set_mb_job_params(job, qp, op, &digest_idx);
> > +#ifdef AESNI_MB_DOCSIS_SEC_ENABLED
> > + if (unlikely(op->type == RTE_CRYPTO_OP_TYPE_SECURITY))
>
> Same comment as above.
[DC] Same reply as above. :)
>
> > + retval = set_sec_mb_job_params(job, qp, op,
> > + &digest_idx);
> > + else
> > +#endif
Re: [dpdk-dev] [PATCH v2 5/6] test/crypto: add DOCSIS security test cases
Hi Pablo
> -Original Message-
> From: De Lara Guarch, Pablo
> Sent: Tuesday, June 23, 2020 7:04 PM
>
> > +static int
> > +test_docsis_proto_uplink(int i, struct docsis_test_data *d_td) {
> > + struct rte_security_op *sec_op;
> > + struct rte_security_docsis_op *doc_op;
> > + struct crypto_testsuite_params *ts_params = &testsuite_params;
> > + struct crypto_unittest_params *ut_params = &unittest_params;
> > + uint8_t *plaintext, *ciphertext;
> > + uint8_t *iv_ptr;
> > + int cipher_len = 0;
> > + int crc_len = 0, crc_data_len;
>
> Minor comment. These "int" should be "unsigned int", as they are not going
> to hold a negative value.
[DC] Kind of correct, but not fully.
There are some calculations further down which subtract cipher_offset,
crc_offset
and RTE_ETHER_CRC_LEN from the overall buffer size, which could (if test cases
aren't
setup correctly) make the results negative. These results get put into the
cipher_len and
crc_len variables. If the result is negative, it just gets reset to 0. This was
just handier than
having if/else checks below. I don't see any major issue with this.
The crc_data_len variable can only have positive values though, so I have
changed that
to a uint32_t
Updates will be in v3 early next week
>
> > + int ret = TEST_SUCCESS;
Re: [dpdk-dev] [PATCH v2 2/6] security: add support for DOCSIS protocol
Hi Pablo
> -Original Message-
> From: De Lara Guarch, Pablo
> > +/**
> > + * DOCSIS security session configuration.
> > + *
> > + * This structure contains data required to create a DOCSIS security
> session.
> > + */
> > +struct rte_security_docsis_xform {
> > + enum rte_security_docsis_direction direction;
> > + /** DOCSIS direction */
>
> Missing "<" here.
[DC] Very good spot... will be fixed in v3
>
Re: [dpdk-dev] [RFC] Accelerator API to chain packet processing functions
Having taken feedback from the community into account, we would like to propose some changes to our approach for combining multiple packet-processing functions into a single operation on a single device, be that an optimized software library or a hardware accelerator. The main feedback on the rte_accelerator API can be summarized as follows: 1) Why are we creating another new library that performs tasks similar to other existing APIs... why not try and converge on one? 2) The term "accelerator" is too broad a term, if the API is primarily focused on Crypto + CRC We also felt that using the rte_cryptodev and rte_compressdev APIs to initialize, configure and reset the devices and then using rte_accelerator for session creation and operation enqueue/dequeue was confusing matters. We believe the new approach addresses the above concerns and also greatly simplifies the solution. Our new approach proposes to use the already existing rte_rawdev API with some added functionality for creating "multi-function" sessions. At the high level, the main changes are: - The rte_accelerator library will no longer be added - The rte_rawdev API will be used to initialize, configure, reset a device - A new rawdev interface for "multi-function" sessions/operations will be added under the new directory 'drivers/raw/common' - this interface's header file will be called 'rte_rawdev_multi_fn.h' (with an accompanying C file for function implementations) - this header file will contain much of what was previously included in rte_accelerator.h and rte_err_detect.h, such as: - enums and structs for defining a multi-function chain of xforms, using xform definitions from rte_cryptodev and rte_compressdev as necessary - enums and structs for defining a multi-function chain of ops, again using op definitions from rte_cryptodev and rte_compressdev as necessary - enums and structs for defining error-detection xforms and ops - two API function definitions to create and destroy a session based on a xform chain - rte_rawdev_multi_fn_session_create() - rte_rawdev_multi_fn_session_destroy() - application code will include rte_rawdev_multi_fn.h to access the structs, enums and functions for creating xform chains, sessions and op chains - keeping the multi-function interface under the 'drivers' directory means that rte_rawdev itself remains completely "raw", with no knowledge of xforms, sessions or ops - a proposal for this header file is included at the end - The rte_rawdev API will be used to enqueue/dequeue the operations using the existing rte_rawdev_enqueue_buffers() and rte_rawdev_dequeue_buffers() - a synchronous API function could potentially be added to rte_rawdev in the future if required, to avoid the overhead of enqueue/dequeue for the optimized software library use-case (e.g. rte_rawdev_process_buffers()) - Two new rawdev PMDs for will be added under 'drivers/raw' for QAT and AESNI-MB - these two rawdev PMDs will use and implement the multi-function interface defined in 'drivers/raw/common/rte_rawdev_multi_fn.h' - as with all other rawdev PMDs, the interface is known only to the application and the PMD itself, and is opaque to rte_rawdev itself - the PMDs will be added under 'drivers/raw/aesni_mb' and 'drivers/raw/qat' - other PMDs (existing or new) could use this multi-function interface in the future if use-cases arise - The rte_rawdev library will be used as is, with no changes required The initial use cases for the multi-function rawdev interface remain the same as for the previously proposed rte_accelerator: - DOCSIS MAC: Crypto + CRC - XGS-PON MAC: Crypto + CRC + BIP However, the API can still also accommodate other chained functions such as Compression + Crypto and UDP Checksum + Crypto. The following diagram shows the new architecture: +---+ | | | Application | |(e.g. vCMTS (DOCSIS), vOLT (XGS-PON), etc.)| | | +---+ | +---|---+ | | DPDK| | | | | +-+ | | | | | |
Re: [dpdk-dev] [RFC] Accelerator API to chain packet processing functions
> > > > Having an API that could be used by parallel hardware does make sense, > > but the DPDK already has multiple packet processing infrastructure pieces. > > > > I would rather the DPDK converge on one widely used, robust and tested > > packet method. Rather than the current "choose your poison or roll > > your own" which is what we have now. The proposed graph seems to be > the best so far. > > I agree. Even I thought of saying graph can do this, as, it has higher > abstraction and runtime chaining support, but then I thought it will be self > markering. > David could you check https://www.mail- > archive.com/dev@dpdk.org/msg156318.html > If this one only focusing crypto dev + compressdev, What if we have ethdev > + compressdev + security device in the future. > graph has higher abstraction so it can accommodate ANY chaining > requirements. i.e AESNI-MB + QAT will go as a separate node [DC] We have looked at the graph node library and we don't feel that using graph is the correct solution for what we are trying to solve here. We want to combine 2 or more packet processing functions on a packet into a single operation on a single device, be that an optimized software library such as AESNI MB or a hardware accelerator such as QAT So yes, these 2 packet processing functions could be a node (or nodes) within a graph. However they would still need to be combined together at some point to be processed on the device as a single operation. Our new proposal is to use rte_rawdev to access the devices and we propose to add a "multi-function" interface which the application and rawdev PMDs will use to create the xform chains, sessions and op chains. The full details on this new proposal have been sent to you in a separate post and we feel it addresses the concerns of the original rte_accelerator API In the future, rawdev enqueue/dequeue calls using this multi-function interface could potentially be configured as a node within a packet processing graph
Re: [dpdk-dev] [RFC] Accelerator API to chain packet processing functions
> >
> > /** Error Detection Algorithms */
> > enum rte_rawdev_multi_fn_err_detect_algorithm {
> > RTE_RAWDEV_MULTI_FN_ERR_DETECT_CRC32_ETH,
>
> IMO, It does not make sense to add protocol specific stuff in rawdev
> symbols.
>
> IMO, It is better to have a separate library for CRC and BIP32 acceleration
> like
> the rte_security library and underneath still it can use rawdev or anydev if
> required.
[DC] This protocol stuff is only in the rawdev interface definition, which is
known only to the application and the rawdev PMDs which will use this interface.
So these defines/enums/structs etc for CRC and BIP are completely opaque to
rte_rawdev itself.
This is how all existing rawdev PMDs interfaces are defined, where the
interface is very specific to the job(s) the PMD is implementing.
Also, these particular defines/enums/structs for CRC and BIP are only for
defining xform and op chains containing these particular operations.
The actual code to do the CRC and BIP is already in the AESNI-MB library or
DPDK rte_net_crc library, which our aesni_mb and qat rawdev PMDs will call/use
>
> IMO, Exposing the public API in
> drivers/raw/common/rte_rawdev_multi_fn.h is a shortcut.
> IMO, public API should be in lib/..
[DC] To be honest, I tend to agree. I don't like that public APIs are exposed
from the drivers directory.
But as I mentioned above, this is how all rawdev PMD interfaces are defined,
where the interface definition is within the PMD directory (e.g.
drivers/raw/dpaa2_cmdif/rte_pmd_dpaa2_cmdif.h)
Our's is slightly different in that we have 2 PMDs which will use the same
interface, which is why we have added it in drivers/raw/common
So by keeping our interface under drivers, we are trying to be consistent with
all existing rawdev PMDs
As I mentioned in my previous post though, this could potentially be moved
under lib in the future if other PMDs would find it useful
We could possibly rename our interface file to rte_pmd_multi_fn.h to be a bit
more consistent with the majority of the existing PMDs and take away the idea
for now that this is some kind of extension to the main rte_rawdev API.
But unfortunately there is no full consistency in the rawdev PMD interface
filenames (e.g. dpaa2_cmdif uses the "rte_pmd_" prefix - rte_pmd_dpaa2_cmdif.h,
octeontx2_dma uses the "_rawdev" suffix - otx2_dpi_rawdev.h)
>
> Just my 2c.
Re: [dpdk-dev] [RFC] Accelerator API to chain packet processing functions
>
> On Fri, Mar 6, 2020 at 8:25 PM Coyle, David wrote:
> >
> > > >
> > > > /** Error Detection Algorithms */
> > > > enum rte_rawdev_multi_fn_err_detect_algorithm {
> > > > RTE_RAWDEV_MULTI_FN_ERR_DETECT_CRC32_ETH,
> > >
> > > IMO, It does not make sense to add protocol specific stuff in rawdev
> > > symbols.
> > >
> > > IMO, It is better to have a separate library for CRC and BIP32
> > > acceleration like the rte_security library and underneath still it
> > > can use rawdev or anydev if required.
> >
> > [DC] This protocol stuff is only in the rawdev interface definition, which
> > is
> known only to the application and the rawdev PMDs which will use this
> interface.
> > So these defines/enums/structs etc for CRC and BIP are completely
> opaque to rte_rawdev itself.
> >
> > This is how all existing rawdev PMDs interfaces are defined, where the
> interface is very specific to the job(s) the PMD is implementing.
>
> If you see .map file in driver/raw/. None of the drivers are exposing any API
> with rte_rawdev_*.
> This addition will be exposing new rte_rawdev_* APIs from driver/rawdev/.
> That's is not correct.
>
> $ find drivers/raw/ -name *.map
> drivers/raw/skeleton/rte_rawdev_skeleton_version.map
> drivers/raw/octeontx2_ep/rte_rawdev_octeontx2_ep_version.map
> drivers/raw/ntb/rte_rawdev_ntb_version.map
> drivers/raw/dpaa2_qdma/rte_rawdev_dpaa2_qdma_version.map
> drivers/raw/dpaa2_cmdif/rte_rawdev_dpaa2_cmdif_version.map
> drivers/raw/ioat/rte_rawdev_ioat_version.map
> drivers/raw/octeontx2_dma/rte_rawdev_octeontx2_dma_version.map
> drivers/raw/ifpga/rte_rawdev_ifpga_version.map
>
> IMO, Correct thing to do will be,
>
> Either of
>
> 1) As mentioned below, If you would like to limit the scope only to a new
> rawdev driver then
> a) Create a new driver at driver/raw//
> b) expose the drier specific customer API as
> rte__...(example:
> drivers/raw/dpaa2_qdma/rte_rawdev_dpaa2_qdma_version.map
>
> 2) If we would like to have public API then create a subsystem like
> libsecurity
> to have features. Let the API exposed from lib/...
>
[DC] Yes you are right here, it was incorrect to include rawdev in the
interface filename and in the symbols within... rawdev will be removed from all
these
And we are going with option 1 above, to limit this to the new rawdev drivers.
As I mentioned in the original post, if it is found that this interface could
be useful to other drivers/applications in the future, then it can be moved to
the public API under lib as a new library or an extension of an existing one
possibly
> >
> > Also, these particular defines/enums/structs for CRC and BIP are only for
> defining xform and op chains containing these particular operations.
> > The actual code to do the CRC and BIP is already in the AESNI-MB
> > library or DPDK rte_net_crc library, which our aesni_mb and qat rawdev
> > PMDs will call/use
> >
> > >
> > > IMO, Exposing the public API in
> > > drivers/raw/common/rte_rawdev_multi_fn.h is a shortcut.
> > > IMO, public API should be in lib/..
> >
> > [DC] To be honest, I tend to agree. I don't like that public APIs are
> > exposed
> from the drivers directory.
> > But as I mentioned above, this is how all rawdev PMD interfaces are
> > defined, where the interface definition is within the PMD directory
> > (e.g. drivers/raw/dpaa2_cmdif/rte_pmd_dpaa2_cmdif.h)
> > Our's is slightly different in that we have 2 PMDs which will use the
> > same interface, which is why we have added it in drivers/raw/common So
> > by keeping our interface under drivers, we are trying to be consistent
> > with all existing rawdev PMDs
> >
> > As I mentioned in my previous post though, this could potentially be
> > moved under lib in the future if other PMDs would find it useful
>
> See above. Point (1).
>
> >
> > We could possibly rename our interface file to rte_pmd_multi_fn.h to be a
> bit more consistent with the majority of the existing PMDs and take away the
> idea for now that this is some kind of extension to the main rte_rawdev API.
> > But unfortunately there is no full consistency in the rawdev PMD
> > interface filenames (e.g. dpaa2_cmdif uses the "rte_pmd_" prefix -
> > rte_pmd_dpaa2_cmdif.h, octeontx2_dma uses the "_rawdev" suffix -
> > otx2_dpi_rawdev.h)
> >
> > >
> > > Just my 2c.
Re: [dpdk-dev] [PATCH v1 2/2] crypto/aesni_mb: improve security instance setup
Hi Pablo
> -Original Message-
> From: De Lara Guarch, Pablo
> Sent: Friday, July 17, 2020 8:29 PM
> >
> > #ifdef AESNI_MB_DOCSIS_SEC_ENABLED
> > + struct rte_security_ctx *security_instance;
> > security_instance = rte_malloc("aesni_mb_sec",
> > sizeof(struct rte_security_ctx),
> > RTE_CACHE_LINE_SIZE);
>
> I see that there could be a potential memory leak here.
> Assuming this malloc works, if alloc_init_mb_mgr() fails, this memory will not
> be freed.
> So I suggest two options:
> 1 - Free security_instance if alloc_init_mb_mgr() fails
> 2 - Move this piece of code after alloc_init_mb_mgr and free mb_mgr if this
> malloc fails.
[DC] Good catch, disappointed I didn't spot that myself :(
This is fixed in v2 coming very shortly - used option 1 above
>
Re: [dpdk-dev] [PATCH v1 1/2] crypto/qat: improve security instance setup
Hi Akhil,
> -Original Message-
> From: Akhil Goyal
> Sent: Saturday, July 18, 2020 10:41 PM
> > > > This patch makes some minor improvements to the security instance
> > > > setup for the QAT SYM PMD. All of this setup code is now in one
> > > > '#ifdef RTE_LIBRTE_SECURITY' block. Enabling the
> > > > RTE_CRYPTODEV_FF_SECURITY feature for the device is also moved to
> this block.
> > > >
> > > > Fixes: 6f0ef237404b ("crypto/qat: support DOCSIS protocol")
> > > >
> > > > Signed-off-by: David Coyle
> > > Acked-by: Fiona Trahe
> >
> > This patch is applied to dpdk-next-crypto
> >
> > Please send next version for 2/2 of this series.
>
> No this patch is pulled back. I suppose the memory leak is there in this patch
> also.
[DC] Yes, memory leak is here too and is fixed in v2
Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
Hi Pablo, > -Original Message- > From: De Lara Guarch, Pablo > Sent: Friday, July 17, 2020 8:04 PM > > @@ -48,6 +48,10 @@ cperf_set_ops_security(struct rte_crypto_op **ops, > > } else > > buf_sz = options->test_buffer_size; > > > > + sym_op->m_src->buf_len = options->segment_sz; > > + sym_op->m_src->data_len = buf_sz; > > + sym_op->m_src->pkt_len = buf_sz; > > + > > Actually, I am wondering why this is needed at all (for DOCSIS and PDCP). This > is already set in " fill_multi_seg_mbuf" or " fill_single_seg_mbuf" (and this > was already working without this patch, right?). [DC] I have found that if a number of buffer sizes are specified like this on the cmd line "--buffer-sz 64,256,1024", then the pkt_len and data_len filled in "fill_multi_seg_mbuf" or " fill_single_seg_mbuf" is always the largest of the sizes specified. The cipher/auth lengths are then set based on the --buffer-sz option. For DOCSIS, I tried to be more accurate and set the correct pkt_len and data_len in the mbuf. This followed what PDCP did too, even though I'm not sure of the background why PDCP did it - possibly spotted the same issue. I have also found that DOCSIS performance figures can be better if the correct pkt_len and data_len are set in the mbuf - I don't have any proper explanation for this though as the cipher/ auth lengths are always the same. I've dug around a bit more on this now though and this is actually a problem across the perf tool. Some of the crypto PMDs have logic based on the mbuf pkt_len and data_len, but because the perf tool isn't always setting these fields correctly, that logic may not work as expected. >
Re: [dpdk-dev] [PATCH] doc: add deprecation notice for security session create API
> The API ``rte_security_session_create`` takes only single mempool for > session and session private data. So the application need to create mempool > for twice the number of sessions needed and will also lead to wastage of > memory as session private data need more memory compared to session. > Hence the API will be modified to take two mempool pointers - one for > session and one for private data. This is very similar to crypto based session > create APIs. > > Signed-off-by: Akhil Goyal > --- > doc/guides/rel_notes/deprecation.rst | 7 +++ > 1 file changed, 7 insertions(+) > Acked-by: David Coyle
Re: [dpdk-dev] [PATCH] security: update session create API
Hi Akhil
> -Original Message-
> From: akhil.go...@nxp.com
> Sent: Thursday, September 3, 2020 9:10 PM
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
> 70bf6fe2c..6d7da1408 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -7219,7 +7219,8 @@ test_pdcp_proto(int i, int oop,
>
> /* Create security session */
> ut_params->sec_session = rte_security_session_create(ctx,
> - &sess_conf, ts_params-
> >session_priv_mpool);
> + &sess_conf, ts_params->session_mpool,
> + ts_params->session_priv_mpool);
[DC] ts_params->session_mpool is a cryptodev sym session pool. The assumption
then in these security tests is that
security sessions are smaller than cryptodev sym sessions. This is currently
true, but may not always be.
There should possibly be a new mempool created for security sessions.
Or at least an assert somewhere to check a security session is smaller than a
cryptodev sym session, so that this doesn't
catch someone out in the future if security session grows in size.
The same comment applies to the crypto-perf-test and test_ipsec too
> diff --git a/app/test/test_security.c b/app/test/test_security.c index
> 77fd5adc6..ed7de348f 100644
> --- a/app/test/test_security.c
> +++ b/app/test/test_security.c
> @@ -237,6 +237,7 @@ static struct mock_session_create_data {
> struct rte_security_session_conf *conf;
> struct rte_security_session *sess;
> struct rte_mempool *mp;
> + struct rte_mempool *priv_mp;
>
> 790,7 +809,7 @@ test_session_create_inv_mempool(void)
> struct rte_security_session *sess;
>
> sess = rte_security_session_create(&ut_params->ctx, &ut_params-
> >conf,
> - NULL);
> + NULL, NULL);
[DC] This test test_session_create_inv_mempool() should have the priv_mp set to
a valid
value (i.e. ts_params->session_priv_mpool), and a new test function should be
added where
mp is valid, but priv_mp is NULL - this way we test for validity of both
mempools independently.
> a/doc/guides/prog_guide/rte_security.rst
> b/doc/guides/prog_guide/rte_security.rst
> index 127da2e4f..cff0653f5 100644
> --- a/doc/guides/prog_guide/rte_security.rst
> +++ b/doc/guides/prog_guide/rte_security.rst
> @@ -533,8 +533,10 @@ and this allows further acceleration of the offload of
> Crypto workloads.
>
> The Security framework provides APIs to create and free sessions for
> crypto/ethernet devices, where sessions are mempool objects. It is the
> application's responsibility -to create and manage the session mempools. The
> mempool object size should be able to -accommodate the driver's private
> data of security session.
> +to create and manage two session mempools - one for session and other
> +for session private data. The mempool object size should be able to
> +accommodate the driver's private data of security session. The
> +application can get the size of session private data using API
> ``rte_security_session_get_size``.
[DC] This sentence should be updated to specify it's the private session data
mempool that is being referred to
"The mempool object size should be able to accommodate the driver's private
data of security session."
=>
"The private session data mempool object size should be able to accommodate the
driver's private data of security
session."
Also, a sentence about the required size of the session mempool should also be
added.
> diff --git a/doc/guides/rel_notes/release_20_11.rst
> b/doc/guides/rel_notes/release_20_11.rst
> index df227a177..04c1a1b81 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -84,6 +84,12 @@ API Changes
> Also, make sure to start the actual text at the margin.
> ===
>
> +* security: The API ``rte_security_session_create`` is updated to take
> +two
> + mempool objects one for session and other for session private data.
> + So the application need to create two mempools and get the size of
> +session
> + private data using API ``rte_security_session_get_size`` for private
> +session
> + mempool.
> +
[DC] Many of the PMDs which support security don't implement the
session_get_size
callback. There's probably a job here for each PMD owner to add support for
this callback.
>
> ABI Changes
> ---
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-
> secgw/ipsec-secgw.c
> index 8ba15d23c..55a5ea9f4 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -2379,12 +2375,8 @@ session_priv_pool_init(struct socket_ctx *ctx,
> int32_t socket_id,
>
> snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
> "sess_mp_priv_%u", socket_id);
> - /*
> - * Doubled due to rte_security_session_create() uses one mempool
> for
> -
Re: [dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD bitwidth
Hi Ciara,
> From: dev On Behalf Of Ciara Power
> When choosing a vector path to take, an extra condition must be satisfied to
> ensure the max SIMD bitwidth allows for the CPU enabled path.
>
> The vector path was initially chosen in RTE_INIT, however this is no longer
> suitable as we cannot check the max SIMD bitwidth at that time.
> The default chosen in RTE_INIT is now scalar. For best performance and to
> use vector paths, apps must explicitly call the set algorithm function before
> using other functions from this library, as this is where vector handlers are
> now chosen.
[DC] Has it been decided that it is ok to now require applications to pick the
CRC algorithm they want to use?
An application which previously automatically got SSE4.2 CRC, for example, will
now automatically only get scalar.
If this is ok, this should probably be called out explicitly in release notes
as it may
not be Immediately noticeable to users that they now need to select the CRC
algo.
Actually, in general, the release notes need to be updated for this patchset.
>
> Suggested-by: Jasvinder Singh
>
> Signed-off-by: Ciara Power
>
> ---
> v3:
> - Moved choosing vector paths out of RTE_INIT.
> - Moved checking max_simd_bitwidth into the set_alg function.
> ---
> lib/librte_net/rte_net_crc.c | 26 +-
> lib/librte_net/rte_net_crc.h | 3 ++-
> 2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_net/rte_net_crc.c b/lib/librte_net/rte_net_crc.c index
> 9fd4794a9d..241eb16399 100644
> --- a/lib/librte_net/rte_net_crc.c
> +++ b/lib/librte_net/rte_net_crc.c
> @@ -145,18 +149,26 @@ rte_crc32_eth_handler(const uint8_t *data,
> uint32_t data_len) void rte_net_crc_set_alg(enum rte_net_crc_alg alg) {
> + if (max_simd_bitwidth == 0)
> + max_simd_bitwidth = rte_get_max_simd_bitwidth();
> +
> switch (alg) {
> #ifdef X86_64_SSE42_PCLMULQDQ
> case RTE_NET_CRC_SSE42:
> - handlers = handlers_sse42;
> - break;
> + if (max_simd_bitwidth >= RTE_MAX_128_SIMD) {
> + handlers = handlers_sse42;
> + return;
> + }
> + RTE_LOG(INFO, NET, "Max SIMD Bitwidth too low, using
> scalar\n");
[DC] Not sure if you're aware but there is another patchset which adds an
AVX512 CRC
implementation and run-time checking of cpuflags to select the CRC path to use:
https://patchwork.dpdk.org/project/dpdk/list/?series=12596
There will be a task to merge these 2 patchsets if both are merged. It looks
fairly
straightforward to me to merge these, but it would be good if you take a look
too
Re: [dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD bitwidth
Hi Jasvinder/Ciara
> -Original Message-
> From: Singh, Jasvinder
> >
> > > From: dev On Behalf Of Ciara Power When
> > > choosing a vector path to take, an extra condition must be satisfied
> > > to ensure the max SIMD bitwidth allows for the CPU enabled path.
> > >
> > > The vector path was initially chosen in RTE_INIT, however this is no
> > > longer suitable as we cannot check the max SIMD bitwidth at that time.
> > > The default chosen in RTE_INIT is now scalar. For best performance
> > > and to use vector paths, apps must explicitly call the set algorithm
> > > function before using other functions from this library, as this is
> > > where vector handlers are now chosen.
> >
> > [DC] Has it been decided that it is ok to now require applications to
> > pick the CRC algorithm they want to use?
> >
> > An application which previously automatically got SSE4.2 CRC, for
> > example, will now automatically only get scalar.
> >
> > If this is ok, this should probably be called out explicitly in
> > release notes as it may not be Immediately noticeable to users that
> > they now need to select the CRC algo.
> >
> > Actually, in general, the release notes need to be updated for this
> patchset.
>
> The decision to move rte_set_alg() out of RTE_INIT was taken to avoid check
> on max_simd_bitwidth in data path for every single time when crc_calc() api
> is invoked. Based on my understanding, max_simd_bitwidth is set after eal
> init, and when used in crc_calc(), it might override the default crc algo set
> during RTE_INIT. Therefore, to avoid extra check on max_simd_bitwidth in
> data path, better option will be to use this static configuration one time
> after
> eal init in the set_algo API.
[DC] Yes that is a good change to have made to avoid extra datapath checks.
Based on off-list discussion, I now also know the reason behind now defaulting
to scalar CRC in RTE_INIT. If a higher bitwidth CRC was chosen by RTE_INIT (e.g.
SSE4.2 CRC) but the max_simd_bitwidth was then set to RTE_NO_SIMD (64) through
the EAL parameter or call to rte_set_max_simd_bitwidth(), then there is a
mismatch
if rte_net_crc_set_alg() is not then called to reconfigure the CRC. Defaulting
to scalar
avoids this mismatch and works on all archs
As I mentioned before, I think this needs to be called out in release notes, as
it's an
under-the-hood change which could cause app performance to drop if app
developers
aren't aware of it - the API itself hasn't changed, so they may not read the
doxygen :)
>
>
> > >
> > > Suggested-by: Jasvinder Singh
> > >
> > > Signed-off-by: Ciara Power
> > >
> > > ---
> > > v3:
> > > - Moved choosing vector paths out of RTE_INIT.
> > > - Moved checking max_simd_bitwidth into the set_alg function.
> > > ---
> > > lib/librte_net/rte_net_crc.c | 26 +-
> > > lib/librte_net/rte_net_crc.h | 3 ++-
> > > 2 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/lib/librte_net/rte_net_crc.c
> > > b/lib/librte_net/rte_net_crc.c index
> > > 9fd4794a9d..241eb16399 100644
> > > --- a/lib/librte_net/rte_net_crc.c
> > > +++ b/lib/librte_net/rte_net_crc.c
> >
> >
> >
> > > @@ -145,18 +149,26 @@ rte_crc32_eth_handler(const uint8_t *data,
> > > uint32_t data_len) void rte_net_crc_set_alg(enum rte_net_crc_alg
> > > alg) {
> > > + if (max_simd_bitwidth == 0)
> > > + max_simd_bitwidth = rte_get_max_simd_bitwidth();
> > > +
> > > switch (alg) {
> > > #ifdef X86_64_SSE42_PCLMULQDQ
> > > case RTE_NET_CRC_SSE42:
> > > - handlers = handlers_sse42;
> > > - break;
> > > + if (max_simd_bitwidth >= RTE_MAX_128_SIMD) {
> > > + handlers = handlers_sse42;
> > > + return;
> > > + }
> > > + RTE_LOG(INFO, NET, "Max SIMD Bitwidth too low, using
> > > scalar\n");
> >
> > [DC] Not sure if you're aware but there is another patchset which adds
> > an
> > AVX512 CRC implementation and run-time checking of cpuflags to select
> > the CRC path to use:
> > https://patchwork.dpdk.org/project/dpdk/list/?series=12596
> >
> > There will be a task to merge these 2 patchsets if both are merged. It
> > looks fairly straightforward to me to merge these, but it would be
> > good if you take a look too
Re: [dpdk-dev] [PATCH v3 01/18] eal: add max SIMD bitwidth
Hi Ciara
> -Original Message-
> From: dev On Behalf Of Ciara Power
> diff --git a/lib/librte_eal/common/eal_internal_cfg.h
> b/lib/librte_eal/common/eal_internal_cfg.h
> index 13f93388a7..367e0cc19e 100644
> --- a/lib/librte_eal/common/eal_internal_cfg.h
> +++ b/lib/librte_eal/common/eal_internal_cfg.h
> @@ -33,6 +33,12 @@ struct hugepage_info {
> int lock_descriptor;/**< file descriptor for hugepage dir */
> };
>
> +struct simd_bitwidth {
> + /**< flag indicating if bitwidth is locked from further modification */
> + bool locked;
> + uint16_t bitwidth; /**< bitwidth value */ };
[DC] The doxygen comment on 'locked' flag uses '/**<' so should come after the
field.
Having the comment after the field seems to be the way it's done in this file
so I'd move
the comment as opposed to removing the '<'
> +
> /**
> * internal configuration
> */
> @@ -85,6 +91,8 @@ struct internal_config {
> volatile unsigned int init_complete;
> /**< indicates whether EAL has completed initialization */
> unsigned int no_telemetry; /**< true to disable Telemetry */
> + /** max simd bitwidth path to use */
> + struct simd_bitwidth max_simd_bitwidth;
[DC] Again the doxygen comments seem to come after the struct fields in this
file
so I'd move the comment for max_simd_bitwidth to after it and add the '<'
> };
>
> void eal_reset_internal_config(struct internal_config *internal_cfg); diff
> --git
>
> diff --git a/lib/librte_eal/include/rte_eal.h
> b/lib/librte_eal/include/rte_eal.h
> index ddcf6a2e7a..fb739f3474 100644
> --- a/lib/librte_eal/include/rte_eal.h
> +++ b/lib/librte_eal/include/rte_eal.h
> @@ -43,6 +43,14 @@ enum rte_proc_type_t {
> RTE_PROC_INVALID
> };
>
> +enum rte_max_simd_t {
> + RTE_NO_SIMD = 64,
> + RTE_MAX_128_SIMD = 128,
> + RTE_MAX_256_SIMD = 256,
> + RTE_MAX_512_SIMD = 512,
> + RTE_MAX_SIMD_DISABLE = UINT16_MAX,
> +};
[DC] Add doxygen comments on enum rte_max_simd_t and each of it's values
> +
> /**
> * Get the process type in a multi-process setup
> *
> @@ -51,6 +59,31 @@ enum rte_proc_type_t {
> */
> enum rte_proc_type_t rte_eal_process_type(void);
>
> +/**
> + * Get the supported SIMD bitwidth.
> + *
> + * @return
> + * uint16_t bitwidth.
> + */
> +__rte_experimental
> +uint16_t rte_get_max_simd_bitwidth(void);
> +
> +/**
> + * Set the supported SIMD bitwidth.
> + * This API should only be called once at initialization, before EAL init.
> + *
> + * @param bitwidth
> + * uint16_t bitwidth.
> + * @return
> + * 0 on success.
> + * @return
> + * -EINVAL on invalid bitwidth parameter.
> + * @return
> + * -EPERM if bitwidth is locked.
[DC] Minor thing.. normally there's just 1 @return tag with all of the return
values under
it as a bullet list
> + */
> +__rte_experimental
> +int rte_set_max_simd_bitwidth(uint16_t bitwidth);
> +
> /**
> * Request iopl privilege for all RPL.
> *
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index c32461c663..17a7195a3d 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -397,6 +397,10 @@ EXPERIMENTAL {
> rte_service_lcore_may_be_active;
> rte_thread_register;
> rte_thread_unregister;
> +
> + # added in 20.11
> + rte_get_max_simd_bitwidth;
> + rte_set_max_simd_bitwidth;
> };
[DC] rte_get_max_simd_bitwidth is called from rte_net_crc (and other libraries)
so this
symbol possibly needs to be added to librte_eal/rte_eal_exports.def file too.
This is the windows symbol export file, used on windows build.
This has caught us out on the AVX512 CRC patchset
https://patchwork.dpdk.org/project/dpdk/list/?series=12596
where a windows build failed in the 'ci/iol-testing' checks in patchwork because
rte_net_crc couldn't find the symbol rte_cpu_get_flag_enabled, which also comes
from rte_eal. We have to add this symbol to rte_eal_exports.def to fix this.
The 'ci/iol-testing' check has not run for your patchset so I can't say for
certain if the
windows build would have failed for you, but I think it would
>
> INTERNAL {
> --
> 2.17.1
Re: [dpdk-dev] [PATCH 2/7] security: modify PDCP xform to support SDAP
Hi Akhil
> -Original Message-
> From: akhil.go...@nxp.com
> diff --git a/doc/guides/prog_guide/rte_security.rst
> b/doc/guides/prog_guide/rte_security.rst
> index 127da2e4f..ab535d1cd 100644
> --- a/doc/guides/prog_guide/rte_security.rst
> +++ b/doc/guides/prog_guide/rte_security.rst
> @@ -1,5 +1,5 @@
> @@ -693,6 +693,23 @@ PDCP related configuration parameters are defined
> in ``rte_security_pdcp_xform``
> uint32_t hfn;
> /** HFN Threshold for key renegotiation */
> uint32_t hfn_threshold;
> +/** HFN can be given as a per packet value also.
> + * As we do not have IV in case of PDCP, and HFN is
> + * used to generate IV. IV field can be used to get the
> + * per packet HFN while enq/deq.
> + * If hfn_ovrd field is set, user is expected to set the
> + * per packet HFN in place of IV. PMDs will extract the HFN
> + * and perform operations accordingly.
> + */
> + uint8_t hfn_ovrd;
> + /** In case of 5G NR, a new protocol(SDAP) header may be set
> + * inside PDCP payload which should be authenticated but not
> + * encrypted. Hence, driver should be notified if SDAP is
> + * enabled or not, so that SDAP header is not encrypted.
> + */
> + uint8_t sdap_enabled;
> + /** Reserved for future */
> + uint16_t reserved;
> };
[DC] Should we consider removing the API code out of the security documentation?
It's a direct copy of the API code itself, and just means 2 files need to be
updated for every API change.
And as with 'hfn_ovrd', sometimes it's forgotten.
>From maintainability point of view, it might be better just remove it.
>
> DOCSIS related configuration parameters are defined in
> ``rte_security_docsis_xform`` diff --git a/lib/librte_security/rte_security.h
> b/lib/librte_security/rte_security.h
> index 16839e539..48b377b20 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -1,5 +1,5 @@
> /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright 2017,2019 NXP
> + * Copyright 2017,2019-2020 NXP
> * Copyright(c) 2017-2020 Intel Corporation.
> */
>
> @@ -290,7 +290,15 @@ struct rte_security_pdcp_xform {
>* per packet HFN in place of IV. PMDs will extract the HFN
>* and perform operations accordingly.
>*/
> - uint32_t hfn_ovrd;
> + uint8_t hfn_ovrd;
> + /** In case of 5G NR, a new protocol(SDAP) header may be set
[DC] Very minor thing... add space between 'protocol' and '(SDAP)' in the
comment block.
And same comment for the documentation if you choose to keep the API code
blocks there too.
> + * inside PDCP payload which should be authenticated but not
> + * encrypted. Hence, driver should be notified if SDAP is
> + * enabled or not, so that SDAP header is not encrypted.
> + */
> + uint8_t sdap_enabled;
> + /** Reserved for future */
> + uint16_t reserved;
> };
>
> /** DOCSIS direction */
> --
> 2.17.1
Re: [dpdk-dev] [PATCH v1] crypto/aesni_mb: fix incorrect clearing of security session
Hi Pablo
> >
> > When destroying a security session, the AESNI-MB PMD attempted to
> > clear the private aesni_mb session object to remove any key material.
> > However, the function aesni_mb_pmd_sec_sess_destroy() cleared the
> > security session object instead of the private session object.
> >
> > This patch fixes this issue by now clearing the private session object.
> >
> > Fixes: fda5216fba55 ("crypto/aesni_mb: support DOCSIS protocol")
> >
> > Signed-off-by: David Coyle
>
> Patch looks good, but you need to CC stable, as this should be backported
> since the issue was introduced in the previous release.
> So, add Cc: sta...@dpdk.org after Fixes: fda... and send a v2.
> Apart from that, you can keep my ack:
[DC] Done, thanks for pointing that out
>
> Acked-by: Pablo de Lara
Re: [dpdk-dev] [PATCH v4 1/2] net: add run-time architecture specific CRC selection
Hi Konstantin, thanks for your review
> -Original Message-
> From: Ananyev, Konstantin
> Sent: Wednesday, October 7, 2020 3:59 PM
>
> >
> > This patch adds support for run-time selection of the optimal
> > architecture-specific CRC path, based on the supported instruction
> > set(s) of the CPU.
> >
> > The compiler option checks have been moved from the C files to the
> > meson script. The rte_cpu_get_flag_enabled function is called
> > automatically by the library at process initialization time to
> > determine which instructions the CPU supports, with the most optimal
> > supported CRC path ultimately selected.
> >
> > Signed-off-by: Mairtin o Loingsigh
> > Signed-off-by: David Coyle
>
> LGTM, just one nit see below.
> With that:
> Series acked-by: Konstantin Ananyev
>
> > ---
> > doc/guides/rel_notes/release_20_11.rst| 4 ++
> > lib/librte_net/meson.build| 34 +++-
> > lib/librte_net/net_crc.h | 34
> > lib/librte_net/{net_crc_neon.h => net_crc_neon.c} | 26 +++--
> > lib/librte_net/{net_crc_sse.h => net_crc_sse.c} | 34
> > lib/librte_net/rte_net_crc.c | 67
> > ++-
> > 6 files changed, 131 insertions(+), 68 deletions(-) create mode
> > 100644 lib/librte_net/net_crc.h rename lib/librte_net/{net_crc_neon.h
> > => net_crc_neon.c} (95%) rename lib/librte_net/{net_crc_sse.h =>
> > net_crc_sse.c} (94%)
> >
> >
> > +#ifdef CC_X86_64_SSE42_PCLMULQDQ_SUPPORT static uint8_t
> > +sse42_pclmulqdq_cpu_supported(void)
> > +{
> > + return rte_cpu_get_flag_enabled(RTE_CPUFLAG_PCLMULQDQ);
> > +}
>
> As a nit, I think it would be better to hide #fidef inside the function, and
> return an 0 when define is not set.
> Something like:
>
> static int
> sse42_pclmulqdq_cpu_supported(void)
> {
> #ifdef CC_X86_64_SSE42_PCLMULQDQ_SUPPORT
> return rte_cpu_get_flag_enabled(RTE_CPUFLAG_PCLMULQDQ);
> #else
> return 0;
> }
>
> Same for other cpu_supported functions.
> And then you can remove these ifdefs in set_alg and other palces, i.e.:
>
> void
> rte_net_crc_set_alg(enum rte_net_crc_alg alg) {
> switch (alg) {
> #ifdef RTE_ARCH_X86_64
> case RTE_NET_CRC_AVX512:
> if (avx512_vpclmulqdq_cpu_supported()) {
> handlers = handlers_avx512;
> break;
> }
> /* fall-through */
> case RTE_NET_CRC_SSE42:
> if (sse42_pclmulqdq_cpu_supported()) {
> handlers = handlers_sse42;
> break;
> }
> #endif
> ...
>
> Same for rte_net_crc_init()
[DC] I have reworked the ifdefs in this file based on your comments here and
off-list discussions.
These are available now in the v5.
All ifdef's have been removed out the API function definitions and moved down
into 'helper' type
functions - looks much cleaner now.
Your Ack has been carried through too to v5 as you mentioned
>
> > +#endif
> > +
> > +#ifdef CC_ARM64_NEON_PMULL_SUPPORT
> > +static uint8_t
> > +neon_pmull_cpu_supported(void)
> > +{
> > + return rte_cpu_get_flag_enabled(RTE_CPUFLAG_PMULL);
> > +}
> > +#endif
> > +
> > void
> > rte_net_crc_set_alg(enum rte_net_crc_alg alg) {
> > switch (alg) {
> > -#ifdef X86_64_SSE42_PCLMULQDQ
> > +#ifdef RTE_ARCH_X86_64
> > case RTE_NET_CRC_SSE42:
> > - handlers = handlers_sse42;
> > - break;
> > -#elif defined ARM64_NEON_PMULL
> > - /* fall-through */
> > +#ifdef CC_X86_64_SSE42_PCLMULQDQ_SUPPORT
> > + if (sse42_pclmulqdq_cpu_supported()) {
> > + handlers = handlers_sse42;
> > + break;
> > + }
> > +#endif
> > +#endif /* RTE_ARCH_X86_64 */
> > +#ifdef RTE_ARCH_ARM64
> > case RTE_NET_CRC_NEON:
> > - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_PMULL)) {
> > +#ifdef CC_ARM64_NEON_PMULL_SUPPORT
> > + if (neon_pmull_cpu_supported()) {
> > handlers = handlers_neon;
> > break;
> > }
> > #endif
> > +#endif /* RTE_ARCH_ARM64 */
> > /* fall-through */
> > case RTE_NET_CRC_SCALAR:
> > /* fall-through */
> > @@ -188,11 +200,14 @@ RTE_INIT(rte_net_crc_init)
> >
> > rte_net_crc_scalar_init();
> >
> > -#ifdef X86_64_SSE42_PCLMULQDQ
> > - alg = RTE_NET_CRC_SSE42;
> > - rte_net_crc_sse42_init();
> > -#elif defined ARM64_NEON_PMULL
> > - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_PMULL)) {
> > +#ifdef CC_X86_64_SSE42_PCLMULQDQ_SUPPORT
> > + if (sse42_pclmulqdq_cpu_supported()) {
> > + alg = RTE_NET_CRC_SSE42;
> > + rte_net_crc_sse42_init();
> > + }
> > +#endif
> > +#ifdef CC_ARM64_NEON_PMULL_SUPPORT
> > + if (neon_pmull_cpu_supported()) {
> > alg = RTE_NET_CRC_NEON;
> > rte_net_crc_neon_init();
> > }
> > --
> > 2.12.3
