| Message ID | 20210202022420.1328397-1-anthony.l.nguyen@intel.com |
|---|---|
| Headers | show |
| Series | 40GbE Intel Wired LAN Driver Updates 2021-02-01 | expand |
On Mon, 1 Feb 2021 18:24:19 -0800 Tony Nguyen wrote: > From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > > New trace indicates that the XDP program was loaded. > The trace has a note that in case of using XDP_REDIRECT, > number of queues on both interfaces shall be the same. > This is required for optimal performance of XDP_REDIRECT, > if interface used for TX has lower number of queues than > a RX interface, the packets may be dropped (depending on > RSS queue assignment). By RSS queue assignment you mean interrupt mapping? > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index 521ea9df38d5..f35bd9164106 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, > /* Kick start the NAPI context if there is an AF_XDP socket open > * on that queue id. This so that receiving will start. > */ > - if (need_reset && prog) > + if (need_reset && prog) { > + dev_info(&pf->pdev->dev, > + "Loading XDP program, please note: XDP_REDIRECT action requires the same number of queues on both interfaces\n"); We try to avoid spamming logs. This message will be helpful to users only the first time, if at all.
Hi Kuba, thank you for the comments! >On Mon, 1 Feb 2021 18:24:19 -0800 Tony Nguyen wrote: >> From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> >> New trace indicates that the XDP program was loaded. >> The trace has a note that in case of using XDP_REDIRECT, >> number of queues on both interfaces shall be the same. >> This is required for optimal performance of XDP_REDIRECT, >> if interface used for TX has lower number of queues than >> a RX interface, the packets may be dropped (depending on >> RSS queue assignment). > >By RSS queue assignment you mean interrupt mapping? Yes, interrupt mapping seems more accurate, will fix it. > >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c >> index 521ea9df38d5..f35bd9164106 100644 >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >> @@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, >> /* Kick start the NAPI context if there is an AF_XDP socket open >> * on that queue id. This so that receiving will start. >> */ >> - if (need_reset && prog) >> + if (need_reset && prog) { >> + dev_info(&pf->pdev->dev, >> + "Loading XDP program, please note: XDP_REDIRECT action requires the same number of queues on both interfaces\n"); > >We try to avoid spamming logs. This message will be helpful to users >only the first time, if at all. You are probably right, it would look like a spam to the one who is continuously loading and unloading the XDP programs. But still, want to remain as much user friendly as possible. Will use dev_info_once(...) instead. --------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN. Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
On Wed, 3 Feb 2021 10:00:07 +0000 Kubalewski, Arkadiusz wrote: > >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > >> index 521ea9df38d5..f35bd9164106 100644 > >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > >> @@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, > >> /* Kick start the NAPI context if there is an AF_XDP socket open > >> * on that queue id. This so that receiving will start. > >> */ > >> - if (need_reset && prog) > >> + if (need_reset && prog) { > >> + dev_info(&pf->pdev->dev, > >> + "Loading XDP program, please note: XDP_REDIRECT action requires the same number of queues on both interfaces\n"); > > > >We try to avoid spamming logs. This message will be helpful to users > >only the first time, if at all. > > You are probably right, it would look like a spam to the one who is > continuously loading and unloading the XDP programs. > But still, want to remain as much user friendly as possible. > Will use dev_info_once(...) instead. Not exactly what I meant, I meant that it's only marginally useful the first time the user sees it. Not first time since boot. The two options that I think could be better are: - work on improving the interfaces in terms of IRQ/queue config and capabilities so the user is not confused in the first place; - detect that the configuration is in fact problematic (IOW #Qs < #CPUs) and setting extack. If you set the extact and return 0 / success the extact will show as "Warning: " in iproute2 output.
>On Wed, 3 Feb 2021 10:00:07 +0000 Kubalewski, Arkadiusz wrote: >> >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c >> >> b/drivers/net/ethernet/intel/i40e/i40e_main.c >> >> index 521ea9df38d5..f35bd9164106 100644 >> >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c >> >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >> >> @@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, >> >> /* Kick start the NAPI context if there is an AF_XDP socket open >> >> * on that queue id. This so that receiving will start. >> >> */ >> >> - if (need_reset && prog) >> >> + if (need_reset && prog) { >> >> + dev_info(&pf->pdev->dev, >> >> + "Loading XDP program, please note: XDP_REDIRECT action >> >> +requires the same number of queues on both interfaces\n"); >> > >> >We try to avoid spamming logs. This message will be helpful to users >> >only the first time, if at all. >> >> You are probably right, it would look like a spam to the one who is >> continuously loading and unloading the XDP programs. >> But still, want to remain as much user friendly as possible. >> Will use dev_info_once(...) instead. > >Not exactly what I meant, I meant that it's only marginally useful the first time the user sees it. Not first time since boot. > >The two options that I think could be better are: Well, I know that this is far from being perfect. If I understand your comments correctly: > - work on improving the interfaces in terms of IRQ/queue config and > capabilities so the user is not confused in the first place; Improved interface would allow the driver which is being loaded with the xdp program to receive configuration of the other NICs on the system. (its number of queues/IRQs), and in such case we could warn the user about possible drops. > - detect that the configuration is in fact problematic > (IOW #Qs < #CPUs) and setting extack. If you set the extact and > return 0 / success the extact will show as "Warning: " in iproute2 > output. > It seems like this is the same idea? Detect number of queues of other NIC. Then warn the user. In general I agree and that was my first idea... but after all, we decided to just try hint the user, that proper configuration is required. (Hopefully) the proper solution.. With my current knowledge and understanding of how XDP_REDIRECT works: It cannot be loaded with iproute2, it uses bpf maps thus it also requires an loader application to create ones (i.e. the one from samples/bpf/) The sample uses /tools/lib/bpf library calls, which uses netlink to eventually do the .ndo_bpf call on two ports. The one responsible for the RX and the other TX one. Although XDP can redirect to more then one TX. Thus proper solution has to work for both cases. In case of two or more devies used for redirecting TX (i.e. properly implemented xdp_redirect_map). The interface which is used for RX shall receive the lowest number of queues/IRQs of all the possible TX interfaces. Then it can properly warn the user. I think this is doable, but it requires changes on all the way from bpf program loader, through: libbpf, netlink, net/core.. Probably finally extending netdev_bpf with a field that stores the lowest number of queues of the interfaces which are used for TX. Real proper solution.. Please, let me know if this is good approach, especially all the XDP experts. Maybe there are similar problems that I am not aware of? This patch.. So we end up with the user which has to properly implement its bpf xdp redirect loader to pass the proper number of queues to the RX interface. Even with all the above changes in the kernel and its interfaces he still might not know that something is wrong with his configuration/code. Thus, even then information added in this patch might be useful. At least that is what I think.