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

prov/efa: Remove rxr_ep->rdm_ep #8418

Merged
merged 2 commits into from
Jan 23, 2023
Merged

Conversation

sunkuamzn
Copy link
Contributor

This commit removes the rdm_ep member of rxr_ep. The RDM endpoint no longer uses the DGRAM endpoint as a lower endpoint

@sunkuamzn sunkuamzn force-pushed the ep-squash-no-rdm-ep branch from f9d2b9d to a7fecf9 Compare January 14, 2023 02:20
@darrylabbate
Copy link
Member

Was it intentional to include the same commit (5b481c0) as in #8416?

/*
* Specific flags and attributes for shm provider
*/
#define EFA_SHM_MAX_AV_COUNT (256)
Copy link
Contributor

Choose a reason for hiding this comment

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

EFA_SHM_XXX should not have been moved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only moved things necessary for efa_base_ep now

@sunkuamzn sunkuamzn force-pushed the ep-squash-no-rdm-ep branch from a7fecf9 to 227a38e Compare January 20, 2023 05:32
Copy link
Contributor

@wzamazon wzamazon left a comment

Choose a reason for hiding this comment

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

Thanks! left A few minor comments

@sunkuamzn sunkuamzn force-pushed the ep-squash-no-rdm-ep branch from 227a38e to ee823d5 Compare January 20, 2023 21:56
@sunkuamzn sunkuamzn force-pushed the ep-squash-no-rdm-ep branch from ee823d5 to cd60297 Compare January 20, 2023 22:24
#include "ofi_util.h"
#include "ofi_file.h"

#include "efa_av.h"
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. efa_base_ep.c isn't importing efa_av.h

also, if we're going to be so careful with imports, we need to spend a week cleaning up the rest of the code base

Copy link
Member

Choose a reason for hiding this comment

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

yes. efa_base_ep.c isn't importing efa_av.h

Does efa.h not include efa_av.h?

also, if we're going to be so careful with imports

It's not really about carefulness in the sense that it's a correctness thing (it will function just fine); it's more of a code quality thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does efa.h not include efa_av.h?

Not directly, no. Is this your end goal: efa.h includes every DGRAM header and all DGRAM C files only include efa.h? That's currently not the case. We can make a task to clean up the includes in this style, if you wish.

@sunkuamzn sunkuamzn force-pushed the ep-squash-no-rdm-ep branch from cd60297 to 6eb50e6 Compare January 20, 2023 22:52
RDM endpoint now uses rxr_ep->base_ep.av

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
This commit removes the rdm_ep member of rxr_ep. The RDM endpoint no
longer uses the DGRAM endpoint as a lower endpoint

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
@wzamazon wzamazon merged commit 25047e2 into ofiwg:main Jan 23, 2023
@sunkuamzn sunkuamzn deleted the ep-squash-no-rdm-ep branch September 19, 2023 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants