We don't really need such a big on-stack buffer.
vmbus_sendpacket() here only uses sizeof(struct pci_child_message).
Signed-off-by: Dexuan Cui <[email protected]>
CC: Jake Oshins <[email protected]>
Cc: KY Srinivasan <[email protected]>
CC: Haiyang Zhang <[email protected]>
CC: Vitaly Kuznetsov <[email protected]>
---
drivers/pci/host/pci-hyperv.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 763ff87..93ed64a 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -1271,9 +1271,9 @@ static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
struct hv_pci_dev *hpdev;
struct pci_child_message *res_req;
struct q_res_req_compl comp_pkt;
- union {
- struct pci_packet init_packet;
- u8 buffer[0x100];
+ struct {
+ struct pci_packet init_packet;
+ u8 buffer[sizeof(struct pci_child_message)];
} pkt;
unsigned long flags;
int ret;
--
2.7.4
> From: Jake Oshins
> > From: Dexuan Cui
> > Sent: Wednesday, November 9, 2016 11:18 PM
> > We don't really need such a big on-stack buffer.
> > vmbus_sendpacket() here only uses sizeof(struct pci_child_message).
> >
> > @@ -1271,9 +1271,9 @@ static struct hv_pci_dev
> > *new_pcichild_device(struct hv_pcibus_device *hbus,
> > struct hv_pci_dev *hpdev;
> > struct pci_child_message *res_req;
> > struct q_res_req_compl comp_pkt;
> > - union {
> > - struct pci_packet init_packet;
> > - u8 buffer[0x100];
> > + struct {
> > + struct pci_packet init_packet;
> > + u8 buffer[sizeof(struct pci_child_message)];
> > } pkt;
> > unsigned long flags;
> > int ret;
>
> This change seems good to me, in that it's always a bad idea to use too much
> stack. But this won't fix the problem with VMAP_STACK. The buffer could still
> end up spanning two pages and the physical addresses of those pages would
> possibly be discontiguous. Do you want to just refactor this so that it uses a
> fixed buffer, one that will work with VMAP_STACK? Or is that coming in a future
> patch?
Hi Jake, I think the VMAP_STACK issue you mentioned should be another different
issue fixed by Long Li: https://patchwork.ozlabs.org/patch/692447/.
The VMAP_STACK issue is only an issue when we pass the buffer's physical address
to the hypercall.
Here the buffer is not passed to any hypercall. We just use vmbus_sendpacket()
to memcpy the buffer into the per-channel ringbuffer.
Thanks,
-- Dexuan
> -----Original Message-----
>
> > From: Jake Oshins
> > > From: Dexuan Cui
> > > Sent: Wednesday, November 9, 2016 11:18 PM
> > > We don't really need such a big on-stack buffer.
> > > vmbus_sendpacket() here only uses sizeof(struct pci_child_message).
> > >
> > > @@ -1271,9 +1271,9 @@ static struct hv_pci_dev
> > > *new_pcichild_device(struct hv_pcibus_device *hbus,
> > > struct hv_pci_dev *hpdev;
> > > struct pci_child_message *res_req;
> > > struct q_res_req_compl comp_pkt;
> > > - union {
> > > - struct pci_packet init_packet;
> > > - u8 buffer[0x100];
> > > + struct {
> > > + struct pci_packet init_packet;
> > > + u8 buffer[sizeof(struct pci_child_message)];
> > > } pkt;
> > > unsigned long flags;
> > > int ret;
> >
> > This change seems good to me, in that it's always a bad idea to use too
> much
> > stack. But this won't fix the problem with VMAP_STACK. The buffer could
> still
> > end up spanning two pages and the physical addresses of those pages
> would
> > possibly be discontiguous. Do you want to just refactor this so that it uses
> a
> > fixed buffer, one that will work with VMAP_STACK? Or is that coming in a
> future
> > patch?
>
> Hi Jake, I think the VMAP_STACK issue you mentioned should be another
> different
> issue fixed by Long Li: https://patchwork.ozlabs.org/patch/692447/.
>
> The VMAP_STACK issue is only an issue when we pass the buffer's physical
> address
> to the hypercall.
>
> Here the buffer is not passed to any hypercall. We just use
> vmbus_sendpacket()
> to memcpy the buffer into the per-channel ringbuffer.
>
> Thanks,
> -- Dexuan
Yes, you're right. This patch looks fine to me. Sorry for confusing the two issues.
-- Jake
> -----Original Message-----
> From: Dexuan Cui
> Sent: Wednesday, November 9, 2016 11:18 PM
> To: Bjorn Helgaas <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; KY Srinivasan <[email protected]>;
> Haiyang Zhang <[email protected]>; Stephen Hemminger
> <[email protected]>; Jake Oshins <[email protected]>; Hadden
> Hoppert <[email protected]>; Vitaly Kuznetsov
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: [PATCH 1/3] PCI: hv: use the correct buffer size in
> new_pcichild_device()
>
> We don't really need such a big on-stack buffer.
> vmbus_sendpacket() here only uses sizeof(struct pci_child_message).
>
> Signed-off-by: Dexuan Cui <[email protected]>
> CC: Jake Oshins <[email protected]>
> Cc: KY Srinivasan <[email protected]>
> CC: Haiyang Zhang <[email protected]>
> CC: Vitaly Kuznetsov <[email protected]>
> ---
> drivers/pci/host/pci-hyperv.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 763ff87..93ed64a 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1271,9 +1271,9 @@ static struct hv_pci_dev
> *new_pcichild_device(struct hv_pcibus_device *hbus,
> struct hv_pci_dev *hpdev;
> struct pci_child_message *res_req;
> struct q_res_req_compl comp_pkt;
> - union {
> - struct pci_packet init_packet;
> - u8 buffer[0x100];
> + struct {
> + struct pci_packet init_packet;
> + u8 buffer[sizeof(struct pci_child_message)];
> } pkt;
> unsigned long flags;
> int ret;
> --
> 2.7.4
This change seems good to me, in that it's always a bad idea to use too much stack. But this won't fix the problem with VMAP_STACK. The buffer could still end up spanning two pages and the physical addresses of those pages would possibly be discontiguous. Do you want to just refactor this so that it uses a fixed buffer, one that will work with VMAP_STACK? Or is that coming in a future patch?
-- Jake Oshins
> -----Original Message-----
> From: Dexuan Cui
> Sent: Wednesday, November 9, 2016 11:18 PM
> To: Bjorn Helgaas <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; KY Srinivasan <[email protected]>;
> Haiyang Zhang <[email protected]>; Stephen Hemminger
> <[email protected]>; Jake Oshins <[email protected]>; Hadden
> Hoppert <[email protected]>; Vitaly Kuznetsov
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: [PATCH 1/3] PCI: hv: use the correct buffer size in
> new_pcichild_device()
>
> We don't really need such a big on-stack buffer.
> vmbus_sendpacket() here only uses sizeof(struct pci_child_message).
>
> Signed-off-by: Dexuan Cui <[email protected]>
> CC: Jake Oshins <[email protected]>
> Cc: KY Srinivasan <[email protected]>
> CC: Haiyang Zhang <[email protected]>
> CC: Vitaly Kuznetsov <[email protected]>
Thanks Dexuan.
Acked-by: K. Y. Srinivasan <[email protected]>
> ---
> drivers/pci/host/pci-hyperv.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 763ff87..93ed64a 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1271,9 +1271,9 @@ static struct hv_pci_dev
> *new_pcichild_device(struct hv_pcibus_device *hbus,
> struct hv_pci_dev *hpdev;
> struct pci_child_message *res_req;
> struct q_res_req_compl comp_pkt;
> - union {
> - struct pci_packet init_packet;
> - u8 buffer[0x100];
> + struct {
> + struct pci_packet init_packet;
> + u8 buffer[sizeof(struct pci_child_message)];
> } pkt;
> unsigned long flags;
> int ret;
> --
> 2.7.4