> On 25 Jun 2020, at 16:35, Christiano F. Haesbaert <[email protected]> > wrote: > > On Thu, 25 Jun 2020 at 14:06, Vitaliy Makkoveev > <[email protected]> wrote: >> >> >> >>> On 25 Jun 2020, at 11:55, Martin Pieuchot <[email protected]> wrote: >>> >>> On 24/06/20(Wed) 17:10, Vitaliy Makkoveev wrote: >>>> While `mbuf' enqueued to `pipexinq' or `pipexoutq' it has the reference >>>> to corresponding pipex(4) session as `ph_cookie'. `ph_cookie' is >>>> accessed by pipexintr() and it's always defferent context from context >>>> where we destroy session. `ph_cookie' is protected only while we destroy >>>> session by pipex_timer() but this protection is not effective. While we >>>> destroy session related to pppx(4) `ph_cookie' is not potected. While we >>>> destroy session related to pppac(4) by `PIPEXSMODE' ioctl() or by >>>> closing pppac(4) device node `ph_cookie' is also not protected. >>>> >>>> I'am going to use reference counters to protect `ph_cookie' but some >>>> additional steps required to be done. >>> >>> Please no. Store an ifidx in session instead of a pointer. Such >>> index are guaranteed to be unique and can be used with if_get(9). >> >> This means I should do if_get(9) before each `ifnet’ usage. Also I >> should do checks and introduce error path. It's ugly. >> > > My 2 cents, > > Not really, this is actually easier to reason with, it produces less > dependencies between data structures and the protocol is kinda clear, > you have to account for the loss of atomicity. > Reference counting is pretty much the worst thing you can do in MP > development. > Very often it is actually desirable to pay for the cost of a lookup > (which is not the case here, since it's a silly O(1)) than to pay the > human/debug cost of interlinking data structures. > Also, in an ideal world if_get() would be RCUed (or SMRed) and would > not require bumping a reference count which implies an atomic > operation, it would also remove the need for if_put() since the epoch > is "until we context switch", as the kernel is not preemptive this is > easy to do without introducing new bugs. >
But I can’t store interface index with session instead of pointer. Session can be alive after corresponding `ifnet’ was destroyed and replaced by new `ifnet’. > >> Alternatively, I can pass `ifnet->if_index’ instead of `ifnet’ to >> sessinon being linked and store obtained reference up to session >> destruction. I should add one error path to pipex_link_session(). >> It’s ugly but a little bit. >> >>> >>> We deliberately kept if_ref() private to keep the code base coherent. >> >> Could you explain please why if_ref() and if_get() are incoherent?
