[PATCH rdma-core 0/2] pyverbs: Support CQ events properly

[PATCH rdma-core 0/2] pyverbs: Support CQ events properly

[Noa Osherovich]

CQ events have to be acked prior to CQ destruction. Otherwise, CQ
destruction will wait indefinitely for those events to be acked.
In Python it's possible for a simple syntax error to cause a runtime
error which leads to teardown of all objects. For CQ, this can mean
a destruction without acking CQ events.

To avoid that, keep track of the number of events and during teardown,
if there are events that weren't acked, do so implicitly.

The first patch adds this support. The second one adds a simple
traffic test that uses CQ events mechanism rather than poll.

Noa Osherovich (2):
  pyverbs: Handle CQ events properly
  tests: Add a test for completion events

 pyverbs/cq.pxd          |  2 ++
 pyverbs/cq.pyx          | 14 ++++++++++++-
 tests/CMakeLists.txt    |  1 +
 tests/test_cq_events.py | 45 +++++++++++++++++++++++++++++++++++++++++
 tests/utils.py          | 11 +++++++---
 5 files changed, 69 insertions(+), 4 deletions(-)
 create mode 100644 tests/test_cq_events.py

-- 
2.21.0

[PATCH rdma-core 1/2] pyverbs: Handle CQ events properly

[Noa Osherovich]

When using events, the CQ has to ack all the retrieved events prior to
destruction. Since this call uses a mutex, the events are usually acked
together at the end of the datapath to avoid performance degradation.

