JeffersonLab / qphix

QCD for Intel Xeon Phi and Xeon processors
http://jeffersonlab.github.io/qphix/
Other
13 stars 11 forks source link

SpinorBlock memory management inconsistent #55

Closed kostrzewa closed 7 years ago

kostrzewa commented 7 years ago

@bjoo @martin-ueding

I remember that this was discussed some time ago, an additional spinor block is allocated for the spinor fields and the allocator returns a pointer to the second element. Consequently, the corresponding free is passed p-1 rather than p. Now this works fine, of course, but it breaks the convention that a nullptr is okay to be free'd. Do we want to remove the additional element or should I check for null in the free?

  FourSpinorBlock *allocCBFourSpinor()
  {

    FourSpinorBlock *ret_val = (FourSpinorBlock *)BUFFER_MALLOC(spinor_bytes, 128);                                                                         
    if (ret_val == (FourSpinorBlock *)0x0) {
      masterPrintf("Failed to allocate FourSpinorBlock\n");
      abort();
    }

    // Zero the field.
    // Cast the pointer.
    T *ret_val_ft = (T *)ret_val;

    // change from number of bytes to number of T type elements
    size_t num_ft = spinor_bytes / sizeof(T);

    // Zero it all (including) (especially) the pad regions.
    // FIXME: this is not NUMA friendly necessarily
    for (int i = 0; i < num_ft; i++) {
      ret_val_ft[i] = rep<T, double>(0.0);
    }

    return ret_val + 1;
  }

  void free(FourSpinorBlock *p)
  {
    FourSpinorBlock *freeme = p - 1;
    BUFFER_FREE(freeme, spinor_bytes);
  }
martin-ueding commented 7 years ago

The additional element is some fudging such that accesses to that area are not access violations. I think it would be safer to let that in there. Then a check in the free function would be a good idea to make it more safe.

In the longer run, these functions should be hidden from the user and only the RAII containers shall be used. Then the user does not have to call free and that problem is solved as well. Except when you want to have a couple of pointers and not allocate memory for all of them. But I would think that one could solve this neatly with RAII as well.

martin-ueding commented 7 years ago

I have added such a check.