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.

Sign up for GitHub

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

Jump to bottom

issue: 3613619 Avoid posting RX WQEs for Neigh ring #78

Closed
wants to merge 1 commit into from
Closed

issue: 3613619 Avoid posting RX WQEs for Neigh ring #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/dev/cq_mgr_rx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ void cq_mgr_rx::add_hqrx(hw_queue_rx *hqrx_ptr)
/* return_extra_buffers(); */ // todo??

// Initial fill of receiver work requests
uint32_t hqrx_wr_num = hqrx_ptr->get_rx_max_wr_num();
uint32_t hqrx_wr_num = (!hqrx_ptr->is_dummy_rq() ? hqrx_ptr->get_rx_max_wr_num() : 0);
cq_logdbg("Trying to push %d WRE to allocated hqrx (%p)", hqrx_wr_num, hqrx_ptr);
while (hqrx_wr_num) {
uint32_t n_num_mem_bufs = m_n_sysvar_rx_num_wr_to_post_recv;
Expand Down
3 changes: 2 additions & 1 deletion src/core/dev/hw_queue_rx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,14 @@
#define ALIGN_WR_DOWN(_num_wr_) (std::max(32, ((_num_wr_) & ~(0xf))))

hw_queue_rx::hw_queue_rx(ring_simple *ring, ib_ctx_handler *ib_ctx,
ibv_comp_channel *rx_comp_event_channel, uint16_t vlan)
ibv_comp_channel *rx_comp_event_channel, uint16_t vlan, bool dummy)
: 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)
Copy link
Member

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.

{
hwqrx_logfunc("");

Expand Down
4 changes: 3 additions & 1 deletion src/core/dev/hw_queue_rx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class hw_queue_rx : public xlio_ti_owner {

public:
hw_queue_rx(ring_simple *ring, ib_ctx_handler *ib_ctx, ibv_comp_channel *rx_comp_event_channel,
uint16_t vlan);
uint16_t vlan, bool dummy);
virtual ~hw_queue_rx();

virtual void ti_released(xlio_ti *ti) override;
Expand All @@ -70,6 +70,7 @@ class hw_queue_rx : public xlio_ti_owner {
cq_mgr_rx *get_rx_cq_mgr() const { return m_p_cq_mgr_rx; }
uint32_t get_rx_max_wr_num() const { return m_rx_num_wr; }
uint16_t get_vlan() const { return m_vlan; };
bool is_dummy_rq() const { return m_dummy; }
void modify_queue_to_ready_state();
void modify_queue_to_error_state();
void release_rx_buffers();
Expand Down Expand Up @@ -128,6 +129,7 @@ class hw_queue_rx : public xlio_ti_owner {
uint32_t m_rx_sge = MCE_DEFAULT_RX_NUM_SGE;
const uint32_t m_n_sysvar_rx_prefetch_bytes_before_poll;
uint16_t m_vlan;
bool m_dummy; // Avoid posting WQEs to hw_queue_rx of a TX only ring.
};

#endif // HW_QUEUE_RX_H
17 changes: 5 additions & 12 deletions src/core/dev/net_device_val.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,6 @@ void ring_alloc_logic_attr::set_user_id_key(uint64_t user_id_key)
}
}

void ring_alloc_logic_attr::set_use_locks(bool use_locks)
{
if (m_use_locks != use_locks) {
m_use_locks = use_locks;
init();
}
}

