[PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[akhil.goyal]

From: Akhil Goyal <akhil.goyal@nxp.com>

In IPsec-secgw application when TTL is decremented in IP header
before forwarding the packet, checksum needs to be updated.

In this patch an incremental checksum is added.
Other applications(like l3fwd) are also doing so.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
---
 examples/ipsec-secgw/ipip.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
index ff1dccd..ef059a9 100644
--- a/examples/ipsec-secgw/ipip.h
+++ b/examples/ipsec-secgw/ipip.h
@@ -56,6 +56,7 @@ ipip_outbound(struct rte_mbuf *m, uint32_t offset, uint32_t is_ipv6,
 	if (inip4->ip_v == IPVERSION) {
 		/* XXX This should be done by the forwarding engine instead */
 		inip4->ip_ttl -= 1;
+		inip4->ip_sum += 1;
 		ds_ecn = inip4->ip_tos;
 	} else {
 		inip6 = (struct ip6_hdr *)inip4;
-- 
1.9.1

Re: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[Sergio Gonzalez Monroy]

Hi Akhil,

This application relies on checksum offload in both outbound and inbound 
paths (PKT_TX_IP_CKSUM flag).

Because we assume that we always forward the packet in both paths, we 
decrement the ttl in both inbound and outbound.
You seem to only increment (recalculate) the checksum of the inner IP 
header in the outbound path but not the inbound path.

Also, in the inbound path you have to consider a possible ECN value update.

Sergio


On 26/09/2016 17:32, akhil.goyal@nxp.com wrote:
> From: Akhil Goyal <akhil.goyal@nxp.com>
>
> In IPsec-secgw application when TTL is decremented in IP header
> before forwarding the packet, checksum needs to be updated.
>
> In this patch an incremental checksum is added.
> Other applications(like l3fwd) are also doing so.
>
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---
>   examples/ipsec-secgw/ipip.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
> index ff1dccd..ef059a9 100644
> --- a/examples/ipsec-secgw/ipip.h
> +++ b/examples/ipsec-secgw/ipip.h
> @@ -56,6 +56,7 @@ ipip_outbound(struct rte_mbuf *m, uint32_t offset, uint32_t is_ipv6,
>   	if (inip4->ip_v == IPVERSION) {
>   		/* XXX This should be done by the forwarding engine instead */
>   		inip4->ip_ttl -= 1;
> +		inip4->ip_sum += 1;
>   		ds_ecn = inip4->ip_tos;
>   	} else {
>   		inip6 = (struct ip6_hdr *)inip4;

Re: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio Gonzalez
> Monroy
> Sent: Monday, September 26, 2016 6:28 AM
> To: akhil.goyal@nxp.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update checksum
> while decrementing ttl
> 
> Hi Akhil,
> 
> This application relies on checksum offload in both outbound and inbound
> paths (PKT_TX_IP_CKSUM flag).
> 
> Because we assume that we always forward the packet in both paths, we
> decrement the ttl in both inbound and outbound.
> You seem to only increment (recalculate) the checksum of the inner IP
> header in the outbound path but not the inbound path.
> 
> Also, in the inbound path you have to consider a possible ECN value update.

Any further comments here, Akhil?

Thanks,
Pablo

> 
> Sergio
> 
> 
> On 26/09/2016 17:32, akhil.goyal@nxp.com wrote:
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> >
> > In IPsec-secgw application when TTL is decremented in IP header
> > before forwarding the packet, checksum needs to be updated.
> >
> > In this patch an incremental checksum is added.
> > Other applications(like l3fwd) are also doing so.
> >
> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> > ---
> >   examples/ipsec-secgw/ipip.h | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
> > index ff1dccd..ef059a9 100644
> > --- a/examples/ipsec-secgw/ipip.h
> > +++ b/examples/ipsec-secgw/ipip.h
> > @@ -56,6 +56,7 @@ ipip_outbound(struct rte_mbuf *m, uint32_t offset,
> uint32_t is_ipv6,
> >   	if (inip4->ip_v == IPVERSION) {
> >   		/* XXX This should be done by the forwarding engine instead
> */
> >   		inip4->ip_ttl -= 1;
> > +		inip4->ip_sum += 1;
> >   		ds_ecn = inip4->ip_tos;
> >   	} else {
> >   		inip6 = (struct ip6_hdr *)inip4;
> 
> 

Re: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[Akhil Goyal]

On 10/5/2016 6:04 AM, De Lara Guarch, Pablo wrote:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio Gonzalez
>> Monroy
>> Sent: Monday, September 26, 2016 6:28 AM
>> To: akhil.goyal@nxp.com; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update checksum
>> while decrementing ttl
>>
>> Hi Akhil,
>>
>> This application relies on checksum offload in both outbound and inbound
>> paths (PKT_TX_IP_CKSUM flag).
[Akhil]Agreed that the application relies on checksum offload, but here 
we are talking about the inner ip header. Inner IP checksum will be 
updated on the next end point after decryption. This would expect that 
the next end point must have checksum offload capability. What if we are 
capturing the encrypted packets on wireshark or say send it to some 
other machine which does not run DPDK and do not know about checksum 
offload, then wireshark/other machine will not be able to get the 
correct the checksum and will show error.
>>
>> Because we assume that we always forward the packet in both paths, we
>> decrement the ttl in both inbound and outbound.
>> You seem to only increment (recalculate) the checksum of the inner IP
>> header in the outbound path but not the inbound path.
[Akhil]Correct I missed out the inbound path.
>>
>> Also, in the inbound path you have to consider a possible ECN value update.
[Akhil]If I take care of the ECN then it would mean I need to calculate 
the checksum completely, incremental checksum wont give correct results. 
This would surely impact performance. Any suggestion on how should we 
take care of ECN update. Should I recalculate the checksum and send the 
patch for ECN update? Or do we have a better solution.
>
> Any further comments here, Akhil?
>
> Thanks,
> Pablo
>
[Akhil] Sorry I missed out the previous reply from Sergio.

Thanks,
Akhil
>>
>> Sergio
>>
>>
>> On 26/09/2016 17:32, akhil.goyal@nxp.com wrote:
>>> From: Akhil Goyal <akhil.goyal@nxp.com>
>>>
>>> In IPsec-secgw application when TTL is decremented in IP header
>>> before forwarding the packet, checksum needs to be updated.
>>>
>>> In this patch an incremental checksum is added.
>>> Other applications(like l3fwd) are also doing so.
>>>
>>> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
>>> ---
>>>   examples/ipsec-secgw/ipip.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
>>> index ff1dccd..ef059a9 100644
>>> --- a/examples/ipsec-secgw/ipip.h
>>> +++ b/examples/ipsec-secgw/ipip.h
>>> @@ -56,6 +56,7 @@ ipip_outbound(struct rte_mbuf *m, uint32_t offset,
>> uint32_t is_ipv6,
>>>   	if (inip4->ip_v == IPVERSION) {
>>>   		/* XXX This should be done by the forwarding engine instead
>> */
>>>   		inip4->ip_ttl -= 1;
>>> +		inip4->ip_sum += 1;
>>>   		ds_ecn = inip4->ip_tos;
>>>   	} else {
>>>   		inip6 = (struct ip6_hdr *)inip4;
>>
>>
>
>

Re: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Tuesday, October 04, 2016 11:33 PM
> To: De Lara Guarch, Pablo; Gonzalez Monroy, Sergio; dev@dpdk.org
> Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while
> decrementing ttl
> 
> On 10/5/2016 6:04 AM, De Lara Guarch, Pablo wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio Gonzalez
> >> Monroy
> >> Sent: Monday, September 26, 2016 6:28 AM
> >> To: akhil.goyal@nxp.com; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update checksum
> >> while decrementing ttl
> >>
> >> Hi Akhil,
> >>
> >> This application relies on checksum offload in both outbound and
> inbound
> >> paths (PKT_TX_IP_CKSUM flag).
> [Akhil]Agreed that the application relies on checksum offload, but here
> we are talking about the inner ip header. Inner IP checksum will be
> updated on the next end point after decryption. This would expect that
> the next end point must have checksum offload capability. What if we are
> capturing the encrypted packets on wireshark or say send it to some
> other machine which does not run DPDK and do not know about checksum
> offload, then wireshark/other machine will not be able to get the
> correct the checksum and will show error.
> >>
> >> Because we assume that we always forward the packet in both paths, we
> >> decrement the ttl in both inbound and outbound.
> >> You seem to only increment (recalculate) the checksum of the inner IP
> >> header in the outbound path but not the inbound path.
> [Akhil]Correct I missed out the inbound path.
> >>
> >> Also, in the inbound path you have to consider a possible ECN value
> update.
> [Akhil]If I take care of the ECN then it would mean I need to calculate
> the checksum completely, incremental checksum wont give correct results.
> This would surely impact performance. Any suggestion on how should we
> take care of ECN update. Should I recalculate the checksum and send the
> patch for ECN update? Or do we have a better solution.
> >
> > Any further comments here, Akhil?
> >
> > Thanks,
> > Pablo
> >
> [Akhil] Sorry I missed out the previous reply from Sergio.

Any more comments, Sergio?

Pablo
> 
> Thanks,
> Akhil
> >>

Re: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[Sergio Gonzalez Monroy]

On 07/10/2016 21:53, De Lara Guarch, Pablo wrote:
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Tuesday, October 04, 2016 11:33 PM
>> To: De Lara Guarch, Pablo; Gonzalez Monroy, Sergio; dev@dpdk.org
>> Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while
>> decrementing ttl
>>
>> On 10/5/2016 6:04 AM, De Lara Guarch, Pablo wrote:
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio Gonzalez
>>>> Monroy
>>>> Sent: Monday, September 26, 2016 6:28 AM
>>>> To: akhil.goyal@nxp.com; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update checksum
>>>> while decrementing ttl
>>>>
>>>> Hi Akhil,
>>>>
>>>> This application relies on checksum offload in both outbound and
>> inbound
>>>> paths (PKT_TX_IP_CKSUM flag).
>> [Akhil]Agreed that the application relies on checksum offload, but here
>> we are talking about the inner ip header. Inner IP checksum will be
>> updated on the next end point after decryption. This would expect that
>> the next end point must have checksum offload capability. What if we are
>> capturing the encrypted packets on wireshark or say send it to some
>> other machine which does not run DPDK and do not know about checksum
>> offload, then wireshark/other machine will not be able to get the
>> correct the checksum and will show error.

Understood, we need to have a valid inner checksum.
RFC1624 states that the computation would be incorrect in 
corner/boundary case.
I reckon you are basing your incremental update on RFC1141?

Also I think you should take care of endianess and increment the 
checksum with
host_to_be(0x0100) instead of +1.

>>>> Because we assume that we always forward the packet in both paths, we
>>>> decrement the ttl in both inbound and outbound.
>>>> You seem to only increment (recalculate) the checksum of the inner IP
>>>> header in the outbound path but not the inbound path.
>> [Akhil]Correct I missed out the inbound path.
>>>> Also, in the inbound path you have to consider a possible ECN value
>> update.
>> [Akhil]If I take care of the ECN then it would mean I need to calculate
>> the checksum completely, incremental checksum wont give correct results.
>> This would surely impact performance. Any suggestion on how should we
>> take care of ECN update. Should I recalculate the checksum and send the
>> patch for ECN update? Or do we have a better solution.

If I am understanding the RFCs mentioned above correctly, you should be 
able to do
incremental checksum update for any 16bit field/value of the IP header.
I don't see no reason why you couldn't do something like that, except 
that you would
have to follow the full equation instead of just adding 0x0100, which 
would be always
the case when decrementing TTL.

What do you think?

Sergio

>>> Any further comments here, Akhil?
>>>
>>> Thanks,
>>> Pablo
>>>
>> [Akhil] Sorry I missed out the previous reply from Sergio.
> Any more comments, Sergio?
>
> Pablo
>> Thanks,
>> Akhil

Re: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Gonzalez Monroy, Sergio
> Sent: Monday, October 10, 2016 5:05 AM
> To: De Lara Guarch, Pablo; Akhil Goyal; dev@dpdk.org
> Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while
> decrementing ttl
> 
> On 07/10/2016 21:53, De Lara Guarch, Pablo wrote:
> >> -----Original Message-----
> >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> >> Sent: Tuesday, October 04, 2016 11:33 PM
> >> To: De Lara Guarch, Pablo; Gonzalez Monroy, Sergio; dev@dpdk.org
> >> Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while
> >> decrementing ttl
> >>
> >> On 10/5/2016 6:04 AM, De Lara Guarch, Pablo wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio
> Gonzalez
> >>>> Monroy
> >>>> Sent: Monday, September 26, 2016 6:28 AM
> >>>> To: akhil.goyal@nxp.com; dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update
> checksum
> >>>> while decrementing ttl
> >>>>
> >>>> Hi Akhil,
> >>>>
> >>>> This application relies on checksum offload in both outbound and
> >> inbound
> >>>> paths (PKT_TX_IP_CKSUM flag).
> >> [Akhil]Agreed that the application relies on checksum offload, but here
> >> we are talking about the inner ip header. Inner IP checksum will be
> >> updated on the next end point after decryption. This would expect that
> >> the next end point must have checksum offload capability. What if we are
> >> capturing the encrypted packets on wireshark or say send it to some
> >> other machine which does not run DPDK and do not know about
> checksum
> >> offload, then wireshark/other machine will not be able to get the
> >> correct the checksum and will show error.
> 
> Understood, we need to have a valid inner checksum.
> RFC1624 states that the computation would be incorrect in
> corner/boundary case.
> I reckon you are basing your incremental update on RFC1141?
> 
> Also I think you should take care of endianess and increment the
> checksum with
> host_to_be(0x0100) instead of +1.
> 
> >>>> Because we assume that we always forward the packet in both paths,
> we
> >>>> decrement the ttl in both inbound and outbound.
> >>>> You seem to only increment (recalculate) the checksum of the inner IP
> >>>> header in the outbound path but not the inbound path.
> >> [Akhil]Correct I missed out the inbound path.
> >>>> Also, in the inbound path you have to consider a possible ECN value
> >> update.
> >> [Akhil]If I take care of the ECN then it would mean I need to calculate
> >> the checksum completely, incremental checksum wont give correct results.
> >> This would surely impact performance. Any suggestion on how should we
> >> take care of ECN update. Should I recalculate the checksum and send the
> >> patch for ECN update? Or do we have a better solution.
> 
> If I am understanding the RFCs mentioned above correctly, you should be
> able to do
> incremental checksum update for any 16bit field/value of the IP header.
> I don't see no reason why you couldn't do something like that, except
> that you would
> have to follow the full equation instead of just adding 0x0100, which
> would be always
> the case when decrementing TTL.
> 
> What do you think?

Any comments, Akhil?

Re: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[Akhil Goyal]

-----Original Message-----
From: De Lara Guarch, Pablo [mailto:pablo.de.lara.guarch@intel.com] 
Sent: Monday, October 17, 2016 10:35 PM
To: Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; dev@dpdk.org
Subject: RE: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl



> -----Original Message-----
> From: Gonzalez Monroy, Sergio
> Sent: Monday, October 10, 2016 5:05 AM
> To: De Lara Guarch, Pablo; Akhil Goyal; dev@dpdk.org
> Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while 
> decrementing ttl
> 
> On 07/10/2016 21:53, De Lara Guarch, Pablo wrote:
> >> -----Original Message-----
> >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> >> Sent: Tuesday, October 04, 2016 11:33 PM
> >> To: De Lara Guarch, Pablo; Gonzalez Monroy, Sergio; dev@dpdk.org
> >> Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while 
> >> decrementing ttl
> >>
> >> On 10/5/2016 6:04 AM, De Lara Guarch, Pablo wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio
> Gonzalez
> >>>> Monroy
> >>>> Sent: Monday, September 26, 2016 6:28 AM
> >>>> To: akhil.goyal@nxp.com; dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update
> checksum
> >>>> while decrementing ttl
> >>>>
> >>>> Hi Akhil,
> >>>>
> >>>> This application relies on checksum offload in both outbound and
> >> inbound
> >>>> paths (PKT_TX_IP_CKSUM flag).
> >> [Akhil]Agreed that the application relies on checksum offload, but 
> >> here we are talking about the inner ip header. Inner IP checksum 
> >> will be updated on the next end point after decryption. This would 
> >> expect that the next end point must have checksum offload 
> >> capability. What if we are capturing the encrypted packets on 
> >> wireshark or say send it to some other machine which does not run 
> >> DPDK and do not know about
> checksum
> >> offload, then wireshark/other machine will not be able to get the 
> >> correct the checksum and will show error.
> 
> Understood, we need to have a valid inner checksum.
> RFC1624 states that the computation would be incorrect in 
> corner/boundary case.
> I reckon you are basing your incremental update on RFC1141?
> 
> Also I think you should take care of endianess and increment the 
> checksum with
> host_to_be(0x0100) instead of +1.
> 
> >>>> Because we assume that we always forward the packet in both 
> >>>> paths,
> we
> >>>> decrement the ttl in both inbound and outbound.
> >>>> You seem to only increment (recalculate) the checksum of the 
> >>>> inner IP header in the outbound path but not the inbound path.
> >> [Akhil]Correct I missed out the inbound path.
> >>>> Also, in the inbound path you have to consider a possible ECN 
> >>>> value
> >> update.
> >> [Akhil]If I take care of the ECN then it would mean I need to 
> >> calculate the checksum completely, incremental checksum wont give correct results.
> >> This would surely impact performance. Any suggestion on how should 
> >> we take care of ECN update. Should I recalculate the checksum and 
> >> send the patch for ECN update? Or do we have a better solution.
> 
> If I am understanding the RFCs mentioned above correctly, you should 
> be able to do incremental checksum update for any 16bit field/value of 
> the IP header.
> I don't see no reason why you couldn't do something like that, except 
> that you would have to follow the full equation instead of just adding 
> 0x0100, which would be always the case when decrementing TTL.
> 
> What do you think?

Any comments, Akhil?

Ok.. will send next version soon.

Re: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Wednesday, October 19, 2016 1:38 AM
> To: De Lara Guarch, Pablo; Gonzalez Monroy, Sergio; dev@dpdk.org
> Subject: RE: [PATCH] examples/ipsec-secgw: Update checksum while
> decrementing ttl
> 
> 
> 
> -----Original Message-----
> From: De Lara Guarch, Pablo [mailto:pablo.de.lara.guarch@intel.com]
> Sent: Monday, October 17, 2016 10:35 PM
> To: Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>; Akhil
> Goyal <akhil.goyal@nxp.com>; dev@dpdk.org
> Subject: RE: [PATCH] examples/ipsec-secgw: Update checksum while
> decrementing ttl
> 
> 
> 
> > -----Original Message-----
> > From: Gonzalez Monroy, Sergio
> > Sent: Monday, October 10, 2016 5:05 AM
> > To: De Lara Guarch, Pablo; Akhil Goyal; dev@dpdk.org
> > Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while
> > decrementing ttl
> >
> > On 07/10/2016 21:53, De Lara Guarch, Pablo wrote:
> > >> -----Original Message-----
> > >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> > >> Sent: Tuesday, October 04, 2016 11:33 PM
> > >> To: De Lara Guarch, Pablo; Gonzalez Monroy, Sergio; dev@dpdk.org
> > >> Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while
> > >> decrementing ttl
> > >>
> > >> On 10/5/2016 6:04 AM, De Lara Guarch, Pablo wrote:
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio
> > Gonzalez
> > >>>> Monroy
> > >>>> Sent: Monday, September 26, 2016 6:28 AM
> > >>>> To: akhil.goyal@nxp.com; dev@dpdk.org
> > >>>> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update
> > checksum
> > >>>> while decrementing ttl
> > >>>>
> > >>>> Hi Akhil,
> > >>>>
> > >>>> This application relies on checksum offload in both outbound and
> > >> inbound
> > >>>> paths (PKT_TX_IP_CKSUM flag).
> > >> [Akhil]Agreed that the application relies on checksum offload, but
> > >> here we are talking about the inner ip header. Inner IP checksum
> > >> will be updated on the next end point after decryption. This would
> > >> expect that the next end point must have checksum offload
> > >> capability. What if we are capturing the encrypted packets on
> > >> wireshark or say send it to some other machine which does not run
> > >> DPDK and do not know about
> > checksum
> > >> offload, then wireshark/other machine will not be able to get the
> > >> correct the checksum and will show error.
> >
> > Understood, we need to have a valid inner checksum.
> > RFC1624 states that the computation would be incorrect in
> > corner/boundary case.
> > I reckon you are basing your incremental update on RFC1141?
> >
> > Also I think you should take care of endianess and increment the
> > checksum with
> > host_to_be(0x0100) instead of +1.
> >
> > >>>> Because we assume that we always forward the packet in both
> > >>>> paths,
> > we
> > >>>> decrement the ttl in both inbound and outbound.
> > >>>> You seem to only increment (recalculate) the checksum of the
> > >>>> inner IP header in the outbound path but not the inbound path.
> > >> [Akhil]Correct I missed out the inbound path.
> > >>>> Also, in the inbound path you have to consider a possible ECN
> > >>>> value
> > >> update.
> > >> [Akhil]If I take care of the ECN then it would mean I need to
> > >> calculate the checksum completely, incremental checksum wont give
> correct results.
> > >> This would surely impact performance. Any suggestion on how should
> > >> we take care of ECN update. Should I recalculate the checksum and
> > >> send the patch for ECN update? Or do we have a better solution.
> >
> > If I am understanding the RFCs mentioned above correctly, you should
> > be able to do incremental checksum update for any 16bit field/value of
> > the IP header.
> > I don't see no reason why you couldn't do something like that, except
> > that you would have to follow the full equation instead of just adding
> > 0x0100, which would be always the case when decrementing TTL.
> >
> > What do you think?
> 
> Any comments, Akhil?
> 
> Ok.. will send next version soon.

Hi Akhil,
Are you sending that version soon? It won't make it the RC2, but it may be merged for RC3.

Thanks,
Pablo

[PATCH] app/test: Remove hard coding for nb_queue_pairs in test_cryptodev

[akhil.goyal]

From: Akhil Goyal <akhil.goyal@nxp.com>

nb_queue_pairs should not be hard coded with device specific number.
It should be retrieved from the device infos.
Also in ut_setup, ts_params->conf.nb_queue_pairs is already set in
testsuite_setup and we are not modifying it.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
---
 app/test/test_cryptodev.c      | 1 -
 app/test/test_cryptodev_perf.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 647787d..12b6b04 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -338,7 +338,6 @@ ut_setup(void)
 	memset(ut_params, 0, sizeof(*ut_params));
 
 	/* Reconfigure device to default parameters */
-	ts_params->conf.nb_queue_pairs = DEFAULT_NUM_QPS_PER_QAT_DEVICE;
 	ts_params->conf.socket_id = SOCKET_ID_ANY;
 	ts_params->conf.session_mp.nb_objs =
 			(gbl_cryptodev_type == RTE_CRYPTODEV_QAT_SYM_PMD) ?
diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
index 2398d84..0ea7ec1 100644
--- a/app/test/test_cryptodev_perf.c
+++ b/app/test/test_cryptodev_perf.c
@@ -301,7 +301,7 @@ testsuite_setup(void)
 
 	rte_cryptodev_info_get(ts_params->dev_id, &info);
 
-	ts_params->conf.nb_queue_pairs = DEFAULT_NUM_QPS_PER_QAT_DEVICE;
+	ts_params->conf.nb_queue_pairs = info.max_nb_queue_pairs;
 	ts_params->conf.socket_id = SOCKET_ID_ANY;
 	ts_params->conf.session_mp.nb_objs = info.sym.max_nb_sessions;
 
-- 
1.9.1

Re: [PATCH] app/test: Remove hard coding for nb_queue_pairs in test_cryptodev

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> akhil.goyal@nxp.com
> Sent: Monday, September 26, 2016 9:33 AM
> To: dev@dpdk.org
> Cc: Akhil Goyal
> Subject: [dpdk-dev] [PATCH] app/test: Remove hard coding for
> nb_queue_pairs in test_cryptodev
> 
> From: Akhil Goyal <akhil.goyal@nxp.com>
> 
> nb_queue_pairs should not be hard coded with device specific number.
> It should be retrieved from the device infos.
> Also in ut_setup, ts_params->conf.nb_queue_pairs is already set in
> testsuite_setup and we are not modifying it.
> 
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [PATCH] app/test: Remove hard coding for nb_queue_pairs in test_cryptodev

[Trahe, Fiona]

Hi Ahkil

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of De Lara Guarch,
> Pablo
> Sent: Monday, September 26, 2016 8:37 PM
> To: akhil.goyal@nxp.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] app/test: Remove hard coding for
> nb_queue_pairs in test_cryptodev
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> > akhil.goyal@nxp.com
> > Sent: Monday, September 26, 2016 9:33 AM
> > To: dev@dpdk.org
> > Cc: Akhil Goyal
> > Subject: [dpdk-dev] [PATCH] app/test: Remove hard coding for
> > nb_queue_pairs in test_cryptodev
> >
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> >
> > nb_queue_pairs should not be hard coded with device specific number.
> > It should be retrieved from the device infos.
> > Also in ut_setup, ts_params->conf.nb_queue_pairs is already set in
> > testsuite_setup and we are not modifying it.
> >
> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> 
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

The above code is correct, however it exposes a bug in QAT PMD unit tests.
And some cleanup needed for unnecessary qp setup code.
That cleanup then exposed a bug in aesni_mb PMD which prevents re-creating queue pairs of a different size.
 
I have a fix and cleanup patch ready. 
Just not sure how best to push it?
The original patch also needs rebasing, doesn't apply cleanly to the latest dpdk-next-crypto

Pablo should I push all as a reply to the first patch - waiting first for that to be rebased?
Or 
It would save Akhil a rebase and be simpler if I can include the original change in my patch and push all as a v2 superceding the original patch?  Is this possible?
Or 
should I Nack the original patch and push all instead?

Re: [PATCH] app/test: Remove hard coding for nb_queue_pairs in test_cryptodev

[Thomas Monjalon]

2016-09-29 14:12, Trahe, Fiona:
> > > From: Akhil Goyal <akhil.goyal@nxp.com>
> > >
> > > nb_queue_pairs should not be hard coded with device specific number.
> > > It should be retrieved from the device infos.
> > > Also in ut_setup, ts_params->conf.nb_queue_pairs is already set in
> > > testsuite_setup and we are not modifying it.
> > >
> > > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> > 
> > Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> 
> The above code is correct, however it exposes a bug in QAT PMD unit tests.
> And some cleanup needed for unnecessary qp setup code.
> That cleanup then exposed a bug in aesni_mb PMD which prevents re-creating queue pairs of a different size.
>  
> I have a fix and cleanup patch ready. 
> Just not sure how best to push it?
> The original patch also needs rebasing, doesn't apply cleanly to the latest dpdk-next-crypto
> 
> Pablo should I push all as a reply to the first patch - waiting first for that to be rebased?
> Or 
> It would save Akhil a rebase and be simpler if I can include the original change in my patch and push all as a v2 superceding the original patch?  Is this possible?
> Or 
> should I Nack the original patch and push all instead?

My preference goes to a v2.

Re: [PATCH] app/test: Remove hard coding for nb_queue_pairs in test_cryptodev

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, September 29, 2016 7:25 AM
> To: Trahe, Fiona
> Cc: dev@dpdk.org; De Lara Guarch, Pablo; akhil.goyal@nxp.com
> Subject: Re: [dpdk-dev] [PATCH] app/test: Remove hard coding for
> nb_queue_pairs in test_cryptodev
> 
> 2016-09-29 14:12, Trahe, Fiona:
> > > > From: Akhil Goyal <akhil.goyal@nxp.com>
> > > >
> > > > nb_queue_pairs should not be hard coded with device specific number.
> > > > It should be retrieved from the device infos.
> > > > Also in ut_setup, ts_params->conf.nb_queue_pairs is already set in
> > > > testsuite_setup and we are not modifying it.
> > > >
> > > > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> > >
> > > Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> >
> > The above code is correct, however it exposes a bug in QAT PMD unit tests.
> > And some cleanup needed for unnecessary qp setup code.
> > That cleanup then exposed a bug in aesni_mb PMD which prevents re-
> creating queue pairs of a different size.
> >
> > I have a fix and cleanup patch ready.
> > Just not sure how best to push it?
> > The original patch also needs rebasing, doesn't apply cleanly to the latest
> dpdk-next-crypto
> >
> > Pablo should I push all as a reply to the first patch - waiting first for that to
> be rebased?
> > Or
> > It would save Akhil a rebase and be simpler if I can include the original
> change in my patch and push all as a v2 superceding the original patch?  Is
> this possible?
> > Or
> > should I Nack the original patch and push all instead?
> 
> My preference goes to a v2.

Agree, send a v2, including your name and Akhil's. 

Thanks,
Pablo

[PATCH] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[akhil.goyal]

From: Akhil Goyal <akhil.goyal@nxp.com>

For physical crypto devices, IV and digest are processed by the crypto
device which need the contents to be written on some DMA able address.

So in order to do that, IV and digest are accomodated in the packet.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
---
 app/test/test_cryptodev_perf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
index 0ea7ec1..930d5b8 100644
--- a/app/test/test_cryptodev_perf.c
+++ b/app/test/test_cryptodev_perf.c
@@ -2366,9 +2366,13 @@ test_perf_set_crypto_op(struct rte_crypto_op *op, struct rte_mbuf *m,
 	op->sym->auth.aad.length = AES_CBC_CIPHER_IV_LENGTH;
 
 	/* Cipher Parameters */
-	op->sym->cipher.iv.data = aes_cbc_iv;
+	op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off;
+	op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m);
 	op->sym->cipher.iv.length = AES_CBC_CIPHER_IV_LENGTH;
 
+	rte_memcpy(op->sym->cipher.iv.data, aes_cbc_iv,
+			AES_CBC_CIPHER_IV_LENGTH);
+
 	/* Data lengths/offsets Parameters */
 	op->sym->auth.data.offset = 0;
 	op->sym->auth.data.length = data_len;
@@ -2468,7 +2472,9 @@ test_perf_aes_sha(uint8_t dev_id, uint16_t queue_id,
 				rte_pktmbuf_free(mbufs[k]);
 			return -1;
 		}
-
+		/* Make room for Digest and IV in mbuf */
+		rte_pktmbuf_append(mbufs[i], digest_length);
+		rte_pktmbuf_prepend(mbufs[i], AES_CBC_CIPHER_IV_LENGTH);
 	}
 
 
-- 
1.9.1

Re: [PATCH] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[Akhil Goyal]

On 9/26/2016 10:03 PM, akhil.goyal@nxp.com wrote:
> From: Akhil Goyal <akhil.goyal@nxp.com>
>
> For physical crypto devices, IV and digest are processed by the crypto
> device which need the contents to be written on some DMA able address.
>
> So in order to do that, IV and digest are accomodated in the packet.
>
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---
>  app/test/test_cryptodev_perf.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
> index 0ea7ec1..930d5b8 100644
> --- a/app/test/test_cryptodev_perf.c
> +++ b/app/test/test_cryptodev_perf.c
> @@ -2366,9 +2366,13 @@ test_perf_set_crypto_op(struct rte_crypto_op *op, struct rte_mbuf *m,
>  	op->sym->auth.aad.length = AES_CBC_CIPHER_IV_LENGTH;
>
>  	/* Cipher Parameters */
> -	op->sym->cipher.iv.data = aes_cbc_iv;
> +	op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off;
> +	op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m);
>  	op->sym->cipher.iv.length = AES_CBC_CIPHER_IV_LENGTH;
>
> +	rte_memcpy(op->sym->cipher.iv.data, aes_cbc_iv,
> +			AES_CBC_CIPHER_IV_LENGTH);
> +
>  	/* Data lengths/offsets Parameters */
>  	op->sym->auth.data.offset = 0;
>  	op->sym->auth.data.length = data_len;
> @@ -2468,7 +2472,9 @@ test_perf_aes_sha(uint8_t dev_id, uint16_t queue_id,
>  				rte_pktmbuf_free(mbufs[k]);
>  			return -1;
>  		}
> -
> +		/* Make room for Digest and IV in mbuf */
> +		rte_pktmbuf_append(mbufs[i], digest_length);
> +		rte_pktmbuf_prepend(mbufs[i], AES_CBC_CIPHER_IV_LENGTH);
>  	}
>
>
>
Hi Declan,

Sorry I missed out copy your name in the TO list. Do we have some 
comments on this patch.

Regards,
Akhil

Re: [PATCH] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[Kusztal, ArkadiuszX]

Hi Akhil,

Could you rebase it against newest next-crypto subtree, there were changes with function names in the meantime.
And to make it really consistent across all hw tests could you add this change to qat_snow3g too, 
for snow3g I assume aad need to obtain correct physical address too.

Regards,
Arek

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
> Sent: Wednesday, October 05, 2016 7:40 AM
> To: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] test_cryptodev_perf: IV and digest should
> be stored at a DMAeble address
> 
> On 9/26/2016 10:03 PM, akhil.goyal@nxp.com wrote:
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> >
> > For physical crypto devices, IV and digest are processed by the crypto
> > device which need the contents to be written on some DMA able address.
> >
> > So in order to do that, IV and digest are accomodated in the packet.
> >
> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> > ---
> >  app/test/test_cryptodev_perf.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev_perf.c
> > b/app/test/test_cryptodev_perf.c index 0ea7ec1..930d5b8 100644
> > --- a/app/test/test_cryptodev_perf.c
> > +++ b/app/test/test_cryptodev_perf.c
> > @@ -2366,9 +2366,13 @@ test_perf_set_crypto_op(struct rte_crypto_op
> *op, struct rte_mbuf *m,
> >  	op->sym->auth.aad.length = AES_CBC_CIPHER_IV_LENGTH;
> >
> >  	/* Cipher Parameters */
> > -	op->sym->cipher.iv.data = aes_cbc_iv;
> > +	op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off;
> > +	op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m);
> >  	op->sym->cipher.iv.length = AES_CBC_CIPHER_IV_LENGTH;
> >
> > +	rte_memcpy(op->sym->cipher.iv.data, aes_cbc_iv,
> > +			AES_CBC_CIPHER_IV_LENGTH);
> > +
> >  	/* Data lengths/offsets Parameters */
> >  	op->sym->auth.data.offset = 0;
> >  	op->sym->auth.data.length = data_len; @@ -2468,7 +2472,9 @@
> > test_perf_aes_sha(uint8_t dev_id, uint16_t queue_id,
> >  				rte_pktmbuf_free(mbufs[k]);
> >  			return -1;
> >  		}
> > -
> > +		/* Make room for Digest and IV in mbuf */
> > +		rte_pktmbuf_append(mbufs[i], digest_length);
> > +		rte_pktmbuf_prepend(mbufs[i],
> AES_CBC_CIPHER_IV_LENGTH);
> >  	}
> >
> >
> >
> Hi Declan,
> 
> Sorry I missed out copy your name in the TO list. Do we have some
> comments on this patch.
> 
> Regards,
> Akhil

Re: [PATCH] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[Akhil Goyal]

Hi Arek,

Ok. I would rebase the patch.
Regarding changes required to qat_snow3g, I do not have setup to test on qat and the hardware that I test, currently snow3g support is not added. I can send the patches for snow3g at some later stage.

Regards,
Akhil

-----Original Message-----
From: Kusztal, ArkadiuszX [mailto:arkadiuszx.kusztal@intel.com] 
Sent: Wednesday, October 05, 2016 2:57 PM
To: Akhil Goyal <akhil.goyal@nxp.com>; dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>
Cc: Jain, Deepak K <deepak.k.jain@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>; Griffin, John <john.griffin@intel.com>
Subject: RE: [dpdk-dev] [PATCH] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

Hi Akhil,

Could you rebase it against newest next-crypto subtree, there were changes with function names in the meantime.
And to make it really consistent across all hw tests could you add this change to qat_snow3g too, for snow3g I assume aad need to obtain correct physical address too.

Regards,
Arek

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
> Sent: Wednesday, October 05, 2016 7:40 AM
> To: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] test_cryptodev_perf: IV and digest 
> should be stored at a DMAeble address
> 
> On 9/26/2016 10:03 PM, akhil.goyal@nxp.com wrote:
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> >
> > For physical crypto devices, IV and digest are processed by the 
> > crypto device which need the contents to be written on some DMA able address.
> >
> > So in order to do that, IV and digest are accomodated in the packet.
> >
> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> > ---
> >  app/test/test_cryptodev_perf.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev_perf.c 
> > b/app/test/test_cryptodev_perf.c index 0ea7ec1..930d5b8 100644
> > --- a/app/test/test_cryptodev_perf.c
> > +++ b/app/test/test_cryptodev_perf.c
> > @@ -2366,9 +2366,13 @@ test_perf_set_crypto_op(struct rte_crypto_op
> *op, struct rte_mbuf *m,
> >  	op->sym->auth.aad.length = AES_CBC_CIPHER_IV_LENGTH;
> >
> >  	/* Cipher Parameters */
> > -	op->sym->cipher.iv.data = aes_cbc_iv;
> > +	op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off;
> > +	op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m);
> >  	op->sym->cipher.iv.length = AES_CBC_CIPHER_IV_LENGTH;
> >
> > +	rte_memcpy(op->sym->cipher.iv.data, aes_cbc_iv,
> > +			AES_CBC_CIPHER_IV_LENGTH);
> > +
> >  	/* Data lengths/offsets Parameters */
> >  	op->sym->auth.data.offset = 0;
> >  	op->sym->auth.data.length = data_len; @@ -2468,7 +2472,9 @@ 
> > test_perf_aes_sha(uint8_t dev_id, uint16_t queue_id,
> >  				rte_pktmbuf_free(mbufs[k]);
> >  			return -1;
> >  		}
> > -
> > +		/* Make room for Digest and IV in mbuf */
> > +		rte_pktmbuf_append(mbufs[i], digest_length);
> > +		rte_pktmbuf_prepend(mbufs[i],
> AES_CBC_CIPHER_IV_LENGTH);
> >  	}
> >
> >
> >
> Hi Declan,
> 
> Sorry I missed out copy your name in the TO list. Do we have some 
> comments on this patch.
> 
> Regards,
> Akhil

[PATCH v2] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[akhil.goyal]

From: Akhil Goyal <akhil.goyal@nxp.com>

For physical crypto devices, IV and digest are processed by the crypto
device which need the contents to be written on some DMA able address.

So in order to do that, IV and digest are accomodated in the packet.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
v2: patch rebased
---
 app/test/test_cryptodev_perf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
index 4aee9af..08eda81 100644
--- a/app/test/test_cryptodev_perf.c
+++ b/app/test/test_cryptodev_perf.c
@@ -2722,9 +2722,12 @@ test_perf_set_crypto_op_aes(struct rte_crypto_op *op, struct rte_mbuf *m,
 	op->sym->auth.aad.length = AES_CIPHER_IV_LENGTH;
 
 	/* Cipher Parameters */
-	op->sym->cipher.iv.data = aes_iv;
+	op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off;
+	op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m);
 	op->sym->cipher.iv.length = AES_CIPHER_IV_LENGTH;
 
+	rte_memcpy(op->sym->cipher.iv.data, aes_iv, AES_CIPHER_IV_LENGTH);
+
 	/* Data lengths/offsets Parameters */
 	op->sym->auth.data.offset = 0;
 	op->sym->auth.data.length = data_len;
@@ -2893,7 +2896,9 @@ test_perf_aes_sha(uint8_t dev_id, uint16_t queue_id,
 				rte_pktmbuf_free(mbufs[k]);
 			return -1;
 		}
-
+		/* Make room for Digest and IV in mbuf */
+		rte_pktmbuf_append(mbufs[i], digest_length);
+		rte_pktmbuf_prepend(mbufs[i], AES_CBC_CIPHER_IV_LENGTH);
 	}
 
 
-- 
2.9.3

Re: [PATCH v2] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[De Lara Guarch, Pablo]

Hi Akhil,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> akhil.goyal@nxp.com
> Sent: Friday, October 07, 2016 10:06 AM
> To: Kusztal, ArkadiuszX; Doherty, Declan
> Cc: Griffin, John; Trahe, Fiona; Jain, Deepak K; dev@dpdk.org; Akhil Goyal
> Subject: [dpdk-dev] [PATCH v2] test_cryptodev_perf: IV and digest should be
> stored at a DMAeble address
> 
> From: Akhil Goyal <akhil.goyal@nxp.com>
> 
> For physical crypto devices, IV and digest are processed by the crypto
> device which need the contents to be written on some DMA able address.
> 
> So in order to do that, IV and digest are accomodated in the packet.
> 
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> v2: patch rebased

You need to rebase against the HEAD of the dpdk-next-crypto subtree:
(http://dpdk.org/browse/next/dpdk-next-crypto/).

Thanks!
Pablo

Re: [PATCH v2] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[Akhil Goyal]

On 10/8/2016 3:06 AM, De Lara Guarch, Pablo wrote:
> Hi Akhil,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
>> akhil.goyal@nxp.com
>> Sent: Friday, October 07, 2016 10:06 AM
>> To: Kusztal, ArkadiuszX; Doherty, Declan
>> Cc: Griffin, John; Trahe, Fiona; Jain, Deepak K; dev@dpdk.org; Akhil Goyal
>> Subject: [dpdk-dev] [PATCH v2] test_cryptodev_perf: IV and digest should be
>> stored at a DMAeble address
>>
>> From: Akhil Goyal <akhil.goyal@nxp.com>
>>
>> For physical crypto devices, IV and digest are processed by the crypto
>> device which need the contents to be written on some DMA able address.
>>
>> So in order to do that, IV and digest are accomodated in the packet.
>>
>> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
>> v2: patch rebased
>
> You need to rebase against the HEAD of the dpdk-next-crypto subtree:
> (http://dpdk.org/browse/next/dpdk-next-crypto/).
>
> Thanks!
> Pablo
>
>
>
>
>
Hi Pablo,

I have already rebased this patch to dpdk-next-crypto subtree. Please 
let me know if there is any issue.
Here is my git log
779f1301a382b6b9e2877fd0357bba33d1242d65 test_cryptodev_perf: IV and 
digest should be stored at a DMAeble address
8c9fdf4568768b7765fc2176e400a860dc758020 app/test: remove hard-coding of 
crypto num qps
c1876c1cb90f0882ada0acd9e430be7cf63bc765 app/test: cleanup unnecessary 
ring size setup
136592c3a350ded56438b59cc4921a243f08e1d0 app/test: remove pointless for loop
fca4f966b42adc0c8f3e1d43a94ddddd93ea4fcb crypto/aesni_mb: free ring 
memory on qp release in PMD


Regards

Re: [PATCH v2] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[De Lara Guarch, Pablo]

Hi Akhil,

> -----Original Message-----

[...]

> Hi Pablo,
> 
> I have already rebased this patch to dpdk-next-crypto subtree. Please
> let me know if there is any issue.
> Here is my git log
> 779f1301a382b6b9e2877fd0357bba33d1242d65 test_cryptodev_perf: IV and
> digest should be stored at a DMAeble address
> 8c9fdf4568768b7765fc2176e400a860dc758020 app/test: remove hard-coding
> of
> crypto num qps
> c1876c1cb90f0882ada0acd9e430be7cf63bc765 app/test: cleanup
> unnecessary
> ring size setup
> 136592c3a350ded56438b59cc4921a243f08e1d0 app/test: remove pointless
> for loop
> fca4f966b42adc0c8f3e1d43a94ddddd93ea4fcb crypto/aesni_mb: free ring
> memory on qp release in PMD
> 
> 
> Regards

The problem is that AES_CBC_CIPHER_IV_LENGTH does not exist anymore, so compilation is broken.
Also, you could add this change for another tests, like what Arek suggested (for SNOW3G),
or someone can send a new version of this patch, if you prefer that.

Thanks,
Pablo

[PATCH v3] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[akhil.goyal]

From: Akhil Goyal <akhil.goyal@nxp.com>

For physical crypto devices, IV and digest are processed by the crypto
device which need the contents to be written on some DMA able address.

So in order to do that, IV and digest are accomodated in the packet.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
v2: patch rebased
v3: fix broken compilation
---
 app/test/test_cryptodev_perf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
index 4aee9af..d498195 100644
--- a/app/test/test_cryptodev_perf.c
+++ b/app/test/test_cryptodev_perf.c
@@ -2722,9 +2722,12 @@ test_perf_set_crypto_op_aes(struct rte_crypto_op *op, struct rte_mbuf *m,
 	op->sym->auth.aad.length = AES_CIPHER_IV_LENGTH;
 
 	/* Cipher Parameters */
-	op->sym->cipher.iv.data = aes_iv;
+	op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off;
+	op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m);
 	op->sym->cipher.iv.length = AES_CIPHER_IV_LENGTH;
 
+	rte_memcpy(op->sym->cipher.iv.data, aes_iv, AES_CIPHER_IV_LENGTH);
+
 	/* Data lengths/offsets Parameters */
 	op->sym->auth.data.offset = 0;
 	op->sym->auth.data.length = data_len;
@@ -2893,7 +2896,9 @@ test_perf_aes_sha(uint8_t dev_id, uint16_t queue_id,
 				rte_pktmbuf_free(mbufs[k]);
 			return -1;
 		}
-
+		/* Make room for Digest and IV in mbuf */
+		rte_pktmbuf_append(mbufs[i], digest_length);
+		rte_pktmbuf_prepend(mbufs[i], AES_CIPHER_IV_LENGTH);
 	}
 
 
-- 
2.9.3

Re: [PATCH v3] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[Trahe, Fiona]

> -----Original Message-----
> From: akhil.goyal@nxp.com [mailto:akhil.goyal@nxp.com]
> Sent: Wednesday, October 12, 2016 12:16 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>
> Cc: Griffin, John <john.griffin@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>;
> dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>
> Subject: [PATCH v3] test_cryptodev_perf: IV and digest should be stored at a
> DMAeble address
> 
> From: Akhil Goyal <akhil.goyal@nxp.com>
> 
> For physical crypto devices, IV and digest are processed by the crypto device
> which need the contents to be written on some DMA able address.
> 
> So in order to do that, IV and digest are accomodated in the packet.
> 
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> v2: patch rebased
> v3: fix broken compilation
> ---
>  app/test/test_cryptodev_perf.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
> index 4aee9af..d498195 100644
> --- a/app/test/test_cryptodev_perf.c
> +++ b/app/test/test_cryptodev_perf.c
> @@ -2722,9 +2722,12 @@ test_perf_set_crypto_op_aes(struct
> rte_crypto_op *op, struct rte_mbuf *m,
>  	op->sym->auth.aad.length = AES_CIPHER_IV_LENGTH;
> 
>  	/* Cipher Parameters */
> -	op->sym->cipher.iv.data = aes_iv;
> +	op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off;
> +	op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m);
>  	op->sym->cipher.iv.length = AES_CIPHER_IV_LENGTH;
> 
> +	rte_memcpy(op->sym->cipher.iv.data, aes_iv,
> AES_CIPHER_IV_LENGTH);
> +
>  	/* Data lengths/offsets Parameters */
>  	op->sym->auth.data.offset = 0;
>  	op->sym->auth.data.length = data_len;
> @@ -2893,7 +2896,9 @@ test_perf_aes_sha(uint8_t dev_id, uint16_t
> queue_id,
>  				rte_pktmbuf_free(mbufs[k]);
>  			return -1;
>  		}
> -
> +		/* Make room for Digest and IV in mbuf */
> +		rte_pktmbuf_append(mbufs[i], digest_length);
> +		rte_pktmbuf_prepend(mbufs[i], AES_CIPHER_IV_LENGTH);
>  	}
> 
> 
> --
> 2.9.3
Acked-by: Fiona Trahe <fiona.trahe@intel.com>

Re: [PATCH v3] test_cryptodev_perf: IV and digest should be stored at a DMAeble address

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Trahe, Fiona
> Sent: Wednesday, October 12, 2016 11:26 AM
> To: akhil.goyal@nxp.com; Kusztal, ArkadiuszX; Doherty, Declan
> Cc: Griffin, John; Jain, Deepak K; dev@dpdk.org; Trahe, Fiona
> Subject: Re: [dpdk-dev] [PATCH v3] test_cryptodev_perf: IV and digest should
> be stored at a DMAeble address
> 
> 
> 
> > -----Original Message-----
> > From: akhil.goyal@nxp.com [mailto:akhil.goyal@nxp.com]
> > Sent: Wednesday, October 12, 2016 12:16 PM
> > To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Doherty, Declan
> > <declan.doherty@intel.com>
> > Cc: Griffin, John <john.griffin@intel.com>; Trahe, Fiona
> > <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>;
> > dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>
> > Subject: [PATCH v3] test_cryptodev_perf: IV and digest should be stored at a
> > DMAeble address
> >
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> >
> > For physical crypto devices, IV and digest are processed by the crypto device
> > which need the contents to be written on some DMA able address.
> >
> > So in order to do that, IV and digest are accomodated in the packet.
> >
> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> Acked-by: Fiona Trahe <fiona.trahe@intel.com>

Applied to dpdk-next-crypto.
Thanks,

Pablo

[PATCH v2] app/test: remove hard-coding of crypto num qps

[Fiona Trahe]

ts_params->conf.nb_queue_pairs should not be hard coded with device
specific number. It should be retrieved from the device info.
Any test which changes it should restore it to orig value.

Also related cleanup of test code setting number and size of
queue-pairs on a device, e.g.
* Removed irrelevant “for” loop – was hardcoded to only loop once.
* Removed obsolete comment re inability to free and re-allocate queu memory
  and obsolete workaround for it which used to create maximum size queues.

And added freeing of ring memory on queue-pair release in aesni_mb PMD, 
else releasing and setting up queue-pair of a different size fails.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---

v2:
  Fix for broken QAT PMD unit tests exposed by v1
  i.e. In test_device_configure_invalid_queue_pair_ids() after running tests 
  for invalid values restore original nb_queue_pairs.
  Also cleanup of test code setting number and size of queue-pairs on a device
  Also fix for aesni_mb PMD not freeing ring memory on qp release

 app/test/test_cryptodev.c                      | 54 ++++++++++----------------
 app/test/test_cryptodev_perf.c                 | 19 +--------
 drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c | 10 ++++-
 3 files changed, 30 insertions(+), 53 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index d960d72..cbcfb3f 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -307,37 +307,27 @@ testsuite_setup(void)
 		return TEST_FAILED;
 
 	/* Set up all the qps on the first of the valid devices found */
-	for (i = 0; i < 1; i++) {
-		dev_id = ts_params->valid_devs[i];
+	dev_id = ts_params->valid_devs[0];
 
-		rte_cryptodev_info_get(dev_id, &info);
+	rte_cryptodev_info_get(dev_id, &info);
 
-		/*
-		 * Since we can't free and re-allocate queue memory always set
-		 * the queues on this device up to max size first so enough
-		 * memory is allocated for any later re-configures needed by
-		 * other tests
-		 */
-
-		ts_params->conf.nb_queue_pairs = info.max_nb_queue_pairs;
-		ts_params->conf.socket_id = SOCKET_ID_ANY;
-		ts_params->conf.session_mp.nb_objs = info.sym.max_nb_sessions;
+	ts_params->conf.nb_queue_pairs = info.max_nb_queue_pairs;
+	ts_params->conf.socket_id = SOCKET_ID_ANY;
+	ts_params->conf.session_mp.nb_objs = info.sym.max_nb_sessions;
 
-		TEST_ASSERT_SUCCESS(rte_cryptodev_configure(dev_id,
-				&ts_params->conf),
-				"Failed to configure cryptodev %u with %u qps",
-				dev_id, ts_params->conf.nb_queue_pairs);
+	TEST_ASSERT_SUCCESS(rte_cryptodev_configure(dev_id,
+			&ts_params->conf),
+			"Failed to configure cryptodev %u with %u qps",
+			dev_id, ts_params->conf.nb_queue_pairs);
 
-		ts_params->qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT;
+	ts_params->qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
 
-		for (qp_id = 0; qp_id < info.max_nb_queue_pairs; qp_id++) {
-			TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
-					dev_id, qp_id, &ts_params->qp_conf,
-					rte_cryptodev_socket_id(dev_id)),
-					"Failed to setup queue pair %u on "
-					"cryptodev %u",
-					qp_id, dev_id);
-		}
+	for (qp_id = 0; qp_id < info.max_nb_queue_pairs; qp_id++) {
+		TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
+			dev_id, qp_id, &ts_params->qp_conf,
+			rte_cryptodev_socket_id(dev_id)),
+			"Failed to setup queue pair %u on cryptodev %u",
+			qp_id, dev_id);
 	}
 
 	return TEST_SUCCESS;
