| Message ID | 20240912155830.14688-1-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tg3: Link IRQs to NAPI instances | expand |
On 9/12/2024 8:58 AM, Joe Damato wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > Link IRQs to NAPI instances with netif_napi_set_irq. This information > can be queried with the netdev-genl API. > > Compare the output of /proc/interrupts for my tg3 device with the output of > netdev-genl after applying this patch: > > $ cat /proc/interrupts | grep eth0 | cut -f1 --delimiter=':' > 331 > 332 > 333 > 334 > 335 > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump napi-get --json='{"ifindex": 2}' > > [{'id': 149, 'ifindex': 2, 'irq': 335}, > {'id': 148, 'ifindex': 2, 'irq': 334}, > {'id': 147, 'ifindex': 2, 'irq': 333}, > {'id': 146, 'ifindex': 2, 'irq': 332}, > {'id': 145, 'ifindex': 2, 'irq': 331}] > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > drivers/net/ethernet/broadcom/tg3.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > index 378815917741..c187b13ab3e6 100644 > --- a/drivers/net/ethernet/broadcom/tg3.c > +++ b/drivers/net/ethernet/broadcom/tg3.c > @@ -7393,6 +7393,14 @@ static int tg3_poll(struct napi_struct *napi, int budget) > return work_done; > } > > +static void tg3_napi_set_irq(struct tg3 *tp) > +{ > + int i; > + > + for (i = 0; i < tp->irq_cnt; i++) > + netif_napi_set_irq(&tp->napi[i].napi, tp->napi[i].irq_vec); > +} > + > static void tg3_napi_disable(struct tg3 *tp) > { > int i; > @@ -11652,7 +11660,7 @@ static int tg3_start(struct tg3 *tp, bool reset_phy, bool test_irq, > goto out_ints_fini; > > tg3_napi_init(tp); > - > + tg3_napi_set_irq(tp); I haven't used this API yet, so I was trying to figure out if this is the right place to call it. I think it is, but other examples may not have put it in the right place. I think there is a minor bug in fbnic/fbnic_txrx.c. I say this based on the following: fbnic_alloc_napi_vector() { <snip> -> netif_napi_set_irq() # napi->irq = irq -> netif_napi_add() -> netif_napi_add_weight() # napi->irq = -1 <snip> } Just as an FYI I submitted a patch to fix this just now based on reviewing your code: https://lore.kernel.org/netdev/20240912174922.10550-1-brett.creeley@amd.com/T/#u LGTM. Reviewed-by: Brett Creeley <brett.creeley@amd.com> > tg3_napi_enable(tp); > > for (i = 0; i < tp->irq_cnt; i++) { > -- > 2.25.1 > >
On Thu, Sep 12, 2024 at 8:58 AM Joe Damato <jdamato@fastly.com> wrote: > > Link IRQs to NAPI instances with netif_napi_set_irq. This information > can be queried with the netdev-genl API. > > Compare the output of /proc/interrupts for my tg3 device with the output of > netdev-genl after applying this patch: > > $ cat /proc/interrupts | grep eth0 | cut -f1 --delimiter=':' > 331 > 332 > 333 > 334 > 335 > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump napi-get --json='{"ifindex": 2}' > > [{'id': 149, 'ifindex': 2, 'irq': 335}, > {'id': 148, 'ifindex': 2, 'irq': 334}, > {'id': 147, 'ifindex': 2, 'irq': 333}, > {'id': 146, 'ifindex': 2, 'irq': 332}, > {'id': 145, 'ifindex': 2, 'irq': 331}] > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > drivers/net/ethernet/broadcom/tg3.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > index 378815917741..c187b13ab3e6 100644 > --- a/drivers/net/ethernet/broadcom/tg3.c > +++ b/drivers/net/ethernet/broadcom/tg3.c > @@ -7393,6 +7393,14 @@ static int tg3_poll(struct napi_struct *napi, int budget) > return work_done; > } > > +static void tg3_napi_set_irq(struct tg3 *tp) > +{ > + int i; > + > + for (i = 0; i < tp->irq_cnt; i++) > + netif_napi_set_irq(&tp->napi[i].napi, tp->napi[i].irq_vec); Looks good, but why not just add netif_napi_set_irq() to the existing loop in tg3_napi_init()? It will reduce the lines of code a bit. Thanks.
On Thu, Sep 12, 2024 at 11:04:02AM -0700, Michael Chan wrote: > On Thu, Sep 12, 2024 at 8:58 AM Joe Damato <jdamato@fastly.com> wrote: > > > > Link IRQs to NAPI instances with netif_napi_set_irq. This information > > can be queried with the netdev-genl API. > > > > Compare the output of /proc/interrupts for my tg3 device with the output of > > netdev-genl after applying this patch: > > > > $ cat /proc/interrupts | grep eth0 | cut -f1 --delimiter=':' > > 331 > > 332 > > 333 > > 334 > > 335 > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > --dump napi-get --json='{"ifindex": 2}' > > > > [{'id': 149, 'ifindex': 2, 'irq': 335}, > > {'id': 148, 'ifindex': 2, 'irq': 334}, > > {'id': 147, 'ifindex': 2, 'irq': 333}, > > {'id': 146, 'ifindex': 2, 'irq': 332}, > > {'id': 145, 'ifindex': 2, 'irq': 331}] > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > drivers/net/ethernet/broadcom/tg3.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > > index 378815917741..c187b13ab3e6 100644 > > --- a/drivers/net/ethernet/broadcom/tg3.c > > +++ b/drivers/net/ethernet/broadcom/tg3.c > > @@ -7393,6 +7393,14 @@ static int tg3_poll(struct napi_struct *napi, int budget) > > return work_done; > > } > > > > +static void tg3_napi_set_irq(struct tg3 *tp) > > +{ > > + int i; > > + > > + for (i = 0; i < tp->irq_cnt; i++) > > + netif_napi_set_irq(&tp->napi[i].napi, tp->napi[i].irq_vec); > > Looks good, but why not just add netif_napi_set_irq() to the existing > loop in tg3_napi_init()? It will reduce the lines of code a bit. > Thanks. I made a separate function because: - tg3_napi_init would need two calls (once for i=0, and once in the loop), and - tg3_napi_init and tg3_napi_enable are separated in the driver, so I figured I'd separate the IRQ linking, too. Can you let me know if you want me to submit a v2 which modifies tg3_napi_init instead?
On Thu, Sep 12, 2024 at 11:13 AM Joe Damato <jdamato@fastly.com> wrote: > > On Thu, Sep 12, 2024 at 11:04:02AM -0700, Michael Chan wrote: > > Looks good, but why not just add netif_napi_set_irq() to the existing > > loop in tg3_napi_init()? It will reduce the lines of code a bit. > > Thanks. > > I made a separate function because: > - tg3_napi_init would need two calls (once for i=0, and once in > the loop), and > - tg3_napi_init and tg3_napi_enable are separated in the driver, > so I figured I'd separate the IRQ linking, too. > tg3_napi_enable() needs to be separated because it is called in multiple places and tg3_napi_init() is only called once. Even with 2 calls in tg3_napi_init(), I think it will still reduce the lines of code. Or we can do this to do everything in the loop: for (i = 0; i < tp->irq_cnt; i++) { netif_napi_add(tp->dev, &tp->napi[i].napi, i ? tg3_poll_msix : tg3_poll); netif_set_napi_irq(...); }
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 378815917741..c187b13ab3e6 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -7393,6 +7393,14 @@ static int tg3_poll(struct napi_struct *napi, int budget) return work_done; } +static void tg3_napi_set_irq(struct tg3 *tp) +{ + int i; + + for (i = 0; i < tp->irq_cnt; i++) + netif_napi_set_irq(&tp->napi[i].napi, tp->napi[i].irq_vec); +} + static void tg3_napi_disable(struct tg3 *tp) { int i; @@ -11652,7 +11660,7 @@ static int tg3_start(struct tg3 *tp, bool reset_phy, bool test_irq, goto out_ints_fini; tg3_napi_init(tp); - + tg3_napi_set_irq(tp); tg3_napi_enable(tp); for (i = 0; i < tp->irq_cnt; i++) {
Link IRQs to NAPI instances with netif_napi_set_irq. This information can be queried with the netdev-genl API. Compare the output of /proc/interrupts for my tg3 device with the output of netdev-genl after applying this patch: $ cat /proc/interrupts | grep eth0 | cut -f1 --delimiter=':' 331 332 333 334 335 $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump napi-get --json='{"ifindex": 2}' [{'id': 149, 'ifindex': 2, 'irq': 335}, {'id': 148, 'ifindex': 2, 'irq': 334}, {'id': 147, 'ifindex': 2, 'irq': 333}, {'id': 146, 'ifindex': 2, 'irq': 332}, {'id': 145, 'ifindex': 2, 'irq': 331}] Signed-off-by: Joe Damato <jdamato@fastly.com> --- drivers/net/ethernet/broadcom/tg3.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)