[PATCH v2 05/10] baseband/acc: enhance SW ring alignment

[Chautru, Nicolas]

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin at redhat.com>
> Sent: Tuesday, October 8, 2024 12:52 AM
> To: Vargas, Hernan <hernan.vargas at intel.com>; dev at dpdk.org;
> gakhil at marvell.com; trix at redhat.com
> Cc: Chautru, Nicolas <nicolas.chautru at intel.com>; Zhang, Qi Z
> <qi.z.zhang at intel.com>
> Subject: Re: [PATCH v2 05/10] baseband/acc: enhance SW ring alignment
> 
> 
> 
> On 10/3/24 22:49, Hernan Vargas wrote:
> > Calculate the aligned total size required for queue rings, ensuring
> > that the size is a power of two for proper memory allocation.
> >
> > Signed-off-by: Hernan Vargas <hernan.vargas at intel.com>
> > ---
> >   drivers/baseband/acc/acc_common.h | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/baseband/acc/acc_common.h
> > b/drivers/baseband/acc/acc_common.h
> > index 0d1c26166ff2..8ac1ca001c1d 100644
> > --- a/drivers/baseband/acc/acc_common.h
> > +++ b/drivers/baseband/acc/acc_common.h
> > @@ -767,19 +767,20 @@ alloc_sw_rings_min_mem(struct rte_bbdev
> *dev, struct acc_device *d,
> >   	int i = 0;
> >   	uint32_t q_sw_ring_size = ACC_MAX_QUEUE_DEPTH *
> get_desc_len();
> >   	uint32_t dev_sw_ring_size = q_sw_ring_size * num_queues;
> > -	/* Free first in case this is a reconfiguration */
> > +	uint32_t alignment = q_sw_ring_size *
> rte_align32pow2(num_queues);
> > +	/* Free first in case this is dev_sw_ring_size, q_sw_ring_size,
> > +socket); reconfiguration */
> 
> There is a copy/paste mistake in the comment?

Thanks, yes. We missed it in the rebase somehow. 

> 
> >   	rte_free(d->sw_rings_base);
> >
> >   	/* Find an aligned block of memory to store sw rings */
> >   	while (i < ACC_SW_RING_MEM_ALLOC_ATTEMPTS) {
> >   		/*
> >   		 * sw_ring allocated memory is guaranteed to be aligned to
> > -		 * q_sw_ring_size at the condition that the requested size is
> > +		 * alignment at the condition that the requested size is
> 
> This comment is really unclear "aligned to alignment"

Unclear indeed when reading it again. Should be "aligned to the variable `alignment` ..."
Ie the change is purely to use now the new variable `alignment` instead of `queue_ring_size`
Thanks

> 
> >   		 * less than the page size
> >   		 */
> >   		sw_rings_base = rte_zmalloc_socket(
> >   				dev->device->driver->name,
> > -				dev_sw_ring_size, q_sw_ring_size, socket);
> > +				dev_sw_ring_size, alignment, socket);
> >
> >   		if (sw_rings_base == NULL) {
> >   			rte_acc_log(ERR,