Hi, The patch LGTM overall. I had tested the v1 and it worked fine.
> That function was added by commit ee1b30f, which AFAICT used an exclusive > lock just to stay consistent with the rest of dsa.c [0]. I don't see any > discussion about this in the original DSA thread [1]. Perhaps we could go > through dsa.c and switch to LW_SHARED where appropriate, although I doubt > it makes much difference. > Thank you for highlighting the discussions. I'm unsure about the best approach here, but I think it would be safe to stay consistent with the rest of the code in dsa.c, especially since it's unclear that the use of LW_EXCLUSIVE for reading values in dsa is a mistake. > > > +size_t > > +dsa_get_total_size_from_handle(dsa_handle handle) > > > > I believe this function will report the size as long as the dsa control > > structure is created within a dsm segment, since all dsm segments are > > tracked by the global list - dsm_segment_list, regardless of whether the > > dsa is created with dsa_create or dsa_create_in_place. In that case, > > perhaps we should update the comment above to reflect this. > > Sorry, I'm not following what you think we should update the comment to > say. > Sorry for the confusion, I am trying to say that we can change the following comment + *The area must have + * been created with dsa_create (not dsa_create_in_place). to say this: "The area must have been created using dsm_segments" Since, this function can report the size of an area created with dsa_create_in_place too, as long as the area is created using dsm_segments. > > 4. Since, with this change, the size column will show memory allocation > > regardless of whether it is currently mapped in the local process, I > > think it would be helpful to add a boolean column to display the mapped > > status as a future enhancement. > > Maybe, although I'm struggling to think of a scenario where that > information would be useful. > > Fair enough. I was thinking of a scenario where a user might want to see how much dsa memory is allocated in the client backend process. However, I understand now that this view is designed for the entire cluster, and adding a column which is process-specific could lead to confusion. Thank you, Rahila Syed
