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(-)
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);
}
--
2.43.2Sven 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 Virtualisation Tech Lead @ Linaro
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?
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 Virtualisation Tech Lead @ Linaro
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.
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.
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~
© 2016 - 2024 Red Hat, Inc.