-
Notifications
You must be signed in to change notification settings - Fork 19
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
issue: 3613619 Avoid posting RX WQEs for Neigh ring #78
Conversation
a5e4106
to
1a1ec20
Compare
I called this RING_ETH_TX. Let's discuss later if we want to change it to RING_ETH_NEIGH.. |
1a1ec20
to
5a67a4d
Compare
src/core/util/xlio_stats.h
Outdated
@@ -351,9 +351,9 @@ typedef struct { | |||
cq_stats_t cq_stats; | |||
} cq_instance_block_t; | |||
|
|||
typedef enum { RING_ETH = 0, RING_TAP } ring_type_t; | |||
typedef enum { RING_ETH = 0, RING_TAP, RING_ETH_TX } ring_type_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order ETH, ETH_TX, TAP looks more readable (if you change order here, change it for the strings array too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -271,7 +271,7 @@ typedef enum { | |||
RING_LOGIC_PER_THREAD = 20, //!< RING_LOGIC_PER_THREAD | |||
RING_LOGIC_PER_CORE = 30, //!< RING_LOGIC_PER_CORE | |||
RING_LOGIC_PER_CORE_ATTACH_THREADS = 31, //!< RING_LOGIC_PER_CORE_ATTACH_THREADS | |||
RING_LOGIC_PER_OBJECT = 32, //!< RING_LOGIC_PER_OBJECT | |||
RING_LOGIC_NEIGH = 32, //!< RING_LOGIC_NEIGH | |||
RING_LOGIC_ISOLATE = 33, //!< RING_LOGIC_ISOLATE | |||
RING_LOGIC_LAST //!< RING_LOGIC_LAST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion]
xlio_extra.h:
enum {
...
RING_LOGIC_PER_CORE_ATTACH_THREADS = 31,
RING_LOGIC_LAST = 100
};
ring_allocation_logic.h:
/* Internal ring allocation logic which is hidden from user. */
enum {
RING_LOGIC_NEIGH = RING_LOGIC_LAST + 1,
RING_LOGIC_ISOLATE,
};
ring_allocation_logic.cpp:
bool is_logic_support_migration()
{
...
m_res_key.get_ring_alloc_logic() < **RING_LOGIC_LAST**
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this change. It will create confusing conversions between ring_logic_t and ring_logic_public_t.
If we have to hide internal logics from the user of xlio_extra maybe it is better:
move ring_logic_t to ring_allocation_logic.h and make
enum {
...
RING_LOGIC_PER_CORE_ATTACH_THREADS = 31
} ring_logic_public_t
just for extra_api.h usage. The negative of this approach is that we have kind of duplicate definitions.
The best would be deriving one enum from another, but this is not possible in CPP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is extra API and will be replaced by storage api with the groups. Depending on further steps, we may even remove all the obsolete extra API. So lets keep it as is and improve once we do cleanup.
Neigh ring is used for TX only, to send ARP/ICMPv6 packets. However each XLIO ring creates both SQ and RQ. By avoiding posting RX WQEs to Neigh ring RQ, a considerable amount of memory is saved. Signed-off-by: Alexander Grissik <[email protected]>
5a67a4d
to
bc77ea7
Compare
I rebased on top of vNext. |
@@ -61,11 +61,10 @@ class ring_alloc_logic_attr { | |||
ring_alloc_logic_attr(const ring_alloc_logic_attr &other); | |||
void set_ring_alloc_logic(ring_logic_t logic); | |||
void set_user_id_key(uint64_t user_id_key); | |||
void set_use_locks(bool use_locks); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove it, new storage API uses it.
: m_p_ring(ring) | ||
, m_p_ib_ctx_handler(ib_ctx) | ||
, m_n_sysvar_rx_num_wr_to_post_recv(safe_mce_sys().rx_num_wr_to_post_recv) | ||
, m_rx_num_wr(align32pow2(safe_mce_sys().rx_num_wr)) | ||
, m_n_sysvar_rx_prefetch_bytes_before_poll(safe_mce_sys().rx_prefetch_bytes_before_poll) | ||
, m_vlan(vlan) | ||
, m_dummy(dummy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to make decision on dummy attribute in the constructor instead of storing it (lets say pass the information about dummy or number of buffers to confiure_rq()
). If the RQ is really dummy, it won't be used and won't trigger any methods to post new buffers to the RQ.
If this cannot be done in context of constructor, then leave it in current state.
The right approach is to split ring to ring_tx and ring_rx. Closing this PR as this does not resolve anything serious. |
Description
Neigh ring was used only for TX and wasted RX buffers.
What
For Neigh ring we mark this ring as TX only and when QP is created RX WQEs are not posted to it's RQ.
This allows saving memory especially for multi process use cases.
Why ?
Avoid waste of memory.
Change type
What kind of change does this PR introduce?
Check list