In Python, each exception triggers a teardown of all pyverbs' resources,
including CQs (which may haven't acked their events yet). Exception can
be caused by something as trivial as a syntax error.

To avoid that, let CQ object keep track of the number of events waiting
to be acked. This number will be incremented by the completion channel
during get_cq_event() and decremented by the CQ during ack_events().
During close(), if there are still events waiting to be acked, the CQ
will ack them prior to its destruction.

Also add a reference from a CQ object to its completion channel object
to allow users to know if it is connected to one or not during poll.

Signed-off-by: Noa Osherovich <noaos@mellanox.com>
Reviewed-by: Edward Srouji <edwards@mellanox.com>
---
 pyverbs/cq.pxd |  2 ++
 pyverbs/cq.pyx | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/pyverbs/cq.pxd b/pyverbs/cq.pxd
index 8eeb2e1fd0c2..9704b96f3ff7 100644
--- a/pyverbs/cq.pxd
+++ b/pyverbs/cq.pxd
@@ -20,6 +20,8 @@ cdef class CQ(PyverbsCM):
     cdef add_ref(self, obj)
     cdef object qps
     cdef object srqs
+    cdef object channel
+    cdef object num_events
 
 cdef class CqInitAttrEx(PyverbsObject):
     cdef v.ibv_cq_init_attr_ex attr
diff --git a/pyverbs/cq.pyx b/pyverbs/cq.pyx
index dda47207507f..defb37646034 100755
--- a/pyverbs/cq.pyx
+++ b/pyverbs/cq.pyx
@@ -47,7 +47,7 @@ cdef class CompChannel(PyverbsCM):
     def get_cq_event(self, CQ expected_cq):
         """
         Waits for the next completion event in the completion event channel
-        :param expected_cq: The CQ that got the event
+        :param expected_cq: The CQ that is expected to get the event
         :return: None
         """
         cdef v.ibv_cq *cq
@@ -58,6 +58,7 @@ cdef class CompChannel(PyverbsCM):
             raise PyverbsRDMAErrno('Failed to get CQ event')
         if cq != expected_cq.cq:
             raise PyverbsRDMAErrno('Received event on an unexpected CQ')
+        expected_cq.num_events += 1
 
     cdef add_ref(self, obj):
         if isinstance(obj, CQ) or isinstance(obj, CQEX):
@@ -87,15 +88,18 @@ cdef class CQ(PyverbsCM):
             self.cq = v.ibv_create_cq(context.context, cqe, <void*>cq_context,
                                       channel.cc, comp_vector)
             channel.add_ref(self)
+            self.channel = channel
         else:
             self.cq = v.ibv_create_cq(context.context, cqe, <void*>cq_context,
                                       NULL, comp_vector)
+            self.channel = None
         if self.cq == NULL:
             raise PyverbsRDMAErrno('Failed to create a CQ')
         self.context = context
         context.add_ref(self)
         self.qps = weakref.WeakSet()
         self.srqs = weakref.WeakSet()
+        self.num_events = 0
         self.logger.debug('Created a CQ')
 
     cdef add_ref(self, obj):
@@ -112,12 +116,15 @@ cdef class CQ(PyverbsCM):
     cpdef close(self):
         self.logger.debug('Closing CQ')
         close_weakrefs([self.qps, self.srqs])
+        if self.num_events:
+            self.ack_events(self.num_events)
         if self.cq != NULL:
             rc = v.ibv_destroy_cq(self.cq)
             if rc != 0:
                 raise PyverbsRDMAErrno('Failed to close CQ')
             self.cq = NULL
             self.context = None
+            self.channel = None
 
     def poll(self, num_entries=1):
         """
@@ -166,6 +173,7 @@ cdef class CQ(PyverbsCM):
         :return: None
         """
         v.ibv_ack_cq_events(self.cq, num_events)
+        self.num_events -= num_events
 
     def __str__(self):
         print_format = '{:22}: {:<20}\n'
@@ -173,6 +181,10 @@ cdef class CQ(PyverbsCM):
                print_format.format('Handle', self.cq.handle) +\
                print_format.format('CQEs', self.cq.cqe)
 
+    @property
+    def comp_channel(self):
+        return self.channel
+
 
 cdef class CqInitAttrEx(PyverbsObject):
     def __init__(self, cqe = 100, CompChannel channel = None, comp_vector = 0,
-- 
2.21.0

[PATCH rdma-core 2/2] tests: Add a test for completion events

[Noa Osherovich]

Add a test which runs RC/UD traffic and uses a completion channel.
Add support for utils method poll_cq to use the completion channel
when a CQ has one.
This commit also fixes a bug in poll_cq - only the last polled CQEs
were returned to the user.

Signed-off-by: Noa Osherovich <noaos@mellanox.com>
Reviewed-by: Edward Srouji <edwards@mellanox.com>
---
 tests/CMakeLists.txt    |  1 +
 tests/test_cq_events.py | 45 +++++++++++++++++++++++++++++++++++++++++
 tests/utils.py          | 11 +++++++---
 3 files changed, 54 insertions(+), 3 deletions(-)
 create mode 100644 tests/test_cq_events.py

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 6d702425886c..8d946349bd67 100755
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -7,6 +7,7 @@ rdma_python_test(tests
   test_addr.py
   base.py
   test_cq.py
+  test_cq_events.py
   test_cqex.py
   test_device.py
   test_mr.py
diff --git a/tests/test_cq_events.py b/tests/test_cq_events.py
new file mode 100644
index 000000000000..bcb3f7d158ee
--- /dev/null
+++ b/tests/test_cq_events.py
@@ -0,0 +1,45 @@
+from tests.base import RCResources, UDResources
+from tests.base import RDMATestCase
+from tests.utils import traffic
+
+from pyverbs.cq import CQ, CompChannel
+
+
+def create_cq_with_comp_channel(agr_obj):
+    agr_obj.comp_channel = CompChannel(agr_obj.ctx)
+    agr_obj.cq = CQ(agr_obj.ctx, agr_obj.num_msgs, None, agr_obj.comp_channel)
+    agr_obj.cq.req_notify()
+
+
+class CqEventsUD(UDResources):
+    def create_cq(self):
+        create_cq_with_comp_channel(self)
+
+
+class CqEventsRC(RCResources):
+    def create_cq(self):
+        create_cq_with_comp_channel(self)
+
+
+class CqEventsTestCase(RDMATestCase):
+    def setUp(self):
+        super().setUp()
+        self.iters = 100
+        self.qp_dict = {'ud': CqEventsUD, 'rc': CqEventsRC}
+
+    def create_players(self, qp_type):
+        client = self.qp_dict[qp_type](self.dev_name, self.ib_port,
+                                       self.gid_index)
+        server = self.qp_dict[qp_type](self.dev_name, self.ib_port,
+                                       self.gid_index)
+        client.pre_run(server.psn, server.qpn)
+        server.pre_run(client.psn, client.qpn)
+        return client, server
+
+    def test_cq_events_ud(self):
+        client, server = self.create_players('ud')
+        traffic(client, server, self.iters, self.gid_index, self.ib_port)
+
+    def test_cq_events_rc(self):
+        client, server = self.create_players('rc')
+        traffic(client, server, self.iters, self.gid_index, self.ib_port)
diff --git a/tests/utils.py b/tests/utils.py
index c45170dbd329..d59ab54eec19 100755
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -332,14 +332,19 @@ def poll_cq(cq, count=1):
     :return: An array of work completions of length <count>, None
              when events are used
     """
-    wcs = None
+    wcs = []
+    channel = cq.comp_channel
     while count > 0:
-        nc, wcs = cq.poll(count)
-        for wc in wcs:
+        if channel:
+            channel.get_cq_event(cq)
+            cq.req_notify()
+        nc, tmp_wcs = cq.poll(count)
+        for wc in tmp_wcs:
             if wc.status != e.IBV_WC_SUCCESS:
                 raise PyverbsRDMAError('Completion status is {s}'.
                                        format(s=wc_status_to_str(wc.status)))
         count -= nc
+        wcs.extend(tmp_wcs)
     return wcs
 
 
-- 
2.21.0