On 2/21/23 5:42 PM, Michał Mirosław wrote:
On Tue, 21 Feb 2023 at 11:28, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
Hi Michał,
Thank you so much for comment!
On 2/17/23 8:18 PM, Michał Mirosław wrote:
[...]
For the page-selection mechanism, currently required_mask and excluded_mask have conflicting
They are opposite of each other: All the set bits in required_mask must be set for the page to be selected. All the set bits in excluded_mask must _not_ be set for the page to be selected.
responsibilities. I suggest to rework that to:
- negated_flags: page flags which are to be negated before applying
the page selection using following masks;
Sorry I'm unable to understand the negation (which is XOR?). Lets look at the truth table: Page Flag negated_flags 0 0 0 0 1 1 1 0 1 1 1 0
If a page flag is 0 and negated_flag is 1, the result would be 1 which has changed the page flag. It isn't making sense to me. Why the page flag bit is being fliped?
When Anrdei had proposed these masks, they seemed like a fancy way of filtering inside kernel and it was straight forward to understand. These masks would help his use cases for CRIU. So I'd included it. Please can you elaborate what is the purpose of negation?
The XOR is a way to invert the tested value of a flag (from positive to negative and the other way) without having the API with invalid values (with required_flags and excluded_flags you need to define a rule about what happens if a flag is present in both of the masks - either prioritise one mask over the other or reject the call).
At minimum, one mask (required, any or excluded) must be specified. For a page to get selected, the page flags must fulfill the criterion of all the specified masks.
If a flag is present in both required_mask and excluded_mask, the required_mask would select a page. But exculded_mask would drop the page. So page page would be dropped. It is responsibility of the user to correctly specify the flags.
matched = true; if (p->required_mask) matched = ((p->required_mask & bitmap) == p->required_mask); if (matched && p->anyof_mask) matched = (p->anyof_mask & bitmap); if (matched && p->excluded_mask) matched = !(p->excluded_mask & bitmap);
if (matched && bitmap) { // page selected }
Do you accept/like this behavior of masks after explaintation?
(Note: the XOR is applied only to the value of the flags for the purpose of testing page-selection criteria.)
So:
- if a flag is not set in negated_flags, but set in required_flags,
then it means "this flag must be one" - equivalent to it being set in required_flag (in your current version of the API). 2. if a flag is set in negated_flags and also in required_flags, then it means "this flag must be zero" - equivalent to it being set in excluded_flags.
Lets translate words into table: pageflags required_flags negated_flags matched 1 1 0 yes 0 1 1 yes
The same thing goes for anyof_flags: if a flag is set in anyof_flags, then for it to be considered matched:
- it must have a value of 1 if it is not set in negated_flags
- it must have a value of 0 if it is set in negated_flags
pageflags anyof_flags negated_flags matched 1 1 0 yes 0 1 1 yes
BTW, I think I assumed that both conditions (all flags in required_flags and at least one in anyof_flags is present) need to be true for the page to be selected - is this your intention?
All the masks are optional. If all or any of the 3 masks are specified, the page flags must pass these masks to get selected.
The example code has a bug though, in that if anyof_flags is zero it will never match. Let me fix the selection part:
// calc. a mask of flags that have expected ("active") values tested_flags = page_flags ^ negated_flags; // are all required flags in "active" state? [== all zero when negated] if (~tested_flags & required_mask) skip page; // is any extra flag "active"? if (anyof_flags && !(tested_flags & anyof_flags)) skip page;
After taking a while to understand this and compare with already present flag system, `negated flags` is comparatively difficult to understand while already present flags seem easier.
Best Regards Michał Mirosław