| Message ID | 20240301174609.1964379-2-svens@stackframe.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | plugins/execlog: add data address match and address range support | expand |
Sven Schnelle <svens@stackframe.org> writes: > In preparation of making the parsing part of qemu_set_dfilter_ranges() > available to other users, convert it to return a GList, so the result > can be used with other functions taking a GList of struct Range. Why do we need to convert it to a Glist? When I originally wrote the dfilter code I deliberately chose GArray over GList to avoid bouncing across cache lines during the tight loop that is checking ranges. It's not like we can't pass GArray's to plugins as well?
Alex Bennée <alex.bennee@linaro.org> writes: > Sven Schnelle <svens@stackframe.org> writes: > >> In preparation of making the parsing part of qemu_set_dfilter_ranges() >> available to other users, convert it to return a GList, so the result >> can be used with other functions taking a GList of struct Range. > > Why do we need to convert it to a Glist? When I originally wrote the > dfilter code I deliberately chose GArray over GList to avoid bouncing > across cache lines during the tight loop that is checking ranges. It's > not like we can't pass GArray's to plugins as well? Good point. I'll change it back to a GArray in the next iteration.
Alex Bennée <alex.bennee@linaro.org> writes: > Sven Schnelle <svens@stackframe.org> writes: > >> In preparation of making the parsing part of qemu_set_dfilter_ranges() >> available to other users, convert it to return a GList, so the result >> can be used with other functions taking a GList of struct Range. > > Why do we need to convert it to a Glist? When I originally wrote the > dfilter code I deliberately chose GArray over GList to avoid bouncing > across cache lines during the tight loop that is checking ranges. It's > not like we can't pass GArray's to plugins as well? Looking at the code again, i remember that the reason for choosing GList was that the other range matching function all take GList's - having some functions take GArray's, and some not would be odd. So i wonder whether we should convert all of those functions to GArray? (if possible, i haven't checked) What do you think?
On 3/4/24 03:13, Sven Schnelle wrote: > Alex Bennée <alex.bennee@linaro.org> writes: > >> Sven Schnelle <svens@stackframe.org> writes: >> >>> In preparation of making the parsing part of qemu_set_dfilter_ranges() >>> available to other users, convert it to return a GList, so the result >>> can be used with other functions taking a GList of struct Range. >> >> Why do we need to convert it to a Glist? When I originally wrote the >> dfilter code I deliberately chose GArray over GList to avoid bouncing >> across cache lines during the tight loop that is checking ranges. It's >> not like we can't pass GArray's to plugins as well? > > Good point. I'll change it back to a GArray in the next iteration. How many address ranges do you expect to have? If more than a couple, then perhaps IntervalTree would be better for a balanced binary search. r~
Sven Schnelle <svens@stackframe.org> writes: > Alex Bennée <alex.bennee@linaro.org> writes: > >> Sven Schnelle <svens@stackframe.org> writes: >> >>> In preparation of making the parsing part of qemu_set_dfilter_ranges() >>> available to other users, convert it to return a GList, so the result >>> can be used with other functions taking a GList of struct Range. >> >> Why do we need to convert it to a Glist? When I originally wrote the >> dfilter code I deliberately chose GArray over GList to avoid bouncing >> across cache lines during the tight loop that is checking ranges. It's >> not like we can't pass GArray's to plugins as well? > > Looking at the code again, i remember that the reason for choosing GList > was that the other range matching function all take GList's - having > some functions take GArray's, and some not would be odd. Ahh I see. There wasn't intended to be much of a relationship between the dfilter code and the range code apart from the fact I re-used the Range typedef to avoid duplication. > So i wonder > whether we should convert all of those functions to GArray? (if > possible, i haven't checked) I think that would depend on access patterns and flexibility. For the dfilter there is no particular need for sorting and the principle operation is a sequence of lookups. For the other use cases keeping a sorted list and quick insertion might be more useful. While its tempting to go on a wider re-factoring I would caution against it so close to softfreeze. > What do you think? Lets stick to the dfilter case and worry about wider clean-ups later. As Richard points out it might be the interval tree makes more sense for some of these things.
Alex Bennée <alex.bennee@linaro.org> writes: >> So i wonder >> whether we should convert all of those functions to GArray? (if >> possible, i haven't checked) > > I think that would depend on access patterns and flexibility. For the > dfilter there is no particular need for sorting and the principle > operation is a sequence of lookups. For the other use cases keeping a > sorted list and quick insertion might be more useful. > > While its tempting to go on a wider re-factoring I would caution against > it so close to softfreeze. > >> What do you think? > > Lets stick to the dfilter case and worry about wider clean-ups later. As > Richard points out it might be the interval tree makes more sense for > some of these things. I think i go with the GArray variant for now. I'd guess that -dfilter is usually only used with one or a few arguments, so using a Interval Tree is probably not neccessary.
diff --git a/util/log.c b/util/log.c index d36c98da0b..779c0689a9 100644 --- a/util/log.c +++ b/util/log.c @@ -46,7 +46,7 @@ static __thread Notifier qemu_log_thread_cleanup_notifier; int qemu_loglevel; static bool log_per_thread; -static GArray *debug_regions; +static GList *debug_regions; /* Returns true if qemu_log() will really write somewhere. */ bool qemu_log_enabled(void) @@ -368,38 +368,36 @@ bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp) */ bool qemu_log_in_addr_range(uint64_t addr) { - if (debug_regions) { - int i = 0; - for (i = 0; i < debug_regions->len; i++) { - Range *range = &g_array_index(debug_regions, Range, i); - if (range_contains(range, addr)) { - return true; - } - } - return false; - } else { + GList *p; + + if (!debug_regions) { return true; } -} + for (p = g_list_first(debug_regions); p; p = g_list_next(p)) { + if (range_contains(p->data, addr)) { + return true; + } + } + return false; +} void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) { gchar **ranges = g_strsplit(filter_spec, ",", 0); + struct Range *range = NULL; int i; if (debug_regions) { - g_array_unref(debug_regions); + g_list_free_full(debug_regions, g_free); debug_regions = NULL; } - debug_regions = g_array_sized_new(FALSE, FALSE, - sizeof(Range), g_strv_length(ranges)); for (i = 0; ranges[i]; i++) { const char *r = ranges[i]; const char *range_op, *r2, *e; uint64_t r1val, r2val, lob, upb; - struct Range range; + range = g_new0(struct Range, 1); range_op = strstr(r, "-"); r2 = range_op ? range_op + 1 : NULL; @@ -448,10 +446,12 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) error_setg(errp, "Invalid range"); goto out; } - range_set_bounds(&range, lob, upb); - g_array_append_val(debug_regions, range); + range_set_bounds(range, lob, upb); + debug_regions = g_list_append(debug_regions, range); + range = NULL; } out: + g_free(range); g_strfreev(ranges); }
In preparation of making the parsing part of qemu_set_dfilter_ranges() available to other users, convert it to return a GList, so the result can be used with other functions taking a GList of struct Range. Signed-off-by: Sven Schnelle <svens@stackframe.org> --- util/log.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)