[PATCH] Adding sysctl for errors statistics to ahd(4)

[Attilio Rao-2]

This patch introduces some mechanisms for collecting informations on
errors frequency and debugging (and relative sysctls for printing them
out) for the ahd(4) driver:
http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current2.diff

The usage of array for sysctls is a bit too paranoid but it allows for
further extendibility of the code and doesn't loose of cleaness.
This code has been contributed back by Sandvine Incorporated with some cleanups.
Please review.

Thanks,
Attilio


--
Peace can only be achieved by understanding - A. Einstein
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[Justin T. Gibbs-2]

On 11/6/2009 7:43 AM, Attilio Rao wrote:
>  This patch introduces some mechanisms for collecting informations on
>  errors frequency and debugging (and relative sysctls for printing them
>  out) for the ahd(4) driver:
>  http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current2.diff
>
>  The usage of array for sysctls is a bit too paranoid but it allows for
>  further extendibility of the code and doesn't loose of cleaness.
>  This code has been contributed back by Sandvine Incorporated with some
cleanups.
>  Please review.
>
>  Thanks,
>  Attilio

In general, I think the patch is fine.  It violates the existing style
of the driver in some places (e.g. the aic7xxx drivers wrap function
arguments to the opening '(' not to a 4 space indent), which should
probably be addressed so the code remains consistent.

My only real complaint is that there is not some generic reporting
mechanism for these types of statistics.  The ahd chips are legacy.
Will Sandvine or vendor X add similar but slightly different sysctls
to the next driver they need to integrate into their product?  Higher
level monitoring software can't be written to be agnostic of the storage
controller and thus survive largely untouched as the HW under the software
changes.  I don't expect Sandvine to address this, but the project should.
In the mean time, I'm okay with the patch going in since it is obviously
useful to at least one company.

--
Justin

_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[Attilio Rao-2]

2009/11/11 Justin T. Gibbs <gibbs@...>:

> On 11/6/2009 7:43 AM, Attilio Rao wrote:
>> This patch introduces some mechanisms for collecting informations on
>> errors frequency and debugging (and relative sysctls for printing them
>> out) for the ahd(4) driver:
>> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current2.diff
>>
>> The usage of array for sysctls is a bit too paranoid but it allows for
>> further extendibility of the code and doesn't loose of cleaness.
>> This code has been contributed back by Sandvine Incorporated with some
>> cleanups.
>> Please review.
>>
>> Thanks,
>> Attilio
>
> In general, I think the patch is fine.  It violates the existing style
> of the driver in some places (e.g. the aic7xxx drivers wrap function
> arguments to the opening '(' not to a 4 space indent), which should
> probably be addressed so the code remains consistent.

Sorry, I cannot find where these existing style breakage happens,
could you be a bit more precise?
(I just found an un-sorted header introduction in aic79xx.h that I will fix).

Anyways, your idea about a generalized interface in newbus for
handling that is not bad and as long as I'm planning some works in
this area I can add this item to the TODO list.

Thanks,
Attilio


--
Peace can only be achieved by understanding - A. Einstein
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[Scott Long-2]

I don't think that there was a specific mention of newbus.  I actually  
sees storage stats being the domain of either CAM or GEOM, and far too  
specific for newbus.

Scott

Sent from my iPhone

On Nov 11, 2009, at 6:46 PM, Attilio Rao <attilio@...> wrote:

> 2009/11/11 Justin T. Gibbs <gibbs@...>:
>> On 11/6/2009 7:43 AM, Attilio Rao wrote:
>>> This patch introduces some mechanisms for collecting informations on
>>> errors frequency and debugging (and relative sysctls for printing  
>>> them
>>> out) for the ahd(4) driver:
>>> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current2.diff
>>>
>>> The usage of array for sysctls is a bit too paranoid but it allows  
>>> for
>>> further extendibility of the code and doesn't loose of cleaness.
>>> This code has been contributed back by Sandvine Incorporated with  
>>> some
>>> cleanups.
>>> Please review.
>>>
>>> Thanks,
>>> Attilio
>>
>> In general, I think the patch is fine.  It violates the existing  
>> style
>> of the driver in some places (e.g. the aic7xxx drivers wrap function
>> arguments to the opening '(' not to a 4 space indent), which should
>> probably be addressed so the code remains consistent.
>
> Sorry, I cannot find where these existing style breakage happens,
> could you be a bit more precise?
> (I just found an un-sorted header introduction in aic79xx.h that I  
> will fix).
>
> Anyways, your idea about a generalized interface in newbus for
> handling that is not bad and as long as I'm planning some works in
> this area I can add this item to the TODO list.
>
> Thanks,
> Attilio
>
>
> --
> Peace can only be achieved by understanding - A. Einstein

_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[Justin T. Gibbs-2]

On 11/11/2009 6:46 PM, Attilio Rao wrote:

