On Fri, Apr 24, 2020 at 05:55:21PM +0530, Syed Nayyar Waris wrote:
> +static inline void bitmap_set_value(unsigned long *map,
> + unsigned long value,
> + unsigned long start, unsigned long nbits)
> +{
> + const size_t index = BIT_WORD(start);
> + const unsigned long offset = start % BITS_PER_LONG;
> + const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
> + const unsigned long space = ceiling - start;
> +
> + value &= GENMASK(nbits - 1, 0);
> +
> + if (space >= nbits) {
> + map[index] &= ~(GENMASK(nbits + offset - 1, offset));
> + map[index] |= value << offset;
> + } else {
> + map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
> + map[index] |= value << offset;
> + map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
> + map[index + 1] |= (value >> space);
> + }
> +}
Sorry but what's the advantage of using this complicated function
as a replacement for the much simpler bitmap_set_value8()?
The drivers calling bitmap_set_value8() *know* that 8-bit accesses
are possible and take advantage of that knowledge by using a small,
speed-optimized function. Replacing that with a more complicated
(potentially less performant) function doesn't seem to be a step
forward.
Thanks,
Lukas
On Fri, Apr 24, 2020 at 7:40 PM Lukas Wunner <[email protected]> wrote:
>
> On Fri, Apr 24, 2020 at 05:55:21PM +0530, Syed Nayyar Waris wrote:
> > +static inline void bitmap_set_value(unsigned long *map,
> > + unsigned long value,
> > + unsigned long start, unsigned long nbits)
> > +{
> > + const size_t index = BIT_WORD(start);
> > + const unsigned long offset = start % BITS_PER_LONG;
> > + const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
> > + const unsigned long space = ceiling - start;
> > +
> > + value &= GENMASK(nbits - 1, 0);
> > +
> > + if (space >= nbits) {
> > + map[index] &= ~(GENMASK(nbits + offset - 1, offset));
> > + map[index] |= value << offset;
> > + } else {
> > + map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
> > + map[index] |= value << offset;
> > + map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
> > + map[index + 1] |= (value >> space);
> > + }
> > +}
>
> Sorry but what's the advantage of using this complicated function
> as a replacement for the much simpler bitmap_set_value8()?
>
> The drivers calling bitmap_set_value8() *know* that 8-bit accesses
> are possible and take advantage of that knowledge by using a small,
> speed-optimized function. Replacing that with a more complicated
> (potentially less performant) function doesn't seem to be a step
> forward.
>
> Thanks,
>
> Lukas
Actually this generic function can work with n-bits of any size (less
than equal to BITS_PER_LONG), while the earlier bitmap_set_value8
worked with n-bits having size of 8 bits only.
In the case when n-bits is 8-bits, this new bitmap_set_value()
function would behave very similar to the earlier bitmap_set_value8()
function. For example, in case of n-bits being 8-bits it will always
execute the 'if' condition and not the 'else' condition, hence
offering the same performance (because of encountering similar code
statements) as earlier bitmap_set_value8() function, most probably.
There is an additional advantage (this can happen when n-bits is not 8
bits): during setting value of n-bit in bitmap, if a situation arise
that the width of next n-bit is exceeding the word boundary, then it
will divide itself such that some portion of it is stored in that
word, while the remaining portion is stored in the next higher word.
So, this function preserves the behaviour of earlier
bitmap_set_value8() function and also adds extra functionality to
that.
Thanks
Syed Nayyar Waris
On Fri, Apr 24, 2020 at 08:22:38PM +0530, Syed Nayyar Waris wrote:
> On Fri, Apr 24, 2020 at 7:40 PM Lukas Wunner <[email protected]> wrote:
> >
> > On Fri, Apr 24, 2020 at 05:55:21PM +0530, Syed Nayyar Waris wrote:
> > > +static inline void bitmap_set_value(unsigned long *map,
> > > + unsigned long value,
> > > + unsigned long start, unsigned long nbits)
> > > +{
> > > + const size_t index = BIT_WORD(start);
> > > + const unsigned long offset = start % BITS_PER_LONG;
> > > + const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
> > > + const unsigned long space = ceiling - start;
> > > +
> > > + value &= GENMASK(nbits - 1, 0);
> > > +
> > > + if (space >= nbits) {
> > > + map[index] &= ~(GENMASK(nbits + offset - 1, offset));
> > > + map[index] |= value << offset;
> > > + } else {
> > > + map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
> > > + map[index] |= value << offset;
> > > + map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
> > > + map[index + 1] |= (value >> space);
> > > + }
> > > +}
> >
> > Sorry but what's the advantage of using this complicated function
> > as a replacement for the much simpler bitmap_set_value8()?
> >
> > The drivers calling bitmap_set_value8() *know* that 8-bit accesses
> > are possible and take advantage of that knowledge by using a small,
> > speed-optimized function. Replacing that with a more complicated
> > (potentially less performant) function doesn't seem to be a step
> > forward.
>
> Actually this generic function can work with n-bits of any size (less
> than equal to BITS_PER_LONG), while the earlier bitmap_set_value8
> worked with n-bits having size of 8 bits only.
>
> In the case when n-bits is 8-bits, this new bitmap_set_value()
> function would behave very similar to the earlier bitmap_set_value8()
> function. For example, in case of n-bits being 8-bits it will always
> execute the 'if' condition and not the 'else' condition, hence
> offering the same performance (because of encountering similar code
> statements) as earlier bitmap_set_value8() function, most probably.
>
> There is an additional advantage (this can happen when n-bits is not 8
> bits): during setting value of n-bit in bitmap, if a situation arise
> that the width of next n-bit is exceeding the word boundary, then it
> will divide itself such that some portion of it is stored in that
> word, while the remaining portion is stored in the next higher word.
>
> So, this function preserves the behaviour of earlier
> bitmap_set_value8() function and also adds extra functionality to
> that.
Please leave drivers as is which use exclusively 8-bit accesses,
e.g. gpio-max3191x.c and gpio-74x164.c. I'm fearing a performance
regression if your new generic variant is used. They work perfectly
fine the way they are and I don't see any benefit this series may have
for them.
If there are other drivers which benefit from the flexibility of your
generic variant then I'm not opposed to changing those.
Thanks,
Lukas
On Fri, Apr 24, 2020 at 05:00:58PM +0200, Lukas Wunner wrote:
> On Fri, Apr 24, 2020 at 08:22:38PM +0530, Syed Nayyar Waris wrote:
> > On Fri, Apr 24, 2020 at 7:40 PM Lukas Wunner <[email protected]> wrote:
> > >
> > > On Fri, Apr 24, 2020 at 05:55:21PM +0530, Syed Nayyar Waris wrote:
> > > > +static inline void bitmap_set_value(unsigned long *map,
> > > > + unsigned long value,
> > > > + unsigned long start, unsigned long nbits)
> > > > +{
> > > > + const size_t index = BIT_WORD(start);
> > > > + const unsigned long offset = start % BITS_PER_LONG;
> > > > + const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
> > > > + const unsigned long space = ceiling - start;
> > > > +
> > > > + value &= GENMASK(nbits - 1, 0);
> > > > +
> > > > + if (space >= nbits) {
> > > > + map[index] &= ~(GENMASK(nbits + offset - 1, offset));
> > > > + map[index] |= value << offset;
> > > > + } else {
> > > > + map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
> > > > + map[index] |= value << offset;
> > > > + map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
> > > > + map[index + 1] |= (value >> space);
> > > > + }
> > > > +}
> > >
> > > Sorry but what's the advantage of using this complicated function
> > > as a replacement for the much simpler bitmap_set_value8()?
> > >
> > > The drivers calling bitmap_set_value8() *know* that 8-bit accesses
> > > are possible and take advantage of that knowledge by using a small,
> > > speed-optimized function. Replacing that with a more complicated
> > > (potentially less performant) function doesn't seem to be a step
> > > forward.
> >
> > Actually this generic function can work with n-bits of any size (less
> > than equal to BITS_PER_LONG), while the earlier bitmap_set_value8
> > worked with n-bits having size of 8 bits only.
> >
> > In the case when n-bits is 8-bits, this new bitmap_set_value()
> > function would behave very similar to the earlier bitmap_set_value8()
> > function. For example, in case of n-bits being 8-bits it will always
> > execute the 'if' condition and not the 'else' condition, hence
> > offering the same performance (because of encountering similar code
> > statements) as earlier bitmap_set_value8() function, most probably.
> >
> > There is an additional advantage (this can happen when n-bits is not 8
> > bits): during setting value of n-bit in bitmap, if a situation arise
> > that the width of next n-bit is exceeding the word boundary, then it
> > will divide itself such that some portion of it is stored in that
> > word, while the remaining portion is stored in the next higher word.
> >
> > So, this function preserves the behaviour of earlier
> > bitmap_set_value8() function and also adds extra functionality to
> > that.
>
> Please leave drivers as is which use exclusively 8-bit accesses,
> e.g. gpio-max3191x.c and gpio-74x164.c. I'm fearing a performance
> regression if your new generic variant is used. They work perfectly
> fine the way they are and I don't see any benefit this series may have
> for them.
>
> If there are other drivers which benefit from the flexibility of your
> generic variant then I'm not opposed to changing those.
>
> Thanks,
>
> Lukas
We can leave of course bitmap_set_value8 alone, but for 8-bit values the
difference in latency I suspect is primarily due to the conditional test
for the word boundaries. This latency is surely overshadowed by the I/O
latency of the GPIO drivers, so I don't think there's much harm in
changing those to use the generic function when the bottleneck will not
be due to the bitmap_set_value/bitmap_get_value operations.
William Breathitt Gray
On Fri, Apr 24, 2020 at 11:09:26AM -0400, William Breathitt Gray wrote:
> On Fri, Apr 24, 2020 at 05:00:58PM +0200, Lukas Wunner wrote:
> > On Fri, Apr 24, 2020 at 08:22:38PM +0530, Syed Nayyar Waris wrote:
> > > On Fri, Apr 24, 2020 at 7:40 PM Lukas Wunner <[email protected]> wrote:
> > > > On Fri, Apr 24, 2020 at 05:55:21PM +0530, Syed Nayyar Waris wrote:
...
> > > So, this function preserves the behaviour of earlier
> > > bitmap_set_value8() function and also adds extra functionality to
> > > that.
> >
> > Please leave drivers as is which use exclusively 8-bit accesses,
> > e.g. gpio-max3191x.c and gpio-74x164.c. I'm fearing a performance
> > regression if your new generic variant is used. They work perfectly
> > fine the way they are and I don't see any benefit this series may have
> > for them.
> >
> > If there are other drivers which benefit from the flexibility of your
> > generic variant then I'm not opposed to changing those.
> We can leave of course bitmap_set_value8 alone, but for 8-bit values the
> difference in latency I suspect is primarily due to the conditional test
> for the word boundaries. This latency is surely overshadowed by the I/O
> latency of the GPIO drivers, so I don't think there's much harm in
> changing those to use the generic function when the bottleneck will not
> be due to the bitmap_set_value/bitmap_get_value operations.
Okay, how many new (non-8-bit) users this will target?
--
With Best Regards,
Andy Shevchenko
On Fri, Apr 24, 2020 at 07:34:10PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 24, 2020 at 11:09:26AM -0400, William Breathitt Gray wrote:
> > On Fri, Apr 24, 2020 at 05:00:58PM +0200, Lukas Wunner wrote:
> > > On Fri, Apr 24, 2020 at 08:22:38PM +0530, Syed Nayyar Waris wrote:
> > > > On Fri, Apr 24, 2020 at 7:40 PM Lukas Wunner <[email protected]> wrote:
> > > > > On Fri, Apr 24, 2020 at 05:55:21PM +0530, Syed Nayyar Waris wrote:
>
> ...
>
> > > > So, this function preserves the behaviour of earlier
> > > > bitmap_set_value8() function and also adds extra functionality to
> > > > that.
> > >
> > > Please leave drivers as is which use exclusively 8-bit accesses,
> > > e.g. gpio-max3191x.c and gpio-74x164.c. I'm fearing a performance
> > > regression if your new generic variant is used. They work perfectly
> > > fine the way they are and I don't see any benefit this series may have
> > > for them.
> > >
> > > If there are other drivers which benefit from the flexibility of your
> > > generic variant then I'm not opposed to changing those.
>
> > We can leave of course bitmap_set_value8 alone, but for 8-bit values the
> > difference in latency I suspect is primarily due to the conditional test
> > for the word boundaries. This latency is surely overshadowed by the I/O
> > latency of the GPIO drivers, so I don't think there's much harm in
> > changing those to use the generic function when the bottleneck will not
> > be due to the bitmap_set_value/bitmap_get_value operations.
>
> Okay, how many new (non-8-bit) users this will target?
>
> --
> With Best Regards,
> Andy Shevchenko
Within this patchset the only non-8-bit users are gpio-thunderx and
gpio-xilinix. The gpio-xilinx has configurable port widths so in some
instances it can behave like the 8-bit users, but not always.
If you want to keep the existing for_each_set_clump8 and related
functions, ignore [PATCH 3/6] and [PATCH 4/6]. That should allow this
patchset to be just an introduction of the new generic functions without
affecting the existing 8-bit users.
William Breathitt Gray
On Fri, Apr 24, 2020 at 12:42:00PM -0400, William Breathitt Gray wrote:
> Within this patchset the only non-8-bit users are gpio-thunderx and
> gpio-xilinix. The gpio-xilinx has configurable port widths so in some
> instances it can behave like the 8-bit users, but not always.
>
> If you want to keep the existing for_each_set_clump8 and related
> functions, ignore [PATCH 3/6] and [PATCH 4/6]. That should allow this
> patchset to be just an introduction of the new generic functions without
> affecting the existing 8-bit users.
Yes I don't mind the changes to gpio-thunderx and gpio-xilinx at all
but please leave the 8-bit users as they are wherever possible.
Actually my concern is not just performance but the existing 8-bit
variant is simpler to understand than the generic variant,
making it easier to follow the code in the drivers.
Thanks,
Lukas