@@ -372,7 +362,6 @@ ut_setup(void)
 	memset(ut_params, 0, sizeof(*ut_params));
 
 	/* Reconfigure device to default parameters */
-	ts_params->conf.nb_queue_pairs = DEFAULT_NUM_QPS_PER_QAT_DEVICE;
 	ts_params->conf.socket_id = SOCKET_ID_ANY;
 	ts_params->conf.session_mp.nb_objs = DEFAULT_NUM_OPS_INFLIGHT;
 
@@ -381,12 +370,6 @@ ut_setup(void)
 			"Failed to configure cryptodev %u",
 			ts_params->valid_devs[0]);
 
-	/*
-	 * Now reconfigure queues to size we actually want to use in this
-	 * test suite.
-	 */
-	ts_params->qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
-
 	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs ; qp_id++) {
 		TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
 			ts_params->valid_devs[0], qp_id,
@@ -396,7 +379,6 @@ ut_setup(void)
 			qp_id, ts_params->valid_devs[0]);
 	}
 
-
 	rte_cryptodev_stats_reset(ts_params->valid_devs[0]);
 
 	/* Start the device */
@@ -490,6 +472,7 @@ static int
 test_device_configure_invalid_queue_pair_ids(void)
 {
 	struct crypto_testsuite_params *ts_params = &testsuite_params;
+	uint16_t orig_nb_qps = ts_params->conf.nb_queue_pairs;
 
 	/* Stop the device in case it's started so it can be configured */
 	rte_cryptodev_stop(ts_params->valid_devs[0]);
@@ -544,6 +527,9 @@ test_device_configure_invalid_queue_pair_ids(void)
 			ts_params->valid_devs[0],
 			ts_params->conf.nb_queue_pairs);
 
