Looks good.
Acked-by: Andy Zhou <az...@nicira.com>
On Fri, Aug 16, 2013 at 1:09 PM, Ben Pfaff <b...@nicira.com> wrote:
> htb_parse_qdisc_details__(), which is called with the netdev mutex, called
> netdev_get_mtu(), which tried to reacquire the mutex and thus deadlocked.
> This commit fixes the problem and similar problems in
> htb_parse_class_details__() and hfsc_parse_qdisc_details__().
>
> Bug #19180.
> Reported-by: Dhaval Badiani <dbadi...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/netdev-linux.c | 41 ++++++++++++++++++++++++++++-------------
> 1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 9a80b67..c3684da 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1041,21 +1041,17 @@ netdev_linux_get_etheraddr(const struct netdev
> *netdev_,
> return error;
> }
>
> -/* Returns the maximum size of transmitted (and received) packets on
> 'netdev',
> - * in bytes, not including the hardware header; thus, this is typically
> 1500
> - * bytes for Ethernet devices. */
> static int
> -netdev_linux_get_mtu(const struct netdev *netdev_, int *mtup)
> +netdev_linux_get_mtu__(struct netdev_linux *netdev, int *mtup)
> + OVS_REQUIRES(netdev->mutex)
> {
> - struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> int error;
>
> - ovs_mutex_lock(&netdev->mutex);
> if (!(netdev->cache_valid & VALID_MTU)) {
> struct ifreq ifr;
>
> netdev->netdev_mtu_error = af_inet_ifreq_ioctl(
> - netdev_get_name(netdev_), &ifr, SIOCGIFMTU, "SIOCGIFMTU");
> + netdev_get_name(&netdev->up), &ifr, SIOCGIFMTU, "SIOCGIFMTU");
> netdev->mtu = ifr.ifr_mtu;
> netdev->cache_valid |= VALID_MTU;
> }
> @@ -1064,6 +1060,21 @@ netdev_linux_get_mtu(const struct netdev *netdev_,
> int *mtup)
> if (!error) {
> *mtup = netdev->mtu;
> }
> +
> + return error;
> +}
> +
> +/* Returns the maximum size of transmitted (and received) packets on
> 'netdev',
> + * in bytes, not including the hardware header; thus, this is typically
> 1500
> + * bytes for Ethernet devices. */
> +static int
> +netdev_linux_get_mtu(const struct netdev *netdev_, int *mtup)
> +{
> + struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> + int error;
> +
> + ovs_mutex_lock(&netdev->mutex);
> + error = netdev_linux_get_mtu__(netdev, mtup);
> ovs_mutex_unlock(&netdev->mutex);
>
> return error;
> @@ -2707,7 +2718,7 @@ htb_setup_class__(struct netdev *netdev, unsigned
> int handle,
> int error;
> int mtu;
>
> - error = netdev_get_mtu(netdev, &mtu);
> + error = netdev_linux_get_mtu__(netdev_linux_cast(netdev), &mtu);
> if (error) {
> VLOG_WARN_RL(&rl, "cannot set up HTB on device %s that lacks MTU",
> netdev_get_name(netdev));
> @@ -2803,9 +2814,10 @@ htb_parse_tcmsg__(struct ofpbuf *tcmsg, unsigned
> int *queue_id,
> }
>
> static void
> -htb_parse_qdisc_details__(struct netdev *netdev,
> +htb_parse_qdisc_details__(struct netdev *netdev_,
> const struct smap *details, struct htb_class
> *hc)
> {
> + struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> const char *max_rate_s;
>
> max_rate_s = smap_get(details, "max-rate");
> @@ -2813,7 +2825,8 @@ htb_parse_qdisc_details__(struct netdev *netdev,
> if (!hc->max_rate) {
> enum netdev_features current;
>
> - netdev_get_features(netdev, ¤t, NULL, NULL, NULL);
> + netdev_linux_read_features(netdev);
> + current = !netdev->get_features_error ? netdev->current : 0;
> hc->max_rate = netdev_features_to_bps(current, 100 * 1000 * 1000)
> / 8;
> }
> hc->min_rate = hc->max_rate;
> @@ -2832,7 +2845,7 @@ htb_parse_class_details__(struct netdev *netdev,
> const char *priority_s = smap_get(details, "priority");
> int mtu, error;
>
> - error = netdev_get_mtu(netdev, &mtu);
> + error = netdev_linux_get_mtu__(netdev_linux_cast(netdev), &mtu);
> if (error) {
> VLOG_WARN_RL(&rl, "cannot parse HTB class on device %s that lacks
> MTU",
> netdev_get_name(netdev));
> @@ -3280,9 +3293,10 @@ hfsc_query_class__(const struct netdev *netdev,
> unsigned int handle,
> }
>
> static void
> -hfsc_parse_qdisc_details__(struct netdev *netdev, const struct smap
> *details,
> +hfsc_parse_qdisc_details__(struct netdev *netdev_, const struct smap
> *details,
> struct hfsc_class *class)
> {
> + struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> uint32_t max_rate;
> const char *max_rate_s;
>
> @@ -3292,7 +3306,8 @@ hfsc_parse_qdisc_details__(struct netdev *netdev,
> const struct smap *details,
> if (!max_rate) {
> enum netdev_features current;
>
> - netdev_get_features(netdev, ¤t, NULL, NULL, NULL);
> + netdev_linux_read_features(netdev);
> + current = !netdev->get_features_error ? netdev->current : 0;
> max_rate = netdev_features_to_bps(current, 100 * 1000 * 1000) / 8;
> }
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
