Test the return value of omMMapBinaryFile function and terminate the main program elegantly #3002

tungld · 2024-11-08T12:55:29Z

Currently the main program still runs even if omMMapBinaryFile fails to mmap the constant file, so it's difficult for debugging.

This patch changes omMMapBinaryFile to return a boolean value indicating success and failure, so that the main program can exit elegantly.

This patch also includes a fix for z/OS we just found.

Signed-off-by: Tung D. Le <[email protected]>

tungld · 2024-11-08T12:56:49Z

src/Conversion/KrnlToLLVM/KrnlEntryPoint.cpp

-          create.llvm._return(create.llvm.null(getI8PointerType(context)));
-        });
-  }
-


Move this code into KrnlToLLVMHelper.{hpp/cpp} so that it can be reused.

Signed-off-by: Tung D. Le <[email protected]>

tungld · 2024-11-08T14:22:25Z

@christopherlmunoz FYI. This is not to fix issues about mmapping a constant file on z/OS, but to make debugging easier.

Signed-off-by: Tung D. Le <[email protected]>

tungld · 2024-11-11T01:43:51Z

src/Runtime/OMExternalConstant.inc

    }

    /* Prepare to compare-and-swap to setup the shared constAddr.
     * If we fail, another thread beat us so free our mmap.
     */
 #ifdef __MVS__
    void *expected = NULL;
-    if (cds((cds_t *)&expected, (cds_t *)&constAddr[0], *(cds_t *)tempAddr))
+    if (cds((cds_t *)&expected, (cds_t *)&constAddr[0], *(cds_t *)&tempAddr))


Include a fix for z/OS.

gongsu832 · 2024-11-11T16:05:07Z

src/Runtime/OMExternalConstant.inc

