4
\$\begingroup\$

The below program is a write() based workload generator. I am creating a buffer that starts with a sequence number (starting at 0x11111111) before being filled to capacity with 0xDEADBEEF. This workload generator is being traced, and therefore I want to minimize the amount of work being done (as that creates extra, unnecessary trace data) while still achieving the above effect.

Edit: Having the buffer start from 0x11111111 was after noticing that the block layer had troubles extracting from its structures the buffers which started with 0x0-------. This may be a byproduct of the buffer handling issues that users have pointed out to me, which is something I will have to test on my own. Do not consider the buffer starting from 0x11111111 as a constraint.

Edit: After a bit of testing, it seems that using 0x00000000 as the starting sequence will cause the buffer to be: 0000FFFFFFEFFFFFFFBE... instead of what I expected, 00000000FFFFFFEFFFFFFFBE... Starting from 0x11111111 produces the output I expected, 11111111FFFFFFEFFFFFFFBE...

Constraints:

  • I must ensure that each write is written to disk before I proceed to a new write, hence fsync().
  • I must write the sequence number into the buffer before writing anything else.
  • The current set of arguments cannot be removed.

You may assume that limited error checking (likely not much more than what I already have) is needed.

#define _GNU_SOURCE is listed to avoid the 'implicit declaration of usleep' warning.

How can I optimize my code?

#define _GNU_SOURCE

#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <sys/wait.h>

#define sequence_size  (sizeof(int)*2)
#define DEBUG 0

int main(int argc, char** argv)
{
    if(argc != 5) {
        printf("Usage: <executable> <outfile> <num_bytes> <num_writes> <delay(us)>\n"
            "outfile: the path of the target output file\n"
            "num_bytes: the size of each write request\n"
            "num_writes: the number of write requests\n"
            "delay: delay between writes (in us)\n"
            "sequence_size=%lu\n", sequence_size
            );
        return 1;
    }

    char *fileName = argv[1];
    int bytes = atoi(argv[2]);
    int writes = atoi(argv[3]);
    int delay = atoi(argv[4]);

    if (bytes <= sequence_size || writes <= 0 || delay < 0) {
        printf("Bytes must not exceed %lu, writes must be 1 or greater,"
                        "and delay cannot be negative.\n", sequence_size);
        return 1;
    }

    // The sequence number, which should be embedded at the beginning of a buffer
    // that will be written to fileName.  This value should be unique for each new
    // write()
    unsigned int sequence_num = 0x11111111;

    while(writes--) {
        // Need O_DIRECT to minimize cache effects?
        // TODO:  Adding in O_DIRECT causes writes not to be seen at the block layer.  Why?
        // O_DIRECT seems to be interfering with the intended effect of using fsync()
        int fp = open(fileName, O_CREAT|O_WRONLY/*|O_DIRECT|O_SYNC*/, 0600);

        if(fp < 0){
            printf("Could not open %s\n", fileName);
            return 1;
        }

        char *buff = malloc(bytes);
        if (!buff) {
            printf("Our buffer's malloc() failed.\n");
            return 1;
        }
        memset(buff, 0, bytes);

        int deadbeef = 0xDEADBEEF;

        // Ensure our sequence number is the first thing we see in the buffer.
        *(unsigned int*)buff = sequence_num;

        // Writes DEADBEEF into the buffer over and over, if possible
        for(int i = 1; i < bytes - sizeof(int); i += sizeof(int)) {
            *(unsigned int*)(buff + i) = deadbeef;
        }

        if(DEBUG) {
            printf("Buffer: ");
            for(int i = 0; i < bytes - 1; ++i)
                printf("%X", buff[i]);
            printf("\n");
        }

        write(fp, buff, bytes);

        // Flush it all to disk.  Without this fsync(), we cannot ensure that
        // all writes will go to the block layer before tracing ends.
        fsync(fp);
        close(fp);

        free(buff);

        // If there should be a delay, it must be done before the next write()
        if (delay > 0)
            usleep(delay);

        // Incrememnt the sequence number in case there is another write()
        sequence_num++;
    }

    return 0;
}
\$\endgroup\$
5
\$\begingroup\$
  • Do not hardcode executable in the help message. Prefer "%s ...", argv[0]

  • Bytes must not exceed is misleading. According to the condition you've meant Bytes must exceed.

  • If bytes is not a multiple of sizeof(int), the loop

    for(int i = 1; i < bytes - sizeof(int); i += sizeof(int)) {
        *(unsigned int*)(buff + i) = deadbeef;
    }
    

    has a buffer overrun problem.

  • To minimize the amount of work, do not allocate/fill/free the buffer on each iteration. Do it it once, and only modify the sequence number.

