RE: [PATCH v2 2/2] lib/graph: default-align rte_graph_cluster_stats

[Marat Khalili <marat.khalili@huawei.com>]

Gently reminding about this issue, I still hope to see graph tests passing under sanitizer.

We cannot just remove __rte_cache_aligned from rte_graph_cluster_stats without removing it from rte_graph_cluster_node_stats because of how C works. But we can perhaps use rte_malloc to align our allocations in a way that matches the declaration. Would it be an ok solution?

> -----Original Message-----
> From: Marat Khalili <marat.khalili@huawei.com>
> Sent: Tuesday 17 June 2025 17:40
> To: Jerin Jacob <jerinjacobk@gmail.com>
> Cc: Jerin Jacob <jerinj@marvell.com>; Kiran Kumar K
> <kirankumark@marvell.com>; Nithin Dabilpuram
> <ndabilpuram@marvell.com>; Zhirun Yan <yanzhirun_163@163.com>; Pavan
> Nikhilesh <pbhagavatula@marvell.com>; dev@dpdk.org
> Subject: RE: [PATCH v2 2/2] lib/graph: default-align rte_graph_cluster_stats
> 
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Tuesday 17 June 2025 16:50
> >
> > > > > -struct __rte_cache_aligned rte_graph_cluster_node_stats {
> > > > > +struct rte_graph_cluster_node_stats {
> > > >
> > > > This is a fastpath structure. No need to change the alignment here.
> > >
> > > rte_graph_cluster_stats includes it, so unfortunately would stay cache-
> > aligned regardless of the attributes unless we make
> > rte_graph_cluster_node_stats default-aligned as well. If you are sure that
> > we need to keep node one cache-aligned we can return to rte_malloc
> > solution (or posix_memalign, but I would prefer not to hand-code aligned
> > realloc).
> >
> > I think, existing following code will take care of this. Are you
> > seeing the sanitizer issue if the change is only updating
> > rte_graph_cluster_stats alignment?
> 
> Yes, still seeing the sanitizer issue. And if fails even before reaching clusters,
> on the struct rte_graph_cluster_stats itself because I believe alignment
> propagates from members to enclosing structs.
> 
> >         /* For a given cluster, max nodes will be the max number of graphs */
> >         cluster_node_size += cluster->nb_graphs * sizeof(struct rte_node *);
> >         cluster_node_size = RTE_ALIGN(cluster_node_size,
> > RTE_CACHE_LINE_SIZE);
> 
> Please correct me if I am wrong, AFAIU it looks like this in memory:
> - rte_graph_cluster_stats
> - cluster_node
>   - rte_graph_cluster_node_stats
> - rte_node *[nb_graphs]
> - padding of rte_node pointers array to cacheline
> - cluster_node
>   - rte_graph_cluster_node_stats
> - rte_node *[nb_graphs]...
> 
> cluster_node_size calculation above ensures correct distance between
> cluster_node structs due to presence of the rte_node pointers arrays in
> between, but alignment of the first cluster_node still depends on the
> alignment of rte_graph_cluster_stats, and each of the next ones depends on
> the previous. We could of course re-align the first one manually (in each loop,
> after allocating some extra space), but to me it looks like the job rte_malloc
> could easier do for us. What do you think would be the right way forward?