status of bpf_mtap()/bpf_mtap_softint() ?

[Manuel Bouyer]

Hello
looking more closely at bpf for CAN, there is something I don't understand.
It looks like bpf_mtap() can't be called from interrupt context any more,
as shown by the KASSERT() in bpf_deliver().
bpf_mtap_softint() was was introduced for this.

But it looks like most ethernet drivers will still call bpf_mtap() from
hardware interrupt context.
For example, if_tl.c calls bpf_mtap() from tl_ifstart(), which is called
on TX interrpt. ic/hme.c is another example.

Did I miss somehing, or have these drivers missed some conversion (and
have not been tested with a DIAGNOSTIC kernel) ?

[Ryota Ozaki]

Post by Manuel Bouyer
Hello
looking more closely at bpf for CAN, there is something I don't understand.
It looks like bpf_mtap() can't be called from interrupt context any more,
as shown by the KASSERT() in bpf_deliver().
bpf_mtap_softint() was was introduced for this.

bpf_mtap needs different treatments for Rx and Tx.

For Rx, network device drivers should (1) implement the Rx interrupt handler
as sofint or (2) use if_percpuq facility to ensure the interface input
routine (traditionally it's ifp->if_input) and bpf_mtap (and upper layers)
run in softint. Most drivers need to just use if_percpuq_enqueue instead of
ifp->if_input.

bpf_mtap_softint was introduced for bpf_mtap that were difficult to run
it in softint (e.g., to do so, fundamental changes are needed). Basically
we shouldn't use bpf_mtap_softint and should adopt either of the above two
approaches.

For Tx, bpf_mtap is called in XXX_start that is normally executed in
a normal packet transmission path (LWP or softint). However, some drivers
try to call XXX_start in the Tx interrupt handler. To avoid such calls,
the deferred if_start mechanism was introduced. What we need is just to
replace XXX_start called in an interrupt handler with
if_schedule_deffered_start.

Post by Manuel Bouyer
But it looks like most ethernet drivers will still call bpf_mtap() from
hardware interrupt context.
For example, if_tl.c calls bpf_mtap() from tl_ifstart(), which is called
on TX interrpt. ic/hme.c is another example.
Did I miss somehing, or have these drivers missed some conversion (and
have not been tested with a DIAGNOSTIC kernel) ?

Just passed over to convert... I fixed some more drivers including the two.
Let me know if there are others to convert.

ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Manuel Bouyer]

Post by Ryota Ozaki

Post by Manuel Bouyer
Hello
looking more closely at bpf for CAN, there is something I don't understand.
It looks like bpf_mtap() can't be called from interrupt context any more,
as shown by the KASSERT() in bpf_deliver().
bpf_mtap_softint() was was introduced for this.

bpf_mtap needs different treatments for Rx and Tx.
For Rx, network device drivers should (1) implement the Rx interrupt handler
as sofint or (2) use if_percpuq facility to ensure the interface input
routine (traditionally it's ifp->if_input) and bpf_mtap (and upper layers)
run in softint. Most drivers need to just use if_percpuq_enqueue instead of
ifp->if_input.
bpf_mtap_softint was introduced for bpf_mtap that were difficult to run
it in softint (e.g., to do so, fundamental changes are needed). Basically
we shouldn't use bpf_mtap_softint and should adopt either of the above two
approaches.
For Tx, bpf_mtap is called in XXX_start that is normally executed in
a normal packet transmission path (LWP or softint). However, some drivers
try to call XXX_start in the Tx interrupt handler. To avoid such calls,
the deferred if_start mechanism was introduced. What we need is just to
replace XXX_start called in an interrupt handler with
if_schedule_deffered_start.

thanks.
For CAN this is quite a bit of overhead. CAN controllers usually don't do
DMA and can handle only one packet at a time. This means one extra softint
per received packet, and eventually one extra softint per transmitted packet.

bpf_mtap_softint() is probably cheaper because it will schedule an
extra softint only if there is a bpf listener.
Can we remove the KASSERT(cpu_intr_p()) in bpf_mtap_softint() ?
I can't see why it couldn't be called from thread context. Using
bpf_mtap() or bpf_mtap_softint() depending on context can lead to
out of order packet delivery at the bpf level.

[Thor Lancelot Simon]

Post by Manuel Bouyer

Post by Ryota Ozaki

Post by Manuel Bouyer
Hello
looking more closely at bpf for CAN, there is something I don't understand.
It looks like bpf_mtap() can't be called from interrupt context any more,
as shown by the KASSERT() in bpf_deliver().
bpf_mtap_softint() was was introduced for this.

bpf_mtap needs different treatments for Rx and Tx.
For Rx, network device drivers should (1) implement the Rx interrupt handler
as sofint or (2) use if_percpuq facility to ensure the interface input
routine (traditionally it's ifp->if_input) and bpf_mtap (and upper layers)
run in softint. Most drivers need to just use if_percpuq_enqueue instead of
ifp->if_input.
bpf_mtap_softint was introduced for bpf_mtap that were difficult to run
it in softint (e.g., to do so, fundamental changes are needed). Basically
we shouldn't use bpf_mtap_softint and should adopt either of the above two
approaches.
For Tx, bpf_mtap is called in XXX_start that is normally executed in
a normal packet transmission path (LWP or softint). However, some drivers
try to call XXX_start in the Tx interrupt handler. To avoid such calls,
the deferred if_start mechanism was introduced. What we need is just to
replace XXX_start called in an interrupt handler with
if_schedule_deffered_start.

thanks.
For CAN this is quite a bit of overhead. CAN controllers usually don't do
DMA and can handle only one packet at a time. This means one extra softint
per received packet, and eventually one extra softint per transmitted packet.

Is it possible to coalesce -- accumulate packets until a given count, or
enough time passes, and then schedule the softint?

Thor

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Manuel Bouyer]

Post by Ryota Ozaki
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
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 ?

[Ryota Ozaki]

Post by Manuel Bouyer

Post by Ryota Ozaki
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
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.

Post by Manuel Bouyer
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.

Post by Manuel Bouyer
Also awin_can_ifstart() should not clear IFF_OACTIVE, only the
interrupt handler should.

Sure. That line should be removed.

Post by Manuel Bouyer
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

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Manuel Bouyer]

