Hey,
On Thu, Mar 10, 2022 at 06:59:37AM +0000, Schmidt, Adriaan wrote:
> Evgeni Golov, 18. Februar 2022 12:49:
> > On Fri, Feb 18, 2022 at 12:34:49PM +0100, Evgeni Golov wrote:
> > > Hi Adriaan,
> > >
> > > I was looking at this for the next tuned upload, and have a few
> > > questions.
> > >
> > > On Thu, May 20, 2021 at 01:27:01PM +0000, Schmidt, Adriaan wrote:
> > > > Paths related to grub (required by the bootloader plugin):
> > > >
> > > > diff --git a/tuned/consts.py b/tuned/consts.py
> > > > index 733ad51..f0acf9e 100644
> > > > --- a/tuned/consts.py
> > > > +++ b/tuned/consts.py
> > > > @@ -24,7 +24,7 @@ ERROR_THRESHOLD = 3
> > > >
> > > > # bootloader plugin configuration
> > > > BOOT_DIR = "/boot"
> > > > -GRUB2_CFG_FILES = ["/etc/grub2.cfg", "/etc/grub2-efi.cfg"]
> > > > +GRUB2_CFG_FILES = ["/boot/grub/grub.cfg"]
> > >
> > > This is the *generated* file, right?
> > > So when the user regenerates it (e.g. by installing a new kernel) all
> > > changes are wiped, until tuned detects that?
> > >
> > > Sounds like a source for possible confusion. How do you handle this in
> > > your environment?
> > >
> > > (And you are aware of
> > > https://github.com/redhat-performance/tuned/pull/387,
> > > it seems)
> >
> > I just realized, on EL-systems, those /etc paths are symlinks to the
> > generated file in /boot anyways. So at least the expirience is identical
> > here.
>
> Sorry, it took me a while to look into this.
After it took me almost a year to pick up your original report? No need
to be sorry :)
> I did not run any systematic tests,
> but looking at the source, there is code to modify both the generated grub.cfg
> and /etc/default/grub, so I'm assuming kernel updates and externally triggered
> calls to update-grub should be fine.
Good enough for me, thanks :)
> And I just noticed that I have one more local change:
>
> diff --git a/tuned/plugins/plugin_bootloader.py
> b/tuned/plugins/plugin_bootloader.py
> index c3e25a6..7d5c2e6 100644
> --- a/tuned/plugins/plugin_bootloader.py
> +++ b/tuned/plugins/plugin_bootloader.py
> @@ -348,7 +348,7 @@ class BootloaderPlugin(base.Plugin):
> def _update_grubenv(self, d):
> log.debug("updating grubenv, setting %s" % str(d))
> l = ["%s=%s" % (str(option), str(value)) for option, value in
> d.items()]
> - (rc, out) = self._cmd.execute(["grub2-editenv", "-", "set"] + l)
> + (rc, out) = self._cmd.execute(["grub-editenv", "-", "set"] + l)
> if rc != 0:
> log.warn("cannot update grubenv: '%s'" % out)
> return False
Yeah, this was part of your original report, I just omited it in my
reply as that part was "obvious" :)
> I will propose something upstream to detect the executable, but for current
> releases we'd need this patch.
Yepp, sounds good!
> > > > Python bindings for perf (required by the scheduler and irqbalance
> > plugins):
> > > > This is a little more tricky, because it needs to be fixed elsewhere...
> > currently these plugins simply fail when trying to "import perf". The
> > required module is part of the kernel sources, and is currently not
> > packaged.
> > > > Two things are required:
> > > > * The package linux-perf needs to ship the binding itself (perf.so)
> > > > * A wrapper is needed to select the correct version based on the
> > running kernel, same as for the "perf" executable, where this wrapper is
> > located in package linux-base
> > > > For Linux 4.19, we posted a patch (https://bugs.debian.org/cgi-
> > bin/bugreport.cgi?bug=860957#10), for 5.10 we have something similar which
> > we'd be happy to contribute.
> > >
> > > I recall posting this bug, yeah.
> > > Sadly, I don't see much that can be done here from the tuned side, until
> > > the kernel packages aren't adjusted :(
>
> Actually, there is some movement:
> https://salsa.debian.org/kernel-team/linux/-/merge_requests/425
>
> https://salsa.debian.org/kernel-team/linux/-/commit/91c110c0cfaa5366b880382f86a84a03d8011ffd
>
> This removes the versioning of perf and the need for wrappers, which makes
> it easier to package the Python bindings. The bindings themselves are still
> not included, but we're working on it...
Awesome!
> > > > Systemd unit file (tuned.service):
> > > > Currently passes -P to have tuned write its own PID file, and -l to have
> > tuned write its own log file.
> > > > Wouldn't it be better practice, to
> > > > * remove -P, as systemd will take care of the PID file
> > >
> > > I'd rather argue this is just default behaviour?
> > > https://github.com/redhat-
> > performance/tuned/commit/9520364fcae362e7181cd1057591054e3407c756
> > > https://github.com/redhat-
> > performance/tuned/blob/dc8808cb394e52e0d11c7d7b3a53264421d21d47/tuned.py#L77-
> > L79
> > >
> > > > * remove -l, and have systemd direct tuned's stdout to the journal
> > >
> > > Can you propose those changes upstream? They do seem to make sense, but
> > > I'd prefer not to diverge from upstream unless really necessary.
>
> You are right. I must have read things wrong and assumed the service file was
> part of the Debian packaging. Let's keep it the way it is.
Thanks
Evgeni
