linuxkpi: Linux KPI shim, moved out of OFED

[dumbbell]

Summary

The goals are:

1. Move the current Linux shim from OFED to a central place, so it can be used by other drivers such as the video drivers.
2. Make it possible to support multiple versions of Linux, to be able to gradually upgrade drivers, when the shim is updated.

Symbols are prefixed with linuxkpi${linux_version}_. A Coccinelle script is used to automate a large part of this prefixing, but manual edits are required.

Currently, the version is set as Linux 3.15 but it is arbitrary. I need confirmation from the developers of the original shim in OFED.

Test Plan

What I did is to build an amd64 kernel and tried to kldload iw_cxgb to verify symbols were resolved. I also checked symbols using nm(1) to ensure no Linux symbols were outside of the linuxkpi namespace.

[• hselasky]

I'll do a proper review once I'm back to work next week.

BTW: Does this patch include all the latest changes made in the sys/ofed in 11-current ?

[dumbbell]

Hi @hselasky!

Yes, it includes the latest changes. The last one should be rS285088.

[• hselasky]

That sounds good. --HPS

[markj]

sys/compat/linuxkpi/3.15/src/idr_3.15.c

91

Is this correct? I'd think it's unnecessary to prefix a static function, and other static functions in this file are not prefixed.

[adrian]

Hm, what else should be tested with the change here? any other ofed consumers?

[dumbbell]

sys/compat/linuxkpi/3.15/src/idr_3.15.c

91

idr_remove_layer ends up being exported:

00000000000021a0 t linuxkpi315_idr_remove
00000000000020e0 t linuxkpi315_idr_remove_all
0000000000002140 t linuxkpi315_idr_remove_layer

Other static functions are inline, maybe this one should be inline as well.

[markj]

I'm porting this change to Isilon's OS so that I can test Infiniband. I'll post an update if any problems come up.

sys/compat/linuxkpi/3.15/src/idr_3.15.c

91

Lowercase 't' there means it's a local symbol. All static functions in the KLD will appear this way. I'm not sure why externally-visible functions (like idr_remove) in KLDs are also local, but I can't find any KLD that contains a global function symbol. So it seems that the symbol table can't be used to determine whether a function is "exported" or not?

I don't know if this function can be inlined - it's recursive.

[• hselasky]

Changes are OK by me, given you address Mark's comments.

I see some files are missing $FreeBSD$ ID tags, I guess you'll see when you try to commit.

Thank you for spending the time to separate the code.

Also I would like to run a style script on the sources after the move of the files. What do you think?

[• hselasky]

Test plan: Build kernel "WITH_OFED=YES". kldload mlx4en

--HPS

[jhb]

sys/compat/linuxkpi/3.15/include/linux/_linuxkpi_shim.h

11

I wonder if this shouldn't instead be:

#define LINUXKPI_PREFIXED_SYM(sym)     LINUXKPI_VERSIONED_PREFIXED_SYM(sym, LINUXKPI_VERSION_MAJOR, LINUXKPI_VERSION_MINOR)

?

This would mean that changing the VERSION_MAJOR/MINOR would automatically affect the default kpi symbols in one place (so less error prone if that is the desired effect).

It might help to know what use cases you expect for the VERSION_MAJOR/MINOR macros as well as when you think they should be bumped (and what effect bumping them should have?)

[markb_mellanox.com]

sys/modules/Makefile

533–534

Why leave it under OFED?

[dumbbell]

sys/compat/linuxkpi/3.15/include/linux/_linuxkpi_shim.h

11

The goal is to allow multiple versions of the KPI to leave together in the same module, at least during migration, because all consumers might not be able to move at the same time.

The KPI version in this file (.../3.15/.../_linuxkpi_shim.h) is not meant to be bumped. Instead, a new directory (eg. .../4.0/) is created (perhaps copied from a previous version). Consumers then indicate explicitely which version they want:

CFLAGS+= -I${.CURDIR}/../../compat/linuxkpi/3.15/include -I${.CURDIR}/../../compat/linuxkpi/common/include

As for the use of multiple macros instead of just the line you suggest, IIRC, it is because macros passed as arguments (here, LINUXKPI_VERSION_MAJOR and LINUXKPI_VERSION_MINOR) were not expanded. My preprocessor-fu was too weak to find a clean solution, so I repeated the integers.

I can get rid of the LINUXKPI_PREFIXED_SYM_3_15 intermediary macro, it's a left over from the WIP.

sys/compat/linuxkpi/3.15/src/idr_3.15.c

91

I'm going to remove the prefix to static symbols.

sys/modules/Makefile

533–534

Initially, to avoid the build of a module without consumers currently. But I'm ok to always build it.

[dumbbell]

Remove prefix from all static symbols

While here, remove the intermediary LINUXKPI_PREFIXED_SYM_3_15 macro.

[markb_mellanox.com]

sys/modules/Makefile

533–534

Seeing as your idea is to standardize the compat layer, I think it's best to disconnect it from the OFED stack.
Note that you already did it at: sys/conf/files.

[jhb]

I haven't reviewed this in detail, but I think the idea sounds ok.

sys/compat/linuxkpi/3.15/include/linux/_linuxkpi_shim.h

12

Ok. I had missed the directory location bit, thanks for explaining!

I think it is fine to duplicate the numbers though the expansion should actually work in this case (sometimes you have to do a double expansion see, e.g. how __CONCAT is implemented in sys/cdefs.h.

[dumbbell]

Integrate r286418 from HEAD

Improve the LINUXKPI_* macro to avoid any copies of the version numbers.

[dumbbell]

@markj: I removed the ifdef OFED in sys/modules/Makefile. I also integrated your last change in rS286418.

@jhb: Thanks, I used double-expansion and it works!

[dumbbell]

sys/kern/subr_taskqueue.c should not be part of this patch

[markj]

This change is difficult to review properly since every file in the compat layer was moved, so I can't see what actually changed. IMHO it should have been split into two reviews even if the result is to be committed as a single change. But overall I think it's ok.

[dumbbell]

Update after rS287440

[dumbbell]

Fix build in device_3.15.c; an include was missing

[• hselasky]

See work in progress on D3835 .

[• hselasky]

Hi,

Can this patch be updated to match r290135?

[• hselasky]

Hi,

I think the goal of this review should be changed into something like:

Add support for multiple version of the Linux KPI.

Because after r290135 the LinuxKPI is now out of OFED.

[dumbbell]

This must be recreated.