On 26/06/20(Fri) 12:35, Vitaliy Makkoveev wrote: > On Fri, Jun 26, 2020 at 10:23:42AM +0200, Martin Pieuchot wrote: > > On 25/06/20(Thu) 19:56, Vitaliy Makkoveev wrote: > > > Updated diff. > > > > > > OpenBSD uses 16 bit counter for allocate interface indexes. So we can't > > > store index in session and be sure if_get(9) returned `ifnet' is our > > > original `ifnet'. > > > > Why not? The point of if_get(9) is to be sure. If that doesn't work > > for whatever reason then the if_get(9) interface has to be fixed. Which > > case doesn't work for you? Do you have a reproducer? > > > > How does sessions stay around if their corresponding interface is > > destroyed? > > We have `pipexinq' and `pipexoutq' which can store pointers to session. > pipexintr() process these queues. pipexintr() and > pipex_destroy_session() are *always* different context. This mean we > *can't* free pipex(4) session without be sure there is no reference to > this session in `pipexinq' or `pipexoutq'. Elsewhere this will cause use > afret free issue. Look please at net/pipex.c:846. The way pppx(4) > destroy sessions is wrong. While pppac(4) destroys sessions by > pipex_iface_fini() it's also wrong. Because we don't check `pipexinq' > and `pipexoutq' state. I'am said it again and again.
I understand. Why is it a problem? Using reference counting the way you're suggesting is *one* possible solution to a problem we don't fully understand. What are we trying to achieve? Which problem are we trying to solve? Are we trying to move pipexintr() out of KERNEL_LOCK()? If so are we trying to guarantee the memory pointed by `ph_cookie' representing a session pointer is always valid without the KERNEL_LOCK()? If that's what we're trying to do, shouldn't we start with a baby-step and trade the KERNEL_LOCK() for another lock first? I understand it is not some sexy-mp-rcu-atomic-crazy-refcount-clever solution, but it is simpler to do. Since all the packets (mbuf) are being processed with the NET_LOCK(). we should be able to start trading the KERNEL_LOCK() by the NET_LOCK(), no? Once that works and we've built knowledge about the existing architecture we might consider some finer grain locking. Is there any downside to this suggestion?
