Re: [PATCH] vDPA/ifcvf: fix the Calculation of queuepair

Re: [PATCH] vDPA/ifcvf: fix the Calculation of queuepair

[Michael S. Tsirkin]

On Thu, Sep 22, 2022 at 08:55:26AM +0000, Angus Chen wrote:
> >From 4f65eae86ab15d7abb8bd30401187cb195dfd27b Mon Sep 17 00:00:00 2001
> From: "angus.chen" <angus.chen@jaguarmicro.com>
> Date: Thu, 22 Sep 2022 14:47:28 +0800
> Subject: [PATCH]     vDPA/ifcvf: fix the Calculation of queuepair
> 
>         The queuepair should be divided by 2


this is just repeating what the patch does.
can you include more info pls?

Documentation/process/5.Posting.rst says among other things:

To that end, the summary line should describe the effects of and motivation
for the change as well as possible given the one-line constraint.  The
detailed description can then amplify on those topics and provide any
needed additional information.  If the patch fixes a bug, cite the commit
which introduced the bug if possible (and please provide both the commit ID
and the title when citing commits).  If a problem is associated with
specific log or compiler output, include that output to help others
searching for a solution to the same problem.  If the change is meant to
support other changes coming in later patch, say so.  If internal APIs are
changed, detail those changes and how other developers should respond.  In
general, the more you can put yourself into the shoes of everybody who will
be reading your changelog, the better that changelog (and the kernel as a
whole) will be.

 

> Signed-off-by: angus.chen <angus.chen@jaguarmicro.com>

Format should be

Angus Chen <angus.chen@jaguarmicro.com>

Also pls drop leading space.


> ---
> drivers/vdpa/ifcvf/ifcvf_base.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 75a703b803a2..3e4486bfa0b7 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -323,7 +323,7 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
>         u32 q_pair_id;
> 
>         ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> -       q_pair_id = qid / hw->nr_vring;
> +       q_pair_id = qid / 2;
>         avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
>         last_avail_idx = vp_ioread16(avail_idx_addr);
> 
> @@ -337,7 +337,7 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
>         u32 q_pair_id;
> 
>         ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> -       q_pair_id = qid / hw->nr_vring;
> +       q_pair_id = qid / 2;
>         avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
>         hw->vring[qid].last_avail_idx = num;
>         vp_iowrite16(num, avail_idx_addr);


Pls CC more widely.
$ ./scripts/get_maintainer.pl -f drivers/vdpa/ifcvf/ifcvf_base.c
"Michael S. Tsirkin" <mst@redhat.com> (maintainer:VIRTIO CORE AND NET DRIVERS,commit_signer:6/6=100%)
Jason Wang <jasowang@redhat.com> (maintainer:VIRTIO CORE AND NET DRIVERS,commit_signer:1/6=17%)
Zhu Lingshan <lingshan.zhu@intel.com> (commit_signer:5/6=83%,authored:5/6=83%,added_lines:102/102=100%,removed_lines:108/109=99%)
Stefano Garzarella <sgarzare@redhat.com> (commit_signer:1/6=17%)
Tom Rix <trix@redhat.com> (commit_signer:1/6=17%)
Colin Ian King <colin.king@intel.com> (authored:1/6=17%)
virtualization@lists.linux-foundation.org (open list:VIRTIO CORE AND NET DRIVERS)

Thanks!

BTW, Zhu Lingshan, would you like to be listed a patch reviewer in
MAINTAINERS so people rememeber to CC you? You are working a lot on this
driver.



> --

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] vDPA/ifcvf: fix the Calculation of queuepair

[Michael S. Tsirkin]

