Message ID | 20181030135427.19036-5-andrew.boyer@dell.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | RDMA/rxe: skb handling, link state, statistics | expand |
> On Oct 30, 2018, at 4:02 PM, Andrew Boyer <andrew.boyer@dell.com> wrote: > > Signed-off-by: Andrew Boyer <andrew.boyer@dell.com> > --- > drivers/infiniband/sw/rxe/rxe_comp.c | 6 ++++++ > drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++++-- > drivers/infiniband/sw/rxe/rxe_hw_counters.h | 3 +++ > drivers/infiniband/sw/rxe/rxe_net.c | 1 + > drivers/infiniband/sw/rxe/rxe_resp.c | 3 ++- > 5 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c > index 6cdc40ed8a9f..6dc99ca31b5f 100644 > --- a/drivers/infiniband/sw/rxe/rxe_comp.c > +++ b/drivers/infiniband/sw/rxe/rxe_comp.c > @@ -428,6 +428,7 @@ static void make_send_cqe(struct rxe_qp *qp, struct rxe_send_wqe *wqe, > */ > static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe) > { > + struct rxe_dev *rxe = to_rdev(qp->ibqp.device); > struct rxe_cqe cqe; > > if ((qp->sq_sig_type == IB_SIGNAL_ALL_WR) || > @@ -440,6 +441,11 @@ static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe) > advance_consumer(qp->sq.queue); > } > > + if (wqe->wr.opcode == IB_WR_SEND || > + wqe->wr.opcode == IB_WR_SEND_WITH_IMM || > + wqe->wr.opcode == IB_WR_SEND_WITH_INV) > + rxe_counter_inc(rxe, RXE_CNT_RDMA_SEND); > + > /* > * we completed something so let req run again > * if it is trying to fence > diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c b/drivers/infiniband/sw/rxe/rxe_hw_counters.c > index 6aeb7a165e46..4a24895846d3 100644 > --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c > +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c > @@ -37,15 +37,18 @@ static const char * const rxe_counter_name[] = { > [RXE_CNT_SENT_PKTS] = "sent_pkts", > [RXE_CNT_RCVD_PKTS] = "rcvd_pkts", Can you please add some description maybe in the commit message what’s the difference between above two counters and the new ones introduced in this patch? > [RXE_CNT_DUP_REQ] = "duplicate_request", > - [RXE_CNT_OUT_OF_SEQ_REQ] = "out_of_sequence", > + [RXE_CNT_OUT_OF_SEQ_REQ] = "out_of_seq_request", IMO it’s worth mentioning in the commit message the two renaming changes since they are visible in sysfs.. > [RXE_CNT_RCV_RNR] = "rcvd_rnr_err", > [RXE_CNT_SND_RNR] = "send_rnr_err", > [RXE_CNT_RCV_SEQ_ERR] = "rcvd_seq_err", > - [RXE_CNT_COMPLETER_SCHED] = "ack_deffered", > + [RXE_CNT_COMPLETER_SCHED] = "ack_deferred", > [RXE_CNT_RETRY_EXCEEDED] = "retry_exceeded_err", > [RXE_CNT_RNR_RETRY_EXCEEDED] = "retry_rnr_exceeded_err", > [RXE_CNT_COMP_RETRY] = "completer_retry_err", > [RXE_CNT_SEND_ERR] = "send_err", > + [RXE_CNT_LINK_DOWNED] = "link_downed", > + [RXE_CNT_RDMA_SEND] = "rdma_sends", > + [RXE_CNT_RDMA_RECV] = "rdma_recvs", > }; > > int rxe_ib_get_hw_stats(struct ib_device *ibdev, > diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.h b/drivers/infiniband/sw/rxe/rxe_hw_counters.h > index f44df1b76742..72c0d63c79e0 100644 > --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.h > +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.h > @@ -50,6 +50,9 @@ enum rxe_counters { > RXE_CNT_RNR_RETRY_EXCEEDED, > RXE_CNT_COMP_RETRY, > RXE_CNT_SEND_ERR, > + RXE_CNT_LINK_DOWNED, > + RXE_CNT_RDMA_SEND, > + RXE_CNT_RDMA_RECV, > RXE_NUM_OF_COUNTERS > }; > > diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c > index d292f590c72f..2e4f296bdab4 100644 > --- a/drivers/infiniband/sw/rxe/rxe_net.c > +++ b/drivers/infiniband/sw/rxe/rxe_net.c > @@ -657,6 +657,7 @@ void rxe_port_down(struct rxe_dev *rxe) > port->attr.state = IB_PORT_DOWN; > > rxe_port_event(rxe, IB_EVENT_PORT_ERR); > + rxe_counter_inc(rxe, RXE_CNT_LINK_DOWNED); > pr_info("set %s down\n", rxe->ib_dev.name); > } > > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c > index 871bd6d8a11c..8da6928efba3 100644 > --- a/drivers/infiniband/sw/rxe/rxe_resp.c > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c > @@ -836,6 +836,7 @@ static enum resp_states do_complete(struct rxe_qp *qp, > struct ib_wc *wc = &cqe.ibwc; > struct ib_uverbs_wc *uwc = &cqe.uibwc; > struct rxe_recv_wqe *wqe = qp->resp.wqe; > + struct rxe_dev *rxe = to_rdev(qp->ibqp.device); > > if (unlikely(!wqe)) > return RESPST_CLEANUP; > @@ -848,6 +849,7 @@ static enum resp_states do_complete(struct rxe_qp *qp, > > /* fields after status are not required for errors */ > if (wc->status == IB_WC_SUCCESS) { > + rxe_counter_inc(rxe, RXE_CNT_RDMA_RECV); > wc->opcode = (pkt->mask & RXE_IMMDT_MASK && > pkt->mask & RXE_WRITE_MASK) ? > IB_WC_RECV_RDMA_WITH_IMM : IB_WC_RECV; > @@ -891,7 +893,6 @@ static enum resp_states do_complete(struct rxe_qp *qp, > } > > if (pkt->mask & RXE_IETH_MASK) { > - struct rxe_dev *rxe = to_rdev(qp->ibqp.device); > struct rxe_mem *rmr; > > wc->wc_flags |= IB_WC_WITH_INVALIDATE; > -- > 2.16.2 >
On 10/30/2018 6:54 AM, Andrew Boyer wrote: > Signed-off-by: Andrew Boyer <andrew.boyer@dell.com> > --- > drivers/infiniband/sw/rxe/rxe_comp.c | 6 ++++++ > drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++++-- > drivers/infiniband/sw/rxe/rxe_hw_counters.h | 3 +++ > drivers/infiniband/sw/rxe/rxe_net.c | 1 + > drivers/infiniband/sw/rxe/rxe_resp.c | 3 ++- > 5 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c > index 6cdc40ed8a9f..6dc99ca31b5f 100644 > --- a/drivers/infiniband/sw/rxe/rxe_comp.c > +++ b/drivers/infiniband/sw/rxe/rxe_comp.c > @@ -428,6 +428,7 @@ static void make_send_cqe(struct rxe_qp *qp, struct rxe_send_wqe *wqe, > */ > static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe) > { > + struct rxe_dev *rxe = to_rdev(qp->ibqp.device); > struct rxe_cqe cqe; > > if ((qp->sq_sig_type == IB_SIGNAL_ALL_WR) || > @@ -440,6 +441,11 @@ static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe) > advance_consumer(qp->sq.queue); > } > > + if (wqe->wr.opcode == IB_WR_SEND || > + wqe->wr.opcode == IB_WR_SEND_WITH_IMM || > + wqe->wr.opcode == IB_WR_SEND_WITH_INV) > + rxe_counter_inc(rxe, RXE_CNT_RDMA_SEND); > + > /* > * we completed something so let req run again > * if it is trying to fence > diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c b/drivers/infiniband/sw/rxe/rxe_hw_counters.c > index 6aeb7a165e46..4a24895846d3 100644 > --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c > +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c > @@ -37,15 +37,18 @@ static const char * const rxe_counter_name[] = { > [RXE_CNT_SENT_PKTS] = "sent_pkts", > [RXE_CNT_RCVD_PKTS] = "rcvd_pkts", > [RXE_CNT_DUP_REQ] = "duplicate_request", > - [RXE_CNT_OUT_OF_SEQ_REQ] = "out_of_sequence", > + [RXE_CNT_OUT_OF_SEQ_REQ] = "out_of_seq_request", > [RXE_CNT_RCV_RNR] = "rcvd_rnr_err", > [RXE_CNT_SND_RNR] = "send_rnr_err", > [RXE_CNT_RCV_SEQ_ERR] = "rcvd_seq_err", > - [RXE_CNT_COMPLETER_SCHED] = "ack_deffered", > + [RXE_CNT_COMPLETER_SCHED] = "ack_deferred", We really can't change those two, you are changing a sysfs entry with this. > [RXE_CNT_RETRY_EXCEEDED] = "retry_exceeded_err", > [RXE_CNT_RNR_RETRY_EXCEEDED] = "retry_rnr_exceeded_err", > [RXE_CNT_COMP_RETRY] = "completer_retry_err", > [RXE_CNT_SEND_ERR] = "send_err", > + [RXE_CNT_LINK_DOWNED] = "link_downed", > + [RXE_CNT_RDMA_SEND] = "rdma_sends", > + [RXE_CNT_RDMA_RECV] = "rdma_recvs", > }; > > int rxe_ib_get_hw_stats(struct ib_device *ibdev, > diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.h b/drivers/infiniband/sw/rxe/rxe_hw_counters.h > index f44df1b76742..72c0d63c79e0 100644 > --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.h > +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.h > @@ -50,6 +50,9 @@ enum rxe_counters { > RXE_CNT_RNR_RETRY_EXCEEDED, > RXE_CNT_COMP_RETRY, > RXE_CNT_SEND_ERR, > + RXE_CNT_LINK_DOWNED, > + RXE_CNT_RDMA_SEND, > + RXE_CNT_RDMA_RECV, > RXE_NUM_OF_COUNTERS > }; > > diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c > index d292f590c72f..2e4f296bdab4 100644 > --- a/drivers/infiniband/sw/rxe/rxe_net.c > +++ b/drivers/infiniband/sw/rxe/rxe_net.c > @@ -657,6 +657,7 @@ void rxe_port_down(struct rxe_dev *rxe) > port->attr.state = IB_PORT_DOWN; > > rxe_port_event(rxe, IB_EVENT_PORT_ERR); > + rxe_counter_inc(rxe, RXE_CNT_LINK_DOWNED); > pr_info("set %s down\n", rxe->ib_dev.name); > } > > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c > index 871bd6d8a11c..8da6928efba3 100644 > --- a/drivers/infiniband/sw/rxe/rxe_resp.c > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c > @@ -836,6 +836,7 @@ static enum resp_states do_complete(struct rxe_qp *qp, > struct ib_wc *wc = &cqe.ibwc; > struct ib_uverbs_wc *uwc = &cqe.uibwc; > struct rxe_recv_wqe *wqe = qp->resp.wqe; > + struct rxe_dev *rxe = to_rdev(qp->ibqp.device); > > if (unlikely(!wqe)) > return RESPST_CLEANUP; > @@ -848,6 +849,7 @@ static enum resp_states do_complete(struct rxe_qp *qp, > > /* fields after status are not required for errors */ > if (wc->status == IB_WC_SUCCESS) { > + rxe_counter_inc(rxe, RXE_CNT_RDMA_RECV); > wc->opcode = (pkt->mask & RXE_IMMDT_MASK && > pkt->mask & RXE_WRITE_MASK) ? > IB_WC_RECV_RDMA_WITH_IMM : IB_WC_RECV; > @@ -891,7 +893,6 @@ static enum resp_states do_complete(struct rxe_qp *qp, > } > > if (pkt->mask & RXE_IETH_MASK) { > - struct rxe_dev *rxe = to_rdev(qp->ibqp.device); > struct rxe_mem *rmr; > > wc->wc_flags |= IB_WC_WITH_INVALIDATE; > Mark
On Tue, Oct 30, 2018 at 07:31:18PM +0000, Mark Bloch wrote: > > > On 10/30/2018 6:54 AM, Andrew Boyer wrote: > > Signed-off-by: Andrew Boyer <andrew.boyer@dell.com> > > --- > > drivers/infiniband/sw/rxe/rxe_comp.c | 6 ++++++ > > drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++++-- > > drivers/infiniband/sw/rxe/rxe_hw_counters.h | 3 +++ > > drivers/infiniband/sw/rxe/rxe_net.c | 1 + > > drivers/infiniband/sw/rxe/rxe_resp.c | 3 ++- > > 5 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c > > index 6cdc40ed8a9f..6dc99ca31b5f 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_comp.c > > +++ b/drivers/infiniband/sw/rxe/rxe_comp.c > > @@ -428,6 +428,7 @@ static void make_send_cqe(struct rxe_qp *qp, struct rxe_send_wqe *wqe, > > */ > > static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe) > > { > > + struct rxe_dev *rxe = to_rdev(qp->ibqp.device); > > struct rxe_cqe cqe; > > > > if ((qp->sq_sig_type == IB_SIGNAL_ALL_WR) || > > @@ -440,6 +441,11 @@ static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe) > > advance_consumer(qp->sq.queue); > > } > > > > + if (wqe->wr.opcode == IB_WR_SEND || > > + wqe->wr.opcode == IB_WR_SEND_WITH_IMM || > > + wqe->wr.opcode == IB_WR_SEND_WITH_INV) > > + rxe_counter_inc(rxe, RXE_CNT_RDMA_SEND); > > + > > /* > > * we completed something so let req run again > > * if it is trying to fence > > diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c b/drivers/infiniband/sw/rxe/rxe_hw_counters.c > > index 6aeb7a165e46..4a24895846d3 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c > > +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c > > @@ -37,15 +37,18 @@ static const char * const rxe_counter_name[] = { > > [RXE_CNT_SENT_PKTS] = "sent_pkts", > > [RXE_CNT_RCVD_PKTS] = "rcvd_pkts", > > [RXE_CNT_DUP_REQ] = "duplicate_request", > > - [RXE_CNT_OUT_OF_SEQ_REQ] = "out_of_sequence", > > + [RXE_CNT_OUT_OF_SEQ_REQ] = "out_of_seq_request", > > [RXE_CNT_RCV_RNR] = "rcvd_rnr_err", > > [RXE_CNT_SND_RNR] = "send_rnr_err", > > [RXE_CNT_RCV_SEQ_ERR] = "rcvd_seq_err", > > - [RXE_CNT_COMPLETER_SCHED] = "ack_deffered", > > + [RXE_CNT_COMPLETER_SCHED] = "ack_deferred", > > We really can't change those two, you are changing a sysfs entry with this. > Mark, In this specific case, I wouldn't be so strict, for one main reason, most probably we won't break any user space applications with such change. It is long-going debate what is actually included in famous "don't break user space" narrative, but in this case, all users of RXE are using upstream code and there are no any known to me tools which monitor those RXE counters. It means that in this case, it is safe to rename. In general, the rule is that kernel devs are allowed to make user visible changes as long as they don't break user space applications and every cycle you can find endless number of such examples, many of them in fs/ part of kernel. Thanks
diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c index 6cdc40ed8a9f..6dc99ca31b5f 100644 --- a/drivers/infiniband/sw/rxe/rxe_comp.c +++ b/drivers/infiniband/sw/rxe/rxe_comp.c @@ -428,6 +428,7 @@ static void make_send_cqe(struct rxe_qp *qp, struct rxe_send_wqe *wqe, */ static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe) { + struct rxe_dev *rxe = to_rdev(qp->ibqp.device); struct rxe_cqe cqe; if ((qp->sq_sig_type == IB_SIGNAL_ALL_WR) || @@ -440,6 +441,11 @@ static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe) advance_consumer(qp->sq.queue); } + if (wqe->wr.opcode == IB_WR_SEND || + wqe->wr.opcode == IB_WR_SEND_WITH_IMM || + wqe->wr.opcode == IB_WR_SEND_WITH_INV) + rxe_counter_inc(rxe, RXE_CNT_RDMA_SEND); + /* * we completed something so let req run again * if it is trying to fence diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c b/drivers/infiniband/sw/rxe/rxe_hw_counters.c index 6aeb7a165e46..4a24895846d3 100644 --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c @@ -37,15 +37,18 @@ static const char * const rxe_counter_name[] = { [RXE_CNT_SENT_PKTS] = "sent_pkts", [RXE_CNT_RCVD_PKTS] = "rcvd_pkts", [RXE_CNT_DUP_REQ] = "duplicate_request", - [RXE_CNT_OUT_OF_SEQ_REQ] = "out_of_sequence", + [RXE_CNT_OUT_OF_SEQ_REQ] = "out_of_seq_request", [RXE_CNT_RCV_RNR] = "rcvd_rnr_err", [RXE_CNT_SND_RNR] = "send_rnr_err", [RXE_CNT_RCV_SEQ_ERR] = "rcvd_seq_err", - [RXE_CNT_COMPLETER_SCHED] = "ack_deffered", + [RXE_CNT_COMPLETER_SCHED] = "ack_deferred", [RXE_CNT_RETRY_EXCEEDED] = "retry_exceeded_err", [RXE_CNT_RNR_RETRY_EXCEEDED] = "retry_rnr_exceeded_err", [RXE_CNT_COMP_RETRY] = "completer_retry_err", [RXE_CNT_SEND_ERR] = "send_err", + [RXE_CNT_LINK_DOWNED] = "link_downed", + [RXE_CNT_RDMA_SEND] = "rdma_sends", + [RXE_CNT_RDMA_RECV] = "rdma_recvs", }; int rxe_ib_get_hw_stats(struct ib_device *ibdev, diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.h b/drivers/infiniband/sw/rxe/rxe_hw_counters.h index f44df1b76742..72c0d63c79e0 100644 --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.h +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.h @@ -50,6 +50,9 @@ enum rxe_counters { RXE_CNT_RNR_RETRY_EXCEEDED, RXE_CNT_COMP_RETRY, RXE_CNT_SEND_ERR, + RXE_CNT_LINK_DOWNED, + RXE_CNT_RDMA_SEND, + RXE_CNT_RDMA_RECV, RXE_NUM_OF_COUNTERS }; diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c index d292f590c72f..2e4f296bdab4 100644 --- a/drivers/infiniband/sw/rxe/rxe_net.c +++ b/drivers/infiniband/sw/rxe/rxe_net.c @@ -657,6 +657,7 @@ void rxe_port_down(struct rxe_dev *rxe) port->attr.state = IB_PORT_DOWN; rxe_port_event(rxe, IB_EVENT_PORT_ERR); + rxe_counter_inc(rxe, RXE_CNT_LINK_DOWNED); pr_info("set %s down\n", rxe->ib_dev.name); } diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c index 871bd6d8a11c..8da6928efba3 100644 --- a/drivers/infiniband/sw/rxe/rxe_resp.c +++ b/drivers/infiniband/sw/rxe/rxe_resp.c @@ -836,6 +836,7 @@ static enum resp_states do_complete(struct rxe_qp *qp, struct ib_wc *wc = &cqe.ibwc; struct ib_uverbs_wc *uwc = &cqe.uibwc; struct rxe_recv_wqe *wqe = qp->resp.wqe; + struct rxe_dev *rxe = to_rdev(qp->ibqp.device); if (unlikely(!wqe)) return RESPST_CLEANUP; @@ -848,6 +849,7 @@ static enum resp_states do_complete(struct rxe_qp *qp, /* fields after status are not required for errors */ if (wc->status == IB_WC_SUCCESS) { + rxe_counter_inc(rxe, RXE_CNT_RDMA_RECV); wc->opcode = (pkt->mask & RXE_IMMDT_MASK && pkt->mask & RXE_WRITE_MASK) ? IB_WC_RECV_RDMA_WITH_IMM : IB_WC_RECV; @@ -891,7 +893,6 @@ static enum resp_states do_complete(struct rxe_qp *qp, } if (pkt->mask & RXE_IETH_MASK) { - struct rxe_dev *rxe = to_rdev(qp->ibqp.device); struct rxe_mem *rmr; wc->wc_flags |= IB_WC_WITH_INVALIDATE;
Signed-off-by: Andrew Boyer <andrew.boyer@dell.com> --- drivers/infiniband/sw/rxe/rxe_comp.c | 6 ++++++ drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++++-- drivers/infiniband/sw/rxe/rxe_hw_counters.h | 3 +++ drivers/infiniband/sw/rxe/rxe_net.c | 1 + drivers/infiniband/sw/rxe/rxe_resp.c | 3 ++- 5 files changed, 17 insertions(+), 3 deletions(-)