| Message ID | 20200401142127.13715-11-mk@semihalf.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Ferruh Yigit |
| Headers | show |
| Series | Update ENA driver to v2.1.0 | expand |
| Context | Check | Description |
|---|---|---|
| ci/Intel-compilation | success | Compilation OK |
| ci/checkpatch | warning | coding style issues |
On 4/1/2020 3:21 PM, Michal Krawczyk wrote: > To make the debugging easier, the error logs were added in the Tx path. > > Signed-off-by: Michal Krawczyk <mk@semihalf.com> > Reviewed-by: Igor Chauskin <igorch@amazon.com> > Reviewed-by: Guy Tzalik <gtzalik@amazon.com> > --- > drivers/net/ena/base/ena_eth_com.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ena/base/ena_eth_com.c b/drivers/net/ena/base/ena_eth_com.c > index aabc294fb7..747450fec5 100644 > --- a/drivers/net/ena/base/ena_eth_com.c > +++ b/drivers/net/ena/base/ena_eth_com.c > @@ -148,8 +148,10 @@ static int ena_com_close_bounce_buffer(struct ena_com_io_sq *io_sq) > if (pkt_ctrl->idx) { > rc = ena_com_write_bounce_buffer_to_dev(io_sq, > pkt_ctrl->curr_bounce_buf); > - if (unlikely(rc)) > + if (unlikely(rc)) { > + ena_trc_err("failed to write bounce buffer to device\n"); Is 'ena_trc_err()' for datapath? DPDK has two types of logs, - for control path, dynamic logging, whose logging level can be dynamically changed - for data path, a compile time controlled logging, this is to prevent performance loss in data path. For ena these flags are CONFIG_RTE_LIBRTE_ENA_DEBUG_RX=n CONFIG_RTE_LIBRTE_ENA_DEBUG_TX=n CONFIG_RTE_LIBRTE_ENA_DEBUG_TX_FREE=n I can see ena also has 'RTE_LIBRTE_ENA_COM_DEBUG' compile to flag, which controls above 'ena_trc_err' variants. If this is control path, what do you think removing the compile time flag and convert these 'COM' logs to dynamic logging?
czw., 2 kwi 2020 o 14:53 Ferruh Yigit <ferruh.yigit@intel.com> napisał(a): > > On 4/1/2020 3:21 PM, Michal Krawczyk wrote: > > To make the debugging easier, the error logs were added in the Tx path. > > > > Signed-off-by: Michal Krawczyk <mk@semihalf.com> > > Reviewed-by: Igor Chauskin <igorch@amazon.com> > > Reviewed-by: Guy Tzalik <gtzalik@amazon.com> > > --- > > drivers/net/ena/base/ena_eth_com.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/ena/base/ena_eth_com.c b/drivers/net/ena/base/ena_eth_com.c > > index aabc294fb7..747450fec5 100644 > > --- a/drivers/net/ena/base/ena_eth_com.c > > +++ b/drivers/net/ena/base/ena_eth_com.c > > @@ -148,8 +148,10 @@ static int ena_com_close_bounce_buffer(struct ena_com_io_sq *io_sq) > > if (pkt_ctrl->idx) { > > rc = ena_com_write_bounce_buffer_to_dev(io_sq, > > pkt_ctrl->curr_bounce_buf); > > - if (unlikely(rc)) > > + if (unlikely(rc)) { > > + ena_trc_err("failed to write bounce buffer to device\n"); > > Is 'ena_trc_err()' for datapath? > ena_trc_err() is being used in both control and datapath. > DPDK has two types of logs, > - for control path, dynamic logging, whose logging level can be dynamically changed > - for data path, a compile time controlled logging, this is to prevent > performance loss in data path. For ena these flags are > CONFIG_RTE_LIBRTE_ENA_DEBUG_RX=n > CONFIG_RTE_LIBRTE_ENA_DEBUG_TX=n > CONFIG_RTE_LIBRTE_ENA_DEBUG_TX_FREE=n > > > I can see ena also has 'RTE_LIBRTE_ENA_COM_DEBUG' compile to flag, which > controls above 'ena_trc_err' variants. If this is control path, what do you > think removing the compile time flag and convert these 'COM' logs to dynamic > logging? I agree with you that it would be great if we would use dynamic logging for control path - I must talk with engineers responsible for ena_com to raise that issue and, hopefully, it'll be possible to adjust that in the future. I suggest to postpone that kind of change for the next release, as making ena_com changes is a bit time consuming.
diff --git a/drivers/net/ena/base/ena_eth_com.c b/drivers/net/ena/base/ena_eth_com.c index aabc294fb7..747450fec5 100644 --- a/drivers/net/ena/base/ena_eth_com.c +++ b/drivers/net/ena/base/ena_eth_com.c @@ -148,8 +148,10 @@ static int ena_com_close_bounce_buffer(struct ena_com_io_sq *io_sq) if (pkt_ctrl->idx) { rc = ena_com_write_bounce_buffer_to_dev(io_sq, pkt_ctrl->curr_bounce_buf); - if (unlikely(rc)) + if (unlikely(rc)) { + ena_trc_err("failed to write bounce buffer to device\n"); return rc; + } pkt_ctrl->curr_bounce_buf = ena_com_get_next_bounce_buffer(&io_sq->bounce_buf_ctrl); @@ -179,8 +181,10 @@ static int ena_com_sq_update_llq_tail(struct ena_com_io_sq *io_sq) if (!pkt_ctrl->descs_left_in_line) { rc = ena_com_write_bounce_buffer_to_dev(io_sq, pkt_ctrl->curr_bounce_buf); - if (unlikely(rc)) + if (unlikely(rc)) { + ena_trc_err("failed to write bounce buffer to device\n"); return rc; + } pkt_ctrl->curr_bounce_buf = ena_com_get_next_bounce_buffer(&io_sq->bounce_buf_ctrl); @@ -394,8 +398,10 @@ int ena_com_prepare_tx(struct ena_com_io_sq *io_sq, } if (unlikely(io_sq->mem_queue_type == ENA_ADMIN_PLACEMENT_POLICY_DEV - && !buffer_to_push)) + && !buffer_to_push)) { + ena_trc_err("push header wasn't provided on LLQ mode\n"); return ENA_COM_INVAL; + } rc = ena_com_write_header_to_bounce(io_sq, buffer_to_push, header_len); if (unlikely(rc)) @@ -410,6 +416,8 @@ int ena_com_prepare_tx(struct ena_com_io_sq *io_sq, /* If the caller doesn't want to send packets */ if (unlikely(!num_bufs && !header_len)) { rc = ena_com_close_bounce_buffer(io_sq); + if (rc) + ena_trc_err("failed to write buffers to LLQ\n"); *nb_hw_desc = io_sq->tail - start_tail; return rc; } @@ -469,8 +477,10 @@ int ena_com_prepare_tx(struct ena_com_io_sq *io_sq, /* The first desc share the same desc as the header */ if (likely(i != 0)) { rc = ena_com_sq_update_tail(io_sq); - if (unlikely(rc)) + if (unlikely(rc)) { + ena_trc_err("failed to update sq tail\n"); return rc; + } desc = get_sq_desc(io_sq); if (unlikely(!desc)) @@ -499,10 +509,14 @@ int ena_com_prepare_tx(struct ena_com_io_sq *io_sq, desc->len_ctrl |= ENA_ETH_IO_TX_DESC_LAST_MASK; rc = ena_com_sq_update_tail(io_sq); - if (unlikely(rc)) + if (unlikely(rc)) { + ena_trc_err("failed to update sq tail of the last descriptor\n"); return rc; + } rc = ena_com_close_bounce_buffer(io_sq); + if (rc) + ena_trc_err("failed when closing bounce buffer\n"); *nb_hw_desc = io_sq->tail - start_tail; return rc;