On Sun, Sep 03, 2023 at 06:30:58PM +0500, Roman Mamedov wrote: > On Sun, 3 Sep 2023 17:50:59 +0800 > Kuan-Wei Chiu <visitorckw@xxxxxxxxx> wrote: > > > Replace the conditional statements in the cmp_stripe() function with a > > branchless version to improve code readability and potentially enhance > > performance. > > The new code will always do two comparisons and a subtraction (3 > instructions in total), whereas the old version could return after just 1 > comparison, or after 2 comparisons. So depending on the data values it is 3x > to 1.5x as much operations performed than before, there unlikely to be any > enhancement of performance. > > Also IMO the previous version is more easily readable. > The reason behind my proposed changes was to eliminate conditional branches in the code. While the original code could occasionally achieve early returns, many compilers, such as x86-64 gcc 13.2 compiling with -O2 flag, still generate branch instructions. Processors typically have deep pipelines, and a branch prediction miss can result in a high penalty. Therefore, even though early return might not be possible, the new branchless version of code could still offer efficiency improvements. > > The new code calculates the result using a subtraction of > > comparison results, making it more concise and avoiding conditional > > branches. This change does not alter the functionality of the code. > > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@xxxxxxxxx> > > --- > > drivers/md/raid5.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > > index 4cb9c608ee19..b14d7ba38f0f 100644 > > --- a/drivers/md/raid5.c > > +++ b/drivers/md/raid5.c > > @@ -1035,11 +1035,7 @@ static int cmp_stripe(void *priv, const struct list_head *a, > > struct r5pending_data, sibling); > > const struct r5pending_data *db = list_entry(b, > > struct r5pending_data, sibling); > > - if (da->sector > db->sector) > > - return 1; > > - if (da->sector < db->sector) > > - return -1; > > - return 0; > > + return (da->sector > db->sector) - (da->sector < db->sector); > > } > > > > static void dispatch_defer_bios(struct r5conf *conf, int target, > > > -- > With respect, > Roman -- Best regards, Kuan-Wei Chiu
