<div dir="ltr">Hi Chris,<div><br></div><div>Based on my understanding, the patch in your email is different:</div><div><br></div><div>1) rtems_rfs_bitmap_map_set:</div><div>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.</div><div><br></div><div>This is not quite right, as we are explicitly updating the 'offset' bit in map[index]. That bit should be updated regardless.</div><div><br></div><div>2) rtems_rfs_bitmap_map_clear:</div><div>Same problem, whether we modify map[index] should not depend on if map[index] == RTEMS_RFS_BITMAP_ELEMENT_SET.</div><div><br></div><div>I believe my patch corrects the logic flaw and is the right fix. Please let me know if you have any further questions, and I will be happy to explain in detail.</div><div><br></div><div>Thanks,</div><div>Fan</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 12, 2017 at 9:48 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
Many thanks for post the patches. The other patches look fine. I am wondering is this is equivalent to what you have?<br>
<br>
diff --git a/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c b/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
index 15a9050133..d14082691a 100644<br>
--- a/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
+++ b/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
@@ -196,9 +196,9 @@ rtems_rfs_bitmap_map_set (rtems_rfs_bitmap_control* control,<br>
<span class=""> search_map = control->search_bits;<br>
index = rtems_rfs_bitmap_map_index (bit);<br>
offset = rtems_rfs_bitmap_map_offset (bit);<br>
- map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);<br>
</span>- 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>
+ map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);<br>
<span class=""> bit = index;<br>
index = rtems_rfs_bitmap_map_index (bit);<br>
offset = rtems_rfs_bitmap_map_offset (bit);<br>
</span>@@ -226,6 +226,8 @@ rtems_rfs_bitmap_map_clear (rtems_rfs_bitmap_control* control,<br>
<span class=""> search_map = control->search_bits;<br>
index = rtems_rfs_bitmap_map_index (bit);<br>
offset = rtems_rfs_bitmap_map_offset (bit);<br>
</span>+ if (rtems_rfs_bitmap_match(map[<wbr>index], RTEMS_RFS_BITMAP_ELEMENT_SET))<br>
+ {<br>
map[index] = rtems_rfs_bitmap_clear (map[index], 1 << offset);<br>
<span class=""> bit = index;<br>
index = rtems_rfs_bitmap_map_index (bit);<br>
</span>@@ -233,6 +235,7 @@ rtems_rfs_bitmap_map_clear (rtems_rfs_bitmap_control* control,<br>
search_map[index] = rtems_rfs_bitmap_clear (search_map[index], 1 << offset);<br>
rtems_rfs_buffer_mark_dirty (control->buffer);<br>
control->free++;<br>
+ }<br>
return 0;<br>
}<br>
<span class="HOEnZb"><font color="#888888"><br>
Chris<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On 10/10/17 4:28 pm, Fan Deng wrote:<br>
> The bitmap allocation accounting logic in rtems-rfs-bitmaps.c is flawed<br>
> around control->free. Specifically:<br>
><br>
> In rtems_rfs_bitmap_map_set():<br>
> control->free is only decremented when its corresponding search bit is<br>
> toggled. This is wrong and will miss on average 31/32 set updates.<br>
><br>
> In rtems_rfs_bitmap_map_clear():<br>
> control->free is incremented unconditionally.<br>
><br>
> The correct behavior is:<br>
> When updating the map, check if the bit is already set/clear. Only update<br>
> control->free when the bit is toggled.<br>
><br>
> This change enforced the correct behavior.<br>
><br>
> Tested by inspecting the internal data structure.<br>
> ---<br>
> cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c | 40 +++++++++++++++++++++---------<wbr>--<br>
> 1 file changed, 26 insertions(+), 14 deletions(-)<br>
><br>
> diff --git a/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c b/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
> index 15a9050133..c0c6921db1 100644<br>
> --- a/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
> +++ b/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
> @@ -183,11 +183,12 @@ int<br>
> rtems_rfs_bitmap_map_set (rtems_rfs_bitmap_control* control,<br>
> rtems_rfs_bitmap_bit bit)<br>
> {<br>
> - rtems_rfs_bitmap_map map;<br>
> - rtems_rfs_bitmap_map search_map;<br>
> - int index;<br>
> - int offset;<br>
> - int rc;<br>
> + rtems_rfs_bitmap_map map;<br>
> + rtems_rfs_bitmap_map search_map;<br>
> + int index;<br>
> + int offset;<br>
> + int rc;<br>
> + rtems_rfs_bitmap_element element;<br>
> rc = rtems_rfs_bitmap_load_map (control, &map);<br>
> if (rc > 0)<br>
> return rc;<br>
> @@ -196,15 +197,20 @@ rtems_rfs_bitmap_map_set (rtems_rfs_bitmap_control* control,<br>
> search_map = control->search_bits;<br>
> index = rtems_rfs_bitmap_map_index (bit);<br>
> offset = rtems_rfs_bitmap_map_offset (bit);<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>
> + 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>
> bit = index;<br>
> index = rtems_rfs_bitmap_map_index (bit);<br>
> offset = rtems_rfs_bitmap_map_offset (bit);<br>
> search_map[index] = rtems_rfs_bitmap_set (search_map[index], 1 << offset);<br>
> - control->free--;<br>
> - rtems_rfs_buffer_mark_dirty (control->buffer);<br>
> }<br>
> return 0;<br>
> }<br>
> @@ -213,11 +219,12 @@ int<br>
> rtems_rfs_bitmap_map_clear (rtems_rfs_bitmap_control* control,<br>
> rtems_rfs_bitmap_bit bit)<br>
> {<br>
> - rtems_rfs_bitmap_map map;<br>
> - rtems_rfs_bitmap_map search_map;<br>
> - int index;<br>
> - int offset;<br>
> - int rc;<br>
> + rtems_rfs_bitmap_map map;<br>
> + rtems_rfs_bitmap_map search_map;<br>
> + int index;<br>
> + int offset;<br>
> + int rc;<br>
> + rtems_rfs_bitmap_element element;<br>
> rc = rtems_rfs_bitmap_load_map (control, &map);<br>
> if (rc > 0)<br>
> return rc;<br>
> @@ -226,7 +233,12 @@ rtems_rfs_bitmap_map_clear (rtems_rfs_bitmap_control* control,<br>
> search_map = control->search_bits;<br>
> index = rtems_rfs_bitmap_map_index (bit);<br>
> offset = rtems_rfs_bitmap_map_offset (bit);<br>
> - map[index] = rtems_rfs_bitmap_clear (map[index], 1 << offset);<br>
> + element = map[index];<br>
> + map[index] = rtems_rfs_bitmap_clear (element, 1 << offset);<br>
> + // If the element does not change, the bit was already clear. There will be no<br>
> + // further action to take.<br>
> + if (rtems_rfs_bitmap_match(<wbr>element, map[index]))<br>
> + return 0;<br>
> bit = index;<br>
> index = rtems_rfs_bitmap_map_index (bit);<br>
> offset = rtems_rfs_bitmap_map_offset(<wbr>bit);<br>
><br>
</div></div></blockquote></div><br></div></div>