Project 0 Mistakes

• Ignoring compiler warnings. You should not ignore compiler warnings. The compiler is your friend. Example:

  if (old_length = length + 12){
  

  uses an assignment (=) instead of (==), which is always true. The compiler warns. You ignore the warning, your code does not work. An avoidable mistake.

• Assuming that there is always at least one element in the free list. The free list can be empty if all memory is allocated.

• Returning a pointer to the used memory_block blk from mem_alloc rather than a pointer blk->data to the memory area the client can use. The client will overwrite your header immediately.

• Not initializing your mutex. The mutex must be initialized, either statically via

  static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
  

  or by calling pthread_mutex_init().

• Using sizeof(blk) instead of sizeof(*blk) or sizeof(struct used_block)/sizeof(struct free_block). The size used to store a pointer is 4, the size of a free block is 12.

• Coding

          pthread_mutex_lock(&lock);
          return list_size(&used_list);
          pthread_mutex_unlock(&lock);
  

  You don't unlock the mutex, ever.

• Coding

          pthread_mutex_lock(&lock);
  	if (*)
  		return NULL;
          pthread_mutex_unlock(&lock);
  

  You don't unlock if (*) is true. In general, you need to unlock the mutex on all paths leaving the function.

• Using any list function, including list_empty and list_size outside the protection of a mutex. Note that both calls have to be protected, so this attempt is just as wrong:

  size_t mem_sizeof_free_list(void)
  {
          pthread_mutex_lock(&my_mutex);  // leave this function alone dangerous threads
  
          if(!list_empty(&free_list))
          {
                  pthread_mutex_unlock(&my_mutex);        // release the function
                  return list_size(&free_list);           // take adantage of lists count
          }
          else
          {
                  pthread_mutex_unlock(&my_mutex);        // release the function
                  return 0;
          }
  }
  

  The correct way to write this code is

  size_t mem_sizeof_free_list(void)
  {
          pthread_mutex_lock(&my_mutex);
  	size_t sz = list_size(&free_list);
          pthread_mutex_unlock(&my_mutex);
  	return sz;
  }
  

  Note that 'sz' is declared as a local variable. Local variable are not shared among threads. Thus, we store the result of list_size() - which has to be inside lock/unlock - into a variable that is not shared and which can therefore be safely accessed after the unlock.

Project 0 Near Misses

• Requiring that a free block accommodate length bytes plus an header. You only need room for an additional header if you're splitting a block. Consequently, your allocator rejects allocation requests it could handle.

• Assigning anything to blk->data[0]. data[0] is the first byte in the area that the client will use. Writing anything there is pointless since the client will overwrite it. Major code quality issue indicative of confusion.

• Hardwiring sizes (4, 8, or 12) instead of using sizeof(). Breaks your program if size of int or layout of memory_block header changes. Major code quality issue.

Project 0 Minor Comments

• Coding past 80 columns. If you don't follow a self-imposed limit, you tend to nest code too deeply. Read what Linus Torvalds has to say about that (Chapter 1, 2). He prefers an 8-column indentation style, limited at 80. Pintos is written in GNU style, which your code should follow. The GNU style uses 4-column indentation (per nesting level), but also disallows coding past 80 columns.

  My advice is to heed the warning about nesting level (no more than 3), but because of today's larger screens, we will not grade you down if comments stretch past 80 columns. (It's still hard to print such code, though.)

• Using a global variable int headersize = sizeof(struct memory_block). You should declare it const to allow the compiler to resolve its value at compile-time, rather than reading the value from memory repeatedly at run time. Equally readable, but faster. Very minor code quality issue.

• Using

  list_insert(list_begin(&list), &blk->elem);
  

  Use list_push_front(&list, &blk->elem); instead. (Slightly slower, but a lot more readable.) Similarly, use list_push_back instead of list_insert(list_end(...), ...).

• Prefixing an array constant with a &. That is, coding return &blk->data; instead of return blk->data;. Both statements do the same thing, because the compiler ignores a & that precedes an array constant. However, your program will break if someone changes the array to a pointer - in addition, the ampersand shows you don't fully understand the relationship between arrays and pointers in C/C++.

• Accessing head, tail, next, and prev elements directly instead of using list_begin(), list_end(), list_next(), list_prev(). This breaks encapsulation: in C++, these would be private fields. C doesn't have "private", but that doesn't mean the rules change. Major code quality issue: if the implementation of the list changes, your code breaks.

• Checking that list is not empty before calling list_insert_ordered is not necessary. That's the beauty of using separate list heads/tail elements.