Re: [Crash-utility] [PATCH v2] Improve the performance of symbol searching for kernel modules

Hi Tao, great patch. I have some comments and questions though. On Sun, 22 Aug 2021 09:51:12 +0800 Tao Liu <ltao(a)redhat.com&gt; wrote: ^^^^^^^^ there are quite some whitespace damages throughout the patch. Most of them seem to origin from old code that gets copy & pasted. It would be great if you could fix them on the lines you are touching. 1. 'if (sp)' is the same like 'if (sp != NULL)' but shorter. That's why personally I prefer the first version :) 2. Wouldn't it make sense to move this check to the beginning of symname_hash_{install,remove}? Then also the other users like symname_hash_init would benefit. true, in theory index could be negative which would be really bad. But isn't that an independent problem from the rest of the patch? If so this change should go in an extra patch. Furthermore the fix should be done in SYMNAME_HASH_INDEX so that all of its users are fixed. The way I see it this should be enough (using abs from stdlib.h) #define SYMNAME_HASH_INDEX(name) \ - ((name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2])) % SYMNAME_HASH) + (abs((name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2])) % SYMNAME_HASH)) similar to above, isn't explicitly setting spn->name_hash_next = NULL an independent problem from the rest of the changes and thus should go to a separate patch? What do you need tmp for? The way I see it you only assign to it but never really use it. this line should be indented to match the open parentheses after the function name. It took really long for me to wrap my head around what is happening here but in the end I'm pretty sure that the extra filtering is unnecessary and can simply be dropped without problem. To be fair what you are doing seems correct it's just by cleaning up the code the problem became more obvious... Let's see what is happening here: 1) strstr returns a pointer to the start of the second string if is is contained in the first one and NULL otherwise. This means 'pseudos' is true if 's' contains any of the _MODULE_* strings, i.e. if s is a pseudo symbol. 2) MODULE_PSEUDO_SYMBOL does basically the same only that it checks 'sp->name' instead of 's' and enforces that the "_MODULE_*" strings are at the beginning of the symbol name not just contained in it. Let's look at the different cases 3.1) both 's' and 'sp' are proper, i.e. no pseudo, symbols This means skip_symbols returns false so symname_hash_search doesn't skip the symbol but compares 's' to 'sp->name' to check if 'sp' is the symbol you are searching for. This is basically the case you want. 3.2) both 's' and 'sp' are pseudo symbols Again skip_symbols returns false and symname_hash_search compares 's' with 'sp->name' to check if 'sp' is the symbol you are searching for. I'm not entirely sure if this way symname_hash_search is intended to be used but I also don't see a reason why it shouldn't be done. 3.3) 's' is a pseudo and 'sp' a proper symbol same like 3.2). 3.4) 's' is a proper symbol and 'sp' a psudo symbol here skip_symbols returns true and symname_hash_search skips this case. So the only case that is filtered out is 3.4 in which 's' must not contain any '_MODULES_*' while 'sp->name' has to start with one. But that's something a simple STREQ can handle like in case 3.3. So what's the point in having this extra filtering? Isn't this function basically identical to symbol_search and thus can be abbreviated to return !!(symbol_search(symbol)); same like above. This could be abbreviated to return !!(symname_hash_search(st->symname_hash, symbol, NULL)); I must admit I don't understand how the last two functions work, so I'm relying on Kazu to comment on those. Thanks Philipp