| Message ID | 20230308162619.329372-1-lsahn@ooseel.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: stmmac: call stmmac_finalize_xdp_rx() on a condition | expand |
On Thu, Mar 09, 2023 at 01:26:18AM +0900, Leesoo Ahn wrote: > The current codebase calls the function no matter net device has XDP > programs or not. So the finalize function is being called everytime when RX > bottom-half in progress. It needs a few machine instructions for nothing > in the case that XDP programs are not attached at all. > > Lets it call the function on a condition that if xdp_status variable has > not zero value. That means XDP programs are attached to the net device > and it should be finalized based on the variable. > > The following instructions show that it's better than calling the function > unconditionally. > > 0.31 │6b8: ldr w0, [sp, #196] > │ ┌──cbz w0, 6cc > │ │ mov x1, x0 > │ │ mov x0, x27 > │ │→ bl stmmac_finalize_xdp_rx > │6cc:└─→ldr x1, [sp, #176] > > with 'if (xdp_status)' statement, jump to '6cc' label if xdp_status has > zero value. > > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net> Hi Leesoo, I am curious to know if you considered going a step further and using a static key. Link: https://www.kernel.org/doc/html/latest/staging/static-keys.html
On 23. 3. 9. 22:40, Simon Horman wrote: > On Thu, Mar 09, 2023 at 01:26:18AM +0900, Leesoo Ahn wrote: >> The current codebase calls the function no matter net device has XDP >> programs or not. So the finalize function is being called everytime when RX >> bottom-half in progress. It needs a few machine instructions for nothing >> in the case that XDP programs are not attached at all. >> >> Lets it call the function on a condition that if xdp_status variable has >> not zero value. That means XDP programs are attached to the net device >> and it should be finalized based on the variable. >> >> The following instructions show that it's better than calling the function >> unconditionally. >> >> 0.31 │6b8: ldr w0, [sp, #196] >> │ ┌──cbz w0, 6cc >> │ │ mov x1, x0 >> │ │ mov x0, x27 >> │ │→ bl stmmac_finalize_xdp_rx >> │6cc:└─→ldr x1, [sp, #176] >> >> with 'if (xdp_status)' statement, jump to '6cc' label if xdp_status has >> zero value. >> >> Signed-off-by: Leesoo Ahn <lsahn@ooseel.net> > Hi Leesoo, > > I am curious to know if you considered going a step further and using > a static key. > > Link: https://www.kernel.org/doc/html/latest/staging/static-keys.html Thank you for the review. The function must be called for only XDP_TX or XDP_REDIRECT cases. So using a static key doesn't look good and the commit message is not clear for 'why' as well. I think that's why you suggested for using 'static key' by the latter reason. I will edit the message and post v2 soon. Best regards, Leesoo
On Fri, Mar 10, 2023 at 12:08:29AM +0900, Leesoo Ahn wrote: > > > On 23. 3. 9. 22:40, Simon Horman wrote: > > On Thu, Mar 09, 2023 at 01:26:18AM +0900, Leesoo Ahn wrote: > > > The current codebase calls the function no matter net device has XDP > > > programs or not. So the finalize function is being called everytime when RX > > > bottom-half in progress. It needs a few machine instructions for nothing > > > in the case that XDP programs are not attached at all. > > > > > > Lets it call the function on a condition that if xdp_status variable has > > > not zero value. That means XDP programs are attached to the net device > > > and it should be finalized based on the variable. > > > > > > The following instructions show that it's better than calling the function > > > unconditionally. > > > > > > 0.31 │6b8: ldr w0, [sp, #196] > > > │ ┌──cbz w0, 6cc > > > │ │ mov x1, x0 > > > │ │ mov x0, x27 > > > │ │→ bl stmmac_finalize_xdp_rx > > > │6cc:└─→ldr x1, [sp, #176] > > > > > > with 'if (xdp_status)' statement, jump to '6cc' label if xdp_status has > > > zero value. > > > > > > Signed-off-by: Leesoo Ahn <lsahn@ooseel.net> > > Hi Leesoo, > > > > I am curious to know if you considered going a step further and using > > a static key. > > > > Link: https://www.kernel.org/doc/html/latest/staging/static-keys.html > > Thank you for the review. > > The function must be called for only XDP_TX or XDP_REDIRECT cases. > So using a static key doesn't look good and the commit message is not clear > for 'why' as well. > I think that's why you suggested for using 'static key' by the latter > reason. Yes, my suggestion was based on the performance optimisation aspect of your patch. > I will edit the message and post v2 soon. Thanks
> The function must be called for only XDP_TX or XDP_REDIRECT cases. Which constraints should be taken better into account for the discussed function call? > I will edit the message and post v2 soon. Did the improvement idea evolve further in the meantime? Regards, Markus
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index e4902a7bb61e..53c6e9b3a0c2 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -5145,7 +5145,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue) rx_q->state.len = len; } - stmmac_finalize_xdp_rx(priv, xdp_status); + if (xdp_status) + stmmac_finalize_xdp_rx(priv, xdp_status); priv->xstats.rx_pkt_n += count; priv->xstats.rxq_stats[queue].rx_pkt_n += count; @@ -5425,7 +5426,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) rx_q->state.len = len; } - stmmac_finalize_xdp_rx(priv, xdp_status); + if (xdp_status) + stmmac_finalize_xdp_rx(priv, xdp_status); stmmac_rx_refill(priv, queue);
The current codebase calls the function no matter net device has XDP programs or not. So the finalize function is being called everytime when RX bottom-half in progress. It needs a few machine instructions for nothing in the case that XDP programs are not attached at all. Lets it call the function on a condition that if xdp_status variable has not zero value. That means XDP programs are attached to the net device and it should be finalized based on the variable. The following instructions show that it's better than calling the function unconditionally. 0.31 │6b8: ldr w0, [sp, #196] │ ┌──cbz w0, 6cc │ │ mov x1, x0 │ │ mov x0, x27 │ │→ bl stmmac_finalize_xdp_rx │6cc:└─→ldr x1, [sp, #176] with 'if (xdp_status)' statement, jump to '6cc' label if xdp_status has zero value. Signed-off-by: Leesoo Ahn <lsahn@ooseel.net> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)