On Thu, Sep 22, 2022 at 06:07:58PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 9/22/2022 5:06 PM, Michael S. Tsirkin wrote:
> > On Thu, Sep 22, 2022 at 08:55:26AM +0000, Angus Chen wrote:
> > > >From 4f65eae86ab15d7abb8bd30401187cb195dfd27b Mon Sep 17 00:00:00 2001
> > > From: "angus.chen" <angus.chen@jaguarmicro.com>
> > > Date: Thu, 22 Sep 2022 14:47:28 +0800
> > > Subject: [PATCH]     vDPA/ifcvf: fix the Calculation of queuepair
> > > 
> > >          The queuepair should be divided by 2
> > 
> > this is just repeating what the patch does.
> > can you include more info pls?
> > 
> > Documentation/process/5.Posting.rst says among other things:
> > 
> > To that end, the summary line should describe the effects of and motivation
> > for the change as well as possible given the one-line constraint.  The
> > detailed description can then amplify on those topics and provide any
> > needed additional information.  If the patch fixes a bug, cite the commit
> > which introduced the bug if possible (and please provide both the commit ID
> > and the title when citing commits).  If a problem is associated with
> > specific log or compiler output, include that output to help others
> > searching for a solution to the same problem.  If the change is meant to
> > support other changes coming in later patch, say so.  If internal APIs are
> > changed, detail those changes and how other developers should respond.  In
> > general, the more you can put yourself into the shoes of everybody who will
> > be reading your changelog, the better that changelog (and the kernel as a
> > whole) will be.
> > 
> > 
> > > Signed-off-by: angus.chen <angus.chen@jaguarmicro.com>
> > Format should be
> > 
> > Angus Chen <angus.chen@jaguarmicro.com>
> > 
> > Also pls drop leading space.
> > 
> > 
> > > ---
> > > drivers/vdpa/ifcvf/ifcvf_base.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> > > index 75a703b803a2..3e4486bfa0b7 100644
> > > --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> > > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> > > @@ -323,7 +323,7 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
> > >          u32 q_pair_id;
> > > 
> > >          ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> > > -       q_pair_id = qid / hw->nr_vring;
> > > +       q_pair_id = qid / 2;
> Yes, this should be 2 and actually this cap never work as expected, we are
> re-designing this.

Do you ack this patch then?

> > >          avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
> > >          last_avail_idx = vp_ioread16(avail_idx_addr);
> > > 
> > > @@ -337,7 +337,7 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
> > >          u32 q_pair_id;
> > > 
> > >          ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> > > -       q_pair_id = qid / hw->nr_vring;
> > > +       q_pair_id = qid / 2;
> > >          avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
> > >          hw->vring[qid].last_avail_idx = num;
> > >          vp_iowrite16(num, avail_idx_addr);
> > 
> > Pls CC more widely.
> > $ ./scripts/get_maintainer.pl -f drivers/vdpa/ifcvf/ifcvf_base.c
> > "Michael S. Tsirkin" <mst@redhat.com> (maintainer:VIRTIO CORE AND NET DRIVERS,commit_signer:6/6=100%)
> > Jason Wang <jasowang@redhat.com> (maintainer:VIRTIO CORE AND NET DRIVERS,commit_signer:1/6=17%)
> > Zhu Lingshan <lingshan.zhu@intel.com> (commit_signer:5/6=83%,authored:5/6=83%,added_lines:102/102=100%,removed_lines:108/109=99%)
> > Stefano Garzarella <sgarzare@redhat.com> (commit_signer:1/6=17%)
> > Tom Rix <trix@redhat.com> (commit_signer:1/6=17%)
> > Colin Ian King <colin.king@intel.com> (authored:1/6=17%)
> > virtualization@lists.linux-foundation.org (open list:VIRTIO CORE AND NET DRIVERS)
> > 
> > Thanks!
> > 
> > BTW, Zhu Lingshan, would you like to be listed a patch reviewer in
> > MAINTAINERS so people rememeber to CC you? You are working a lot on this
> > driver.
> sure, please add me in the list.
> 
> Thanks
> Zhu Lingshan
> > 
> > 
> > 
> > > --

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] vDPA/ifcvf: fix the Calculation of queuepair

[Michael S. Tsirkin]

Thanks! Let's see if we get an ack.
Minor things for the future (in case a new version will be needed):


