Skip to content

prov/efa: Minor changes necessary for the protocol path refactor#12010

Open
sunkuamzn wants to merge 4 commits intoofiwg:mainfrom
sunkuamzn:proto-callback-prereq
Open

prov/efa: Minor changes necessary for the protocol path refactor#12010
sunkuamzn wants to merge 4 commits intoofiwg:mainfrom
sunkuamzn:proto-callback-prereq

Conversation

@sunkuamzn
Copy link
Copy Markdown
Contributor

Prerequisite for #12002

Comment thread prov/efa/src/rdm/efa_rdm_ep_utils.c
Comment thread prov/efa/src/rdm/efa_rdm_msg.c Outdated
Comment thread prov/efa/src/rdm/efa_rdm_msg.c Outdated
@sunkuamzn sunkuamzn force-pushed the proto-callback-prereq branch from a486551 to 76b9f74 Compare March 17, 2026 21:10
@sunkuamzn sunkuamzn requested a review from a team March 18, 2026 01:20
Comment thread prov/efa/src/rdm/efa_rdm_ep.h Outdated
Comment thread prov/efa/src/rdm/efa_rdm_ep.h Outdated
/* Work arrays for efa_rdm_ope_post_send to avoid stack allocation */
struct efa_rdm_pke **send_pkt_entry_vec;
int *send_pkt_entry_size_vec;
int *send_pkt_entry_data_sizes;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should it not be rename send_pkt_entry_vec?

{
ssize_t err;
int rtm_type, use_p2p;

Copy link
Copy Markdown

@talavr-amazon talavr-amazon Mar 18, 2026

Choose a reason for hiding this comment

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

please use zero copy in the commit message. also the commit message should explain the dependency

Comment thread prov/efa/src/rdm/efa_rdm_msg.c Outdated
Comment thread prov/efa/src/rdm/efa_rdm_msg.c Outdated
Comment thread prov/efa/src/rdm/efa_rdm_atomic.c
@sunkuamzn sunkuamzn force-pushed the proto-callback-prereq branch 5 times, most recently from ebd47a2 to afaf415 Compare April 10, 2026 23:38
The pkt entries are stored in send_pkt_entry_vec, so the size should
also be stored in the endpoint

Also rename send_pkt_entry_size_vec to send_pkt_entry_data_sizes to
avoid confusion between the two members

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
If the send queue is full, no point creating txe and pkes

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
@sunkuamzn sunkuamzn force-pushed the proto-callback-prereq branch from afaf415 to 88ed8af Compare April 11, 2026 01:26
Remove the efa_rdm_ep_alloc_txe helper and inline its logic at each call
site. The function combined pool allocation, TXE construction, tagged
field setup, and list insertion in a way that made it difficult to
separate allocation from initialization for the new protocol interface.

Move the dlist_insert_tail for ep->txe_list into efa_rdm_txe_construct
so that all callers get consistent list management without duplicating
the insertion call.

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
Move efa_rdm_ep_get_memory_alignment from efa_rdm_ep_utils.c to
efa_rdm_ep.h as a static inline function. The function is small and
called in the packet construction hot path, so inlining avoids function
call overhead. Also move the EFA_RDM_*_ALIGNMENT constant definitions
from efa.h to efa_rdm_ep.h to keep them co-located with the function
that uses them.

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
@@ -195,7 +195,8 @@ struct efa_rdm_ep {
struct efa_rdm_pke **pke_vec;
/* Work arrays for efa_rdm_ope_post_send to avoid stack allocation */
struct efa_rdm_pke **send_pkt_entry_vec;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will it be cleaner if we create a send_pkt_entry struct to wrap these 3 together?

/* Work arrays for efa_rdm_ope_post_send to avoid stack allocation */
struct efa_rdm_pke **send_pkt_entry_vec;
int *send_pkt_entry_size_vec;
size_t *send_pkt_entry_data_sizes;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: if they're not bundled together, maybe name it send_pkt_entry_vec_data/payload_sizes

ssize_t err;
struct efa_rdm_ope *txe;
struct efa_rdm_peer *peer;
int available_tx_pkts;
Copy link
Copy Markdown

@talavr-amazon talavr-amazon Apr 15, 2026

Choose a reason for hiding this comment

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

unsigned or add an assert that it's >0?

if (!txe)
return NULL;

efa_rdm_txe_construct(txe, efa_rdm_ep, peer, &msg, op, 0);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should we not check for ofi_op_tagged here?

/*
* The default memory alignment
*/
#define EFA_RDM_DEFAULT_MEMORY_ALIGNMENT (8)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if these are place in this header and are endpoint specific values, they should be prefixed with EFA_RDM_EP_

struct efa_rdm_pke **send_pkt_entry_vec;
int *send_pkt_entry_size_vec;
size_t *send_pkt_entry_data_sizes;
size_t send_pkt_entry_vec_size;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought we need a new member in ep only when

  1. The member needs a persistent allocation
  2. The member can be used in different function calls through the ep lifecycle

Is the send_pkt_entry_vec_size fitting any of them? Per this commit it seems it is only used in the efa_rdm_ope_post_send? Is it a preparation for future refactor?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it is a prerequisite for protocol refactor (callback), I think this field should belong to a struct for protocol instead of ep

}

// Handle case when there are no TX packets available
available_tx_pkts = ep->efa_max_outstanding_tx_ops -
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe better to have it as a static inline util function as it may be used by multiple places

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.

4 participants