+	/* revert to original testsuite value */
+	ts_params->conf.nb_queue_pairs = orig_nb_qps;
+
 	return TEST_SUCCESS;
 }
 
diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
index 6af0896..323995e 100644
--- a/app/test/test_cryptodev_perf.c
+++ b/app/test/test_cryptodev_perf.c
@@ -386,14 +386,12 @@ testsuite_setup(void)
 
 	/*
 	 * Using Crypto Device Id 0 by default.
-	 * Since we can't free and re-allocate queue memory always set the queues
-	 * on this device up to max size first so enough memory is allocated for
-	 * any later re-configures needed by other tests
+	 * Set up all the qps on this device.
 	 */
 
 	rte_cryptodev_info_get(ts_params->dev_id, &info);
 
-	ts_params->conf.nb_queue_pairs = DEFAULT_NUM_QPS_PER_QAT_DEVICE;
+	ts_params->conf.nb_queue_pairs = info.max_nb_queue_pairs;
 	ts_params->conf.socket_id = SOCKET_ID_ANY;
 	ts_params->conf.session_mp.nb_objs = info.sym.max_nb_sessions;
 
@@ -402,19 +400,6 @@ testsuite_setup(void)
 			"Failed to configure cryptodev %u",
 			ts_params->dev_id);
 
