On 2/1/22 14:14, Jason Gunthorpe wrote: > On Tue, Feb 01, 2022 at 02:00:09PM -0600, Bob Pearson wrote: > > >> as currently written the local variable has a kref obtained from the kref_get in >> rxe_lookup_mcg or the kref_init in rxe_init_mcg if it is newly created. This ref is >> dropped when the local variable goes out of scope. To protect the mcg when it is >> inactive at least one more ref is required. I take an additional ref in rxe_get_mcg >> if the mcg is created to protect the pointer in the red-black tree. This persists >> for the lifetime of the object until it is destroyed when it is removed from the tree >> and has the longest scope. This is enough to keep the object alive (it works fine BTW.) >> It is also possible to take ref's representing the pointers in the list but they >> wouldn't add anything. > > I think I got it upside down, but OK this works for me. What was > kbuild complaining about? > >> On the other point. Is it standard practice to user ERRPTRs in the >> kernel rather than return arguments? I seem to have seen both styles >> used but it may have been my imagination. I don't have any >> preference here but sometimes choose one or the other in parallel >> flows to make comparable routines in the flows have similar >> interfaces. > > Always prefer errptrs. > > Jason this API is a little different than most of the verbs APIs where typically we have obj = xx_create_obj() takes a reference at kref_init() and doesn't drop it on the non error path. returns obj. xx_other_code_using_obj(obj) uses obj holding ref xx_destroy_obj(obj) drops the ref to obj here the object is a local variable in xx_mcast_attach() and nothing is returned. For InfiniBand the create and destroy mcast group function is triggered by MADs from some central mcast server. For RoCEv2 that doesn't happen and these APIs don't exist. a missing static declaration at line 262. If you want me to I can fix it and resend. Do I really have to mint new version? Bob
