On Thu, Nov 14, 2019 at 10:35 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote: > > > > > Am 14.11.2019 um 19:14 schrieb Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>: > > > > On Thu, Nov 14, 2019 at 9:40 AM Song Liu <liu.song.a23@xxxxxxxxx> wrote: > >> > >> On Wed, Nov 13, 2019 at 9:20 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote: > >>> > >>> Currently passing alignment greater than 4 to bpf_jit_binary_alloc does > >>> not work: in such cases it aligns only to 4 bytes. > >>> > >>> However, this is required on s390, where in order to load a constant > >>> from memory in a large (>512k) BPF program, one must use lgrl > >>> instruction, whose memory operand must be aligned on an 8-byte boundary. > >>> > >>> This patch makes it possible to request an arbitrary power-of-2 > >>> alignment from bpf_jit_binary_alloc by allocating extra padding bytes > >>> and aligning the resulting pointer rather than the start offset. > >>> > >>> An alternative would be to simply increase the alignment of > >>> bpf_binary_header.image to 8, but this would increase the risk of > >>> wasting a page on arches that don't need it, and would also be > >>> insufficient in case someone needs e.g. 16-byte alignment in the > >>> future. > > > > why not 8 or 16? I don't follow why that would waste a page. > > It might waste a page because bpf_jit_binary_alloc rounds up allocation > size to PAGE_SIZE, and unnecessary padding might be the last straw that > would cause another page to be allocated. But that would apply only to > a tiny amount of programs, whose JITed size is slightly smaller than a > multiple of PAGE_SIZE. > > Sorry, I didn't fully get the 8 vs 16 question. At the moment, for > s390-specific purpose 8 would be enough. I used 16 just to demonstrate > that this solution wouldn't be future-proof. AFAIK some Intel > instructions might want 16 (VMOVDQA?). But maybe it's better to think > about it when someone actually needs to use them. > > >>> > >>> Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx> > >> > >> Maybe we can just make it 8 byte aligned for all architectures? > >> > >> #define BPF_IMAGE_ALIGNMENT 8 > > Seems like I'm overthinking this. If just bumping the alignment to 8 is > OK, then I'll send a simpler patch. yes. I think that's better.
