RE: [PATCH 0/1] Add security perf application

[Akhil Goyal <gakhil@marvell.com>]

> Hi Akhil,
> 
> +Techboard for guidance
> 
The proposal is accepted in techboard.

Please fix compilation issues reported in CI.


> > I agree that common part would be init only but it can scale for non-security
> > sessions easily.
> 
> Currently, dpdk-test-crypto-perf has a data path framework which prepares
> crypto_operations based on session. The proposed application is about
> measuring performance when creating and destroying sessions. So it would take
> rte_security_conf as the argument and current data path framework would be
> completely bypassed. Current dpdk-test-crypto-perf creates mbuf_pool etc and
> creates one session per core. This app would need larger pool for sessions and
> no pool for crypto_op or mbuf. Moreover, dpdk-test-crypto-perf works on
> cryptodev while security-perf can work on rte_ethdevs as well. I still do not see
> any community feedback on whether plugging rte_ethdev init etc in dpdk-test-
> crypto-perf is the right thing to do.
> 
> So other than basic eal_init(), I do not see anything common and even in the
> long run, this gulf is bound to grow. If the app has to be integrated into dpdk-
> test-crypto-perf, then it will be separate .c & .h files and completely branch out
> after very early init phase. The testing methodology and philosophy would also
> be different (for security-perf, we are running all algos supported as there is no
> need for command line parsing of all algos. CL parsing would be added for
> protocol features like custom AR window size). DPDK community had earlier
> encountered same issue with "test-flow-perf" which could have been integrated
> into "test-pmd" in a similar manner. But DPDK community decided to allow
> "test-flow-perf" and so the same logic can be applied here as well.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Akhil Goyal <gakhil@marvell.com>
> > Sent: Wednesday, September 28, 2022 12:47 AM
> > To: Anoob Joseph <anoobj@marvell.com>
> > Cc: Aakash Sasidharan <asasidharan@marvell.com>; dev@dpdk.org;
> > techboard@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> > Thomas Monjalon <thomas@monjalon.net>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@oss.nxp.com>; Ciara Power <ciara.power@intel.com>
> > Subject: RE: [PATCH 0/1] Add security perf application
> >
> > Hi Anoob,
> > > Hi Akhil,
> > >
> > > Do you have any further comments?
> > > > > > Subject: [PATCH 0/1] Add security perf application
> > > > > >
> > > > > > Add performance application to test security session create &
> > > > > > destroy rates supported by the security enabled cryptodev PMD.
> > > > > > The application would create specified number of sessions and
> > > > > > captures the time taken for the same before proceeding to
> > > > > > destroy of the same. When operating on multi-core, the number of
> > > > > > sessions would be evenly distributed across all cores.
> > > > > >
> > > > > > The application would test with all combinations of cipher &
> > > > > > auth algorithms supported by the PMD.
> > > > > >
> > > > > > The app is similar to 'test-flow-perf' tool which captures the
> > > > > > rate at which flow rules can be created and destroyed.
> > > > > >
> > > > > Is it not good to add this into dpdk-test-crypto-perf?
> > > >
> > > > [Anoob] IMO, It is not good. Following are the reasons,
> > > >
> > > > Dpdk-test-crypto-perf is primarily for capturing crypto operation
> > throughputs.
> > > > And so the framework allocates minimal number of sessions and the
> > > > datapath function pointer etc deals with only one session. The
> > > > entire framework
> > > available
> > > > in that application is for populating crypto_op and mbuf, which is
> > > > not required for this app. Touching that framework would mean
> > > > throughput tests would get affected, which I don't think is the
> > > > right thing to do. And for PMDs like Intel's (which don't have
> > > > security support), it would be an unnecessary performance drop.
> > > >
> > > > The proposed app currently runs for all supported ciphers while in
> > > > dpdk-test- crypto-perf, it runs only for a specific algorithm
> > > > combination. If we want to
> > > limit
> > > > the functionality of the proposed app to match dpdk-test-crypto-perf
> > > > usage, that also calls for a major rework.
> > > >
> > > > And the only thing that can be reused is probably cryptodev init &
> > > > queue pair configuration. As you are well aware, security device can
> > > > be cryptodev or an ethdev. Dpdk-test-crypto-perf doesn't have
> > > > support for initializing ethdev and rightfully so. Adding this to an
> > > > already complicated framework will be counter productive in the long
> > run.
> > > >
> > > > > Can we add as a separate .c file, say, cperf_test_sec_session.c in
> > > > > test-crypto- perf folder and use the existing framework.
> > > >
> > > > [Anoob] As I mentioned earlier, nothing from the framework can be
> > > > leveraged for this application. If you insist on not having a new
> > > > app, then all this can be integrated into dpdk-test-crypto-perf, but
> > > > that will follow it's own path from very early stage (mempool
> > > > allocations etc need to happen differently). And it would mean
> > > > adding more command line options (which is currently at 37) as
> > > we
> > > > add more options for measuring security perf.
> > > >
> > Are you planning to add more options is that app?
> > if not, then adding just one more option about nb_sess would do trick in
> > test-crypto-perf.
> > You would just need to add 2 new functions (test_security_session_perf and
> > sec_conf_init) in a new .c file in app/test-crypto-perf/ and the mempool_init
> > is being called from
> > cperf_initialize_cryptodev() which we can hook to get the nb_sessions from
> > the command line arguments.
> > I do not suspect any changes in datapath - so it won't be an issue.
> > The point is not about the things being common in the two apps. The point is
> > whether we can accommodate in existing app or not. We cannot have too
> > many different apps.
> > We only introduce apps which are not possible to accommodate in existing
> > ones.
> > I remember, there was discussion in past about having a new app for testing
> > multi-process for crypto.
> > But that was dropped as we do not want too many apps.
> > I agree that common part would be init only but it can scale for non-security
> > sessions easily.
> >
> > Regards,
> > Akhil