Re: [PATCH v7 2/4] topology: use bin_attribute to break the size limitation of cpumap ABI

["andriy.shevchenko@linux.intel.com" <andriy.shevchenko@linux.intel.com>]

On Fri, Jul 16, 2021 at 06:12:21PM -0700, Yury Norov wrote:
> On Sat, Jul 17, 2021 at 12:16:48AM +0000, Song Bao Hua (Barry Song) wrote:
> > > From: Yury Norov [mailto:yury.norov@gmail.com]
> > > Sent: Saturday, July 17, 2021 8:04 AM
> > > On Fri, Jul 16, 2021 at 08:49:58AM +0000, Song Bao Hua (Barry Song) wrote:

...

> > Generally good idea. However, for sysfs ABI entries, it might not be
> > that true.
> > 
> > A sysfs entry might never be read for its whole life. As I explained
> > before, a sysfs entry - especially for list,  is randomly "cat" by users.
> > Many of them won't be read forever. And after they are read once, they
> > will probably never be read again. The operations to read ABI could be
> > random and rare. Performance wouldn't be a concern.
> > 
> > To avoid holding the memory which might never be used, it is better to
> > allocate and free the memory during runtime. I mean to allocate in show()
> > and free in show(), aka, to do it on demand.
> > 
> > For example, for a server with 256CPU and each cpu has dozens of sysfs ABI
> > entries, only a few of sysfs list entries might be randomly "cat" by users.
> > Holding 256*entries memory doesn't look good.
> 
> Ok, makes sense.
>  
> > > This would require to add bitmap_max_string_size(list, bitmap, nbits),
> > > but it's O(1), and I think, others will find it helpful.
> > 
> > What about getting size and memory at the same time?
> 
> 1. We already have kasprintf()
> 2. It breaks coding style.
> 
> Documentation/process/coding-style.rst:
>         Functions should be short and sweet, and do just one thing. 
> 
> From practical point of view, there should be some balance between
> granularity and ease-of-use. But in this case, bitmap_list cries for 
> a function that will help to estimate size of output buffer.

According to the vsnprintf() logic the estimated size is what it returns. If
user supplies too few bytes available, the comparison with the returned value
can tell caller that space wasn't big enough.

> And it's
> easy to imagine a case where the estimated length of bitmap is needed
> explicitly:
> 
>         size_t max_size = bitmap_max_string_size(nbits);
>         char *buf = kmalloc(PAGE_ALIGN(max_size) * nr_cpus);
> 
> Thought, I don't insist. In your driver you can do:
> 
>         size_t size = snprintf(NULL, 0, ...);
>         void *buf = kmalloc(size);
> 
> It will be fully correct, and you already have everything you need.
>  
> > ssize_t bitmap_get_print_buf(bool list, char **buf, const unsigned long
> >  *maskp, int nmaskbits)
> > 
> > ssize_t cpumap_get_print_buf(bool list, char **buf, const struct cpumask *mask);
> >
> > This API returns the size of printed buffer, and it also gets the
> > printed result saved in *buf. Then drivers don't need to do three
> > steps:
> > 
> > 1. get cpumap buffer size which is your cpumap_max_string_size()
> > 2. allocate memory for buffer according to size got in step 1
> > 3. print bitmap(cpumap) to buffer by "pbl"
> > 
> > It will only need to call bitmap_get_print_buf() and all three
> > things are done inside bitmap_get_print_buf().
> > 
> > How to use the size and memory allocated in cpumap_get_print_buf
> > will be totally up to users.
> > 
> > The other benefit for this is that if we get string size during initialization,
> > and then we print in show() entries, the size got at the beginning might be not
> > enough as system topology might have changed. Sysfs ABI reflects the status of
> > system at this moment.

-- 
With Best Regards,
Andy Shevchenko