-
-	ts_params->qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT;
-
-	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs ; qp_id++) {
-		TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
-			ts_params->dev_id, qp_id,
-			&ts_params->qp_conf,
-			rte_cryptodev_socket_id(ts_params->dev_id)),
-			"Failed to setup queue pair %u on cryptodev %u",
-			qp_id, ts_params->dev_id);
-	}
-
-	/*Now reconfigure queues to size we actually want to use in this testsuite.*/
 	ts_params->qp_conf.nb_descriptors = PERF_NUM_OPS_INFLIGHT;
 	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs ; qp_id++) {
 
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
index d3c46ac..3d49e2a 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
@@ -311,8 +311,14 @@ aesni_mb_pmd_info_get(struct rte_cryptodev *dev,
 static int
 aesni_mb_pmd_qp_release(struct rte_cryptodev *dev, uint16_t qp_id)
 {
-	if (dev->data->queue_pairs[qp_id] != NULL) {
-		rte_free(dev->data->queue_pairs[qp_id]);
+	struct aesni_mb_qp *qp = dev->data->queue_pairs[qp_id];
+	struct rte_ring *r = NULL;
+
+	if (qp != NULL) {
+		r = rte_ring_lookup(qp->name);
+		if (r)
+			rte_ring_free(r);
+		rte_free(qp);
 		dev->data->queue_pairs[qp_id] = NULL;
 	}
 	return 0;
-- 
2.5.0

Re: [PATCH v2] app/test: remove hard-coding of crypto num qps

[De Lara Guarch, Pablo]

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Fiona Trahe
> Sent: Thursday, September 29, 2016 10:18 AM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo; Trahe, Fiona; Akhil Goyal
> Subject: [dpdk-dev] [PATCH v2] app/test: remove hard-coding of crypto num
> qps
> 
> ts_params->conf.nb_queue_pairs should not be hard coded with device
> specific number. It should be retrieved from the device info.
> Any test which changes it should restore it to orig value.
> 
> Also related cleanup of test code setting number and size of
> queue-pairs on a device, e.g.
> * Removed irrelevant “for” loop – was hardcoded to only loop once.
> * Removed obsolete comment re inability to free and re-allocate queu
> memory
>   and obsolete workaround for it which used to create maximum size queues.
> 
> And added freeing of ring memory on queue-pair release in aesni_mb PMD,
> else releasing and setting up queue-pair of a different size fails.
> 
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
> 
> v2:
>   Fix for broken QAT PMD unit tests exposed by v1
>   i.e. In test_device_configure_invalid_queue_pair_ids() after running tests
>   for invalid values restore original nb_queue_pairs.
>   Also cleanup of test code setting number and size of queue-pairs on a device
>   Also fix for aesni_mb PMD not freeing ring memory on qp release

Sorry, I missed this patch. Could you split this patch into different patches?
It looks like you are making (three?) changes in different places.

Thanks,
Pablo

Re: [PATCH v2] app/test: remove hard-coding of crypto num qps

[Trahe, Fiona]

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Wednesday, October 5, 2016 1:50 AM
> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>
> Subject: RE: [dpdk-dev] [PATCH v2] app/test: remove hard-coding of crypto
> num qps
> 
> Hi,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Fiona Trahe
> > Sent: Thursday, September 29, 2016 10:18 AM
> > To: dev@dpdk.org
> > Cc: De Lara Guarch, Pablo; Trahe, Fiona; Akhil Goyal
> > Subject: [dpdk-dev] [PATCH v2] app/test: remove hard-coding of crypto
> > num qps
> >
> > ts_params->conf.nb_queue_pairs should not be hard coded with device
> > specific number. It should be retrieved from the device info.
> > Any test which changes it should restore it to orig value.
> >
> > Also related cleanup of test code setting number and size of
> > queue-pairs on a device, e.g.
> > * Removed irrelevant “for” loop – was hardcoded to only loop once.
> > * Removed obsolete comment re inability to free and re-allocate queu
> > memory
> >   and obsolete workaround for it which used to create maximum size queues.
> >
> > And added freeing of ring memory on queue-pair release in aesni_mb
> > PMD, else releasing and setting up queue-pair of a different size fails.
> >
> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> > Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> > ---
> >
> > v2:
> >   Fix for broken QAT PMD unit tests exposed by v1
> >   i.e. In test_device_configure_invalid_queue_pair_ids() after running tests
> >   for invalid values restore original nb_queue_pairs.
> >   Also cleanup of test code setting number and size of queue-pairs on a
> device
> >   Also fix for aesni_mb PMD not freeing ring memory on qp release
> 
> Sorry, I missed this patch. Could you split this patch into different patches?
> It looks like you are making (three?) changes in different places.
> 
> Thanks,
> Pablo

Ok, will do,
Fiona

[PATCH v3 0/4] remove hard-coding of crypto num qps and cleanup

[Fiona Trahe]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]


ts_params->conf.nb_queue_pairs should not be hard coded with device
specific number. It should be retrieved from the device info.
Any test which changes it should restore it to orig value.

Also related cleanup of test code setting number and size of
queue-pairs on a device, e.g.
* Removed irrelevant “for” loop – was hardcoded to only loop once.
* Removed obsolete comment re inability to free and re-allocate queu memory
  and obsolete workaround for it which used to create maximum size queues.

And added freeing of ring memory on queue-pair release in aesni_mb PMD, 
else releasing and setting up queue-pair of a different size fails.

v3:
  separate out into 4 patches

v2:
  Fix for broken QAT PMD unit tests exposed by v1
  i.e. In test_device_configure_invalid_queue_pair_ids() after running tests
  for invalid values restore original nb_queue_pairs.
  Also cleanup of test code setting number and size of queue-pairs on a device
  Also fix for aesni_mb PMD not freeing ring memory on qp release


Fiona Trahe (4):
  crypto/aesni_mb: free ring memory on qp release in PMD
  app/test: remove pointless for loop
  app/test: cleanup unnecessary ring size setup
  app/test: remove hard-coding of crypto num qps
Akhil Goyal (1):
  app/test: remove hard-coding of crypto num qps

 app/test/test_cryptodev.c                      | 53 ++++++++++----------------
 app/test/test_cryptodev_perf.c                 | 19 +--------
 drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c | 10 ++++-
 3 files changed, 31 insertions(+), 51 deletions(-)

-- 
2.5.0

Re: [PATCH v3 0/4] remove hard-coding of crypto num qps and cleanup

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Fiona Trahe
> Sent: Thursday, October 06, 2016 10:34 AM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo; Trahe, Fiona; akhil.goyal@nxp.com
> Subject: [dpdk-dev] [PATCH v3 0/4] remove hard-coding of crypto num qps
> and cleanup
> 
> 
> ts_params->conf.nb_queue_pairs should not be hard coded with device
> specific number. It should be retrieved from the device info.
> Any test which changes it should restore it to orig value.
> 
> Also related cleanup of test code setting number and size of
> queue-pairs on a device, e.g.
> * Removed irrelevant “for” loop – was hardcoded to only loop once.
> * Removed obsolete comment re inability to free and re-allocate queu
> memory
>   and obsolete workaround for it which used to create maximum size queues.
> 
> And added freeing of ring memory on queue-pair release in aesni_mb PMD,
> else releasing and setting up queue-pair of a different size fails.
> 
> v3:
>   separate out into 4 patches
> 
> v2:
>   Fix for broken QAT PMD unit tests exposed by v1
>   i.e. In test_device_configure_invalid_queue_pair_ids() after running tests
>   for invalid values restore original nb_queue_pairs.
>   Also cleanup of test code setting number and size of queue-pairs on a device
>   Also fix for aesni_mb PMD not freeing ring memory on qp release
> 
> 
> Fiona Trahe (4):
>   crypto/aesni_mb: free ring memory on qp release in PMD
>   app/test: remove pointless for loop
>   app/test: cleanup unnecessary ring size setup
>   app/test: remove hard-coding of crypto num qps
> Akhil Goyal (1):
>   app/test: remove hard-coding of crypto num qps
> 
>  app/test/test_cryptodev.c                      | 53 ++++++++++----------------
>  app/test/test_cryptodev_perf.c                 | 19 +--------
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c | 10 ++++-
>  3 files changed, 31 insertions(+), 51 deletions(-)
> 
> --
> 2.5.0

Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [PATCH v3 0/4] remove hard-coding of crypto num qps and cleanup

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of De Lara Guarch,
> Pablo
> Sent: Thursday, October 06, 2016 5:30 PM
> To: Trahe, Fiona; dev@dpdk.org
> Cc: Trahe, Fiona; akhil.goyal@nxp.com
> Subject: Re: [dpdk-dev] [PATCH v3 0/4] remove hard-coding of crypto num
> qps and cleanup
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Fiona Trahe
> > Sent: Thursday, October 06, 2016 10:34 AM
> > To: dev@dpdk.org
> > Cc: De Lara Guarch, Pablo; Trahe, Fiona; akhil.goyal@nxp.com
> > Subject: [dpdk-dev] [PATCH v3 0/4] remove hard-coding of crypto num qps
> > and cleanup
> >
> >
> > ts_params->conf.nb_queue_pairs should not be hard coded with device
> > specific number. It should be retrieved from the device info.
> > Any test which changes it should restore it to orig value.
> >
> > Also related cleanup of test code setting number and size of
> > queue-pairs on a device, e.g.
> > * Removed irrelevant “for” loop – was hardcoded to only loop once.
> > * Removed obsolete comment re inability to free and re-allocate queu
> > memory
> >   and obsolete workaround for it which used to create maximum size
> queues.
> >
> > And added freeing of ring memory on queue-pair release in aesni_mb PMD,
> > else releasing and setting up queue-pair of a different size fails.
> >
> > v3:
> >   separate out into 4 patches
> >
> > v2:
> >   Fix for broken QAT PMD unit tests exposed by v1
> >   i.e. In test_device_configure_invalid_queue_pair_ids() after running tests
> >   for invalid values restore original nb_queue_pairs.
> >   Also cleanup of test code setting number and size of queue-pairs on a
> device
> >   Also fix for aesni_mb PMD not freeing ring memory on qp release
> >
> >
> > Fiona Trahe (4):
> >   crypto/aesni_mb: free ring memory on qp release in PMD
> >   app/test: remove pointless for loop
> >   app/test: cleanup unnecessary ring size setup
> >   app/test: remove hard-coding of crypto num qps
> > Akhil Goyal (1):
> >   app/test: remove hard-coding of crypto num qps
> >
> >  app/test/test_cryptodev.c                      | 53 ++++++++++----------------
> >  app/test/test_cryptodev_perf.c                 | 19 +--------
> >  drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c | 10 ++++-
> >  3 files changed, 31 insertions(+), 51 deletions(-)
> >
> > --
> > 2.5.0
> 
> Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Applied to dpdk-next-crypto.
Thanks,

Pablo

[PATCH v3 1/4] crypto/aesni_mb: free ring memory on qp release in PMD

[Fiona Trahe]

Free ring memory on queue_pair release, else
releasing and setting up queue-pair of a different size fails.

Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
index d3c46ac..3d49e2a 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
@@ -311,8 +311,14 @@ aesni_mb_pmd_info_get(struct rte_cryptodev *dev,
 static int
 aesni_mb_pmd_qp_release(struct rte_cryptodev *dev, uint16_t qp_id)
 {
-	if (dev->data->queue_pairs[qp_id] != NULL) {
-		rte_free(dev->data->queue_pairs[qp_id]);
+	struct aesni_mb_qp *qp = dev->data->queue_pairs[qp_id];
+	struct rte_ring *r = NULL;
+
+	if (qp != NULL) {
+		r = rte_ring_lookup(qp->name);
+		if (r)
+			rte_ring_free(r);
+		rte_free(qp);
 		dev->data->queue_pairs[qp_id] = NULL;
 	}
 	return 0;
-- 
2.5.0

[PATCH v3 2/4] app/test: remove pointless for loop

[Fiona Trahe]

Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
 app/test/test_cryptodev.c | 49 +++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 8f40dea..db2f23c 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -357,37 +357,35 @@ testsuite_setup(void)
 		return TEST_FAILED;
 
 	/* Set up all the qps on the first of the valid devices found */
-	for (i = 0; i < 1; i++) {
-		dev_id = ts_params->valid_devs[i];
 
-		rte_cryptodev_info_get(dev_id, &info);
+	dev_id = ts_params->valid_devs[0];
 
-		/*
-		 * Since we can't free and re-allocate queue memory always set
-		 * the queues on this device up to max size first so enough
-		 * memory is allocated for any later re-configures needed by
-		 * other tests
-		 */
+	rte_cryptodev_info_get(dev_id, &info);
 
-		ts_params->conf.nb_queue_pairs = info.max_nb_queue_pairs;
-		ts_params->conf.socket_id = SOCKET_ID_ANY;
-		ts_params->conf.session_mp.nb_objs = info.sym.max_nb_sessions;
+	/*
+	 * Since we can't free and re-allocate queue memory always set
+	 * the queues on this device up to max size first so enough
+	 * memory is allocated for any later re-configures needed by
+	 * other tests
+	 */
 
-		TEST_ASSERT_SUCCESS(rte_cryptodev_configure(dev_id,
-				&ts_params->conf),
-				"Failed to configure cryptodev %u with %u qps",
-				dev_id, ts_params->conf.nb_queue_pairs);
+	ts_params->conf.nb_queue_pairs = info.max_nb_queue_pairs;
+	ts_params->conf.socket_id = SOCKET_ID_ANY;
+	ts_params->conf.session_mp.nb_objs = info.sym.max_nb_sessions;
 
-		ts_params->qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT;
+	TEST_ASSERT_SUCCESS(rte_cryptodev_configure(dev_id,
+			&ts_params->conf),
+			"Failed to configure cryptodev %u with %u qps",
+			dev_id, ts_params->conf.nb_queue_pairs);
 
-		for (qp_id = 0; qp_id < info.max_nb_queue_pairs; qp_id++) {
-			TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
-					dev_id, qp_id, &ts_params->qp_conf,
-					rte_cryptodev_socket_id(dev_id)),
-					"Failed to setup queue pair %u on "
-					"cryptodev %u",
-					qp_id, dev_id);
-		}
+	ts_params->qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT;
+
+	for (qp_id = 0; qp_id < info.max_nb_queue_pairs; qp_id++) {
+		TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
+			dev_id, qp_id, &ts_params->qp_conf,
+			rte_cryptodev_socket_id(dev_id)),
+			"Failed to setup queue pair %u on cryptodev %u",
+			qp_id, dev_id);
 	}
 
 	return TEST_SUCCESS;
-- 
2.5.0

[PATCH v3 3/4] app/test: cleanup unnecessary ring size setup

[Fiona Trahe]

Removed obsolete comments re inability to free and re-allocate
queue memory and obsolete workaround for it
which used to create maximum size queues first, then later
create smaller queues.

Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
 app/test/test_cryptodev.c      | 15 +--------------
 app/test/test_cryptodev_perf.c | 17 +----------------
 2 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index db2f23c..e0b0252 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -362,13 +362,6 @@ testsuite_setup(void)
 
 	rte_cryptodev_info_get(dev_id, &info);
 
-	/*
-	 * Since we can't free and re-allocate queue memory always set
-	 * the queues on this device up to max size first so enough
-	 * memory is allocated for any later re-configures needed by
-	 * other tests
-	 */
-
 	ts_params->conf.nb_queue_pairs = info.max_nb_queue_pairs;
 	ts_params->conf.socket_id = SOCKET_ID_ANY;
 	ts_params->conf.session_mp.nb_objs = info.sym.max_nb_sessions;
@@ -378,7 +371,7 @@ testsuite_setup(void)
 			"Failed to configure cryptodev %u with %u qps",
 			dev_id, ts_params->conf.nb_queue_pairs);
 
-	ts_params->qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT;
+	ts_params->qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
 
 	for (qp_id = 0; qp_id < info.max_nb_queue_pairs; qp_id++) {
 		TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
@@ -430,12 +423,6 @@ ut_setup(void)
 			"Failed to configure cryptodev %u",
 			ts_params->valid_devs[0]);
 
-	/*
-	 * Now reconfigure queues to size we actually want to use in this
-	 * test suite.
-	 */
-	ts_params->qp_conf.nb_descriptors = DEFAULT_NUM_OPS_INFLIGHT;
-
 	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs ; qp_id++) {
 		TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
 			ts_params->valid_devs[0], qp_id,
diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
index e8fc097..27d8cf8 100644
--- a/app/test/test_cryptodev_perf.c
+++ b/app/test/test_cryptodev_perf.c
@@ -426,9 +426,7 @@ testsuite_setup(void)
 
 	/*
 	 * Using Crypto Device Id 0 by default.
-	 * Since we can't free and re-allocate queue memory always set the queues
-	 * on this device up to max size first so enough memory is allocated for
-	 * any later re-configures needed by other tests
+	 * Set up all the qps on this device
 	 */
 
 	rte_cryptodev_info_get(ts_params->dev_id, &info);
@@ -442,19 +440,6 @@ testsuite_setup(void)
 			"Failed to configure cryptodev %u",
 			ts_params->dev_id);
 
-
-	ts_params->qp_conf.nb_descriptors = MAX_NUM_OPS_INFLIGHT;
-
-	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs ; qp_id++) {
-		TEST_ASSERT_SUCCESS(rte_cryptodev_queue_pair_setup(
-			ts_params->dev_id, qp_id,
-			&ts_params->qp_conf,
-			rte_cryptodev_socket_id(ts_params->dev_id)),
-			"Failed to setup queue pair %u on cryptodev %u",
-			qp_id, ts_params->dev_id);
-	}
-
-	/*Now reconfigure queues to size we actually want to use in this testsuite.*/
 	ts_params->qp_conf.nb_descriptors = PERF_NUM_OPS_INFLIGHT;
 	for (qp_id = 0; qp_id < ts_params->conf.nb_queue_pairs ; qp_id++) {
 
-- 
2.5.0

[PATCH v3 4/4] app/test: remove hard-coding of crypto num qps

[Fiona Trahe]

ts_params->conf.nb_queue_pairs should not be hard coded with device
specific number. It should be retrieved from the device info.
Any test which changes it should restore it to orig value.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
 app/test/test_cryptodev.c      | 5 ++++-
 app/test/test_cryptodev_perf.c | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index e0b0252..83036a1 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -414,7 +414,6 @@ ut_setup(void)
 	memset(ut_params, 0, sizeof(*ut_params));
 
 	/* Reconfigure device to default parameters */
-	ts_params->conf.nb_queue_pairs = DEFAULT_NUM_QPS_PER_QAT_DEVICE;
 	ts_params->conf.socket_id = SOCKET_ID_ANY;
 	ts_params->conf.session_mp.nb_objs = DEFAULT_NUM_OPS_INFLIGHT;
 
@@ -526,6 +525,7 @@ static int
 test_device_configure_invalid_queue_pair_ids(void)
 {
 	struct crypto_testsuite_params *ts_params = &testsuite_params;
+	uint16_t orig_nb_qps = ts_params->conf.nb_queue_pairs;
 
 	/* Stop the device in case it's started so it can be configured */
 	rte_cryptodev_stop(ts_params->valid_devs[0]);
@@ -580,6 +580,9 @@ test_device_configure_invalid_queue_pair_ids(void)
 			ts_params->valid_devs[0],
 			ts_params->conf.nb_queue_pairs);
 
+	/* revert to original testsuite value */
+	ts_params->conf.nb_queue_pairs = orig_nb_qps;
+
 	return TEST_SUCCESS;
 }
 
diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
index 27d8cf8..4aee9af 100644
--- a/app/test/test_cryptodev_perf.c
+++ b/app/test/test_cryptodev_perf.c
@@ -431,7 +431,7 @@ testsuite_setup(void)
 
 	rte_cryptodev_info_get(ts_params->dev_id, &info);
 
-	ts_params->conf.nb_queue_pairs = DEFAULT_NUM_QPS_PER_QAT_DEVICE;
+	ts_params->conf.nb_queue_pairs = info.max_nb_queue_pairs;
 	ts_params->conf.socket_id = SOCKET_ID_ANY;
 	ts_params->conf.session_mp.nb_objs = info.sym.max_nb_sessions;
 
-- 
2.5.0