AMD 10GbE driver changes for Netmap support

[rajesh1.kumar_amd.com]

Summary

AMD 10GbE hardware is designed to have two buffers per receive descriptor to
support split header feature. For this purpose, the driver was designed to use
2 iflib freelists per receive queue. So, that buffers from 2 freelists are used
to refill an entry in the receive descriptor. The current design holds good with
regular data traffic.

But, when netmap comes into play, the current design will not fit in. The current
netmap interfaces and netmap implementation in iflib doesn't seem to accomodate the
design of 2 freelists per receive queue. So, exercising Netmap capability with
inbuilt tools like bridge, pkt-gen doesn't work with the 2 freelists driver design.

So, the driver design is changed to accomodate the current netmap interfaces and
netmap implementation in iflib by using single freelist per receive queue approach
when Netmap capability is exercised without disturbing the current 2 freelists
approach.

Thanks to Stephan Dewt for his Initial set of code changes for the stated problem.

Test Plan

As part of the driver design change, two "kenv" variable and associated sysctl's per
device instance has been added now.

dev.ax.netmap_test (default to 0)

When dev.ax.netmap_test is set to 1, AMD 10GbE device driver is configured to use
the single freelist per receive queue. Otherwise, the driver will stick to the
current design of 2 freelists per receive queue.

So, if user want to try netmap application on top of AMD hardware/driver, this
kenv variable can be used.

Another "kenv" variable is,

dev.ax.sph_enable (default to 1)

May it be netmap test or other tests, if user wants to try with split header
feature turned off, this kenv variable can be used.

When dev.ax.sph_enable is set to 1, Split header feature is enabled in hardware.
Otherwise, split header feature is disabled.

For information, both these variables needs to be set manually before loading the
driver or from /boot/loader.conf to take into effect in the next boot.

[stephan.dewt_yahoo.co.uk]

I should mention that this change also includes two bugfixes:

1. Promiscuous mode not being set properly. (thanks Rajesh)
2. A refill clash error when increasing the descriptor count via the iflib sysctl "override_nrxds", since the driver defaults to a (small) descriptor count of 512 (in comparison to other 10G NICs).

[vmaffione]

Are both netmap_test and sph_enable actually necessary? Would it make sense to have only sph_enable, and set isc_nfl = 1 when sph_enable = 0 and isc_nfl = 2 when sph_enable = 1?
If not, I think netmap_test should be called something like single_free_list

[stephan.dewt_yahoo.co.uk]

In D27797#625707, @vmaffione wrote:

Are both netmap_test and sph_enable actually necessary? Would it make sense to have only sph_enable, and set isc_nfl = 1 when sph_enable = 0 and isc_nfl = 2 when sph_enable = 1?
If not, I think netmap_test should be called something like single_free_list

I agree, both sysctl variables now depend on each other. I will create a change and submit it for review here once I find the time.

[rajesh1.kumar_amd.com]

Are both netmap_test and sph_enable actually necessary? Would it make sense to have only sph_enable, and set isc_nfl = 1 when sph_enable = 0 and isc_nfl = 2 when sph_enable = 1?
If not, I think netmap_test should be called something like single_free_list

I agree, both sysctl variables now depend on each other. I will create a change and submit it for review here once I find the time.

@stephan.dewt_yahoo.co.uk, I have already made the changes for this and about to submit it.

@vmaffione, I feel it's better to retain both the variables, since it will give some flexibility to the user to try disabling split header even with non-netmap cases if needed. But as you suggested, I have modified the "netmap_test" variable to "single_fl".

But if you people see no usage for the same, we shall remov it.

[rajesh1.kumar_amd.com]

V2 patch addressing the previous comments

. Modified "netmap_test" sysctl variable to "single_fl"
. Retained "sph_enable" sysctl variable for user flexibility

[stephan.dewt_yahoo.co.uk]

@rajesh1.kumar_amd.com Thanks for the patch. It should be noted that disabling the split header function now requires the single_fl to be set. If that's acceptable, I suggest updating the manpage to explicitly document this, as right now it's documented as "recommended".

[stephan.dewt_yahoo.co.uk]

@rajesh1.kumar_amd.com Also, the sysctl's need to be changed to be interface-specific if we are deciding to use the "dev.ax.X.sph_enable" format for increased flexibility. See https://reviews.freebsd.org/D27800

[vmaffione]

In D27797#626328, @rajesh1.kumar_amd.com wrote:

