Re: [PATCH v5 3/6] alloc_tag: add size-based filtering to ioctl

[Abhishek Bapat]

On Wed, Jun 17, 2026 at 4:01 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> On Wed, Jun 17, 2026 at 3:41 PM Abhishek Bapat <abhishekbapat@xxxxxxxxxx> wrote:
> >
> > On Wed, Jun 17, 2026 at 3:35 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Jun 17, 2026 at 1:55 PM Abhishek Bapat <abhishekbapat@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Jun 17, 2026 at 9:29 AM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, Jun 15, 2026 at 4:04 PM Abhishek Bapat <abhishekbapat@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > Extend the allocinfo filtering mechanism to allow users to filter tags
> > > > > > based on the total number of bytes allocated [min_size, max_size]. The
> > > > > > size range is inclusive.
> > > > > >
> > > > > > Filtering by size involves retrieving allocinfo per-CPU counters, which
> > > > > > is an expensive operation. Hence, the performance of size-based
> > > > > > filtering will be worse than other filters.
> > > > > >
> > > > > > Signed-off-by: Abhishek Bapat <abhishekbapat@xxxxxxxxxx>
> > > > > > Acked-by: Hao Ge <hao.ge@xxxxxxxxx>
> > > > > > ---
> > > > > >  include/uapi/linux/alloc_tag.h |  8 ++++-
> > > > > >  lib/alloc_tag.c                | 63 ++++++++++++++++++++++++++++------
> > > > > >  2 files changed, 59 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/include/uapi/linux/alloc_tag.h b/include/uapi/linux/alloc_tag.h
> > > > > > index 3b11877955b9..7f5acbb44c14 100644
> > > > > > --- a/include/uapi/linux/alloc_tag.h
> > > > > > +++ b/include/uapi/linux/alloc_tag.h
> > > > > > @@ -45,13 +45,17 @@ enum {
> > > > > >         ALLOCINFO_FILTER_FUNCTION,
> > > > > >         ALLOCINFO_FILTER_FILENAME,
> > > > > >         ALLOCINFO_FILTER_LINENO,
> > > > > > -       __ALLOCINFO_FILTER_LAST = ALLOCINFO_FILTER_LINENO
> > > > > > +       ALLOCINFO_FILTER_MIN_SIZE,
> > > > > > +       ALLOCINFO_FILTER_MAX_SIZE,
> > > > > > +       __ALLOCINFO_FILTER_LAST = ALLOCINFO_FILTER_MAX_SIZE
> > > > > >  };
> > > > > >
> > > > > >  #define ALLOCINFO_FILTER_MASK_MODNAME          (1 << ALLOCINFO_FILTER_MODNAME)
> > > > > >  #define ALLOCINFO_FILTER_MASK_FUNCTION         (1 << ALLOCINFO_FILTER_FUNCTION)
> > > > > >  #define ALLOCINFO_FILTER_MASK_FILENAME         (1 << ALLOCINFO_FILTER_FILENAME)
> > > > > >  #define ALLOCINFO_FILTER_MASK_LINENO           (1 << ALLOCINFO_FILTER_LINENO)
> > > > > > +#define ALLOCINFO_FILTER_MASK_MIN_SIZE         (1 << ALLOCINFO_FILTER_MIN_SIZE)
> > > > > > +#define ALLOCINFO_FILTER_MASK_MAX_SIZE         (1 << ALLOCINFO_FILTER_MAX_SIZE)
> > > > > >
> > > > > >  #define ALLOCINFO_FILTER_MASKS \
> > > > > >         ((1 << (__ALLOCINFO_FILTER_LAST + 1)) - 1)
> > > > > > @@ -59,6 +63,8 @@ enum {
> > > > > >  struct allocinfo_filter {
> > > > > >         __u64 mask; /* bitmask of the filter fields used */
> > > > > >         struct allocinfo_tag fields;
> > > > > > +       __u64 min_size;
> > > > > > +       __u64 max_size;
> > > > > >  };
> > > > > >
> > > > > >  struct allocinfo_get_at {
> > > > > > diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> > > > > > index 5feb61d9fb92..b3d21834b61e 100644
> > > > > > --- a/lib/alloc_tag.c
> > > > > > +++ b/lib/alloc_tag.c
> > > > > > @@ -195,15 +195,26 @@ static int allocinfo_cmp_str(const char *str, const char *template)
> > > > > >         return strncmp(allocinfo_str(str), template, ALLOCINFO_STR_SIZE);
> > > > > >  }
> > > > > >
> > > > > > +/* Fetch the per-CPU counters */
> > > > > > +static inline struct alloc_tag_counters allocinfo_prefetch_counters(struct codetag *ct)
> > > > > > +{
> > > > > > +       return alloc_tag_read(ct_to_alloc_tag(ct));
> > > > > > +}
> > > > > > +
> > > > > >  /*
> > > > > >   * Populates the UAPI allocinfo_tag_data structure with active runtime
> > > > > >   * profiling counters extracted from the given kernel codetag.
> > > > > >   */
> > > > > >  static void allocinfo_to_params(struct codetag *ct,
> > > > > > -                               struct allocinfo_tag_data *data)
> > > > > > +                               struct allocinfo_tag_data *data,
> > > > > > +                               struct alloc_tag_counters *counters)
> > > > > >  {
> > > > > > -       struct alloc_tag *tag = ct_to_alloc_tag(ct);
> > > > > > -       struct alloc_tag_counters counter = alloc_tag_read(tag);
> > > > > > +       struct alloc_tag_counters local_counters;
> > > > > > +
> > > > > > +       if (!counters) {
> > > > > > +               local_counters = allocinfo_prefetch_counters(ct);
> > > > > > +               counters = &local_counters;
> > > > > > +       }
> > > > > >
> > > > > >         if (ct->modname)
> > > > > >                 allocinfo_copy_str(data->tag.modname, ct->modname);
> > > > > > @@ -212,9 +223,9 @@ static void allocinfo_to_params(struct codetag *ct,
> > > > > >         allocinfo_copy_str(data->tag.function, ct->function);
> > > > > >         allocinfo_copy_str(data->tag.filename, ct->filename);
> > > > > >         data->tag.lineno = ct->lineno;
> > > > > > -       data->counter.bytes = counter.bytes;
> > > > > > -       data->counter.calls = counter.calls;
> > > > > > -       data->counter.accurate = !alloc_tag_is_inaccurate(tag);
> > > > > > +       data->counter.bytes = counters->bytes;
> > > > > > +       data->counter.calls = counters->calls;
> > > > > > +       data->counter.accurate = !alloc_tag_is_inaccurate(ct_to_alloc_tag(ct));
> > > > > >  }
> > > > > >
> > > > > >  /*
> > > > > > @@ -238,7 +249,9 @@ static int allocinfo_ioctl_get_content_id(struct seq_file *m, void __user *arg)
> > > > > >   * Verifies whether a given codetag satisfies the active filtering criteria by
> > > > > >   * matching its characteristics against the specified filter.
> > > > > >   */
> > > > > > -static bool matches_filter(struct codetag *ct, struct allocinfo_filter *filter)
> > > > > > +static bool matches_filter(struct codetag *ct, struct allocinfo_filter *filter,
> > > > > > +                          struct alloc_tag_counters *counters,
> > > > > > +                          bool *fetched_counters)
> > > > > >  {
> > > > > >         if (!filter || !filter->mask)
> > > > > >                 return true;
> > > > > > @@ -265,6 +278,19 @@ static bool matches_filter(struct codetag *ct, struct allocinfo_filter *filter)
> > > > > >             ct->lineno != filter->fields.lineno)
> > > > > >                 return false;
> > > > > >
> > > > > > +       if (filter->mask & (ALLOCINFO_FILTER_MASK_MIN_SIZE | ALLOCINFO_FILTER_MASK_MAX_SIZE)) {
> > > > > > +               if (!*fetched_counters) {
> > > > > > +                       *counters = allocinfo_prefetch_counters(ct);
> > > > > > +                       *fetched_counters = true;
> > > > > > +               }
> > > > > > +               if ((filter->mask & ALLOCINFO_FILTER_MASK_MIN_SIZE) &&
> > > > > > +                   counters->bytes < filter->min_size)
> > > > > > +                       return false;
> > > > > > +               if ((filter->mask & ALLOCINFO_FILTER_MASK_MAX_SIZE) &&
> > > > > > +                   counters->bytes > filter->max_size)
> > > > > > +                       return false;
> > > > > > +       }
> > > > > > +
> > > > > >         return true;
> > > > > >  }
> > > > > >
> > > > > > @@ -278,6 +304,8 @@ static int allocinfo_ioctl_get_at(struct seq_file *m, void __user *arg)
> > > > > >         struct codetag *ct;
> > > > > >         struct allocinfo_get_at params = {0};
> > > > > >         __u64 skip_count;
> > > > > > +       struct alloc_tag_counters counters;
> > > > > > +       bool fetched_counters;
> > > > > >
> > > > > >         if (copy_from_user(&params, arg, sizeof(params)))
> > > > > >                 return -EFAULT;
> > > > > > @@ -285,6 +313,11 @@ static int allocinfo_ioctl_get_at(struct seq_file *m, void __user *arg)
> > > > > >         if (params.filter.mask & ~ALLOCINFO_FILTER_MASKS)
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > > +       if ((params.filter.mask & ALLOCINFO_FILTER_MASK_MIN_SIZE) &&
> > > > > > +           (params.filter.mask & ALLOCINFO_FILTER_MASK_MAX_SIZE) &&
> > > > > > +           params.filter.min_size > params.filter.max_size)
> > > > > > +               return -EINVAL;
> > > > > > +
> > > > > >         priv = m->private;
> > > > > >
> > > > > >         mutex_lock(&priv->ioctl_lock);
> > > > > > @@ -308,7 +341,8 @@ static int allocinfo_ioctl_get_at(struct seq_file *m, void __user *arg)
> > > > > >         ct = codetag_next_ct(&priv->ioctl_iter);
> > > > > >
> > > > > >         while (ct) {
> > > > > > -               if (matches_filter(ct, &priv->filter)) {
> > > > > > +               fetched_counters = false;
> > > > > > +               if (matches_filter(ct, &priv->filter, &counters, &fetched_counters)) {
> > > > >
> > > > > Do we really need this "fetched_counters" parameter? Here are the
> > > > > possible cases:
> > > > > 1. If the filter does not include ALLOCINFO_FILTER_MASK_MIN_SIZE |
> > > > > ALLOCINFO_FILTER_MASK_MAX_SIZE then counters would not be fetched.
> > > > > 2. If the filter includes ALLOCINFO_FILTER_MASK_MIN_SIZE |
> > > > > ALLOCINFO_FILTER_MASK_MAX_SIZE and
> > > > > 2.1. matches_filter() returns true then we know counters were fetched
> > > > > because they had to be validated.
> > > > > 2.2. matches_filter() returns false then we don't care if the counters
> > > > > were fetched. We do not report that tag anyway.
> > > > >
> > > > > So, instead of passing fetched_counters to matches_filter() we could do this:
> > > > >
> > > > > bool filter_by_size = (params.filter.mask &
> > > > > (ALLOCINFO_FILTER_MASK_MIN_SIZE | ALLOCINFO_FILTER_MASK_MAX_SIZE)) !=
> > > > > 0;
> > > > > while (ct) {
> > > > >            if (matches_filter(ct, &priv->filter, &counters)) {
> > > > > ...
> > > > > }
> > > > > if (ct) {
> > > > >            allocinfo_to_params(ct, &params.data, filter_by_size ?
> > > > > &counters : NULL);
> > > > > ...
> > > > > }
> > > > >
> > > > > Wouldn't that work?
> > > > >
> > > >
> > > > While we can deduce whether counters were fetched outside the
> > > > matches_filter function, I think the current implementation is more
> > > > intuitive from a readability perspective. I believe it  should be kept
> > > > as is for that reason. If we extract the logic, we'll first have to
> > > > replicate the boolean logic at two places. Second, we'd need to add a
> > > > comment explaining the boolean calculation, and the reader might have
> > > > a higher cognitive load trying to determine which function populates
> > > > the counters. The current implementation makes it easy for the reader
> > > > to deduce the original intention. Let me know what you think.
> > >
> > > Ok, I guess you have a point.
> > >
> > > I was also thinking why we are passing NULL to allocinfo_to_params()
> > > to fetch the counters into a local variable? Why can't we simply call
> > > allocinfo_prefetch_counters() before calling allocinfo_to_params()
> > > when fetched_counters==false? Basically:
> > >
> > > if (!fetched_counters)
> > >     counters = allocinfo_prefetch_counters(ct);
> > > allocinfo_to_params(ct, &params.data, &counters);
> > >
> > > This would simplify allocinfo_to_params() because counter will never
> > > be NULL and it would not need local counters.
> > >
> >
> > The only reason I did it that way was to avoid repeating the code at
> > two places i.e. allocinfo_ioctl_get_at and allocinfo_ioctl_get_next.
> > Either way, the per-CPU counters are assimilated only once. I can
> > include this change if you still want me to, but personally I like the
> > way it currently is implemented.
>
> Yeah, I think repeating 2 lines is preferable to passing NULL and
> fetching into a local variable. Please include that change.
>

Ack, I will change this in the next patchset version.

> >
> > > >
> > > > > >                         if (skip_count == 0)
> > > > > >                                 break;
> > > > > >                         skip_count--;
> > > > > > @@ -317,7 +351,7 @@ static int allocinfo_ioctl_get_at(struct seq_file *m, void __user *arg)
> > > > > >         }
> > > > > >
> > > > > >         if (ct) {
> > > > > > -               allocinfo_to_params(ct, &params.data);
> > > > > > +               allocinfo_to_params(ct, &params.data, fetched_counters ? &counters : NULL);
> > > > > >                 priv->positioned = true;
> > > > > >         }
> > > > > >
> > > > > > @@ -343,6 +377,8 @@ static int allocinfo_ioctl_get_next(struct seq_file *m, void __user *arg)
> > > > > >         struct codetag *ct;
> > > > > >         struct allocinfo_tag_data params;
> > > > > >         int ret = 0;
> > > > > > +       struct alloc_tag_counters counters;
> > > > > > +       bool fetched_counters;
> > > > > >
> > > > > >         memset(&params, 0, sizeof(params));
> > > > > >         priv = m->private;
> > > > > > @@ -356,10 +392,15 @@ static int allocinfo_ioctl_get_next(struct seq_file *m, void __user *arg)
> > > > > >         }
> > > > > >
> > > > > >         ct = codetag_next_ct(&priv->ioctl_iter);
> > > > > > -       while (ct && !matches_filter(ct, &priv->filter))
> > > > > > +       while (ct) {
> > > > > > +               fetched_counters = false;
> > > > > > +               if (matches_filter(ct, &priv->filter, &counters, &fetched_counters))
> > > > > > +                       break;
> > > > > >                 ct = codetag_next_ct(&priv->ioctl_iter);
> > > > > > +       }
> > > > > > +
> > > > > >         if (ct)
> > > > > > -               allocinfo_to_params(ct, &params);
> > > > > > +               allocinfo_to_params(ct, &params, fetched_counters ? &counters : NULL);
> > > > > >
> > > > > >         if (!ct) {
> > > > > >                 priv->positioned = false;
> > > > > > --
> > > > > > 2.54.0.1136.gdb2ca164c4-goog
> > > > > >