@@ -67,15 +68,18 @@ void omMMapBinaryFile(
  char *tPath = strdup(fname);
  if (!tPath) {
    fprintf(stderr, "Error while strdup");
-    return;
+    return false;
  }
  __a2e_s(tPath);
  fname = tPath;
 #endif

  if (constAddr == NULL) {
    perror("Error: null pointer");


Instead of mixing fprintf and perror, either use fprintf everywhere but add strerror(errno) for more informative printout, or use perror everywhere.

Sure, now using fprintf everywhere.

gongsu832 · 2024-11-11T16:07:12Z

src/Runtime/OMExternalConstant.inc

@@ -89,7 +93,10 @@ void omMMapBinaryFile(
      filePath = (char *)malloc(filePathLen);


You probably want to +1 to filePathLen to account for the \0 you add to the end later.

Good catch! I updated this.

gongsu832 · 2024-11-11T18:04:55Z

src/Runtime/OMExternalConstant.inc

+#ifdef __MVS__
+        free(tPath);
+#endif
+        return false;
      }
      memcpy(filePath, basePath, baseLen);


The memcpy and adding \0 can be replaced by a single snprintf:

sprintf(filePath, filePathLen, "%s%s%s", basePath, DIR_SEPARATOR, fname);

(assuming filePathLen was incremented by 1 above).

Thanks! I updated by using snprintf

Signed-off-by: Tung D. Le <[email protected]>

gongsu832 · 2024-11-12T03:56:13Z

src/Runtime/OMExternalConstant.inc

@@ -43,7 +43,7 @@ const int i = 1;
 void checkEndianness(const char constPackIsLE) {
  if (XOR(IS_SYSTEM_LE(), constPackIsLE)) {
    fprintf(stderr, "Constant pack is stored in a byte order that is not "
-                    "native to this current system.");
+                    "native to this current system: %s", strerror(errno));


Sorry I probably didn't make it clear. errno is only meaningful after you make a library call such as strdup, open, malloc, etc. Here strerror(errno) is meaningless.

Same thought. Will remove them.

gongsu832 · 2024-11-12T03:56:58Z

src/Runtime/OMExternalConstant.inc

    return false;
  }
  __a2e_s(tPath);
  fname = tPath;
 #endif

  if (constAddr == NULL) {
-    perror("Error: null pointer");
+    fprintf(stderr, "Error: null pointer: %s", strerror(errno));


This strerror(errno) is also meaningless.

gongsu832 · 2024-11-12T04:07:04Z

src/Runtime/OMExternalConstant.inc

@@ -158,7 +155,9 @@ bool omMMapBinaryFile(
    /* Make sure constAddr is the same as the mmap address.
    */
    if (constAddr[0] != tempAddr) {


This check is wrong. Only the first thread that successfully performs the compare-and-swap will have constAddr[0] == tempAddr. All other threads will have constAddr[0] != tempAddr. But it doesn't matter since by design they will throw away their tempAddr and use the constAddr[0] set by the first thread.

Does tempAddr become NULL after munmap? If so, we do the test only when tempAddr != NULL.

I explicitly set tempAddr = NULL after munmap, and check constAddr[0] != tempAddr only when tempAddr != NULL (meaning for the first successful thread).

Does tempAddr become NULL after munmap? If so, we do the test only when tempAddr != NULL.

Not sure what you mean. munmap cannot change tempAddr since C is passing by value (i.e., inside munmap it only has access to a copy of tempAddr). But even if tempAddr somehow becomes NULL after munmap, the check constAddr[0] != tempAddr is still wrong.

I explicitly set tempAddr = NULL after munmap, and check constAddr[0] != tempAddr only when tempAddr != NULL (meaning for the first successful thread).

Not sure why you want to do that. A successful compare-and-swap guarantees that constAddr[0] == tempAddr so the check would be redundant.

Oh, you are right. In the latest code, I set tempAddr = NULL for failed threads, and check if (tempAddr && (constAddr[0] != tempAddr)). What do you think?

I want to avoid the situation we encountered where cds was written wrongly. Perhaps, assert is better.

OK. I removed the check. I was overthinking about having a check to debug, but since we found the issue, and cds would work correctly.

Yes you can use assert but it wouldn't help in this case. Because even if you write your cds wrong like the bug we just had, in the first thread after the cds you will have constAddr[0] == tempAddr. Because cds doesn't know/care if the tempAddr you supplied in the call is garbage. All it does is setting constAddr[0] to tempAddr atomically, regardless of what's in tempAddr.

gongsu832 · 2024-11-12T04:07:55Z

src/Runtime/OMExternalConstant.inc

@@ -181,11 +180,11 @@ bool omMMapBinaryFile(
 void omGetExternalConstantAddr(
    void **outputAddr, void **baseAddr, int64_t offset) {
  if (outputAddr == NULL) {
-    perror("Error: null pointer");
+    fprintf(stderr, "Error: null pointer: %s", strerror(errno));


strerror(errno) is meaningless here.

gongsu832 · 2024-11-12T04:08:17Z

src/Runtime/OMExternalConstant.inc

    return;
  }
  if (baseAddr == NULL) {
-    perror("Error: null pointer");
+    fprintf(stderr, "Error: null pointer: %s", strerror(errno));


strerror(errno) is meaningless here.

Signed-off-by: Tung D. Le <[email protected]>

tungld · 2024-11-13T00:14:20Z

@gongsu832 @AlexandreEichenberger could you take another look at this PR? Apart from handling return value of omMMapBinaryFile, it contains a bug fix for the issue #2993. Thanks!

AlexandreEichenberger

LGTM, please have @gongsu832 provide the feedback specific to the mmap part.

AlexandreEichenberger · 2024-11-13T00:33:06Z

src/Conversion/KrnlToLLVM/KrnlToLLVMHelper.cpp

+        MultiDialectBuilder<LLVMBuilder, KrnlBuilder> create(createLLVM);
+        // Print an error message.
+        if (!errorMsg.empty())
+          create.krnl.printf(StringRef(errorMsg + "\n"));


If we start to use the krnl.printf for more than debugging, I have been often experiencing issues with that call (in debugging situations) where it print too long a string. We have some issues with that call.

I had similar issue in the krnl.PrintTensor and I have alleviated this by having a special %e sequence at the end of the string. For Instrumentation, I think I encoded the string length in an integer flag. All tricks to get around this issue. If someone could try to understand what is happening, it would be great!

AlexandreEichenberger · 2024-11-13T00:34:52Z

src/Runtime/OMExternalConstant.inc

    return;
  }
  if (baseAddr == NULL) {
-    perror("Error: null pointer");
+    fprintf(stderr, "Error: null pointer.");


No need to track these error by returning false or true?

I can be here, but I am a bit hesitated since this is called per each constant so there would have a lot of if then else generated in the main inference for checking the return value of this function.

AlexandreEichenberger · 2024-11-13T00:39:17Z

src/Conversion/KrnlToLLVM/KrnlToLLVMHelper.cpp

@@ -342,5 +343,47 @@ bool isZOS(ModuleOp module) {
  return zOS;
 }

+void equalOrFailed(ModuleOp &module, OpBuilder &rewriter, Location loc,


Would these calls also be potentially useful if we wanted to fail the inference, say after a NNPA severe failure call?
If so, we may want to see if we would benefit from introducing something at a higher level dialects (say Krnl or SCF) that would then feed into this one.

Totally yes. It will be used anywhere in the main inference when we want to fail the inference. We can introduce a krnl op for this.

gongsu832 · 2024-11-13T16:19:12Z

src/Runtime/OMExternalConstant.inc

+    fprintf(stderr, "Error: null pointer.");
+#ifdef __MVS__
+    free(tPath);
+#endif


It's not a big deal but a bit of an eye sore to see the #ifdef __MVS__ everywhere. Maybe it's simpler to just define two macros:

#ifdef __MVS__ #define CONV_PATH(p) \ char *p = strdup(fname); \ if (!p) { \ fprintf(stderr, "Error while strdup: %s", strerror(errno)); \ return false; \ } \ __a2e_s(p); \ fname = p #define FREE_PATH(p) free(p) #else #define CONV_PATH(p) #define FREE_PATH(p) #endif

and then just use CONV_PATH(tPath) and FREE_PATH(tPath) everywhere.

@gongsu832 all of these issues come from the fact that fname is not in EBCDIC. So I modify the code generation to convert fname to EBCDIC during compilation so that fname here would be alread in EBCDIC, then there is no need to handle it here anymore.

OK that makes things simpler.

Signed-off-by: Tung D. Le <[email protected]>

…return_value_mmap

gongsu832

LGTM

jenkins-droid · 2024-11-15T02:25:28Z

Jenkins Linux amd64 Build #15991 [push] Add the return value to ... started at 20:25

jenkins-droid · 2024-11-15T02:25:29Z

Jenkins Linux s390x Build #15994 [push] Add the return value to ... started at 21:25

jenkins-droid · 2024-11-15T02:25:30Z

Jenkins Linux ppc64le Build #15021 [push] Add the return value to ... started at 21:40

jenkins-droid · 2024-11-15T03:47:30Z

Jenkins Linux amd64 Build #15991 [push] Add the return value to ... passed after 1 hr 22 min

jenkins-droid · 2024-11-15T03:54:31Z

Jenkins Linux s390x Build #15994 [push] Add the return value to ... passed after 1 hr 29 min

jenkins-droid · 2024-11-15T04:46:03Z

Jenkins Linux ppc64le Build #15021 [push] Add the return value to ... passed after 2 hr 20 min

tungld added 2 commits November 8, 2024 03:10

Check the return value when mmapping a file and exit gently if failed

afa4cff

Signed-off-by: Tung D. Le <[email protected]>

Edit lit tests

03b00ca

Signed-off-by: Tung D. Le <[email protected]>

tungld commented Nov 8, 2024

View reviewed changes

Fix isLE value in lit tests for little endian machines

79b9b95

Signed-off-by: Tung D. Le <[email protected]>

tungld mentioned this pull request Nov 8, 2024

ZDNN_MISALIGNED_TENSOR when invoking NNPA on zOS when using a constants binary file. #2993

Open

Fix offset value in lit tests for little endian machines

7f8d1e0

Signed-off-by: Tung D. Le <[email protected]>

tungld requested a review from AlexandreEichenberger November 8, 2024 14:19

tungld requested a review from gongsu832 November 8, 2024 22:13

Fix a bug in z/OS code

c0b0a4d

Signed-off-by: Tung D. Le <[email protected]>

tungld commented Nov 11, 2024

View reviewed changes

gongsu832 reviewed Nov 11, 2024

View reviewed changes

tungld added 2 commits November 11, 2024 20:59

Requested changes

8156452

Signed-off-by: Tung D. Le <[email protected]>

Merge branch 'main' into return_value_mmap

9171ead

gongsu832 reviewed Nov 12, 2024

View reviewed changes

tungld added 3 commits November 12, 2024 00:01

Requested changes

d0f8bcb

Signed-off-by: Tung D. Le <[email protected]>

Remove the check

1d93a7d

Signed-off-by: Tung D. Le <[email protected]>

Merge branch 'main' into return_value_mmap

0ae13e7

AlexandreEichenberger approved these changes Nov 13, 2024

View reviewed changes

AlexandreEichenberger reviewed Nov 13, 2024

View reviewed changes

gongsu832 reviewed Nov 13, 2024

View reviewed changes

tungld added 3 commits November 13, 2024 20:33

Convert filename to EBCDIC when generating code for zOS

1042130

Signed-off-by: Tung D. Le <[email protected]>

Merge branch 'return_value_mmap' of github.com:tungld/onnx-mlir into …

4f88c79

…return_value_mmap

Merge branch 'main' into return_value_mmap

e9f794b

gongsu832 approved these changes Nov 14, 2024

View reviewed changes

tungld merged commit 868432d into onnx:main Nov 15, 2024
7 checks passed

		@@ -89,7 +93,10 @@ void omMMapBinaryFile(
		filePath = (char *)malloc(filePathLen);

Test the return value of omMMapBinaryFile function and terminate the main program elegantly #3002

Test the return value of omMMapBinaryFile function and terminate the main program elegantly #3002

Conversation

tungld commented Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

tungld commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongsu832 Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongsu832 Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

gongsu832 Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tungld commented Nov 13, 2024

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongsu832 left a comment

Choose a reason for hiding this comment

jenkins-droid commented Nov 15, 2024

jenkins-droid commented Nov 15, 2024

jenkins-droid commented Nov 15, 2024

jenkins-droid commented Nov 15, 2024

jenkins-droid commented Nov 15, 2024

jenkins-droid commented Nov 15, 2024

tungld commented Nov 8, 2024 •

edited

Loading

gongsu832 Nov 11, 2024 •

edited

Loading

gongsu832 Nov 12, 2024 •

edited

Loading

gongsu832 Nov 12, 2024 •

edited

Loading