| Message ID | 5697DE31.9040309@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
On Thu, Jan 14, 2016 at 06:43:13PM +0100, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Thu, 14 Jan 2016 17:47:59 +0100 > > The variable "status" will be set to an appropriate value a bit later. > Thus omit the explicit initialisation at the beginning. What did you try to achieve by this patch? > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/infiniband/hw/ocrdma/ocrdma_ah.c | 2 +- > drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 4 ++-- > drivers/infiniband/hw/ocrdma/ocrdma_stats.c | 4 ++-- > drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 12 ++++++------ > 4 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_ah.c b/drivers/infiniband/hw/ocrdma/ocrdma_ah.c > index a343e03..41f0171 100644 > --- a/drivers/infiniband/hw/ocrdma/ocrdma_ah.c > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_ah.c > @@ -59,7 +59,7 @@ static inline int set_av_attr(struct ocrdma_dev *dev, struct ocrdma_ah *ah, > struct ib_ah_attr *attr, union ib_gid *sgid, > int pdid, bool *isvlan, u16 vlan_tag) > { > - int status = 0; > + int status; > struct ocrdma_eth_vlan eth; > struct ocrdma_grh grh; > int eth_sz; > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c > index 283ca84..159b1d5 100644 > --- a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c > @@ -1113,7 +1113,7 @@ mbx_err: > static int ocrdma_nonemb_mbx_cmd(struct ocrdma_dev *dev, struct ocrdma_mqe *mqe, > void *payload_va) > { > - int status = 0; > + int status; > struct ocrdma_mbx_rsp *rsp = payload_va; > > if ((mqe->hdr.spcl_sge_cnt_emb & OCRDMA_MQE_HDR_EMB_MASK) >> > @@ -2871,7 +2871,7 @@ int ocrdma_mbx_destroy_srq(struct ocrdma_dev *dev, struct ocrdma_srq *srq) > static int ocrdma_mbx_get_dcbx_config(struct ocrdma_dev *dev, u32 ptype, > struct ocrdma_dcbx_cfg *dcbxcfg) > { > - int status = 0; > + int status; > dma_addr_t pa; > struct ocrdma_mqe cmd; > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > index 86c303a..119baa3 100644 > --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c > @@ -608,7 +608,7 @@ static char *ocrdma_driver_dbg_stats(struct ocrdma_dev *dev) > static void ocrdma_update_stats(struct ocrdma_dev *dev) > { > ulong now = jiffies, secs; > - int status = 0; > + int status; > struct ocrdma_rdma_stats_resp *rdma_stats = > (struct ocrdma_rdma_stats_resp *)dev->stats_mem.va; > struct ocrdma_rsrc_stats *rsrc_stats = &rdma_stats->act_rsrc_stats; > @@ -639,7 +639,7 @@ static ssize_t ocrdma_dbgfs_ops_write(struct file *filp, > { > char tmp_str[32]; > long reset; > - int status = 0; > + int status; > struct ocrdma_stats *pstats = filp->private_data; > struct ocrdma_dev *dev = pstats->dev; > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c > index 4caf167..1d90d18 100644 > --- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c > @@ -419,7 +419,7 @@ static struct ocrdma_pd *_ocrdma_alloc_pd(struct ocrdma_dev *dev, > struct ib_udata *udata) > { > struct ocrdma_pd *pd = NULL; > - int status = 0; > + int status; > > pd = kzalloc(sizeof(*pd), GFP_KERNEL); > if (!pd) > @@ -468,7 +468,7 @@ static inline int is_ucontext_pd(struct ocrdma_ucontext *uctx, > static int _ocrdma_dealloc_pd(struct ocrdma_dev *dev, > struct ocrdma_pd *pd) > { > - int status = 0; > + int status; > > if (dev->pd_mgr->pd_prealloc_valid) > status = ocrdma_put_pd_num(dev, pd->id, pd->dpp_enabled); > @@ -593,7 +593,7 @@ map_err: > > int ocrdma_dealloc_ucontext(struct ib_ucontext *ibctx) > { > - int status = 0; > + int status; > struct ocrdma_mm *mm, *tmp; > struct ocrdma_ucontext *uctx = get_ocrdma_ucontext(ibctx); > struct ocrdma_dev *dev = get_ocrdma_dev(ibctx->device); > @@ -620,7 +620,7 @@ int ocrdma_mmap(struct ib_ucontext *context, struct vm_area_struct *vma) > unsigned long vm_page = vma->vm_pgoff << PAGE_SHIFT; > u64 unmapped_db = (u64) dev->nic_info.unmapped_db; > unsigned long len = (vma->vm_end - vma->vm_start); > - int status = 0; > + int status; > bool found; > > if (vma->vm_start & (PAGE_SIZE - 1)) > @@ -1283,7 +1283,7 @@ static int ocrdma_copy_qp_uresp(struct ocrdma_qp *qp, > struct ib_udata *udata, int dpp_offset, > int dpp_credit_lmt, int srq) > { > - int status = 0; > + int status; > u64 usr_db; > struct ocrdma_create_qp_uresp uresp; > struct ocrdma_pd *pd = qp->pd; > @@ -1947,7 +1947,7 @@ int ocrdma_modify_srq(struct ib_srq *ibsrq, > enum ib_srq_attr_mask srq_attr_mask, > struct ib_udata *udata) > { > - int status = 0; > + int status; > struct ocrdma_srq *srq; > > srq = get_ocrdma_srq(ibsrq); > -- > 2.6.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> The variable "status" will be set to an appropriate value a bit later. >> Thus omit the explicit initialisation at the beginning. > > What did you try to achieve by this patch? I would like to optimise the affected source files a bit. Would you like to clarify any measurable effects around the implementation detail when various variables will only be initialised immediately before they will be read again? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 15, 2016 at 03:50:27PM +0100, SF Markus Elfring wrote: > >> The variable "status" will be set to an appropriate value a bit later. > >> Thus omit the explicit initialisation at the beginning. > > > > What did you try to achieve by this patch? > > I would like to optimise the affected source files a bit. > Would you like to clarify any measurable effects around the implementation > detail when various variables will only be initialised immediately > before they will be read again? Compiler will drop this variable initialization by itself because there are no reads between this variable initialization and write. I recommend you to take a look on the assembly code and ensure it by yourself. The proposed change won't affect performance at all. > > Regards, > Markus -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Would you like to clarify any measurable effects around the implementation >> detail when various variables will only be initialised immediately >> before they will be read again? > > Compiler will drop this variable initialization by itself because > there are no reads between this variable initialization and write. Which compiler variants would you to take into account for such an use case? > I recommend you to take a look on the assembly code and ensure it > by yourself. Will any configuration parameters and command arguments become relevant to improve also a corresponding software comparison? > The proposed change won't affect performance at all. Will unneeded variable assignments be really optimised away by default? By the way: Will a small source code reduction matter also a bit here? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 15, 2016 at 04:26:36PM +0100, SF Markus Elfring wrote: > >> Would you like to clarify any measurable effects around the implementation > >> detail when various variables will only be initialised immediately > >> before they will be read again? > > > > Compiler will drop this variable initialization by itself because > > there are no reads between this variable initialization and write. > > Which compiler variants would you to take into account for such an use case? GCC supported it before 1999 when I saw it first time. My assumption that in 2016 all compilers are doing such optimization now. I would be glad to hear an example of modern compiler which doesn't support this simple optimization. > > > > I recommend you to take a look on the assembly code and ensure it > > by yourself. > > Will any configuration parameters and command arguments become relevant > to improve also a corresponding software comparison? Please suggest us, you are proposing this change, and not me. > > > > The proposed change won't affect performance at all. > > Will unneeded variable assignments be really optimised away by default? Yes > > > By the way: > Will a small source code reduction matter also a bit here? If you are interested in saving space of one latter, you need to take into account git database increase, do you? > > Regards, > Markus -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Doing bogus initializations turns off GCC's checking for uninitialized variables so it's a bad habbit. On the other hand, GCC's checking is not perfect and it sometimes misses bugs so these patches have to be reviewed manually which is maybe too much work to be worthwhile. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> GCC supported it before 1999 when I saw it first time. My assumption > that in 2016 all compilers are doing such optimization now. Interesting … > I would be glad to hear an example of modern compiler which doesn't > support this simple optimization. Would you like to take into account any other source code analysis approaches? >> Will any configuration parameters and command arguments become relevant >> to improve also a corresponding software comparison? > > Please suggest us, you are proposing this change, and not me. Which combination of hardware and software versions would you find representative for a corresponding system check? >>> The proposed change won't affect performance at all. >> >> Will unneeded variable assignments be really optimised away by default? > > Yes Can it be that this result will depend on special parameters so that data flow analysis and optimisation will be performed in the way you seem to expect? > If you are interested in saving space of one latter, you need to take into > account git database increase, do you? There are also other aspects to consider: * Do you insist to initialise a return code at the beginning of every function with a non-void return type? * Does each bit of extra information can result also in unwanted consequences? * Is this a specific source code review concern? * Can this software be improved a bit more only if we dare to talk about potential update candidates? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 15, 2016 at 05:24:50PM +0100, SF Markus Elfring wrote: Since, you didn't answer to my original question, I will repeat it again. [Q.] What did you try to achieve by this patch? P.S. This is mailing list for developers and not for patch bots. We are glad to see patches that clean the code, but they need to be meaningful. Your automated patches add noise without any real benefit. You was suggested to be ignored in MTD mailing list exactly for this type of patches, did you learn anything from that experience? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/15/2016 09:00 AM, Leon Romanovsky wrote: > On Fri, Jan 15, 2016 at 05:24:50PM +0100, SF Markus Elfring wrote: > > Since, you didn't answer to my original question, I will repeat it again. > [Q.] What did you try to achieve by this patch? Hello Leon, Have you noticed Dan's reply: "Doing bogus initializations turns off GCC's checking for uninitialized variables so it's a bad habit." Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 15, 2016 at 09:19:38AM -0800, Bart Van Assche wrote: > On 01/15/2016 09:00 AM, Leon Romanovsky wrote: > >On Fri, Jan 15, 2016 at 05:24:50PM +0100, SF Markus Elfring wrote: > > > >Since, you didn't answer to my original question, I will repeat it again. > >[Q.] What did you try to achieve by this patch? > > Hello Leon, > > Have you noticed Dan's reply: "Doing bogus initializations turns off GCC's > checking for uninitialized variables so it's a bad habit." Yes and his second part of that message too, that uninitialized checks in GCC work as not as expected [1, 2]. Stackoverflow site has a lot examples of these types of bugs [3]. These examples together with Dan's suggestion requires from all reviewers to be extra cautions when removing variable initialization. [1] https://gcc.gnu.org/wiki/Better_Uninitialized_Warnings [2] https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=may%20be%20uninitialized [3] http://stackoverflow.com/questions/27063678/compiler-not-detecting-obviously-uninitialized-variable > > Thanks, > > Bart. > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> [Q.] What did you try to achieve by this patch? I would appreciate a bit more fine-tuning in the affected source files. > P.S. This is mailing list for developers Do you try to express any further restrictions? > and not for patch bots. Would you like to explain such an information a bit more? > We are glad to see patches that clean the code, but they need to > be meaningful. This is usual. > Your automated patches add noise without any real benefit. Are you expecting a kind of special proof? > You was suggested to be ignored in MTD mailing list exactly > for this type of patches, Will the acceptance increase a bit for similar issues over time? > did you learn anything from that experience? Will another acknowledgement by Selvin Xavier influence any corresponding software improvements? How do you think about to add any further constructive comments also for the other proposed update steps? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 15, 2016 at 07:19:31PM +0100, SF Markus Elfring wrote: > > [Q.] What did you try to achieve by this patch? > > I would appreciate a bit more fine-tuning in the affected source files. Please provide the numbers BEFORE and AFTER your change which can support that your so called "fine-tuning" worked. We are waiting to see it together with Tested-By tag to emphasize that your code was tested on real HW and passed minimal sanity checks. NAK on this patch. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please provide the numbers BEFORE and AFTER your change which can > support that your so called "fine-tuning" worked. For which combinations of hardware and software versions would you like to see corresponding results from detailed system checks and special benchmarks? > We are waiting to see it together with Tested-By tag to emphasize > that your code was tested on real HW and passed minimal sanity checks. Are any other contributors interested to collaborate for such a task? > NAK on this patch. Thanks for your feedback. Does it mean that you reject (only) the proposed source code adjustments around the variable "status" in the shown function selection at the moment? Would you like to clarify the other update steps from this patch series a bit more? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_ah.c b/drivers/infiniband/hw/ocrdma/ocrdma_ah.c index a343e03..41f0171 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_ah.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_ah.c @@ -59,7 +59,7 @@ static inline int set_av_attr(struct ocrdma_dev *dev, struct ocrdma_ah *ah, struct ib_ah_attr *attr, union ib_gid *sgid, int pdid, bool *isvlan, u16 vlan_tag) { - int status = 0; + int status; struct ocrdma_eth_vlan eth; struct ocrdma_grh grh; int eth_sz; diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c index 283ca84..159b1d5 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c @@ -1113,7 +1113,7 @@ mbx_err: static int ocrdma_nonemb_mbx_cmd(struct ocrdma_dev *dev, struct ocrdma_mqe *mqe, void *payload_va) { - int status = 0; + int status; struct ocrdma_mbx_rsp *rsp = payload_va; if ((mqe->hdr.spcl_sge_cnt_emb & OCRDMA_MQE_HDR_EMB_MASK) >> @@ -2871,7 +2871,7 @@ int ocrdma_mbx_destroy_srq(struct ocrdma_dev *dev, struct ocrdma_srq *srq) static int ocrdma_mbx_get_dcbx_config(struct ocrdma_dev *dev, u32 ptype, struct ocrdma_dcbx_cfg *dcbxcfg) { - int status = 0; + int status; dma_addr_t pa; struct ocrdma_mqe cmd; diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c index 86c303a..119baa3 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_stats.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_stats.c @@ -608,7 +608,7 @@ static char *ocrdma_driver_dbg_stats(struct ocrdma_dev *dev) static void ocrdma_update_stats(struct ocrdma_dev *dev) { ulong now = jiffies, secs; - int status = 0; + int status; struct ocrdma_rdma_stats_resp *rdma_stats = (struct ocrdma_rdma_stats_resp *)dev->stats_mem.va; struct ocrdma_rsrc_stats *rsrc_stats = &rdma_stats->act_rsrc_stats; @@ -639,7 +639,7 @@ static ssize_t ocrdma_dbgfs_ops_write(struct file *filp, { char tmp_str[32]; long reset; - int status = 0; + int status; struct ocrdma_stats *pstats = filp->private_data; struct ocrdma_dev *dev = pstats->dev; diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c index 4caf167..1d90d18 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c @@ -419,7 +419,7 @@ static struct ocrdma_pd *_ocrdma_alloc_pd(struct ocrdma_dev *dev, struct ib_udata *udata) { struct ocrdma_pd *pd = NULL; - int status = 0; + int status; pd = kzalloc(sizeof(*pd), GFP_KERNEL); if (!pd) @@ -468,7 +468,7 @@ static inline int is_ucontext_pd(struct ocrdma_ucontext *uctx, static int _ocrdma_dealloc_pd(struct ocrdma_dev *dev, struct ocrdma_pd *pd) { - int status = 0; + int status; if (dev->pd_mgr->pd_prealloc_valid) status = ocrdma_put_pd_num(dev, pd->id, pd->dpp_enabled); @@ -593,7 +593,7 @@ map_err: int ocrdma_dealloc_ucontext(struct ib_ucontext *ibctx) { - int status = 0; + int status; struct ocrdma_mm *mm, *tmp; struct ocrdma_ucontext *uctx = get_ocrdma_ucontext(ibctx); struct ocrdma_dev *dev = get_ocrdma_dev(ibctx->device); @@ -620,7 +620,7 @@ int ocrdma_mmap(struct ib_ucontext *context, struct vm_area_struct *vma) unsigned long vm_page = vma->vm_pgoff << PAGE_SHIFT; u64 unmapped_db = (u64) dev->nic_info.unmapped_db; unsigned long len = (vma->vm_end - vma->vm_start); - int status = 0; + int status; bool found; if (vma->vm_start & (PAGE_SIZE - 1)) @@ -1283,7 +1283,7 @@ static int ocrdma_copy_qp_uresp(struct ocrdma_qp *qp, struct ib_udata *udata, int dpp_offset, int dpp_credit_lmt, int srq) { - int status = 0; + int status; u64 usr_db; struct ocrdma_create_qp_uresp uresp; struct ocrdma_pd *pd = qp->pd; @@ -1947,7 +1947,7 @@ int ocrdma_modify_srq(struct ib_srq *ibsrq, enum ib_srq_attr_mask srq_attr_mask, struct ib_udata *udata) { - int status = 0; + int status; struct ocrdma_srq *srq; srq = get_ocrdma_srq(ibsrq);