const std::string ring_alloc_logic_attr::to_str() const
{
std::stringstream ss;
Expand Down Expand Up @@ -1411,10 +1403,11 @@ ring *net_device_val_eth::create_ring(resource_allocation_key *key)

try {
switch (m_bond) {
case NO_BOND:
ring = new ring_eth(get_if_idx(), nullptr, RING_ETH, true,
(key ? key->get_use_locks() : true));
break;
case NO_BOND: {
bool tx_only = (key && key->get_ring_alloc_logic() == RING_LOGIC_NEIGH);
bool use_locks = (!key || key->get_use_locks());
ring = new ring_eth(get_if_idx(), nullptr, tx_only, use_locks);
} break;
case ACTIVE_BACKUP:
case LAG_8023ad:
ring = new ring_bond_eth(get_if_idx());
Expand Down
7 changes: 3 additions & 4 deletions src/core/dev/net_device_val.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

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.

const std::string to_str() const;
inline ring_logic_t get_ring_alloc_logic() { return m_ring_alloc_logic; }
inline uint64_t get_user_id_key() { return m_user_id_key; }
inline bool get_use_locks() { return m_use_locks; }
inline ring_logic_t get_ring_alloc_logic() const { return m_ring_alloc_logic; }
inline uint64_t get_user_id_key() const { return m_user_id_key; }
inline bool get_use_locks() const { return m_use_locks; }

bool operator==(const ring_alloc_logic_attr &other) const
{
Expand Down
4 changes: 1 addition & 3 deletions src/core/dev/ring_allocation_logic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,7 @@ uint64_t ring_allocation_logic::calc_res_key_by_logic()
res_key = sched_getcpu();
break;
BULLSEYE_EXCLUDE_BLOCK_START
case RING_LOGIC_PER_OBJECT:
res_key = reinterpret_cast<uint64_t>(m_source.m_object);
break;
case RING_LOGIC_NEIGH:
case RING_LOGIC_ISOLATE:
res_key = 0;
break;
Expand Down
11 changes: 1 addition & 10 deletions src/core/dev/ring_allocation_logic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,24 +49,15 @@ class source_t {
public:
int m_fd;
ip_address m_ip;
const void *m_object;

source_t(int fd)
: m_fd(fd)
, m_ip(ip_address::any_addr())
, m_object(nullptr)
{
}
source_t(const ip_address &ip)
: m_fd(-1)
, m_ip(ip)
, m_object(nullptr)
{
}
source_t(const void *object)
: m_fd(-1)
, m_ip(ip_address::any_addr())
, m_object(object)
{
}
};
Expand Down Expand Up @@ -97,7 +88,7 @@ class ring_allocation_logic {
{
return m_ring_migration_ratio > 0 &&
m_res_key.get_ring_alloc_logic() >= RING_LOGIC_PER_THREAD &&
m_res_key.get_ring_alloc_logic() < RING_LOGIC_PER_OBJECT;
m_res_key.get_ring_alloc_logic() < RING_LOGIC_NEIGH;
}
uint64_t calc_res_key_by_logic();
inline ring_logic_t get_alloc_logic_type() { return m_res_key.get_ring_alloc_logic(); }
Expand Down
4 changes: 3 additions & 1 deletion src/core/dev/ring_simple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,11 @@ void ring_simple::create_resources()
g_p_fd_collection->add_cq_channel_fd(m_p_tx_comp_event_channel->fd, this);
}

// Currently we create hw_queue_rx also for TX-only ring as a dummy hw_queue_rx.
bool dummy_rq = (get_type() == RING_ETH_TX);
std::unique_ptr<hw_queue_tx> temp_hqtx(new hw_queue_tx(this, p_slave, get_tx_num_wr()));
std::unique_ptr<hw_queue_rx> temp_hqrx(
new hw_queue_rx(this, p_slave->p_ib_ctx, m_p_rx_comp_event_channel, m_vlan));
new hw_queue_rx(this, p_slave->p_ib_ctx, m_p_rx_comp_event_channel, m_vlan, dummy_rq));
BULLSEYE_EXCLUDE_BLOCK_START
if (!temp_hqtx || !temp_hqrx) {
ring_logerr("Failed to allocate hw_queue_tx/hw_queue_rx!");
Expand Down
10 changes: 3 additions & 7 deletions src/core/dev/ring_simple.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,18 +442,14 @@ class ring_simple : public ring_slave {

class ring_eth : public ring_simple {
public:
ring_eth(int if_index, ring *parent = NULL, ring_type_t type = RING_ETH,
bool call_create_res = true, bool use_locks = true)
: ring_simple(if_index, parent, type, use_locks)
ring_eth(int if_index, ring *parent = nullptr, bool tx_only = false, bool use_locks = true)
: ring_simple(if_index, parent, (tx_only ? RING_ETH_TX : RING_ETH), use_locks)
{
net_device_val_eth *p_ndev = dynamic_cast<net_device_val_eth *>(
g_p_net_device_table_mgr->get_net_device_val(m_parent->get_if_index()));
if (p_ndev) {
m_vlan = p_ndev->get_vlan();

if (call_create_res) {
create_resources();
}
create_resources();
}
}
};
Expand Down
4 changes: 2 additions & 2 deletions src/core/proto/neighbour.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,8 @@ neigh_entry::neigh_entry(neigh_key key, transport_type_t _type, bool is_init_res
}

// Allocate one ring for g_p_neigh_table_mgr. All eigh_entry objects will share the same ring.
ring_alloc_logic_attr ring_attr(RING_LOGIC_PER_OBJECT, true);
m_ring_allocation_logic = ring_allocation_logic_tx(g_p_neigh_table_mgr, ring_attr);
ring_alloc_logic_attr ring_attr(RING_LOGIC_NEIGH, true);
m_ring_allocation_logic = ring_allocation_logic_tx(-1, ring_attr);

if (is_init_resources) {
m_p_ring = m_p_dev->reserve_ring(m_ring_allocation_logic.get_key());
Expand Down
4 changes: 2 additions & 2 deletions src/core/util/xlio_stats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_ETH_TX, RING_TAP } ring_type_t;

static const char *const ring_type_str[] = {"RING_ETH", "RING_TAP"};
static const char *const ring_type_str[] = {"RING_ETH", "RING_ETH_TX", "RING_TAP"};

// Ring stat info
typedef struct {
Expand Down
2 changes: 1 addition & 1 deletion src/core/xlio_extra.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,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
Copy link
Member

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**
}

Copy link
Collaborator Author

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.

Copy link
Member

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.

} ring_logic_t;
Expand Down