<div dir="ltr">Thanks Chris!<div><br></div><div>First of all let's make sure a few points are clarified:</div><div><br></div><div>1) What should rtems_rfs_bitmap_map_set(control, bit) do?</div><div>- 'control' is a handle to the bitmap.</div><div>- 'bit' is the offset of the bit to set in the bitmap. 'bit' should be in the range of [0, control->size - 1].</div><div><br></div><div>2) How does the bitmap store bits?</div><div>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.</div><div><br></div><div>3) What is map[index]?</div><div>'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'.</div><div><br></div><div>4) What is 'offset'?</div><div>'offset' is calculated as 'bit % 32'. It locates the 'bit' in the element that contains it.</div><div><br></div><div>5) What is RTEMS_RFS_BITMAP_ELEMENT_SET</div><div>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 <u>update the search map</u>.</div><div><br></div><div><font color="#000000">Now let's see what your change does:</font></div><div><br></div><div><span style="color:rgb(80,0,80);font-size:12.8px"> - map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);</span><br style="color:rgb(80,0,80);font-size:12.8px"><span style="color:rgb(80,0,80);font-size:12.8px"> - if (rtems_rfs_bitmap_match(map[</span><wbr style="color:rgb(80,0,80);font-size:12.8px"><span style="color:rgb(80,0,80);font-size:12.8px">index], RTEMS_RFS_BITMAP_ELEMENT_SET))</span><br style="color:rgb(80,0,80);font-size:12.8px"><span style="color:rgb(80,0,80);font-size:12.8px"> + if (!rtems_rfs_bitmap_match(map[</span><wbr style="color:rgb(80,0,80);font-size:12.8px"><span style="color:rgb(80,0,80);font-size:12.8px">index], RTEMS_RFS_BITMAP_ELEMENT_SET))</span><br style="color:rgb(80,0,80);font-size:12.8px"><span style="color:rgb(80,0,80);font-size:12.8px"> {</span><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Your change:</div><div class="gmail_extra">1. First updates map[index] to set the bit.</div><div class="gmail_extra">2. Compares the updated map[index] with RTEMS_RFS_BITMAP_ELEMENT_SET. Note this:</div><div class="gmail_extra"> - <b>DOES NOT</b> check whether the original bit was set.</div><div class="gmail_extra"> - <b>DOES NOT</b> have a way to check if the bit was previously set, as map[index] has already been overwritten.<br></div><div class="gmail_extra"> - <b>DOES</b> check whether all bits in map[index] are set.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks,</div><div class="gmail_extra">Fan</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 12, 2017 at 10:35 AM, Chris Johns <span dir="ltr"><<a href="mailto:chrisj@rtems.org" target="_blank">chrisj@rtems.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 12/10/17 10:05 am, Fan Deng wrote:<br>
> Hi Chris,<br>
><br>
<br>
Thanks for quick response.<br>
<span class="gmail-"><br>
> Based on my understanding, the patch in your email is different:<br>
><br>
> 1) rtems_rfs_bitmap_map_set:<br>
> By changing negating the if condition, the updated logic only modifies the<br>
> element map[index] when the original value of map[index] is<br>
> RTEMS_RFS_BITMAP_ELEMENT_SET.<br>
<br>
</span>Your change has:<br>
<span class="gmail-"><br>
- map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);<br>
+ element = map[index];<br>
+ map[index] = rtems_rfs_bitmap_set (element, 1 << offset);<br>
+ // If the element does not change, the bit was already set. There will be no<br>
+ // further action to take.<br>
+ if (rtems_rfs_bitmap_match(<wbr>element, map[index]))<br>
+ return 0;<br>
<br>
</span>The change gets a copy of the uint32_t value from the map and then updates the<br>
map setting the bit. The change then checks to see if the original bit in the<br>
map is set (match returns true) and if set you exit the function. I see this<br>
being the same as the code in my patch where I only proceed to do anything if<br>
the bit is not set in the map:<br>
<span class="gmail-"><br>
- map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);<br>
- if (rtems_rfs_bitmap_match(map[<wbr>index], RTEMS_RFS_BITMAP_ELEMENT_SET))<br>
+ if (!rtems_rfs_bitmap_match(map[<wbr>index], RTEMS_RFS_BITMAP_ELEMENT_SET))<br>
{<br>
<br>
</span>If the bit is not set you enter the code to handle a map bit being set. You have:<br>
<span class="gmail-"><br>
+ control->free--;<br>
+ rtems_rfs_buffer_mark_dirty (control->buffer);<br>
if (rtems_rfs_bitmap_match(map[<wbr>index], RTEMS_RFS_BITMAP_ELEMENT_SET))<br>
{<br>
<br>
</span>Given you have set the `map[index]` earlier the if test will always pass and I<br>
suspect the compiler will know this and dead code eliminate it.<br>
<span class="gmail-"><br>
> This is not quite right, as we are explicitly updating the 'offset' bit in<br>
> map[index]. That bit should be updated regardless.<br>
<br>
</span>Why update the bit if it is already set? I seem to be missing something here.<br>
The map is just an array of 32bit unsigned ints.<br>
<span class="gmail-"><br>
><br>
> 2) rtems_rfs_bitmap_map_clear:<br>
> Same problem, whether we modify map[index] should not depend on if map[index] ==<br>
> RTEMS_RFS_BITMAP_ELEMENT_SET.<br>
<br>
</span>Sure, once we agree on the set the clear should fall out of it.<br>
<span class="gmail-HOEnZb"><font color="#888888"><br>
Chris<br>
</font></span></blockquote></div><br></div></div>