On 1/25/23 10:59, Alan Maguire wrote:
On 25/01/2023 17:54, Kui-Feng Lee wrote:
On 1/24/23 05:45, Alan Maguire wrote:
+/*
+ * static functions with suffixes are not added yet - we need to
+ * observe across all CUs to see if the static function has
+ * optimized parameters in any CU, since in such a case it should
+ * not be included in the final BTF. NF_HOOK.constprop.0() is
+ * a case in point - it has optimized-out parameters in some CUs
+ * but not others. In order to have consistency (since we do not
+ * know which instance the BTF-specified function signature will
+ * apply to), we simply skip adding functions which have optimized
+ * out parameters anywhere.
+ */
+static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn)
+{
+ struct btf_encoder *parent = encoder->parent ? encoder->parent : encoder;
+ const char *name = function__name(fn);
+ struct function **nodep;
+ int ret = 0;
+
+ pthread_mutex_lock(&parent->saved_func_lock);
Do you have the number of static functions with suffices?
There are a few thousand, and around 25000 static functions
overall ("."-suffixed are all static) that will participate in
the tree representations (see patch 5). This equates to roughly
half of the vmlinux BTF functions.
To evaluate the effectiveness of your patchset, I conducted an
experiment where I ran a command:
`time env LLVM_OBJCOPY=objcopy pahole -J --btf_gen_floats
--lang_exclude=rust -j .tmp_vmlinux.btf`.
On my machine, it took about
- 9s w/o the patchset (3s waiting for the worker threads)
- 13s w/ the patchset (7s waiting for the worker threads)
It was about 4s difference.
If I turned multi-threading off (w/o -j), it took
- 28s w/o the patchset.
- 32s w/ the patchset.
It was about 4s difference as sell.
Hence, multi-threading does not benefit us in the instance of this
patchset. Lock contention should be taken into account heavily here.
Approximately 4% of the time is spent when executing a Linux incremental
build (about 96s~108s) with an insignificant modification to the source
tree for about four seconds.
Taking into consideration the previous experience that shows a reduction
in BTF info processing time (not including loading and IO) to 13%, I am
uncertain if it pays off to invest my time towards reducing 4s to <1s.
Though, cutting down 3 seconds every single time I need to rebuild the
tree for some small changes might be worth it.
If the number of static functions with suffices is high, the contention of the lock would be an issue.
Is it possible to keep a local pool of static functions with suffices? The pool will be combined with its parent either at the completion of a CU, before ending the thread or when merging into the main thread.
It's possible alright. I'll try to lay out the possibilities so we
can figure out the best way forward.
Option 1: global tree of static functions, created during DWARF loading
Pro: Quick addition/lookup, we can flag optimizations or inconsistent prototypes as
we encounter them.
Con: Lock contention between encoder threads.
Option 2: store static functions in a per-encoder tree, traverse them all
prior to BTF merging to eliminate unwanted functions
Pro: limits contention.
Con: for each static function in each encoder, we need to look it up in all other
encoder trees. In option 1 we paid that price as the function was added, here
we pay it later on prior to merging. So processing here is
O(number_functions * num_encoders). There may be a cleverer way to handle
this but I can't see it right now.
There may be other approaches to this of course, but these were the two I
could come up with. What do you think?
Option 2 appears to be the more convenient and effective solution,
whereas Option 1, I guess, will require considerable effort for a
successful outcome.
Alan
+ nodep = tsearch(fn, &parent->saved_func_tree, function__compare);
+ if (nodep == NULL) {
+ fprintf(stderr, "error: out of memory adding local function '%s'\n",
+ name);
+ ret = -1;
+ goto out;
+ }
+ /* If we find an existing entry, we want to merge observations
+ * across both functions, checking that the "seen optimized-out
+ * parameters" status is reflected in our tree entry.
+ * If the entry is new, record encoder state required
+ * to add the local function later (encoder + type_id_off)
+ * such that we can add the function later.
+ */
+ if (*nodep != fn) {
+ (*nodep)->proto.optimized_parms |= fn->proto.optimized_parms;
+ } else {
+ struct btf_encoder_state *state = zalloc(sizeof(*state));
+
+ if (state == NULL) {
+ fprintf(stderr, "error: out of memory adding local function '%s'\n",
+ name);
+ ret = -1;
+ goto out;
+ }
+ state->encoder = encoder;
+ state->type_id_off = encoder->type_id_off;
+ fn->priv = state;
+ encoder->saved_func_cnt++;
+ if (encoder->verbose)
+ printf("added local function '%s'%s\n", name,
+ fn->proto.optimized_parms ?
+ ", optimized-out params" : "");
+ }
+out:
+ pthread_mutex_unlock(&parent->saved_func_lock);
+ return ret;
+}
+
