Commit c9f2ea22 authored by Ankit Kumar's avatar Ankit Kumar Committed by Konrad Sztyber
Browse files

util: fix total fds to wait for 



Change num_fds type from signed to unsigned int.

The epoll file descriptor for a fd group waits for interrupt event on
the fds from its interrupt sources list as well as from all its children
fd group interrupt sources list. Update add, remove, nest and unnest APIs
to track the correct number of fds registered in a fd group.
Add a check to verify that interrupt sources list is empty before
destroying the fd group.

Change-Id: Ib06de08e1083579540f9d0d0ee5fb8db23caad11
Signed-off-by: default avatarAnkit Kumar <ankit.kumar@samsung.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/25077


Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Community-CI: Community CI Samsung <spdk.community.ci.samsung@gmail.com>
Reviewed-by: default avatarJim Harris <jim.harris@samsung.com>
parent 1239ea17

lib/util/fd_group.c

+53 −10
Original line number Diff line number Diff line
@@ -45,7 +45,12 @@ struct event_handler { 


struct spdk_fd_group {

	int epfd;

	int num_fds; /* Number of fds registered in this group. */



	/* Number of fds registered in this group. The epoll file descriptor of this fd group

	 * i.e. epfd waits for interrupt event on all the fds from its interrupt sources list, as

	 * well as from all its children fd group interrupt sources list.

	 */

	uint32_t num_fds;



	struct spdk_fd_group *parent;
@@ -96,9 +101,10 @@ _fd_group_del_all(int epfd, struct spdk_fd_group *grp) 
				    ehdlr->fd, strerror(errno));

			goto recover;

		}

		ret++;

	}



	return 0;

	return ret;



recover:

	/* We failed to remove everything. Let's try to put everything back into
@@ -147,9 +153,10 @@ _fd_group_add_all(int epfd, struct spdk_fd_group *grp) 
				    ehdlr->fd, strerror(errno));

			goto recover;

		}

		ret++;

	}



	return 0;

	return ret;



recover:

	/* We failed to add everything, so try to remove what we did add. */
@@ -188,11 +195,21 @@ spdk_fd_group_unnest(struct spdk_fd_group *parent, struct spdk_fd_group *child) 
	rc = _fd_group_del_all(parent->epfd, child);

	if (rc < 0) {

		return rc;

	} else {

		assert(parent->num_fds >= (uint32_t)rc);

		parent->num_fds -= rc;

	}



	child->parent = NULL;



	return _fd_group_add_all(child->epfd, child);

	rc = _fd_group_add_all(child->epfd, child);

	if (rc < 0) {

		return rc;

	} else {

		child->num_fds += rc;

	}



	return 0;

}



int
@@ -217,11 +234,21 @@ spdk_fd_group_nest(struct spdk_fd_group *parent, struct spdk_fd_group *child) 
	rc = _fd_group_del_all(child->epfd, child);

	if (rc < 0) {

		return rc;

	} else {

		assert(child->num_fds >= (uint32_t)rc);

		child->num_fds -= rc;

	}



	child->parent = parent;



	return _fd_group_add_all(parent->epfd, child);

	rc =  _fd_group_add_all(parent->epfd, child);

	if (rc < 0) {

		return rc;

	} else {

		parent->num_fds += rc;

	}



	return 0;

}



int
@@ -282,7 +309,11 @@ spdk_fd_group_add_for_events(struct spdk_fd_group *fgrp, int efd, uint32_t event 
	}



	TAILQ_INSERT_TAIL(&fgrp->event_handlers, ehdlr, next);

	if (fgrp->parent) {

		fgrp->parent->num_fds++;

	} else {

		fgrp->num_fds++;

	}



	return 0;

}
@@ -327,8 +358,13 @@ spdk_fd_group_remove(struct spdk_fd_group *fgrp, int efd) 
		return;

	}



	if (fgrp->parent) {

		assert(fgrp->parent->num_fds > 0);

		fgrp->parent->num_fds--;

	} else {

		assert(fgrp->num_fds > 0);

		fgrp->num_fds--;

	}

	TAILQ_REMOVE(&fgrp->event_handlers, ehdlr, next);



	/* Delay ehdlr's free in case it is waiting for execution in fgrp wait loop */
