Instead of always falling back to memcpy_fromio() for any size, prefer using read{b,w,l}(). When reading struct members it's common to read individual integer variables individually. Going through memcpy_fromio() for each of them poses a high penalty.
Employ a similar trick as __seqprop() by using _Generic() to generate only the specific call based on a type-compatible variable.
For a pariticular i915 workload producing GPU context switches, __get_engine_usage_record() is particularly hot since the engine usage is read from device local memory with dgfx, possibly multiple times since it's racy. Test execution time for this test shows a ~12.5% improvement with DG2:
Before: nrepeats = 1000; min = 7.63243e+06; max = 1.01817e+07; median = 9.52548e+06; var = 526149; After: nrepeats = 1000; min = 7.03402e+06; max = 8.8832e+06; median = 8.33955e+06; var = 333113;
Other things attempted that didn't prove very useful: 1) Change the _Generic() on x86 to just dereference the memory address 2) Change __get_engine_usage_record() to do just 1 read per loop, comparing with the previous value read 3) Change __get_engine_usage_record() to access the fields directly as it was before the conversion to iosys-map
(3) did gave a small improvement (~3%), but doesn't seem to scale well to other similar cases in the driver.
Additional test by Chris Wilson using gem_create from igt with some changes to track object creation time. This happens to accidentaly stress this code path:
Pre iosys_map conversion of engine busyness: lmem0: Creating 262144 4KiB objects took 59274.2ms
Unpatched: lmem0: Creating 262144 4KiB objects took 108830.2ms
With readl (this patch): lmem0: Creating 262144 4KiB objects took 61348.6ms
s/readl/READ_ONCE/ lmem0: Creating 262144 4KiB objects took 61333.2ms
So we do take a little bit more time than before the conversion, but that is due to other factors: bringing the READ_ONCE back would be as good as just doing this conversion.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com ---
If this is acceptable we should probably add the write counterpart, too. Sending here only the read for now since this fixes the issue we are seeing and to gather feedback.
include/linux/iosys-map.h | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index e69a002d5aa4..4ae3e459419e 100644 --- a/include/linux/iosys-map.h +++ b/include/linux/iosys-map.h @@ -333,6 +333,20 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset, memset(dst->vaddr + offset, value, len); }
+#ifdef CONFIG_64BIT +#define __iosys_map_rd_io_u64_case(val_, vaddr_iomem_) \ + u64: val_ = readq(vaddr_iomem_), +#else +#define __iosys_map_u64_case(val_, vaddr_iomem_) +#endif + +#define __iosys_map_rd_io(val__, vaddr_iomem__, type__) _Generic(val__, \ + u8: val__ = readb(vaddr_iomem__), \ + u16: val__ = readw(vaddr_iomem__), \ + u32: val__ = readl(vaddr_iomem__), \ + __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ + default: memcpy_fromio(&(val__), vaddr_iomem__, sizeof(val__))) + /** * iosys_map_rd - Read a C-type value from the iosys_map * @@ -346,10 +360,14 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset, * Returns: * The value read from the mapping. */ -#define iosys_map_rd(map__, offset__, type__) ({ \ - type__ val; \ - iosys_map_memcpy_from(&val, map__, offset__, sizeof(val)); \ - val; \ +#define iosys_map_rd(map__, offset__, type__) ({ \ + type__ val; \ + if ((map__)->is_iomem) { \ + __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ + } else { \ + memcpy(&val, (map__)->vaddr + offset__, sizeof(val)); \ + } \ + val; \ })
/**
Hi Lucas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-tip/drm-tip] [also build test ERROR on linus/master v5.19-rc1 next-20220609] [cannot apply to tegra-drm/drm/tegra/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Lucas-De-Marchi/iosys-map-Add... base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220610/202206101010.IAvFUbqo-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 70d35fe1257e429266b83025997b400e9f79110e) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/7b9b2d6b8d738fe2857fa1a96f7f3c... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Lucas-De-Marchi/iosys-map-Add-word-sized-reads/20220610-072113 git checkout 7b9b2d6b8d738fe2857fa1a96f7f3c9d8c11e9cd # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1154:14: error: unknown type name '__iosys_map_rd_io_u64_case'
*last_in = record_read(&rec_map, last_switch_in_stamp); ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:2: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1154:14: error: type-id cannot have a name
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:21: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1154:14: error: expected ')'
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:34: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1154:14: note: to match this '(' drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:28: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1155:9: error: unknown type name '__iosys_map_rd_io_u64_case' *id = record_read(&rec_map, current_context_index); ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:2: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1155:9: error: type-id cannot have a name drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:21: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1155:9: error: expected ')' drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:34: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1155:9: note: to match this '(' drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:28: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1156:12: error: unknown type name '__iosys_map_rd_io_u64_case' *total = record_read(&rec_map, total_runtime); ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:2: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1156:12: error: type-id cannot have a name drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:1134:2: note: expanded from macro 'record_read' iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:21: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ --
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:165:6: error: unknown type name '__iosys_map_rd_io_u64_case'
ads_blob_read(guc, policies.dpc_promote_time)); ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:2: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:165:6: error: type-id cannot have a name
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:21: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:165:6: error: expected ')'
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:34: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:165:6: note: to match this '(' drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:28: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:167:6: error: unknown type name '__iosys_map_rd_io_u64_case' ads_blob_read(guc, policies.max_num_work_items)); ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:2: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:167:6: error: type-id cannot have a name drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:21: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:167:6: error: expected ')' drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:34: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:167:6: note: to match this '(' drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:28: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:169:6: error: unknown type name '__iosys_map_rd_io_u64_case' ads_blob_read(guc, policies.global_flags)); ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:3: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ ^ include/linux/iosys-map.h:347:2: note: expanded from macro '__iosys_map_rd_io' __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:169:6: error: type-id cannot have a name drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:2: note: expanded from macro 'ads_blob_read' iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) ^ include/linux/iosys-map.h:452:2: note: expanded from macro 'iosys_map_rd_field' iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ ^ include/linux/iosys-map.h:366:21: note: expanded from macro 'iosys_map_rd' __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\
vim +/__iosys_map_rd_io_u64_case +1154 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
77cdd054dd2ced Umesh Nerlige Ramappa 2021-10-26 1132 4801b99588a2e0 Lucas De Marchi 2022-02-16 1133 #define record_read(map_, field_) \ 4801b99588a2e0 Lucas De Marchi 2022-02-16 1134 iosys_map_rd_field(map_, 0, struct guc_engine_usage_record, field_) 4801b99588a2e0 Lucas De Marchi 2022-02-16 1135 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1136 /* 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1137 * GuC updates shared memory and KMD reads it. Since this is not synchronized, 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1138 * we run into a race where the value read is inconsistent. Sometimes the 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1139 * inconsistency is in reading the upper MSB bytes of the last_in value when 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1140 * this race occurs. 2 types of cases are seen - upper 8 bits are zero and upper 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1141 * 24 bits are zero. Since these are non-zero values, it is non-trivial to 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1142 * determine validity of these values. Instead we read the values multiple times 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1143 * until they are consistent. In test runs, 3 attempts results in consistent 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1144 * values. The upper bound is set to 6 attempts and may need to be tuned as per 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1145 * any new occurences. 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1146 */ 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1147 static void __get_engine_usage_record(struct intel_engine_cs *engine, 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1148 u32 *last_in, u32 *id, u32 *total) 77cdd054dd2ced Umesh Nerlige Ramappa 2021-10-26 1149 { 4801b99588a2e0 Lucas De Marchi 2022-02-16 1150 struct iosys_map rec_map = intel_guc_engine_usage_record_map(engine); 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1151 int i = 0; 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1152 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1153 do { 4801b99588a2e0 Lucas De Marchi 2022-02-16 @1154 *last_in = record_read(&rec_map, last_switch_in_stamp); 4801b99588a2e0 Lucas De Marchi 2022-02-16 1155 *id = record_read(&rec_map, current_context_index); 4801b99588a2e0 Lucas De Marchi 2022-02-16 1156 *total = record_read(&rec_map, total_runtime); 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1157 4801b99588a2e0 Lucas De Marchi 2022-02-16 1158 if (record_read(&rec_map, last_switch_in_stamp) == *last_in && 4801b99588a2e0 Lucas De Marchi 2022-02-16 1159 record_read(&rec_map, current_context_index) == *id && 4801b99588a2e0 Lucas De Marchi 2022-02-16 1160 record_read(&rec_map, total_runtime) == *total) 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1161 break; 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1162 } while (++i < 6); 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1163 } 512712a824de9b Umesh Nerlige Ramappa 2022-01-24 1164
Am 10.06.22 um 01:20 schrieb Lucas De Marchi:
Instead of always falling back to memcpy_fromio() for any size, prefer using read{b,w,l}(). When reading struct members it's common to read individual integer variables individually. Going through memcpy_fromio() for each of them poses a high penalty.
Employ a similar trick as __seqprop() by using _Generic() to generate only the specific call based on a type-compatible variable.
For a pariticular i915 workload producing GPU context switches, __get_engine_usage_record() is particularly hot since the engine usage is read from device local memory with dgfx, possibly multiple times since it's racy. Test execution time for this test shows a ~12.5% improvement with DG2:
Before: nrepeats = 1000; min = 7.63243e+06; max = 1.01817e+07; median = 9.52548e+06; var = 526149; After: nrepeats = 1000; min = 7.03402e+06; max = 8.8832e+06; median = 8.33955e+06; var = 333113;
Other things attempted that didn't prove very useful:
- Change the _Generic() on x86 to just dereference the memory address
- Change __get_engine_usage_record() to do just 1 read per loop, comparing with the previous value read
- Change __get_engine_usage_record() to access the fields directly as it was before the conversion to iosys-map
(3) did gave a small improvement (~3%), but doesn't seem to scale well to other similar cases in the driver.
Additional test by Chris Wilson using gem_create from igt with some changes to track object creation time. This happens to accidentaly stress this code path:
Pre iosys_map conversion of engine busyness: lmem0: Creating 262144 4KiB objects took 59274.2ms
Unpatched: lmem0: Creating 262144 4KiB objects took 108830.2ms
With readl (this patch): lmem0: Creating 262144 4KiB objects took 61348.6ms
s/readl/READ_ONCE/ lmem0: Creating 262144 4KiB objects took 61333.2ms
So we do take a little bit more time than before the conversion, but that is due to other factors: bringing the READ_ONCE back would be as good as just doing this conversion.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
If this is acceptable we should probably add the write counterpart, too. Sending here only the read for now since this fixes the issue we are seeing and to gather feedback.
As far as I can see looks sane to me, but the kernel test robot tears the patch apart.
Probably just a typo somewhere in the 32bit handling.
Apart from that looks good to me.
Regards, Christian.
include/linux/iosys-map.h | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index e69a002d5aa4..4ae3e459419e 100644 --- a/include/linux/iosys-map.h +++ b/include/linux/iosys-map.h @@ -333,6 +333,20 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset, memset(dst->vaddr + offset, value, len); }
+#ifdef CONFIG_64BIT +#define __iosys_map_rd_io_u64_case(val_, vaddr_iomem_) \
- u64: val_ = readq(vaddr_iomem_),
+#else +#define __iosys_map_u64_case(val_, vaddr_iomem_) +#endif
+#define __iosys_map_rd_io(val__, vaddr_iomem__, type__) _Generic(val__, \
- u8: val__ = readb(vaddr_iomem__), \
- u16: val__ = readw(vaddr_iomem__), \
- u32: val__ = readl(vaddr_iomem__), \
- __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \
- default: memcpy_fromio(&(val__), vaddr_iomem__, sizeof(val__)))
- /**
- iosys_map_rd - Read a C-type value from the iosys_map
@@ -346,10 +360,14 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
- Returns:
- The value read from the mapping.
*/ -#define iosys_map_rd(map__, offset__, type__) ({ \
- type__ val; \
- iosys_map_memcpy_from(&val, map__, offset__, sizeof(val)); \
- val; \
+#define iosys_map_rd(map__, offset__, type__) ({ \
type__ val; \
if ((map__)->is_iomem) { \
__iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\} else { \
memcpy(&val, (map__)->vaddr + offset__, sizeof(val)); \} \
val; \ })
/**
On Fri, Jun 10, 2022 at 09:20:18AM +0200, Christian König wrote:
Am 10.06.22 um 01:20 schrieb Lucas De Marchi:
Instead of always falling back to memcpy_fromio() for any size, prefer using read{b,w,l}(). When reading struct members it's common to read individual integer variables individually. Going through memcpy_fromio() for each of them poses a high penalty.
Employ a similar trick as __seqprop() by using _Generic() to generate only the specific call based on a type-compatible variable.
For a pariticular i915 workload producing GPU context switches, __get_engine_usage_record() is particularly hot since the engine usage is read from device local memory with dgfx, possibly multiple times since it's racy. Test execution time for this test shows a ~12.5% improvement with DG2:
Before: nrepeats = 1000; min = 7.63243e+06; max = 1.01817e+07; median = 9.52548e+06; var = 526149; After: nrepeats = 1000; min = 7.03402e+06; max = 8.8832e+06; median = 8.33955e+06; var = 333113;
Other things attempted that didn't prove very useful:
- Change the _Generic() on x86 to just dereference the memory address
- Change __get_engine_usage_record() to do just 1 read per loop, comparing with the previous value read
- Change __get_engine_usage_record() to access the fields directly as it was before the conversion to iosys-map
(3) did gave a small improvement (~3%), but doesn't seem to scale well to other similar cases in the driver.
Additional test by Chris Wilson using gem_create from igt with some changes to track object creation time. This happens to accidentaly stress this code path:
Pre iosys_map conversion of engine busyness: lmem0: Creating 262144 4KiB objects took 59274.2ms
Unpatched: lmem0: Creating 262144 4KiB objects took 108830.2ms
With readl (this patch): lmem0: Creating 262144 4KiB objects took 61348.6ms
s/readl/READ_ONCE/ lmem0: Creating 262144 4KiB objects took 61333.2ms
So we do take a little bit more time than before the conversion, but that is due to other factors: bringing the READ_ONCE back would be as good as just doing this conversion.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
If this is acceptable we should probably add the write counterpart, too. Sending here only the read for now since this fixes the issue we are seeing and to gather feedback.
As far as I can see looks sane to me, but the kernel test robot tears the patch apart.
Probably just a typo somewhere in the 32bit handling.
oh, yeah... after cleaning it up I renamed s/__iosys_map_u64_case/__iosys_map_rd_io_u64_case/ to prepare for adding the write case and forgot the other side of the ifdef.
Apart from that looks good to me.
thanks Lucas De Marchi
Regards, Christian.
include/linux/iosys-map.h | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index e69a002d5aa4..4ae3e459419e 100644 --- a/include/linux/iosys-map.h +++ b/include/linux/iosys-map.h @@ -333,6 +333,20 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset, memset(dst->vaddr + offset, value, len); } +#ifdef CONFIG_64BIT +#define __iosys_map_rd_io_u64_case(val_, vaddr_iomem_) \
- u64: val_ = readq(vaddr_iomem_),
+#else +#define __iosys_map_u64_case(val_, vaddr_iomem_) +#endif
+#define __iosys_map_rd_io(val__, vaddr_iomem__, type__) _Generic(val__, \
- u8: val__ = readb(vaddr_iomem__), \
- u16: val__ = readw(vaddr_iomem__), \
- u32: val__ = readl(vaddr_iomem__), \
- __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \
- default: memcpy_fromio(&(val__), vaddr_iomem__, sizeof(val__)))
/**
- iosys_map_rd - Read a C-type value from the iosys_map
@@ -346,10 +360,14 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
- Returns:
- The value read from the mapping.
*/ -#define iosys_map_rd(map__, offset__, type__) ({ \
- type__ val; \
- iosys_map_memcpy_from(&val, map__, offset__, sizeof(val)); \
- val; \
+#define iosys_map_rd(map__, offset__, type__) ({ \
- type__ val; \
- if ((map__)->is_iomem) { \
__iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\- } else { \
memcpy(&val, (map__)->vaddr + offset__, sizeof(val)); \- } \
- val; \
}) /**
Hi Lucas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-tip/drm-tip] [also build test ERROR on linus/master v5.19-rc1 next-20220610] [cannot apply to tegra-drm/drm/tegra/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Lucas-De-Marchi/iosys-map-Add... base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: i386-defconfig (https://download.01.org/0day-ci/archive/20220610/202206102230.Ji70o9lR-lkp@i...) compiler: gcc-11 (Debian 11.3.0-3) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/7b9b2d6b8d738fe2857fa1a96f7f3c... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Lucas-De-Marchi/iosys-map-Add-word-sized-reads/20220610-072113 git checkout 7b9b2d6b8d738fe2857fa1a96f7f3c9d8c11e9cd # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
In file included from drivers/gpu/drm/i915/gt/uc/intel_guc.h:10, from drivers/gpu/drm/i915/gt/uc/intel_uc.h:9, from drivers/gpu/drm/i915/gt/intel_gt_types.h:18, from drivers/gpu/drm/i915/gt/intel_gt.h:10, from drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:9: drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c: In function 'intel_guc_ads_print_policy_info':
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:165:20: note: in expansion of macro 'ads_blob_read' 165 | ads_blob_read(guc, policies.dpc_promote_time)); | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/iosys-map.h:347:36: note: in definition of macro '__iosys_map_rd_io' 347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:165:20: note: in expansion of macro 'ads_blob_read' 165 | ads_blob_read(guc, policies.dpc_promote_time)); | ^~~~~~~~~~~~~
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:167:20: note: in expansion of macro 'ads_blob_read' 167 | ads_blob_read(guc, policies.max_num_work_items)); | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/iosys-map.h:347:36: note: in definition of macro '__iosys_map_rd_io' 347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:167:20: note: in expansion of macro 'ads_blob_read' 167 | ads_blob_read(guc, policies.max_num_work_items)); | ^~~~~~~~~~~~~
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:169:20: note: in expansion of macro 'ads_blob_read' 169 | ads_blob_read(guc, policies.global_flags)); | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/iosys-map.h:347:36: note: in definition of macro '__iosys_map_rd_io' 347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:169:20: note: in expansion of macro 'ads_blob_read' 169 | ads_blob_read(guc, policies.global_flags)); | ^~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c: In function 'intel_guc_global_policies_update':
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:192:30: note: in expansion of macro 'ads_blob_read' 192 | scheduler_policies = ads_blob_read(guc, ads.scheduler_policies); | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/iosys-map.h:347:36: note: in definition of macro '__iosys_map_rd_io' 347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:192:30: note: in expansion of macro 'ads_blob_read' 192 | scheduler_policies = ads_blob_read(guc, ads.scheduler_policies); | ^~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c: In function 'guc_prep_golden_context':
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:75:9: note: in expansion of macro 'iosys_map_rd_field' 75 | iosys_map_rd_field(map_, 0, struct guc_gt_system_info, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:505:22: note: in expansion of macro 'info_map_read' 505 | if (!info_map_read(&info_map, engine_enabled_masks[guc_class])) | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/iosys-map.h:347:36: note: in definition of macro '__iosys_map_rd_io' 347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:75:9: note: in expansion of macro 'iosys_map_rd_field' 75 | iosys_map_rd_field(map_, 0, struct guc_gt_system_info, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:505:22: note: in expansion of macro 'info_map_read' 505 | if (!info_map_read(&info_map, engine_enabled_masks[guc_class])) | ^~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c: In function 'guc_init_golden_context':
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:584:22: note: in expansion of macro 'ads_blob_read' 584 | if (!ads_blob_read(guc, system_info.engine_enabled_masks[guc_class])) | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/iosys-map.h:347:36: note: in definition of macro '__iosys_map_rd_io' 347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:584:22: note: in expansion of macro 'ads_blob_read' 584 | if (!ads_blob_read(guc, system_info.engine_enabled_masks[guc_class])) | ^~~~~~~~~~~~~ In file included from include/linux/bitfield.h:10, from drivers/gpu/drm/i915/i915_reg_defs.h:9, from drivers/gpu/drm/i915/gt/intel_engine_regs.h:9, from drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:8:
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:30:63: note: in definition of macro 'BUILD_BUG_ON_INVALID' 30 | #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e)))) | ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:600:17: note: in expansion of macro 'GEM_BUG_ON' 600 | GEM_BUG_ON(ads_blob_read(guc, ads.eng_state_size[guc_class]) != | ^~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:600:28: note: in expansion of macro 'ads_blob_read' 600 | GEM_BUG_ON(ads_blob_read(guc, ads.eng_state_size[guc_class]) != | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/build_bug.h:30:63: note: in definition of macro 'BUILD_BUG_ON_INVALID' 30 | #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e)))) | ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:600:17: note: in expansion of macro 'GEM_BUG_ON' 600 | GEM_BUG_ON(ads_blob_read(guc, ads.eng_state_size[guc_class]) != | ^~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:600:28: note: in expansion of macro 'ads_blob_read' 600 | GEM_BUG_ON(ads_blob_read(guc, ads.eng_state_size[guc_class]) != | ^~~~~~~~~~~~~
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:30:63: note: in definition of macro 'BUILD_BUG_ON_INVALID' 30 | #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e)))) | ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:602:17: note: in expansion of macro 'GEM_BUG_ON' 602 | GEM_BUG_ON(ads_blob_read(guc, ads.golden_context_lrca[guc_class]) != addr_ggtt); | ^~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:602:28: note: in expansion of macro 'ads_blob_read' 602 | GEM_BUG_ON(ads_blob_read(guc, ads.golden_context_lrca[guc_class]) != addr_ggtt); | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/build_bug.h:30:63: note: in definition of macro 'BUILD_BUG_ON_INVALID' 30 | #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e)))) | ^ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:602:17: note: in expansion of macro 'GEM_BUG_ON' 602 | GEM_BUG_ON(ads_blob_read(guc, ads.golden_context_lrca[guc_class]) != addr_ggtt); | ^~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:65:9: note: in expansion of macro 'iosys_map_rd_field' 65 | iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:602:28: note: in expansion of macro 'ads_blob_read' 602 | GEM_BUG_ON(ads_blob_read(guc, ads.golden_context_lrca[guc_class]) != addr_ggtt); | ^~~~~~~~~~~~~ In file included from drivers/gpu/drm/i915/gt/uc/intel_guc.h:10, from drivers/gpu/drm/i915/gt/uc/intel_uc.h:9, from drivers/gpu/drm/i915/gt/intel_gt_types.h:18, from drivers/gpu/drm/i915/gt/intel_gt.h:10, from drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:9: drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c: In function 'guc_capture_prep_lists':
include/linux/iosys-map.h:347:9: error: unknown type name '__iosys_map_rd_io_u64_case'
347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:366:17: note: in expansion of macro '__iosys_map_rd_io' 366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~~~~~~~~~~~~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:75:9: note: in expansion of macro 'iosys_map_rd_field' 75 | iosys_map_rd_field(map_, 0, struct guc_gt_system_info, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:652:30: note: in expansion of macro 'info_map_read' 652 | if (!info_map_read(&info_map, engine_enabled_masks[j])) { | ^~~~~~~~~~~~~
include/linux/iosys-map.h:366:35: error: expected ')' before 'val'
366 | __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ | ^~~ include/linux/iosys-map.h:347:36: note: in definition of macro '__iosys_map_rd_io' 347 | __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \ | ^~~~~ include/linux/iosys-map.h:452:9: note: in expansion of macro 'iosys_map_rd' 452 | iosys_map_rd(map__, struct_offset__ + offsetof(struct_type__, field__), \ | ^~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:75:9: note: in expansion of macro 'iosys_map_rd_field' 75 | iosys_map_rd_field(map_, 0, struct guc_gt_system_info, field_) | ^~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:652:30: note: in expansion of macro 'info_map_read' 652 | if (!info_map_read(&info_map, engine_enabled_masks[j])) { | ^~~~~~~~~~~~~ ..
vim +/__iosys_map_rd_io_u64_case +347 include/linux/iosys-map.h
342 343 #define __iosys_map_rd_io(val__, vaddr_iomem__, type__) _Generic(val__, \ 344 u8: val__ = readb(vaddr_iomem__), \ 345 u16: val__ = readw(vaddr_iomem__), \ 346 u32: val__ = readl(vaddr_iomem__), \
347 __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \
348 default: memcpy_fromio(&(val__), vaddr_iomem__, sizeof(val__))) 349 350 /** 351 * iosys_map_rd - Read a C-type value from the iosys_map 352 * 353 * @map__: The iosys_map structure 354 * @offset__: The offset from which to read 355 * @type__: Type of the value being read 356 * 357 * Read a C type value from iosys_map, handling possible un-aligned accesses to 358 * the mapping. 359 * 360 * Returns: 361 * The value read from the mapping. 362 */ 363 #define iosys_map_rd(map__, offset__, type__) ({ \ 364 type__ val; \ 365 if ((map__)->is_iomem) { \
366 __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\
367 } else { \ 368 memcpy(&val, (map__)->vaddr + offset__, sizeof(val)); \ 369 } \ 370 val; \ 371 }) 372
Hi Lucas
Am 10.06.22 um 01:20 schrieb Lucas De Marchi:
Instead of always falling back to memcpy_fromio() for any size, prefer using read{b,w,l}(). When reading struct members it's common to read individual integer variables individually. Going through memcpy_fromio() for each of them poses a high penalty.
Employ a similar trick as __seqprop() by using _Generic() to generate only the specific call based on a type-compatible variable.
For a pariticular i915 workload producing GPU context switches, __get_engine_usage_record() is particularly hot since the engine usage is read from device local memory with dgfx, possibly multiple times since it's racy. Test execution time for this test shows a ~12.5% improvement with DG2:
Before: nrepeats = 1000; min = 7.63243e+06; max = 1.01817e+07; median = 9.52548e+06; var = 526149; After: nrepeats = 1000; min = 7.03402e+06; max = 8.8832e+06; median = 8.33955e+06; var = 333113;
Other things attempted that didn't prove very useful:
- Change the _Generic() on x86 to just dereference the memory address
- Change __get_engine_usage_record() to do just 1 read per loop, comparing with the previous value read
- Change __get_engine_usage_record() to access the fields directly as it was before the conversion to iosys-map
(3) did gave a small improvement (~3%), but doesn't seem to scale well to other similar cases in the driver.
Additional test by Chris Wilson using gem_create from igt with some changes to track object creation time. This happens to accidentaly stress this code path:
Pre iosys_map conversion of engine busyness: lmem0: Creating 262144 4KiB objects took 59274.2ms
Unpatched: lmem0: Creating 262144 4KiB objects took 108830.2ms
With readl (this patch): lmem0: Creating 262144 4KiB objects took 61348.6ms
s/readl/READ_ONCE/ lmem0: Creating 262144 4KiB objects took 61333.2ms
So we do take a little bit more time than before the conversion, but that is due to other factors: bringing the READ_ONCE back would be as good as just doing this conversion.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
If this is acceptable we should probably add the write counterpart, too. Sending here only the read for now since this fixes the issue we are seeing and to gather feedback.
That would not be a problem, but please only add functions that you use.
include/linux/iosys-map.h | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index e69a002d5aa4..4ae3e459419e 100644 --- a/include/linux/iosys-map.h +++ b/include/linux/iosys-map.h @@ -333,6 +333,20 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset, memset(dst->vaddr + offset, value, len); }
+#ifdef CONFIG_64BIT +#define __iosys_map_rd_io_u64_case(val_, vaddr_iomem_) \
- u64: val_ = readq(vaddr_iomem_),
+#else +#define __iosys_map_u64_case(val_, vaddr_iomem_) +#endif
+#define __iosys_map_rd_io(val__, vaddr_iomem__, type__) _Generic(val__, \
- u8: val__ = readb(vaddr_iomem__), \
- u16: val__ = readw(vaddr_iomem__), \
- u32: val__ = readl(vaddr_iomem__), \
- __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \
- default: memcpy_fromio(&(val__), vaddr_iomem__, sizeof(val__)))
- /**
- iosys_map_rd - Read a C-type value from the iosys_map
@@ -346,10 +360,14 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
- Returns:
- The value read from the mapping.
*/ -#define iosys_map_rd(map__, offset__, type__) ({ \
- type__ val; \
- iosys_map_memcpy_from(&val, map__, offset__, sizeof(val)); \
- val; \
+#define iosys_map_rd(map__, offset__, type__) ({ \
- type__ val; \
- if ((map__)->is_iomem) { \
__iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\- } else { \
memcpy(&val, (map__)->vaddr + offset__, sizeof(val)); \- } \
- val; \
To my knowledge, calls to readw/readl have alignment requirements on some platforms, while memcpy_fromio() has none. Mixing memcpy() and read*() sounds like a problem for subtle bugs. I'd prefer to at least mitigate that to some extend.
For each case in the _Generic statement, there should be an if/else branch on is_iomem. Here's the example
#define iosys_map_rd() \ _Generic( (val__), u8: { if (map__)->is_iomem val__ = readb() else val__ *(volatile u8*)(vaddr_iomem); }, u16: { if (map__)->is_iomem val__ = readw() else val__ *(volatile u16*)(vaddr_iomem); }, u32, u64, ... default: { if (map__)->is_iomem mempy_fromio() else memcpy() })
Using volatile with system memory enforces single instructions or even alignment on some platforms. While experimenting with framebuffer updates, I've also found this to be faster then regular code. With 'volatile' the compiler generated a single movq instead of a number of shorter movs. (But I won't promise anything. :)
Within _Generic, for each type, a macro can generate the case. Like this
#define __iosys_map_rd_case(__type, __map, __read) \ __type: if else ...
In the case of u64, you can simply do
#if CONFIG_64BIT #define __iosys_map_rd_case_u64(__map) \ __iosys_map_rc_case(u64, __map, readq) #else #define __iosys_map_rd_case_u64(__map) #endif
and use that macro in the _Generic. On 64-bit systems, the case will be there. Otherwise it will be empty.
The only user of iosys_map_rd() is i915. I quickly looked through the usage and found no cases where the default memcpy could be used. It's all structs with u32. (Right?) If so, please remove the default case with memcpy entirely. This will result in clear compile-time errors if a certain type is not supported. There's still iosys_mem_memcpy() for those who need it.
Best regards Thomas
})
/**
On Mon, Jun 13, 2022 at 12:58:52PM +0200, Thomas Zimmermann wrote:
Hi Lucas
Am 10.06.22 um 01:20 schrieb Lucas De Marchi:
Instead of always falling back to memcpy_fromio() for any size, prefer using read{b,w,l}(). When reading struct members it's common to read individual integer variables individually. Going through memcpy_fromio() for each of them poses a high penalty.
Employ a similar trick as __seqprop() by using _Generic() to generate only the specific call based on a type-compatible variable.
For a pariticular i915 workload producing GPU context switches, __get_engine_usage_record() is particularly hot since the engine usage is read from device local memory with dgfx, possibly multiple times since it's racy. Test execution time for this test shows a ~12.5% improvement with DG2:
Before: nrepeats = 1000; min = 7.63243e+06; max = 1.01817e+07; median = 9.52548e+06; var = 526149; After: nrepeats = 1000; min = 7.03402e+06; max = 8.8832e+06; median = 8.33955e+06; var = 333113;
Other things attempted that didn't prove very useful:
- Change the _Generic() on x86 to just dereference the memory address
- Change __get_engine_usage_record() to do just 1 read per loop, comparing with the previous value read
- Change __get_engine_usage_record() to access the fields directly as it was before the conversion to iosys-map
(3) did gave a small improvement (~3%), but doesn't seem to scale well to other similar cases in the driver.
Additional test by Chris Wilson using gem_create from igt with some changes to track object creation time. This happens to accidentaly stress this code path:
Pre iosys_map conversion of engine busyness: lmem0: Creating 262144 4KiB objects took 59274.2ms
Unpatched: lmem0: Creating 262144 4KiB objects took 108830.2ms
With readl (this patch): lmem0: Creating 262144 4KiB objects took 61348.6ms
s/readl/READ_ONCE/ lmem0: Creating 262144 4KiB objects took 61333.2ms
So we do take a little bit more time than before the conversion, but that is due to other factors: bringing the READ_ONCE back would be as good as just doing this conversion.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
If this is acceptable we should probably add the write counterpart, too. Sending here only the read for now since this fixes the issue we are seeing and to gather feedback.
That would not be a problem, but please only add functions that you use.
I sent v2 that includes the write as the second patch. We do use it, it's just not in the critical path.
$ git grep iosys_map_wr drivers/ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c: iosys_map_wr_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, \ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c: iosys_map_wr_field(map_, 0, struct guc_gt_system_info, field_, val_)
include/linux/iosys-map.h | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index e69a002d5aa4..4ae3e459419e 100644 --- a/include/linux/iosys-map.h +++ b/include/linux/iosys-map.h @@ -333,6 +333,20 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset, memset(dst->vaddr + offset, value, len); } +#ifdef CONFIG_64BIT +#define __iosys_map_rd_io_u64_case(val_, vaddr_iomem_) \
- u64: val_ = readq(vaddr_iomem_),
+#else +#define __iosys_map_u64_case(val_, vaddr_iomem_) +#endif
+#define __iosys_map_rd_io(val__, vaddr_iomem__, type__) _Generic(val__, \
- u8: val__ = readb(vaddr_iomem__), \
- u16: val__ = readw(vaddr_iomem__), \
- u32: val__ = readl(vaddr_iomem__), \
- __iosys_map_rd_io_u64_case(val__, vaddr_iomem__) \
- default: memcpy_fromio(&(val__), vaddr_iomem__, sizeof(val__)))
/**
- iosys_map_rd - Read a C-type value from the iosys_map
@@ -346,10 +360,14 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
- Returns:
- The value read from the mapping.
*/ -#define iosys_map_rd(map__, offset__, type__) ({ \
- type__ val; \
- iosys_map_memcpy_from(&val, map__, offset__, sizeof(val)); \
- val; \
+#define iosys_map_rd(map__, offset__, type__) ({ \
- type__ val; \
- if ((map__)->is_iomem) { \
__iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\- } else { \
memcpy(&val, (map__)->vaddr + offset__, sizeof(val)); \- } \
- val; \
To my knowledge, calls to readw/readl have alignment requirements on some platforms, while memcpy_fromio() has none. Mixing memcpy() and read*() sounds like a problem for subtle bugs. I'd prefer to at least mitigate that to some extend.
For each case in the _Generic statement, there should be an if/else branch on is_iomem. Here's the example
#define iosys_map_rd() \ _Generic( (val__), u8: { if (map__)->is_iomem val__ = readb() else val__ *(volatile u8*)(vaddr_iomem); }, u16: { if (map__)->is_iomem val__ = readw() else val__ *(volatile u16*)(vaddr_iomem); }, u32, u64, ... default: { if (map__)->is_iomem mempy_fromio() else memcpy() })
Using volatile with system memory enforces single instructions or even alignment on some platforms. While experimenting with framebuffer updates, I've also found this to be faster then regular code. With 'volatile' the compiler generated a single movq instead of a number of shorter movs. (But I won't promise anything. :)
Here I focused on making the *io* part faster instead of the system memory part faster since io is order of magnitudes slower and showing up on perf. Yes, we can also do it on the system memory part.
Note that the READ_ONCE() I mentioned in the commit message does the volatile cast behind the scene. I also checked that the readl in the generated code expands to a mov instruction in 64-bit mode, which is correct.
Within _Generic, for each type, a macro can generate the case. Like this
#define __iosys_map_rd_case(__type, __map, __read) \ __type: if else ...
In the case of u64, you can simply do
#if CONFIG_64BIT #define __iosys_map_rd_case_u64(__map) \ __iosys_map_rc_case(u64, __map, readq) #else #define __iosys_map_rd_case_u64(__map) #endif
and use that macro in the _Generic. On 64-bit systems, the case will be there. Otherwise it will be empty.
/me confused... this is what I did, no?
The only user of iosys_map_rd() is i915. I quickly looked through the usage and found no cases where the default memcpy could be used. It's all structs with u32. (Right?) If so, please remove the default case with memcpy entirely. This will result in clear compile-time errors if a certain type is not supported. There's still iosys_mem_memcpy() for those who need it.
yeah, I think we had cases copying the whole struct before. Just took a look and removing the default doesn't explodes the build, so we should be fine... although it may lead to build issue if we are trying to read a u64 on 32-bit since it won't fallback to memcpy by simply expanding the u64 case to nothing.
Another thing not clear: how does moving the if/else inside the _Generic() help with unaligned accesses?
For your suggestion on also handling system memory, I was actually thinking on leaving the if/else where it is now and in future add a _Generic() for the system memory accesses (hence why my macro has he _io suffix):
diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index d092d30f5812..0e758424088e 100644 --- a/include/linux/iosys-map.h +++ b/include/linux/iosys-map.h @@ -375,7 +375,7 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset, if ((map__)->is_iomem) { \ __iosys_map_rd_io(val, (map__)->vaddr_iomem + offset__, type__);\ } else { \ - memcpy(&val, (map__)->vaddr + offset__, sizeof(val)); \ + __iosys_map_rd_sys(val, (map__)->vaddr_iomem + offset__, type__);\ } \ val; \ })
it seems cleaner than adding the if/else to each case expanded by _Generic().
thanks Lucas De Marchi
dri-devel@lists.freedesktop.org
-
Christian König -
kernel test robot -
Lucas De Marchi -
Thomas Zimmermann