Re: status of bpf_mtap()/bpf_mtap_softint() ?

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Wed, May 24, 2017 at 01:09:02PM +0900, Ryota Ozaki wrote:
> I've looked the codes of bouyer-socketcan. I think can_input doesn't need
> softinit-ification because can_input just soon calls schednetisr(NETISR_CAN)
> and doesn't involve other network components that assume running in
> softint. What we need to do on Rx is just to replace bpf_mtap with
> bpf_mtap_softint. (You may need to implement can_bpf_mtap_softint or the like).
> 
> On Tx, the current implementation is a bit different from ordinary drivers
> and needs some refactoring. I've written a patch that fills the gap:
>   http://www.netbsd.org/~ozaki-r/awin_can.diff
> It moves the Tx operation from awin_can_tx_intr to awin_can_ifstart
> and lets awin_can_tx_intr use the deferred if_start mechanism to kick
> if_start if needed.
> 
> It doesn't require softint on per-Tx on normal load. Under heavy load,
> the deferred if_start may be triggered and that involves softint though.
> 
> How about the change?

thanks. I had something similar in mind.
But with this, we have packets on the RX path hanlded by bpf_mtap_softint(),
and TX packets handled by bpf_mtap().
Can't this cause a TX packet to be delivered ahead of a RX packet to the
bpf listerner ? This is why I wanted to use bpf_mtap_softint() everywhere.
On second look it's not such a big deal, as this can already happen at
the hardware level.

Also you remove KASSERT((ifp->if_flags & IFF_OACTIVE) == 0) from
awin_can_ifstart(). I guess this is because a race is possible between
calls from the softint and from the upper layer. But in this case I think
awin_can_ifstart() should instead test for IFF_OACTIVE and return
immediately if it is set, or we may try to send a packet while the
hardware is already busy.
Also awin_can_ifstart() should not clear IFF_OACTIVE, only the
interrupt handler should.
Does it look right ?

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--