Subject should include the patch version. Also pls avoid
capital letters in the middle of the sentence. E.g.

[PATCH v3] vDPA/ifcvf: fix the calculation of queuepair


On Thu, Sep 22, 2022 at 07:00:17PM +0800, Angus Chen wrote:
> The queuepair should be divided by 2
> It should not be hw->nr_vring when multi-queue feature was enabled
> fix  commit 2ddae773c93b ("detect and use the onboard number ")

An extra " " here at the end.

It's also a good idea to add:

Fixes: 2ddae773c93b ("detect and use the onboard number")

> 
> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>


> ---

A good thing to add here is a changelog.
In this case the patch is unchanged, so you would write:

changes in v2:
- updated commit log with more info, addressing comments by MST
- no code changes


add more versions to this as you are revising the patch.


>  drivers/vdpa/ifcvf/ifcvf_base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 75a703b803a2..3e4486bfa0b7 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -323,7 +323,7 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
>  	u32 q_pair_id;
>  
>  	ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> -	q_pair_id = qid / hw->nr_vring;
> +	q_pair_id = qid / 2;
>  	avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
>  	last_avail_idx = vp_ioread16(avail_idx_addr);
>  
> @@ -337,7 +337,7 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
>  	u32 q_pair_id;
>  
>  	ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> -	q_pair_id = qid / hw->nr_vring;
> +	q_pair_id = qid / 2;
>  	avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
>  	hw->vring[qid].last_avail_idx = num;
>  	vp_iowrite16(num, avail_idx_addr);
> -- 
> 2.17.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] vDPA/ifcvf: fix the Calculation of queuepair

[Jason Wang]

On Thu, Sep 22, 2022 at 7:00 PM Angus Chen <angus.chen@jaguarmicro.com> wrote:
>
> The queuepair should be divided by 2
> It should not be hw->nr_vring when multi-queue feature was enabled
> fix  commit 2ddae773c93b ("detect and use the onboard number ")
>
> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>

Fixed tags should be placed above SOB. Please refer to how it is done
in other commits or Documentation/process/submitting-patches.rst.

Others look good.

Thanks

> ---
>  drivers/vdpa/ifcvf/ifcvf_base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 75a703b803a2..3e4486bfa0b7 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -323,7 +323,7 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
>         u32 q_pair_id;
>
>         ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> -       q_pair_id = qid / hw->nr_vring;
> +       q_pair_id = qid / 2;
>         avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
>         last_avail_idx = vp_ioread16(avail_idx_addr);
>
> @@ -337,7 +337,7 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
>         u32 q_pair_id;
>
>         ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> -       q_pair_id = qid / hw->nr_vring;
> +       q_pair_id = qid / 2;
>         avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
>         hw->vring[qid].last_avail_idx = num;
>         vp_iowrite16(num, avail_idx_addr);
> --
> 2.17.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] vDPA/ifcvf: fix the Calculation of queuepair

[Michael S. Tsirkin]