\$\endgroup\$
  • \$\begingroup\$ My code now includes your suggestions which are related to minimizing the amount of work done. I'm more or less convinced that your proposed change to my memory management is about the only optimization available, so I'll accept it as the answer if there's no more activity for a day or so. I can't believe I've let this file exist for as long as it has while having <executable> hard coded. \$\endgroup\$ – buratino Oct 31 '17 at 0:43
  • 2
    \$\begingroup\$ @buratino I rolled your edits back. You should not edit your question after receiving answers, because it invalidates the review. You are welcome to post an updated code as a follow-up question. \$\endgroup\$ – vnp Oct 31 '17 at 1:04
  • \$\begingroup\$ I see. I mistakenly thought it would be fine to include the suggestions from your answer which weren't related to my question, but I can see what kinds of problems that would make for anybody who visited this question in the future. Thanks for the advice. \$\endgroup\$ – buratino Oct 31 '17 at 2:53
  • \$\begingroup\$ Please update your remark about argv[0], see ref \$\endgroup\$ – malat Oct 31 '17 at 9:06
6
\$\begingroup\$

Bug: sequence number is overwritten

In this code:

    // Ensure our sequence number is the first thing we see in the buffer.
    *(unsigned int*)buff = sequence_num;

    // Writes DEADBEEF into the buffer over and over, if possible
    for(int i = 1; i < bytes - sizeof(int); i += sizeof(int)) {
        *(unsigned int*)(buff + i) = deadbeef;
    }

Assuming a 4 byte int, the first four bytes of buff are set to sequence_num. But the loop overwrites part of the sequence number because it starts the loop at i = 1, meaning byte 1. Thus, three of the four bytes of the sequence number (buff[1], buff[2], and buff[3]) will be overwritten with part of deadbeef. To fix this bug, you should start with i = sizeof(int) instead of i = 1.