@@ -413,13 +449,20 @@ spdk_fd_group_destroy(struct spdk_fd_group *fgrp) 
		if (!fgrp) {

			SPDK_ERRLOG("fd_group doesn't exist.\n");

		} else {

			SPDK_ERRLOG("Cannot delete fd group(%p) as (%d) fds still registered to it.\n",

			SPDK_ERRLOG("Cannot delete fd group(%p) as (%u) fds are still registered to it.\n",

				    fgrp, fgrp->num_fds);

		}

		assert(0);

		return;

	}



	/* Check if someone tried to delete the fd group before unnesting it */

	if (!TAILQ_EMPTY(&fgrp->event_handlers)) {

		SPDK_ERRLOG("Interrupt sources list not empty.\n");

		assert(0);

		return;

	}



	close(fgrp->epfd);

	free(fgrp);
@@ -429,7 +472,7 @@ spdk_fd_group_destroy(struct spdk_fd_group *fgrp) 
int

spdk_fd_group_wait(struct spdk_fd_group *fgrp, int timeout)

{

	int totalfds = fgrp->num_fds;

	uint32_t totalfds = fgrp->num_fds;

	struct epoll_event events[totalfds];

	struct event_handler *ehdlr;

	int n;

test/unit/lib/util/fd_group.c/fd_group_ut.c

+22 −1
Original line number Diff line number Diff line
@@ -60,7 +60,7 @@ static void 
test_fd_group_nest_unnest(void)

{

	struct spdk_fd_group *parent, *child, *not_parent;

	int fd_parent, fd_child;

	int fd_parent, fd_child, fd_child_2;

	int rc;

	int cb_arg;
@@ -79,6 +79,9 @@ test_fd_group_nest_unnest(void) 
	fd_child = epoll_create1(0);

	SPDK_CU_ASSERT_FATAL(fd_child >= 0);



	fd_child_2 = epoll_create1(0);

	SPDK_CU_ASSERT_FATAL(fd_child_2 >= 0);



	rc = SPDK_FD_GROUP_ADD(parent, fd_parent, fd_group_cb_fn, &cb_arg);

	SPDK_CU_ASSERT_FATAL(rc == 0);

	SPDK_CU_ASSERT_FATAL(parent->num_fds == 1);
@@ -91,6 +94,16 @@ test_fd_group_nest_unnest(void) 
	rc = spdk_fd_group_nest(parent, child);

	SPDK_CU_ASSERT_FATAL(rc == 0);

	SPDK_CU_ASSERT_FATAL(child->parent == parent);

	SPDK_CU_ASSERT_FATAL(parent->num_fds == 2);

	SPDK_CU_ASSERT_FATAL(child->num_fds == 0);



	/* Register second child fd to the child fd group and verify that the parent fd group

	 * has the correct number of fds.

	 */

	rc = SPDK_FD_GROUP_ADD(child, fd_child_2, fd_group_cb_fn, &cb_arg);

	SPDK_CU_ASSERT_FATAL(rc == 0);

	SPDK_CU_ASSERT_FATAL(child->num_fds == 0);

	SPDK_CU_ASSERT_FATAL(parent->num_fds == 3);



	/* Unnest child fd group from wrong parent fd group and verify that it fails. */

	rc = spdk_fd_group_unnest(not_parent, child);
@@ -100,8 +113,13 @@ test_fd_group_nest_unnest(void) 
	rc = spdk_fd_group_unnest(parent, child);

	SPDK_CU_ASSERT_FATAL(rc == 0);

	SPDK_CU_ASSERT_FATAL(child->parent == NULL);

	SPDK_CU_ASSERT_FATAL(parent->num_fds == 1);

	SPDK_CU_ASSERT_FATAL(child->num_fds == 2);



	spdk_fd_group_remove(child, fd_child);

	SPDK_CU_ASSERT_FATAL(child->num_fds == 1);



	spdk_fd_group_remove(child, fd_child_2);

	SPDK_CU_ASSERT_FATAL(child->num_fds == 0);



	spdk_fd_group_remove(parent, fd_parent);
@@ -110,6 +128,9 @@ test_fd_group_nest_unnest(void) 
	rc = close(fd_child);

	CU_ASSERT(rc == 0);



	rc = close(fd_child_2);

	CU_ASSERT(rc == 0);



	rc = close(fd_parent);

	CU_ASSERT(rc == 0);