> 2009/11/11 Justin T. Gibbs<gibbs@...>:
>> On 11/6/2009 7:43 AM, Attilio Rao wrote:
>>> This patch introduces some mechanisms for collecting informations on
>>> errors frequency and debugging (and relative sysctls for printing them
>>> out) for the ahd(4) driver:
>>> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current2.diff
>>>
>>> The usage of array for sysctls is a bit too paranoid but it allows for
>>> further extendibility of the code and doesn't loose of cleaness.
>>> This code has been contributed back by Sandvine Incorporated with some
>>> cleanups.
>>> Please review.
>>>
>>> Thanks,
>>> Attilio
>>
>> In general, I think the patch is fine.  It violates the existing style
>> of the driver in some places (e.g. the aic7xxx drivers wrap function
>> arguments to the opening '(' not to a 4 space indent), which should
>> probably be addressed so the code remains consistent.
>
> Sorry, I cannot find where these existing style breakage happens,
> could you be a bit more precise?
> (I just found an un-sorted header introduction in aic79xx.h that I will fix).

For example this code:

+   ahd->sysctl_tree[AHD_SYSCTL_ROOT] = SYSCTL_ADD_NODE(
+       &ahd->sysctl_ctx[AHD_SYSCTL_ROOT], SYSCTL_STATIC_CHILDREN(_hw),
+       OID_AUTO, device_get_nameunit(ahd->dev_softc), CTLFLAG_RD,
+       0, ahd_sysctl_node_descriptions[AHD_SYSCTL_ROOT]);
+   SYSCTL_ADD_PROC(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT],
+       SYSCTL_CHILDREN(ahd->sysctl_tree[AHD_SYSCTL_ROOT]), OID_AUTO,
+       "clear", CTLTYPE_UINT | CTLFLAG_RW, ahd, 0, ahd_clear_allcounters,
+       "IU", "Clear all counters");

should look like this, not style(9), to match the existing style of the
driver:

    ahd->sysctl_tree[AHD_SYSCTL_ROOT] =
        SYSCTL_ADD_NODE(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT],
                        SYSCTL_STATIC_CHILDREN(_hw), OID_AUTO,
                        device_get_nameunit(ahd->dev_softc), CTLFLAG_RD,
                        0, ahd_sysctl_node_descriptions[AHD_SYSCTL_ROOT]);

    SYSCTL_ADD_PROC(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT],
                    SYSCTL_CHILDREN(ahd->sysctl_tree[AHD_SYSCTL_ROOT]),
                    OID_AUTO, "clear", CTLTYPE_UINT|CTLFLAG_RW, ahd, 0,
                    ahd_clear_allcounters, "IU", "Clear all counters");

--
Justin
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[Attilio Rao-2]

2009/11/12 Justin T. Gibbs <gibbs@...>:

> On 11/11/2009 6:46 PM, Attilio Rao wrote:
>> 2009/11/11 Justin T. Gibbs<gibbs@...>:
>>> On 11/6/2009 7:43 AM, Attilio Rao wrote:
>>>> This patch introduces some mechanisms for collecting informations on
>>>> errors frequency and debugging (and relative sysctls for printing them
>>>> out) for the ahd(4) driver:
>>>> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current2.diff
>>>>
>>>> The usage of array for sysctls is a bit too paranoid but it allows for
>>>> further extendibility of the code and doesn't loose of cleaness.
>>>> This code has been contributed back by Sandvine Incorporated with some
>>>> cleanups.
>>>> Please review.
>>>>
>>>> Thanks,
>>>> Attilio
>>>
>>> In general, I think the patch is fine.  It violates the existing style
>>> of the driver in some places (e.g. the aic7xxx drivers wrap function
>>> arguments to the opening '(' not to a 4 space indent), which should
>>> probably be addressed so the code remains consistent.
>>
>> Sorry, I cannot find where these existing style breakage happens,
>> could you be a bit more precise?
>> (I just found an un-sorted header introduction in aic79xx.h that I will fix).
>
>
> For example this code:
>
> +   ahd->sysctl_tree[AHD_SYSCTL_ROOT] = SYSCTL_ADD_NODE(
> +       &ahd->sysctl_ctx[AHD_SYSCTL_ROOT], SYSCTL_STATIC_CHILDREN(_hw),
> +       OID_AUTO, device_get_nameunit(ahd->dev_softc), CTLFLAG_RD,
> +       0, ahd_sysctl_node_descriptions[AHD_SYSCTL_ROOT]);
> +   SYSCTL_ADD_PROC(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT],
> +       SYSCTL_CHILDREN(ahd->sysctl_tree[AHD_SYSCTL_ROOT]), OID_AUTO,
> +       "clear", CTLTYPE_UINT | CTLFLAG_RW, ahd, 0, ahd_clear_allcounters,
> +       "IU", "Clear all counters");
>
> should look like this, not style(9), to match the existing style of the
> driver:
>
>    ahd->sysctl_tree[AHD_SYSCTL_ROOT] =
>        SYSCTL_ADD_NODE(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT],
>                        SYSCTL_STATIC_CHILDREN(_hw), OID_AUTO,
>                        device_get_nameunit(ahd->dev_softc), CTLFLAG_RD,
>                        0, ahd_sysctl_node_descriptions[AHD_SYSCTL_ROOT]);
>
>    SYSCTL_ADD_PROC(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT],
>                    SYSCTL_CHILDREN(ahd->sysctl_tree[AHD_SYSCTL_ROOT]),
>                    OID_AUTO, "clear", CTLTYPE_UINT|CTLFLAG_RW, ahd, 0,
>                    ahd_clear_allcounters, "IU", "Clear all counters");
>

Could you check if this revision of the patch is adeguate?:
http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current3.diff

Thanks,
Attilio


--
Peace can only be achieved by understanding - A. Einstein
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."