Re: [PATCH V2 08/10] i40iw: Control debug error prints using env variable

[Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3294 bytes --]

On Thu, Dec 15, 2016 at 07:55:18PM +0000, Nikolova, Tatyana E wrote:
> Hi Leon,
>
> Please see inline.
>
> > -----Original Message-----
> > From: Leon Romanovsky [mailto:leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org]
> > Sent: Thursday, December 15, 2016 1:18 PM
> > To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > Cc: Nikolova, Tatyana E <tatyana.e.nikolova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>;
> > dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; e1000-
> > rdma-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> > Subject: Re: [PATCH V2 08/10] i40iw: Control debug error prints using env
> > variable
> >
> > On Thu, Dec 15, 2016 at 11:50:34AM -0700, Jason Gunthorpe wrote:
> > > On Thu, Dec 15, 2016 at 08:35:37PM +0200, Leon Romanovsky wrote:
> > >
> > > > I had in mind much simplier infrastucture, just add pr_debug(..)
> > > > call and allow every provider to place in any place in their code.
> > >
> > > I think the bitmask thing has to be hoisted too.
> > >
> > > > My main point is that I want to see all ENV variables in one place.
> > >
> > > Why?
> >
> > There are two reasons:
> > 1. Easy to document and for curious users to spot all possible variables.
>
> [Tatyana Nikolova] Thank you for providing an example for the environment variables. IMO the malloc implementation brings unnecessary overhead in terms of memory and code.

It is in init phase and it will take the same amount of space as other
global varibles now. i40w will have one ENV entry now -> it will have
one field in that struct -> malloc will be the same.

>
> A text file containing all environment variables and updated every time a new environment variable is added sounds like a good idea to keep them in one place.

If it will be one place and won't requre from us to remind and remember
to update the same information in multiple places, I'm ok.

>
> > 2. It helps to potential authors to reuse variables instead of inventing their
> > own.
>
> [Tatyana Nikolova]
>
> Jason gave an example with " VERBS_PROVIDER_DEBUG=qp,ah,blah". Should all of the environment variables, enabling other features in rdma-core use a similar approach?

I think that Jason is over designing it, but it is better to ask him
about it.

>
> Current environment variables are provider specific. How are they going to be reused without renaming? Renaming may create issues with applications and scripts already using the existing ones.

#define NEW_VARIABLE OLD_VARIABLE

>
> > >
> > > I really don't want to see util/ files include provider headers, for
> > > instance, so I don't like your SET() macro idea..
> > >
> > > At this point I'd settle for having all ENV vars *documented* in one
> > > place :(
> >
> > See, reason #1 :)
> >
> > >
> > > Jason
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> > > in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]