[ltt-dev] [LTTNG-TOOLS PATCH 3/3] Cleanup mmap consuming

[Julien Desfossez]

Only one mmap call per buffer, once the first mmap is done, we use the
offset to know where to start consuming the data.
When the buffer is closing, we munmap it.
Also removed the manual padding handling because the kernel already
exposes a padded subbuffer.

Signed-off-by: Julien Desfossez <julien.desfossez at polymtl.ca>
---
 liblttkconsumerd/liblttkconsumerd.c |   60 +++++++++++++++--------------------
 liblttkconsumerd/liblttkconsumerd.h |    2 +
 ltt-kconsumerd/ltt-kconsumerd.c     |    2 +-
 3 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/liblttkconsumerd/liblttkconsumerd.c b/liblttkconsumerd/liblttkconsumerd.c
index 9d8cb00..81ea529 100644
--- a/liblttkconsumerd/liblttkconsumerd.c
+++ b/liblttkconsumerd/liblttkconsumerd.c
@@ -141,6 +141,7 @@ static int kconsumerd_find_session_fd(int fd)
  */
 static void kconsumerd_del_fd(struct kconsumerd_fd *lcf)
 {
+	int ret;
 	pthread_mutex_lock(&kconsumerd_data.lock);
 	cds_list_del(&lcf->list);
 	if (kconsumerd_data.fds_count > 0) {
@@ -148,6 +149,12 @@ static void kconsumerd_del_fd(struct kconsumerd_fd *lcf)
 		if (lcf != NULL) {
 			close(lcf->out_fd);
 			close(lcf->consumerd_fd);
+			if (lcf->mmap_base != NULL) {
+				ret = munmap(lcf->mmap_base, lcf->mmap_len);
+				if (ret != 0) {
+					perror("munmap");
+				}
+			}
 			free(lcf);
 			lcf = NULL;
 		}
@@ -178,6 +185,8 @@ static int kconsumerd_add_fd(struct lttcomm_kconsumerd_msg *buf, int consumerd_f
 	tmp_fd->consumerd_fd = consumerd_fd;
 	tmp_fd->state = buf->state;
 	tmp_fd->max_sb_size = buf->max_sb_size;
+	tmp_fd->mmap_base = NULL;
+	tmp_fd->mmap_len = 0;
 	strncpy(tmp_fd->path_name, buf->path_name, PATH_MAX);
 
 	/* Opening the tracefile in write mode */
@@ -270,31 +279,29 @@ static int kconsumerd_update_poll_array(struct pollfd **pollfd,
 int kconsumerd_on_read_subbuffer_mmap(struct kconsumerd_fd *kconsumerd_fd,
 		unsigned long len)
 {
-	unsigned long mmap_len, mmap_offset, padded_len, padding_len;
-	char *mmap_base;
+	unsigned long mmap_offset;
 	char *padding = NULL;
 	long ret = 0;
 	off_t orig_offset = kconsumerd_fd->out_fd_offset;
 	int fd = kconsumerd_fd->consumerd_fd;
 	int outfd = kconsumerd_fd->out_fd;
 
-	/* get the padded subbuffer size to know the padding required */
-	ret = kernctl_get_padded_subbuf_size(fd, &padded_len);
-	if (ret != 0) {
-		ret = errno;
-		perror("kernctl_get_padded_subbuf_size");
-		goto end;
-	}
-	padding_len = padded_len - len;
-	padding = malloc(padding_len * sizeof(char));
-	memset(padding, '\0', padding_len);
+	if (kconsumerd_fd->mmap_base == NULL) {
+		/* get the len of the mmap region */
+		ret = kernctl_get_mmap_len(fd, &kconsumerd_fd->mmap_len);
+		if (ret != 0) {
+			ret = errno;
+			perror("kernctl_get_mmap_len");
+			goto end;
+		}
 
-	/* get the len of the mmap region */
-	ret = kernctl_get_mmap_len(fd, &mmap_len);
-	if (ret != 0) {
-		ret = errno;
-		perror("kernctl_get_mmap_len");
-		goto end;
+		kconsumerd_fd->mmap_base = mmap(NULL, kconsumerd_fd->mmap_len,
+				PROT_READ, MAP_PRIVATE, fd, 0);
+		if (kconsumerd_fd->mmap_base == MAP_FAILED) {
+			perror("Error mmaping");
+			ret = -1;
+			goto end;
+		}
 	}
 
 	/* get the offset inside the fd to mmap */
@@ -305,15 +312,8 @@ int kconsumerd_on_read_subbuffer_mmap(struct kconsumerd_fd *kconsumerd_fd,
 		goto end;
 	}
 
-	mmap_base = mmap(NULL, mmap_len, PROT_READ, MAP_PRIVATE, fd, mmap_offset);
-	if (mmap_base == MAP_FAILED) {
-		perror("Error mmaping");
-		ret = -1;
-		goto end;
-	}
-
 	while (len > 0) {
-		ret = write(outfd, mmap_base, len);
+		ret = write(outfd, kconsumerd_fd->mmap_base + mmap_offset, len);
 		if (ret >= len) {
 			len = 0;
 		} else if (ret < 0) {
@@ -327,14 +327,6 @@ int kconsumerd_on_read_subbuffer_mmap(struct kconsumerd_fd *kconsumerd_fd,
 		kconsumerd_fd->out_fd_offset += ret;
 	}
 
-	/* once all the data is written, write the padding to disk */
-	ret = write(outfd, padding, padding_len);
-	if (ret < 0) {
-		ret = errno;
-		perror("Error writing padding to file");
-		goto end;
-	}
-
 	/*
 	 * This does a blocking write-and-wait on any page that belongs to the
 	 * subbuffer prior to the one we just wrote.
diff --git a/liblttkconsumerd/liblttkconsumerd.h b/liblttkconsumerd/liblttkconsumerd.h
index f98621f..3de4c04 100644
--- a/liblttkconsumerd/liblttkconsumerd.h
+++ b/liblttkconsumerd/liblttkconsumerd.h
@@ -55,6 +55,8 @@ struct kconsumerd_fd {
 	char path_name[PATH_MAX]; /* tracefile name */
 	enum kconsumerd_fd_state state;
 	unsigned long max_sb_size; /* the subbuffer size for this channel */
+	void *mmap_base;
+	size_t mmap_len;
 };
 
 /*
diff --git a/ltt-kconsumerd/ltt-kconsumerd.c b/ltt-kconsumerd/ltt-kconsumerd.c
index 61ed005..32d439a 100644
--- a/ltt-kconsumerd/ltt-kconsumerd.c
+++ b/ltt-kconsumerd/ltt-kconsumerd.c
@@ -234,7 +234,7 @@ static int read_subbuffer(struct kconsumerd_fd *kconsumerd_fd)
 			break;
 		case LTTNG_EVENT_MMAP:
 			/* read the used subbuffer size */
-			err = kernctl_get_subbuf_size(infd, &len);
+			err = kernctl_get_padded_subbuf_size(infd, &len);
 			if (err != 0) {
 				ret = errno;
 				perror("Getting sub-buffer len failed.");
-- 
1.7.4.1