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. > 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?
