Merge pull request #15891 from markdroth/subchannel_alignment_fix

Fix subchannel call stack alignment

BUILD

@@ -540,6 +540,7 @@ grpc_cc_library(
         "src/core/lib/profiling/stap_timers.cc",
     ],
     hdrs = [
+        "src/core/lib/gpr/alloc.h",
         "src/core/lib/gpr/arena.h",
         "src/core/lib/gpr/env.h",
         "src/core/lib/gpr/host_port.h",

build.yaml

@@ -173,6 +173,7 @@ filegroups:
   - include/grpc/support/thd_id.h
   - include/grpc/support/time.h
   headers:
+  - src/core/lib/gpr/alloc.h
   - src/core/lib/gpr/arena.h
   - src/core/lib/gpr/env.h
   - src/core/lib/gpr/host_port.h

gRPC-C++.podspec

@@ -212,6 +212,7 @@ Pod::Spec.new do |s|
                       'src/cpp/util/string_ref.cc',
                       'src/cpp/util/time_cc.cc',
                       'src/cpp/codegen/codegen_init.cc',
+                      'src/core/lib/gpr/alloc.h',
                       'src/core/lib/gpr/arena.h',
                       'src/core/lib/gpr/env.h',
                       'src/core/lib/gpr/host_port.h',
@@ -503,6 +504,7 @@ Pod::Spec.new do |s|
                               'src/cpp/server/health/health.pb.h',
                               'src/cpp/server/thread_pool_interface.h',
                               'src/cpp/thread_manager/thread_manager.h',
+                              'src/core/lib/gpr/alloc.h',
                               'src/core/lib/gpr/arena.h',
                               'src/core/lib/gpr/env.h',
                               'src/core/lib/gpr/host_port.h',

gRPC-Core.podspec

@@ -183,7 +183,8 @@ Pod::Spec.new do |s|
     ss.dependency 'nanopb', '~> 0.3'
 
     # To save you from scrolling, this is the last part of the podspec.
-    ss.source_files = 'src/core/lib/gpr/arena.h',
+    ss.source_files = 'src/core/lib/gpr/alloc.h',
+                      'src/core/lib/gpr/arena.h',
                       'src/core/lib/gpr/env.h',
                       'src/core/lib/gpr/host_port.h',
                       'src/core/lib/gpr/mpscq.h',
@@ -803,7 +804,8 @@ Pod::Spec.new do |s|
                       'src/core/ext/filters/workarounds/workaround_utils.cc',
                       'src/core/plugin_registry/grpc_plugin_registry.cc'
 
-    ss.private_header_files = 'src/core/lib/gpr/arena.h',
+    ss.private_header_files = 'src/core/lib/gpr/alloc.h',
+                              'src/core/lib/gpr/arena.h',
                               'src/core/lib/gpr/env.h',
                               'src/core/lib/gpr/host_port.h',
                               'src/core/lib/gpr/mpscq.h',

grpc.gemspec

@@ -81,6 +81,7 @@ Gem::Specification.new do |s|
   s.files += %w( include/grpc/impl/codegen/sync_generic.h )
   s.files += %w( include/grpc/impl/codegen/sync_posix.h )
   s.files += %w( include/grpc/impl/codegen/sync_windows.h )
+  s.files += %w( src/core/lib/gpr/alloc.h )
   s.files += %w( src/core/lib/gpr/arena.h )
   s.files += %w( src/core/lib/gpr/env.h )
   s.files += %w( src/core/lib/gpr/host_port.h )

package.xml

@@ -86,6 +86,7 @@
     <file baseinstalldir="/" name="include/grpc/impl/codegen/sync_generic.h" role="src" />
     <file baseinstalldir="/" name="include/grpc/impl/codegen/sync_posix.h" role="src" />
     <file baseinstalldir="/" name="include/grpc/impl/codegen/sync_windows.h" role="src" />
+    <file baseinstalldir="/" name="src/core/lib/gpr/alloc.h" role="src" />
     <file baseinstalldir="/" name="src/core/lib/gpr/arena.h" role="src" />
     <file baseinstalldir="/" name="src/core/lib/gpr/env.h" role="src" />
     <file baseinstalldir="/" name="src/core/lib/gpr/host_port.h" role="src" />

src/core/ext/filters/client_channel/subchannel.cc

@@ -38,6 +38,7 @@
 #include "src/core/lib/channel/channel_args.h"
 #include "src/core/lib/channel/connected_channel.h"
 #include "src/core/lib/debug/stats.h"
+#include "src/core/lib/gpr/alloc.h"
 #include "src/core/lib/gprpp/debug_location.h"
 #include "src/core/lib/gprpp/manual_constructor.h"
 #include "src/core/lib/gprpp/ref_counted_ptr.h"
@@ -140,9 +141,13 @@ struct grpc_subchannel_call {
   grpc_closure* schedule_closure_after_destroy;
 };
 
-#define SUBCHANNEL_CALL_TO_CALL_STACK(call) ((grpc_call_stack*)((call) + 1))
-#define CALLSTACK_TO_SUBCHANNEL_CALL(callstack) \
-  (((grpc_subchannel_call*)(callstack)) - 1)
+#define SUBCHANNEL_CALL_TO_CALL_STACK(call)                          \
+  (grpc_call_stack*)((char*)(call) + GPR_ROUND_UP_TO_ALIGNMENT_SIZE( \
+                                         sizeof(grpc_subchannel_call)))
+#define CALLSTACK_TO_SUBCHANNEL_CALL(callstack)           \
+  (grpc_subchannel_call*)(((char*)(call_stack)) -         \
+                          GPR_ROUND_UP_TO_ALIGNMENT_SIZE( \
+                              sizeof(grpc_subchannel_call)))
 
 static void on_subchannel_connected(void* subchannel, grpc_error* error);
 
@@ -783,9 +788,17 @@ void ConnectedSubchannel::Ping(grpc_closure* on_initiate,
 
 grpc_error* ConnectedSubchannel::CreateCall(const CallArgs& args,
                                             grpc_subchannel_call** call) {
-  *call = static_cast<grpc_subchannel_call*>(gpr_arena_alloc(
-      args.arena, sizeof(grpc_subchannel_call) +
-                      channel_stack_->call_stack_size + args.parent_data_size));
+  size_t allocation_size =
+      GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_subchannel_call));
+  if (args.parent_data_size > 0) {
+    allocation_size +=
+        GPR_ROUND_UP_TO_ALIGNMENT_SIZE(channel_stack_->call_stack_size) +
+        args.parent_data_size;
+  } else {
+    allocation_size += channel_stack_->call_stack_size;
+  }
+  *call = static_cast<grpc_subchannel_call*>(
+      gpr_arena_alloc(args.arena, allocation_size));
   grpc_call_stack* callstk = SUBCHANNEL_CALL_TO_CALL_STACK(*call);
   RefCountedPtr<ConnectedSubchannel> connection =
       Ref(DEBUG_LOCATION, "subchannel_call");

src/core/lib/channel/channel_stack.cc

@@ -21,6 +21,7 @@
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
 #include "src/core/lib/channel/channel_stack.h"
+#include "src/core/lib/gpr/alloc.h"
 
 #include <stdlib.h>
 #include <string.h>
@@ -43,16 +44,12 @@ grpc_core::TraceFlag grpc_trace_channel(false, "channel");
      per-filter memory, aligned to GPR_MAX_ALIGNMENT
    } */
 
-/* Given a size, round up to the next multiple of sizeof(void*) */
-#define ROUND_UP_TO_ALIGNMENT_SIZE(x) \
-  (((x) + GPR_MAX_ALIGNMENT - 1u) & ~(GPR_MAX_ALIGNMENT - 1u))
-
 size_t grpc_channel_stack_size(const grpc_channel_filter** filters,
                                size_t filter_count) {
   /* always need the header, and size for the channel elements */
-  size_t size =
-      ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_channel_stack)) +
-      ROUND_UP_TO_ALIGNMENT_SIZE(filter_count * sizeof(grpc_channel_element));
+  size_t size = GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_channel_stack)) +
+                GPR_ROUND_UP_TO_ALIGNMENT_SIZE(filter_count *
+                                               sizeof(grpc_channel_element));
   size_t i;
 
   GPR_ASSERT((GPR_MAX_ALIGNMENT & (GPR_MAX_ALIGNMENT - 1)) == 0 &&
@@ -60,19 +57,19 @@ size_t grpc_channel_stack_size(const grpc_channel_filter** filters,
 
   /* add the size for each filter */
   for (i = 0; i < filter_count; i++) {
-    size += ROUND_UP_TO_ALIGNMENT_SIZE(filters[i]->sizeof_channel_data);
+    size += GPR_ROUND_UP_TO_ALIGNMENT_SIZE(filters[i]->sizeof_channel_data);
   }
 
   return size;
 }
 
-#define CHANNEL_ELEMS_FROM_STACK(stk)                                 \
-  ((grpc_channel_element*)((char*)(stk) + ROUND_UP_TO_ALIGNMENT_SIZE( \
+#define CHANNEL_ELEMS_FROM_STACK(stk)                                     \
+  ((grpc_channel_element*)((char*)(stk) + GPR_ROUND_UP_TO_ALIGNMENT_SIZE( \
                                               sizeof(grpc_channel_stack))))
 
-#define CALL_ELEMS_FROM_STACK(stk)     \
-  ((grpc_call_element*)((char*)(stk) + \
-                        ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call_stack))))
+#define CALL_ELEMS_FROM_STACK(stk)                                     \
+  ((grpc_call_element*)((char*)(stk) + GPR_ROUND_UP_TO_ALIGNMENT_SIZE( \
+                                           sizeof(grpc_call_stack))))
 
 grpc_channel_element* grpc_channel_stack_element(
     grpc_channel_stack* channel_stack, size_t index) {
@@ -95,8 +92,8 @@ grpc_error* grpc_channel_stack_init(
     const grpc_channel_args* channel_args, grpc_transport* optional_transport,
     const char* name, grpc_channel_stack* stack) {
   size_t call_size =
-      ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call_stack)) +
-      ROUND_UP_TO_ALIGNMENT_SIZE(filter_count * sizeof(grpc_call_element));
+      GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call_stack)) +
+      GPR_ROUND_UP_TO_ALIGNMENT_SIZE(filter_count * sizeof(grpc_call_element));
   grpc_channel_element* elems;
   grpc_channel_element_args args;
   char* user_data;
