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

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Wed, May 24, 2017 at 5:09 PM, Manuel Bouyer <bouyer%antioche.eu.org@localhost> wrote:
> 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.

Reordering of bpf delivering can happen as you said, but if Tx packets
and Rx packets aren't related, I think reordering of them doesn't matter.
OTOH, if there is a relation between Tx and Rx packets for example
a Tx packet that is a reply to a Rx packet is handled by bpf prior to
the Rx packet, that's a problem. Fortunately our implementation of softint
serves softint requests in FIFO, so bpf_mtap_softint for a Rx packet is
always handled prior to canintr softint that may transmit a Tx packet as
a reply to the Rx packet. Someone please correct me if my guess is wrong.

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

I think either of them is ok.

> Also awin_can_ifstart() should not clear IFF_OACTIVE, only the
> interrupt handler should.

Sure. That line should be removed.

> Does it look right ?

Looks right to me (after the above fixes), but I didn't test the
code at all, so please test :)

Thanks,
  ozaki-r