Aubrey, On Thu, 8 Nov 2018, Aubrey Li wrote:
> Subject: .... x86/fpu: detect AVX task What is an AVX task? I know what you mean, but for the casual reader this is not very informative. So something like: x86/fpu: Track AVX usage of tasks would be more informative and precise. The mechanism you add is tracking the state and not just detecting in. > XSAVES and its variants use init optimization to reduce the amount of > data that they save to memory during context switch. Init optimization > uses the state component bitmap to denote if a component is in its init > configuration. We use this information to detect if a task contains AVX > instructions. Please avoid 'We use..'. Changelogs should be factual and precise and not be written as first-person narrative. Aside of that, you very well explained how XSAVES optimization works, but that's only a small part of the picture. The changelog should also contain a justification why this is necessary in the first place along with more missing bits and pieces. And please use paragraphs to structure the changelog instead of having one big lump. Something like this: User space tools which do automated task placement need information about AVX usage of tasks, because of <reasons>.... The extended control register (XCR) allows access to the XINUSE state-component bitmap, which allows software to discover the state of the init optimization used by XSAVEOPT and XSAVES. Set bits in the bitmap denote the usage of AVX, SIMD, FPU and other components. The XSAVE variants store only the state of used components to speed up the operation. The XGETBV instruction, if supported, allows software to read the state-component bitmap and use this information to detect the usage of the tracked components. Add accessor functions and per task state tracking to context switch. The tracking turns on the usage flag immediately, but requires 3 consecutive context switches with no usage to clear it. This decay is required because of <reasons>.... You surely had all this information in your head when writing the code and the changelog, so you could have spared me to dig all this out of the SDM. > +/* > + * This function is called during context switch to update AVX component > state > + */ > +static inline void update_avx_state(struct avx_state *avx) > +{ > + /* > + * Check if XGETBV with ECX = 1 supported. XGETBV with ECX = 1 > + * returns the logical-AND of XCR0 and XINUSE. XINUSE is a bitmap > + * by which the processor tracks the status of various components. > + */ > + if (!use_xgetbv1()) { > + avx->state = 0; avx->state should be initialized to 0 when the task starts, so why does it need to be cleared when XGETBV is not supported? Also this is the only usage site of use_xgetbv1(). So please open code it here and avoid the extra inline which does not provide any extra value in this case. > + return; > + } > + /* > + * XINUSE is dynamic to track component state because VZEROUPPER > + * happens on every function end and reset the bitmap to the > + * initial configuration. > + * > + * State decay is introduced to solve the race condition between > + * context switch and a function end. State is aggressively set > + * once it's detected but need to be cleared by decay 3 context > + * switches I have no idea what _the_ race condition between context switch and a function end is about. Probably most readers wont have. > + */ > + if (xgetbv(XINUSE_STATE_BITMAP_INDEX) & XFEATURE_MASK_Hi16_ZMM) { In the changelog and also in the code you say AVX (avx->state), but this actually looks only for Hi16_ZMM state, which are the upper 16 AVX512 registers. So again, this wants to be documented in the changelog along with an explanation WHY this check is restricted to Hi16_ZMM state. And please rename the variable accordingly. This is confusing at best. > + avx->state = 1; > + avx->decay_count = AVX_STATE_DECAY_COUNT; > + } else { > + if (avx->decay_count) > + avx->decay_count--; > + else > + avx->state = 0; > + } Is there a reason why you need two variables for this? if (xgetbv(XINUSE_STATE_BITMAP_INDEX) & XFEATURE_MASK_Hi16_ZMM) tsk->hi16zmm_usage = HI16ZMM_DECAY_COUNT; else if (tsk->hi16zmm_usage) tsk->hi16zmm_usage--; and the function which exposes it later to user space can just check whether tsk->hi16zmm_usage is 0 or not. This is context switch and we really want to avoid every pointless instruction we can. Thanks, tglx