On Thu, Sep 22, 2022 at 06:31:32PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 9/22/2022 6:17 PM, Michael S. Tsirkin wrote:
> > On Thu, Sep 22, 2022 at 06:07:58PM +0800, Zhu, Lingshan wrote:
> > > 
> > > On 9/22/2022 5:06 PM, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 22, 2022 at 08:55:26AM +0000, Angus Chen wrote:
> > > > > >From 4f65eae86ab15d7abb8bd30401187cb195dfd27b Mon Sep 17 00:00:00 2001
> > > > > From: "angus.chen" <angus.chen@jaguarmicro.com>
> > > > > Date: Thu, 22 Sep 2022 14:47:28 +0800
> > > > > Subject: [PATCH]     vDPA/ifcvf: fix the Calculation of queuepair
> > > > > 
> > > > >           The queuepair should be divided by 2
> > > > this is just repeating what the patch does.
> > > > can you include more info pls?
> > > > 
> > > > Documentation/process/5.Posting.rst says among other things:
> > > > 
> > > > To that end, the summary line should describe the effects of and motivation
> > > > for the change as well as possible given the one-line constraint.  The
> > > > detailed description can then amplify on those topics and provide any
> > > > needed additional information.  If the patch fixes a bug, cite the commit
> > > > which introduced the bug if possible (and please provide both the commit ID
> > > > and the title when citing commits).  If a problem is associated with
> > > > specific log or compiler output, include that output to help others
> > > > searching for a solution to the same problem.  If the change is meant to
> > > > support other changes coming in later patch, say so.  If internal APIs are
> > > > changed, detail those changes and how other developers should respond.  In
> > > > general, the more you can put yourself into the shoes of everybody who will
> > > > be reading your changelog, the better that changelog (and the kernel as a
> > > > whole) will be.
> > > > 
> > > > 
> > > > > Signed-off-by: angus.chen <angus.chen@jaguarmicro.com>
> > > > Format should be
> > > > 
> > > > Angus Chen <angus.chen@jaguarmicro.com>
> > > > 
> > > > Also pls drop leading space.
> > > > 
> > > > 
> > > > > ---
> > > > > drivers/vdpa/ifcvf/ifcvf_base.c | 4 ++--
> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> > > > > index 75a703b803a2..3e4486bfa0b7 100644
> > > > > --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> > > > > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> > > > > @@ -323,7 +323,7 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
> > > > >           u32 q_pair_id;
> > > > > 
> > > > >           ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> > > > > -       q_pair_id = qid / hw->nr_vring;
> > > > > +       q_pair_id = qid / 2;
> > > Yes, this should be 2 and actually this cap never work as expected, we are
> > > re-designing this.
> > Do you ack this patch then?
> Yes,
> 
> Acked-by: Zhu Lingshan<lingshan.zhu@intel.com>


Pls note there should be a space before <>. E.g.

Acked-by: Zhu Lingshan <lingshan.zhu@intel.com>

Thanks!

> > 
> > > > >           avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
> > > > >           last_avail_idx = vp_ioread16(avail_idx_addr);
> > > > > 
> > > > > @@ -337,7 +337,7 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
> > > > >           u32 q_pair_id;
> > > > > 
> > > > >           ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
> > > > > -       q_pair_id = qid / hw->nr_vring;
> > > > > +       q_pair_id = qid / 2;
> > > > >           avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
> > > > >           hw->vring[qid].last_avail_idx = num;
> > > > >           vp_iowrite16(num, avail_idx_addr);
> > > > Pls CC more widely.
> > > > $ ./scripts/get_maintainer.pl -f drivers/vdpa/ifcvf/ifcvf_base.c
> > > > "Michael S. Tsirkin" <mst@redhat.com> (maintainer:VIRTIO CORE AND NET DRIVERS,commit_signer:6/6=100%)
> > > > Jason Wang <jasowang@redhat.com> (maintainer:VIRTIO CORE AND NET DRIVERS,commit_signer:1/6=17%)
> > > > Zhu Lingshan <lingshan.zhu@intel.com> (commit_signer:5/6=83%,authored:5/6=83%,added_lines:102/102=100%,removed_lines:108/109=99%)
> > > > Stefano Garzarella <sgarzare@redhat.com> (commit_signer:1/6=17%)
> > > > Tom Rix <trix@redhat.com> (commit_signer:1/6=17%)
> > > > Colin Ian King <colin.king@intel.com> (authored:1/6=17%)
> > > > virtualization@lists.linux-foundation.org (open list:VIRTIO CORE AND NET DRIVERS)
> > > > 
> > > > Thanks!
> > > > 
> > > > BTW, Zhu Lingshan, would you like to be listed a patch reviewer in
> > > > MAINTAINERS so people rememeber to CC you? You are working a lot on this
> > > > driver.
> > > sure, please add me in the list.
> > > 
> > > Thanks
> > > Zhu Lingshan
> > > > 
> > > > 
> > > > > --

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization