Message-ID: <20200618200010.GA1080112@localhost.localdomain>
Date:   Thu, 18 Jun 2020 20:00:13 +0000
From:   Niklas Cassel <Niklas.Cassel@....com>
To:     Chaitanya Kulkarni <Chaitanya.Kulkarni@....com>
CC:     Christoph Hellwig <hch@....de>, Sagi Grimberg <sagi@...mberg.me>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed
 fields in initializers

On Thu, Jun 18, 2020 at 05:29:00PM +0000, Chaitanya Kulkarni wrote:
> I'm  not against the code cleanup and it always welcome.
> Please also have a look at other comment.
> 
> >> What is the issue with existing code that we need this patch for ?
> >>
> > 
> > Hello Chaitanya,
> > 
> > This is just a cleanup patch, no functional change intended.
> > 
> I can see that.
> > It simply performs the initialization at declaration time, which is how we
> > usually initialize a subset of all fields.
> Absolutely not true in case nvme subsystem.
> > 
> > This is also how it was originally done, but this was changed to a
> > non-standard way in order to workaround a compiler bug.
> > 
> and the existing code matches the pattern present in the repo no need to 
> change if it is not causing any problem.
> > Since the compiler bug is no longer relevant, we can go back to the
> > standard way of doing things.
> Is there any problem with the code now which mandates this change ? 
> which I don't see any.
> > 
> > Performing initialization in a uniform way makes it easier to read and
> > comprehend the code, especially for people unfamiliar with it, since
> > it follows the same pattern used in other places of the kernel.
> > 
> This is absolutely not uniform way infact what you are proposing is true 
> then we need to change all existing function which does not follow this 
> pattern, have a look at the following list.
> 
> In NVMe subsystem case there are more than 10 functions which are on the 
> similar line of this function and doesn't initialize the variable at the 
> time declaration.
> 
> Please have a look the code :-

> nvmf_reg_read32
> nvmf_reg_read64
> nvmf_reg_write32
> nvmf_connect_admin_queue
> nvmf_connect_io_queue
> nvme_features
> nvme_toggle_streams
> nvme_get_stream_params
> nvme_user_cmd
> nvme_user_cmd64

These do not use an initializer at all, these use memset.
So at declaration time, these are not initialized at all.

Basically like:

int a;
a = 2;

I don't have any problem with the memset code pattern per se,
it is very common. Although it is weird that the nvme code
sometimes declares a "struct nvme_command c" on the stack,
and then memsets it, and sometimes uses an initializer.

Perhaps we should create a patch to unify this.
IMHO, using an initializer is more clear.
memset has to be used if the function needs to reset an
existing struct, but in none of the functions above are
we given a nvme_command that we need to reset. In each case
we declare a new nvme_command on the stack (so an initializer
makes more sense).

> nvme_identify_ctrl
> nvme_identify_ns_descs
> nvme_identify_ns_list
> nvme_identify_ns
> nvme_get_log

These used an initializer, and then also later did explict assignments,
e.g.:
struct nvme_command c = { 0 };
...
c.identify.cns = NVME_ID_CNS_CTRL;

which is basically the same as doing:

int a = 0;
a = 2;


However, I have fixed these functions in patch 1/2 in this series,
so that the values are set in the initializer, and then there is
not need for any further assignments.

Basically:
int a = 2;

> 
> Last two are an exception of copy_from_user() call before initialization 
> case, but we can do copy from user from caller and pass that as an 
> argument if we really want to follow the declare-init pattern.
> 
> In several places we don't follow this pattern when function is compact 
> and it looks ugly for larger structures such as this example. this is
> exactly the reason {} used in nvme subsystem.

If this pattern of using an initializer and then doing an assignment
is the desired pattern, then you should at least remove the
/* gcc-4.4.4 (at least) has issues with initializers and anon unions */
comments from drivers/nvme/host/core.c, because then that comment is
not true, because then you want this design pattern because you like it,
not because of a gcc bug.

> 
> > Just reading e.g. struct rdma_conn_param  param = { }; one assumes that
> > all fields will be zero initialized.. reading futher down in the function
> 
> No this is standard style and used in nvme/host/core.c everywhere 
> nothing wrong with this at all, please have a look at the author.

I think that the standard style in the nvme code is to assign the
values inside the initializer when assigning values to a struct
(except when using memset), see e.g.:

nvmet_tcp_try_recv_ddgst
nvmet_tcp_try_recv_pdu
nvmet_try_send_ddgst
nvmet_rdma_init_srq
nvmet_fc_add_port
nvme_fc_parse_traddr
nvmet_copy_ns_identifier
nvme_tcp_try_send_ddgst
nvme_rdma_inv_rkey
nvme_setup_irqs
nvme_fc_create_ctrl
nvme_fc_parse_traddr
nvme_fc_signal_discovery_scan
nvme_aen_uevent

And then there are the many many
static const struct's that are assigned values using initializers.
(Which I didn't count.)


I made a small mistake in v1, so I'll send out a v2,
feel free to continue the discussion there :)


Kind regards,
Niklas

Powered by blists - more mailing lists