@@ -106,9 +103,9 @@ grpc_error* grpc_channel_stack_init(
   GRPC_STREAM_REF_INIT(&stack->refcount, initial_refs, destroy, destroy_arg,
                        name);
   elems = CHANNEL_ELEMS_FROM_STACK(stack);
-  user_data =
-      (reinterpret_cast<char*>(elems)) +
-      ROUND_UP_TO_ALIGNMENT_SIZE(filter_count * sizeof(grpc_channel_element));
+  user_data = (reinterpret_cast<char*>(elems)) +
+              GPR_ROUND_UP_TO_ALIGNMENT_SIZE(filter_count *
+                                             sizeof(grpc_channel_element));
 
   /* init per-filter data */
   grpc_error* first_error = GRPC_ERROR_NONE;
@@ -128,8 +125,9 @@ grpc_error* grpc_channel_stack_init(
         GRPC_ERROR_UNREF(error);
       }
     }
-    user_data += ROUND_UP_TO_ALIGNMENT_SIZE(filters[i]->sizeof_channel_data);
-    call_size += ROUND_UP_TO_ALIGNMENT_SIZE(filters[i]->sizeof_call_data);
+    user_data +=
+        GPR_ROUND_UP_TO_ALIGNMENT_SIZE(filters[i]->sizeof_channel_data);
+    call_size += GPR_ROUND_UP_TO_ALIGNMENT_SIZE(filters[i]->sizeof_call_data);
   }
 
   GPR_ASSERT(user_data > (char*)stack);
@@ -166,7 +164,7 @@ grpc_error* grpc_call_stack_init(grpc_channel_stack* channel_stack,
                        destroy_arg, "CALL_STACK");
   call_elems = CALL_ELEMS_FROM_STACK(elem_args->call_stack);
   user_data = (reinterpret_cast<char*>(call_elems)) +
-              ROUND_UP_TO_ALIGNMENT_SIZE(count * sizeof(grpc_call_element));
+              GPR_ROUND_UP_TO_ALIGNMENT_SIZE(count * sizeof(grpc_call_element));
 
   /* init per-filter data */
   grpc_error* first_error = GRPC_ERROR_NONE;
@@ -184,7 +182,7 @@ grpc_error* grpc_call_stack_init(grpc_channel_stack* channel_stack,
       }
     }
     user_data +=
-        ROUND_UP_TO_ALIGNMENT_SIZE(call_elems[i].filter->sizeof_call_data);
+        GPR_ROUND_UP_TO_ALIGNMENT_SIZE(call_elems[i].filter->sizeof_call_data);
   }
   return first_error;
 }
@@ -243,11 +241,11 @@ grpc_channel_stack* grpc_channel_stack_from_top_element(
     grpc_channel_element* elem) {
   return reinterpret_cast<grpc_channel_stack*>(
       reinterpret_cast<char*>(elem) -
-      ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_channel_stack)));
+      GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_channel_stack)));
 }
 
 grpc_call_stack* grpc_call_stack_from_top_element(grpc_call_element* elem) {
   return reinterpret_cast<grpc_call_stack*>(
       reinterpret_cast<char*>(elem) -
-      ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call_stack)));
+      GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call_stack)));
 }

src/core/lib/gpr/alloc.h

@@ -0,0 +1,28 @@
+/*
+ *
+ * Copyright 2018 gRPC authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+#ifndef GRPC_CORE_LIB_GPR_ALLOC_H
+#define GRPC_CORE_LIB_GPR_ALLOC_H
+
+#include <grpc/support/port_platform.h>
+
+/// Given a size, round up to the next multiple of sizeof(void*).
+#define GPR_ROUND_UP_TO_ALIGNMENT_SIZE(x) \
+  (((x) + GPR_MAX_ALIGNMENT - 1u) & ~(GPR_MAX_ALIGNMENT - 1u))
+
+#endif /* GRPC_CORE_LIB_GPR_ALLOC_H */

src/core/lib/gpr/arena.cc

@@ -26,6 +26,8 @@
 #include <grpc/support/atm.h>
 #include <grpc/support/log.h>
 
+#include "src/core/lib/gpr/alloc.h"
+
 // Uncomment this to use a simple arena that simply allocates the
 // requested amount of memory for each call to gpr_arena_alloc().  This
 // effectively eliminates the efficiency gain of using an arena, but it
@@ -74,8 +76,6 @@ void* gpr_arena_alloc(gpr_arena* arena, size_t size) {
 // arena API to C++, we should consider replacing gpr_arena_alloc() with a
 // template that takes the type of the value being allocated, which
 // would allow us to use the alignment actually needed by the caller.
-#define ROUND_UP_TO_ALIGNMENT_SIZE(x) \
-  (((x) + GPR_MAX_ALIGNMENT - 1u) & ~(GPR_MAX_ALIGNMENT - 1u))
 
 typedef struct zone {
   size_t size_begin;
@@ -95,9 +95,9 @@ static void* zalloc_aligned(size_t size) {
 }
 
 gpr_arena* gpr_arena_create(size_t initial_size) {
-  initial_size = ROUND_UP_TO_ALIGNMENT_SIZE(initial_size);
+  initial_size = GPR_ROUND_UP_TO_ALIGNMENT_SIZE(initial_size);
   gpr_arena* a = static_cast<gpr_arena*>(zalloc_aligned(
-      ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(gpr_arena)) + initial_size));
+      GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(gpr_arena)) + initial_size));
   a->initial_zone.size_end = initial_size;
   return a;
 }
@@ -115,7 +115,7 @@ size_t gpr_arena_destroy(gpr_arena* arena) {
 }
 
 void* gpr_arena_alloc(gpr_arena* arena, size_t size) {
-  size = ROUND_UP_TO_ALIGNMENT_SIZE(size);
+  size = GPR_ROUND_UP_TO_ALIGNMENT_SIZE(size);
   size_t start = static_cast<size_t>(
       gpr_atm_no_barrier_fetch_add(&arena->size_so_far, size));
   zone* z = &arena->initial_zone;
@@ -125,7 +125,7 @@ void* gpr_arena_alloc(gpr_arena* arena, size_t size) {
       size_t next_z_size =
           static_cast<size_t>(gpr_atm_no_barrier_load(&arena->size_so_far));
       next_z = static_cast<zone*>(zalloc_aligned(
-          ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(zone)) + next_z_size));
+          GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(zone)) + next_z_size));
       next_z->size_begin = z->size_end;
       next_z->size_end = z->size_end + next_z_size;
       if (!gpr_atm_rel_cas(&z->next_atm, static_cast<gpr_atm>(NULL),
@@ -143,9 +143,9 @@ void* gpr_arena_alloc(gpr_arena* arena, size_t size) {
   GPR_ASSERT(start + size <= z->size_end);
   char* ptr = (z == &arena->initial_zone)
                   ? reinterpret_cast<char*>(arena) +
-                        ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(gpr_arena))
+                        GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(gpr_arena))
                   : reinterpret_cast<char*>(z) +
-                        ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(zone));
+                        GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(zone));
   return ptr + start - z->size_begin;
 }
 

src/core/lib/surface/call.cc

@@ -34,6 +34,7 @@
 #include "src/core/lib/channel/channel_stack.h"
 #include "src/core/lib/compression/algorithm_metadata.h"
 #include "src/core/lib/debug/stats.h"
+#include "src/core/lib/gpr/alloc.h"
 #include "src/core/lib/gpr/arena.h"
 #include "src/core/lib/gpr/string.h"
 #include "src/core/lib/gpr/useful.h"
@@ -271,16 +272,12 @@ struct grpc_call {
 grpc_core::TraceFlag grpc_call_error_trace(false, "call_error");
 grpc_core::TraceFlag grpc_compression_trace(false, "compression");
 
-/* Given a size, round up to the next multiple of sizeof(void*) */
-#define ROUND_UP_TO_ALIGNMENT_SIZE(x) \
-  (((x) + GPR_MAX_ALIGNMENT - 1u) & ~(GPR_MAX_ALIGNMENT - 1u))
-
 #define CALL_STACK_FROM_CALL(call)   \
   (grpc_call_stack*)((char*)(call) + \
-                     ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call)))
+                     GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call)))
 #define CALL_FROM_CALL_STACK(call_stack) \
   (grpc_call*)(((char*)(call_stack)) -   \
-               ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call)))
+               GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call)))
 
 #define CALL_ELEM_FROM_CALL(call, idx) \
   grpc_call_stack_element(CALL_STACK_FROM_CALL(call), idx)
@@ -353,7 +350,7 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args,
   GRPC_STATS_INC_CALL_INITIAL_SIZE(initial_size);
   gpr_arena* arena = gpr_arena_create(initial_size);
   call = static_cast<grpc_call*>(
-      gpr_arena_alloc(arena, ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call)) +
+      gpr_arena_alloc(arena, GPR_ROUND_UP_TO_ALIGNMENT_SIZE(sizeof(grpc_call)) +
                                  channel_stack->call_stack_size));
   gpr_ref_init(&call->ext_ref, 1);
   call->arena = arena;

tools/doxygen/Doxyfile.c++.internal

@@ -1029,6 +1029,7 @@ src/core/lib/compression/stream_compression_identity.h \
 src/core/lib/debug/stats.h \
 src/core/lib/debug/stats_data.h \
 src/core/lib/debug/trace.h \
+src/core/lib/gpr/alloc.h \
 src/core/lib/gpr/arena.h \
 src/core/lib/gpr/env.h \
 src/core/lib/gpr/host_port.h \

tools/doxygen/Doxyfile.core.internal

@@ -1073,6 +1073,7 @@ src/core/lib/debug/trace.cc \
 src/core/lib/debug/trace.h \
 src/core/lib/gpr/README.md \
 src/core/lib/gpr/alloc.cc \
+src/core/lib/gpr/alloc.h \
 src/core/lib/gpr/arena.cc \
 src/core/lib/gpr/arena.h \
 src/core/lib/gpr/atm.cc \

tools/run_tests/generated/sources_and_headers.json

@@ -9166,6 +9166,7 @@
       "include/grpc/support/sync_windows.h", 
       "include/grpc/support/thd_id.h", 
       "include/grpc/support/time.h", 
+      "src/core/lib/gpr/alloc.h", 
       "src/core/lib/gpr/arena.h", 
       "src/core/lib/gpr/env.h", 
       "src/core/lib/gpr/host_port.h", 
@@ -9212,6 +9213,7 @@
       "include/grpc/support/sync_windows.h", 
       "include/grpc/support/thd_id.h", 
       "include/grpc/support/time.h", 
+      "src/core/lib/gpr/alloc.h", 
       "src/core/lib/gpr/arena.h", 
       "src/core/lib/gpr/env.h", 
       "src/core/lib/gpr/host_port.h",