diff mbox series

[net-next,5/6] i40e: Add info trace at loading XDP program

Message ID 20210202022420.1328397-6-anthony.l.nguyen@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series 40GbE Intel Wired LAN Driver Updates 2021-02-01 | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 2 maintainers not CCed: jesse.brandeburg@intel.com intel-wired-lan@lists.osuosl.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 4 this patch: 4
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 15 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Tony Nguyen Feb. 2, 2021, 2:24 a.m. UTC 
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).

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski Feb. 3, 2021, 2:33 a.m. UTC | #1 
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.
Kubalewski, Arkadiusz Feb. 3, 2021, 10 a.m. UTC | #2 
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.
Jakub Kicinski Feb. 3, 2021, 6:32 p.m. UTC | #3 
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.
Kubalewski, Arkadiusz Feb. 3, 2021, 11:26 p.m. UTC | #4 
>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.
diff mbox series

Patch

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");
 		for (i = 0; i < vsi->num_queue_pairs; i++)
 			if (vsi->xdp_rings[i]->xsk_pool)
 				(void)i40e_xsk_wakeup(vsi->netdev, i,
 						      XDP_WAKEUP_RX);
+	}
 
 	return 0;
 }