19/10/2021 10:32, Harman Kalra: > From: Stephen Hemminger <step...@networkplumber.org> > > On Tue, 19 Oct 2021 01:07:02 +0530 > > Harman Kalra <hka...@marvell.com> wrote: > > > + /* Detect if DPDK malloc APIs are ready to be used. */ > > > + mem_allocator = rte_malloc_is_ready(); > > > + if (mem_allocator) > > > + intr_handle = rte_zmalloc(NULL, sizeof(struct > > rte_intr_handle), > > > + 0); > > > + else > > > + intr_handle = calloc(1, sizeof(struct rte_intr_handle)); > > > > This is problematic way to do this. > > The reason to use rte_malloc vs malloc should be determined by usage. > > > > If the pointer will be shared between primary/secondary process then it has > > to be in hugepages (ie rte_malloc). If it is not shared then then use > > regular > > malloc. > > > > But what you have done is created a method which will be a latent bug for > > anyone using primary/secondary process. > > > > Either: > > intr_handle is not allowed to be used in secondary. > > Then always use malloc(). > > Or. > > intr_handle can be used by both primary and secondary. > > Then always use rte_malloc(). > > Any code path that allocates intr_handle before pool is > > ready is broken. > > Hi Stephan, > > Till V2, I implemented this API in a way where user of the API can choose > If he wants intr handle to be allocated using malloc or rte_malloc by passing > a flag arg to the rte_intr_instanc_alloc API. User of the API will best know > if > the intr handle is to be shared with secondary or not.
Yes the caller should know, but it makes usage more difficult. Using rte_malloc always is simpler. > But after some discussions and suggestions from the community we decided > to drop that flag argument and auto detect on whether rte_malloc APIs are > ready to be used and thereafter make all further allocations via rte_malloc. > Currently alarm subsystem (or any driver doing allocation in constructor) gets > interrupt instance allocated using glibc malloc that too because rte_malloc* > is not ready by rte_eal_alarm_init(), while all further consumers gets > instance > allocated via rte_malloc. Yes the general case is to allocate after rte_malloc is ready. Anyway a constructor should not allocate complicate things. > I think this should not cause any issue in primary/secondary model as all > interrupt > instance pointer will be shared. Infact to avoid any surprises of > primary/secondary > not working we thought of making all allocations via rte_malloc. Yes > David, Thomas, Dmitry, please add if I missed anything. I understand Stephen's concern but I think this choice is a good compromise. Ideally we should avoid doing real stuff in constructors. > Can we please conclude on this series APIs as API freeze deadline (rc1) is > very near. I vote for keeping this design.