\$\endgroup\$
  • \$\begingroup\$ I was going to suggest adding unsigned int * const ibuff = (unsigned int*)buff; to avoid all those casts - that would also make this problem go away (because one would write ibuff[0] = sequence_num; and ibuff[i] = deadbeef; In fact, it might be better to store the result of malloc in ibuff, and then create buff if needed in the debug code. \$\endgroup\$ – Martin Bonner supports Monica Oct 31 '17 at 8:42
3
\$\begingroup\$

You should get quite a speed-up by moving the file open and close outside of the loop, unless that is specifically what you are trying to trace.

You don't need to memset the buffer to 0, since you're immediately overwriting it - although you will want to make sure the buffer is a multiple of sizeof(int) to make sure you don't have a few unset bytes at the end.

And another minor bug, in the DEBUG output

for(int i = 0; i < bytes - 1; ++i)
    printf("%X", buff[i]);

the last byte is not printed.

\$\endgroup\$
  • \$\begingroup\$ Oh! It only just occurred to me, you don't need to set the buffer to OxDEADBEEF inside the loop either. Do it once at the start, and only the first unsigned int changes each time through the loop. \$\endgroup\$ – Harun Nov 1 '17 at 10:49
3
\$\begingroup\$

Other people have commented on your bugs, I'm going to comment a bit about your style,

Don't use runtime evaluation when you are using defines any way, you have no way to control your debug variable from the command line.

//dont define DEBUG unless using command line ie -DDEBUG
...

#ifdef DEBUG
...
#else
...
#endif

#ifndef DEBUG
...
#endif

#ifdef DEBUG
    printf("Buffer: ");
    for(int i = 0; i < bytes - 1; ++i)
        printf("%X", buff[i]);
    printf("\n");
#endif

Be consistent with your use of braces. I've encountered my fair share of no curly brace block errors, so I would recommend just not using this "feature"

   // why don't you do this for the for loops, but do it here?
   if (delay > 0)
        usleep(delay);

   //do this instead
   if (delay > 0){
        usleep(delay);
   }

Also why don't you start unsigned int sequence_num = 0x11111111; at 0x00000000? I know it is part of your constraints, but the constraint itself is dubious...

you should probably also separate your program into more functions, you can at least put all of the sequence code somewhere else. Additionally I don't see a reason to re-allocate the deadbeef buffer, you don't need to change the size.

//put this in its own function
void writeSequences(char* fileName, int bytes, int writes, int delay){

    unsigned int sequence_num = 0x11111111;

    char *buff = malloc(bytes);
    if (!buff) {
        printf("Our buffer's malloc() failed.\n");
        return;
    }
    memset(buff, 0, bytes);

    int deadbeef = 0xDEADBEEF;
    while(writes--) {
        int fp = open(fileName, O_CREAT|O_WRONLY, 0600);

        if(fp < 0){
            printf("Could not open %s\n", fileName);
            return;
        }

        *(unsigned int*)buff = sequence_num;


        for(int i = 1; i < bytes - sizeof(int); i += sizeof(int)) {
            *(unsigned int*)(buff + i) = deadbeef;
        }

        #ifdef DEBUG
            printf("Buffer: ");
            for(int i = 0; i < bytes - 1; ++i)
                printf("%X", buff[i]);
            printf("\n");
        #endif

        write(fp, buff, bytes);


        fsync(fp);
        close(fp);

        if (delay > 0)
            usleep(delay);

        sequence_num++;
    }
    free(buff);
}

and you would end up with a main like this:

int main(int argc, char** argv)
{
    if(argc != 5) {
        printf("Usage: <executable> <outfile> <num_bytes> <num_writes> <delay(us)>\n"
            "outfile: the path of the target output file\n"
            "num_bytes: the size of each write request\n"
            "num_writes: the number of write requests\n"
            "delay: delay between writes (in us)\n"
            "sequence_size=%lu\n", sequence_size
            );
        return 1;
    }

    char *fileName = argv[1];
    int bytes = atoi(argv[2]);
    int writes = atoi(argv[3]);
    int delay = atoi(argv[4]);

    if (bytes <= sequence_size || writes <= 0 || delay < 0) {
        printf("Bytes must not exceed %lu, writes must be 1 or greater,"
                        "and delay cannot be negative.\n", sequence_size);
        return 1;
    }
    writeSequences(fileName, bytes, writes,  delay);
}
\$\endgroup\$
  • \$\begingroup\$ The constraint of starting from 0x11111111 was after noticing that the block layer had troubles extracting from its structures the buffers which started with 0x0-------. This constraint may very well be due to the issues I had with my buffer handling, and I will have to test to see if it is still a problem. I will edit this info in to my question. As far as the inconsistency in braces goes, this is a modified source file that was passed on to me previously. I made my own modifications, which were inconsistent with the previous writer's style. I'll be sure to incorporate your advice. \$\endgroup\$ – buratino Oct 31 '17 at 18:28
  • \$\begingroup\$ I have included an edit in my question which details what happens with a buffer starting with 0x00000000 vs 0x11111111 at the user-space level. \$\endgroup\$ – buratino Oct 31 '17 at 19:59
2
\$\begingroup\$

Error messages should go to stderr, not stdout. They can be improved by being more specific; perror() exists to simplify that:

    int fp = open(fileName, O_CREAT|O_WRONLY, 0600);
    if (fp < 0) {
        perror(fileName);
        return 1;
    }

Avoid working in units of char for writes of int size:

unsigned int *const buff = malloc(bytes);
for (int i = 1;  i < bytes / sizeof *buff;  ++i) {
    buff[i] = deadbeef;
}
/* Consider writing to the remaining `bytes % sizeof *buff` chars here. */
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.