Are both netmap_test and sph_enable actually necessary? Would it make sense to have only sph_enable, and set isc_nfl = 1 when sph_enable = 0 and isc_nfl = 2 when sph_enable = 1?
If not, I think netmap_test should be called something like single_free_list

I agree, both sysctl variables now depend on each other. I will create a change and submit it for review here once I find the time.

@stephan.dewt_yahoo.co.uk, I have already made the changes for this and about to submit it.

@vmaffione, I feel it's better to retain both the variables, since it will give some flexibility to the user to try disabling split header even with non-netmap cases if needed. But as you suggested, I have modified the "netmap_test" variable to "single_fl".

But if you people see no usage for the same, we shall remov it.

There are four cases:

• sph_enable = 1, single_fl = 1 --> I guess not possible, because if split header is in use, you need two free lists (right?)
• sph_enable = 1, single_fl = 0 --> ok, normal case
• sph_enable = 0, single_fl = 1 --> ok, necessary for using netmap, but it can also be used with non-netmap applications (right?)
• sph_enable = 0, single_fl = 0 --> not possible, according to @stephan.dewt_yahoo.co.uk

I may be wrong on anything, but if this is the actual situation, one of the variables is redundant, and can be removed.

[rajesh1.kumar_amd.com]

For now, I will better remove the "single_fl" sysctl and retain just the "sph_enable" and the changes (without any iflib changes) to get netmap bridging to work. Thanks @stephan.dewt_yahoo.co.uk for those changes. So, the next patch will be a driver only patch without any need for iflib changes.

@stephan.dewt_yahoo.co.uk

The sysctl is already interface specific only right? It will be set based on the the "kenv" variable to the same value for both interfaces. Else, if user wishes to configure it per interface, he can use the sysctl directly rather than kenv for this. Please clarify if any more changes needed with respect to sysctl. I will address that and submit the next patch.

@vmaffione

sph_enable = 1, single_fl = 1 --> I guess not possible, because if split header is in use, you need two free lists (right?)

This case also possible. In fact, I have done some iperf and netmap bridge tests with this case with the current patch here. In this case, we use two iflib descriptor for one hardware descriptor

sph_enable = 0, single_fl = 1 --> ok, necessary for using netmap, but it can also be used with non-netmap applications (right?)

Yes, I have tested iperf with this case. But, this is the necessary case for Netmap testing.

sph_enable = 0, single_fl = 0 --> not possible, according to @stephan.dewt_yahoo.co.uk

This case, I have done some iperf test. But needs some more testing. Anyway, this will not work for Netmap.

Overall, we need some more testing and fixes (driver and iflib) for pkt-gen to work seamlessly with axgbe and split header. For now, Netmap bridge works properly with axgbe. Once I get the clarity regarding the per interface sysctl, I will address that and submit the next patch.

[vmaffione]

Il giorno dom 10 gen 2021 alle ore 13:20 rajesh1.kumar_amd.com (Rajesh
Kumar) <[e-mail]> ha scritto:

rajesh1.kumar_amd.com added a comment.

For now, I will better remove the "single_fl" sysctl and retain just the

"sph_enable" and the changes (without any iflib changes) to get netmap
bridging to work. Thanks @stephan.dewt_yahoo.co.uk for those changes.
So, the next patch will be a driver only patch without any need for iflib
changes.

@stephan.dewt_yahoo.co.uk

The sysctl is already interface specific only right?  It will be set

based on the the "kenv" variable to the same value for both interfaces.
Else, if user wishes to configure it per interface, he can use the sysctl
directly rather than kenv for this. Please clarify if any more changes
needed with respect to sysctl. I will address that and submit the next
patch.

@vmaffione

>   sph_enable = 1, single_fl = 1 --> I guess not possible, because if

split header is in use, you need two free lists (right?)

This case also possible.  In fact, I have done some iperf and netmap

bridge tests with this case with the current patch here. In this case, we
use two iflib descriptor for one hardware descriptor

>   sph_enable = 0, single_fl = 1 --> ok, necessary for using netmap,

but it can also be used with non-netmap applications (right?)

Yes, I have tested iperf with this case. But, this is the necessary case

for Netmap testing.

>   sph_enable = 0, single_fl = 0 --> not possible, according to @

stephan.dewt_yahoo.co.uk

This case, I have done some iperf test. But needs some more testing.

Anyway, this will not work for Netmap.

Overall, we need some more testing and fixes (driver and iflib) for

pkt-gen to work seamlessly with axgbe and split header. For now, Netmap
bridge works properly with axgbe. Once I get the clarity regarding the per
interface sysctl, I will address that and submit the next patch.

Ok, thanks for the clarification.
Regarding the case sph_enable = 0 and single_fl = 0, I would like to add a
new isc_flag that you can set to tell iflib that you don't want to allow
netmap on this driver. In this way you can set the flag in case sph_enable

single_fl = 0, so that users won't do mistakes.

It would be even better to do this configuration per interface, but I did
not understand if the sph_enable/single_fl are going to be per interface or
not.

REPOSITORY

rS FreeBSD src repository - subversion

CHANGES SINCE LAST ACTION

https://reviews.freebsd.org/D27797/new/

REVISION DETAIL

https://reviews.freebsd.org/D27797

EMAIL PREFERENCES

https://reviews.freebsd.org/settings/panel/emailpreferences/

To: rajesh1.kumar_amd.com, shurd, iflib, mmacy, manu,
stephan.dewt_yahoo.co.uk, vmaffione
Cc: imp, ae, melifaro, Contributor Reviews (src),
krzysztof.galazka_intel.com

[rajesh1.kumar_amd.com]

V3 patch with following changes

. Removed "single_fl" sysctl for now and retain only "sph_enable" sysctl
. Changed the netmap support to not involve any iflib changes.

[rajesh1.kumar_amd.com]

Regarding the case sph_enable = 0 and single_fl = 0, I would like to add a
new isc_flag that you can set to tell iflib that you don't want to allow
netmap on this driver. In this way you can set the flag in case sph_enable
single_fl = 0, so that users won't do mistakes.

I will work on this comment on a seperate patch as it involves changes in iflib code as well.

It would be even better to do this configuration per interface, but I did
not understand if the sph_enable/single_fl are going to be per interface or
not.

Actually, the sysctl's are already interface specific only. User can either set the global "kenv" dev.ax.sph_enable to configure sph on both interface. Or they can use the interface specific "sysctl" dev.ax.X.sph_enabled to configure the same specific to interface.

For now, as mentioned earlier, modified this patch to be driver specific and get netmap bridging works with the driver. Please let me know if any comments.

[stephan.dewt_yahoo.co.uk]

Actually, the sysctl's are already interface specific only. User can either set the global "kenv" dev.ax.sph_enable to configure sph on both interface. Or they can use the interface specific "sysctl" dev.ax.X.sph_enabled to configure the same specific to interface.

For now, as mentioned earlier, modified this patch to be driver specific and get netmap bridging works with the driver. Please let me know if any comments.

When changing this via the sysctl interface (e.g. # sysctl dev.ax.0.sph_enabled=1), it returns with

sysctl: oid 'dev.ax.0.sph_enabled' is a read only tunable
sysctl: Tunable values are set in /boot/loader.conf

Which is fair, but when setting

dev.ax.0.sph_enable=0
dev.ax.1.sph_enable=1

in /boot/loader.conf, both interfaces still make use of the split header (as confirmed in dmesg/boot). There is no code in this patch to handle this as far as I can see.

ax0: Split header enabled, using 1 free list(s) and 1 RX queue set(s)
ax1: Split header enabled, using 1 free list(s) and 1 RX queue set(s)

By the way, I also tried

dev.ax.0.sph_enabled=0
dev.ax.1.sph_enabled=1

[rajesh1.kumar_amd.com]

When changing this via the sysctl interface (e.g. # sysctl dev.ax.0.sph_enabled=1), it returns with

sysctl: oid 'dev.ax.0.sph_enabled' is a read only tunable
sysctl: Tunable values are set in /boot/loader.conf

Yes, This is a read only tunable. This is because, modifying them after the driver load doesn't have a meaning. This tunable is needed during the driver load to enable/disable the Split header feature.

dev.ax.0.sph_enable=0
dev.ax.1.sph_enable=1

in /boot/loader.conf, both interfaces still make use of the split header (as confirmed in dmesg/boot). There is no code in this patch to handle this as far as I can see.

ax0: Split header enabled, using 1 free list(s) and 1 RX queue set(s)
ax1: Split header enabled, using 1 free list(s) and 1 RX queue set(s)

This log is not there in the patch submitted here. This looks like your custom logs. Setting them from /boot/loader.conf is taking effect with the submitted patch. This can be verified with the logs from xgbe-dev.c (xgbe_config_sph_mode) function. For the interface which has "sph_enabled", you can see the "SPH Enabled" message, but not for the interface which has the sysctl set to 0.

When you set the following in /boot/loader.conf

dev.ax.0.sph_enabled=0
dev.ax.1.sph_enabled=1

You will see the following in dmesg,

ax1: SPH Enabled

By the way, I also tried

dev.ax.0.sph_enabled=0
dev.ax.1.sph_enabled=1

Looks like it's the same combination above. I assume it's a typo and you attempted the other combination as well.

Please let me know if you still face any issue.

[rajesh1.kumar_amd.com]

V4 patch with a minor change to have both kenv and sysctl name to be same as
"sph_enable"

[rajesh1.kumar_amd.com]

@vmaffione

If there is no other comments, can this patch be accepted upstream?

[vmaffione]

I added some comments, mostly about readability.
If you tested all the valid combinations of sph_enable and netmap/non-netmap I don't have any objection in this being merged.

sys/dev/axgbe/if_axgbe_pci.c

316

This could be made shorter and more readable with something like:

`
for (i = 0; i < isc_nrxqs; i++) {
    isc_nrxd_min[i] = XGBE_RX_...;
    isc_nrxd_default[i] = XGBE_...;
    isc_nrxd_max[i] = XGBE_...;
}
`

1974

This looks correct (because IFLIB_INTR_TX is handled below), but then why was it like that? A simple mistake?

sys/dev/axgbe/xgbe-sysctl.c

1621

I don't think we should mention /boot/loader.conf and dev.ax.XXX here, also because the procedure is valid also for the other tunables around this code.

Maybe just mention that 1 is enable and 0 is disable?

sys/dev/axgbe/xgbe-txrx.c

477–478

This comment should go away.

505

If you are putting the same physical address in both address pointers, wouldn't the hardware use the same buffer twice (therefore overwriting?). Or maybe since sph is disabled, the second buffer pointer is actually never used by the hardware?

552–553

We could avoid code duplication, by using a combined condition
if (!pdata->sph_enable || flidx == 1)

553–558

Out of curiosity: shouldn't the write memory barrier be executed only if we actually IOWRITE?

750

We could avoid code duplication by executing line 760-761 inconditionally, and lines 762-763 if !sph_enable.

764

same as above

[rajesh1.kumar_amd.com]

@vmaffione

If you tested all the valid combinations of sph_enable and netmap/non-netmap I don't have any objection in this being merged.

I verified iperf working with sph enabled and disabled. And verified Netmap briding with sph disabled.

sys/dev/axgbe/if_axgbe_pci.c

1974

Recently, IFLIB_INTR_RX is changed to IFLIB_INTR_RXTX as part of D27683.

I verified it doesn't break things. But need to see whether softirq for TX is still needed. If needed will change and submit as separate review. For now, retaining the change from the above review as IFLIB_INTR_RXTX.

sys/dev/axgbe/xgbe-txrx.c

505

Yes. In this case, the second buffer pointer will not be used by hardware since the split header is disabled.

553–558

Removed it. As wmb is done already in rxd_refill.

[rajesh1.kumar_amd.com]

V5 patch addressing all the previous comments.

[rajesh1.kumar_amd.com]

sys/dev/axgbe/xgbe-txrx.c

553–558

Missed to mention. Inline with this, similar change has been done in txd_flush routine as well. Also removed an unwanted check in txd_flush routine.

[vmaffione]

Looks good to me.

sys/dev/axgbe/xgbe-txrx.c

505

Ok, so we could perform less operations (and shorten the code):

`
rdesc->desc0 = rdesc->desc2 = ...;
rdesc->desc1 = rdesc->desc3 = ...;
`

[rajesh1.kumar_amd.com]

V6 patch addressing the last comment

[rajesh1.kumar_amd.com]

I see "iflib" group review is blocking here. Now that this patch is only driver specific and doesn't have any change in iflib, does this patch need any other action to get it pushed upstream?

[gallatin]

In D27797#629663, @rajesh1.kumar_amd.com wrote:

I see "iflib" group review is blocking here. Now that this patch is only driver specific and doesn't have any change in iflib, does this patch need any other action to get it pushed upstream?

Yes, I think you should feel free to push it.

[vmaffione]

Shall I commit this change?

[rajesh1.kumar_amd.com]

Shall I commit this change?

Yes @vmaffione. I am not sure whether I have the write access to the git repo. Thank you.

[rajesh1.kumar_amd.com]

Thanks @vmaffione for committing the changes.