Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update context attribute query #1020

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nsarka
Copy link
Collaborator

@nsarka nsarka commented Sep 10, 2024

From the UCC Backlog:

Check all TLs and CLs lib and context attributes

Attribute query function should follow this scheme:
if attr.mask is not zero check each attributed defined in mask
Set attribute according to Tl/CL capabilities
Set attr.flags always

This logic is generally broker in UCC, for example
ucc_tl_self_get_context_attr(const ucc_base_context_t *context, /* NOLINT */
                             ucc_base_ctx_attr_t      *attr)
{
    return UCC_OK;
}

attr.topo_required is not set (returns something random)
attr.global_work_buffer_size is not set
etc.

This PR updates UCC so that all context attribute queries read the mask for each field and then write a value, usually 0. Some of these were missing.

The backlog task mentions updating lib attributes as well, but it seems to me that both CL and TL lib attributes are filled correctly. There are two unused fields not filled--reduction types and sync type--but if the mask has these bits set UCC will error before it reaches the CL or TL (see ucc_lib_get_attr).

@nsarka nsarka self-assigned this Sep 10, 2024
@nsarka nsarka requested a review from janjust September 10, 2024 20:14
@nsarka nsarka changed the title CODESTYLE: Update ctx attrs Update context attribute query Sep 10, 2024
@janjust
Copy link
Collaborator

janjust commented Sep 10, 2024

@nsarka IMO, I think an easier to read and understand approach is to simply define a zero_attr() function that will zero out the attributes prior to going over the mask bit and setting everything as needed.
Rather than going through all the bitmask fields and individually zero everything out

@nsarka
Copy link
Collaborator Author

nsarka commented Sep 11, 2024

@nsarka IMO, I think an easier to read and understand approach is to simply define a zero_attr() function that will zero out the attributes prior to going over the mask bit and setting everything as needed. Rather than going through all the bitmask fields and individually zero everything out

Updated

@janjust
Copy link
Collaborator

janjust commented Sep 16, 2024

@nsarka please squash commits and rebase

Copy link
Collaborator

@janjust janjust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I just have to caution that the user must know, either through documentation or otherwise that the attr is in fact cleared and set to only masked values

Copy link
Collaborator

@samnordmann samnordmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Please update copyrights

@nsarka
Copy link
Collaborator Author

nsarka commented Sep 18, 2024

Copyright updated and commits squashed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants