| Message ID | 397bf325-f81e-e104-6142-e8c9c4955475@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | swiotlb-xen: fixes and adjustments | expand |
On Tue, Sep 07, 2021 at 02:05:32PM +0200, Jan Beulich wrote: > While the hypervisor hasn't been enforcing this, we would still better > avoid issuing requests with GFNs not aligned to the requested order. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I wonder how useful it is to include the alignment in the panic() > message. I further wonder how useful it is to wrap "bytes" in > PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at > least was supposed to be, prior to "swiotlb-xen: maintain slab count > properly"). > > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -231,10 +231,10 @@ retry: > /* > * Get IO TLB memory from any location. > */ > - start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE); > + start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT); > if (!start) > - panic("%s: Failed to allocate %lu bytes align=0x%lx\n", > - __func__, PAGE_ALIGN(bytes), PAGE_SIZE); > + panic("%s: Failed to allocate %lu bytes align=%#x\n", > + __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT); CAn you avoid the overly long lines here? A good way to make it more readable would be a variable to hold the byte count.
On 08.09.2021 08:57, Christoph Hellwig wrote: > On Tue, Sep 07, 2021 at 02:05:32PM +0200, Jan Beulich wrote: >> While the hypervisor hasn't been enforcing this, we would still better >> avoid issuing requests with GFNs not aligned to the requested order. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I wonder how useful it is to include the alignment in the panic() >> message. I further wonder how useful it is to wrap "bytes" in >> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at >> least was supposed to be, prior to "swiotlb-xen: maintain slab count >> properly"). >> >> --- a/drivers/xen/swiotlb-xen.c >> +++ b/drivers/xen/swiotlb-xen.c >> @@ -231,10 +231,10 @@ retry: >> /* >> * Get IO TLB memory from any location. >> */ >> - start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE); >> + start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT); >> if (!start) >> - panic("%s: Failed to allocate %lu bytes align=0x%lx\n", >> - __func__, PAGE_ALIGN(bytes), PAGE_SIZE); >> + panic("%s: Failed to allocate %lu bytes align=%#x\n", >> + __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT); > > CAn you avoid the overly long lines here? A good way to make it more > readable would be a variable to hold the byte count. There already is a variable for the byte count - bytes. Did you read the post-commit-message remark? _That's_ what I'd prefer to shorten things. Meanwhile I can of course wrap lines; I will admit that I failed to pay attention to line length here. Jan
On Tue, 7 Sep 2021, Jan Beulich wrote: > While the hypervisor hasn't been enforcing this, we would still better > avoid issuing requests with GFNs not aligned to the requested order. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I wonder how useful it is to include the alignment in the panic() > message. Not very useful given that it is static. I don't mind either way but you can go ahead and remove it if you prefer (and it would make the line shorter.) > I further wonder how useful it is to wrap "bytes" in > PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at > least was supposed to be, prior to "swiotlb-xen: maintain slab count > properly"). This one I would keep, to make sure to print out the same amount passed to memblock_alloc. > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -231,10 +231,10 @@ retry: > /* > * Get IO TLB memory from any location. > */ > - start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE); > + start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT); > if (!start) > - panic("%s: Failed to allocate %lu bytes align=0x%lx\n", > - __func__, PAGE_ALIGN(bytes), PAGE_SIZE); > + panic("%s: Failed to allocate %lu bytes align=%#x\n", > + __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT); > > /* > * And replace that memory with pages under 4GB. >
On 11.09.2021 01:14, Stefano Stabellini wrote: > On Tue, 7 Sep 2021, Jan Beulich wrote: >> While the hypervisor hasn't been enforcing this, we would still better >> avoid issuing requests with GFNs not aligned to the requested order. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I wonder how useful it is to include the alignment in the panic() >> message. > > Not very useful given that it is static. I don't mind either way but you > can go ahead and remove it if you prefer (and it would make the line > shorter.) > > >> I further wonder how useful it is to wrap "bytes" in >> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at >> least was supposed to be, prior to "swiotlb-xen: maintain slab count >> properly"). > > This one I would keep, to make sure to print out the same amount passed > to memblock_alloc. Oh - if I was to drop it from the printk(), I would have been meaning to also drop it there. If it's useless, then it's useless everywhere. Jan
On Mon, 13 Sep 2021, Jan Beulich wrote: > On 11.09.2021 01:14, Stefano Stabellini wrote: > > On Tue, 7 Sep 2021, Jan Beulich wrote: > >> While the hypervisor hasn't been enforcing this, we would still better > >> avoid issuing requests with GFNs not aligned to the requested order. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> I wonder how useful it is to include the alignment in the panic() > >> message. > > > > Not very useful given that it is static. I don't mind either way but you > > can go ahead and remove it if you prefer (and it would make the line > > shorter.) > > > > > >> I further wonder how useful it is to wrap "bytes" in > >> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at > >> least was supposed to be, prior to "swiotlb-xen: maintain slab count > >> properly"). > > > > This one I would keep, to make sure to print out the same amount passed > > to memblock_alloc. > > Oh - if I was to drop it from the printk(), I would have been meaning to > also drop it there. If it's useless, then it's useless everywhere. That's fine too
On 13.09.2021 22:31, Stefano Stabellini wrote: > On Mon, 13 Sep 2021, Jan Beulich wrote: >> On 11.09.2021 01:14, Stefano Stabellini wrote: >>> On Tue, 7 Sep 2021, Jan Beulich wrote: >>>> While the hypervisor hasn't been enforcing this, we would still better >>>> avoid issuing requests with GFNs not aligned to the requested order. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> I wonder how useful it is to include the alignment in the panic() >>>> message. >>> >>> Not very useful given that it is static. I don't mind either way but you >>> can go ahead and remove it if you prefer (and it would make the line >>> shorter.) >>> >>> >>>> I further wonder how useful it is to wrap "bytes" in >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count >>>> properly"). >>> >>> This one I would keep, to make sure to print out the same amount passed >>> to memblock_alloc. >> >> Oh - if I was to drop it from the printk(), I would have been meaning to >> also drop it there. If it's useless, then it's useless everywhere. > > That's fine too Thanks, I'll see about dropping that then. Another Arm-related question has occurred to me: Do you actually mind the higher-than-necessary alignment there? If so, a per-arch definition of the needed alignment would need introducing. Maybe that could default to PAGE_SIZE, allowing Arm and alike to get away without explicitly specifying a value ... Jan
On Tue, 14 Sep 2021, Jan Beulich wrote: > On 13.09.2021 22:31, Stefano Stabellini wrote: > > On Mon, 13 Sep 2021, Jan Beulich wrote: > >> On 11.09.2021 01:14, Stefano Stabellini wrote: > >>> On Tue, 7 Sep 2021, Jan Beulich wrote: > >>>> While the hypervisor hasn't been enforcing this, we would still better > >>>> avoid issuing requests with GFNs not aligned to the requested order. > >>>> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>>> --- > >>>> I wonder how useful it is to include the alignment in the panic() > >>>> message. > >>> > >>> Not very useful given that it is static. I don't mind either way but you > >>> can go ahead and remove it if you prefer (and it would make the line > >>> shorter.) > >>> > >>> > >>>> I further wonder how useful it is to wrap "bytes" in > >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at > >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count > >>>> properly"). > >>> > >>> This one I would keep, to make sure to print out the same amount passed > >>> to memblock_alloc. > >> > >> Oh - if I was to drop it from the printk(), I would have been meaning to > >> also drop it there. If it's useless, then it's useless everywhere. > > > > That's fine too > > Thanks, I'll see about dropping that then. > > Another Arm-related question has occurred to me: Do you actually > mind the higher-than-necessary alignment there? If so, a per-arch > definition of the needed alignment would need introducing. Maybe > that could default to PAGE_SIZE, allowing Arm and alike to get away > without explicitly specifying a value ... Certainly a patch like that could be good. Given that it is only one allocation I was assuming that the higher-than-necessary alignment wouldn't be a problem worth addressing (and I cannot completely rule out that one day we might have to use XENMEM_exchange on ARM too).
On Tue, 14 Sep 2021, Stefano Stabellini wrote: > On Tue, 14 Sep 2021, Jan Beulich wrote: > > On 13.09.2021 22:31, Stefano Stabellini wrote: > > > On Mon, 13 Sep 2021, Jan Beulich wrote: > > >> On 11.09.2021 01:14, Stefano Stabellini wrote: > > >>> On Tue, 7 Sep 2021, Jan Beulich wrote: > > >>>> While the hypervisor hasn't been enforcing this, we would still better > > >>>> avoid issuing requests with GFNs not aligned to the requested order. > > >>>> > > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > >>>> --- > > >>>> I wonder how useful it is to include the alignment in the panic() > > >>>> message. > > >>> > > >>> Not very useful given that it is static. I don't mind either way but you > > >>> can go ahead and remove it if you prefer (and it would make the line > > >>> shorter.) > > >>> > > >>> > > >>>> I further wonder how useful it is to wrap "bytes" in > > >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at > > >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count > > >>>> properly"). > > >>> > > >>> This one I would keep, to make sure to print out the same amount passed > > >>> to memblock_alloc. > > >> > > >> Oh - if I was to drop it from the printk(), I would have been meaning to > > >> also drop it there. If it's useless, then it's useless everywhere. > > > > > > That's fine too > > > > Thanks, I'll see about dropping that then. > > > > Another Arm-related question has occurred to me: Do you actually > > mind the higher-than-necessary alignment there? If so, a per-arch > > definition of the needed alignment would need introducing. Maybe > > that could default to PAGE_SIZE, allowing Arm and alike to get away > > without explicitly specifying a value ... > > Certainly a patch like that could be good. Given that it is only one > allocation I was assuming that the higher-than-necessary alignment > wouldn't be a problem worth addressing (and I cannot completely rule out > that one day we might have to use XENMEM_exchange on ARM too). Also this code is currently #ifdef CONFIG_X86
On 15.09.2021 03:54, Stefano Stabellini wrote: > On Tue, 14 Sep 2021, Stefano Stabellini wrote: >> On Tue, 14 Sep 2021, Jan Beulich wrote: >>> On 13.09.2021 22:31, Stefano Stabellini wrote: >>>> On Mon, 13 Sep 2021, Jan Beulich wrote: >>>>> On 11.09.2021 01:14, Stefano Stabellini wrote: >>>>>> On Tue, 7 Sep 2021, Jan Beulich wrote: >>>>>>> While the hypervisor hasn't been enforcing this, we would still better >>>>>>> avoid issuing requests with GFNs not aligned to the requested order. >>>>>>> >>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>>> --- >>>>>>> I wonder how useful it is to include the alignment in the panic() >>>>>>> message. >>>>>> >>>>>> Not very useful given that it is static. I don't mind either way but you >>>>>> can go ahead and remove it if you prefer (and it would make the line >>>>>> shorter.) >>>>>> >>>>>> >>>>>>> I further wonder how useful it is to wrap "bytes" in >>>>>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at >>>>>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count >>>>>>> properly"). >>>>>> >>>>>> This one I would keep, to make sure to print out the same amount passed >>>>>> to memblock_alloc. >>>>> >>>>> Oh - if I was to drop it from the printk(), I would have been meaning to >>>>> also drop it there. If it's useless, then it's useless everywhere. >>>> >>>> That's fine too >>> >>> Thanks, I'll see about dropping that then. >>> >>> Another Arm-related question has occurred to me: Do you actually >>> mind the higher-than-necessary alignment there? If so, a per-arch >>> definition of the needed alignment would need introducing. Maybe >>> that could default to PAGE_SIZE, allowing Arm and alike to get away >>> without explicitly specifying a value ... >> >> Certainly a patch like that could be good. Given that it is only one >> allocation I was assuming that the higher-than-necessary alignment >> wouldn't be a problem worth addressing (and I cannot completely rule out >> that one day we might have to use XENMEM_exchange on ARM too). > > Also this code is currently #ifdef CONFIG_X86 Oh, good point. When writing the patch I did take this into consideration, but I had managed to forget that aspect in the meantime. No adjustment to this effect needed then. Jan
--- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -231,10 +231,10 @@ retry: /* * Get IO TLB memory from any location. */ - start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE); + start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT); if (!start) - panic("%s: Failed to allocate %lu bytes align=0x%lx\n", - __func__, PAGE_ALIGN(bytes), PAGE_SIZE); + panic("%s: Failed to allocate %lu bytes align=%#x\n", + __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT); /* * And replace that memory with pages under 4GB.
While the hypervisor hasn't been enforcing this, we would still better avoid issuing requests with GFNs not aligned to the requested order. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I wonder how useful it is to include the alignment in the panic() message. I further wonder how useful it is to wrap "bytes" in PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at least was supposed to be, prior to "swiotlb-xen: maintain slab count properly").