Post by Thor Lancelot Simon

Post by Manuel Bouyer
thanks.
For CAN this is quite a bit of overhead. CAN controllers usually don't do
DMA and can handle only one packet at a time. This means one extra softint
per received packet, and eventually one extra softint per transmitted packet.

Is it possible to coalesce -- accumulate packets until a given count, or
enough time passes, and then schedule the softint?

I guess this then requires a callout. It may be more overhead than
always scheduling the softint.

[Ryota Ozaki]

Post by Manuel Bouyer

Post by Thor Lancelot Simon

Post by Manuel Bouyer
thanks.
For CAN this is quite a bit of overhead. CAN controllers usually don't do
DMA and can handle only one packet at a time. This means one extra softint
per received packet, and eventually one extra softint per transmitted packet.

Is it possible to coalesce -- accumulate packets until a given count, or
enough time passes, and then schedule the softint?

I guess this then requires a callout. It may be more overhead than
always scheduling the softint.

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?
ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Ryota Ozaki]

Post by Manuel Bouyer

Post by Ryota Ozaki

Post by Manuel Bouyer
Hello
looking more closely at bpf for CAN, there is something I don't understand.
It looks like bpf_mtap() can't be called from interrupt context any more,
as shown by the KASSERT() in bpf_deliver().
bpf_mtap_softint() was was introduced for this.

bpf_mtap needs different treatments for Rx and Tx.
For Rx, network device drivers should (1) implement the Rx interrupt handler
as sofint or (2) use if_percpuq facility to ensure the interface input
routine (traditionally it's ifp->if_input) and bpf_mtap (and upper layers)
run in softint. Most drivers need to just use if_percpuq_enqueue instead of
ifp->if_input.
bpf_mtap_softint was introduced for bpf_mtap that were difficult to run
it in softint (e.g., to do so, fundamental changes are needed). Basically
we shouldn't use bpf_mtap_softint and should adopt either of the above two
approaches.
For Tx, bpf_mtap is called in XXX_start that is normally executed in
a normal packet transmission path (LWP or softint). However, some drivers
try to call XXX_start in the Tx interrupt handler. To avoid such calls,
the deferred if_start mechanism was introduced. What we need is just to
replace XXX_start called in an interrupt handler with
if_schedule_deffered_start.

thanks.
For CAN this is quite a bit of overhead. CAN controllers usually don't do
DMA and can handle only one packet at a time. This means one extra softint
per received packet, and eventually one extra softint per transmitted packet.
bpf_mtap_softint() is probably cheaper because it will schedule an
extra softint only if there is a bpf listener.
Can we remove the KASSERT(cpu_intr_p()) in bpf_mtap_softint() ?
I can't see why it couldn't be called from thread context. Using
bpf_mtap() or bpf_mtap_softint() depending on context can lead to
out of order packet delivery at the bpf level.

bpf_mtap_softint intends to be used at a place that unavoidably
runs in hardware interrupt context but needs to do bpf_mtap.
It shouldn't be used at other places.

And of course bpf_mtap and bpf_mtap_softint shouldn't be mixed
on one direction.

ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de