| Message ID | 20210105104326.67895-2-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Host information userspace version | expand |
On Tue, Jan 05, 2021 at 12:43:25PM +0200, Gal Pressman wrote: > Downstream patch will require the userspace version which is passed as > part of ucontext allocation. Move the host info set there and make sure > it's only called once (on the first allocation). > > Reviewed-by: Firas JahJah <firasj@amazon.com> > Reviewed-by: Leonid Feschuk <lfesch@amazon.com> > Signed-off-by: Gal Pressman <galpress@amazon.com> > --- > drivers/infiniband/hw/efa/efa.h | 7 +++++++ > drivers/infiniband/hw/efa/efa_main.c | 4 +--- > drivers/infiniband/hw/efa/efa_verbs.c | 3 +++ > 3 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h > index e5d9712e98c4..9c9cd5867489 100644 > --- a/drivers/infiniband/hw/efa/efa.h > +++ b/drivers/infiniband/hw/efa/efa.h > @@ -45,6 +45,11 @@ struct efa_stats { > atomic64_t keep_alive_rcvd; > }; > > +enum { > + EFA_FLAGS_HOST_INFO_SET_BIT, > + EFA_FLAGS_NUM, > +}; > + > struct efa_dev { > struct ib_device ibdev; > struct efa_com_dev edev; > @@ -62,6 +67,7 @@ struct efa_dev { > struct efa_irq admin_irq; > > struct efa_stats stats; > + DECLARE_BITMAP(flags, EFA_FLAGS_NUM); > }; Why do you need such over-engineering? What is wrong with old school "u8 flag"? Thanks
On 05/01/2021 13:21, Leon Romanovsky wrote: > On Tue, Jan 05, 2021 at 12:43:25PM +0200, Gal Pressman wrote: >> Downstream patch will require the userspace version which is passed as >> part of ucontext allocation. Move the host info set there and make sure >> it's only called once (on the first allocation). >> >> Reviewed-by: Firas JahJah <firasj@amazon.com> >> Reviewed-by: Leonid Feschuk <lfesch@amazon.com> >> Signed-off-by: Gal Pressman <galpress@amazon.com> >> --- >> drivers/infiniband/hw/efa/efa.h | 7 +++++++ >> drivers/infiniband/hw/efa/efa_main.c | 4 +--- >> drivers/infiniband/hw/efa/efa_verbs.c | 3 +++ >> 3 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h >> index e5d9712e98c4..9c9cd5867489 100644 >> --- a/drivers/infiniband/hw/efa/efa.h >> +++ b/drivers/infiniband/hw/efa/efa.h >> @@ -45,6 +45,11 @@ struct efa_stats { >> atomic64_t keep_alive_rcvd; >> }; >> >> +enum { >> + EFA_FLAGS_HOST_INFO_SET_BIT, >> + EFA_FLAGS_NUM, >> +}; >> + >> struct efa_dev { >> struct ib_device ibdev; >> struct efa_com_dev edev; >> @@ -62,6 +67,7 @@ struct efa_dev { >> struct efa_irq admin_irq; >> >> struct efa_stats stats; >> + DECLARE_BITMAP(flags, EFA_FLAGS_NUM); >> }; > > Why do you need such over-engineering? > What is wrong with old school "u8 flag"? The main reason is for the atomic test_and_set_bit() usage, otherwise it would be an atomic flag, not u8 flag.
On Tue, Jan 05, 2021 at 02:22:23PM +0200, Gal Pressman wrote: > On 05/01/2021 13:21, Leon Romanovsky wrote: > > On Tue, Jan 05, 2021 at 12:43:25PM +0200, Gal Pressman wrote: > >> Downstream patch will require the userspace version which is passed as > >> part of ucontext allocation. Move the host info set there and make sure > >> it's only called once (on the first allocation). > >> > >> Reviewed-by: Firas JahJah <firasj@amazon.com> > >> Reviewed-by: Leonid Feschuk <lfesch@amazon.com> > >> Signed-off-by: Gal Pressman <galpress@amazon.com> > >> --- > >> drivers/infiniband/hw/efa/efa.h | 7 +++++++ > >> drivers/infiniband/hw/efa/efa_main.c | 4 +--- > >> drivers/infiniband/hw/efa/efa_verbs.c | 3 +++ > >> 3 files changed, 11 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h > >> index e5d9712e98c4..9c9cd5867489 100644 > >> --- a/drivers/infiniband/hw/efa/efa.h > >> +++ b/drivers/infiniband/hw/efa/efa.h > >> @@ -45,6 +45,11 @@ struct efa_stats { > >> atomic64_t keep_alive_rcvd; > >> }; > >> > >> +enum { > >> + EFA_FLAGS_HOST_INFO_SET_BIT, > >> + EFA_FLAGS_NUM, > >> +}; > >> + > >> struct efa_dev { > >> struct ib_device ibdev; > >> struct efa_com_dev edev; > >> @@ -62,6 +67,7 @@ struct efa_dev { > >> struct efa_irq admin_irq; > >> > >> struct efa_stats stats; > >> + DECLARE_BITMAP(flags, EFA_FLAGS_NUM); > >> }; > > > > Why do you need such over-engineering? > > What is wrong with old school "u8 flag"? > > The main reason is for the atomic test_and_set_bit() usage, otherwise it would > be an atomic flag, not u8 flag. But efa_dev can be opened with different applications and they can have different user space versions, but you are setting this info for the first caller only. How will it help for the debug? Thanks
On 05/01/2021 14:40, Leon Romanovsky wrote: > On Tue, Jan 05, 2021 at 02:22:23PM +0200, Gal Pressman wrote: >> On 05/01/2021 13:21, Leon Romanovsky wrote: >>> On Tue, Jan 05, 2021 at 12:43:25PM +0200, Gal Pressman wrote: >>>> Downstream patch will require the userspace version which is passed as >>>> part of ucontext allocation. Move the host info set there and make sure >>>> it's only called once (on the first allocation). >>>> >>>> Reviewed-by: Firas JahJah <firasj@amazon.com> >>>> Reviewed-by: Leonid Feschuk <lfesch@amazon.com> >>>> Signed-off-by: Gal Pressman <galpress@amazon.com> >>>> --- >>>> drivers/infiniband/hw/efa/efa.h | 7 +++++++ >>>> drivers/infiniband/hw/efa/efa_main.c | 4 +--- >>>> drivers/infiniband/hw/efa/efa_verbs.c | 3 +++ >>>> 3 files changed, 11 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h >>>> index e5d9712e98c4..9c9cd5867489 100644 >>>> --- a/drivers/infiniband/hw/efa/efa.h >>>> +++ b/drivers/infiniband/hw/efa/efa.h >>>> @@ -45,6 +45,11 @@ struct efa_stats { >>>> atomic64_t keep_alive_rcvd; >>>> }; >>>> >>>> +enum { >>>> + EFA_FLAGS_HOST_INFO_SET_BIT, >>>> + EFA_FLAGS_NUM, >>>> +}; >>>> + >>>> struct efa_dev { >>>> struct ib_device ibdev; >>>> struct efa_com_dev edev; >>>> @@ -62,6 +67,7 @@ struct efa_dev { >>>> struct efa_irq admin_irq; >>>> >>>> struct efa_stats stats; >>>> + DECLARE_BITMAP(flags, EFA_FLAGS_NUM); >>>> }; >>> >>> Why do you need such over-engineering? >>> What is wrong with old school "u8 flag"? >> >> The main reason is for the atomic test_and_set_bit() usage, otherwise it would >> be an atomic flag, not u8 flag. > > But efa_dev can be opened with different applications and they can have > different user space versions, but you are setting this info for the > first caller only. How will it help for the debug? Right, it is possible for the userspace version to change between contexts, but that's a tradeoff we're willing to take for the meantime so we don't have to send a host info command for each ucontext allocation. We can change that in the future if that won't prove to be reliable enough.
diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h index e5d9712e98c4..9c9cd5867489 100644 --- a/drivers/infiniband/hw/efa/efa.h +++ b/drivers/infiniband/hw/efa/efa.h @@ -45,6 +45,11 @@ struct efa_stats { atomic64_t keep_alive_rcvd; }; +enum { + EFA_FLAGS_HOST_INFO_SET_BIT, + EFA_FLAGS_NUM, +}; + struct efa_dev { struct ib_device ibdev; struct efa_com_dev edev; @@ -62,6 +67,7 @@ struct efa_dev { struct efa_irq admin_irq; struct efa_stats stats; + DECLARE_BITMAP(flags, EFA_FLAGS_NUM); }; struct efa_ucontext { @@ -117,6 +123,7 @@ struct efa_ah { u8 id[EFA_GID_SIZE]; }; +void efa_set_host_info(struct efa_dev *dev); int efa_query_device(struct ib_device *ibdev, struct ib_device_attr *props, struct ib_udata *udata); diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c index 0f578734bddb..90a033a6af6c 100644 --- a/drivers/infiniband/hw/efa/efa_main.c +++ b/drivers/infiniband/hw/efa/efa_main.c @@ -191,7 +191,7 @@ static void efa_stats_init(struct efa_dev *dev) atomic64_set(s, 0); } -static void efa_set_host_info(struct efa_dev *dev) +void efa_set_host_info(struct efa_dev *dev) { struct efa_admin_set_feature_resp resp = {}; struct efa_admin_set_feature_cmd cmd = {}; @@ -301,8 +301,6 @@ static int efa_ib_device_add(struct efa_dev *dev) if (err) goto err_release_doorbell_bar; - efa_set_host_info(dev); - dev->ibdev.node_type = RDMA_NODE_UNSPECIFIED; dev->ibdev.phys_port_cnt = 1; dev->ibdev.num_comp_vectors = 1; diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c index 2fe5708b2d9d..5c12bdc28ef0 100644 --- a/drivers/infiniband/hw/efa/efa_verbs.c +++ b/drivers/infiniband/hw/efa/efa_verbs.c @@ -1695,6 +1695,9 @@ int efa_alloc_ucontext(struct ib_ucontext *ibucontext, struct ib_udata *udata) goto err_out; } + if (!test_and_set_bit(EFA_FLAGS_HOST_INFO_SET_BIT, dev->flags)) + efa_set_host_info(dev); + err = efa_user_comp_handshake(ibucontext, &cmd); if (err) goto err_out;