Page MenuHomeFreeBSD

Modularize xz.
ClosedPublic

Authored by kib on Feb 20 2019, 10:23 AM.

Details

Summary

Embedded lzma decompression library becomes a module usable by other consumers, in addition to geom_uzip.

Most important code changes are

  • removal of XZ_DEC_SINGLE define, we need the code to work with XZ_DEC_DYNALLOC
  • xz_crc32_init() call is removed from geom_uzip, xz module handles initialization on its own.

xz is no longer embedded into geom_uzip, instead the depend line for the module is provided, and corresponding kernel option is added to each MIPS kernel config file using geom_uzip.

Sponsored by: Mellanox Technologies

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 22615

Event Timeline

kib created this revision.Feb 20 2019, 10:23 AM
hselasky accepted this revision.Feb 20 2019, 10:37 AM

Looks good to me.

This revision is now accepted and ready to land.Feb 20 2019, 10:37 AM
ray accepted this revision.Feb 20 2019, 10:56 AM

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

kib added a comment.Feb 20 2019, 11:47 AM
In D19266#412252, @ray wrote:

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

Both module, module build directory, and linker file are named xz. What part name don't you like ?

slavash accepted this revision.Feb 20 2019, 11:55 AM

Looks good to me.

In D19266#412263, @kib wrote:
In D19266#412252, @ray wrote:

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

Both module, module build directory, and linker file are named xz. What part name don't you like ?

I think he meant the kernel option name.

ray added a comment.Feb 20 2019, 12:28 PM
In D19266#412263, @kib wrote:
In D19266#412252, @ray wrote:

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

Both module, module build directory, and linker file are named xz. What part name don't you like ?

device xz_embedded

kib added a comment.Feb 20 2019, 12:55 PM
In D19266#412271, @ray wrote:
In D19266#412263, @kib wrote:
In D19266#412252, @ray wrote:

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

Both module, module build directory, and linker file are named xz. What part name don't you like ?

device xz_embedded

This is how it called now. Look at sys/conf/files, where xz is under xz_embedded. In fact there is a missed bit of the patch, I will rename it to xz together with the rename.

kib updated this revision to Diff 54134.Feb 20 2019, 1:14 PM

Actually include the chunk which removes xz files from under geom_uzip when compiled into kernel.
Rename xz_embedded to xz, as requested by ray.

This revision now requires review to proceed.Feb 20 2019, 1:14 PM
ray accepted this revision.Feb 20 2019, 1:27 PM

Thanks!

This revision is now accepted and ready to land.Feb 20 2019, 1:27 PM
hselasky accepted this revision.Feb 20 2019, 1:58 PM
cem accepted this revision.Feb 21 2019, 12:58 AM
sobomax requested changes to this revision.EditedFeb 21 2019, 1:37 AM

