PlatformLab / HomaModule

A Linux kernel module that implements the Homa transport protocol.
175 stars 43 forks source link

Makefile and Python fixes #44

Closed joft-mle closed 1 year ago

joft-mle commented 1 year ago

Hi Team HOMA, hi John,

attached is a small Pull Request with changes for topics we stumbled over while experimenting.

johnousterhout commented 1 year ago

Thanks for the pull request. I have a few questions, mostly for my own edification:

-John-

joft-mle commented 1 year ago

Hi @johnousterhout ,

again, I'm sorry for the huge delay in answering.

Thanks for the pull request. I have a few questions, mostly for my own edification:

  • What's the purpose of the new line "ifneq ($(KERNELRELEASE),)" in Makefile?

The conditional check on KERNELRELEASE essentially checks for the current context - whether the Makefile (called a "Shared Makefile", see [1]) is run from within the Linux kernel's build system, or whether it is run in direct relation to a user's make invocation. So the checks is basically to "separate" the two contexts, thus "Shared Makefile".

[1] https://www.kernel.org/doc/html/latest/kbuild/modules.html#shared-makefile

There is one more thing I found to be "solved" by this commit in the meantime - compared to what I listed in commit message: Constructing the Makefile the way the commit does, makes sure sudo make install works. With the current state of the Makefile, I seem to always end up with a broken /lib/modules/<ver>/modules.dep file after doing make install - at least when not signing the module. Very strange effect, which I cannot explain.

  • The "continue" in homa_incoming.c appears to me to be unnecessary; did you add it for clarification, or does it serve some essential purpose?

This kind of construct ("label at end of compound statement") is apparently only supported in "newer" GCC versions. I did not check which version exactly started to accept it. I can just tell that GCC v7.5 obviously does not support this. As listed in the commit message, GCC v7.5 complains with

$ make
make -C /lib/modules/6.1.0-10-amd64/build M=$WORKDIR/HomaModule modules
make[1]: Entering directory '/usr/src/linux-headers-6.1.0-10-amd64'
  CC [M]  $WORKDIR/HomaModule/homa_incoming.o
$WORKDIR/HomaModule/homa_incoming.c: In function ‘homa_choose_rpcs_to_grant’:
$WORKDIR/HomaModule/homa_incoming.c:1029:5: error: label at end of compound statement
     next_rpc:
     ^~~~~~~~
  • Why is the "list(clients)" needed in cperf.py? Are there situations where clients might not already be a list?

The script utils/cp_basic regularly calls run_experiment() with range objects (example), which are apparently cast'able to lists, but such casts are not happening automatically - at least not within the Python versions I specified in the commit (v3.8, v3.10, v3.11). Which version are you using?

johnousterhout commented 1 year ago

I finally got around to merging this pull request; sorry it took me so long.