Hi, Thanks for the detailed explanation of your testing procedure! It is good to know what is behind a local patched driver, just in case.. :)
Best regards, Natanael. On 6 August 2016 at 16:20, David Haller <[email protected]> wrote: > Hello, > > On Sat, 06 Aug 2016, Natanael Olaiz wrote: > >Thank you for your patch. It was a good example to answer my question. > > > >But about the patch itself, I see that you are commented the code for > >radix_tree_empty(...). In my patch I renamed it and it only usage instead, > >so I'm sure it's calling the same code. I don't know the expected > >compatibility with the kernel function implementation... But without > >knowing the specific code for neither the nvidia driver nor the kernel, I > >think the rename is safer... > > If you're in doubt or hit any trouble, yes, definitely change/adapt > the patch to only rename that function and use the renamed function in > the nvidia-driver as in your original patch. Keep/adapt the stuff > about kernel-versions though. That's the "safe" approach. > > That "just put it in /etc/portage/patches/..." should work tough :) > > > I've looked quite sharp at the code, i.e. in the nvidia code > > static bool radix_tree_empty(struct radix_tree_root *tree) > { > void *dummy; > return radix_tree_gang_lookup(tree, &dummy, 0, 1) == 0; > } > > vs. the kernel function > > static inline bool radix_tree_empty(struct radix_tree_root *root) > { > return root->rnode == NULL; > } > > *oik* I miss a check for root != NULL there ;) > > Anyway, it was quite clear, that the driver calls kernel-stuff at this > point anyway, radix_tree_gang_lookup() is a kernel function. (I love > to use mc for digging for stuff like this!) > > So, digging into the kernel-source I came to the conclusion that > calling the _kernel code_(!) > > radix_tree_gang_lookup(tree, &dummy, 0, 1); > > actually _is_ (or SHOULD BE) equivalent to the kernel code for > the new > > radix_tree_empty(tree); > > and the latter should be faster (unless the compiler optimizes the > 'radix_tree_gang_lookup(root, &dummy, 0, 1)' call away). > > All this applies if and only if the last two arguments of > radix_tree_gang_lookup() are 0 and 1! And yes, I did go through the > code of radix_tree_gang_lookup() step by step (repeatedly) until I was > sure enough that the code is equivalent). But I'm not a C guru. I > might have missed something even important. So, if more guys firm in C > can check this ... > > As I'm a guy generally trusting the kernel guys _a lot_ and that > nvidia assumedly got it right implementing 'radix_tree_empty(tree)' > via 'radix_tree_gang_lookup(tree, &dummy, 0, 1);' and I think those > two equivalent (with 0, 1 being the 3rd and 4th parameters!), I tried > it, and it worked. > > And Meino has not yet complained. AFAIR he's around long enough to > have complained by now (and circumvented problems with the patch). > I'll ping him on the -user ML though in a parallel mail. > > Recap: calling the kernel function in this way (via the nvidia function) > > radix_tree_gang_lookup(tree, &dummy, 0, 1); > > is IMHO equivalent to calling the kernel function > > radix_tree_empty(tree); > > and on this I based my patch on. I'm rather sure from looking at the > code. It worked in a short test. Meino has not complained yet. If you > want a "sure"/"safe" approach, go with renaming the function in the > nvidia-code, or wait a bit if Meino pipes up, he should've been > running the driver with my patch for a week by now. Or until an > official patch crops up. > > Or, take precautions, some other way to boot+chroot, and keep the > "old" driver handy or to disable it, build a binary package of the > previous driver, have an older kernel ready etc. pp., you know the > drill, don't you? > > -dnh > > -- > MCSE: "Microsoft Certified Stupidity enclosed" -- A. Spengler > >