Looks good overall. I am little bit puzzled over usage of "option GEOM_UZIP" vs. "device geom_uzip + device xz". I suspect those are functionally equivalent. Half of the kernel configs in the tree use former (including NOTES) and half the latter. Some subset of the two use both. :( Since you are touching those files anyways, perhaps you can go and convert all cases of "device geom_uzip" into plain "option GEOM_UZIP" and not patch it up by adding device xz? It might be also a good idea to adjust geom_uzip(4) accordingly to provide "One-True-Way" to compile geom_uzip(4) statically into the kernel? Thanks!

This revision now requires changes to proceed.Feb 21 2019, 1:37 AM

Also "device xz" needs to be added to the conf/NOTES.

kib added a comment.Feb 21 2019, 1:57 PM

Looks good overall. I am little bit puzzled over usage of "option GEOM_UZIP" vs. "device geom_uzip + device xz". I suspect those are functionally equivalent. Half of the kernel configs in the tree use former (including NOTES) and half the latter. Some subset of the two use both. :( Since you are touching those files anyways, perhaps you can go and convert all cases of "device geom_uzip" into plain "option GEOM_UZIP" and not patch it up by adding device xz? It might be also a good idea to adjust geom_uzip(4) accordingly to provide "One-True-Way" to compile geom_uzip(4) statically into the kernel? Thanks!

option GEOM_UZIP/device geom_uzip has nothing to do with my patch, this redundancy is already present and cleaning it up is unrelated. Apparently the same redundancy is there with GEOM_MAP. It seems that all geom classes added using options and not device. Also I do not see it would allow me to avoid adding device xz.

I removed device geom_uzip from configs which I touch, but really I should not.

kib updated this revision to Diff 54180.Feb 21 2019, 1:59 PM

Remove 'device geom_uzip', add xz to notes.

ray accepted this revision.Feb 21 2019, 2:46 PM
sobomax requested changes to this revision.Feb 22 2019, 2:59 AM
In D19266#412644, @kib wrote:

Remove 'device geom_uzip', add xz to notes.

Thanks for cleaning the mess, it looks better now however some further work is needed I think to either document new dependency in the share/man/man4/geom_uzip.4 or maybe get rid of the need to explicitly specify "device xz" when option GEOM_UZIP is given? geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different? For once xz(4) has no other consumers in the kernel yet, while zlib certainly does, so it should be the reference model IMHO.

[sobomax@pioneer ~/projects/FreeBSD/head]$ find sys/ -type f | xargs grep -w zlib | grep MODULE_DEPEND
sys/netgraph/ng_deflate.c:MODULE_DEPEND(ng_deflate, zlib, 1, 1, 1);
sys/dev/mxge/if_mxge.c:MODULE_DEPEND(mxge, zlib, 1, 1, 1);
sys/geom/uzip/g_uzip.c:MODULE_DEPEND(g_uzip, zlib, 1, 1, 1);
sys/opencrypto/crypto.c:MODULE_DEPEND(crypto, zlib, 1, 1, 1);
sys/opencrypto/cryptodev.c:MODULE_DEPEND(cryptodev, zlib, 1, 1, 1);
This revision now requires changes to proceed.Feb 22 2019, 2:59 AM
cem added a comment.Feb 22 2019, 4:58 AM

geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different?

zlib is built if geom_uzip is enabled in conf/files:

libkern/zlib.c                  optional crypto | geom_uzip | ipsec | \
        ipsec_support | mxge | netgraph_deflate | ddb_ctf | gzio

(Also, zlib has many other consumers. xz-embedded has limited general utility, due to being a decompress-only subset of lzma. So I am not sure xz will gain many more consumers. I agree adding 'device xz' to a million MIPS kernel configurations is an ugly repercussion of this choice.)

kib added a comment.EditedFeb 22 2019, 11:45 AM
In D19266#412909, @cem wrote:

geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different?

zlib is built if geom_uzip is enabled in conf/files:

libkern/zlib.c                  optional crypto | geom_uzip | ipsec | \
        ipsec_support | mxge | netgraph_deflate | ddb_ctf | gzio

(Also, zlib has many other consumers. xz-embedded has limited general utility, due to being a decompress-only subset of lzma. So I am not sure xz will gain many more consumers. I agree adding 'device xz' to a million MIPS kernel configurations is an ugly repercussion of this choice.)

What you cited is clear example why I (and others) do not do this dependency tracking insanity in the conf/files syntax. Config(8) is only effective at flat dependencies where all required config options are explicitly provided in the kernel config file. Tracking dependencies like turn on B when A is on, but also consider side effects from C, D, E, architecture, and so on, is impossible with it.

I thought it is obvious that the only reason why I wrote this patch is because there are more planned consumers of xz in kernel, which alone is the reason why I want it to be a dedicated option, and why I want (need) to avoid the dependency drama.

Also see the recent discussion of iflib modularization.

That said, are there any other _technical_ notes about the diff ?

sobomax added a comment.EditedFeb 22 2019, 10:46 PM
In D19266#412957, @kib wrote:
In D19266#412909, @cem wrote:

geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different?

zlib is built if geom_uzip is enabled in conf/files:

libkern/zlib.c                  optional crypto | geom_uzip | ipsec | \
        ipsec_support | mxge | netgraph_deflate | ddb_ctf | gzio

(Also, zlib has many other consumers. xz-embedded has limited general utility, due to being a decompress-only subset of lzma. So I am not sure xz will gain many more consumers. I agree adding 'device xz' to a million MIPS kernel configurations is an ugly repercussion of this choice.)

What you cited is clear example why I (and others) do not do this dependency tracking insanity in the conf/files syntax. Config(8) is only effective at flat dependencies where all required config options are explicitly provided in the kernel config file. Tracking dependencies like turn on B when A is on, but also consider side effects from C, D, E, architecture, and so on, is impossible with it.
I thought it is obvious that the only reason why I wrote this patch is because there are more planned consumers of xz in kernel, which alone is the reason why I want it to be a dedicated option, and why I want (need) to avoid the dependency drama.
Also see the recent discussion of iflib modularization.
That said, are there any other _technical_ notes about the diff ?

Yes, you've missed my yesterday's request to correct synopsis of the static option GEOM_UZIP compilation in the geom_uzip.4 manual page in case you want to make the "device xz" necessary. See geli(8) for example:

SYNOPSIS
     To compile GEOM_ELI into your kernel, add the following lines to your
     kernel configuration file:

           device crypto
           options GEOM_ELI

Other than that I don't have any major technical problems with the patch, assuming everyone else agrees that needing to have "device xz" is a necessary evil. There are some minor things I'd leave as inline comments though.

sobomax added inline comments.Feb 22 2019, 10:55 PM
sys/conf/NOTES
3004 ↗(On Diff #54180)
compression -> de-compression
sobomax added inline comments.Feb 22 2019, 11:02 PM
sys/contrib/xz-embedded/freebsd/xz_malloc.c
54

I don't think this function and all things below (moduledata_t xz_moduledata etc) belongs to the xz_malloc.c. It clearly has nothing to do with "malloc". I'd suggest something like xz_mod.c or something.

sobomax added inline comments.Feb 22 2019, 11:12 PM
sys/contrib/xz-embedded/freebsd/xz_malloc.c
54

P.S. xz_mod.c (xz.c?) itself should probably live in the sys/dev/xz, like most other FreeBSD-specific device code. It is not contributed in any way and I don't think anyone but us would care to maintain it.

sobomax added inline comments.Feb 22 2019, 11:17 PM
sys/modules/xz/Makefile
7
KMOD=xz -> KMOD=<tab>xz
kib updated this revision to Diff 54250.Feb 23 2019, 10:39 AM

Rebase; fix conflicts.
Formatting and comment changes.

ray accepted this revision.Feb 23 2019, 10:48 PM
sobomax requested changes to this revision.EditedFeb 25 2019, 8:52 PM
In D19266#413225, @kib wrote:

Rebase; fix conflicts.
Formatting and comment changes.

Some of technical issues that I raised remain unaddressed (documentation, module-specific code placement). So the diff still needs bit more work IMHO.

This revision now requires changes to proceed.Feb 25 2019, 8:52 PM
cem accepted this revision.Feb 25 2019, 10:34 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Feb 26 2019, 7:55 PM
Closed by commit rS344605: Modularize xz. (authored by kib, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.