Thanks Chris!
First of all let's make sure a few points are clarified:
1) What should rtems_rfs_bitmap_map_set(control, bit) do?
- 'control' is a handle to the bitmap.
- 'bit' is the offset of the bit to set in the bitmap. 'bit' should be in
the range of [0, control->size - 1].
2) How does the bitmap store bits?
By storing bits in an array of rtems_rfs_bitmap_element objects.
Each rtems_rfs_bitmap_element is 32bit. So the number of elements is
control->size / 32.
3) What is map[index]?
'index' is calculated as 'bit / 32' and is the offset into the block buffer
of the element that contains the 'bit'. So map[index] is the value of the
element that contains the 'bit'.
4) What is 'offset'?
'offset' is calculated as 'bit % 32'. It locates the 'bit' in the element
that contains it.
5) What is RTEMS_RFS_BITMAP_ELEMENT_SET
This is a constant. If map[index] == RTEMS_RFS_BITMAP_ELEMENT_SET, all bits
in this element (map[index]) are set. It is used to check that condition
and *update the search map*.
Now let's see what your change does:
- map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);
- if (rtems_rfs_bitmap_match(map[index], RTEMS_RFS_BITMAP_ELEMENT_SET))
+ if (!rtems_rfs_bitmap_match(map[index], RTEMS_RFS_BITMAP_ELEMENT_SET))
{
Your change:
1. First updates map[index] to set the bit.
2. Compares the updated map[index] with RTEMS_RFS_BITMAP_ELEMENT_SET. Note
this:
- *DOES NOT* check whether the original bit was set.
- *DOES NOT* have a way to check if the bit was previously set, as
map[index] has already been overwritten.
- *DOES* check whether all bits in map[index] are set.
Please let me know if you agree with my assertions in #2 above. Note the
name of "RTEMS_RFS_BITMAP_ELEMENT_SET" is slightly misleading. See my
points #5 for a clarification.
Thanks,
Fan
On Thu, Oct 12, 2017 at 10:35 AM, Chris Johns <chr...@rtems.org> wrote:
> On 12/10/17 10:05 am, Fan Deng wrote:
> > Hi Chris,
> >
>
> Thanks for quick response.
>
> > Based on my understanding, the patch in your email is different:
> >
> > 1) rtems_rfs_bitmap_map_set:
> > By changing negating the if condition, the updated logic only modifies
> the
> > element map[index] when the original value of map[index] is
> > RTEMS_RFS_BITMAP_ELEMENT_SET.
>
> Your change has:
>
> - map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);
> + element = map[index];
> + map[index] = rtems_rfs_bitmap_set (element, 1 << offset);
> + // If the element does not change, the bit was already set. There will
> be no
> + // further action to take.
> + if (rtems_rfs_bitmap_match(element, map[index]))
> + return 0;
>
> The change gets a copy of the uint32_t value from the map and then updates
> the
> map setting the bit. The change then checks to see if the original bit in
> the
> map is set (match returns true) and if set you exit the function. I see
> this
> being the same as the code in my patch where I only proceed to do anything
> if
> the bit is not set in the map:
>
> - map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);
> - if (rtems_rfs_bitmap_match(map[index], RTEMS_RFS_BITMAP_ELEMENT_SET))
> + if (!rtems_rfs_bitmap_match(map[index], RTEMS_RFS_BITMAP_ELEMENT_SET))
> {
>
> If the bit is not set you enter the code to handle a map bit being set.
> You have:
>
> + control->free--;
> + rtems_rfs_buffer_mark_dirty (control->buffer);
> if (rtems_rfs_bitmap_match(map[index], RTEMS_RFS_BITMAP_ELEMENT_SET))
> {
>
> Given you have set the `map[index]` earlier the if test will always pass
> and I
> suspect the compiler will know this and dead code eliminate it.
>
> > This is not quite right, as we are explicitly updating the 'offset' bit
> in
> > map[index]. That bit should be updated regardless.
>
> Why update the bit if it is already set? I seem to be missing something
> here.
> The map is just an array of 32bit unsigned ints.
>
> >
> > 2) rtems_rfs_bitmap_map_clear:
> > Same problem, whether we modify map[index] should not depend on if
> map[index] ==
> > RTEMS_RFS_BITMAP_ELEMENT_SET.
>
> Sure, once we agree on the set the clear should fall out of it.
>
> Chris
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
